[Intel-gfx] [PATCH i-g-t] lib: Poll for snd_hda_intel discovery

[Imre Deak]

On Fri, Aug 17, 2018 at 10:48:29AM +0100, Chris Wilson wrote:
> Quoting Imre Deak (2018-08-17 10:38:01)
> > On Fri, Aug 17, 2018 at 10:24:30AM +0100, Chris Wilson wrote:
> > > Quoting Imre Deak (2018-08-17 10:14:51)
> > > > On Thu, Aug 16, 2018 at 07:01:36PM +0100, Chris Wilson wrote:
> > > > > Loading the sounds modules is asynchronous with the sysfs device
> > > > > hierarchy being instantiated sometime after modprobe returns. As such
> > > > > while we are probing for the sound device, poll a few times to
> > > > > accommodate the async discovery.
> > > > > 
> > > > > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > > > > Cc: Imre Deak <imre.deak at intel.com>
> > > > > Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> > > > 
> > > > > ---
> > > > >  lib/igt_pm.c | 65 +++++++++++++++++++++++++++++++++-------------------
> > > > >  1 file changed, 41 insertions(+), 24 deletions(-)
> > > > > 
> > > > > diff --git a/lib/igt_pm.c b/lib/igt_pm.c
> > > > > index 6e96db22b..f82707231 100644
> > > > > --- a/lib/igt_pm.c
> > > > > +++ b/lib/igt_pm.c
> > > > > @@ -138,31 +138,17 @@ static void strchomp(char *str)
> > > > >               str[len - 1] = 0;
> > > > >  }
> > > > >  
> > > > > -/**
> > > > > - * igt_pm_enable_audio_runtime_pm:
> > > > > - *
> > > > > - * We know that if we don't enable audio runtime PM, snd_hda_intel will never
> > > > > - * release its power well refcount, and we'll never reach the LPSP state.
> > > > > - * There's no guarantee that it will release the power well if we enable
> > > > > - * runtime PM, but at least we can try.
> > > > > - *
> > > > > - * We don't have any assertions on open since the user may not even have
> > > > > - * snd_hda_intel loaded, which is not a problem.
> > > > > - */
> > > > > -void igt_pm_enable_audio_runtime_pm(void)
> > > > > +static int __igt_pm_enable_audio_runtime_pm(void)
> > > > >  {
> > > > >       char *path = NULL;
> > > > >       struct dirent *de;
> > > > >       DIR *dir;
> > > > > +     int err;
> > > > >       int fd;
> > > > >  
> > > > > -     /* Check if already enabled. */
> > > > > -     if (__igt_pm_audio_runtime_power_save[0])
> > > > > -             return;
> > > > > -
> > > > >       dir = opendir("/sys/class/sound");
> > > > >       if (!dir)
> > > > > -             return;
> > > > > +             return 0;
> > > > >  
> > > > >       /* Find PCI device claimed by snd_hda_intel and tied to i915. */
> > > > >       while ((de = readdir(dir))) {
> > > > > @@ -196,18 +182,17 @@ void igt_pm_enable_audio_runtime_pm(void)
> > > > >                                   de->d_name));
> > > > >  
> > > > >               igt_debug("Audio device path is %s\n", path);
> > > > > -
> > > > >               break;
> > > > >       }
> > > > 
> > > > Could close dir while at it.
> > > 
> > > Good point.
> > >  
> > > > Reviewed-by: Imre Deak <imre.deak at intel.com>
> > > > 
> > > > This time the catch was that snd_hda_intel's second half of probe is run
> > > > from a scheduled work. (even though the first half returns synchronously
> > > > due to modprobe being synchronous as you found last time)
> > > 
> > > The results from the CI didn't look too promising, but I'm a bit dubious
> > > as to what exactly was tested and so pushed anyway ;)
> > > 
> > > What next if it still doesn't discover an audio device?
> > 
> > We need to register the i915 audio component (in its current form or
> > just a stub version) even with disable_display=1, so that the audio
> > driver can continue its own probing (before timing out on the 10sec
> > wait for the audio component). Atm the
> > 
> > if (INTEL_INFO(dev_priv)->num_pipes == 0)
> >         return;
> > 
> > in i915_audio_component_init() prevents the registration.
> 
> Right, that's the other question: do we need that at all...
>  
> > > Hmm, is the /sys/class/sound construction async? I was assuming it was
> > > constructed by the parent snd module long before sna_hda_intel does it
> > > thing.
> > 
> > AFAICS, azx_probe_continue() is the scheduled work and it will do
> > snd_card_register()->device_add() which will add the sysfs entries.
> 
> As far as the probe we do here, the key question is whether
> opendir("/sys/class/sound") will succeed after modprobe but before the
> devices have been constructed. We using that as a predicate as to
> whether there are any sound modules to wait for.

Ok, /sys/class/sound appears immediately after modprobe, I meant
/sys/class/sound/hwC* which appears only after component binding. In any
case both dirs are polled for after your igt patch so that part is ok
now imo.

> -Chris