Re: [PATCH 03/17] drm: convert crtc and mode_config to ww_mutex

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, May 26, 2014 at 04:35:04PM +0200, Daniel Vetter wrote:
> On Mon, May 26, 2014 at 07:56:50AM -0400, Rob Clark wrote:
> > On Mon, May 26, 2014 at 4:23 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > On Sun, May 25, 2014 at 07:16:43PM -0400, Rob Clark wrote:
> > >> On Sun, May 25, 2014 at 6:10 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > >> > On Sat, May 24, 2014 at 8:30 PM, Rob Clark <robdclark@gmail.com> wrote:
> > >> >> @@ -8002,7 +8002,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
> > >> >>         if (encoder->crtc) {
> > >> >>                 crtc = encoder->crtc;
> > >> >>
> > >> >> -               mutex_lock(&crtc->mutex);
> > >> >> +               drm_modeset_lock(&crtc->mutex, NULL);
> > >> >
> > >> >
> > >> > This is pretty much the reason why I think switching the
> > >> > mode_config.mutex to a ww_mutex is a bad idea: This call here nests
> > >> > within the mode_config.mutex and so must be acquired. Wiring the
> > >> > acquire context through everything is going to be fairly horrible,
> > >> > especially since you must be able to bail out when trying to lock with
> > >> > an axquire context.
> > >>
> > >> which is the call-path to here from mode_config.mutex?  Is it possible
> > >> to just move the locking to a higher level for a
> > >> drm_modeset_lock_all()?
> > >
> > > Connector probing. And the entire point of crtc locks was to _not_ block
> > > all screen updates while we poke for a new edid or do load balancing. If
> > > you want to test this you need a gen3/4 with tv-out (native, not through
> > > sdvo) or a gen2 or i915g/gm with vga.
> > 
> > 
> > hmm, I guess I'm still not quite seeing the issue.  For non-atomic
> > paths, we are grabbing mode_config and/or crtc mutex as bare mutexes
> > in same spots as we did before.  So if it worked before without
> > nested_lock stuff it should still work now.
> 
> Thread A is doing output probing.
> 
> 				Thread B is doing atomic modeset
> 
> Grabs mode_config.mutex
> 
> 				Grabs crtc_A->ww_mutex
> 
> Tries to grab crtc_A->ww_mutex,
> blocks since normal ww_mutex_lock
.. blocks since normal ww_mutex_lock without acquire ticket.

Otherwise it's a bit unclear.
-Daniel


> 
> 				Tries to grab mode_config.mutex with ww
> 				acuiquire context, blocks since current
> 				holder hasn't acquired the mutex with a ww
> 				ticket
> 
> -> Deadlock.
> 
> You really can't mix lock nesting with w/w and lock nesting with a static
> hierarchy. It's all or nothing.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch