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