Re: [alsa-devel] [PATCH v1 2/5] ALSA: hda - Add DP-MST jack support

[Nikhil Mahale <nmahale@nvidia.com>]

On 11/14/19 4:24 PM, Takashi Iwai wrote:
> On Thu, 14 Nov 2019 04:37:01 +0100,
> Nikhil Mahale wrote:
>>
>> This patch adds DP-MST jack support which will be used on NVIDIA
>> platforms. Today, DP-MST audio is supported only if the codec has
>> acomp support. This patch makes it possible to add DP-MST support
>> for non-acomp codecs.
>>
>> For the codecs supporting DP-MST audio, each pin can contain several
>> device entries. Each device entry is a virtual pin, described by
>> pin_nid and dev_id in struct hdmi_spec_per_pin. For monitor hotplug
>> event handling, non-acomp codecs enable and register jack-detection
>> for every hdmi_spec_per_pin.
>>
>> This patch updates every relevant function in hda_jack.h and its
>> implementation in hda_jack.c, to consider dev_id along with pin_nid.
>>
>> Changes to the HD Audio specification to support DP-MST audio are
>> described in the Intel Document Change Notification (DCN) number
>> HDA040-A.
>>
>> >From HDA040-A, "For the case of multi stream capable Digital Display
>> Pin Widget, [the Get Pin Sense verb] can be used to read a specific
>> Device Entry state as reported in Get Device List Entry verb." This
>> patch updates the read_pin_sense() function to take the dev_id as an
>> argument and pass it as a parameter to the Get Pin Sense verb.
>>
>> Bits 15 through 20 from the Unsolicited Response for intrinsic
>> events contain the index of the Device Entry that generated the
>> event. This patch updates the Unsolicited Response event handlers to
>> extract the device entry index from the response and pass it to
>> snd_hda_jack_tbl_get_from_tag().
>>
>> This patch updates snd_hda_jack_tbl_new() to take a dev_id argument
>> and store it in the jack structure, and to make sure not to generate
>> a different tag when called more than once for the same nid.
>>
>> Signed-off-by: Nikhil Mahale <nmahale@nvidia.com>
>> Reviewed-by: Aaron Plattner <aplattner@nvidia.com>
>> ---
>>  sound/pci/hda/hda_generic.c    |  16 +++---
>>  sound/pci/hda/hda_jack.c       | 107 +++++++++++++++++++++++++++++------------
>>  sound/pci/hda/hda_jack.h       |  26 ++++++----
>>  sound/pci/hda/patch_ca0132.c   |  24 ++++-----
>>  sound/pci/hda/patch_cirrus.c   |   4 +-
>>  sound/pci/hda/patch_conexant.c |   2 +-
>>  sound/pci/hda/patch_hdmi.c     |  47 +++++++++---------
>>  sound/pci/hda/patch_realtek.c  |  46 +++++++++---------
>>  sound/pci/hda/patch_sigmatel.c |  12 ++---
> 
> So this patch touches quite wide range of code just for passing the
> additional 0.  I prefer keeping the old (non-MST) functions as is,
> while adding a couple of new mst-capable jack function, e.g.
> 
> snd_hda_jack_tbl_get_mst(codec, nid, dev_nid);
> snd_hda_jack_detect_enable_mst(codec, nid, dev_nid, callback);
> 
> etc.  snd_hda_jack_detect_eanble() and *_callback() can be unified for
> MST variant, as it's called only from HDMI codec, and also
> *_get_from_tag() can be extended as it's called only from hda_jack.c
> and patch_hdmi.c.  That is, keep the functions that are accessed
> outside hda_jack.c and patch_hdmi.c should be kept, while adding a few
> for handling dev_id.
> 
> A few more nitpicks:
> 
>> --- a/sound/pci/hda/hda_jack.c
>> +++ b/sound/pci/hda/hda_jack.c
>> @@ -55,7 +55,7 @@ static u32 read_pin_sense(struct hda_codec *codec, hda_nid_t nid)
>>  					AC_VERB_SET_PIN_SENSE, 0);
> 
> Don't we need to pass dev_id for PIN_SENSE verb, too?

As per specification, No. I am referring to section 7.3.3.15 from https://www.intel.com/content/dam/www/public/us/en/documents/specification-updates/high-definition-audio-multi-stream.pdf.

Thanks,
Nikhil Mahale

>>  	}
>>  	val = snd_hda_codec_read(codec, nid, 0,
>> -				  AC_VERB_GET_PIN_SENSE, 0);
>> +				  AC_VERB_GET_PIN_SENSE, dev_id);
> 
> 
>> --- a/sound/pci/hda/patch_hdmi.c
>> +++ b/sound/pci/hda/patch_hdmi.c
>> @@ -784,24 +783,18 @@ static void hdmi_intrinsic_event(struct hda_codec *codec, unsigned int res)
>>  	struct hda_jack_tbl *jack;
>>  	int dev_entry = (res & AC_UNSOL_RES_DE) >> AC_UNSOL_RES_DE_SHIFT;
>>  
>> -	/*
>> -	 * assume DP MST uses dyn_pcm_assign and acomp and
>> -	 * never comes here
>> -	 * if DP MST supports unsol event, below code need
>> -	 * consider dev_entry
>> -	 */
>> -	jack = snd_hda_jack_tbl_get_from_tag(codec, tag);
>> +	jack = snd_hda_jack_tbl_get_from_tag(codec, tag, dev_entry);
> 
> Passing dev_entry unconditionally might be broken on old HDMI codecs.
> Pass 0 if codec->dp_mst is false.
> 
> 
>>  	if (!jack)
>>  		return;
>>  	jack->jack_dirty = 1;
>>  
>>  	codec_dbg(codec,
>>  		"HDMI hot plug event: Codec=%d Pin=%d Device=%d Inactive=%d Presence_Detect=%d ELD_Valid=%d\n",
>> -		codec->addr, jack->nid, dev_entry, !!(res & AC_UNSOL_RES_IA),
>> +		codec->addr, jack->nid, jack->dev_id, !!(res & AC_UNSOL_RES_IA),
>>  		!!(res & AC_UNSOL_RES_PD), !!(res & AC_UNSOL_RES_ELDV));
>>  
>>  	/* hda_jack don't support DP MST */
>> -	check_presence_and_report(codec, jack->nid, 0);
>> +	check_presence_and_report(codec, jack->nid, jack->dev_id);
> 
> This comment is invalid.
> 
>> @@ -831,11 +824,12 @@ static void hdmi_unsol_event(struct hda_codec *codec, unsigned int res)
>>  {
>>  	int tag = res >> AC_UNSOL_RES_TAG_SHIFT;
>>  	int subtag = (res & AC_UNSOL_RES_SUBTAG) >> AC_UNSOL_RES_SUBTAG_SHIFT;
>> +	int dev_entry = (res & AC_UNSOL_RES_DE) >> AC_UNSOL_RES_DE_SHIFT;
> 
> Ditto, let's pass 0 for !codec->dp_mst.
> 
> 
> thanks,
> 
> Takashi
> 

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel