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

[Daniel Vetter <daniel@ffwll.ch>]

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.

> > My original design behind the crtc->mutex and mode_config.mutex split
> > was that as long as the connector->crtc links didn't change you can
> > get away with the crtc lock. setplane made a bit a mess out of this,
> > but strictly speaking as long as you acquire all crtc locks involved
> > in a potential plane switch (which ww_mtuxes can do) it'll be fine.
> > Since noticing whether any connector properties change should be
> > doable upfront I think we should try _really_ hard to keep the
> > mode_config.mutex a plain mutex which wraps all the more fine-grained
> > locks and is a catch-all for everything else but crtcs/planes.
> 
> That is basically how it was in one of the earlier iterations of
> atomic.. but didn't hold mode_config.mutex in a lot of places where it
> was previously held.. and while I do want to make locking more fine
> grained I didn't want to try and do it at the same time as landing all
> these other changes.

Hm, maybe we should land the locking stuff first? I.e. just convert
crtc->mutex into a ww_mutex, and then add more fine-grained locking to
e.g. setplane by only grabbing the locks of the involved crtcs with the
w/w logic. We might need an additional plane mutex to make it work. Iirc
Ville had some patches for just this.

I'm arguing this since locking at the current interface I have a really
hard time seeing how we're going to implement this in i915. Still reading
around though.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch