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

From: Lukas Wunner <lukas@wunner.de>
To: Daniel Vetter <daniel@ffwll.ch>
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 20:27:44 +0200	[thread overview]
Message-ID: <20151006182744.GA15532@wunner.de> (raw)
In-Reply-To: <20151006111000.GG3383@phenom.ffwll.local>

Hi Daniel,

On Tue, Oct 06, 2015 at 01:10:00PM +0200, Daniel Vetter wrote:
> On Tue, Oct 06, 2015 at 12:10:48PM +0200, Lukas Wunner wrote:
> > On Tue, Oct 06, 2015 at 09:27:24AM +0200, Daniel Vetter wrote:
> > > 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).

Interesting observation. Actually the intention is to use ddc_lock to
only lock hardware state of the DDC switch, but you're right, it also
locks hardware state of the panel switch. That's an unintended
consequence of the ->switchto callback in apple-gmux also switching
DDC, and the need to avoid races because of this.

Now that you mention it I'm contemplating removing DDC switching from the
->switchto callback in apple-gmux. Then I don't need to take the ddc_lock
when switching the full panel.

> > > 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.

Your argument was that including it in drm.tmpl will increase the chances
it's read. Quite honestly I think that reasoning is a bit thin. One might
as well argue that people won't find the information in the juggernaut of
180 kByte XML text that is drm.tmpl.

The (honest) question is this, vga_switcheroo is not located in the DRM
tree, so it's a separate subsystem. Then why should someone be looking
for its documentation in the DRM DocBook?

> > 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.

I updated my vga_switcheroo_docs branch on GitHub as the Reviewed-Bys
came in and just rebased it to current topic/drm-misc, so you can pull
from there if you're comfortable with that:
https://github.com/l1k/linux/commits/vga_switcheroo_docs

If on the other hand you prefer merging stuff via the mailing list,
I'll be happy to resend.

Thanks,

Lukas
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel