[Intel-gfx] [PATCH v2 1/3] drm/i915/hda: Add audio component stub

Tue May 17 12:11:02 UTC 2016

On ti, 2016-05-17 at 13:10 +0200, Daniel Vetter wrote:
> On Tue, May 17, 2016 at 01:22:52PM +0300, Imre Deak wrote:
> > On ti, 2016-05-17 at 11:59 +0200, Takashi Iwai wrote:
> > > On Tue, 17 May 2016 11:42:17 +0200,
> > > Daniel Vetter wrote:
> > > > 
> > > > On Tue, May 17, 2016 at 09:37:12AM +0200, Takashi Iwai wrote:
> > > > > On Tue, 17 May 2016 09:20:48 +0200,
> > > > > Daniel Vetter wrote:
> > > > > > 
> > > > > > On Thu, May 12, 2016 at 09:06:53PM +0300, Imre Deak wrote:
> > > > > > > User may pass nomodeset or i915.modeset=0 option to
> > > > > > > disable i915 KMS
> > > > > > > explicitly.  Although this itself works fine, it breaks
> > > > > > > the weak
> > > > > > > dependency the HD-audio driver requires, and it's the
> > > > > > > reason the
> > > > > > > delayed component binding isn't implemented in HD-
> > > > > > > audio.  Since i915
> > > > > > > doesn't notify its disablement, HD-audio would be blocked
> > > > > > > unnecessarily endlessly, waiting for the bind with i915.
> > > > > > > 
> > > > > > > This patch introduces a stub audio component binding when
> > > > > > > i915 driver
> > > > > > > is loaded with KMS off like the boot options above.  Then
> > > > > > > i915 driver
> > > > > > > still registers the slave component but with the new
> > > > > > > "disabled" ops
> > > > > > > flag, so that the master component (HD-audio) can know
> > > > > > > clearly the
> > > > > > > slave state.
> > > > > > > 
> > > > > > > v2:
> > > > > > > - Fail the probe in case component registration fails,
> > > > > > > instead of
> > > > > > >   suppressing the error. (Ville)
> > > > > > > - Register the component only for the real PCI device
> > > > > > > function.
> > > > > > > 
> > > > > > > CC: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > > > > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > > > > > > Signed-off-by: Takashi Iwai <tiwai at suse.de>
> > > > > > 
> > > > > > We don't support not running with modesetting. Why do we
> > > > > > suddenly care?
> > > > > 
> > > > > This is needed for the patch 2 and 3.  Right now we have no
> > > > > blocking
> > > > > or deferred component binding, so far, in HD-audio
> > > > > side.  This caused
> > > > > problems when async module probe was done.
> > > > > 
> > > > > So, the patch 3 implements the blocking behavior of HD-audio
> > > > > side.  It
> > > > > would lead to another regression when i915 doesn't notify its
> > > > > disabled
> > > > > state by this patch.  Otherwise the HD-audio driver will be
> > > > > blocked
> > > > > endlessly of unnecessarily long.
> > > > > 
> > > > > > Same for users creating a .config that fails to boot or
> > > > > > work ...
> > > > > 
> > > > > The config isn't cared much, but the problem is about the
> > > > > runtime boot
> > > > > option.
> > > > > 
> > > > > > If HDA needs to coporate with gfx to get things done, then
> > > > > > imo we should
> > > > > > just require that i915.ko is there.
> > > > > 
> > > > > Other way round: we do already require i915 in HD-audio
> > > > > side.  But in
> > > > > this case, we do *not* want to require i915 when it's
> > > > > disabled in
> > > > > runtime. 
> > > > 
> > > > That's what I mean: If you boot with i915.nomodeset you're
> > > > explicitly fine
> > > > with a somewhat non-useable system - that option is for
> > > > debugging only
> > > > really. If that means audio also doesn't work, then I think
> > > > that's ok.
> > > 
> > > It's not only "it doesn't work".  The module load gets stuck.  So
> > > we
> > > need some notification for the blocked component binding.
> > > 
> > > > Adding complexity for this case (which means more error paths
> > > > we don't
> > > > ever test and hence _will_ break) seems over the top.
> > > > 
> > > > I'm quite opposed to adding error handling for every condition
> > > > in general
> > > > because the combinatorial testing madness just can't be
> > > > handled. The one
> > > > exception in the i915.ko driver is that when the render side
> > > > died
> > > > (terminal gpu hang) we'll try our best to keep the display
> > > > alive. But
> > > > that's it, and the justification for that is "we want users to
> > > > be able to
> > > > get the bug report out". I don't see a justification of that
> > > > magnitude for
> > > > this feature here at all.
> > > 
> > > Well, actually the patchset was proposed just because Intel CI
> > > tests
> > > failed due to async module probes.  If Intel is happy with
> > > continued
> > > CI test failures, I'm also happy with the current situation ;)
> > 
> > It's not only due to those particular failures. That was caused by
> > a
> > kmod bug and as such would be good to not depend on that mechanism.
> > But
> > things will fail atm even in the normal case when audio is built-in 
> > and
> > i915 is a module. This patchset would solve that too.
> 
> I'm not against patch 3 from this series, which seems to be the
> bugfix we
> want. I'm against the kludge in patch 1 here only (and maybe patch 2,
> not
> sure about that). Patch 1 here looks like a workaround purely for the
> i915.modeset=0 case. And I don't want special code for debug knobs if
> we
> can avoid it.

We have discussed this in more detail, see:
https://bugs.freedesktop.org/show_bug.cgi?id=94566#c26

The point from Takashi in that discussion was that in order to do the
probing via the component bind hook as we want and as patch 3 does we
need to make sure basic audio functionality works even with the
nomodeset option. For that you need the kludge in patch 1. I think of
this as a similar kludge we have for not failing modprobing i915 in
case of nomodeset. Legacy userspace depends on this for basic graphics
functionality and in the same way some other userspace bits may depend
on basic audio functionality.

--Imre