On Tue, Mar 18, 2014 at 12:24 PM, Rob Clark <robdclark@xxxxxxxxx> wrote:
> 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.
>
Right, OK, I was slightly mistaken. It seems like what was happening
is that we were setting lock->atomic_pending the first time we grabbed
each lock and then blocking on atomic_pending in the second go-around.
> 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.
>
Indeed, this seems to be fixed in your cold-fusion branch since it
only acquires the locks once, when the state is allocated.
Sean
> 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
