[alsa-devel] [PATCH] ALSA: hda - Fix DP-MST support for NVIDIA codecs

[alsa-devel] [PATCH] ALSA: hda - Fix DP-MST support for NVIDIA codecs

[Nikhil Mahale]

If dyn_pcm_assign is set, different jack objects are being created
for pcm and pins.

If dyn_pcm_assign is set, generic_hdmi_build_jack() calls into
add_hdmi_jack_kctl() to create and track separate jack object for
pcm. Like sync_eld_via_acomp(), hdmi_present_sense_via_verbs() also
need to report status change of the pcm jack.

Rename pin_idx_to_jack() to pin_idx_to_pcm_jack(). The code to
report status change of pcm jack, move it to update_eld() which is
common for acomp and !acomp code paths.

Fixes: 5398e94fb753 ALSA: hda - Add DP-MST support for NVIDIA codecs
Signed-off-by: Nikhil Mahale <nmahale@nvidia.com>
---
 sound/pci/hda/patch_hdmi.c | 87 ++++++++++++++++++++++++----------------------
 1 file changed, 45 insertions(+), 42 deletions(-)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 48bddc218829..7b5360037217 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -1480,6 +1480,35 @@ static void hdmi_pcm_reset_pin(struct hdmi_spec *spec,
 	per_pin->channels = 0;
 }
 
+static struct snd_jack *pin_idx_to_pcm_jack(struct hda_codec *codec,
+					    struct hdmi_spec_per_pin *per_pin)
+{
+	struct hdmi_spec *spec = codec->spec;
+	struct snd_jack *jack = NULL;
+	struct hda_jack_tbl *jack_tbl;
+
+	/* if !dyn_pcm_assign, get jack from hda_jack_tbl
+	 * in !dyn_pcm_assign case, spec->pcm_rec[].jack is not
+	 * NULL even after snd_hda_jack_tbl_clear() is called to
+	 * free snd_jack. This may cause access invalid memory
+	 * when calling snd_jack_report
+	 */
+	if (per_pin->pcm_idx >= 0 && spec->dyn_pcm_assign) {
+		jack = spec->pcm_rec[per_pin->pcm_idx].jack;
+	} else if (!spec->dyn_pcm_assign) {
+		/*
+		 * jack tbl doesn't support DP MST
+		 * DP MST will use dyn_pcm_assign,
+		 * so DP MST will never come here
+		 */
+		jack_tbl = snd_hda_jack_tbl_get_mst(codec, per_pin->pin_nid,
+						    per_pin->dev_id);
+		if (jack_tbl)
+			jack = jack_tbl->jack;
+	}
+	return jack;
+}
+
 /* update per_pin ELD from the given new ELD;
  * setup info frame and notification accordingly
  */
@@ -1490,9 +1519,15 @@ static bool update_eld(struct hda_codec *codec,
 	struct hdmi_eld *pin_eld = &per_pin->sink_eld;
 	struct hdmi_spec *spec = codec->spec;
 	bool old_eld_valid = pin_eld->eld_valid;
+	struct snd_jack *pcm_jack;
 	bool eld_changed;
 	int pcm_idx;
 
+	/* pcm_idx >=0 before update_eld() means it is in monitor
+	 * disconnected event. Jack must be fetched before update_eld()
+	 */
+	pcm_jack = pin_idx_to_pcm_jack(codec, per_pin);
+
 	/* for monitor disconnection, save pcm_idx firstly */
 	pcm_idx = per_pin->pcm_idx;
 	if (spec->dyn_pcm_assign) {
@@ -1547,6 +1582,14 @@ static bool update_eld(struct hda_codec *codec,
 			       SNDRV_CTL_EVENT_MASK_VALUE |
 			       SNDRV_CTL_EVENT_MASK_INFO,
 			       &get_hdmi_pcm(spec, pcm_idx)->eld_ctl->id);
+
+	if (!pcm_jack)
+		pcm_jack = pin_idx_to_pcm_jack(codec, per_pin);
+	if (eld_changed && pcm_jack)
+		snd_jack_report(pcm_jack,
+				(eld->monitor_present && eld->eld_valid) ?
+				SND_JACK_AVOUT : 0);
+
 	return eld_changed;
 }
 
@@ -1615,43 +1658,12 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
 	return ret;
 }
 
-static struct snd_jack *pin_idx_to_jack(struct hda_codec *codec,
-				 struct hdmi_spec_per_pin *per_pin)
-{
-	struct hdmi_spec *spec = codec->spec;
-	struct snd_jack *jack = NULL;
-	struct hda_jack_tbl *jack_tbl;
-
-	/* if !dyn_pcm_assign, get jack from hda_jack_tbl
-	 * in !dyn_pcm_assign case, spec->pcm_rec[].jack is not
-	 * NULL even after snd_hda_jack_tbl_clear() is called to
-	 * free snd_jack. This may cause access invalid memory
-	 * when calling snd_jack_report
-	 */
-	if (per_pin->pcm_idx >= 0 && spec->dyn_pcm_assign)
-		jack = spec->pcm_rec[per_pin->pcm_idx].jack;
-	else if (!spec->dyn_pcm_assign) {
-		/*
-		 * jack tbl doesn't support DP MST
-		 * DP MST will use dyn_pcm_assign,
-		 * so DP MST will never come here
-		 */
-		jack_tbl = snd_hda_jack_tbl_get_mst(codec, per_pin->pin_nid,
-						    per_pin->dev_id);
-		if (jack_tbl)
-			jack = jack_tbl->jack;
-	}
-	return jack;
-}
-
 /* update ELD and jack state via audio component */
 static void sync_eld_via_acomp(struct hda_codec *codec,
 			       struct hdmi_spec_per_pin *per_pin)
 {
 	struct hdmi_spec *spec = codec->spec;
 	struct hdmi_eld *eld = &spec->temp_eld;
-	struct snd_jack *jack = NULL;
-	bool changed;
 	int size;
 
 	mutex_lock(&per_pin->lock);
@@ -1674,17 +1686,8 @@ static void sync_eld_via_acomp(struct hda_codec *codec,
 		eld->eld_size = 0;
 	}
 
-	/* pcm_idx >=0 before update_eld() means it is in monitor
-	 * disconnected event. Jack must be fetched before update_eld()
-	 */
-	jack = pin_idx_to_jack(codec, per_pin);
-	changed = update_eld(codec, per_pin, eld);
-	if (jack == NULL)
-		jack = pin_idx_to_jack(codec, per_pin);
-	if (changed && jack)
-		snd_jack_report(jack,
-				(eld->monitor_present && eld->eld_valid) ?
-				SND_JACK_AVOUT : 0);
+	update_eld(codec, per_pin, eld);
+
 	mutex_unlock(&per_pin->lock);
 }
 
-- 
2.16.4

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH] ALSA: hda - Fix DP-MST support for NVIDIA codecs

[Takashi Iwai]

On Mon, 03 Feb 2020 11:06:17 +0100,
Nikhil Mahale wrote:
> 
> If dyn_pcm_assign is set, different jack objects are being created
> for pcm and pins.
> 
> If dyn_pcm_assign is set, generic_hdmi_build_jack() calls into
> add_hdmi_jack_kctl() to create and track separate jack object for
> pcm. Like sync_eld_via_acomp(), hdmi_present_sense_via_verbs() also
> need to report status change of the pcm jack.
> 
> Rename pin_idx_to_jack() to pin_idx_to_pcm_jack(). The code to
> report status change of pcm jack, move it to update_eld() which is
> common for acomp and !acomp code paths.

Thanks, that's the cause of the regression.
However, this needs yet more careful handling, I'm afraid.

- hdmi_present_sense_via_verbs() may return true, and its callers
(both check_presence_and_report() and hdmi_repoll_eld()) do call
snd_hda_jack_report_sync() again.

- For non-dyn_pcm_assign case, we shouldn't call jack report there,
but rather simply return true for calling report sync.

- There is another workaround to block the jack report in
hdmi_present_sense_via_verbs() which is applied after update_eld(),
and this will be ignored.

So, I guess that we need the conditional application of the individual
snd_jack_report() only for spec->dyn_pcm_assign==true, and assure that
hdmi_present() returns false.

The last item (the jack report block) is still unclear to me; it's a
workaround that was needed for Nvidia drivers in the past due to
instability.  If this is still needed for DP-MST case, we have to
reconsider how to deal with it.  Otherwise, this can be applied only
for non-dyn_pcm_assign case.

BTW, the condition for jack->block_report and return value in
hdmi_present_sense_via_verbs() looks currently complicated, but it
could have been simplified like:

-- 8< --
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -1569,7 +1569,6 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
 	 * the unsolicited response to avoid custom WARs.
 	 */
 	int present;
-	bool ret;
 	bool do_repoll = false;
 
 	present = snd_hda_jack_pin_sense(codec, pin_nid, dev_id);
@@ -1603,16 +1602,14 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
 	else
 		update_eld(codec, per_pin, eld);
 
-	ret = !repoll || !eld->monitor_present || eld->eld_valid;
-
 	jack = snd_hda_jack_tbl_get_mst(codec, pin_nid, per_pin->dev_id);
 	if (jack) {
-		jack->block_report = !ret;
+		jack->block_report = do_repoll;
 		jack->pin_sense = (eld->monitor_present && eld->eld_valid) ?
 			AC_PINSENSE_PRESENCE : 0;
 	}
 	mutex_unlock(&per_pin->lock);
-	return ret;
+	return !do_repoll;
 }
 
 static struct snd_jack *pin_idx_to_jack(struct hda_codec *codec,
-- 8< --


Takashi

> 
> Fixes: 5398e94fb753 ALSA: hda - Add DP-MST support for NVIDIA codecs
> Signed-off-by: Nikhil Mahale <nmahale@nvidia.com>
> ---
>  sound/pci/hda/patch_hdmi.c | 87 ++++++++++++++++++++++++----------------------
>  1 file changed, 45 insertions(+), 42 deletions(-)
> 
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index 48bddc218829..7b5360037217 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -1480,6 +1480,35 @@ static void hdmi_pcm_reset_pin(struct hdmi_spec *spec,
>  	per_pin->channels = 0;
>  }
>  
> +static struct snd_jack *pin_idx_to_pcm_jack(struct hda_codec *codec,
> +					    struct hdmi_spec_per_pin *per_pin)
> +{
> +	struct hdmi_spec *spec = codec->spec;
> +	struct snd_jack *jack = NULL;
> +	struct hda_jack_tbl *jack_tbl;
> +
> +	/* if !dyn_pcm_assign, get jack from hda_jack_tbl
> +	 * in !dyn_pcm_assign case, spec->pcm_rec[].jack is not
> +	 * NULL even after snd_hda_jack_tbl_clear() is called to
> +	 * free snd_jack. This may cause access invalid memory
> +	 * when calling snd_jack_report
> +	 */
> +	if (per_pin->pcm_idx >= 0 && spec->dyn_pcm_assign) {
> +		jack = spec->pcm_rec[per_pin->pcm_idx].jack;
> +	} else if (!spec->dyn_pcm_assign) {
> +		/*
> +		 * jack tbl doesn't support DP MST
> +		 * DP MST will use dyn_pcm_assign,
> +		 * so DP MST will never come here
> +		 */
> +		jack_tbl = snd_hda_jack_tbl_get_mst(codec, per_pin->pin_nid,
> +						    per_pin->dev_id);
> +		if (jack_tbl)
> +			jack = jack_tbl->jack;
> +	}
> +	return jack;
> +}
> +
>  /* update per_pin ELD from the given new ELD;
>   * setup info frame and notification accordingly
>   */
> @@ -1490,9 +1519,15 @@ static bool update_eld(struct hda_codec *codec,
>  	struct hdmi_eld *pin_eld = &per_pin->sink_eld;
>  	struct hdmi_spec *spec = codec->spec;
>  	bool old_eld_valid = pin_eld->eld_valid;
> +	struct snd_jack *pcm_jack;
>  	bool eld_changed;
>  	int pcm_idx;
>  
> +	/* pcm_idx >=0 before update_eld() means it is in monitor
> +	 * disconnected event. Jack must be fetched before update_eld()
> +	 */
> +	pcm_jack = pin_idx_to_pcm_jack(codec, per_pin);
> +
>  	/* for monitor disconnection, save pcm_idx firstly */
>  	pcm_idx = per_pin->pcm_idx;
>  	if (spec->dyn_pcm_assign) {
> @@ -1547,6 +1582,14 @@ static bool update_eld(struct hda_codec *codec,
>  			       SNDRV_CTL_EVENT_MASK_VALUE |
>  			       SNDRV_CTL_EVENT_MASK_INFO,
>  			       &get_hdmi_pcm(spec, pcm_idx)->eld_ctl->id);
> +
> +	if (!pcm_jack)
> +		pcm_jack = pin_idx_to_pcm_jack(codec, per_pin);
> +	if (eld_changed && pcm_jack)
> +		snd_jack_report(pcm_jack,
> +				(eld->monitor_present && eld->eld_valid) ?
> +				SND_JACK_AVOUT : 0);
> +
>  	return eld_changed;
>  }
>  
> @@ -1615,43 +1658,12 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
>  	return ret;
>  }
>  
> -static struct snd_jack *pin_idx_to_jack(struct hda_codec *codec,
> -				 struct hdmi_spec_per_pin *per_pin)
> -{
> -	struct hdmi_spec *spec = codec->spec;
> -	struct snd_jack *jack = NULL;
> -	struct hda_jack_tbl *jack_tbl;
> -
> -	/* if !dyn_pcm_assign, get jack from hda_jack_tbl
> -	 * in !dyn_pcm_assign case, spec->pcm_rec[].jack is not
> -	 * NULL even after snd_hda_jack_tbl_clear() is called to
> -	 * free snd_jack. This may cause access invalid memory
> -	 * when calling snd_jack_report
> -	 */
> -	if (per_pin->pcm_idx >= 0 && spec->dyn_pcm_assign)
> -		jack = spec->pcm_rec[per_pin->pcm_idx].jack;
> -	else if (!spec->dyn_pcm_assign) {
> -		/*
> -		 * jack tbl doesn't support DP MST
> -		 * DP MST will use dyn_pcm_assign,
> -		 * so DP MST will never come here
> -		 */
> -		jack_tbl = snd_hda_jack_tbl_get_mst(codec, per_pin->pin_nid,
> -						    per_pin->dev_id);
> -		if (jack_tbl)
> -			jack = jack_tbl->jack;
> -	}
> -	return jack;
> -}
> -
>  /* update ELD and jack state via audio component */
>  static void sync_eld_via_acomp(struct hda_codec *codec,
>  			       struct hdmi_spec_per_pin *per_pin)
>  {
>  	struct hdmi_spec *spec = codec->spec;
>  	struct hdmi_eld *eld = &spec->temp_eld;
> -	struct snd_jack *jack = NULL;
> -	bool changed;
>  	int size;
>  
>  	mutex_lock(&per_pin->lock);
> @@ -1674,17 +1686,8 @@ static void sync_eld_via_acomp(struct hda_codec *codec,
>  		eld->eld_size = 0;
>  	}
>  
> -	/* pcm_idx >=0 before update_eld() means it is in monitor
> -	 * disconnected event. Jack must be fetched before update_eld()
> -	 */
> -	jack = pin_idx_to_jack(codec, per_pin);
> -	changed = update_eld(codec, per_pin, eld);
> -	if (jack == NULL)
> -		jack = pin_idx_to_jack(codec, per_pin);
> -	if (changed && jack)
> -		snd_jack_report(jack,
> -				(eld->monitor_present && eld->eld_valid) ?
> -				SND_JACK_AVOUT : 0);
> +	update_eld(codec, per_pin, eld);
> +
>  	mutex_unlock(&per_pin->lock);
>  }
>  
> -- 
> 2.16.4
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH] ALSA: hda - Fix DP-MST support for NVIDIA codecs

[Martin Regner]

The patch neatly applies and seems to fix my issue.
I had to acivate the device once with pavucontrol (not unplugged 
anymore) and after that the device is working without any extra actions 
after reboot.

Thank you very much.

On 03.02.20 11:06, Nikhil Mahale wrote:
> If dyn_pcm_assign is set, different jack objects are being created
> for pcm and pins.
>
> If dyn_pcm_assign is set, generic_hdmi_build_jack() calls into
> add_hdmi_jack_kctl() to create and track separate jack object for
> pcm. Like sync_eld_via_acomp(), hdmi_present_sense_via_verbs() also
> need to report status change of the pcm jack.
>
> Rename pin_idx_to_jack() to pin_idx_to_pcm_jack(). The code to
> report status change of pcm jack, move it to update_eld() which is
> common for acomp and !acomp code paths.
>
> Fixes: 5398e94fb753 ALSA: hda - Add DP-MST support for NVIDIA codecs
> Signed-off-by: Nikhil Mahale <nmahale@nvidia.com>
> ---
>   sound/pci/hda/patch_hdmi.c | 87 ++++++++++++++++++++++++----------------------
>   1 file changed, 45 insertions(+), 42 deletions(-)
>
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index 48bddc218829..7b5360037217 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -1480,6 +1480,35 @@ static void hdmi_pcm_reset_pin(struct hdmi_spec *spec,
>   	per_pin->channels = 0;
>   }
>   
> +static struct snd_jack *pin_idx_to_pcm_jack(struct hda_codec *codec,
> +					    struct hdmi_spec_per_pin *per_pin)
> +{
> +	struct hdmi_spec *spec = codec->spec;
> +	struct snd_jack *jack = NULL;
> +	struct hda_jack_tbl *jack_tbl;
> +
> +	/* if !dyn_pcm_assign, get jack from hda_jack_tbl
> +	 * in !dyn_pcm_assign case, spec->pcm_rec[].jack is not
> +	 * NULL even after snd_hda_jack_tbl_clear() is called to
> +	 * free snd_jack. This may cause access invalid memory
> +	 * when calling snd_jack_report
> +	 */
> +	if (per_pin->pcm_idx >= 0 && spec->dyn_pcm_assign) {
> +		jack = spec->pcm_rec[per_pin->pcm_idx].jack;
> +	} else if (!spec->dyn_pcm_assign) {
> +		/*
> +		 * jack tbl doesn't support DP MST
> +		 * DP MST will use dyn_pcm_assign,
> +		 * so DP MST will never come here
> +		 */
> +		jack_tbl = snd_hda_jack_tbl_get_mst(codec, per_pin->pin_nid,
> +						    per_pin->dev_id);
> +		if (jack_tbl)
> +			jack = jack_tbl->jack;
> +	}
> +	return jack;
> +}
> +
>   /* update per_pin ELD from the given new ELD;
>    * setup info frame and notification accordingly
>    */
> @@ -1490,9 +1519,15 @@ static bool update_eld(struct hda_codec *codec,
>   	struct hdmi_eld *pin_eld = &per_pin->sink_eld;
>   	struct hdmi_spec *spec = codec->spec;
>   	bool old_eld_valid = pin_eld->eld_valid;
> +	struct snd_jack *pcm_jack;
>   	bool eld_changed;
>   	int pcm_idx;
>   
> +	/* pcm_idx >=0 before update_eld() means it is in monitor
> +	 * disconnected event. Jack must be fetched before update_eld()
> +	 */
> +	pcm_jack = pin_idx_to_pcm_jack(codec, per_pin);
> +
>   	/* for monitor disconnection, save pcm_idx firstly */
>   	pcm_idx = per_pin->pcm_idx;
>   	if (spec->dyn_pcm_assign) {
> @@ -1547,6 +1582,14 @@ static bool update_eld(struct hda_codec *codec,
>   			       SNDRV_CTL_EVENT_MASK_VALUE |
>   			       SNDRV_CTL_EVENT_MASK_INFO,
>   			       &get_hdmi_pcm(spec, pcm_idx)->eld_ctl->id);
> +
> +	if (!pcm_jack)
> +		pcm_jack = pin_idx_to_pcm_jack(codec, per_pin);
> +	if (eld_changed && pcm_jack)
> +		snd_jack_report(pcm_jack,
> +				(eld->monitor_present && eld->eld_valid) ?
> +				SND_JACK_AVOUT : 0);
> +
>   	return eld_changed;
>   }
>   
> @@ -1615,43 +1658,12 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
>   	return ret;
>   }
>   
> -static struct snd_jack *pin_idx_to_jack(struct hda_codec *codec,
> -				 struct hdmi_spec_per_pin *per_pin)
> -{
> -	struct hdmi_spec *spec = codec->spec;
> -	struct snd_jack *jack = NULL;
> -	struct hda_jack_tbl *jack_tbl;
> -
> -	/* if !dyn_pcm_assign, get jack from hda_jack_tbl
> -	 * in !dyn_pcm_assign case, spec->pcm_rec[].jack is not
> -	 * NULL even after snd_hda_jack_tbl_clear() is called to
> -	 * free snd_jack. This may cause access invalid memory
> -	 * when calling snd_jack_report
> -	 */
> -	if (per_pin->pcm_idx >= 0 && spec->dyn_pcm_assign)
> -		jack = spec->pcm_rec[per_pin->pcm_idx].jack;
> -	else if (!spec->dyn_pcm_assign) {
> -		/*
> -		 * jack tbl doesn't support DP MST
> -		 * DP MST will use dyn_pcm_assign,
> -		 * so DP MST will never come here
> -		 */
> -		jack_tbl = snd_hda_jack_tbl_get_mst(codec, per_pin->pin_nid,
> -						    per_pin->dev_id);
> -		if (jack_tbl)
> -			jack = jack_tbl->jack;
> -	}
> -	return jack;
> -}
> -
>   /* update ELD and jack state via audio component */
>   static void sync_eld_via_acomp(struct hda_codec *codec,
>   			       struct hdmi_spec_per_pin *per_pin)
>   {
>   	struct hdmi_spec *spec = codec->spec;
>   	struct hdmi_eld *eld = &spec->temp_eld;
> -	struct snd_jack *jack = NULL;
> -	bool changed;
>   	int size;
>   
>   	mutex_lock(&per_pin->lock);
> @@ -1674,17 +1686,8 @@ static void sync_eld_via_acomp(struct hda_codec *codec,
>   		eld->eld_size = 0;
>   	}
>   
> -	/* pcm_idx >=0 before update_eld() means it is in monitor
> -	 * disconnected event. Jack must be fetched before update_eld()
> -	 */
> -	jack = pin_idx_to_jack(codec, per_pin);
> -	changed = update_eld(codec, per_pin, eld);
> -	if (jack == NULL)
> -		jack = pin_idx_to_jack(codec, per_pin);
> -	if (changed && jack)
> -		snd_jack_report(jack,
> -				(eld->monitor_present && eld->eld_valid) ?
> -				SND_JACK_AVOUT : 0);
> +	update_eld(codec, per_pin, eld);
> +
>   	mutex_unlock(&per_pin->lock);
>   }
>   


_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH] ALSA: hda - Fix DP-MST support for NVIDIA codecs

[Nikhil Mahale]

On 2/3/20 4:10 PM, Takashi Iwai wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Mon, 03 Feb 2020 11:06:17 +0100,
> Nikhil Mahale wrote:
>>
>> If dyn_pcm_assign is set, different jack objects are being created
>> for pcm and pins.
>>
>> If dyn_pcm_assign is set, generic_hdmi_build_jack() calls into
>> add_hdmi_jack_kctl() to create and track separate jack object for
>> pcm. Like sync_eld_via_acomp(), hdmi_present_sense_via_verbs() also
>> need to report status change of the pcm jack.
>>
>> Rename pin_idx_to_jack() to pin_idx_to_pcm_jack(). The code to
>> report status change of pcm jack, move it to update_eld() which is
>> common for acomp and !acomp code paths.
> 
> Thanks, that's the cause of the regression.
> However, this needs yet more careful handling, I'm afraid.
> 
> - hdmi_present_sense_via_verbs() may return true, and its callers
> (both check_presence_and_report() and hdmi_repoll_eld()) do call
> snd_hda_jack_report_sync() again.
> 
> - For non-dyn_pcm_assign case, we shouldn't call jack report there,
> but rather simply return true for calling report sync.
> 
> - There is another workaround to block the jack report in
> hdmi_present_sense_via_verbs() which is applied after update_eld(),
> and this will be ignored.
> 
> So, I guess that we need the conditional application of the individual
> snd_jack_report() only for spec->dyn_pcm_assign==true, and assure that
> hdmi_present() returns false.
Yeah, you are right. But I don't think we should return false from
hdmi_present().

Before dyn_pcm_assign support for non-acomp drivers:
1) pcm and pin plug detection were controlled by same jack object, and
2) change in plug status was reported from snd_hda_jack_report_sync().

If dyn_pcm_assign support is enabled for non-acomp drivers, then pcm
and pin detection are controlled by different jack object. Now, report
for plug status change of both jack object, requires to be in sync.
snd_hda_jack_report_sync() reports change in plug status of pin jack
object. I think after snd_hda_jack_report_sync() we should loop over
all pins, detect change in plug status, and report change in plug
status of pcm jack.
 
> The last item (the jack report block) is still unclear to me; it's a
> workaround that was needed for Nvidia drivers in the past due to
> instability.  If this is still needed for DP-MST case, we have to
> reconsider how to deal with it.  Otherwise, this can be applied only
> for non-dyn_pcm_assign case.
The jack report block, was added by commit 464837a7bc0a (ALSA: hda -
block HDMI jack reports while repolling), to avoid race condition
with repolling. That is not NVIDIA specific.

> BTW, the condition for jack->block_report and return value in
> hdmi_present_sense_via_verbs() looks currently complicated, but it
> could have been simplified like:
> 
> -- 8< --
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -1569,7 +1569,6 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
>          * the unsolicited response to avoid custom WARs.
>          */
>         int present;
> -       bool ret;
>         bool do_repoll = false;
> 
>         present = snd_hda_jack_pin_sense(codec, pin_nid, dev_id);
> @@ -1603,16 +1602,14 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
>         else
>                 update_eld(codec, per_pin, eld);
> 
> -       ret = !repoll || !eld->monitor_present || eld->eld_valid;
> -
>         jack = snd_hda_jack_tbl_get_mst(codec, pin_nid, per_pin->dev_id);
>         if (jack) {
> -               jack->block_report = !ret;
> +               jack->block_report = do_repoll;
>                 jack->pin_sense = (eld->monitor_present && eld->eld_valid) ?
>                         AC_PINSENSE_PRESENCE : 0;
>         }
>         mutex_unlock(&per_pin->lock);
> -       return ret;
> +       return !do_repoll;
>  }
> 
>  static struct snd_jack *pin_idx_to_jack(struct hda_codec *codec,
> -- 8< --
Yeah, this is simple to understand.

I am sending fresh patches, see if they make sense.

Thanks,
Nikhil Mahale

> 
> Takashi
> 
>>
>> Fixes: 5398e94fb753 ALSA: hda - Add DP-MST support for NVIDIA codecs
>> Signed-off-by: Nikhil Mahale <nmahale@nvidia.com>
>> ---
>>  sound/pci/hda/patch_hdmi.c | 87 ++++++++++++++++++++++++----------------------
>>  1 file changed, 45 insertions(+), 42 deletions(-)
>>
>> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
>> index 48bddc218829..7b5360037217 100644
>> --- a/sound/pci/hda/patch_hdmi.c
>> +++ b/sound/pci/hda/patch_hdmi.c
>> @@ -1480,6 +1480,35 @@ static void hdmi_pcm_reset_pin(struct hdmi_spec *spec,
>>       per_pin->channels = 0;
>>  }
>>
>> +static struct snd_jack *pin_idx_to_pcm_jack(struct hda_codec *codec,
>> +                                         struct hdmi_spec_per_pin *per_pin)
>> +{
>> +     struct hdmi_spec *spec = codec->spec;
>> +     struct snd_jack *jack = NULL;
>> +     struct hda_jack_tbl *jack_tbl;
>> +
>> +     /* if !dyn_pcm_assign, get jack from hda_jack_tbl
>> +      * in !dyn_pcm_assign case, spec->pcm_rec[].jack is not
>> +      * NULL even after snd_hda_jack_tbl_clear() is called to
>> +      * free snd_jack. This may cause access invalid memory
>> +      * when calling snd_jack_report
>> +      */
>> +     if (per_pin->pcm_idx >= 0 && spec->dyn_pcm_assign) {
>> +             jack = spec->pcm_rec[per_pin->pcm_idx].jack;
>> +     } else if (!spec->dyn_pcm_assign) {
>> +             /*
>> +              * jack tbl doesn't support DP MST
>> +              * DP MST will use dyn_pcm_assign,
>> +              * so DP MST will never come here
>> +              */
>> +             jack_tbl = snd_hda_jack_tbl_get_mst(codec, per_pin->pin_nid,
>> +                                                 per_pin->dev_id);
>> +             if (jack_tbl)
>> +                     jack = jack_tbl->jack;
>> +     }
>> +     return jack;
>> +}
>> +
>>  /* update per_pin ELD from the given new ELD;
>>   * setup info frame and notification accordingly
>>   */
>> @@ -1490,9 +1519,15 @@ static bool update_eld(struct hda_codec *codec,
>>       struct hdmi_eld *pin_eld = &per_pin->sink_eld;
>>       struct hdmi_spec *spec = codec->spec;
>>       bool old_eld_valid = pin_eld->eld_valid;
>> +     struct snd_jack *pcm_jack;
>>       bool eld_changed;
>>       int pcm_idx;
>>
>> +     /* pcm_idx >=0 before update_eld() means it is in monitor
>> +      * disconnected event. Jack must be fetched before update_eld()
>> +      */
>> +     pcm_jack = pin_idx_to_pcm_jack(codec, per_pin);
>> +
>>       /* for monitor disconnection, save pcm_idx firstly */
>>       pcm_idx = per_pin->pcm_idx;
>>       if (spec->dyn_pcm_assign) {
>> @@ -1547,6 +1582,14 @@ static bool update_eld(struct hda_codec *codec,
>>                              SNDRV_CTL_EVENT_MASK_VALUE |
>>                              SNDRV_CTL_EVENT_MASK_INFO,
>>                              &get_hdmi_pcm(spec, pcm_idx)->eld_ctl->id);
>> +
>> +     if (!pcm_jack)
>> +             pcm_jack = pin_idx_to_pcm_jack(codec, per_pin);
>> +     if (eld_changed && pcm_jack)
>> +             snd_jack_report(pcm_jack,
>> +                             (eld->monitor_present && eld->eld_valid) ?
>> +                             SND_JACK_AVOUT : 0);
>> +
>>       return eld_changed;
>>  }
>>
>> @@ -1615,43 +1658,12 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
>>       return ret;
>>  }
>>
>> -static struct snd_jack *pin_idx_to_jack(struct hda_codec *codec,
>> -                              struct hdmi_spec_per_pin *per_pin)
>> -{
>> -     struct hdmi_spec *spec = codec->spec;
>> -     struct snd_jack *jack = NULL;
>> -     struct hda_jack_tbl *jack_tbl;
>> -
>> -     /* if !dyn_pcm_assign, get jack from hda_jack_tbl
>> -      * in !dyn_pcm_assign case, spec->pcm_rec[].jack is not
>> -      * NULL even after snd_hda_jack_tbl_clear() is called to
>> -      * free snd_jack. This may cause access invalid memory
>> -      * when calling snd_jack_report
>> -      */
>> -     if (per_pin->pcm_idx >= 0 && spec->dyn_pcm_assign)
>> -             jack = spec->pcm_rec[per_pin->pcm_idx].jack;
>> -     else if (!spec->dyn_pcm_assign) {
>> -             /*
>> -              * jack tbl doesn't support DP MST
>> -              * DP MST will use dyn_pcm_assign,
>> -              * so DP MST will never come here
>> -              */
>> -             jack_tbl = snd_hda_jack_tbl_get_mst(codec, per_pin->pin_nid,
>> -                                                 per_pin->dev_id);
>> -             if (jack_tbl)
>> -                     jack = jack_tbl->jack;
>> -     }
>> -     return jack;
>> -}
>> -
>>  /* update ELD and jack state via audio component */
>>  static void sync_eld_via_acomp(struct hda_codec *codec,
>>                              struct hdmi_spec_per_pin *per_pin)
>>  {
>>       struct hdmi_spec *spec = codec->spec;
>>       struct hdmi_eld *eld = &spec->temp_eld;
>> -     struct snd_jack *jack = NULL;
>> -     bool changed;
>>       int size;
>>
>>       mutex_lock(&per_pin->lock);
>> @@ -1674,17 +1686,8 @@ static void sync_eld_via_acomp(struct hda_codec *codec,
>>               eld->eld_size = 0;
>>       }
>>
>> -     /* pcm_idx >=0 before update_eld() means it is in monitor
>> -      * disconnected event. Jack must be fetched before update_eld()
>> -      */
>> -     jack = pin_idx_to_jack(codec, per_pin);
>> -     changed = update_eld(codec, per_pin, eld);
>> -     if (jack == NULL)
>> -             jack = pin_idx_to_jack(codec, per_pin);
>> -     if (changed && jack)
>> -             snd_jack_report(jack,
>> -                             (eld->monitor_present && eld->eld_valid) ?
>> -                             SND_JACK_AVOUT : 0);
>> +     update_eld(codec, per_pin, eld);
>> +
>>       mutex_unlock(&per_pin->lock);
>>  }
>>
>> --
>> 2.16.4
>>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH] ALSA: hda - Fix DP-MST support for NVIDIA codecs

[Takashi Iwai]

On Tue, 04 Feb 2020 06:08:19 +0100,
Nikhil Mahale wrote:
> 
> On 2/3/20 4:10 PM, Takashi Iwai wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Mon, 03 Feb 2020 11:06:17 +0100,
> > Nikhil Mahale wrote:
> >>
> >> If dyn_pcm_assign is set, different jack objects are being created
> >> for pcm and pins.
> >>
> >> If dyn_pcm_assign is set, generic_hdmi_build_jack() calls into
> >> add_hdmi_jack_kctl() to create and track separate jack object for
> >> pcm. Like sync_eld_via_acomp(), hdmi_present_sense_via_verbs() also
> >> need to report status change of the pcm jack.
> >>
> >> Rename pin_idx_to_jack() to pin_idx_to_pcm_jack(). The code to
> >> report status change of pcm jack, move it to update_eld() which is
> >> common for acomp and !acomp code paths.
> > 
> > Thanks, that's the cause of the regression.
> > However, this needs yet more careful handling, I'm afraid.
> > 
> > - hdmi_present_sense_via_verbs() may return true, and its callers
> > (both check_presence_and_report() and hdmi_repoll_eld()) do call
> > snd_hda_jack_report_sync() again.
> > 
> > - For non-dyn_pcm_assign case, we shouldn't call jack report there,
> > but rather simply return true for calling report sync.
> > 
> > - There is another workaround to block the jack report in
> > hdmi_present_sense_via_verbs() which is applied after update_eld(),
> > and this will be ignored.
> > 
> > So, I guess that we need the conditional application of the individual
> > snd_jack_report() only for spec->dyn_pcm_assign==true, and assure that
> > hdmi_present() returns false.
> Yeah, you are right. But I don't think we should return false from
> hdmi_present().
> 
> Before dyn_pcm_assign support for non-acomp drivers:
> 1) pcm and pin plug detection were controlled by same jack object, and
> 2) change in plug status was reported from snd_hda_jack_report_sync().
> 
> If dyn_pcm_assign support is enabled for non-acomp drivers, then pcm
> and pin detection are controlled by different jack object. Now, report
> for plug status change of both jack object, requires to be in sync.

That's my concern.  Basically we're seeing two different jacks for a
single hotplug.  OTOH, from the user-space POV, it must be one jack
that should report back.  IOW, with dyn_pcm_assign, you should ignore
the jack triggered from the unsol handler but report back only for the
jack that is assigned to the PCM.

> snd_hda_jack_report_sync() reports change in plug status of pin jack
> object.
>
> I think after snd_hda_jack_report_sync() we should loop over
> all pins, detect change in plug status, and report change in plug
> status of pcm jack.

This is what snd_hda_jack_report_sync() does; it loops over all
registered jacks and report the changes.

So, I think that in dyn_pcm_assign=true without acomp, we need to
clear jack->dirty for the originally reported jack in addition to the
translation to the PCM jack and reporting.  FWIW, the dirty flag is
set in hdmi_intrinsic_event() and hdmi_repoll_eld().

And if you call snd_jack_report() for the necessary jack, there is no
need to call snd_jack_report_sync() at all.  This is utterly
superfluous.  Hence, if the repoll isn't needed and dyn_pcm_assign=1,
the function should return false.

> > The last item (the jack report block) is still unclear to me; it's a
> > workaround that was needed for Nvidia drivers in the past due to
> > instability.  If this is still needed for DP-MST case, we have to
> > reconsider how to deal with it.  Otherwise, this can be applied only
> > for non-dyn_pcm_assign case.
> The jack report block, was added by commit 464837a7bc0a (ALSA: hda -
> block HDMI jack reports while repolling), to avoid race condition
> with repolling. That is not NVIDIA specific.

Ah yes, that was for some unstable state handling.  I thought the same
workaround applied to some old Nvidia driver, too, though.

> > BTW, the condition for jack->block_report and return value in
> > hdmi_present_sense_via_verbs() looks currently complicated, but it
> > could have been simplified like:
> > 
> > -- 8< --
> > --- a/sound/pci/hda/patch_hdmi.c
> > +++ b/sound/pci/hda/patch_hdmi.c
> > @@ -1569,7 +1569,6 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
> >          * the unsolicited response to avoid custom WARs.
> >          */
> >         int present;
> > -       bool ret;
> >         bool do_repoll = false;
> > 
> >         present = snd_hda_jack_pin_sense(codec, pin_nid, dev_id);
> > @@ -1603,16 +1602,14 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
> >         else
> >                 update_eld(codec, per_pin, eld);
> > 
> > -       ret = !repoll || !eld->monitor_present || eld->eld_valid;
> > -
> >         jack = snd_hda_jack_tbl_get_mst(codec, pin_nid, per_pin->dev_id);
> >         if (jack) {
> > -               jack->block_report = !ret;
> > +               jack->block_report = do_repoll;
> >                 jack->pin_sense = (eld->monitor_present && eld->eld_valid) ?
> >                         AC_PINSENSE_PRESENCE : 0;
> >         }
> >         mutex_unlock(&per_pin->lock);
> > -       return ret;
> > +       return !do_repoll;
> >  }
> > 
> >  static struct snd_jack *pin_idx_to_jack(struct hda_codec *codec,
> > -- 8< --
> Yeah, this is simple to understand.
> 
> I am sending fresh patches, see if they make sense.

The cleanup like the above shouldn't be applied before the critical
fix.  That is, we have to fix the regression at the very first patch
with Cc to stable.  This must be applicable cleanly and working on 5.5
kernel as is.  After the fix, we may apply the remaining cleanups like
the above.

So, let's concentrate on one (or split if needed) fix patch for now.


thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH] ALSA: hda - Fix DP-MST support for NVIDIA codecs

[Takashi Iwai]

On Tue, 04 Feb 2020 08:46:17 +0100,
Takashi Iwai wrote:
> 
> On Tue, 04 Feb 2020 06:08:19 +0100,
> Nikhil Mahale wrote:
> > 
> > On 2/3/20 4:10 PM, Takashi Iwai wrote:
> > > External email: Use caution opening links or attachments
> > > 
> > > 
> > > On Mon, 03 Feb 2020 11:06:17 +0100,
> > > Nikhil Mahale wrote:
> > >>
> > >> If dyn_pcm_assign is set, different jack objects are being created
> > >> for pcm and pins.
> > >>
> > >> If dyn_pcm_assign is set, generic_hdmi_build_jack() calls into
> > >> add_hdmi_jack_kctl() to create and track separate jack object for
> > >> pcm. Like sync_eld_via_acomp(), hdmi_present_sense_via_verbs() also
> > >> need to report status change of the pcm jack.
> > >>
> > >> Rename pin_idx_to_jack() to pin_idx_to_pcm_jack(). The code to
> > >> report status change of pcm jack, move it to update_eld() which is
> > >> common for acomp and !acomp code paths.
> > > 
> > > Thanks, that's the cause of the regression.
> > > However, this needs yet more careful handling, I'm afraid.
> > > 
> > > - hdmi_present_sense_via_verbs() may return true, and its callers
> > > (both check_presence_and_report() and hdmi_repoll_eld()) do call
> > > snd_hda_jack_report_sync() again.
> > > 
> > > - For non-dyn_pcm_assign case, we shouldn't call jack report there,
> > > but rather simply return true for calling report sync.
> > > 
> > > - There is another workaround to block the jack report in
> > > hdmi_present_sense_via_verbs() which is applied after update_eld(),
> > > and this will be ignored.
> > > 
> > > So, I guess that we need the conditional application of the individual
> > > snd_jack_report() only for spec->dyn_pcm_assign==true, and assure that
> > > hdmi_present() returns false.
> > Yeah, you are right. But I don't think we should return false from
> > hdmi_present().
> > 
> > Before dyn_pcm_assign support for non-acomp drivers:
> > 1) pcm and pin plug detection were controlled by same jack object, and
> > 2) change in plug status was reported from snd_hda_jack_report_sync().
> > 
> > If dyn_pcm_assign support is enabled for non-acomp drivers, then pcm
> > and pin detection are controlled by different jack object. Now, report
> > for plug status change of both jack object, requires to be in sync.
> 
> That's my concern.  Basically we're seeing two different jacks for a
> single hotplug.  OTOH, from the user-space POV, it must be one jack
> that should report back.  IOW, with dyn_pcm_assign, you should ignore
> the jack triggered from the unsol handler but report back only for the
> jack that is assigned to the PCM.

Scratch this.  The code is so complex and fuzzing me.  Oh well.

For dyn_pcm_assign, the snd_hda_jack stuff is used only for the unsol
event notification but it's not bound with snd_jack object.  Hence
snd_hda_jack_report_sync() is harmless -- but it's also useless for
dyn_pcm_assign, too.

So basically the logic of your patch 4 is OK.  But it's still
misleading in a few points.

- The snd_hda_jack state was already updated via
  snd_hda_jack_pin_sense() call at the beginning of
  hdmi_present_sense_via_verbs() before calling
  snd_hda_jack_report_sync().

  That implies that what's jack->pinse_sense assignment at the end
  of this function does is to override the jack->pin_sense value that
  was already updated.

- snd_hda_jack_report_sync() is superfluous for dyn_pcm_assign case.
  The jack update was already performed, as mentioned in the above,
  and hda_jack->jack is NULL for dyn_pcm_assign.

  Moreover, snd_hda_jack_report_sync() can be replaced with the
  individual snd_jack_report() call even for non-dyn_pcm_assign case,
  too.  The difference is only how to get snd_jack object; for
  dyn_pcm_assign, it's pin_idx_to_jack() while non-dyn_pcm_assign,
  it's hda_jack->jack.  I guess the call of snd_jack_report() can be
  unified in a helper (that can be called from sync_eld_via_acomp()
  too).


thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH] ALSA: hda - Fix DP-MST support for NVIDIA codecs

[Nikhil Mahale]

On 2/4/20 2:33 PM, Takashi Iwai wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Tue, 04 Feb 2020 08:46:17 +0100,
> Takashi Iwai wrote:
>>
>> On Tue, 04 Feb 2020 06:08:19 +0100,
>> Nikhil Mahale wrote:
>>>
>>> On 2/3/20 4:10 PM, Takashi Iwai wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> On Mon, 03 Feb 2020 11:06:17 +0100,
>>>> Nikhil Mahale wrote:
>>>>>
>>>>> If dyn_pcm_assign is set, different jack objects are being created
>>>>> for pcm and pins.
>>>>>
>>>>> If dyn_pcm_assign is set, generic_hdmi_build_jack() calls into
>>>>> add_hdmi_jack_kctl() to create and track separate jack object for
>>>>> pcm. Like sync_eld_via_acomp(), hdmi_present_sense_via_verbs() also
>>>>> need to report status change of the pcm jack.
>>>>>
>>>>> Rename pin_idx_to_jack() to pin_idx_to_pcm_jack(). The code to
>>>>> report status change of pcm jack, move it to update_eld() which is
>>>>> common for acomp and !acomp code paths.
>>>>
>>>> Thanks, that's the cause of the regression.
>>>> However, this needs yet more careful handling, I'm afraid.
>>>>
>>>> - hdmi_present_sense_via_verbs() may return true, and its callers
>>>> (both check_presence_and_report() and hdmi_repoll_eld()) do call
>>>> snd_hda_jack_report_sync() again.
>>>>
>>>> - For non-dyn_pcm_assign case, we shouldn't call jack report there,
>>>> but rather simply return true for calling report sync.
>>>>
>>>> - There is another workaround to block the jack report in
>>>> hdmi_present_sense_via_verbs() which is applied after update_eld(),
>>>> and this will be ignored.
>>>>
>>>> So, I guess that we need the conditional application of the individual
>>>> snd_jack_report() only for spec->dyn_pcm_assign==true, and assure that
>>>> hdmi_present() returns false.
>>> Yeah, you are right. But I don't think we should return false from
>>> hdmi_present().
>>>
>>> Before dyn_pcm_assign support for non-acomp drivers:
>>> 1) pcm and pin plug detection were controlled by same jack object, and
>>> 2) change in plug status was reported from snd_hda_jack_report_sync().
>>>
>>> If dyn_pcm_assign support is enabled for non-acomp drivers, then pcm
>>> and pin detection are controlled by different jack object. Now, report
>>> for plug status change of both jack object, requires to be in sync.
>>
>> That's my concern.  Basically we're seeing two different jacks for a
>> single hotplug.  OTOH, from the user-space POV, it must be one jack
>> that should report back.  IOW, with dyn_pcm_assign, you should ignore
>> the jack triggered from the unsol handler but report back only for the
>> jack that is assigned to the PCM.
> 
> Scratch this.  The code is so complex and fuzzing me.  Oh well.
> 
> For dyn_pcm_assign, the snd_hda_jack stuff is used only for the unsol
> event notification but it's not bound with snd_jack object.  Hence
> snd_hda_jack_report_sync() is harmless -- but it's also useless for
> dyn_pcm_assign, too.
> 
> So basically the logic of your patch 4 is OK.  But it's still
> misleading in a few points.
> 
> - The snd_hda_jack state was already updated via
>   snd_hda_jack_pin_sense() call at the beginning of
>   hdmi_present_sense_via_verbs() before calling
>   snd_hda_jack_report_sync().
> 
>   That implies that what's jack->pinse_sense assignment at the end
>   of this function does is to override the jack->pin_sense value that
>   was already updated.
> 
> - snd_hda_jack_report_sync() is superfluous for dyn_pcm_assign case.
>   The jack update was already performed, as mentioned in the above,
>   and hda_jack->jack is NULL for dyn_pcm_assign.
> 
>   Moreover, snd_hda_jack_report_sync() can be replaced with the
>   individual snd_jack_report() call even for non-dyn_pcm_assign case,
>   too.  The difference is only how to get snd_jack object; for
>   dyn_pcm_assign, it's pin_idx_to_jack() while non-dyn_pcm_assign,
>   it's hda_jack->jack.  I guess the call of snd_jack_report() can be
>   unified in a helper (that can be called from sync_eld_via_acomp()
>   too).
The code is really complex and fuzzing me too. Anyways, I have send
out fresh patch for review, see that is exactly what we are
looking for. As you said earlier, cleanup change will go in separate
patch set.

Thanks,
Nikhil Mahale

> 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

Re: [alsa-devel] [PATCH] ALSA: hda - Fix DP-MST support for NVIDIA codecs

[Takashi Iwai]

On Tue, 04 Feb 2020 17:10:45 +0100,
Martin Regner wrote:
> 
> Applied the new patch. The device is detected correctly by pulseaudio.
> Thanks for your efforts.

Great, thanks for quick testing.
Now I applied and pushed out.


Takashi

> 
> On 04.02.20 12:18, Takashi Iwai wrote:
> > On Tue, 04 Feb 2020 11:27:46 +0100,
> > Nikhil Mahale wrote:
> >> If dyn_pcm_assign is set, different jack objects are being created
> >> for pcm and pins.
> >>
> >> If dyn_pcm_assign is set, generic_hdmi_build_jack() calls into
> >> add_hdmi_jack_kctl() to create and track separate jack object for
> >> pcm. Like sync_eld_via_acomp(), hdmi_present_sense_via_verbs() also
> >> need to report status change of the pcm jack.
> >>
> >> Rename pin_idx_to_jack() to pin_idx_to_pcm_jack(). Update
> >> hdmi_present_sense_via_verbs() to report plug state of pcm jack
> >> object. Unlike sync_eld_via_acomp(), for !acomp drivers the pcm
> >> jack's plug state must be consistent with plug state
> >> of pin's jack.
> > Thanks, the new patch looks better.
> >
> >> Fixes: 5398e94fb753 ("ALSA: hda - Add DP-MST support for NVIDIA codecs")
> >> Signed-off-by: Nikhil Mahale <nmahale@nvidia.com>
> > We need Cc to stable here.  I'll add it when applying.
> >
> > Also, it deserves Reported-by from Martin.
> > Martin, could you retest with this patch?  I'll queue the patch once
> > after confirmation.
> >
> > Just one minor nitpick:
> >
> >> +		if (spec->dyn_pcm_assign && pcm_jack && !do_repoll) {
> >> +			int state = 0;
> >> +
> >> +			if (!!(jack->pin_sense & AC_PINSENSE_PRESENCE))
> >> +				state = SND_JACK_AVOUT;
> > The "!!" is superfluous.  I'll drop it.
> >
> >
> > Takashi
> 
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH] ALSA: hda - Fix DP-MST support for NVIDIA codecs

[Martin Regner]

Applied the new patch. The device is detected correctly by pulseaudio.
Thanks for your efforts.

On 04.02.20 12:18, Takashi Iwai wrote:
> On Tue, 04 Feb 2020 11:27:46 +0100,
> Nikhil Mahale wrote:
>> If dyn_pcm_assign is set, different jack objects are being created
>> for pcm and pins.
>>
>> If dyn_pcm_assign is set, generic_hdmi_build_jack() calls into
>> add_hdmi_jack_kctl() to create and track separate jack object for
>> pcm. Like sync_eld_via_acomp(), hdmi_present_sense_via_verbs() also
>> need to report status change of the pcm jack.
>>
>> Rename pin_idx_to_jack() to pin_idx_to_pcm_jack(). Update
>> hdmi_present_sense_via_verbs() to report plug state of pcm jack
>> object. Unlike sync_eld_via_acomp(), for !acomp drivers the pcm
>> jack's plug state must be consistent with plug state
>> of pin's jack.
> Thanks, the new patch looks better.
>
>> Fixes: 5398e94fb753 ("ALSA: hda - Add DP-MST support for NVIDIA codecs")
>> Signed-off-by: Nikhil Mahale <nmahale@nvidia.com>
> We need Cc to stable here.  I'll add it when applying.
>
> Also, it deserves Reported-by from Martin.
> Martin, could you retest with this patch?  I'll queue the patch once
> after confirmation.
>
> Just one minor nitpick:
>
>> +		if (spec->dyn_pcm_assign && pcm_jack && !do_repoll) {
>> +			int state = 0;
>> +
>> +			if (!!(jack->pin_sense & AC_PINSENSE_PRESENCE))
>> +				state = SND_JACK_AVOUT;
> The "!!" is superfluous.  I'll drop it.
>
>
> Takashi


_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH] ALSA: hda - Fix DP-MST support for NVIDIA codecs

[Kai Vehmanen]

Hi,

On Tue, 4 Feb 2020, Nikhil Mahale wrote:

> If dyn_pcm_assign is set, different jack objects are being created
> for pcm and pins.

seems good. With the one change Takashi spotted:
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>

Br, Kai
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH] ALSA: hda - Fix DP-MST support for NVIDIA codecs

[Takashi Iwai]

On Tue, 04 Feb 2020 11:27:46 +0100,
Nikhil Mahale wrote:
> 
> If dyn_pcm_assign is set, different jack objects are being created
> for pcm and pins.
> 
> If dyn_pcm_assign is set, generic_hdmi_build_jack() calls into
> add_hdmi_jack_kctl() to create and track separate jack object for
> pcm. Like sync_eld_via_acomp(), hdmi_present_sense_via_verbs() also
> need to report status change of the pcm jack.
> 
> Rename pin_idx_to_jack() to pin_idx_to_pcm_jack(). Update
> hdmi_present_sense_via_verbs() to report plug state of pcm jack
> object. Unlike sync_eld_via_acomp(), for !acomp drivers the pcm
> jack's plug state must be consistent with plug state
> of pin's jack.

Thanks, the new patch looks better.

> Fixes: 5398e94fb753 ("ALSA: hda - Add DP-MST support for NVIDIA codecs")
> Signed-off-by: Nikhil Mahale <nmahale@nvidia.com>

We need Cc to stable here.  I'll add it when applying.

Also, it deserves Reported-by from Martin.
Martin, could you retest with this patch?  I'll queue the patch once
after confirmation.

Just one minor nitpick:

> +		if (spec->dyn_pcm_assign && pcm_jack && !do_repoll) {
> +			int state = 0;
> +
> +			if (!!(jack->pin_sense & AC_PINSENSE_PRESENCE))
> +				state = SND_JACK_AVOUT;

The "!!" is superfluous.  I'll drop it.


Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] [PATCH] ALSA: hda - Fix DP-MST support for NVIDIA codecs

[Nikhil Mahale]

If dyn_pcm_assign is set, different jack objects are being created
for pcm and pins.

If dyn_pcm_assign is set, generic_hdmi_build_jack() calls into
add_hdmi_jack_kctl() to create and track separate jack object for
pcm. Like sync_eld_via_acomp(), hdmi_present_sense_via_verbs() also
need to report status change of the pcm jack.

Rename pin_idx_to_jack() to pin_idx_to_pcm_jack(). Update
hdmi_present_sense_via_verbs() to report plug state of pcm jack
object. Unlike sync_eld_via_acomp(), for !acomp drivers the pcm
jack's plug state must be consistent with plug state
of pin's jack.

Fixes: 5398e94fb753 ("ALSA: hda - Add DP-MST support for NVIDIA codecs")
Signed-off-by: Nikhil Mahale <nmahale@nvidia.com>
---
 sound/pci/hda/patch_hdmi.c | 94 +++++++++++++++++++++++++++++++---------------
 1 file changed, 63 insertions(+), 31 deletions(-)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 48bddc218829..c1d3ce423142 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -1550,6 +1550,34 @@ static bool update_eld(struct hda_codec *codec,
 	return eld_changed;
 }
 
+static struct snd_jack *pin_idx_to_pcm_jack(struct hda_codec *codec,
+					    struct hdmi_spec_per_pin *per_pin)
+{
+	struct hdmi_spec *spec = codec->spec;
+	struct snd_jack *jack = NULL;
+	struct hda_jack_tbl *jack_tbl;
+
+	/* if !dyn_pcm_assign, get jack from hda_jack_tbl
+	 * in !dyn_pcm_assign case, spec->pcm_rec[].jack is not
+	 * NULL even after snd_hda_jack_tbl_clear() is called to
+	 * free snd_jack. This may cause access invalid memory
+	 * when calling snd_jack_report
+	 */
+	if (per_pin->pcm_idx >= 0 && spec->dyn_pcm_assign) {
+		jack = spec->pcm_rec[per_pin->pcm_idx].jack;
+	} else if (!spec->dyn_pcm_assign) {
+		/*
+		 * jack tbl doesn't support DP MST
+		 * DP MST will use dyn_pcm_assign,
+		 * so DP MST will never come here
+		 */
+		jack_tbl = snd_hda_jack_tbl_get_mst(codec, per_pin->pin_nid,
+						    per_pin->dev_id);
+		if (jack_tbl)
+			jack = jack_tbl->jack;
+	}
+	return jack;
+}
 /* update ELD and jack state via HD-audio verbs */
 static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
 					 int repoll)
@@ -1571,6 +1599,7 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
 	int present;
 	bool ret;
 	bool do_repoll = false;
+	struct snd_jack *pcm_jack = NULL;
 
 	present = snd_hda_jack_pin_sense(codec, pin_nid, dev_id);
 
@@ -1598,10 +1627,19 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
 			do_repoll = true;
 	}
 
-	if (do_repoll)
+	if (do_repoll) {
 		schedule_delayed_work(&per_pin->work, msecs_to_jiffies(300));
-	else
+	} else {
+		/*
+		 * pcm_idx >=0 before update_eld() means it is in monitor
+		 * disconnected event. Jack must be fetched before
+		 * update_eld().
+		 */
+		pcm_jack = pin_idx_to_pcm_jack(codec, per_pin);
 		update_eld(codec, per_pin, eld);
+		if (!pcm_jack)
+			pcm_jack = pin_idx_to_pcm_jack(codec, per_pin);
+	}
 
 	ret = !repoll || !eld->monitor_present || eld->eld_valid;
 
@@ -1610,38 +1648,32 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
 		jack->block_report = !ret;
 		jack->pin_sense = (eld->monitor_present && eld->eld_valid) ?
 			AC_PINSENSE_PRESENCE : 0;
-	}
-	mutex_unlock(&per_pin->lock);
-	return ret;
-}
 
-static struct snd_jack *pin_idx_to_jack(struct hda_codec *codec,
-				 struct hdmi_spec_per_pin *per_pin)
-{
-	struct hdmi_spec *spec = codec->spec;
-	struct snd_jack *jack = NULL;
-	struct hda_jack_tbl *jack_tbl;
+		if (spec->dyn_pcm_assign && pcm_jack && !do_repoll) {
+			int state = 0;
+
+			if (!!(jack->pin_sense & AC_PINSENSE_PRESENCE))
+				state = SND_JACK_AVOUT;
+			snd_jack_report(pcm_jack, state);
+		}
 
-	/* if !dyn_pcm_assign, get jack from hda_jack_tbl
-	 * in !dyn_pcm_assign case, spec->pcm_rec[].jack is not
-	 * NULL even after snd_hda_jack_tbl_clear() is called to
-	 * free snd_jack. This may cause access invalid memory
-	 * when calling snd_jack_report
-	 */
-	if (per_pin->pcm_idx >= 0 && spec->dyn_pcm_assign)
-		jack = spec->pcm_rec[per_pin->pcm_idx].jack;
-	else if (!spec->dyn_pcm_assign) {
 		/*
-		 * jack tbl doesn't support DP MST
-		 * DP MST will use dyn_pcm_assign,
-		 * so DP MST will never come here
+		 * snd_hda_jack_pin_sense() call at the beginning of this
+		 * function, updates jack->pins_sense and clears
+		 * jack->jack_dirty, therefore snd_hda_jack_report_sync() will
+		 * not override the jack->pin_sense.
+		 *
+		 * snd_hda_jack_report_sync() is superfluous for dyn_pcm_assign
+		 * case. The jack->pin_sense update was already performed, and
+		 * hda_jack->jack is NULL for dyn_pcm_assign.
+		 *
+		 * Don't call snd_hda_jack_report_sync() for
+		 * dyn_pcm_assign.
 		 */
-		jack_tbl = snd_hda_jack_tbl_get_mst(codec, per_pin->pin_nid,
-						    per_pin->dev_id);
-		if (jack_tbl)
-			jack = jack_tbl->jack;
+		ret = ret && !spec->dyn_pcm_assign;
 	}
-	return jack;
+	mutex_unlock(&per_pin->lock);
+	return ret;
 }
 
 /* update ELD and jack state via audio component */
@@ -1677,10 +1709,10 @@ static void sync_eld_via_acomp(struct hda_codec *codec,
 	/* pcm_idx >=0 before update_eld() means it is in monitor
 	 * disconnected event. Jack must be fetched before update_eld()
 	 */
-	jack = pin_idx_to_jack(codec, per_pin);
+	jack = pin_idx_to_pcm_jack(codec, per_pin);
 	changed = update_eld(codec, per_pin, eld);
 	if (jack == NULL)
-		jack = pin_idx_to_jack(codec, per_pin);
+		jack = pin_idx_to_pcm_jack(codec, per_pin);
 	if (changed && jack)
 		snd_jack_report(jack,
 				(eld->monitor_present && eld->eld_valid) ?
-- 
2.16.4

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel