Re: [PATCH 05/15] vga_switcheroo: Drop client power state VGA_SWITCHEROO_INIT

[Lukas Wunner <lukas@wunner.de>]

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