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

[Takashi Iwai]

On Sat, 04 May 2019 04:45:36 +0200,
Hui Wang wrote:
> 
> 
> 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.

Right...

> 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.

OK, that should work, just overriding and correcting the pin_sense.

I guess the only missing code path is the case where jack->dirty is
set manually without the callback call via
snd_hda_jack_set_dirty_all().  Through a quick glance, it's called
from the common resume code, hda_call_codec_resume().  But, again,
HDMI codec has its own resume to refresh pin detection
(generic_hdmi_resume()), and the changed code-path should be
involved.

The rest callers of snd_hda_jack_set_dirty_all() are the polling mode
and the unsol event handler for the old HDMI codecs, both of which we
don't care much.

That said, your minimal change looks good to me, and I'll happily
apply as long as it's tested.  Of course, it needs some more careful
comments about the behavior.


Thanks!

Takashi