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

[Imre Deak <imre.deak@intel.com>]

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.

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