Re: [RFCv4 09/14] drm: convert plane to properties/state

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Feb 26, 2014 at 4:30 PM, Sean Paul <seanpaul@xxxxxxxxxxxx> wrote:
> On Mon, Nov 25, 2013 at 9:47 AM, Rob Clark <robdclark@xxxxxxxxx> wrote:
>> Break the mutable state of a plane out into a separate structure
>> and use atomic properties mechanism to set plane attributes.  This
>> makes it easier to have some helpers for plane->set_property()
>> and for checking for invalid params.  The idea is that individual
>> drivers can wrap the state struct in their own struct which adds
>> driver specific parameters, for easy build-up of state across
>> multiple set_property() calls and for easy atomic commit or roll-
>> back.
>>
>> The same should be done for CRTC, encoder, and connector, but this
>> patch only includes the first part (plane).
>
>
> Hi Rob,
> I've been tracking down a crash that came up on my exynos board when I
> applied this patch. I hope you can hold my hand a little bit and give
> some guidance.
>
> <snip>
>
>> +static int
>> +drm_atomic_helper_commit_plane_state(struct drm_plane *plane,
>> +               struct drm_plane_state *pstate)
>> +{
>> +       struct drm_framebuffer *old_fb = NULL, *fb = NULL;
>> +       int ret = 0;
>> +
>> +       fb = pstate->fb;
>> +
>> +       if (pstate->crtc && fb) {
>> +               ret = plane->funcs->update_plane(plane, pstate->crtc, pstate->fb,
>> +                       pstate->crtc_x, pstate->crtc_y, pstate->crtc_w, pstate->crtc_h,
>> +                       pstate->src_x,  pstate->src_y,  pstate->src_w,  pstate->src_h);
>> +               if (!ret) {
>> +                       /* on success, update state and fb refcnting: */
>> +                       /* NOTE: if we ensure no driver sets plane->state->fb = NULL
>> +                        * on disable, we can move this up a level and not duplicate
>> +                        * nearly the same thing for both update_plane and disable_plane
>> +                        * cases..  I leave it like this for now to be paranoid due to
>> +                        * the slightly different ordering in the two cases in the
>> +                        * original code.
>> +                        */
>> +                       old_fb = plane->state->fb;
>
> I think this is slightly different than what we had before in
> setplane. In the previous code, we had:
>
> ret = plane->funcs->update_plane(plane, crtc, fb,
>                                                   plane_req->crtc_x,
> plane_req->crtc_y,
>                                                   plane_req->crtc_w,
> plane_req->crtc_h,
>                                                   plane_req->src_x,
> plane_req->src_y,
>                                                   plane_req->src_w,
> plane_req->src_h);
> if (!ret) {
>         old_fb = plane->fb;
>         fb = NULL;
>         plane->crtc = crtc;
>         plane->fb = fb;
> }
>
> In this case, we'd unreference old_fb which is the previous plane->fb.
> AFAICT, update_plane did not set plane->fb to fb, so it would, in
> fact, be the old plane.

to be honest, I need to dig up that branch again and remember (and
rebase it while I'm at it)..

I don't think the driver should be changing state->fb (ie. the state
object should in theory, once constructed, be a sort of constant
thing).  So until swap_plane_state() plane->state->fb is *supposed* to
be the old fb.

For crtc, as there were so many places in code accessing crtc->fb (and
it was pretty intertwined in the crtc helpers), the way I left it as:
crtc->state->fb is what is requested by userspace, and crtc->fb is
what is currently used (which is updated to the new cstate->fb in the
course of modeset).

I don't remember if I left it the same way for plane (which isn't
referenced in nearly as many places, and which doesn't have a big
chunk of modeset helper to care about).

> In the new code, drm_mode_setplane calls drm_mode_plane_set_obj_prop
> on fb_id, which will update plane->state->fb. Then we come in here and
> unreference it as old_fb. I _believe_ we're taking one more reference
> than we need in this case.

well, it should be updating 'pstate', what will become the new
plane->state..  But not the current plane->state.

BR,
-R

> What I'm seeing in exynos is that when we setplane to an fb, we leave
> it with refcount == 1. If we disable that plane, we'll free the fb.
> Unfortunately, this leaves the fb in the fbs list and the next time we
> try to iterate that list Bad Things Happen.
>
> Does this make any sense?
>
> Sean
>
>> +                       swap_plane_state(plane, pstate->state);
>> +                       fb = NULL;
>> +               }
>> +       } else {
>> +               old_fb = plane->state->fb;
>> +               plane->funcs->disable_plane(plane);
>> +               swap_plane_state(plane, pstate->state);
>> +       }
>> +
>> +
>> +       if (fb)
>> +               drm_framebuffer_unreference(fb);
>> +       if (old_fb)
>> +               drm_framebuffer_unreference(old_fb);
>> +
>> +       return ret;
>> +}
>>
>
> <snip>
>
>> +int drm_plane_set_property(struct drm_plane *plane,
>> +               struct drm_plane_state *state,
>> +               struct drm_property *property,
>> +               uint64_t value, void *blob_data)
>> +{
>> +       struct drm_device *dev = plane->dev;
>> +       struct drm_mode_config *config = &dev->mode_config;
>> +
>> +       drm_object_property_set_value(&plane->base,
>> +                       &state->propvals, property, value, blob_data);
>> +
>> +       if (property == config->prop_fb_id) {
>> +               state->new_fb = true;
>> +               state->fb = drm_framebuffer_lookup(dev, value);
>> +       } else if (property == config->prop_crtc_id) {
>> +               struct drm_mode_object *obj = drm_property_get_obj(property, value);
>> +               struct drm_crtc *crtc = obj ? obj_to_crtc(obj) : NULL;
>> +               /* take the lock of the incoming crtc as well, moving
>> +                * plane between crtcs is synchronized on both incoming
>> +                * and outgoing crtc.
>> +                */
>> +               if (crtc) {
>> +                       int ret = drm_modeset_lock(&crtc->mutex, state->state);
>> +                       if (ret)
>> +                               return ret;
>> +               }
>> +               state->crtc = crtc;
>> +       } else if (property == config->prop_crtc_x) {
>> +               state->crtc_x = U642I64(value);
>> +       } else if (property == config->prop_crtc_y) {
>> +               state->crtc_y = U642I64(value);
>> +       } else if (property == config->prop_crtc_w) {
>> +               state->crtc_w = value;
>> +       } else if (property == config->prop_crtc_h) {
>> +               state->crtc_h = value;
>> +       } else if (property == config->prop_src_x) {
>> +               state->src_x = value;
>> +       } else if (property == config->prop_src_y) {
>> +               state->src_y = value;
>> +       } else if (property == config->prop_src_w) {
>> +               state->src_w = value;
>> +       } else if (property == config->prop_src_h) {
>> +               state->src_h = value;
>> +       } else {
>> +               return -EINVAL;
>> +       }
>> +
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL(drm_plane_set_property);
>> +
>
> <snip>
>
>> @@ -1987,20 +2192,19 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
>>                         struct drm_file *file_priv)
>>  {
>>         struct drm_mode_set_plane *plane_req = data;
>> +       struct drm_mode_config *config = &dev->mode_config;
>>         struct drm_plane *plane;
>> -       struct drm_crtc *crtc;
>> -       struct drm_framebuffer *fb = NULL, *old_fb = NULL;
>> +       void *state;
>>         int ret = 0;
>> -       unsigned int fb_width, fb_height;
>> -       int i;
>>
>>         if (!drm_core_check_feature(dev, DRIVER_MODESET))
>>                 return -EINVAL;
>>
>> -       /*
>> -        * First, find the plane, crtc, and fb objects.  If not available,
>> -        * we don't bother to call the driver.
>> -        */
>> +retry:
>> +       state = dev->driver->atomic_begin(dev, 0);
>> +       if (IS_ERR(state))
>> +               return PTR_ERR(state);
>> +
>>         plane = drm_plane_find(dev, plane_req->plane_id);
>>         if (!plane) {
>>                 DRM_DEBUG_KMS("Unknown plane ID %d\n",
>> @@ -2008,98 +2212,37 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
>>                 return -ENOENT;
>>         }
>>
>> -       /* No fb means shut it down */
>> -       if (!plane_req->fb_id) {
>> -               drm_modeset_lock_all(dev);
>> -               old_fb = plane->fb;
>> -               plane->funcs->disable_plane(plane);
>> -               plane->crtc = NULL;
>> -               plane->fb = NULL;
>> -               drm_modeset_unlock_all(dev);
>> -               goto out;
>> -       }
>> -
>> -       crtc = drm_crtc_find(dev, plane_req->crtc_id);
>> -       if (!crtc) {
>> -               DRM_DEBUG_KMS("Unknown crtc ID %d\n",
>> -                             plane_req->crtc_id);
>> -               ret = -ENOENT;
>> -               goto out;
>> -       }
>> -
>> -       fb = drm_framebuffer_lookup(dev, plane_req->fb_id);
>> -       if (!fb) {
>> -               DRM_DEBUG_KMS("Unknown framebuffer ID %d\n",
>> -                             plane_req->fb_id);
>> -               ret = -ENOENT;
>> -               goto out;
>> -       }
>> -
>> -       /* Check whether this plane supports the fb pixel format. */
>> -       for (i = 0; i < plane->format_count; i++)
>> -               if (fb->pixel_format == plane->format_types[i])
>> -                       break;
>> -       if (i == plane->format_count) {
>> -               DRM_DEBUG_KMS("Invalid pixel format %s\n",
>> -                             drm_get_format_name(fb->pixel_format));
>> -               ret = -EINVAL;
>> -               goto out;
>> -       }
>> -
>> -       fb_width = fb->width << 16;
>> -       fb_height = fb->height << 16;
>> -
>> -       /* Make sure source coordinates are inside the fb. */
>> -       if (plane_req->src_w > fb_width ||
>> -           plane_req->src_x > fb_width - plane_req->src_w ||
>> -           plane_req->src_h > fb_height ||
>> -           plane_req->src_y > fb_height - plane_req->src_h) {
>> -               DRM_DEBUG_KMS("Invalid source coordinates "
>> -                             "%u.%06ux%u.%06u+%u.%06u+%u.%06u\n",
>> -                             plane_req->src_w >> 16,
>> -                             ((plane_req->src_w & 0xffff) * 15625) >> 10,
>> -                             plane_req->src_h >> 16,
>> -                             ((plane_req->src_h & 0xffff) * 15625) >> 10,
>> -                             plane_req->src_x >> 16,
>> -                             ((plane_req->src_x & 0xffff) * 15625) >> 10,
>> -                             plane_req->src_y >> 16,
>> -                             ((plane_req->src_y & 0xffff) * 15625) >> 10);
>> -               ret = -ENOSPC;
>> -               goto out;
>> -       }
>> -
>> -       /* Give drivers some help against integer overflows */
>> -       if (plane_req->crtc_w > INT_MAX ||
>> -           plane_req->crtc_x > INT_MAX - (int32_t) plane_req->crtc_w ||
>> -           plane_req->crtc_h > INT_MAX ||
>> -           plane_req->crtc_y > INT_MAX - (int32_t) plane_req->crtc_h) {
>> -               DRM_DEBUG_KMS("Invalid CRTC coordinates %ux%u+%d+%d\n",
>> -                             plane_req->crtc_w, plane_req->crtc_h,
>> -                             plane_req->crtc_x, plane_req->crtc_y);
>> -               ret = -ERANGE;
>> +       ret =
>> +               drm_mode_plane_set_obj_prop(plane, state,
>> +                       config->prop_crtc_id, plane_req->crtc_id, NULL) ||
>> +               drm_mode_plane_set_obj_prop(plane, state,
>> +                       config->prop_fb_id, plane_req->fb_id, NULL) ||
>> +               drm_mode_plane_set_obj_prop(plane, state,
>> +                       config->prop_crtc_x, I642U64(plane_req->crtc_x), NULL) ||
>> +               drm_mode_plane_set_obj_prop(plane, state,
>> +                       config->prop_crtc_y, I642U64(plane_req->crtc_y), NULL) ||
>> +               drm_mode_plane_set_obj_prop(plane, state,
>> +                       config->prop_crtc_w, plane_req->crtc_w, NULL) ||
>> +               drm_mode_plane_set_obj_prop(plane, state,
>> +                       config->prop_crtc_h, plane_req->crtc_h, NULL) ||
>> +               drm_mode_plane_set_obj_prop(plane, state,
>> +                       config->prop_src_w, plane_req->src_w, NULL) ||
>> +               drm_mode_plane_set_obj_prop(plane, state,
>> +                       config->prop_src_h, plane_req->src_h, NULL) ||
>> +               drm_mode_plane_set_obj_prop(plane, state,
>> +                       config->prop_src_x, plane_req->src_x, NULL) ||
>> +               drm_mode_plane_set_obj_prop(plane, state,
>> +                       config->prop_src_y, plane_req->src_y, NULL) ||
>> +               dev->driver->atomic_check(dev, state);
>> +       if (ret)
>>                 goto out;
>> -       }
>>
>> -       drm_modeset_lock_all(dev);
>> -       ret = plane->funcs->update_plane(plane, crtc, fb,
>> -                                        plane_req->crtc_x, plane_req->crtc_y,
>> -                                        plane_req->crtc_w, plane_req->crtc_h,
>> -                                        plane_req->src_x, plane_req->src_y,
>> -                                        plane_req->src_w, plane_req->src_h);
>> -       if (!ret) {
>> -               old_fb = plane->fb;
>> -               plane->crtc = crtc;
>> -               plane->fb = fb;
>> -               fb = NULL;
>> -       }
>> -       drm_modeset_unlock_all(dev);
>> +       ret = dev->driver->atomic_commit(dev, state);
>>
>>  out:
>> -       if (fb)
>> -               drm_framebuffer_unreference(fb);
>> -       if (old_fb)
>> -               drm_framebuffer_unreference(old_fb);
>> -
>> +       dev->driver->atomic_end(dev, state);
>> +       if (ret == -EDEADLK)
>> +               goto retry;
>>         return ret;
>>  }
>>
>
> <snip>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux