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

[Daniel Vetter <daniel@ffwll.ch>]

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.
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.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx