[PATCH] vga_switcheroo: Fix missing gpu_bound call at audio client registration

[PATCH] vga_switcheroo: Fix missing gpu_bound call at audio client registration

[Takashi Iwai]

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);
 
-- 
2.19.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] vga_switcheroo: Fix missing gpu_bound call at audio client registration

[Lukas Wunner]

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>

Re: [PATCH] vga_switcheroo: Fix missing gpu_bound call at audio client registration

[Takashi Iwai]

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

Re: [PATCH] vga_switcheroo: Fix missing gpu_bound call at audio client registration

[Lukas Wunner]

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

Re: [PATCH] vga_switcheroo: Fix missing gpu_bound call at audio client registration

[Takashi Iwai]

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

Re: [PATCH] vga_switcheroo: Fix missing gpu_bound call at audio client registration

[Lukas Wunner]

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

Re: [PATCH] vga_switcheroo: Fix missing gpu_bound call at audio client registration

[Takashi Iwai]

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