[alsa-devel] [RFC PATCH 0/2] ALSA: hda - DP MST audio for Jack support

[Takashi Iwai]

On Tue, 22 Dec 2015 12:45:25 +0100,
Yang, Libin wrote:
> 
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai at suse.de]
> > Sent: Tuesday, December 22, 2015 5:00 PM
> > To: Libin Yang
> > Cc: Yang, Libin; mengdong.lin at linux.intel.com; alsa-devel at alsa-
> > project.org
> > Subject: Re: [alsa-devel] [RFC PATCH 0/2] ALSA: hda - DP MST audio for
> > Jack support
> > 
> > On Tue, 22 Dec 2015 09:47:12 +0100,
> > Libin Yang wrote:
> > >
> > > Hi Takashi,
> > >
> > > On 11/09/2015 11:15 PM, Takashi Iwai wrote:
> > > > On Mon, 09 Nov 2015 16:00:01 +0100,
> > > > Yang, Libin wrote:
> > > >>
> > > >> Hi Takashi,
> > > >>
> > > >>> -----Original Message-----
> > > >>> From: Takashi Iwai [mailto:tiwai at suse.de]
> > > >>> Sent: Monday, November 09, 2015 3:59 PM
> > > >>> To: Libin Yang
> > > >>> Cc: alsa-devel at alsa-project.org; mengdong.lin at linux.intel.com;
> > Yang,
> > > >>> Libin
> > > >>> Subject: Re: [alsa-devel] [RFC PATCH 0/2] ALSA: hda - DP MST
> > audio for
> > > >>> Jack support
> > > >>>
> > > >>> On Wed, 04 Nov 2015 07:23:14 +0100,
> > > >>> Libin Yang wrote:
> > > >>>>
> > > >>>>
> > > >>>> On 11/04/2015 12:49 AM, Takashi Iwai wrote:
> > > >>>>> On Tue, 03 Nov 2015 09:42:54 +0100,
> > > >>>>> libin.yang at linux.intel.com wrote:
> > > >>>>>>
> > > >>>>>> From: Libin Yang <libin.yang at linux.intel.com>
> > > >>>>>>
> > > >>>>>> This is the patch set for Jack support for DP MST audio
> > > >>>>>>
> > > >>>>>> Libin Yang (2):
> > > >>>>>>     ALSA: hda - jack support DP MST audio
> > > >>>>>>     ALSA: hda - hdmi audio add dynamically jack binding to pin
> > > >>>>>
> > > >>>>> I'm somehow confused by this patch series.
> > > >>>>> Is the unsolicited event handling itself changed for MST?  I mean,
> > you
> > > >>>>> cannot know which MST dev# is given beforehand, while you
> > seem
> > > >>> trying
> > > >>>>> to assign such a number to the jack object.
> > > >>>>
> > > >>>> Oh, sorry I forgot to mention that we only operate on device
> > entry 0
> > > >>>> so far in the code. It's misleading.
> > > >>>>
> > > >>>> These patches don't operate on MST audio. The full MST audio
> > driver
> > > >>>> support will be implemented in the later patches. The patches help
> > to
> > > >>>> prepare supporting MST audio.
> > > >>>>
> > > >>>> The purpose of the patches is try to dynamically attach the Jack.
> > > >>>
> > > >>> Well, only judging from the quick glance, I don't buy this strategy,
> > > >>> sorry.  Currently for HDMI/DP, the jack is for PCM, not for pin.
> > > >>> Namely, user-space watches the jack notification to judge whether
> > the
> > > >>> corresponding PCM is available or not.  You're trying to change this
> > > >>> scheme completely.
> > > >>
> > > >> The patch is to make sure the jack is bound to PCM statically.
> > > >> This patch is needed because in old code,  it will create jack with:
> > > >> for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
> > > >> 	...
> > > >> 	err = generic_hdmi_build_jack(codec, pin_idx);
> > > >> 	...
> > > >> }
> > > >> Which means the jack is created based on pin number and pin_idx.
> > > >> As a pin is bound statically to the PCM, the jack is statically bound to
> > PCM.
> > > >
> > > > OK, so far, so good.
> > > >
> > > >> Now we are using the dynamic pcm bound. If we use the old code to
> > > >> create the jack, the jack will be bound to pin. So we need the patch,
> > > >> which will use the new code:
> > > >> for (pcm_idx = 0; pcm_idx < spec-> pcms_used; pcm_idx++) {
> > > >> 	...
> > > >> 	err = generic_hdmi_build_jack(codec, pcm_idx);
> > > >> 	...
> > > >> }
> > > >> which makes sure that the jack is bound to PCM (jack[0] is bound
> > > >> to pcm[0], jack[1] is bound to pcm[1] and so on).
> > > >>
> > > >> If there is no monitor, no pin is bound to the PCM.
> > > >> So no pin is bound to the Jack. When there is a monitor connected,
> > > >> the pin is bound to a proper PCM. So we should bind the PCM's jack
> > > >> to the pin.
> > > >>
> > > >> For example, jack0 is bound to the PCM 3 (the first jack is bound
> > > >> to the first PCM statically). When a monitor is connected to pin 5,
> > > >> PCM 3 will be assigned to the pin. We need bind jack 0 to pin 5.
> > > >> When monitor is disconnected, we must unbind the jack 0 from pin
> > 5.
> > > >> These operation are done in function
> > > >> snd_hda_jack_bind/ snd_hda_jack_unbind.
> > > >
> > > > Then we shouldn't use the hda_jack.c code at all for HDMI/DP.
> > > > For HDMI/DP with the dynamic PCM binding, the only state to check
> > is
> > > > its PCM assignment.  It doesn't have to issue the jack state check via
> > > > HDA verb at all.  This should make things a lot easier.
> > >
> > > I'm now porting the jack supporting for MST audio. The code on
> > > hdmi-jack branch will create jack differently based codec_has_acomp()
> > > condition. Does this mean MST Jack binding will handle both situation?
> > 
> > Ideally, yes.
> > 
> > > My idea is:
> > > 1. Add struct snd_jack *jack[] member in hdmi_spec.
> > > 2. Create the jack based on PCM
> > > 3. Assign spec->jack[i] to per_pin->acomp_jack when pin is connected.
> > >
> > > This should be OK for codec_has_acomp sitation.
> > 
> > Hrm, not really.  As already discussed, a jack is associated to each
> > PCM, and notified plug/unplug when it's assigned / unassigned.
> > That is, you don't have to keep extra jack array.  When a PCM is
> > assigned or unassigned, simply notify per_pin->acomp_jack of that PCM.
> > That's all.  So there shouldn't be too many difference from the
> > current situation.
> 
> If so, the snd_jack is dynamically bound to PCM. This means
> the jack is bound to the pin, and user space must get the pcm-pin
> mapping to know the jack is bound to which PCM. Then it can decide
> how to operate on the PCM. Not sure it is compatible with current
> pulseaudio. 

Erm, I was confused.  Yes, for dynamic attach/detaching PCM, we need
to keep jack object apart from per_pin.  I guess your current patchset
is broken in this regard?

Though, for having the jack pointer, I prefer avoiding to introduce
yet another array but rather extending hdmi_spec.pcm_rec[].  Currently
it contains the hda_pcm pointers.  But we can define a new object,
e.g.

struct hdmi_pcm {
	struct hda_pcm *pcm;
	struct snd_jack *acomp_jack;
	....
};

and keeping this in hdmi_spec.

struct hdmi_spec {
	....
	struct hdmi_pcm pcm_rec[16];
	....
};

get_pcm_rec() macro needs to be adjusted properly, but other than
that, there aren't so many places accessing pcm_rec[] directly, so
easy to convert.

Maybe we can start from converting this at first, moving acomp_jack to
hdmi_spec, then apply MST patchset on its top.  This might be a bit
cleaner.

In the case of non-component: the question beforehand is how to handle
MST PCM assignment at all.  Namely, the non-component case assumes
that there is 1:1 object matching with the actual pin to per_pin.  If
this assumption isn't kept, we'd need to use the same trick like the
component case, and stop using hda_jack infrastructure.
(So acomp_jack might be better renamed to just "jack" or so.)


thanks,

Takashi