[alsa-devel] HDMI codec, way forward?

[Takashi Iwai]

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.


thanks,

Takashi

> 
> The whole point of the component helper is that you don't care when the
> slaves come along.  The above code does care about that, because it
> expects that the master and slaves will be bound either inside
> component_master_add_with_match(), or via that request_module (which
> may happen asynchronously) and ends up setting ->ops.
> 
> This won't work for systems in the presence of deferred probing, since
> there's only a weak connection between loading a module and the driver
> successfully probing its device.
> 
> If you arrange for the audio device to finish probing in the master's
> bind callback, that would be _far_ better, and would be in line with
> how the component helper is supposed to work.  It also means that if
> your slave (the video side) goes away, you'll know about it as the
> unbind callback will be called (at which point you should tear down
> the audio device.)
> 
> However, there's a lurking problem: you can't register the same struct
> device as a slave more than once into the component helpers - that's
> because it's designed to look for devices.  There's intentionally no
> linkage between the slave ops and master ops to allow for generic
> slave drivers (like tda998x) to be used with different master drivers
> (armada, tilcdc, etc).  In theory, you can register the master device
> of one componentised system as a slave device of another, but that
> requires a much more complicated locking setup in component.c (I have
> patches for that, but I've resisted sending them because it makes the
> locking very messy.)
> 
> -- 
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.
>