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

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: Daniel Vetter <daniel@ffwll.ch>,
	Thierry Reding <thierry.reding@gmail.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH] drm: Implement drm_modeset_{,un}lock_all_ctx()
Date: Thu, 10 Sep 2015 15:35:06 +0200	[thread overview]
Message-ID: <55F1870A.3060109@linux.intel.com> (raw)
In-Reply-To: <20150910095154.GW2767@phenom.ffwll.local>

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@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@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel