On Tue, Aug 18, 2015 at 5:21 PM, Daniel Vetter <daniel@xxxxxxxx> wrote:
> On Tue, Aug 18, 2015 at 11:54 AM, Rob Clark <robdclark@xxxxxxxxx> 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@xxxxxxxx>
>> 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@xxxxxxxxxxx>
>> Tested-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxxxxxx>
>> Signed-off-by: Rob Clark <robdclark@xxxxxxxxx>
>
> 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..
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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
