| Message ID | s5hr3eagdy6.wl-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2016? 04? 13? 16:33? Takashi Iwai ?(?) ? ?: > On Wed, 13 Apr 2016 02:27:39 +0200, > Hyungwon Hwang wrote: >> >> pin_eld is a temporary structure. So before calling update_eld(), >> eld should be updated from pin_eld. >> >> Signed-off-by: Hyungwon Hwang <hyungwon.hwang7@gmail.com> > > It's not clear what you're trying to solve only by the description > above alone. It's about the inconsistent "monitor present" appearance > in eld proc file, right? You need to write "why your patch is > necessary" in your patch description. > > Now back to the code change: > >> --- >> sound/pci/hda/patch_hdmi.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c >> index 5af372d..9de114d 100644 >> --- a/sound/pci/hda/patch_hdmi.c >> +++ b/sound/pci/hda/patch_hdmi.c >> @@ -1414,6 +1414,8 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin, >> >> mutex_lock(&per_pin->lock); >> pin_eld->monitor_present = !!(present & AC_PINSENSE_PRESENCE); >> + eld->monitor_present = pin_eld->monitor_present; >> + >> if (pin_eld->monitor_present) >> eld->eld_valid = !!(present & AC_PINSENSE_ELDV); >> else > > OK, this would work, but the fundamental problem is that pin_eld is > modified in this function. This used to work in the past casually due > to a bug, but I plugged it in the commit bd48128539ab > ALSA: hda - Fix forgotten HDMI monitor_present update > but overlooked the regression in non-component side. My bad. > > So, the real fix is to replace the all pin_eld with eld in this > function. The untested patch is below. Could you check whether this > works for you? Your code works well in my case, and seems more reasonable even to me. Thanks for your review and work. Thanks, Hyungwon Hwang > > Though, as a stable fix, maybe applying your oneliner patch would be > safer. It restores to the old behavior, at least. Then we can apply > the patch below on the top. > > > thanks, > > Takashi > > --- > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c > index 5af372d01834..c83c1a8d9742 100644 > --- a/sound/pci/hda/patch_hdmi.c > +++ b/sound/pci/hda/patch_hdmi.c > @@ -1396,7 +1396,6 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin, > struct hda_codec *codec = per_pin->codec; > struct hdmi_spec *spec = codec->spec; > struct hdmi_eld *eld = &spec->temp_eld; > - struct hdmi_eld *pin_eld = &per_pin->sink_eld; > hda_nid_t pin_nid = per_pin->pin_nid; > /* > * Always execute a GetPinSense verb here, even when called from > @@ -1413,15 +1412,15 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin, > present = snd_hda_pin_sense(codec, pin_nid); > > mutex_lock(&per_pin->lock); > - pin_eld->monitor_present = !!(present & AC_PINSENSE_PRESENCE); > - if (pin_eld->monitor_present) > + eld->monitor_present = !!(present & AC_PINSENSE_PRESENCE); > + if (eld->monitor_present) > eld->eld_valid = !!(present & AC_PINSENSE_ELDV); > else > eld->eld_valid = false; > > codec_dbg(codec, > "HDMI status: Codec=%d Pin=%d Presence_Detect=%d ELD_Valid=%d\n", > - codec->addr, pin_nid, pin_eld->monitor_present, eld->eld_valid); > + codec->addr, pin_nid, eld->monitor_present, eld->eld_valid); > > if (eld->eld_valid) { > if (spec->ops.pin_get_eld(codec, pin_nid, eld->eld_buffer, > @@ -1441,7 +1440,7 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin, > else > update_eld(codec, per_pin, eld); > > - ret = !repoll || !pin_eld->monitor_present || pin_eld->eld_valid; > + ret = !repoll || !eld->monitor_present || eld->eld_valid; > > jack = snd_hda_jack_tbl_get(codec, pin_nid); > if (jack) >
On Wed, 13 Apr 2016 16:53:59 +0200, Hyungwon Hwang wrote: > > 2016? 04? 13? 16:33? Takashi Iwai ?(?) ? ?: > > On Wed, 13 Apr 2016 02:27:39 +0200, > > Hyungwon Hwang wrote: > >> > >> pin_eld is a temporary structure. So before calling update_eld(), > >> eld should be updated from pin_eld. > >> > >> Signed-off-by: Hyungwon Hwang <hyungwon.hwang7@gmail.com> > > > > It's not clear what you're trying to solve only by the description > > above alone. It's about the inconsistent "monitor present" appearance > > in eld proc file, right? You need to write "why your patch is > > necessary" in your patch description. > > > > Now back to the code change: > > > >> --- > >> sound/pci/hda/patch_hdmi.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c > >> index 5af372d..9de114d 100644 > >> --- a/sound/pci/hda/patch_hdmi.c > >> +++ b/sound/pci/hda/patch_hdmi.c > >> @@ -1414,6 +1414,8 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin, > >> > >> mutex_lock(&per_pin->lock); > >> pin_eld->monitor_present = !!(present & AC_PINSENSE_PRESENCE); > >> + eld->monitor_present = pin_eld->monitor_present; > >> + > >> if (pin_eld->monitor_present) > >> eld->eld_valid = !!(present & AC_PINSENSE_ELDV); > >> else > > > > OK, this would work, but the fundamental problem is that pin_eld is > > modified in this function. This used to work in the past casually due > > to a bug, but I plugged it in the commit bd48128539ab > > ALSA: hda - Fix forgotten HDMI monitor_present update > > but overlooked the regression in non-component side. My bad. > > > > So, the real fix is to replace the all pin_eld with eld in this > > function. The untested patch is below. Could you check whether this > > works for you? > > Your code works well in my case, and seems more reasonable even to me. > Thanks for your review and work. OK, I resubmit your and my patches with slightly more comments, then apply them to for-linus branch. thanks, Takashi
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 5af372d01834..c83c1a8d9742 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -1396,7 +1396,6 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin, struct hda_codec *codec = per_pin->codec; struct hdmi_spec *spec = codec->spec; struct hdmi_eld *eld = &spec->temp_eld; - struct hdmi_eld *pin_eld = &per_pin->sink_eld; hda_nid_t pin_nid = per_pin->pin_nid; /* * Always execute a GetPinSense verb here, even when called from @@ -1413,15 +1412,15 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin, present = snd_hda_pin_sense(codec, pin_nid); mutex_lock(&per_pin->lock); - pin_eld->monitor_present = !!(present & AC_PINSENSE_PRESENCE); - if (pin_eld->monitor_present) + eld->monitor_present = !!(present & AC_PINSENSE_PRESENCE); + if (eld->monitor_present) eld->eld_valid = !!(present & AC_PINSENSE_ELDV); else eld->eld_valid = false; codec_dbg(codec, "HDMI status: Codec=%d Pin=%d Presence_Detect=%d ELD_Valid=%d\n", - codec->addr, pin_nid, pin_eld->monitor_present, eld->eld_valid); + codec->addr, pin_nid, eld->monitor_present, eld->eld_valid); if (eld->eld_valid) { if (spec->ops.pin_get_eld(codec, pin_nid, eld->eld_buffer, @@ -1441,7 +1440,7 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin, else update_eld(codec, per_pin, eld); - ret = !repoll || !pin_eld->monitor_present || pin_eld->eld_valid; + ret = !repoll || !eld->monitor_present || eld->eld_valid; jack = snd_hda_jack_tbl_get(codec, pin_nid); if (jack)