[alsa-devel] HDMI codec, way forward?

[Takashi Iwai]

On Wed, 21 Oct 2015 16:03:07 +0200,
Russell King - ARM Linux wrote:
> 
> On Wed, Oct 21, 2015 at 03:41:44PM +0200, Takashi Iwai wrote:
> > On Wed, 21 Oct 2015 11:27:44 +0200,
> > Russell King - ARM Linux wrote:
> > > 
> > > On Tue, Oct 20, 2015 at 07:31:48PM +0530, Vinod Koul wrote:
> > > > On Tue, Oct 20, 2015 at 09:08:00AM +0100, Russell King - ARM Linux wrote:
> > > > > > > Currently i915/audio component works as you described.  The audio is
> > > > > > > optional and HDMI graphics works without audio, while HDMI HD-audio
> > > > > > > mandates i915 graphics.
> > > > > > 
> > > > > > Right, but we also add additional interface on top of this to allow
> > > > > > things like ensuring display is on when audio wants to run and now
> > > > > > notification for events.
> > > > > > 
> > > > > > I don't see a reason why this can be used as single interface to bind as
> > > > > > well as talk to display from various components, unless I missed obvious
> > > > > > which prevent us from doing this in non i915 cases...
> > > > > 
> > > > > Maybe I can comment more specifically if I saw the code.  Right now all
> > > > > I'm aware of is this idea without any code, and I don't like it.
> > > > 
> > > > Ok, i will be post my patches tomorrow. FWIW uses interface in
> > > > sound/hda/hdac_i915.c for display power up/down
> > > 
> > > This:
> > > 
> > >         component_match_add(dev, &match, hdac_component_master_match, bus);
> > >         ret = component_master_add_with_match(dev, &hdac_component_master_ops,
> > >                                               match);
> > >         if (ret < 0)
> > >                 goto out_err;
> > > 
> > >         /*
> > >          * Atm, we don't support deferring the component binding, so make sure
> > >          * i915 is loaded and that the binding successfully completes.
> > >          */
> > >         request_module("i915");
> > > 
> > >         if (!acomp->ops) {
> > >                 ret = -ENODEV;
> > >                 goto out_master_del;
> > >         }
> > > 
> > > is really not nice.
> > 
> > Yeah, admittedly it's a really ugly workaround.  It's coded in that
> > way just to make our lives easier: it works well in most cases for
> > i915 / hd-audio.
> > 
> > Actually, it's easy to modify the hda code in the right way, i.e. to
> > defer via component bind ops.  However, one drawback is that it's not
> > trivial to handle the fallback case; namely, HD-audio might not need
> > i915 binding, depending on the chip model and the configuration.
> > 
> > Braswell and Skylake have a (virtually) single audio controller as
> > default, and this manages both the analog and HDMI/DP codecs.  The
> > i915 interaction is required only for the latter codecs, and thus for
> > the former, it's optional.  If we defer binding, the instantiation
> > won't happen unless it's bound with i915.  So, if i915 isn't
> > initialized (e.g. by booting with nomodeset option), the whole audio
> > will be gone, too.  OTOH, Haswell and Braswell have a dedicated HDA
> > controller for HDMI/DP, and they must be disabled (or fail) unless
> > bound with graphics.
> > 
> > It's a corner case, so we might better ignore this.  But it'd be
> > certainly a "regression" -- at least, I used to test this in that way
> > sometimes in the past.  So it remains in the current way as is.
> > 
> > It's one of the reasons I mentioned it being a stop gap.  But, I think
> > the concept, passing ops via component, is actually working, and
> > should be applicable to other drm/audio drivers.  That's the point.
> 
> It's only the point if you can code it up properly, which from what I
> read in that file, it isn't.

An idea can fly without coding, too :)

> Build the i915 DRM drivers as a module, load up the system, and then
> try removing the i915 DRM module and see what happens to the audio part.
> For starters, you have no protection what so ever against acomp->ops or
> acomp->dev becoming NULL - it's hellishly racy.

Yes, very likely.

> Secondly, you reject the initialisation if acomp->ops isn't set, but you
> allow acomp->ops to later become unset by the i915 DRM module being
> removed.  If you can cope with acomp->ops being unset at a random point
> during the audio driver's use, why can't you cope with it being set at
> some random point later?

Setting/unsetting on the fly would be picky because the code does
refcounting.  Maybe an easier option is to inc/dec module usage
count appropriately in use.

> I don't think this code has been thought through at all.

True, more hardening needed.


thanks,

Takashi