[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