[alsa-devel] HDMI codec, way forward?

[Takashi Iwai]

On Wed, 21 Oct 2015 16:10:31 +0200,
Takashi Iwai wrote:
> 
> 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.

... and actually we pin at master bind:

static int hdac_component_master_bind(struct device *dev)
{
.....
	/*
	 * Atm, we don't support dynamic unbinding initiated by the child
	 * component, so pin its containing module until we unbind.
	 */
	if (!try_module_get(acomp->ops->owner)) {

so unloading i915 module won't happen (but other method for unbinding
still possible).


Takashi