Re: [PATCH] ALSA: hda/hdmi - Don't report Jack event if no need to do that

[Hui Wang <hui.wang@canonical.com>]

On 2019/5/3 下午11:57, Takashi Iwai wrote:
> On Fri, 03 May 2019 06:05:09 +0200,
> Hui Wang wrote:
>>
>> On 2019/5/2 下午11:49, Takashi Iwai wrote:
>>> On Thu, 02 May 2019 04:52:46 +0200,
>>> Hui Wang wrote:
>>>> On 2019/4/30 下午5:02, Takashi Iwai wrote:
>>>>> On Tue, 30 Apr 2019 10:42:55 +0200,
>>>>> Hui Wang wrote:
>>>>>> On 2019/4/30 下午3:35, Takashi Iwai wrote:
>>>>>>> On Tue, 30 Apr 2019 08:57:11 +0200,
>>>>>>> Hui Wang wrote:
>>>>>>>> On the machines with AMD GPU or Nvidia GPU, we often meet this issues:
>>>>>>>> after s3, there are 4 HDMI/DP audio devices in the gnome-sound-setting
>>>>>>>> even there is no any monitors plugged.
>>>>>>>>
>>>>>>>> When this problem happens, we check the /proc/asound/cardX/eld#N.M, we
>>>>>>>> will find the monitor_present=1, eld_valid=0.
>>>>>>>>
>>>>>>>> So the fix is putting your change and my change together like below?
>>> Well, removing "!repoll" should be treated as a separate patch, and
>>> I'm not sure whether we want to get rid of it.
>>>
>>> The "!repoll" check is a bit misleading.  This condition indicates
>>> that the jack status sync is required, e.g. it's the case where
>>> polling goes to timeout or at the beginning of the probe.  For
>>> example, in the case of resume, it starts with repoll=1.  If an
>>> incomplete state (monitor=1, eld_valid=0) is seen at this point, the
>>> code schedules for the retry at a later point.  And if it reaches to
>>> the max count, it clears to repoll=0 and try the last attempt, which
>>> won't schedule any longer.
>>>
>>> So, at this point, we *have to* notify the result no matter whether
>>> it's satisfying or not.
>>>
>>>
>>> thanks,
>>>
>>> Takashi
>> So let us do sth in the hda_jack.c, adding two members check_eld and
>> eld_valid in the struct hda_jack_tbl{},
>>
>> When building new jacks, since there is no eld_data yet, just follow
>> old rules (only checking AC_PINSENSE_PRESENCE)
>>
>> After building new jacks, if it is not a phantom one, set check_eld to 1
>>
>> every time we get eld (via verbs or acomp), we will sync
>> jack->eld_valid to eld->eld_valid.
>>
>> in the report_sync(), we will check check_eld and eld_valid, not
>> depends on AC_PINSENSE_PRESENCE only.
>>
>> No matter interrupt, s3 or poll,  the jack->eld_valid will be synced
>> and the report_sync() will report jack state depeneds on both
>> AC_PINSENSE_PRESENCE and eld_valid
>>
>> Does it sound good?
> OK, I was confused -- I overlooked the fact that hda_jack.c updates
> the pin sense locally.  And thanks for the suggestion, we're heading
> to the right direction.
>
> One thing that doesn't fit is introducing HDMI-specific things like
> eld_valid and check_eld.  Basically this is about the falsely reported
> jack detection, after all.  So, what we need is the correction of the
> incorrect jack detection -- or a way to override the value.
>
> Actually, for HDMI, there is no reason to read the pin sense at all.
> All we need is already provided by the HDMI codec driver (monitor
> present and eld valid).
>
> Then I can think of adding a new flag hda_jack_tbl.manual_pin_sense,
> for example, indicating that the pin sense is updated by the driver.
> That's a change something like below.
>
Looks like the hdmi_present_sense_via_verbs() still has some issue. At 
the entry of this function,  it calls snd_hda_pin_sense() and read the 
eld_data depending on the return value,  here it expects reading the pin 
sense register, after applying your change, this function returns a 
shadow value instead of a register value, if we set the pin_sense to 0, 
then it is possible that the pin_get_eld() will not be called anymore.

And inspired by your change, maybe we just make this change, then it is 
enough to fix the falsely report issue here.

@@ -1551,8 +1551,11 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
  	ret = !repoll || !eld->monitor_present || eld->eld_valid;
  
  	jack = snd_hda_jack_tbl_get(codec, pin_nid);
-	if (jack)
+	if (jack) {
  		jack->block_report = !ret;
+		jack->pin_sense = (eld->monitor_present && eld->eld_valid) ?
+			AC_PINSENSE_PRESENCE : 0;
+	}

because in the snd_hda_pin_sense(), the jack_dirty is set to 0, then we 
change the jack->pin_sense,  and in the report_sync() it will decide the 
jack state according to the jack->pin_sense we changed.

Thanks,

Hui.


> thanks,
>
> Takashi
>
> --- a/sound/pci/hda/hda_jack.c
> +++ b/sound/pci/hda/hda_jack.c
> @@ -189,7 +189,7 @@ void snd_hda_jack_set_dirty_all(struct hda_codec *codec)
>   	int i;
>   
>   	for (i = 0; i < codec->jacktbl.used; i++, jack++)
> -		if (jack->nid)
> +		if (jack->nid && !jack->manual_pin_sense)
>   			jack->jack_dirty = 1;
>   }
>   EXPORT_SYMBOL_GPL(snd_hda_jack_set_dirty_all);
> @@ -568,7 +568,8 @@ void snd_hda_jack_unsol_event(struct hda_codec *codec, unsigned int res)
>   	event = snd_hda_jack_tbl_get_from_tag(codec, tag);
>   	if (!event)
>   		return;
> -	event->jack_dirty = 1;
> +	if (!event->manual_pin_sense)
> +		event->jack_dirty = 1;
>   
>   	call_jack_callback(codec, res, event);
>   	snd_hda_jack_report_sync(codec);
> --- a/sound/pci/hda/hda_jack.h
> +++ b/sound/pci/hda/hda_jack.h
> @@ -40,6 +40,7 @@ struct hda_jack_tbl {
>   	unsigned int jack_dirty:1;	/* needs to update? */
>   	unsigned int phantom_jack:1;    /* a fixed, always present port? */
>   	unsigned int block_report:1;    /* in a transitional state - do not report to userspace */
> +	unsigned int manual_pin_sense:1; /* pin_sense is updated by codec driver */
>   	hda_nid_t gating_jack;		/* valid when gating jack plugged */
>   	hda_nid_t gated_jack;		/* gated is dependent on this jack */
>   	int type;
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -170,6 +170,7 @@ struct hdmi_spec {
>   
>   	bool dyn_pin_out;
>   	bool dyn_pcm_assign;
> +	bool use_pin_sense;
>   	/*
>   	 * Non-generic VIA/NVIDIA specific
>   	 */
> @@ -797,7 +798,6 @@ static void hdmi_intrinsic_event(struct hda_codec *codec, unsigned int res)
>   	jack = snd_hda_jack_tbl_get_from_tag(codec, tag);
>   	if (!jack)
>   		return;
> -	jack->jack_dirty = 1;
>   
>   	codec_dbg(codec,
>   		"HDMI hot plug event: Codec=%d Pin=%d Device=%d Inactive=%d Presence_Detect=%d ELD_Valid=%d\n",
> @@ -1551,8 +1551,11 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
>   	ret = !repoll || !eld->monitor_present || eld->eld_valid;
>   
>   	jack = snd_hda_jack_tbl_get(codec, pin_nid);
> -	if (jack)
> +	if (jack) {
>   		jack->block_report = !ret;
> +		jack->pin_sense = (eld->monitor_present && eld->eld_valid) ?
> +			AC_PINSENSE_PRESENCE : 0;
> +	}
>   
>   	mutex_unlock(&per_pin->lock);
>   	return ret;
> @@ -2163,6 +2166,7 @@ static int generic_hdmi_build_jack(struct hda_codec *codec, int pcm_idx)
>   	 * align with dyn_pcm_assign mode
>   	 */
>   	spec->pcm_rec[pcm_idx].jack = jack->jack;
> +	jack->manual_pin_sense = !spec->use_pin_sense;
>   	return 0;
>   }
>   
> @@ -2972,6 +2976,7 @@ static int patch_simple_hdmi(struct hda_codec *codec,
>   	spec->multiout.dig_out_nid = cvt_nid;
>   	spec->num_cvts = 1;
>   	spec->num_pins = 1;
> +	spec->use_pin_sense = true;
>   	per_pin = snd_array_new(&spec->pins);
>   	per_cvt = snd_array_new(&spec->cvts);
>   	if (!per_pin || !per_cvt) {
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel