On Mon, Nov 25, 2013 at 9:47 AM, Rob Clark robdclark@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);elseret = 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;+}