[PATCH] drm: Implement drm_modeset_{,un}lock_all_ctx()

[Maarten Lankhorst]

Op 10-09-15 om 11:51 schreef Daniel Vetter:
> On Tue, Sep 08, 2015 at 03:26:47PM +0200, Thierry Reding wrote:
>> From: Thierry Reding <treding at nvidia.com>
>>
>> These functions are like drm_modeset_{,un}lock_all(), but they take the
>> lock acquisition context as a parameter rather than storing it in the
>> DRM device's drm_mode_config structure.
>>
>> Implement drm_modeset_{,un}lock_all() in terms of the new function for
>> better code reuse, and add a note to the kerneldoc that new code should
>> use the new functions.
>>
>> Signed-off-by: Thierry Reding <treding at nvidia.com>
> For the record quick summary of what I've mentioned on irc:
> - lock_all_ctx can't lock dev->mode_config.mutex due to locking inversion
>   between that plain mutex and the ww dance.
I think that lock should die and all its callers should use connection_mutex instead. :-)
> - As a consequence we can only acquire ww mutexes. And we have a function
>   which does most of that already: lock_all_crtc, and that even takes an
>   acquire ctx! So my proposal would be to move the
>   ww_mutex_lock(mode_config->connection_mutex) into that function too and
>   rename it to lock_all_ctx. That nicely cleans up a bunch of callers too.
>
>   The behind leaving out the ww backoff dance is that any caller who has
>   an explicit acquire_ctx needs to have that backoff dance anyway. And if
>   we hide it in lock_all_ctx this might result in driver-private ww
>   mutexes getting silently dropped (since we only retry lock_all_ctx and
>   not the top-level loop), leading to really subtle bugs. Atm there's no
>   driver-private locks yet, but might very well happen.
I'm not sure there are benefits to extra driver-private locks. I've considered it for
protecting some of i915 state, but connection_mutex is always taken during
modeset, so that lock works. For other state normal mutexes were sufficient.

> - With that design for lock_all_ctx to only take ww mutexes there's no
>   need for unlock_all_ctx - we already have the drm_modeset_drop_locks
>   function for that.
Agreed. :-)

~Maarten