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

[Nikhil Mahale <nmahale@nvidia.com>]

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