[PATCH] drm/atomic: skip ww_acquire_done() for legacy paths

[Rob Clark]

On Tue, Aug 18, 2015 at 5:29 PM, Rob Clark <robdclark at gmail.com> wrote:
> On Tue, Aug 18, 2015 at 5:21 PM, Daniel Vetter <daniel at ffwll.ch> wrote:
>> On Tue, Aug 18, 2015 at 11:54 AM, Rob Clark <robdclark at gmail.com> wrote:
>>> The addition of ww_acquire_done() call to mark the point where all locks
>>> should already be held, added in:
>>>
>>>   commit 992cbf19b32900efa17850b9fa0031fd623edd4d
>>>   Author:     Daniel Vetter <daniel.vetter at ffwll.ch>
>>>   AuthorDate: Thu Aug 6 15:06:40 2015 +0200
>>>
>>>       drm/atomic: Call ww_acquire_done after check phase is complete
>>>
>>> missed the case that legacy paths (which are already called between
>>> drm_modeset_lock_all()/unlock_all()) re-use the single acquire ctx, even
>>> if there are multiple atomic updates.  For example, restore_fbdev_mode()
>>> clearing rotation properties individually on each plane.
>>>
>>> Inki already sent a patch which completely removed the ww_acquire_done()
>>> call, but I think this is less ideal (since it is a nice debugging
>>> feature of ww_mutex to confirm that everyone is following the rules).
>>> So instead, just skip the call only in the special legacy-path case.
>>>
>>> Reported-by: Inki Dae <inki.dae at samsung.com>
>>> Tested-by: Bjorn Andersson <bjorn.andersson at sonymobile.com>
>>> Signed-off-by: Rob Clark <robdclark at gmail.com>
>>
>> Should we instead just do it in the atomic ioclt path? Your patch here
>> is missing the legacy crtc-update case (where the acquire_ctx is
>> stored in the drm_crtc). Also this won't work once we have a
>> drm_modeset_lock_all_ctx.
>
> Well, doing it here (vs in ioctl fxn, outside of drm_atomic_commit())
> is that it will catch cases where drivers try to acquire locks in
> ->commit()..
>
> Maybe we just need to pass a legacy_dont_acquire_done flag down?  Or
> just take Inki's patch for now until we sort out a better plan..

actually, I don't think we'll hit it w/ legacy crtc-update paths..

but one possible approach is just stuff a no_acquire_fini flag in the
state object..

BR,
-R

> BR,
> -R
>
>> -Daniel
>>
>>> ---
>>>  drivers/gpu/drm/drm_atomic.c | 11 +++++++++--
>>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>>> index 1066e4b..b90068c 100644
>>> --- a/drivers/gpu/drm/drm_atomic.c
>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>> @@ -1230,8 +1230,15 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
>>>                 }
>>>         }
>>>
>>> -       if (ret == 0)
>>> -               ww_acquire_done(&state->acquire_ctx->ww_ctx);
>>> +       if (ret == 0) {
>>> +               /* in legacy paths, there can be multiple atomic updates,
>>> +                * such as drm_mode_plane_set_obj_prop(), which happen
>>> +                * under a single drm_modeset_lock_all().  In the legacy
>>> +                * paths, skip marking acquire-done lest we anger ww-mutex:
>>> +                */
>>> +               if (state->acquire_ctx != dev->mode_config.acquire_ctx)
>>> +                       ww_acquire_done(&state->acquire_ctx->ww_ctx);
>>> +       }
>>>
>>>         return ret;
>>>  }
>>> --
>>> 2.4.3
>>>
>>
>>
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch