On Tue, Mar 18, 2014 at 11:48 AM, 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>
>
>> +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.
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.
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;
>> +}
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
