Re: [PATCH v3 1/6] vga_switcheroo: Add support for switching only the DDC

From: Daniel Vetter <daniel@ffwll.ch>
To: Lukas Wunner <lukas@wunner.de>
Cc: William Brown <william@blackhats.net.au>,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v3 1/6] vga_switcheroo: Add support for switching only the DDC
Date: Tue, 6 Oct 2015 13:10:00 +0200	[thread overview]
Message-ID: <20151006111000.GG3383@phenom.ffwll.local> (raw)
In-Reply-To: <20151006101048.GA15385@wunner.de>

On Tue, Oct 06, 2015 at 12:10:48PM +0200, Lukas Wunner wrote:
> Hi Daniel,
> 
> thank you for taking a look at the patch set and shepherding this
> through the review process.
> 
> On Tue, Oct 06, 2015 at 09:27:24AM +0200, Daniel Vetter wrote:
> > On Fri, Aug 14, 2015 at 06:50:15PM +0200, Lukas Wunner wrote:
> > >     Don't lock vgasr_mutex in _lock_ddc() / _unlock_ddc(), it can cause
> > >     deadlocks because (a) during switch (with vgasr_mutex already held),
> > >     GPU is woken and probes its outputs, tries to re-acquire vgasr_mutex
> > >     to lock DDC lines; (b) Likewise during switch, GPU is suspended and
> > >     calls cancel_delayed_work_sync() to stop output polling, if poll
> > >     task is running at this moment we may wait forever for it to finish.
> > > 
> > >     Instead, lock ddc_lock when unregistering the handler because the
> > >     only reason why we'd want to lock vgasr_mutex in _lock_ddc() /
> > >     _unlock_ddc() is to block the handler from disappearing while DDC
> > >     lines are switched.
> > 
> > Shouldn't we also take the new look in register_handler, for consistency?
> > I know that if you look the mux provider too late it's fairly hopeless ...
> 
> With the chosen approach that's not necessary: vga_switcheroo_lock_ddc()
> records in vgasr_priv.old_ddc_owner if there's no mux:
> 
> 	if (!vgasr_priv.handler || !vgasr_priv.handler->switch_ddc) {
> 		vgasr_priv.old_ddc_owner = -ENODEV;
> 		return -ENODEV;
> 	}
> 
> And vga_switcheroo_unlock_ddc() does nothing if vgasr_priv.old_ddc_owner
> is negative, it just releases the lock and returns:
> 
> 	if (vgasr_priv.old_ddc_owner >= 0) {
> 		id = vgasr_priv.handler->get_client_id(pdev);
> 		if (vgasr_priv.old_ddc_owner != id)
> 			ret = vgasr_priv.handler->switch_ddc(
> 						     vgasr_priv.old_ddc_owner);
> 	}
> 	mutex_unlock(&vgasr_priv.ddc_lock);
> 
> So it has no consequences if the mux registers between the calls to
> _lock_ddc() and _unlock_ddc().
> 
> 
> > Also while reading the patch I realized that the new lock really protects
> > hw state (instead of our software-side tracking and driver resume/suspend
> > code). And at least myself I have no idea what vgasr means, so what about
> > renaming it to hw_mutex? We have this pattern of low-level locks to avoid
> > concurrent hw access in a lot of places like i2c, dp_aux, and it's very
> > often called hw_lock or similar.
> 
> Hm, hw_lock sounds a bit ambiguous.
> 
> vgasr_mutex is really a catch-all that protects access to the vgasr_priv
> structure and also protects various hardware state. (Power state of each
> GPU, mux state.) Up until now this approach worked fine, it looks like
> there was no need for additional locks. We may need to move to more
> fine-grained locking as new features get added to vga_switcheroo.
> The newly introduced ddc_lock is a first step and I think is aptly
> named since it only protects the mux state for the DDC lines.

Oops sorry mixed up the names. I meant rename the ddc_lock to hw_lock
since it protects a bit more than just the ddc (it protects the entire hw
switch state since we grab it both for dcc switching and for full-blown
switching of everything).
> 
> 
> > Wrt the overall patch series, can you pls haggle driver-maintainers (gmux,
> > radeon, nouveau) for acks/reviews? scripts/get_maintainers.pl should help.
> 
> Will do.
> 
> 
> > Also, the revised docbook patch seems to be missing from this iteration,
> > please follow up with that one.
> 
> The docbook patch posted September 17 automatically picks up the
> kernel-doc for the new functions through the !E directive.

I asked for the inclusion to be moved to drm.tmpl instead of creating a
new docbook.

> However I used markdown syntax for the unsorted lists in the DOC
> sections, so it will look a bit funny unless markdown gets merged,
> which as we all know is contentious. (Which is sad, though I can
> understand Jonathan Corbet's concerns.)
> 
> 
> By the way, Jani has kindly provided a Reviewed-By: for patch 6 of
> the vga_switcheroo docs series. The patch is not dependent on the
> preceding patch 5 which is awaiting an ack from Takashi, so could
> be merged now:
> [PATCH 06/15] vga_switcheroo: Use enum vga_switcheroo_state instead
> 
> Patches 7 and 8 are similarly trivial cleanups:
> [PATCH 07/15] vga_switcheroo: Use VGA_SWITCHEROO_UNKNOWN_ID instead
> [PATCH 08/15] vga_switcheroo: Use enum vga_switcheroo_client_id
> 
> And patch 10 has gotten a Reviewed-By: from Takashi:
> [PATCH 10/15] ALSA: hda - Spell vga_switcheroo consistently

Hm those patches aren't in this series, that makes it really hard for me
to know what's still pending. I think it would be best to resend
_everything_, so including docs and cleanups and all that. Otherwise I
think this will take a lot longer than necessary to pull in.

Also when resending patches unchanged please directly add r-b tags you've
collected already - replying top-level with them all again makes it a bit
harder to sort everything our here for me. And one small nit: The usual
style for patch bombing is flat threading, not nested. It should be the
default for new-ish git, but if that's not the case please use git
send-email --no-chain-reply-to. Note also if you generate patches first
with git format-patch that has a separate knob for deep threading, you
need to disable both iirc to avoid deep threading, there it's git
format-patch --thread=shallow.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel