Re: [PATCH v2 2/9] ALSA: hda/i915: Allow override of gpu binding.

[Maarten Lankhorst]

Hey,

On 2023-07-24 12:25, Pierre-Louis Bossart wrote:
> On 7/21/23 14:19, Péter Ujfalusi wrote:
> > On 19/07/2023 19:41, Maarten Lankhorst wrote:
> > > Selecting CONFIG_DRM selects CONFIG_VIDEO_NOMODESET, which exports
> > > video_firmware_drivers_only(). This can be used as a first
> > > approximation on whether i915 will be available. It's safe to use as
> > > this is only built when CONFIG_SND_HDA_I915 is selected by CONFIG_I915.
> > >
> > > It's not completely fool proof, as you can boot with "nomodeset
> > > i915.modeset=1" to make i915 load regardless, or use
> > > "i915.force_probe=!*" to never load i915, but the common case of
> > > booting with nomodeset to disable all GPU drivers this will work as
> > > intended.
> > >
> > > Because of this, we add an extra module parameter,
> > > snd_hda_core.gpu_bind that can be used to signal users intent.
> > > -1 follows nomodeset, 0 disables binding, 1 forces wait/-EPROBE_DEFER
> > > on binding.
> >
> > Reviewed-by: Peter Ujfalusi <peter.ujfalusi@xxxxxxxxxxxxxxx>
> >
> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > > ---
> > >   sound/hda/hdac_i915.c | 10 ++++++++++
> > >   1 file changed, 10 insertions(+)
> > >
> > > diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
> > > index 161a9711cd63e..c32709fa4115f 100644
> > > --- a/sound/hda/hdac_i915.c
> > > +++ b/sound/hda/hdac_i915.c
> > > @@ -11,6 +11,13 @@
> > >   #include <sound/hda_i915.h>
> > >   #include <sound/hda_register.h>
> > >   
> > > +#include <video/nomodeset.h>
> > > +
> > > +static int gpu_bind = -1;
> > > +module_param(gpu_bind, int, 0644);
> > > +MODULE_PARM_DESC(gpu_bind, "Whether to bind sound component to GPU "
> > > +			   "(1=always, 0=never, -1=on nomodeset(default))");
> > > +
> > >   #define IS_HSW_CONTROLLER(pci) (((pci)->device == 0x0a0c) || \
> > >   				((pci)->device == 0x0c0c) || \
> > >   				((pci)->device == 0x0d0c) || \
> > > @@ -121,6 +128,9 @@ static int i915_gfx_present(struct pci_dev *hdac_pci)
> > >   {
> > >   	struct pci_dev *display_dev = NULL;
> > >   
> > > +	if (!gpu_bind || (gpu_bind < 0 && video_firmware_drivers_only()))
> > > +		return false;
>
>
> Bear with me since I am just going back to work mode but I can't figure
> out what the second part of the test does
>
> bool video_firmware_drivers_only(void)
> {
> 	return video_nomodeset;
> }
> EXPORT_SYMBOL(video_firmware_drivers_only);
>
> and video_nomodeset=1 by default, so why would we return false when
> gpu_bind = -1?
>
> This seems to be a change of functionality, what am I missing?
video_nomodeset is 0 by default, only when nomodeset is given as 
argument is it set to 1. :-)

Cheers,
~Maarten