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

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

 



On Thu, Feb 6, 2014 at 9:53 PM, Sean Paul <seanpaul@xxxxxxxxxxxx> wrote:
> On Wed, Jan 29, 2014 at 8:44 AM, Rob Clark <robdclark@xxxxxxxxx> wrote:
>> On Tue, Jan 28, 2014 at 4:52 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).
>>>> ---
>>>>  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>
>>>
>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
>>>> index 2e31fb8..d585a4c 100644
>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
>>>> @@ -10,6 +10,7 @@
>>>>   */
>>>>
>>>>  #include <drm/drmP.h>
>>>> +#include <drm/drm_atomic_helper.h>
>>>>
>>>>  #include <drm/exynos_drm.h>
>>>>  #include "exynos_drm_drv.h"
>>>> @@ -149,7 +150,7 @@ void exynos_plane_commit(struct drm_plane *plane)
>>>>         struct exynos_plane *exynos_plane = to_exynos_plane(plane);
>>>>         struct exynos_drm_overlay *overlay = &exynos_plane->overlay;
>>>>
>>>> -       exynos_drm_fn_encoder(plane->crtc, &overlay->zpos,
>>>> +       exynos_drm_fn_encoder(plane->state->crtc, &overlay->zpos,
>>>>                         exynos_drm_encoder_plane_commit);
>>>>  }
>>>>
>>>> @@ -162,7 +163,7 @@ void exynos_plane_dpms(struct drm_plane *plane, int mode)
>>>>                 if (exynos_plane->enabled)
>>>>                         return;
>>>>
>>>> -               exynos_drm_fn_encoder(plane->crtc, &overlay->zpos,
>>>> +               exynos_drm_fn_encoder(plane->state->crtc, &overlay->zpos,
>>>>                                 exynos_drm_encoder_plane_enable);
>>>>
>>>>                 exynos_plane->enabled = true;
>>>> @@ -170,7 +171,7 @@ void exynos_plane_dpms(struct drm_plane *plane, int mode)
>>>>                 if (!exynos_plane->enabled)
>>>>                         return;
>>>>
>>>> -               exynos_drm_fn_encoder(plane->crtc, &overlay->zpos,
>>>> +               exynos_drm_fn_encoder(plane->state->crtc, &overlay->zpos,
>>>>                                 exynos_drm_encoder_plane_disable);
>>>>
>>>>                 exynos_plane->enabled = false;
>>>> @@ -192,7 +193,7 @@ exynos_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>>>>         if (ret < 0)
>>>>                 return ret;
>>>>
>>>> -       plane->crtc = crtc;
>>>> +       plane->state->crtc = crtc;
>>>>
>>>>         exynos_plane_commit(plane);
>>>>         exynos_plane_dpms(plane, DRM_MODE_DPMS_ON);
>>>> @@ -225,13 +226,17 @@ static int exynos_plane_set_property(struct drm_plane *plane,
>>>>         struct drm_device *dev = plane->dev;
>>>>         struct exynos_plane *exynos_plane = to_exynos_plane(plane);
>>>>         struct exynos_drm_private *dev_priv = dev->dev_private;
>>>> +       struct drm_plane_state *pstate = drm_atomic_get_plane_state(plane, state);
>>>
>>> Hi Rob,
>>> This seems to cause a deadlock. drm_mode_obj_set_property_ioctl
>>> performs a drm_modeset_lock_all first, then
>>> drm_atomic_helper_get_plane_state tries to lock the crtc[s] again.
>>
>> oh, whoops, I guess neither modetest or glplane test does a
>> set-property ioctl.  I think we simply need to drop the locking at the
>> top level ioctl fxn, and let the atomic locking take care of things.
>>
>
> Seems like there's another one in the drm_fb_helper_restore_fbdev_mode path.
>
> (modeset_lock_state+0x58/0x174) from [<c028e300>] (drm_modeset_lock+0x20/0x30)
> (drm_modeset_lock+0x20/0x30) from [<c028e34c>]
> (drm_modeset_lock_all_crtcs+0x3c/0x54)
> (drm_modeset_lock_all_crtcs+0x3c/0x54) from [<c029b54c>]
> (drm_atomic_helper_get_plane_state+0x40/0xc4)
> (drm_atomic_helper_get_plane_state+0x40/0xc4) from [<c02a2008>]
> (exynos_plane_set_property+0x3c/0xbc)
> (exynos_plane_set_property+0x3c/0xbc) from [<c028dcb8>]
> (drm_mode_plane_set_obj_prop+0x3c/0x4c)
> (drm_mode_plane_set_obj_prop+0x3c/0x4c) from [<c028dd08>]
> (drm_plane_force_disable+0x40/0x60)
> (drm_fb_helper_restore_fbdev_mode+0x80/0x158) from [<c029ee78>]
> (exynos_drm_fbdev_restore_mode+0x54/0x6c)
> (exynos_drm_fbdev_restore_mode+0x54/0x6c) from [<c029bc28>]
> (exynos_drm_lastclose+0x34/0x44)
> (exynos_drm_lastclose+0x34/0x44) from [<c0284bb0>] (drm_lastclose+0x44/0x158)
> (drm_lastclose+0x44/0x158) from [<c02857c0>] (drm_release+0x4c4/0x51c)
> (drm_release+0x4c4/0x51c) from [<c0101410>] (__fput+0x108/0x208)
>

In theory the drm_modeset_{lock,unlock}_all() stuff should be removed
from exynos_drm_fbdev_restore_mode().. perhaps I missed updating
exynos?

BR,
-R

>
> Sean
>
>
>> BR,
>> -R
>>
>>> Trace:
>>> (__schedule+0x4d8/0x71c) from [<c0512590>] (schedule+0x90/0x94)
>>> (schedule+0x90/0x94) from [<c0512b1c>] (schedule_preempt_disabled+0x18/0x1c)
>>> (schedule_preempt_disabled+0x18/0x1c) from [<c0510c78>]
>>> (__ww_mutex_lock_slowpath+0x208/0x2cc)
>>> (__ww_mutex_lock_slowpath+0x208/0x2cc) from [<c0510d8c>]
>>> (__ww_mutex_lock+0x50/0xc4)
>>> (__ww_mutex_lock+0x50/0xc4) from [<c028dfc0>] (modeset_lock_state+0x58/0x174)
>>> (modeset_lock_state+0x58/0x174) from [<c028e0fc>] (drm_modeset_lock+0x20/0x30)
>>> (drm_modeset_lock+0x20/0x30) from [<c028e148>]
>>> (drm_modeset_lock_all_crtcs+0x3c/0x54)
>>> (drm_modeset_lock_all_crtcs+0x3c/0x54) from [<c029c234>]
>>> (drm_atomic_helper_get_plane_state+0x40/0xc4)
>>> (drm_atomic_helper_get_plane_state+0x40/0xc4) from [<c02a2d58>]
>>> (exynos_plane_set_property+0x3c/0xbc)
>>> (exynos_plane_set_property+0x3c/0xbc) from [<c028dab4>]
>>> (drm_mode_plane_set_obj_prop+0x3c/0x4c)
>>> (drm_mode_plane_set_obj_prop+0x3c/0x4c) from [<c0290734>]
>>> (drm_mode_set_obj_prop+0xac/0x128)
>>> (drm_mode_set_obj_prop+0xac/0x128) from [<c0294814>]
>>> (drm_mode_obj_set_property_ioctl+0xe4/0x15c)
>>> (drm_mode_obj_set_property_ioctl+0xe4/0x15c) from [<c028468c>]
>>> (drm_ioctl+0x2e0/0x40c)
>>> (drm_ioctl+0x2e0/0x40c) from [<c0110c48>] (do_vfs_ioctl+0x508/0x5dc)
>>>
>>> Sean
>>>
>>>
>>>> +
>>>> +       if (IS_ERR(pstate))
>>>> +               return PTR_ERR(pstate);
>>>>
>>>>         if (property == dev_priv->plane_zpos_property) {
>>>>                 exynos_plane->overlay.zpos = val;
>>>>                 return 0;
>>>>         }
>>>>
>>>> -       return -EINVAL;
>>>> +       return drm_plane_set_property(plane, pstate, property, val, blob_data);
>>>>  }
>>>>
>>>>  static struct drm_plane_funcs exynos_plane_funcs = {
>>>
>>> <snip>
>>>
>>>> --
>>>> 1.8.4.2
>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@xxxxxxxxxxxxxxxxxxxxx
>>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
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