drm/i915: Fix MST audio enabling

Message ID	1457710386-4466-1-git-send-email-ander.conselvan.de.oliveira@intel.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <intel-gfx-bounces@lists.freedesktop.org> From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> To: intel-gfx@lists.freedesktop.org Date: Fri, 11 Mar 2016 17:33:06 +0200 Message-Id: <1457710386-4466-1-git-send-email-ander.conselvan.de.oliveira@intel.com> Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>, Libin Yang <libin.yang@linux.intel.com>, drm-intel-fixes@lists.freedesktop.org Subject: [Intel-gfx] [PATCH] drm/i915: Fix MST audio enabling Precedence: list MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" <intel-gfx-bounces@lists.freedesktop.org>

Message ID

1457710386-4466-1-git-send-email-ander.conselvan.de.oliveira@intel.com (mailing list archive)

State

New, archived

Headers

From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
To: intel-gfx@lists.freedesktop.org
Date: Fri, 11 Mar 2016 17:33:06 +0200
Message-Id: <1457710386-4466-1-git-send-email-ander.conselvan.de.oliveira@intel.com>
Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>,
	Libin Yang <libin.yang@linux.intel.com>,
	drm-intel-fixes@lists.freedesktop.org
Subject: [Intel-gfx] [PATCH] drm/i915: Fix MST audio enabling
Precedence: list
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: base64
Errors-To: intel-gfx-bounces@lists.freedesktop.org
Sender: "Intel-gfx" <intel-gfx-bounces@lists.freedesktop.org>

Commit Message

Ander Conselvan de Oliveira March 11, 2016, 3:33 p.m. UTC

Commit 3d52ccf52f2c ("drm/i915: start adding dp mst audio") erroneously
added a check for MST encoder to ilk_audio_codec_enable instead of the
hsw+ equivalent. That causes the HDMI path to be used, resulting in a
wrong register value.

Cc: Libin Yang <libin.yang@linux.intel.com>
Cc: drm-intel-fixes@lists.freedesktop.org
Fixes: commit 3d52ccf52f2c ("drm/i915: start adding dp mst audio")
Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_audio.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Ville Syrjälä March 11, 2016, 3:37 p.m. UTC | #1

On Fri, Mar 11, 2016 at 05:33:06PM +0200, Ander Conselvan de Oliveira wrote:
> Commit 3d52ccf52f2c ("drm/i915: start adding dp mst audio") erroneously
> added a check for MST encoder to ilk_audio_codec_enable instead of the
> hsw+ equivalent. That causes the HDMI path to be used, resulting in a
> wrong register value.
> 
> Cc: Libin Yang <libin.yang@linux.intel.com>
> Cc: drm-intel-fixes@lists.freedesktop.org
> Fixes: commit 3d52ccf52f2c ("drm/i915: start adding dp mst audio")
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_audio.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index 30f9214..f5fccf7 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -329,7 +329,8 @@ static void hsw_audio_codec_enable(struct drm_connector *connector,
>  	tmp = I915_READ(HSW_AUD_CFG(pipe));
>  	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
>  	tmp &= ~AUD_CONFIG_PIXEL_CLOCK_HDMI_MASK;
> -	if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT))
> +	if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT) ||
> +	    intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DP_MST))

Humm. I thought had I asked all of these to be changed to has_dp_encoder.
I wonder if I imagined it or if I was just ignored.

>  		tmp |= AUD_CONFIG_N_VALUE_INDEX;
>  	else
>  		tmp |= audio_config_hdmi_pixel_clock(adjusted_mode);
> @@ -476,8 +477,7 @@ static void ilk_audio_codec_enable(struct drm_connector *connector,
>  	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
>  	tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
>  	tmp &= ~AUD_CONFIG_PIXEL_CLOCK_HDMI_MASK;
> -	if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT) ||
> -	    intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DP_MST))
> +	if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT))
>  		tmp |= AUD_CONFIG_N_VALUE_INDEX;
>  	else
>  		tmp |= audio_config_hdmi_pixel_clock(adjusted_mode);
> -- 
> 2.4.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Ander Conselvan de Oliveira March 11, 2016, 3:52 p.m. UTC | #2

On Fri, 2016-03-11 at 17:37 +0200, Ville Syrjälä wrote:
> On Fri, Mar 11, 2016 at 05:33:06PM +0200, Ander Conselvan de Oliveira wrote:
> > Commit 3d52ccf52f2c ("drm/i915: start adding dp mst audio") erroneously
> > added a check for MST encoder to ilk_audio_codec_enable instead of the
> > hsw+ equivalent. That causes the HDMI path to be used, resulting in a
> > wrong register value.
> > 
> > Cc: Libin Yang <libin.yang@linux.intel.com>
> > Cc: drm-intel-fixes@lists.freedesktop.org
> > Fixes: commit 3d52ccf52f2c ("drm/i915: start adding dp mst audio")
> > Signed-off-by: Ander Conselvan de Oliveira <
> > ander.conselvan.de.oliveira@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_audio.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> > b/drivers/gpu/drm/i915/intel_audio.c
> > index 30f9214..f5fccf7 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -329,7 +329,8 @@ static void hsw_audio_codec_enable(struct drm_connector
> > *connector,
> >  	tmp = I915_READ(HSW_AUD_CFG(pipe));
> >  	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
> >  	tmp &= ~AUD_CONFIG_PIXEL_CLOCK_HDMI_MASK;
> > -	if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT))
> > +	if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT) ||
> > +	    intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DP_MST))
> 
> Humm. I thought had I asked all of these to be changed to has_dp_encoder.
> I wonder if I imagined it or if I was just ignored.

Ah, yes. Libin sent a patch to fix the same bug and you asked for that [1] and
even R-b the follow up version even though it didn't use it everywhere. But it
looks like Daniel didn't merge it since he wanted all uses converted to
has_dp_encoders.

[1] https://patchwork.freedesktop.org/patch/68902/
[2] https://patchwork.freedesktop.org/patch/69525/


> 
> >  		tmp |= AUD_CONFIG_N_VALUE_INDEX;
> >  	else
> >  		tmp |= audio_config_hdmi_pixel_clock(adjusted_mode);
> > @@ -476,8 +477,7 @@ static void ilk_audio_codec_enable(struct drm_connector
> > *connector,
> >  	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
> >  	tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> >  	tmp &= ~AUD_CONFIG_PIXEL_CLOCK_HDMI_MASK;
> > -	if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT) ||
> > -	    intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DP_MST))
> > +	if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT))
> >  		tmp |= AUD_CONFIG_N_VALUE_INDEX;
> >  	else
> >  		tmp |= audio_config_hdmi_pixel_clock(adjusted_mode);
> > -- 
> > 2.4.3
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

Jani Nikula March 14, 2016, 6:40 a.m. UTC | #3

On Fri, 11 Mar 2016, Ander Conselvan De Oliveira <conselvan2@gmail.com> wrote:
> [ text/plain ]
> On Fri, 2016-03-11 at 17:37 +0200, Ville Syrjälä wrote:
>> On Fri, Mar 11, 2016 at 05:33:06PM +0200, Ander Conselvan de Oliveira wrote:
>> > Commit 3d52ccf52f2c ("drm/i915: start adding dp mst audio") erroneously
>> > added a check for MST encoder to ilk_audio_codec_enable instead of the
>> > hsw+ equivalent. That causes the HDMI path to be used, resulting in a
>> > wrong register value.
>> > 
>> > Cc: Libin Yang <libin.yang@linux.intel.com>
>> > Cc: drm-intel-fixes@lists.freedesktop.org
>> > Fixes: commit 3d52ccf52f2c ("drm/i915: start adding dp mst audio")
>> > Signed-off-by: Ander Conselvan de Oliveira <
>> > ander.conselvan.de.oliveira@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_audio.c | 6 +++---
>> >  1 file changed, 3 insertions(+), 3 deletions(-)
>> > 
>> > diff --git a/drivers/gpu/drm/i915/intel_audio.c
>> > b/drivers/gpu/drm/i915/intel_audio.c
>> > index 30f9214..f5fccf7 100644
>> > --- a/drivers/gpu/drm/i915/intel_audio.c
>> > +++ b/drivers/gpu/drm/i915/intel_audio.c
>> > @@ -329,7 +329,8 @@ static void hsw_audio_codec_enable(struct drm_connector
>> > *connector,
>> >  	tmp = I915_READ(HSW_AUD_CFG(pipe));
>> >  	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
>> >  	tmp &= ~AUD_CONFIG_PIXEL_CLOCK_HDMI_MASK;
>> > -	if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT))
>> > +	if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT) ||
>> > +	    intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DP_MST))
>> 
>> Humm. I thought had I asked all of these to be changed to has_dp_encoder.
>> I wonder if I imagined it or if I was just ignored.
>
> Ah, yes. Libin sent a patch to fix the same bug and you asked for that [1] and
> even R-b the follow up version even though it didn't use it everywhere. But it
> looks like Daniel didn't merge it since he wanted all uses converted to
> has_dp_encoders.

We want the minimal fix first now because by bikeshedding we let the bug
slip to v4.5, and we need cc: stable on this.

BR,
Jani.


>
> [1] https://patchwork.freedesktop.org/patch/68902/
> [2] https://patchwork.freedesktop.org/patch/69525/
>
>
>> 
>> >  		tmp |= AUD_CONFIG_N_VALUE_INDEX;
>> >  	else
>> >  		tmp |= audio_config_hdmi_pixel_clock(adjusted_mode);
>> > @@ -476,8 +477,7 @@ static void ilk_audio_codec_enable(struct drm_connector
>> > *connector,
>> >  	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
>> >  	tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
>> >  	tmp &= ~AUD_CONFIG_PIXEL_CLOCK_HDMI_MASK;
>> > -	if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT) ||
>> > -	    intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DP_MST))
>> > +	if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT))
>> >  		tmp |= AUD_CONFIG_N_VALUE_INDEX;
>> >  	else
>> >  		tmp |= audio_config_hdmi_pixel_clock(adjusted_mode);
>> > -- 
>> > 2.4.3
>> > 
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index 30f9214..f5fccf7 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -329,7 +329,8 @@  static void hsw_audio_codec_enable(struct drm_connector *connector,
 	tmp = I915_READ(HSW_AUD_CFG(pipe));
 	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
 	tmp &= ~AUD_CONFIG_PIXEL_CLOCK_HDMI_MASK;
-	if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT))
+	if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT) ||
+	    intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DP_MST))
 		tmp |= AUD_CONFIG_N_VALUE_INDEX;
 	else
 		tmp |= audio_config_hdmi_pixel_clock(adjusted_mode);
@@ -476,8 +477,7 @@  static void ilk_audio_codec_enable(struct drm_connector *connector,
 	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
 	tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
 	tmp &= ~AUD_CONFIG_PIXEL_CLOCK_HDMI_MASK;
-	if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT) ||
-	    intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DP_MST))
+	if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT))
 		tmp |= AUD_CONFIG_N_VALUE_INDEX;
 	else
 		tmp |= audio_config_hdmi_pixel_clock(adjusted_mode);

drm/i915: Fix MST audio enabling

Commit Message

Comments

Patch