Re: [PATCH v2 1/3] drm/i915/hda: Add audio component stub

[Takashi Iwai <tiwai@suse.de>]

On Tue, 17 May 2016 14:39:26 +0200,
Daniel Vetter wrote:
> 
> On Tue, May 17, 2016 at 02:37:49PM +0200, Daniel Vetter wrote:
> > 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@linux.intel.com>
> > > > > > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > > > > > > > Signed-off-by: Takashi Iwai <tiwai@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@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.
> 
> And if we _really_ want this to work at runtime too, then we need a
> system-wide nomodeset option that snd-hda can look up and act accordingly.

Yes, this would be one option.


Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx