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