| Message ID | 20181105110253.8655-1-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vga_switcheroo: Fix missing gpu_bound call at audio client registration | expand |
On Mon, Nov 05, 2018 at 12:02:53PM +0100, Takashi Iwai wrote: > The commit 37a3a98ef601 ("ALSA: hda - Enable runtime PM only for > discrete GPU") added a new ops gpu_bound to be called when GPU gets > bound. The patch overlooked, however, that vga_switcheroo_enable() is > called only once at GPU is bound. When an audio client is registered > after that point, it would miss the gpu_bound call. This leads to the > unexpected lack of runtime PM in HD-audio side. > > For addressing that regression, just call gpu_bound callback manually > at vga_switcheroo_register_audio_client() when the GPU was already > bound. > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=201615 > Fixes: 37a3a98ef601 ("ALSA: hda - Enable runtime PM only for discrete GPU") > Cc: <stable@vger.kernel.org> > Signed-off-by: Takashi Iwai <tiwai@suse.de> > --- > drivers/gpu/vga/vga_switcheroo.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c > index cf2a18571d48..a132c37d7334 100644 > --- a/drivers/gpu/vga/vga_switcheroo.c > +++ b/drivers/gpu/vga/vga_switcheroo.c > @@ -380,6 +380,9 @@ int vga_switcheroo_register_audio_client(struct pci_dev *pdev, > mutex_unlock(&vgasr_mutex); > return -EINVAL; > } > + /* notify if GPU has been already bound */ > + if (ops->gpu_bound) > + ops->gpu_bound(pdev, id); > } > mutex_unlock(&vgasr_mutex); If the audio client registers before vga_switcheroo becomes enabled, ->gpu_bound will be executed twice. AFAICS this only causes a minor cosmetic issue, namely that dev_info() is called twice for blacklisted devices in set_default_power_save(). This could be eliminated by amending the if-condition with "&& vgasr_priv.active". Since this is only minor, up to you if you want to respin. Either way, Reviewed-by: Lukas Wunner <lukas@wunner.de>
On Mon, 05 Nov 2018 12:55:17 +0100, Lukas Wunner wrote: > > On Mon, Nov 05, 2018 at 12:02:53PM +0100, Takashi Iwai wrote: > > The commit 37a3a98ef601 ("ALSA: hda - Enable runtime PM only for > > discrete GPU") added a new ops gpu_bound to be called when GPU gets > > bound. The patch overlooked, however, that vga_switcheroo_enable() is > > called only once at GPU is bound. When an audio client is registered > > after that point, it would miss the gpu_bound call. This leads to the > > unexpected lack of runtime PM in HD-audio side. > > > > For addressing that regression, just call gpu_bound callback manually > > at vga_switcheroo_register_audio_client() when the GPU was already > > bound. > > > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=201615 > > Fixes: 37a3a98ef601 ("ALSA: hda - Enable runtime PM only for discrete GPU") > > Cc: <stable@vger.kernel.org> > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > --- > > drivers/gpu/vga/vga_switcheroo.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c > > index cf2a18571d48..a132c37d7334 100644 > > --- a/drivers/gpu/vga/vga_switcheroo.c > > +++ b/drivers/gpu/vga/vga_switcheroo.c > > @@ -380,6 +380,9 @@ int vga_switcheroo_register_audio_client(struct pci_dev *pdev, > > mutex_unlock(&vgasr_mutex); > > return -EINVAL; > > } > > + /* notify if GPU has been already bound */ > > + if (ops->gpu_bound) > > + ops->gpu_bound(pdev, id); > > } > > mutex_unlock(&vgasr_mutex); > > If the audio client registers before vga_switcheroo becomes enabled, > ->gpu_bound will be executed twice. Is it? The addition is in the if-block of vgasr_priv.active, so this is executed only when vga_switcheroo_enable() was already called beforehand. > AFAICS this only causes a minor > cosmetic issue, namely that dev_info() is called twice for blacklisted > devices in set_default_power_save(). This could be eliminated by > amending the if-condition with "&& vgasr_priv.active". Since this > is only minor, up to you if you want to respin. > > Either way, > Reviewed-by: Lukas Wunner <lukas@wunner.de> Thanks! Takashi
On Mon, Nov 05, 2018 at 01:51:23PM +0100, Takashi Iwai wrote: > On Mon, 05 Nov 2018 12:55:17 +0100, Lukas Wunner wrote: > > On Mon, Nov 05, 2018 at 12:02:53PM +0100, Takashi Iwai wrote: > > > diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c > > > index cf2a18571d48..a132c37d7334 100644 > > > --- a/drivers/gpu/vga/vga_switcheroo.c > > > +++ b/drivers/gpu/vga/vga_switcheroo.c > > > @@ -380,6 +380,9 @@ int vga_switcheroo_register_audio_client(struct pci_dev *pdev, > > > mutex_unlock(&vgasr_mutex); > > > return -EINVAL; > > > } > > > + /* notify if GPU has been already bound */ > > > + if (ops->gpu_bound) > > > + ops->gpu_bound(pdev, id); > > > } > > > mutex_unlock(&vgasr_mutex); > > > > If the audio client registers before vga_switcheroo becomes enabled, > > ->gpu_bound will be executed twice. > > Is it? The addition is in the if-block of vgasr_priv.active, so this > is executed only when vga_switcheroo_enable() was already called > beforehand. Ugh, you're absolutely right, sorry for the noise. Lukas
On Mon, 05 Nov 2018 13:59:37 +0100, Lukas Wunner wrote: > > On Mon, Nov 05, 2018 at 01:51:23PM +0100, Takashi Iwai wrote: > > On Mon, 05 Nov 2018 12:55:17 +0100, Lukas Wunner wrote: > > > On Mon, Nov 05, 2018 at 12:02:53PM +0100, Takashi Iwai wrote: > > > > diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c > > > > index cf2a18571d48..a132c37d7334 100644 > > > > --- a/drivers/gpu/vga/vga_switcheroo.c > > > > +++ b/drivers/gpu/vga/vga_switcheroo.c > > > > @@ -380,6 +380,9 @@ int vga_switcheroo_register_audio_client(struct pci_dev *pdev, > > > > mutex_unlock(&vgasr_mutex); > > > > return -EINVAL; > > > > } > > > > + /* notify if GPU has been already bound */ > > > > + if (ops->gpu_bound) > > > > + ops->gpu_bound(pdev, id); > > > > } > > > > mutex_unlock(&vgasr_mutex); > > > > > > If the audio client registers before vga_switcheroo becomes enabled, > > > ->gpu_bound will be executed twice. > > > > Is it? The addition is in the if-block of vgasr_priv.active, so this > > is executed only when vga_switcheroo_enable() was already called > > beforehand. > > Ugh, you're absolutely right, sorry for the noise. OK, then let's queue the fix. Would you take it and go through drm merges, or shall I take it via sound git tree? I don't mind either way. thanks, Takashi
On Mon, Nov 05, 2018 at 02:11:06PM +0100, Takashi Iwai wrote: > On Mon, 05 Nov 2018 13:59:37 +0100, Lukas Wunner wrote: > > On Mon, Nov 05, 2018 at 01:51:23PM +0100, Takashi Iwai wrote: > > > On Mon, 05 Nov 2018 12:55:17 +0100, Lukas Wunner wrote: > > > > On Mon, Nov 05, 2018 at 12:02:53PM +0100, Takashi Iwai wrote: > > > > > diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c > > > > > index cf2a18571d48..a132c37d7334 100644 > > > > > --- a/drivers/gpu/vga/vga_switcheroo.c > > > > > +++ b/drivers/gpu/vga/vga_switcheroo.c > > > > > @@ -380,6 +380,9 @@ int vga_switcheroo_register_audio_client(struct pci_dev *pdev, > > > > > mutex_unlock(&vgasr_mutex); > > > > > return -EINVAL; > > > > > } > > > > > + /* notify if GPU has been already bound */ > > > > > + if (ops->gpu_bound) > > > > > + ops->gpu_bound(pdev, id); > > > > > } > > > > > mutex_unlock(&vgasr_mutex); > > > > > > > > If the audio client registers before vga_switcheroo becomes enabled, > > > > ->gpu_bound will be executed twice. > > > > > > Is it? The addition is in the if-block of vgasr_priv.active, so this > > > is executed only when vga_switcheroo_enable() was already called > > > beforehand. > > > > Ugh, you're absolutely right, sorry for the noise. > > OK, then let's queue the fix. > > Would you take it and go through drm merges, or shall I take it via > sound git tree? I don't mind either way. It looks like commit 37a3a98ef601 (which this one is fixing) went in through the sound tree, so I think it makes sense to use it for this patch as well. Thanks, Lukas
On Mon, 05 Nov 2018 14:38:42 +0100, Lukas Wunner wrote: > > On Mon, Nov 05, 2018 at 02:11:06PM +0100, Takashi Iwai wrote: > > On Mon, 05 Nov 2018 13:59:37 +0100, Lukas Wunner wrote: > > > On Mon, Nov 05, 2018 at 01:51:23PM +0100, Takashi Iwai wrote: > > > > On Mon, 05 Nov 2018 12:55:17 +0100, Lukas Wunner wrote: > > > > > On Mon, Nov 05, 2018 at 12:02:53PM +0100, Takashi Iwai wrote: > > > > > > diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c > > > > > > index cf2a18571d48..a132c37d7334 100644 > > > > > > --- a/drivers/gpu/vga/vga_switcheroo.c > > > > > > +++ b/drivers/gpu/vga/vga_switcheroo.c > > > > > > @@ -380,6 +380,9 @@ int vga_switcheroo_register_audio_client(struct pci_dev *pdev, > > > > > > mutex_unlock(&vgasr_mutex); > > > > > > return -EINVAL; > > > > > > } > > > > > > + /* notify if GPU has been already bound */ > > > > > > + if (ops->gpu_bound) > > > > > > + ops->gpu_bound(pdev, id); > > > > > > } > > > > > > mutex_unlock(&vgasr_mutex); > > > > > > > > > > If the audio client registers before vga_switcheroo becomes enabled, > > > > > ->gpu_bound will be executed twice. > > > > > > > > Is it? The addition is in the if-block of vgasr_priv.active, so this > > > > is executed only when vga_switcheroo_enable() was already called > > > > beforehand. > > > > > > Ugh, you're absolutely right, sorry for the noise. > > > > OK, then let's queue the fix. > > > > Would you take it and go through drm merges, or shall I take it via > > sound git tree? I don't mind either way. > > It looks like commit 37a3a98ef601 (which this one is fixing) went in > through the sound tree, so I think it makes sense to use it for this > patch as well. Alright, merged now. Thanks! Takashi
diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c index cf2a18571d48..a132c37d7334 100644 --- a/drivers/gpu/vga/vga_switcheroo.c +++ b/drivers/gpu/vga/vga_switcheroo.c @@ -380,6 +380,9 @@ int vga_switcheroo_register_audio_client(struct pci_dev *pdev, mutex_unlock(&vgasr_mutex); return -EINVAL; } + /* notify if GPU has been already bound */ + if (ops->gpu_bound) + ops->gpu_bound(pdev, id); } mutex_unlock(&vgasr_mutex);
The commit 37a3a98ef601 ("ALSA: hda - Enable runtime PM only for discrete GPU") added a new ops gpu_bound to be called when GPU gets bound. The patch overlooked, however, that vga_switcheroo_enable() is called only once at GPU is bound. When an audio client is registered after that point, it would miss the gpu_bound call. This leads to the unexpected lack of runtime PM in HD-audio side. For addressing that regression, just call gpu_bound callback manually at vga_switcheroo_register_audio_client() when the GPU was already bound. Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=201615 Fixes: 37a3a98ef601 ("ALSA: hda - Enable runtime PM only for discrete GPU") Cc: <stable@vger.kernel.org> Signed-off-by: Takashi Iwai <tiwai@suse.de> --- drivers/gpu/vga/vga_switcheroo.c | 3 +++ 1 file changed, 3 insertions(+)