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@xxxxxxxxxx>
> Reviewed-by: Aaron Plattner <aplattner@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
