[05/15] vga_switcheroo: Drop client power state VGA_SWITCHEROO_INIT
diff mbox

Message ID f4decc6a8b5c9a8d6db06d89813e11497d82bd40.1442497843.git.lukas@wunner.de
State New
Headers show

Commit Message

Lukas Wunner Sept. 8, 2015, 12:17 p.m. UTC
hda_intel.c:azx_probe() defers initialization of an audio controller
on the discrete GPU if the GPU is powered off. The power state of the
GPU is determined by calling vga_switcheroo_get_client_state().

vga_switcheroo_get_client_state() returns VGA_SWITCHEROO_INIT if
vga_switcheroo is not enabled, i.e. if no second GPU or no handler
has registered.

This can go wrong in the following scenario:
- Driver for the integrated GPU is not loaded.
- Driver for the discrete GPU registers with vga_switcheroo, uses driver
  power control to power down the GPU, handler cuts power to the GPU.
- Driver for the audio controller gets loaded after the GPU was powered
  down, calls vga_switcheroo_get_client_state() which returns
  VGA_SWITCHEROO_INIT instead of VGA_SWITCHEROO_OFF.
- Consequence: azx_probe() tries to initialize the audio controller even
  though the GPU is powered down.

The power state VGA_SWITCHEROO_INIT was introduced by c8e9cf7bb240
("vga_switcheroo: Add a helper function to get the client state").
It is not apparent what its benefit might be. The idea seems to
be to initialize the audio controller even if the power state is
VGA_SWITCHEROO_OFF (were vga_switcheroo enabled), but as shown
above this can fail.

Drop VGA_SWITCHEROO_INIT to solve this.

Cc: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/vga/vga_switcheroo.c | 2 --
 include/linux/vga_switcheroo.h   | 5 -----
 2 files changed, 7 deletions(-)

Comments

Daniel Vetter Oct. 2, 2015, 8:22 a.m. UTC | #1
On Tue, Sep 08, 2015 at 02:17:47PM +0200, Lukas Wunner wrote:
> hda_intel.c:azx_probe() defers initialization of an audio controller
> on the discrete GPU if the GPU is powered off. The power state of the
> GPU is determined by calling vga_switcheroo_get_client_state().
> 
> vga_switcheroo_get_client_state() returns VGA_SWITCHEROO_INIT if
> vga_switcheroo is not enabled, i.e. if no second GPU or no handler
> has registered.
> 
> This can go wrong in the following scenario:
> - Driver for the integrated GPU is not loaded.
> - Driver for the discrete GPU registers with vga_switcheroo, uses driver
>   power control to power down the GPU, handler cuts power to the GPU.
> - Driver for the audio controller gets loaded after the GPU was powered
>   down, calls vga_switcheroo_get_client_state() which returns
>   VGA_SWITCHEROO_INIT instead of VGA_SWITCHEROO_OFF.
> - Consequence: azx_probe() tries to initialize the audio controller even
>   though the GPU is powered down.
> 
> The power state VGA_SWITCHEROO_INIT was introduced by c8e9cf7bb240
> ("vga_switcheroo: Add a helper function to get the client state").
> It is not apparent what its benefit might be. The idea seems to
> be to initialize the audio controller even if the power state is
> VGA_SWITCHEROO_OFF (were vga_switcheroo enabled), but as shown
> above this can fail.
> 
> Drop VGA_SWITCHEROO_INIT to solve this.
> 
> Cc: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Takashi, does this look good to you? Ack for merging through drm-misc?

I pulled in patch 4 meanwhile.
-Daniel

> ---
>  drivers/gpu/vga/vga_switcheroo.c | 2 --
>  include/linux/vga_switcheroo.h   | 5 -----
>  2 files changed, 7 deletions(-)
> 
> diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> index 2138162..845e47d 100644
> --- a/drivers/gpu/vga/vga_switcheroo.c
> +++ b/drivers/gpu/vga/vga_switcheroo.c
> @@ -355,8 +355,6 @@ int vga_switcheroo_get_client_state(struct pci_dev *pdev)
>  	client = find_client_from_pci(&vgasr_priv.clients, pdev);
>  	if (!client)
>  		ret = VGA_SWITCHEROO_NOT_FOUND;
> -	else if (!vgasr_priv.active)
> -		ret = VGA_SWITCHEROO_INIT;
>  	else
>  		ret = client->pwr_state;
>  	mutex_unlock(&vgasr_mutex);
> diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
> index 3764991..95bfbeb0 100644
> --- a/include/linux/vga_switcheroo.h
> +++ b/include/linux/vga_switcheroo.h
> @@ -39,10 +39,6 @@ struct pci_dev;
>   * enum vga_switcheroo_state - client power state
>   * @VGA_SWITCHEROO_OFF: off
>   * @VGA_SWITCHEROO_ON: on
> - * @VGA_SWITCHEROO_INIT: client has registered with vga_switcheroo but
> - * 	vga_switcheroo is not enabled, i.e. no second client or no handler
> - * 	has registered. Only used in vga_switcheroo_get_client_state() which
> - * 	in turn is only called from hda_intel.c
>   * @VGA_SWITCHEROO_NOT_FOUND: client has not registered with vga_switcheroo.
>   * 	Only used in vga_switcheroo_get_client_state() which in turn is only
>   * 	called from hda_intel.c
> @@ -53,7 +49,6 @@ enum vga_switcheroo_state {
>  	VGA_SWITCHEROO_OFF,
>  	VGA_SWITCHEROO_ON,
>  	/* below are referred only from vga_switcheroo_get_client_state() */
> -	VGA_SWITCHEROO_INIT,
>  	VGA_SWITCHEROO_NOT_FOUND,
>  };
>  
> -- 
> 1.8.5.2 (Apple Git-48)
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Lukas Wunner Oct. 25, 2015, 6:19 p.m. UTC | #2
Hi Takashi,

gentle ping -- could you take a look at the patch below,
would this change be okay with you?

hda_intel.c:check_hdmi_disabled() is the sole caller of
vga_switcheroo_get_client_state() and only checks for the
return value VGA_SWITCHEROO_OFF, not for VGA_SWITCHEROO_INIT.

So why was VGA_SWITCHEROO_INIT introduced in the first place?
The only explanation I can think of is that for some reason you
wanted to *avoid* returning VGA_SWITCHEROO_OFF (even if the card
is in fact asleep) if no second GPU or no handler have registered
with vga_switcheroo. But as shown in the scenario below, this can
actually go awry.

With VGA_SWITCHEROO_INIT apparently not having a purpose but on the
other hand being harmful, I suggest to drop it with the patch below.

For the meaning of the vga_switcheroo power states, refer to
vga_switcheroo.h in drm-next:
http://cgit.freedesktop.org/~airlied/linux/tree/include/linux/vga_switcheroo.h?h=drm-next#n38

Thanks & best regards,

Lukas

On Fri, Oct 02, 2015 at 10:22:48AM +0200, Daniel Vetter wrote:
> On Tue, Sep 08, 2015 at 02:17:47PM +0200, Lukas Wunner wrote:
> > hda_intel.c:azx_probe() defers initialization of an audio controller
> > on the discrete GPU if the GPU is powered off. The power state of the
> > GPU is determined by calling vga_switcheroo_get_client_state().
> > 
> > vga_switcheroo_get_client_state() returns VGA_SWITCHEROO_INIT if
> > vga_switcheroo is not enabled, i.e. if no second GPU or no handler
> > has registered.
> > 
> > This can go wrong in the following scenario:
> > - Driver for the integrated GPU is not loaded.
> > - Driver for the discrete GPU registers with vga_switcheroo, uses driver
> >   power control to power down the GPU, handler cuts power to the GPU.
> > - Driver for the audio controller gets loaded after the GPU was powered
> >   down, calls vga_switcheroo_get_client_state() which returns
> >   VGA_SWITCHEROO_INIT instead of VGA_SWITCHEROO_OFF.
> > - Consequence: azx_probe() tries to initialize the audio controller even
> >   though the GPU is powered down.
> > 
> > The power state VGA_SWITCHEROO_INIT was introduced by c8e9cf7bb240
> > ("vga_switcheroo: Add a helper function to get the client state").
> > It is not apparent what its benefit might be. The idea seems to
> > be to initialize the audio controller even if the power state is
> > VGA_SWITCHEROO_OFF (were vga_switcheroo enabled), but as shown
> > above this can fail.
> > 
> > Drop VGA_SWITCHEROO_INIT to solve this.
> > 
> > Cc: Takashi Iwai <tiwai@suse.de>
> > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> 
> Takashi, does this look good to you? Ack for merging through drm-misc?
> 
> I pulled in patch 4 meanwhile.
> -Daniel
> 
> > ---
> >  drivers/gpu/vga/vga_switcheroo.c | 2 --
> >  include/linux/vga_switcheroo.h   | 5 -----
> >  2 files changed, 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> > index 2138162..845e47d 100644
> > --- a/drivers/gpu/vga/vga_switcheroo.c
> > +++ b/drivers/gpu/vga/vga_switcheroo.c
> > @@ -355,8 +355,6 @@ int vga_switcheroo_get_client_state(struct pci_dev *pdev)
> >  	client = find_client_from_pci(&vgasr_priv.clients, pdev);
> >  	if (!client)
> >  		ret = VGA_SWITCHEROO_NOT_FOUND;
> > -	else if (!vgasr_priv.active)
> > -		ret = VGA_SWITCHEROO_INIT;
> >  	else
> >  		ret = client->pwr_state;
> >  	mutex_unlock(&vgasr_mutex);
> > diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
> > index 3764991..95bfbeb0 100644
> > --- a/include/linux/vga_switcheroo.h
> > +++ b/include/linux/vga_switcheroo.h
> > @@ -39,10 +39,6 @@ struct pci_dev;
> >   * enum vga_switcheroo_state - client power state
> >   * @VGA_SWITCHEROO_OFF: off
> >   * @VGA_SWITCHEROO_ON: on
> > - * @VGA_SWITCHEROO_INIT: client has registered with vga_switcheroo but
> > - * 	vga_switcheroo is not enabled, i.e. no second client or no handler
> > - * 	has registered. Only used in vga_switcheroo_get_client_state() which
> > - * 	in turn is only called from hda_intel.c
> >   * @VGA_SWITCHEROO_NOT_FOUND: client has not registered with vga_switcheroo.
> >   * 	Only used in vga_switcheroo_get_client_state() which in turn is only
> >   * 	called from hda_intel.c
> > @@ -53,7 +49,6 @@ enum vga_switcheroo_state {
> >  	VGA_SWITCHEROO_OFF,
> >  	VGA_SWITCHEROO_ON,
> >  	/* below are referred only from vga_switcheroo_get_client_state() */
> > -	VGA_SWITCHEROO_INIT,
> >  	VGA_SWITCHEROO_NOT_FOUND,
> >  };
> >  
> > -- 
> > 1.8.5.2 (Apple Git-48)
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Takashi Iwai Oct. 26, 2015, 7:24 a.m. UTC | #3
On Sun, 25 Oct 2015 19:19:53 +0100,
Lukas Wunner wrote:
> 
> Hi Takashi,
> 
> gentle ping -- could you take a look at the patch below,
> would this change be okay with you?

I thought I gave already my ack?  Hmm..  The series is unnecessarily
confusing...
In anyway, feel free to take my ack if not pushed yet.

> hda_intel.c:check_hdmi_disabled() is the sole caller of
> vga_switcheroo_get_client_state() and only checks for the
> return value VGA_SWITCHEROO_OFF, not for VGA_SWITCHEROO_INIT.
> 
> So why was VGA_SWITCHEROO_INIT introduced in the first place?
> The only explanation I can think of is that for some reason you
> wanted to *avoid* returning VGA_SWITCHEROO_OFF (even if the card
> is in fact asleep) if no second GPU or no handler have registered
> with vga_switcheroo. But as shown in the scenario below, this can
> actually go awry.
> 
> With VGA_SWITCHEROO_INIT apparently not having a purpose but on the
> other hand being harmful, I suggest to drop it with the patch below.
> 
> For the meaning of the vga_switcheroo power states, refer to
> vga_switcheroo.h in drm-next:
> http://cgit.freedesktop.org/~airlied/linux/tree/include/linux/vga_switcheroo.h?h=drm-next#n38

The initial concept was that the audio component may be independent
from the GPU state.  Indeed, this may happen in theory, as it's an
individual PCI entry that has nothing to do with the GPU per se.
The actual existing hardware for AMD and Nvidia, however, requires the
implicitly tight binding with the GPU state.  Thus the hackish
function like check_hdmi_disabled() was introduced.

That said, conceptually we had to think of the different states, and
it was the reason that each client (even audio) has the individual
active field and passed by ops.  In reality, though, we got only these
a few GPUs.  So, it's fine to drop the initial concept, and adjust to
the existing code.


Takashi


> 
> Thanks & best regards,
> 
> Lukas
> 
> On Fri, Oct 02, 2015 at 10:22:48AM +0200, Daniel Vetter wrote:
> > On Tue, Sep 08, 2015 at 02:17:47PM +0200, Lukas Wunner wrote:
> > > hda_intel.c:azx_probe() defers initialization of an audio controller
> > > on the discrete GPU if the GPU is powered off. The power state of the
> > > GPU is determined by calling vga_switcheroo_get_client_state().
> > > 
> > > vga_switcheroo_get_client_state() returns VGA_SWITCHEROO_INIT if
> > > vga_switcheroo is not enabled, i.e. if no second GPU or no handler
> > > has registered.
> > > 
> > > This can go wrong in the following scenario:
> > > - Driver for the integrated GPU is not loaded.
> > > - Driver for the discrete GPU registers with vga_switcheroo, uses driver
> > >   power control to power down the GPU, handler cuts power to the GPU.
> > > - Driver for the audio controller gets loaded after the GPU was powered
> > >   down, calls vga_switcheroo_get_client_state() which returns
> > >   VGA_SWITCHEROO_INIT instead of VGA_SWITCHEROO_OFF.
> > > - Consequence: azx_probe() tries to initialize the audio controller even
> > >   though the GPU is powered down.
> > > 
> > > The power state VGA_SWITCHEROO_INIT was introduced by c8e9cf7bb240
> > > ("vga_switcheroo: Add a helper function to get the client state").
> > > It is not apparent what its benefit might be. The idea seems to
> > > be to initialize the audio controller even if the power state is
> > > VGA_SWITCHEROO_OFF (were vga_switcheroo enabled), but as shown
> > > above this can fail.
> > > 
> > > Drop VGA_SWITCHEROO_INIT to solve this.
> > > 
> > > Cc: Takashi Iwai <tiwai@suse.de>
> > > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > 
> > Takashi, does this look good to you? Ack for merging through drm-misc?
> > 
> > I pulled in patch 4 meanwhile.
> > -Daniel
> > 
> > > ---
> > >  drivers/gpu/vga/vga_switcheroo.c | 2 --
> > >  include/linux/vga_switcheroo.h   | 5 -----
> > >  2 files changed, 7 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> > > index 2138162..845e47d 100644
> > > --- a/drivers/gpu/vga/vga_switcheroo.c
> > > +++ b/drivers/gpu/vga/vga_switcheroo.c
> > > @@ -355,8 +355,6 @@ int vga_switcheroo_get_client_state(struct pci_dev *pdev)
> > >  	client = find_client_from_pci(&vgasr_priv.clients, pdev);
> > >  	if (!client)
> > >  		ret = VGA_SWITCHEROO_NOT_FOUND;
> > > -	else if (!vgasr_priv.active)
> > > -		ret = VGA_SWITCHEROO_INIT;
> > >  	else
> > >  		ret = client->pwr_state;
> > >  	mutex_unlock(&vgasr_mutex);
> > > diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
> > > index 3764991..95bfbeb0 100644
> > > --- a/include/linux/vga_switcheroo.h
> > > +++ b/include/linux/vga_switcheroo.h
> > > @@ -39,10 +39,6 @@ struct pci_dev;
> > >   * enum vga_switcheroo_state - client power state
> > >   * @VGA_SWITCHEROO_OFF: off
> > >   * @VGA_SWITCHEROO_ON: on
> > > - * @VGA_SWITCHEROO_INIT: client has registered with vga_switcheroo but
> > > - * 	vga_switcheroo is not enabled, i.e. no second client or no handler
> > > - * 	has registered. Only used in vga_switcheroo_get_client_state() which
> > > - * 	in turn is only called from hda_intel.c
> > >   * @VGA_SWITCHEROO_NOT_FOUND: client has not registered with vga_switcheroo.
> > >   * 	Only used in vga_switcheroo_get_client_state() which in turn is only
> > >   * 	called from hda_intel.c
> > > @@ -53,7 +49,6 @@ enum vga_switcheroo_state {
> > >  	VGA_SWITCHEROO_OFF,
> > >  	VGA_SWITCHEROO_ON,
> > >  	/* below are referred only from vga_switcheroo_get_client_state() */
> > > -	VGA_SWITCHEROO_INIT,
> > >  	VGA_SWITCHEROO_NOT_FOUND,
> > >  };
> > >  
> > > -- 
> > > 1.8.5.2 (Apple Git-48)
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
>

Patch
diff mbox

diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index 2138162..845e47d 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -355,8 +355,6 @@  int vga_switcheroo_get_client_state(struct pci_dev *pdev)
 	client = find_client_from_pci(&vgasr_priv.clients, pdev);
 	if (!client)
 		ret = VGA_SWITCHEROO_NOT_FOUND;
-	else if (!vgasr_priv.active)
-		ret = VGA_SWITCHEROO_INIT;
 	else
 		ret = client->pwr_state;
 	mutex_unlock(&vgasr_mutex);
diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
index 3764991..95bfbeb0 100644
--- a/include/linux/vga_switcheroo.h
+++ b/include/linux/vga_switcheroo.h
@@ -39,10 +39,6 @@  struct pci_dev;
  * enum vga_switcheroo_state - client power state
  * @VGA_SWITCHEROO_OFF: off
  * @VGA_SWITCHEROO_ON: on
- * @VGA_SWITCHEROO_INIT: client has registered with vga_switcheroo but
- * 	vga_switcheroo is not enabled, i.e. no second client or no handler
- * 	has registered. Only used in vga_switcheroo_get_client_state() which
- * 	in turn is only called from hda_intel.c
  * @VGA_SWITCHEROO_NOT_FOUND: client has not registered with vga_switcheroo.
  * 	Only used in vga_switcheroo_get_client_state() which in turn is only
  * 	called from hda_intel.c
@@ -53,7 +49,6 @@  enum vga_switcheroo_state {
 	VGA_SWITCHEROO_OFF,
 	VGA_SWITCHEROO_ON,
 	/* below are referred only from vga_switcheroo_get_client_state() */
-	VGA_SWITCHEROO_INIT,
 	VGA_SWITCHEROO_NOT_FOUND,
 };