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

[Nikhil Mahale <nmahale@nvidia.com>]

On 2/4/20 2:33 PM, Takashi Iwai wrote:
> External email: Use caution opening links or attachments
> 
> 
> 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).
The code is really complex and fuzzing me too. Anyways, I have send
out fresh patch for review, see that is exactly what we are
looking for. As you said earlier, cleanup change will go in separate
patch set.

Thanks,
Nikhil Mahale

> thanks,
> 
> Takashi
> 

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel