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

Daniel Vetter daniel at ffwll.ch
Wed Oct 7 00:52:54 PDT 2015


On Tue, Oct 06, 2015 at 08:27:44PM +0200, Lukas Wunner wrote:
> 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.

I think switching everything together makes some sense, but either way
should be find.

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

DRM has essentially become the gfx subsystem of the kernel, the name is
purely historical accident. And DRM maintainers generally take care of
everything under drivers/gpu/* so I think it makes sense to include it
there.

If you think the DRM in the docbook is confusing then we can fix that
easily by renaming it to gpu.tmpl and GPU Developer's Guide. Actually that
seems like a decent idea, so let me write that patch.

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

Mailing list is highly preferred.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch


More information about the dri-devel mailing list