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>
> +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;
> +}
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
