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@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
