Nouveau - Jul 2016 - vga_switcheroo audio runpm

Hi Peter,
> [12:42] Lekensteyn: Should the video card always be powered up when a
>	  register is read from the HDMI audio controller? Or would it be
>	  better to leave the video card suspended and just fail the HDA
>	  register reads?
It should probably be powered up.

> [12:43] Lekensteyn: I'm trying to figure out how vga_switcheroo should
	  handle this, maybe l1k has an idea?
> [12:48] Lekensteyn: The current implemented policy is that the video card
>	  dictates the power state and that the HDMI audio device has to
>	  adapt (even if it is seemingly in use)
This current architecture seems to have come about historically (Dave
Airlie first implemented vga_switcheroo with manual power control,
then added runtime pm in a second step).

It may also be motivated by the fact that the driver core is currently
not supporting dependencies between devices beyond the hierarchical
parent/child relationship.

Rafael Wysocki (cc:) posted a series in January addressing the latter
problem with so-called "device links". The series was reposted
recently
by Marek Szyprowski:
https://www.mail-archive.com/linux-kernel at vger.kernel.org/msg1170031.html

The vga_switcheroo audio handling should probably be reworked to use such
"device links". The result would be that the GPU won't runtime
suspend
as long as a ref is held for the audio card. The audio card needs to then
make sure that it releases any refs if it has nothing to do.

The "device links" series seems to impose more restrictions than we
actually need here, it seems to require that the "supplier" is bound
to a driver before the "consumer" may probe. IOW nouveau needs to be
bound before snd_hda_audio can probe. I'm not sure if that additional
(unnecessary) restriction is a problem.

I've recently tried to add runtime pm for muxed laptops to vga_switcheroo,
this is something that Pierre Moreau (cc:) has expressed an interest in
for his MacBook Pro. I came across a major roadblock in the form of
vga_switcheroo_set_dynamic_switch(). That function is called from the
DRM driver when the GPU runtime suspends and resumes. It takes the
vgasr_mutex. The problem is, if the user initiates a switch of the mux,
that mutex is already taken in vga_switcheroo_debugfs_write(). If the
GPU we're switching to is asleep, it'll wake up for the switch and
we'll get a deadlock when the DRM driver subsequently calls
vga_switcheroo_set_dynamic_switch().

I would like to get rid of vga_switcheroo_set_dynamic_switch() to solve
this. The function has two purposes: Number one, vga_switcheroo updates
its internal power state representation for the GPU. That is actually
pointless for driver power control because we can always query the
driver core for this information (e.g. pm_runtime_suspended()).
The second purpose is to switch the audio client on and off. If we would
use a "device link" to express the dependency between the audio device
and the GPU, we wouldn't need this. So moving to "device links" is
a prerequisite to make runtime pm work for muxed laptops.

If you want to take a stab at using "device links" for vga_switcheroo
then please go ahead as I'm swamped with other work. (I've reverse-
engineered retrieval of Apple device properties from EFI this week,
which is needed for Thunderbolt.) Let me know if you have any questions
or need stuff reviewed or whatever. Since the "device links" series
hasn't landed yet, it's still possible I think to ask for feature
requests should it not work for this particular use case. The
linux-pm at vger.kernel.org mailing list is the right place to inquire
about the series.

Thanks for raising this important question.

Lukas

On Sun, Jul 10, 2016 at 03:20:13PM +0200, Lukas Wunner
wrote:
> Hi Peter,
> 
> > [12:42] Lekensteyn: Should the video card always be powered up when a
> >	  register is read from the HDMI audio controller? Or would it be
> >	  better to leave the video card suspended and just fail the HDA
> >	  register reads?
> 
> It should probably be powered up.
> 
> 
> > [12:43] Lekensteyn: I'm trying to figure out how vga_switcheroo
should
> 	  handle this, maybe l1k has an idea?
> > [12:48] Lekensteyn: The current implemented policy is that the video
card
> >	  dictates the power state and that the HDMI audio device has to
> >	  adapt (even if it is seemingly in use)
> 
> This current architecture seems to have come about historically (Dave
> Airlie first implemented vga_switcheroo with manual power control,
> then added runtime pm in a second step).
> 
> It may also be motivated by the fact that the driver core is currently
> not supporting dependencies between devices beyond the hierarchical
> parent/child relationship.
> 
> Rafael Wysocki (cc:) posted a series in January addressing the latter
> problem with so-called "device links". The series was reposted
recently
> by Marek Szyprowski:
> https://www.mail-archive.com/linux-kernel at
vger.kernel.org/msg1170031.html
> 
> The vga_switcheroo audio handling should probably be reworked to use such
> "device links". The result would be that the GPU won't
runtime suspend
> as long as a ref is held for the audio card. The audio card needs to then
> make sure that it releases any refs if it has nothing to do.
> 
> The "device links" series seems to impose more restrictions than
we
> actually need here, it seems to require that the "supplier" is
bound
> to a driver before the "consumer" may probe. IOW nouveau needs to
be
> bound before snd_hda_audio can probe. I'm not sure if that additional
> (unnecessary) restriction is a problem.
The above (including the load ordering restriction, which we resolve using
EPROBE_DEFER) is how we hand-rolled a fix for this between i915 and
snd-hda-intel. Not for vga-switcheroo, but for rpm depencies between gpu
and snd in general. Not sure why exactly vga-switcheroo jumps into this
here ... but sounds like the device links stuff is the way to go.
> I've recently tried to add runtime pm for muxed laptops to
vga_switcheroo,
> this is something that Pierre Moreau (cc:) has expressed an interest in
> for his MacBook Pro. I came across a major roadblock in the form of
> vga_switcheroo_set_dynamic_switch(). That function is called from the
> DRM driver when the GPU runtime suspends and resumes. It takes the
> vgasr_mutex. The problem is, if the user initiates a switch of the mux,
> that mutex is already taken in vga_switcheroo_debugfs_write(). If the
> GPU we're switching to is asleep, it'll wake up for the switch and
> we'll get a deadlock when the DRM driver subsequently calls
> vga_switcheroo_set_dynamic_switch().
> 
> I would like to get rid of vga_switcheroo_set_dynamic_switch() to solve
> this. The function has two purposes: Number one, vga_switcheroo updates
> its internal power state representation for the GPU. That is actually
> pointless for driver power control because we can always query the
> driver core for this information (e.g. pm_runtime_suspended()).
> The second purpose is to switch the audio client on and off. If we would
> use a "device link" to express the dependency between the audio
device
> and the GPU, we wouldn't need this. So moving to "device
links" is
> a prerequisite to make runtime pm work for muxed laptops.
right, expressing the depencies explicitly (and managing it on the audio
side) would also solve this deadlock.
> If you want to take a stab at using "device links" for
vga_switcheroo
> then please go ahead as I'm swamped with other work. (I've reverse-
> engineered retrieval of Apple device properties from EFI this week,
> which is needed for Thunderbolt.) Let me know if you have any questions
> or need stuff reviewed or whatever. Since the "device links"
series
> hasn't landed yet, it's still possible I think to ask for feature
> requests should it not work for this particular use case. The
> linux-pm at vger.kernel.org mailing list is the right place to inquire
> about the series.
> 
> Thanks for raising this important question.
+1 from me on the overall plan.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

On Sun, Jul 10, 2016 at 03:20:13PM +0200, Lukas Wunner
wrote:
> Hi Peter,
> 
> > [12:42] Lekensteyn: Should the video card always be powered up when a
> >	  register is read from the HDMI audio controller? Or would it be
> >	  better to leave the video card suspended and just fail the HDA
> >	  register reads?
> 
> It should probably be powered up.
Seems sensible, a video signal is apparently also required if you want
to play audio.
> > [12:43] Lekensteyn: I'm trying to figure out how vga_switcheroo
should
> 	  handle this, maybe l1k has an idea?
> > [12:48] Lekensteyn: The current implemented policy is that the video
card
> >	  dictates the power state and that the HDMI audio device has to
> >	  adapt (even if it is seemingly in use)
> 
> This current architecture seems to have come about historically (Dave
> Airlie first implemented vga_switcheroo with manual power control,
> then added runtime pm in a second step).
> 
> It may also be motivated by the fact that the driver core is currently
> not supporting dependencies between devices beyond the hierarchical
> parent/child relationship.
> 
> Rafael Wysocki (cc:) posted a series in January addressing the latter
> problem with so-called "device links". The series was reposted
recently
> by Marek Szyprowski:
> https://www.mail-archive.com/linux-kernel at
vger.kernel.org/msg1170031.html
> 
> The vga_switcheroo audio handling should probably be reworked to use such
> "device links". The result would be that the GPU won't
runtime suspend
> as long as a ref is held for the audio card. The audio card needs to then
> make sure that it releases any refs if it has nothing to do.
> 
> The "device links" series seems to impose more restrictions than
we
> actually need here, it seems to require that the "supplier" is
bound
> to a driver before the "consumer" may probe. IOW nouveau needs to
be
> bound before snd_hda_audio can probe. I'm not sure if that additional
> (unnecessary) restriction is a problem.
The device links feature looks promising. My initial guess that it is OK
to wait for nouveau to become available (as supplier), the audio port
(as consumer) probably does not work anyway without a video signal.
> I've recently tried to add runtime pm for muxed laptops to
vga_switcheroo,
> this is something that Pierre Moreau (cc:) has expressed an interest in
> for his MacBook Pro. I came across a major roadblock in the form of
> vga_switcheroo_set_dynamic_switch(). That function is called from the
> DRM driver when the GPU runtime suspends and resumes. It takes the
> vgasr_mutex. The problem is, if the user initiates a switch of the mux,
> that mutex is already taken in vga_switcheroo_debugfs_write(). If the
> GPU we're switching to is asleep, it'll wake up for the switch and
> we'll get a deadlock when the DRM driver subsequently calls
> vga_switcheroo_set_dynamic_switch().
> 
> I would like to get rid of vga_switcheroo_set_dynamic_switch() to solve
> this. The function has two purposes: Number one, vga_switcheroo updates
> its internal power state representation for the GPU. That is actually
> pointless for driver power control because we can always query the
> driver core for this information (e.g. pm_runtime_suspended()).
> The second purpose is to switch the audio client on and off. If we would
> use a "device link" to express the dependency between the audio
device
> and the GPU, we wouldn't need this. So moving to "device
links" is
> a prerequisite to make runtime pm work for muxed laptops.
This internal state is likely a historical artifact due to the manual
control (ON / OFF) that was needed in the past. I have recently tried to
draw the dependencies on paper and the suspend/resume to dynamic switch
flow is not the prettiest ;) Using runtime pm would probably make the
dependencies clearer in a logical way.
> If you want to take a stab at using "device links" for
vga_switcheroo
> then please go ahead as I'm swamped with other work. (I've reverse-
> engineered retrieval of Apple device properties from EFI this week,
> which is needed for Thunderbolt.) Let me know if you have any questions
> or need stuff reviewed or whatever. Since the "device links"
series
> hasn't landed yet, it's still possible I think to ask for feature
> requests should it not work for this particular use case. The
> linux-pm at vger.kernel.org mailing list is the right place to inquire
> about the series.
> 
> Thanks for raising this important question.
I'll give this a go after finishing the PR3 nouveau patches and fixing
some locking issues.
-- 
Kind regards,
Peter Wu
https://lekensteyn.nl