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

Tue May 17 12:37:49 UTC 2016

On Tue, May 17, 2016 at 02:34:21PM +0200, Daniel Vetter wrote:
> On Tue, May 17, 2016 at 03:11:02PM +0300, Imre Deak wrote:
> > 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
> 
> I disagree with Takashi here. modeset=0 is a debug option, and we should
> probably mark it as _unsafe.
> > 
> > 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.
> 
> nomodeset isn't supported really anymore in upstream, we've thrown out the
> Kconfig option needed by distros for that:
> 
> commit fd930478fb797e4cbaa799d9ddd970e9a1fa1b4a
> Author: Chris Wilson <chris at chris-wilson.co.uk>
> Date:   Fri Jun 19 20:27:27 2015 +0100
> 
>     drm/i915: Remove KMS Kconfig option
> 
> That was almost a year ago, and no one complained. I don't think modeset=0
> is a use-case any more, and hence I don't think we need special code to
> support it. If some users scream that we broke their setup we can fix this
> later on, but right now I'm firmly in the "nope" territory.

If audio without i915.ko is really a use-case we care about then I guess
we could make it all work if you disable CONFIG_DRM_I915. So #ifdef around
the corresponding code in patch 3. That way the complexity is in the
snd-hda driver, and sound folks can deal with it if they want this. But
adding fallback code for every totally insane combination of config
options a users might come up with is not something I like at all. At
least not in i915.ko. We have the same discussion with lack of firmware
blobs, fbc, psr, everything else. It's impossible to test all those paths
and keep them working.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch