[PATCH] drm/i915: intel_audio clear eld buf when disconnecting monitor

[PATCH] drm/i915: intel_audio clear eld buf when disconnecting monitor

[libin.yang]

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>
---
 drivers/gpu/drm/i915/intel_audio.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index fdc8b2a..9d3879e 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -732,6 +732,13 @@ static int i915_audio_component_get_eld(struct device *dev, int port,
 			ret = drm_eld_size(eld);
 			memcpy(buf, eld, min(max_bytes, ret));
 		}
+	} else {
+		/* dev_priv->dig_port_map[port] will be set NULL in
+		 * intel_audio_codec_disable when disconnecting monitor.
+		 * We need clear the eld buf.
+		 */
+		memset(buf, 0, max_bytes);
+		ret = 0;
 	}
 
 	mutex_unlock(&dev_priv->av_mutex);
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: intel_audio clear eld buf when disconnecting monitor

[Takashi Iwai]

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.

Actually, if we do zero-clear, a safer fix would be to do it at the
beginning.  The similar "leak" can be seen, as we do memcpy() only for
drm_eld_size() bytes at max.


thanks,

Takashi

> ---
>  drivers/gpu/drm/i915/intel_audio.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index fdc8b2a..9d3879e 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -732,6 +732,13 @@ static int i915_audio_component_get_eld(struct device *dev, int port,
>  			ret = drm_eld_size(eld);
>  			memcpy(buf, eld, min(max_bytes, ret));
>  		}
> +	} else {
> +		/* dev_priv->dig_port_map[port] will be set NULL in
> +		 * intel_audio_codec_disable when disconnecting monitor.
> +		 * We need clear the eld buf.
> +		 */
> +		memset(buf, 0, max_bytes);
> +		ret = 0;
>  	}
>  
>  	mutex_unlock(&dev_priv->av_mutex);
> -- 
> 1.9.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for drm/i915: intel_audio clear eld buf when disconnecting monitor

[Patchwork]

== Series Details ==

Series: drm/i915: intel_audio clear eld buf when disconnecting monitor
URL   : https://patchwork.freedesktop.org/series/4685/
State : warning

== Summary ==

Series 4685v1 drm/i915: intel_audio clear eld buf when disconnecting monitor
http://patchwork.freedesktop.org/api/1.0/series/4685/revisions/1/mbox/

Test gem_ringfill:
        Subgroup basic-default-s3:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                pass       -> DMESG-WARN (ilk-hp8440p) UNSTABLE
        Subgroup basic-flip-vs-wf_vblank:
                fail       -> PASS       (byt-nuc)
Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-a:
                dmesg-warn -> PASS       (snb-x220t)
        Subgroup read-crc-pipe-b-frame-sequence:
                pass       -> DMESG-WARN (snb-x220t)
        Subgroup suspend-read-crc-pipe-c:
                incomplete -> PASS       (hsw-gt2)
Test kms_setmode:
        Subgroup basic-clone-single-crtc:
                pass       -> DMESG-WARN (snb-dellxps)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> DMESG-WARN (bsw-nuc-2)
                dmesg-warn -> PASS       (snb-dellxps)
                pass       -> DMESG-WARN (byt-nuc)
        Subgroup basic-rte:
                pass       -> DMESG-WARN (snb-x220t)
                pass       -> DMESG-WARN (snb-dellxps)

bdw-nuci7        total:194  pass:182  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:194  pass:173  dwarn:0   dfail:0   fail:0   skip:21 
bsw-nuc-2        total:194  pass:156  dwarn:1   dfail:0   fail:0   skip:37 
byt-nuc          total:194  pass:158  dwarn:1   dfail:0   fail:0   skip:35 
hsw-brixbox      total:194  pass:172  dwarn:0   dfail:0   fail:0   skip:22 
hsw-gt2          total:194  pass:175  dwarn:2   dfail:0   fail:0   skip:17 
ilk-hp8440p      total:194  pass:130  dwarn:1   dfail:0   fail:0   skip:63 
ivb-t430s        total:194  pass:169  dwarn:0   dfail:0   fail:0   skip:25 
skl-i7k-2        total:194  pass:171  dwarn:0   dfail:0   fail:0   skip:23 
snb-dellxps      total:194  pass:158  dwarn:2   dfail:0   fail:0   skip:34 
snb-x220t        total:194  pass:158  dwarn:2   dfail:0   fail:1   skip:33 

Results at /archive/results/CI_IGT_test/Patchwork_1653/

e7a7673e9840fe8b50a5a2894c75565ec7858a00 drm-intel-nightly: 2016y-03m-19d-10h-09m-53s UTC integration manifest
ca71453a80d21b9b3b4647d994bdd2cd8d68565a drm/i915: intel_audio clear eld buf when disconnecting monitor

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: intel_audio clear eld buf when disconnecting monitor

[Yang, Libin]

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

> 
> Actually, if we do zero-clear, a safer fix would be to do it at the
> beginning.  The similar "leak" can be seen, as we do memcpy() only for
> drm_eld_size() bytes at max.

OK. I will clear at the beginning.

> 
> 
> thanks,
> 
> Takashi
> 
> > ---
> >  drivers/gpu/drm/i915/intel_audio.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> b/drivers/gpu/drm/i915/intel_audio.c
> > index fdc8b2a..9d3879e 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -732,6 +732,13 @@ static int
> i915_audio_component_get_eld(struct device *dev, int port,
> >  			ret = drm_eld_size(eld);
> >  			memcpy(buf, eld, min(max_bytes, ret));
> >  		}
> > +	} else {
> > +		/* dev_priv->dig_port_map[port] will be set NULL in
> > +		 * intel_audio_codec_disable when disconnecting
> monitor.
> > +		 * We need clear the eld buf.
> > +		 */
> > +		memset(buf, 0, max_bytes);
> > +		ret = 0;
> >  	}
> >
> >  	mutex_unlock(&dev_priv->av_mutex);
> > --
> > 1.9.1
> >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: intel_audio clear eld buf when disconnecting monitor

[Takashi Iwai]

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.


Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] drm/i915: Don't handle NULL encoder as an error in get_eld
 notifier

Since we fixed the dig_port_map[] assignment rather in a dynamic
manner by the commit [2f791908a70e: drm/i915: Fix bogus dig_port_map[]
assignment for pre-HSW], the NULL encoder is no longer an error but it
just indicates that the unconnected state.  However, in the caller
(HD-audio) side, it's taken as a serious error, and it leads to the
missing update of ELD after unplugging.

As a fix, this patch changes get_eld notifier to return zero and
initialize the enabled argument properly as false when no encoder is
mapped for indicating the unconnected state.

Fixes: 2f791908a70e ('drm/i915: Fix bogus dig_port_map[] assignment for pre-HSW')
Cc: <stable@vger.kernel.org> # v4.5
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/i915/intel_audio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index 31f6d212fb1b..63493a63500e 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -717,13 +717,13 @@ static int i915_audio_component_get_eld(struct device *dev, int port,
 	struct intel_encoder *intel_encoder;
 	struct intel_digital_port *intel_dig_port;
 	const u8 *eld;
-	int ret = -EINVAL;
+	int ret = 0;
 
+	*enabled = false;
 	mutex_lock(&dev_priv->av_mutex);
 	intel_encoder = dev_priv->dig_port_map[port];
 	/* intel_encoder might be NULL for DP MST */
 	if (intel_encoder) {
-		ret = 0;
 		intel_dig_port = enc_to_dig_port(&intel_encoder->base);
 		*enabled = intel_dig_port->audio_connector != NULL;
 		if (*enabled) {
-- 
2.7.4


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: intel_audio clear eld buf when disconnecting monitor

[Takashi Iwai]

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.
Both patches don't conflict.


Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] 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.

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 a4bda74cb4e4..cb382de1d11c 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -1485,11 +1485,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->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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: intel_audio clear eld buf when disconnecting monitor

[Takashi Iwai]

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.


Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH v2] 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..d9a557ea7a5b 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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: intel_audio clear eld buf when disconnecting monitor

[Takashi Iwai]

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.


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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] ALSA: hda - Fix missing ELD update at unplugging

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1431 bytes --]

Hi Takashi,

[auto build test ERROR on v4.5-rc7]
[also build test ERROR on next-20160321]
[cannot apply to sound/for-next]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Takashi-Iwai/ALSA-hda-Fix-missing-ELD-update-at-unplugging/20160322-045538
config: x86_64-randconfig-s5-03220710 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   sound/pci/hda/patch_hdmi.c: In function 'sync_eld_via_acomp':
>> sound/pci/hda/patch_hdmi.c:1668:2: error: invalid type argument of unary '*' (have 'int')
     *eld->monitor_present = false;
     ^
   sound/pci/hda/patch_hdmi.c:1690:2: warning: label 'unlock' defined but not used [-Wunused-label]
     unlock:
     ^

vim +1668 sound/pci/hda/patch_hdmi.c

  1662	{
  1663		struct hdmi_spec *spec = codec->spec;
  1664		struct hdmi_eld *eld = &spec->temp_eld;
  1665		int size;
  1666	
  1667		mutex_lock(&per_pin->lock);
> 1668		*eld->monitor_present = false;
  1669		size = snd_hdac_acomp_get_eld(&codec->bus->core, per_pin->pin_nid,
  1670					      &eld->monitor_present, eld->eld_buffer,
  1671					      ELD_MAX_SIZE);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 24635 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: intel_audio clear eld buf when disconnecting monitor

[Yang, Libin]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: intel_audio clear eld buf when disconnecting monitor

[Takashi Iwai]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: intel_audio clear eld buf when disconnecting monitor

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx