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

[Sean Paul]

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:

[   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.

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;
> +}