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

Sean Paul seanpaul at chromium.org
Tue Mar 18 10:33:41 PDT 2014


On Tue, Mar 18, 2014 at 12:24 PM, Rob Clark <robdclark at gmail.com> wrote:
> On Tue, Mar 18, 2014 at 11:48 AM, Sean Paul <seanpaul at chromium.org> wrote:
>> On Mon, Nov 25, 2013 at 9:47 AM, Rob Clark <robdclark at gmail.com> 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).
>>> ---
>>>  drivers/gpu/drm/drm_atomic_helper.c         | 156 ++++++++++-
>>>  drivers/gpu/drm/drm_crtc.c                  | 397 +++++++++++++++++++---------
>>>  drivers/gpu/drm/drm_fb_helper.c             |  17 +-
>>>  drivers/gpu/drm/exynos/exynos_drm_crtc.c    |   4 +-
>>>  drivers/gpu/drm/exynos/exynos_drm_encoder.c |   6 +-
>>>  drivers/gpu/drm/exynos/exynos_drm_plane.c   |  15 +-
>>>  drivers/gpu/drm/i915/intel_sprite.c         |  21 +-
>>>  drivers/gpu/drm/msm/mdp4/mdp4_crtc.c        |   2 +-
>>>  drivers/gpu/drm/msm/mdp4/mdp4_plane.c       |  19 +-
>>>  drivers/gpu/drm/nouveau/dispnv04/overlay.c  |   8 +-
>>>  drivers/gpu/drm/omapdrm/omap_crtc.c         |   4 +-
>>>  drivers/gpu/drm/omapdrm/omap_drv.c          |   2 +-
>>>  drivers/gpu/drm/omapdrm/omap_plane.c        |  33 ++-
>>>  drivers/gpu/drm/rcar-du/rcar_du_plane.c     |   8 +-
>>>  drivers/gpu/drm/shmobile/shmob_drm_crtc.c   |   2 +-
>>>  drivers/gpu/drm/shmobile/shmob_drm_plane.c  |   6 +-
>>>  drivers/gpu/drm/tegra/dc.c                  |  16 +-
>>>  include/drm/drm_atomic_helper.h             |  39 ++-
>>>  include/drm/drm_crtc.h                      |  88 +++++-
>>>  19 files changed, 654 insertions(+), 189 deletions(-)
>>>
>>
>> <snip>
>>
>>> +static struct drm_plane_state *
>>> +drm_atomic_helper_get_plane_state(struct drm_plane *plane, void *state)
>>> +{
>>> +       struct drm_atomic_helper_state *a = state;
>>> +       struct drm_plane_state *pstate;
>>> +       int ret;
>>> +
>>> +       /* grab lock of current crtc.. if crtc is NULL then grab all: */
>>> +       if (plane->state->crtc)
>>> +               ret = drm_modeset_lock(&plane->state->crtc->mutex, state);
>>> +       else
>>> +               ret = drm_modeset_lock_all_crtcs(plane->dev, state);
>>
>>
>> Hi Rob,
>> There seems to be a recursive lock in the
>> drm_fb_helper_restore_fbdev_mode path. The code loops through all
>> planes and does a force disable. If 2 of the planes do not have a crtc
>> assigned, we'll try to lock all crtcs twice. Here's a stack trace:
>
> so what is *supposed* to happen is that in both cases, the crtc's are
> locked against the same ww_ctx.  So the second time will be a no-op.
>

Right, OK, I was slightly mistaken. It seems like what was happening
is that we were setting lock->atomic_pending the first time we grabbed
each lock and then blocking on atomic_pending in the second go-around.


> It gets a bit tricky with the whole lock-handoff-to-worker feature
> which drivers can choose to use, since when the locks are re-acquired
> it is (out of necessity) with a different ww_ctx.
>
> I did fix something around those lines with the recent updated atomic
> series (rebased on primary-planes, fwiw).  And also added a WARN_ON()
> to catch places which don't acquire their needed locks before the
> commit phase.
>

Indeed, this seems to be fixed in your cold-fusion branch since it
only acquires the locks once, when the state is allocated.

Sean



> I'm pretty sure this is the same thing you are hitting here
>
>> [   14.040141] Call Trace:
>> [   14.040144]  [<ffffffff9669023a>] drm_modeset_lock+0x46/0x1b7
>> [   14.040147]  [<ffffffff96689eb6>] ? drm_err+0x6e/0x85
>> [   14.040149]  [<ffffffff96690425>] drm_modeset_lock_all_crtcs+0x7a/0xdf
>> [   14.040152]  [<ffffffff9669b6b2>] drm_atomic_helper_get_plane_state+0x34/0x99
>> [   14.040154]  [<ffffffff9669af9d>]
>> drm_atomic_helper_plane_set_property+0x30/0x56
>> [   14.040157]  [<ffffffff9668ee04>] drm_mode_plane_set_obj_prop+0x18/0x20
>> [   14.040159]  [<ffffffff9668ee34>] drm_plane_force_disable+0x28/0x46
>> [   14.040161]  [<ffffffff9667e1f7>] drm_fb_helper_restore_fbdev_mode+0x84/0x140
>> [   14.040163]  [<ffffffff966e0bea>] intel_fb_restore_mode+0x26/0x8c
>> [   14.040166]  [<ffffffff966a1f41>] i915_driver_lastclose+0x2c/0x3e
>> [   14.040169]  [<ffffffff96685399>] drm_lastclose+0x49/0x238
>> [   14.040171]  [<ffffffff96685d24>] drm_release+0x513/0x546
>> [   14.040174]  [<ffffffff964e8842>] __fput+0xfb/0x1d3
>> [   14.040178]  [<ffffffff964e8928>] ____fput+0xe/0x10
>> [   14.040180]  [<ffffffff9644bf49>] task_work_run+0x7d/0x94
>> [   14.040182]  [<ffffffff96401c8f>] do_notify_resume+0x57/0x5b
>> [   14.040184]  [<ffffffff968b5308>] int_signal+0x12/0x17
>>
>>
>>> +       if (ret)
>>> +               return ERR_PTR(ret);
>>> +
>>> +       pstate = a->pstates[plane->id];
>>> +
>>> +       if (!pstate) {
>>> +               pstate = kzalloc(sizeof(*pstate), GFP_KERNEL);
>>> +               if (!pstate)
>>> +                       return ERR_PTR(-ENOMEM);
>>> +               drm_atomic_helper_init_plane_state(plane, pstate, state);
>>> +               a->planes[plane->id] = plane;
>>> +               a->pstates[plane->id] = pstate;
>>> +       }
>>> +
>>> +       return pstate;
>>> +}
>>> +
>>
>> <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);
>>
>> I haven't tested this, but it seems it might be susceptible to the
>> same issue if the plane does not have an assigned crtc.
>
> well, as long as the locks that will be needed in commit() are grabbed
> prior to commit(), it should be all good.  But there was a case where
> I needed to grab crtc locks earlier.
>
> Eventually we should just have plane locks, which would reduce the
> cases where we have to grab crtc locks.  But I was really trying to
> not *completely* re-write kms in one go ;-)
>
> BR,
> -R
>
>>
>> Sean
>>
>>> +                       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;
>>> +}


More information about the dri-devel mailing list