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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
