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@xxxxxxx> > > Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx> > > 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@xxxxxxxxxxxxxxxxxxxxx > > http://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > 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
