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

[Takashi Iwai <tiwai@suse.de>]

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?

>  	}
>  	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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel