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:
- pcm and pin plug detection were controlled by same jack object, and
- 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. -----------------------------------------------------------------------------------