| Message ID | s5hoaa726ja.wl-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Takashi, > -----Original Message----- > From: Takashi Iwai [mailto:tiwai@suse.de] > Sent: Monday, March 21, 2016 11:30 PM > To: Yang, Libin > Cc: libin.yang@linux.intel.com; intel-gfx@lists.freedesktop.org; > conselvan2@gmail.com; jani.nikula@linux.intel.com; > ville.syrjala@linux.intel.com; Vetter, Daniel > Subject: Re: [PATCH] drm/i915: intel_audio clear eld buf when > disconnecting monitor > > On Mon, 21 Mar 2016 16:19:31 +0100, > Takashi Iwai wrote: > > > > On Mon, 21 Mar 2016 16:12:05 +0100, > > Takashi Iwai wrote: > > > > > > On Mon, 21 Mar 2016 16:00:21 +0100, > > > Takashi Iwai wrote: > > > > > > > > On Mon, 21 Mar 2016 15:17:37 +0100, > > > > Yang, Libin wrote: > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > From: Takashi Iwai [mailto:tiwai@suse.de] > > > > > > Sent: Monday, March 21, 2016 6:45 PM > > > > > > To: libin.yang@linux.intel.com > > > > > > Cc: intel-gfx@lists.freedesktop.org; conselvan2@gmail.com; > > > > > > jani.nikula@linux.intel.com; ville.syrjala@linux.intel.com; Vetter, > Daniel; > > > > > > tiwai@suse.de; Yang, Libin > > > > > > Subject: Re: [PATCH] drm/i915: intel_audio clear eld buf when > > > > > > disconnecting monitor > > > > > > > > > > > > On Mon, 21 Mar 2016 06:03:29 +0100, > > > > > > libin.yang@linux.intel.com wrote: > > > > > > > > > > > > > > From: Libin Yang <libin.yang@linux.intel.com> > > > > > > > > > > > > > > When disconnecting monitor, dev_priv->dig_port_map[port] > > > > > > > will be set NULL, which causes eld will not be updated in > > > > > > > i915_audio_component_get_eld(). > > > > > > > > > > > > > > This patch clears the eld buf when dev_priv- > >dig_port_map[port] > > > > > > > is NULL. > > > > > > > > > > > > > > Signed-off-by: Libin Yang <libin.yang@linux.intel.com> > > > > > > > > > > > > While this isn't certainly bad, I don't think it's mandatory. The > > > > > > function returns zero, i.e. no data is copied. So the caller > > > > > > shouldn't expect that the buffer is cleared in this case. > > > > > > > > > > Without the patch, we find when unplug the monitor, the eld info > > > > > will not be updated. The means the eld info in the procfs still > remains > > > > > the old info after the monitor is disconnected. > > > > > > > > Well, it's not about zero-clear but rather because the function > > > > returns an error (-EINVAL), and the caller takes it too seriously. > > > > Upon receiving an error code, the HDA driver doesn't read ELD at all, > > > > so it won't help even if you do zero-clear there. > > > > > > > > The alternative fix patch is below. > > > > > > ... or we can fix it in HDA side like below, too. > > > > This patch won't be applied cleanly on the upstream tree as I wrote it > > on the my working tree, sorry. Below is the revised one that is > > applicable to upstream tree. > > Gah, a typo was included in this patch, too. Sorry, the correct one > is attached below. Yes. The patch seems more reasonable than mine. Do we still need the patch to set i915_audio_component_get_eld() return value to 0? It seems -EINVAL makes sense. Regards, Libin > > > Takashi -- obviously needs a coffee break > > -- 8< -- > From: Takashi Iwai <tiwai@suse.de> > Subject: [PATCH v3] ALSA: hda - Fix missing ELD update at unplugging > > i915 get_eld ops may return an error when no encoder is connected, and > currently we regard the error as fatal and skip the whole ELD > handling. This ended up with the missing ELD update at unplugging. > > This patch fixes the issue by treating the error as the unplugged > state, instead of skipping the rest. > > Reported-by: Libin Yang <libin.yang@linux.intel.com> > Cc: <stable@vger.kernel.org> # v4.5 > Signed-off-by: Takashi Iwai <tiwai@suse.de> > --- > sound/pci/hda/patch_hdmi.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c > index 7ae614d27954..5af372d01834 100644 > --- a/sound/pci/hda/patch_hdmi.c > +++ b/sound/pci/hda/patch_hdmi.c > @@ -1484,11 +1484,10 @@ static void sync_eld_via_acomp(struct > hda_codec *codec, > int size; > > mutex_lock(&per_pin->lock); > + eld->monitor_present = false; > size = snd_hdac_acomp_get_eld(&codec->bus->core, per_pin- > >pin_nid, > &eld->monitor_present, eld- > >eld_buffer, > ELD_MAX_SIZE); > - if (size < 0) > - goto unlock; > if (size > 0) { > size = min(size, ELD_MAX_SIZE); > if (snd_hdmi_parse_eld(codec, &eld->info, > -- > 2.7.4
On Tue, 22 Mar 2016 02:47:14 +0100, Yang, Libin wrote: > > Hi Takashi, > > > -----Original Message----- > > From: Takashi Iwai [mailto:tiwai@suse.de] > > Sent: Monday, March 21, 2016 11:30 PM > > To: Yang, Libin > > Cc: libin.yang@linux.intel.com; intel-gfx@lists.freedesktop.org; > > conselvan2@gmail.com; jani.nikula@linux.intel.com; > > ville.syrjala@linux.intel.com; Vetter, Daniel > > Subject: Re: [PATCH] drm/i915: intel_audio clear eld buf when > > disconnecting monitor > > > > On Mon, 21 Mar 2016 16:19:31 +0100, > > Takashi Iwai wrote: > > > > > > On Mon, 21 Mar 2016 16:12:05 +0100, > > > Takashi Iwai wrote: > > > > > > > > On Mon, 21 Mar 2016 16:00:21 +0100, > > > > Takashi Iwai wrote: > > > > > > > > > > On Mon, 21 Mar 2016 15:17:37 +0100, > > > > > Yang, Libin wrote: > > > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > > From: Takashi Iwai [mailto:tiwai@suse.de] > > > > > > > Sent: Monday, March 21, 2016 6:45 PM > > > > > > > To: libin.yang@linux.intel.com > > > > > > > Cc: intel-gfx@lists.freedesktop.org; conselvan2@gmail.com; > > > > > > > jani.nikula@linux.intel.com; ville.syrjala@linux.intel.com; Vetter, > > Daniel; > > > > > > > tiwai@suse.de; Yang, Libin > > > > > > > Subject: Re: [PATCH] drm/i915: intel_audio clear eld buf when > > > > > > > disconnecting monitor > > > > > > > > > > > > > > On Mon, 21 Mar 2016 06:03:29 +0100, > > > > > > > libin.yang@linux.intel.com wrote: > > > > > > > > > > > > > > > > From: Libin Yang <libin.yang@linux.intel.com> > > > > > > > > > > > > > > > > When disconnecting monitor, dev_priv->dig_port_map[port] > > > > > > > > will be set NULL, which causes eld will not be updated in > > > > > > > > i915_audio_component_get_eld(). > > > > > > > > > > > > > > > > This patch clears the eld buf when dev_priv- > > >dig_port_map[port] > > > > > > > > is NULL. > > > > > > > > > > > > > > > > Signed-off-by: Libin Yang <libin.yang@linux.intel.com> > > > > > > > > > > > > > > While this isn't certainly bad, I don't think it's mandatory. The > > > > > > > function returns zero, i.e. no data is copied. So the caller > > > > > > > shouldn't expect that the buffer is cleared in this case. > > > > > > > > > > > > Without the patch, we find when unplug the monitor, the eld info > > > > > > will not be updated. The means the eld info in the procfs still > > remains > > > > > > the old info after the monitor is disconnected. > > > > > > > > > > Well, it's not about zero-clear but rather because the function > > > > > returns an error (-EINVAL), and the caller takes it too seriously. > > > > > Upon receiving an error code, the HDA driver doesn't read ELD at all, > > > > > so it won't help even if you do zero-clear there. > > > > > > > > > > The alternative fix patch is below. > > > > > > > > ... or we can fix it in HDA side like below, too. > > > > > > This patch won't be applied cleanly on the upstream tree as I wrote it > > > on the my working tree, sorry. Below is the revised one that is > > > applicable to upstream tree. > > > > Gah, a typo was included in this patch, too. Sorry, the correct one > > is attached below. > > Yes. The patch seems more reasonable than mine. > > Do we still need the patch to set i915_audio_component_get_eld() > return value to 0? It seems -EINVAL makes sense. Yes, we can drop i915 change. I queued my fix for HD-audio side for the next pull request. thanks, Takashi
On Tue, Mar 22, 2016 at 08:15:42AM +0100, Takashi Iwai wrote: > On Tue, 22 Mar 2016 02:47:14 +0100, > Yang, Libin wrote: > > > > Hi Takashi, > > > > > -----Original Message----- > > > From: Takashi Iwai [mailto:tiwai@suse.de] > > > Sent: Monday, March 21, 2016 11:30 PM > > > To: Yang, Libin > > > Cc: libin.yang@linux.intel.com; intel-gfx@lists.freedesktop.org; > > > conselvan2@gmail.com; jani.nikula@linux.intel.com; > > > ville.syrjala@linux.intel.com; Vetter, Daniel > > > Subject: Re: [PATCH] drm/i915: intel_audio clear eld buf when > > > disconnecting monitor > > > > > > On Mon, 21 Mar 2016 16:19:31 +0100, > > > Takashi Iwai wrote: > > > > > > > > On Mon, 21 Mar 2016 16:12:05 +0100, > > > > Takashi Iwai wrote: > > > > > > > > > > On Mon, 21 Mar 2016 16:00:21 +0100, > > > > > Takashi Iwai wrote: > > > > > > > > > > > > On Mon, 21 Mar 2016 15:17:37 +0100, > > > > > > Yang, Libin wrote: > > > > > > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > > > From: Takashi Iwai [mailto:tiwai@suse.de] > > > > > > > > Sent: Monday, March 21, 2016 6:45 PM > > > > > > > > To: libin.yang@linux.intel.com > > > > > > > > Cc: intel-gfx@lists.freedesktop.org; conselvan2@gmail.com; > > > > > > > > jani.nikula@linux.intel.com; ville.syrjala@linux.intel.com; Vetter, > > > Daniel; > > > > > > > > tiwai@suse.de; Yang, Libin > > > > > > > > Subject: Re: [PATCH] drm/i915: intel_audio clear eld buf when > > > > > > > > disconnecting monitor > > > > > > > > > > > > > > > > On Mon, 21 Mar 2016 06:03:29 +0100, > > > > > > > > libin.yang@linux.intel.com wrote: > > > > > > > > > > > > > > > > > > From: Libin Yang <libin.yang@linux.intel.com> > > > > > > > > > > > > > > > > > > When disconnecting monitor, dev_priv->dig_port_map[port] > > > > > > > > > will be set NULL, which causes eld will not be updated in > > > > > > > > > i915_audio_component_get_eld(). > > > > > > > > > > > > > > > > > > This patch clears the eld buf when dev_priv- > > > >dig_port_map[port] > > > > > > > > > is NULL. > > > > > > > > > > > > > > > > > > Signed-off-by: Libin Yang <libin.yang@linux.intel.com> > > > > > > > > > > > > > > > > While this isn't certainly bad, I don't think it's mandatory. The > > > > > > > > function returns zero, i.e. no data is copied. So the caller > > > > > > > > shouldn't expect that the buffer is cleared in this case. > > > > > > > > > > > > > > Without the patch, we find when unplug the monitor, the eld info > > > > > > > will not be updated. The means the eld info in the procfs still > > > remains > > > > > > > the old info after the monitor is disconnected. > > > > > > > > > > > > Well, it's not about zero-clear but rather because the function > > > > > > returns an error (-EINVAL), and the caller takes it too seriously. > > > > > > Upon receiving an error code, the HDA driver doesn't read ELD at all, > > > > > > so it won't help even if you do zero-clear there. > > > > > > > > > > > > The alternative fix patch is below. > > > > > > > > > > ... or we can fix it in HDA side like below, too. > > > > > > > > This patch won't be applied cleanly on the upstream tree as I wrote it > > > > on the my working tree, sorry. Below is the revised one that is > > > > applicable to upstream tree. > > > > > > Gah, a typo was included in this patch, too. Sorry, the correct one > > > is attached below. > > > > Yes. The patch seems more reasonable than mine. > > > > Do we still need the patch to set i915_audio_component_get_eld() > > return value to 0? It seems -EINVAL makes sense. > > Yes, we can drop i915 change. I queued my fix for HD-audio side for > the next pull request. Awesome, thx everyone for tracking this down. -Daniel
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 7ae614d27954..5af372d01834 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -1484,11 +1484,10 @@ static void sync_eld_via_acomp(struct hda_codec *codec, int size; mutex_lock(&per_pin->lock); + eld->monitor_present = false; size = snd_hdac_acomp_get_eld(&codec->bus->core, per_pin->pin_nid, &eld->monitor_present, eld->eld_buffer, ELD_MAX_SIZE); - if (size < 0) - goto unlock; if (size > 0) { size = min(size, ELD_MAX_SIZE); if (snd_hdmi_parse_eld(codec, &eld->info,