* [PATCH] ALSA: hda - Update HDMI monitor_present from a temporary structure @ 2016-04-13 0:27 Hyungwon Hwang 2016-04-13 7:33 ` Takashi Iwai 0 siblings, 1 reply; 4+ messages in thread From: Hyungwon Hwang @ 2016-04-13 0:27 UTC (permalink / raw) To: perex, tiwai, alsa-devel; +Cc: Hyungwon Hwang 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> --- 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 -- 2.5.5 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] ALSA: hda - Update HDMI monitor_present from a temporary structure 2016-04-13 0:27 [PATCH] ALSA: hda - Update HDMI monitor_present from a temporary structure Hyungwon Hwang @ 2016-04-13 7:33 ` Takashi Iwai 2016-04-13 14:53 ` Hyungwon Hwang 0 siblings, 1 reply; 4+ messages in thread From: Takashi Iwai @ 2016-04-13 7:33 UTC (permalink / raw) To: Hyungwon Hwang; +Cc: alsa-devel 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? 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) ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] ALSA: hda - Update HDMI monitor_present from a temporary structure 2016-04-13 7:33 ` Takashi Iwai @ 2016-04-13 14:53 ` Hyungwon Hwang 2016-04-13 15:01 ` Takashi Iwai 0 siblings, 1 reply; 4+ messages in thread From: Hyungwon Hwang @ 2016-04-13 14:53 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel 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) > _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ALSA: hda - Update HDMI monitor_present from a temporary structure 2016-04-13 14:53 ` Hyungwon Hwang @ 2016-04-13 15:01 ` Takashi Iwai 0 siblings, 0 replies; 4+ messages in thread From: Takashi Iwai @ 2016-04-13 15:01 UTC (permalink / raw) To: Hyungwon Hwang; +Cc: alsa-devel 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 _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-04-13 15:01 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-04-13 0:27 [PATCH] ALSA: hda - Update HDMI monitor_present from a temporary structure Hyungwon Hwang 2016-04-13 7:33 ` Takashi Iwai 2016-04-13 14:53 ` Hyungwon Hwang 2016-04-13 15:01 ` Takashi Iwai
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.