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 7:18 PM, Rob Clark <robdclark@xxxxxxxxx> wrote:
> 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.
>

Hey Rob,
Sorry for the delay getting back to you. Indeed, I was confused in my
last email, thanks for straightening me out.

I traced through things a little more carefully and it looks like my
fb is being unreferenced every time we call set_property on the plane.
On exynos, we set a private zpos property via the set_property ioctl.
In this case, drm_mode_set_property_ioctl does not take a reference on
the plane's current fb, but we put a reference in atomic_commit
(assuming it's got a valid crtc/fb). Eventually, this runs the
refcount down to 0 and we end up freeing the fb early.

I think the fix here is to only unreference the plane's current fb in
the case where new_fb is true. ie:

diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index 14e0571..ec60d4e 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -394,7 +394,8 @@ drm_atomic_helper_commit_plane_state(struct
drm_plane *plane,
                         * the slightly different ordering in the two
cases in the
                         * original code.
                         */
-                       old_fb = plane->state->fb;
+                       if (pstate->new_fb)
+                               old_fb = plane->state->fb;
                        swap_plane_state(plane, pstate->state);
                        fb = NULL;
                }

Seem reasonable?

Sean



> 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