[PATCH v7 3/4] drm/i915: Haswell HDMI audio initialization

[PATCH v7 3/4] drm/i915: Haswell HDMI audio initialization

[Wang Xingchao]

Added new haswell_write_eld() to initialize Haswell HDMI audio registers
to generate an unsolicited response to the audio controller driver to
indicate that the controller sequence should start.

Signed-off-by: Wang Xingchao <xingchao.wang@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h      |    1 +
 drivers/gpu/drm/i915/intel_display.c |   98 +++++++++++++++++++++++++++++++++-
 2 files changed, 98 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 55aa10e..08f8b65 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4294,6 +4294,7 @@
 #define   AUD_DIG_CNVT(pipe) _PIPE(pipe, \
 					HSW_AUD_DIG_CNVT_1, \
 					HSW_AUD_DIG_CNVT_2)
+#define   DIP_PORT_SEL_MASK		0x3
 
 #define   HSW_AUD_EDID_DATA_A		0x65050
 #define   HSW_AUD_EDID_DATA_B		0x65150
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 70d30fc..aad0a1b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5067,6 +5067,102 @@ static void g4x_write_eld(struct drm_connector *connector,
 	I915_WRITE(G4X_AUD_CNTL_ST, i);
 }
 
+static void haswell_write_eld(struct drm_connector *connector,
+				     struct drm_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = connector->dev->dev_private;
+	uint8_t *eld = connector->eld;
+	struct drm_device *dev = crtc->dev;
+	uint32_t eldv;
+	uint32_t i;
+	int len;
+	int pipe = to_intel_crtc(crtc)->pipe;
+	int tmp;
+
+	int hdmiw_hdmiedid = HSW_AUD_EDID_DATA(pipe);
+	int aud_cntl_st = HSW_AUD_DIP_ELD_CTRL(pipe);
+	int aud_config = HSW_AUD_CFG(pipe);
+	int aud_cntrl_st2 = HSW_AUD_PIN_ELD_CP_VLD;
+
+
+	DRM_DEBUG_DRIVER("HDMI: Haswell Audio initialize....\n");
+
+	/* Audio output enable */
+	DRM_DEBUG_DRIVER("HDMI audio: enable codec\n");
+	tmp = I915_READ(aud_cntrl_st2);
+	tmp |= (AUDIO_OUTPUT_ENABLE_A | AUDIO_OUTPUT_ENABLE_B | AUDIO_OUTPUT_ENABLE_C);
+	I915_WRITE(aud_cntrl_st2, tmp);
+
+	/* Wait for 1 vertical blank */
+	intel_wait_for_vblank(dev, pipe);
+
+	/* Set ELD valid state */
+	tmp = I915_READ(aud_cntrl_st2);
+	DRM_DEBUG_DRIVER("HDMI audio: pin eld vld status=0x%8x\n", tmp);
+	tmp |= (AUDIO_ELD_VALID_A << (pipe * 4));
+	I915_WRITE(aud_cntrl_st2, tmp);
+	tmp = I915_READ(aud_cntrl_st2);
+	DRM_DEBUG_DRIVER("HDMI audio: eld vld status=0x%8x\n", tmp);
+
+	/* Enable HDMI mode */
+	tmp = I915_READ(aud_config);
+	DRM_DEBUG_DRIVER("HDMI audio: audio conf: 0x%8x\n", tmp);
+	/* clear N_programing_enable and N_value_index */
+	tmp &= ~(AUD_CONFIG_N_VALUE_INDEX | AUD_CONFIG_N_PROG_ENABLE);
+	I915_WRITE(aud_config, tmp);
+
+	DRM_DEBUG_DRIVER("ELD on pipe %c\n", pipe_name(pipe));
+
+	i = I915_READ(aud_cntl_st);
+	i = (i >> 29) & DIP_PORT_SEL_MASK;		/* DIP_Port_Select, 0x1 = PortB */
+	if (!i) {
+		DRM_DEBUG_DRIVER("Audio directed to unknown port\n");
+		/* operate blindly on all ports */
+		eldv = AUDIO_ELD_VALID_A;
+		eldv |= AUDIO_ELD_VALID_B;
+		eldv |= AUDIO_ELD_VALID_C;
+	} else {
+		DRM_DEBUG_DRIVER("ELD on port %c\n", 'A' + i);
+		eldv = AUDIO_ELD_VALID_A << ((i - 1) * 4);
+	}
+
+	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT)) {
+		DRM_DEBUG_DRIVER("ELD: DisplayPort detected\n");
+		eld[5] |= (1 << 2);	/* Conn_Type, 0x1 = DisplayPort */
+		I915_WRITE(aud_config, AUD_CONFIG_N_VALUE_INDEX); /* 0x1 = DP */
+	} else
+		I915_WRITE(aud_config, 0);
+
+	if (intel_eld_uptodate(connector,
+			       aud_cntrl_st2, eldv,
+			       aud_cntl_st, IBX_ELD_ADDRESS,
+			       hdmiw_hdmiedid))
+		return;
+
+	i = I915_READ(aud_cntrl_st2);
+	i &= ~eldv;
+	I915_WRITE(aud_cntrl_st2, i);
+
+	if (!eld[0])
+		return;
+
+	i = I915_READ(aud_cntl_st);
+	i &= ~IBX_ELD_ADDRESS;
+	I915_WRITE(aud_cntl_st, i);
+	i = (i >> 29) & DIP_PORT_SEL_MASK;		/* DIP_Port_Select, 0x1 = PortB */
+	DRM_DEBUG_DRIVER("port num:%d\n", i);
+
+	len = min_t(uint8_t, eld[2], 21);	/* 84 bytes of hw ELD buffer */
+	DRM_DEBUG_DRIVER("ELD size %d\n", len);
+	for (i = 0; i < len; i++)
+		I915_WRITE(hdmiw_hdmiedid, *((uint32_t *)eld + i));
+
+	i = I915_READ(aud_cntrl_st2);
+	i |= eldv;
+	I915_WRITE(aud_cntrl_st2, i);
+
+}
+
 static void ironlake_write_eld(struct drm_connector *connector,
 				     struct drm_crtc *crtc)
 {
@@ -7041,7 +7137,7 @@ static void intel_init_display(struct drm_device *dev)
 			dev_priv->display.write_eld = ironlake_write_eld;
 		} else if (IS_HASWELL(dev)) {
 			dev_priv->display.fdi_link_train = hsw_fdi_link_train;
-			dev_priv->display.write_eld = ironlake_write_eld;
+			dev_priv->display.write_eld = haswell_write_eld;
 		} else
 			dev_priv->display.update_wm = NULL;
 	} else if (IS_G4X(dev)) {
-- 
1.7.9.5

Re: [PATCH v7 3/4] drm/i915: Haswell HDMI audio initialization

[Wang, Xingchao]

Hi Daniel/Imre,

This revised version changelog:
-  add " Wait for 1 vertical blank" after enable audio output port
-  configure pipe related transcoder instead of operate all transcoders blindly

Thanks
--xingchao

> -----Original Message-----
> From: Wang, Xingchao
> Sent: Wednesday, August 15, 2012 11:11 AM
> To: daniel@ffwll.ch; imre.deak@gmail.com
> Cc: intel-gfx@lists.freedesktop.org; przanoni@gmail.com; Wang, Xingchao
> Subject: [PATCH v7 3/4] drm/i915: Haswell HDMI audio initialization
> 
> Added new haswell_write_eld() to initialize Haswell HDMI audio registers to
> generate an unsolicited response to the audio controller driver to indicate that
> the controller sequence should start.
> 
> Signed-off-by: Wang Xingchao <xingchao.wang@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      |    1 +
>  drivers/gpu/drm/i915/intel_display.c |   98
> +++++++++++++++++++++++++++++++++-
>  2 files changed, 98 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h index 55aa10e..08f8b65 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4294,6 +4294,7 @@
>  #define   AUD_DIG_CNVT(pipe) _PIPE(pipe, \
>  					HSW_AUD_DIG_CNVT_1, \
>  					HSW_AUD_DIG_CNVT_2)
> +#define   DIP_PORT_SEL_MASK		0x3
> 
>  #define   HSW_AUD_EDID_DATA_A		0x65050
>  #define   HSW_AUD_EDID_DATA_B		0x65150
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 70d30fc..aad0a1b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5067,6 +5067,102 @@ static void g4x_write_eld(struct drm_connector
> *connector,
>  	I915_WRITE(G4X_AUD_CNTL_ST, i);
>  }
> 
> +static void haswell_write_eld(struct drm_connector *connector,
> +				     struct drm_crtc *crtc)
> +{
> +	struct drm_i915_private *dev_priv = connector->dev->dev_private;
> +	uint8_t *eld = connector->eld;
> +	struct drm_device *dev = crtc->dev;
> +	uint32_t eldv;
> +	uint32_t i;
> +	int len;
> +	int pipe = to_intel_crtc(crtc)->pipe;
> +	int tmp;
> +
> +	int hdmiw_hdmiedid = HSW_AUD_EDID_DATA(pipe);
> +	int aud_cntl_st = HSW_AUD_DIP_ELD_CTRL(pipe);
> +	int aud_config = HSW_AUD_CFG(pipe);
> +	int aud_cntrl_st2 = HSW_AUD_PIN_ELD_CP_VLD;
> +
> +
> +	DRM_DEBUG_DRIVER("HDMI: Haswell Audio initialize....\n");
> +
> +	/* Audio output enable */
> +	DRM_DEBUG_DRIVER("HDMI audio: enable codec\n");
> +	tmp = I915_READ(aud_cntrl_st2);
> +	tmp |= (AUDIO_OUTPUT_ENABLE_A | AUDIO_OUTPUT_ENABLE_B |
> AUDIO_OUTPUT_ENABLE_C);
> +	I915_WRITE(aud_cntrl_st2, tmp);
> +
> +	/* Wait for 1 vertical blank */
> +	intel_wait_for_vblank(dev, pipe);
> +
> +	/* Set ELD valid state */
> +	tmp = I915_READ(aud_cntrl_st2);
> +	DRM_DEBUG_DRIVER("HDMI audio: pin eld vld status=0x%8x\n", tmp);
> +	tmp |= (AUDIO_ELD_VALID_A << (pipe * 4));
> +	I915_WRITE(aud_cntrl_st2, tmp);
> +	tmp = I915_READ(aud_cntrl_st2);
> +	DRM_DEBUG_DRIVER("HDMI audio: eld vld status=0x%8x\n", tmp);
> +
> +	/* Enable HDMI mode */
> +	tmp = I915_READ(aud_config);
> +	DRM_DEBUG_DRIVER("HDMI audio: audio conf: 0x%8x\n", tmp);
> +	/* clear N_programing_enable and N_value_index */
> +	tmp &= ~(AUD_CONFIG_N_VALUE_INDEX |
> AUD_CONFIG_N_PROG_ENABLE);
> +	I915_WRITE(aud_config, tmp);
> +
> +	DRM_DEBUG_DRIVER("ELD on pipe %c\n", pipe_name(pipe));
> +
> +	i = I915_READ(aud_cntl_st);
> +	i = (i >> 29) & DIP_PORT_SEL_MASK;		/* DIP_Port_Select, 0x1 =
> PortB */
> +	if (!i) {
> +		DRM_DEBUG_DRIVER("Audio directed to unknown port\n");
> +		/* operate blindly on all ports */
> +		eldv = AUDIO_ELD_VALID_A;
> +		eldv |= AUDIO_ELD_VALID_B;
> +		eldv |= AUDIO_ELD_VALID_C;
> +	} else {
> +		DRM_DEBUG_DRIVER("ELD on port %c\n", 'A' + i);
> +		eldv = AUDIO_ELD_VALID_A << ((i - 1) * 4);
> +	}
> +
> +	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT)) {
> +		DRM_DEBUG_DRIVER("ELD: DisplayPort detected\n");
> +		eld[5] |= (1 << 2);	/* Conn_Type, 0x1 = DisplayPort */
> +		I915_WRITE(aud_config, AUD_CONFIG_N_VALUE_INDEX); /* 0x1 =
> DP */
> +	} else
> +		I915_WRITE(aud_config, 0);
> +
> +	if (intel_eld_uptodate(connector,
> +			       aud_cntrl_st2, eldv,
> +			       aud_cntl_st, IBX_ELD_ADDRESS,
> +			       hdmiw_hdmiedid))
> +		return;
> +
> +	i = I915_READ(aud_cntrl_st2);
> +	i &= ~eldv;
> +	I915_WRITE(aud_cntrl_st2, i);
> +
> +	if (!eld[0])
> +		return;
> +
> +	i = I915_READ(aud_cntl_st);
> +	i &= ~IBX_ELD_ADDRESS;
> +	I915_WRITE(aud_cntl_st, i);
> +	i = (i >> 29) & DIP_PORT_SEL_MASK;		/* DIP_Port_Select, 0x1 =
> PortB */
> +	DRM_DEBUG_DRIVER("port num:%d\n", i);
> +
> +	len = min_t(uint8_t, eld[2], 21);	/* 84 bytes of hw ELD buffer */
> +	DRM_DEBUG_DRIVER("ELD size %d\n", len);
> +	for (i = 0; i < len; i++)
> +		I915_WRITE(hdmiw_hdmiedid, *((uint32_t *)eld + i));
> +
> +	i = I915_READ(aud_cntrl_st2);
> +	i |= eldv;
> +	I915_WRITE(aud_cntrl_st2, i);
> +
> +}
> +
>  static void ironlake_write_eld(struct drm_connector *connector,
>  				     struct drm_crtc *crtc)
>  {
> @@ -7041,7 +7137,7 @@ static void intel_init_display(struct drm_device
> *dev)
>  			dev_priv->display.write_eld = ironlake_write_eld;
>  		} else if (IS_HASWELL(dev)) {
>  			dev_priv->display.fdi_link_train = hsw_fdi_link_train;
> -			dev_priv->display.write_eld = ironlake_write_eld;
> +			dev_priv->display.write_eld = haswell_write_eld;
>  		} else
>  			dev_priv->display.update_wm = NULL;
>  	} else if (IS_G4X(dev)) {
> --
> 1.7.9.5

Re: [PATCH v7 3/4] drm/i915: Haswell HDMI audio initialization

[Imre Deak]

On Wed, Aug 15, 2012 at 6:27 AM, Wang, Xingchao <xingchao.wang@intel.com> wrote:
> Hi Daniel/Imre,
>
> This revised version changelog:
> -  add " Wait for 1 vertical blank" after enable audio output port
> -  configure pipe related transcoder instead of operate all transcoders blindly

Thanks for the explanation. I'd have still a couple of questions inlined:

>
> Thanks
> --xingchao
>
>> -----Original Message-----
>> From: Wang, Xingchao
>> Sent: Wednesday, August 15, 2012 11:11 AM
>> To: daniel@ffwll.ch; imre.deak@gmail.com
>> Cc: intel-gfx@lists.freedesktop.org; przanoni@gmail.com; Wang, Xingchao
>> Subject: [PATCH v7 3/4] drm/i915: Haswell HDMI audio initialization
>>
>> Added new haswell_write_eld() to initialize Haswell HDMI audio registers to
>> generate an unsolicited response to the audio controller driver to indicate that
>> the controller sequence should start.
>>
>> Signed-off-by: Wang Xingchao <xingchao.wang@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h      |    1 +
>>  drivers/gpu/drm/i915/intel_display.c |   98
>> +++++++++++++++++++++++++++++++++-
>>  2 files changed, 98 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h index 55aa10e..08f8b65 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -4294,6 +4294,7 @@
>>  #define   AUD_DIG_CNVT(pipe) _PIPE(pipe, \
>>                                       HSW_AUD_DIG_CNVT_1, \
>>                                       HSW_AUD_DIG_CNVT_2)
>> +#define   DIP_PORT_SEL_MASK          0x3
>>
>>  #define   HSW_AUD_EDID_DATA_A                0x65050
>>  #define   HSW_AUD_EDID_DATA_B                0x65150
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 70d30fc..aad0a1b 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -5067,6 +5067,102 @@ static void g4x_write_eld(struct drm_connector
>> *connector,
>>       I915_WRITE(G4X_AUD_CNTL_ST, i);
>>  }
>>
>> +static void haswell_write_eld(struct drm_connector *connector,
>> +                                  struct drm_crtc *crtc)
>> +{
>> +     struct drm_i915_private *dev_priv = connector->dev->dev_private;
>> +     uint8_t *eld = connector->eld;
>> +     struct drm_device *dev = crtc->dev;
>> +     uint32_t eldv;
>> +     uint32_t i;
>> +     int len;
>> +     int pipe = to_intel_crtc(crtc)->pipe;
>> +     int tmp;
>> +
>> +     int hdmiw_hdmiedid = HSW_AUD_EDID_DATA(pipe);
>> +     int aud_cntl_st = HSW_AUD_DIP_ELD_CTRL(pipe);
>> +     int aud_config = HSW_AUD_CFG(pipe);
>> +     int aud_cntrl_st2 = HSW_AUD_PIN_ELD_CP_VLD;
>> +
>> +
>> +     DRM_DEBUG_DRIVER("HDMI: Haswell Audio initialize....\n");
>> +
>> +     /* Audio output enable */
>> +     DRM_DEBUG_DRIVER("HDMI audio: enable codec\n");
>> +     tmp = I915_READ(aud_cntrl_st2);
>> +     tmp |= (AUDIO_OUTPUT_ENABLE_A | AUDIO_OUTPUT_ENABLE_B |
>> AUDIO_OUTPUT_ENABLE_C);

The output bits are also transcoder specific, so why don't we enable
only the required output?
I.e. similarly as you do below tmp |= AUDIO_OUTPUT_ENABLE_A << (pipe *
4);. Otherwise
this function will leave also unrelated outputs enabled which is not
that nice power
management-wise.

>> +     I915_WRITE(aud_cntrl_st2, tmp);
>> +
>> +     /* Wait for 1 vertical blank */
>> +     intel_wait_for_vblank(dev, pipe);
>> +
>> +     /* Set ELD valid state */
>> +     tmp = I915_READ(aud_cntrl_st2);
>> +     DRM_DEBUG_DRIVER("HDMI audio: pin eld vld status=0x%8x\n", tmp);
>> +     tmp |= (AUDIO_ELD_VALID_A << (pipe * 4));
>> +     I915_WRITE(aud_cntrl_st2, tmp);
>> +     tmp = I915_READ(aud_cntrl_st2);
>> +     DRM_DEBUG_DRIVER("HDMI audio: eld vld status=0x%8x\n", tmp);
>> +
>> +     /* Enable HDMI mode */
>> +     tmp = I915_READ(aud_config);
>> +     DRM_DEBUG_DRIVER("HDMI audio: audio conf: 0x%8x\n", tmp);
>> +     /* clear N_programing_enable and N_value_index */
>> +     tmp &= ~(AUD_CONFIG_N_VALUE_INDEX |
>> AUD_CONFIG_N_PROG_ENABLE);
>> +     I915_WRITE(aud_config, tmp);
>> +
>> +     DRM_DEBUG_DRIVER("ELD on pipe %c\n", pipe_name(pipe));
>> +
>> +     i = I915_READ(aud_cntl_st);
>> +     i = (i >> 29) & DIP_PORT_SEL_MASK;              /* DIP_Port_Select, 0x1 =
>> PortB */
>> +     if (!i) {
>> +             DRM_DEBUG_DRIVER("Audio directed to unknown port\n");
>> +             /* operate blindly on all ports */
>> +             eldv = AUDIO_ELD_VALID_A;
>> +             eldv |= AUDIO_ELD_VALID_B;
>> +             eldv |= AUDIO_ELD_VALID_C;
>> +     } else {
>> +             DRM_DEBUG_DRIVER("ELD on port %c\n", 'A' + i);
>> +             eldv = AUDIO_ELD_VALID_A << ((i - 1) * 4);
>> +     }

Again, if we handle the ELD_VALID bits on a transcoder basis, as above
when enabling and
disabling it, why are we doing it here differently, on a port basis?
This should read then just
as above eldv = AUDIO_ELD_VALID_A << (pipe * 4);

As a sidenote I guess at the moment due to the bug you mentioned
DIP_PORT_SEL will
always read 0, hence the else branch never runs and we just enable
blindly all valid bits.

--Imre

Re: [PATCH v7 3/4] drm/i915: Haswell HDMI audio initialization

[Wang Xingchao]

On Wed, Aug 15, 2012 at 08:05:14PM +0300, Imre Deak wrote:
> On Wed, Aug 15, 2012 at 6:27 AM, Wang, Xingchao <xingchao.wang@intel.com> wrote:
> > Hi Daniel/Imre,
> >
> > This revised version changelog:
> > -  add " Wait for 1 vertical blank" after enable audio output port
> > -  configure pipe related transcoder instead of operate all transcoders blindly
> 
> Thanks for the explanation. I'd have still a couple of questions inlined:
> 
> >
> >> +
> >> +     /* Audio output enable */
> >> +     DRM_DEBUG_DRIVER("HDMI audio: enable codec\n");
> >> +     tmp = I915_READ(aud_cntrl_st2);
> >> +     tmp |= (AUDIO_OUTPUT_ENABLE_A | AUDIO_OUTPUT_ENABLE_B |
> >> AUDIO_OUTPUT_ENABLE_C);
> 
> The output bits are also transcoder specific, so why don't we enable
> only the required output?
> I.e. similarly as you do below tmp |= AUDIO_OUTPUT_ENABLE_A << (pipe *
> 4);. Otherwise
> this function will leave also unrelated outputs enabled which is not
> that nice power
> management-wise.

Thanks, makes sense. :)
I left the code without change because as the Audio enable sequence
description, there's no particular requirement to set the bit based on which
transcoder would be used. So i just enable *ALL* transcoders. Anyway your
suggestion makes sense, i should only enable the related transcoder. Wil make
that change in next version.
> 
> >> +     I915_WRITE(aud_cntrl_st2, tmp);
> >> +
> >> +     /* Wait for 1 vertical blank */
> >> +     intel_wait_for_vblank(dev, pipe);
> >> +
> >> +     /* Set ELD valid state */
> >> +     tmp = I915_READ(aud_cntrl_st2);
> >> +     DRM_DEBUG_DRIVER("HDMI audio: pin eld vld status=0x%8x\n", tmp);
> >> +     tmp |= (AUDIO_ELD_VALID_A << (pipe * 4));
> >> +     I915_WRITE(aud_cntrl_st2, tmp);
> >> +     tmp = I915_READ(aud_cntrl_st2);
> >> +     DRM_DEBUG_DRIVER("HDMI audio: eld vld status=0x%8x\n", tmp);
> >> +
> >> +     /* Enable HDMI mode */
> >> +     tmp = I915_READ(aud_config);
> >> +     DRM_DEBUG_DRIVER("HDMI audio: audio conf: 0x%8x\n", tmp);
> >> +     /* clear N_programing_enable and N_value_index */
> >> +     tmp &= ~(AUD_CONFIG_N_VALUE_INDEX |
> >> AUD_CONFIG_N_PROG_ENABLE);
> >> +     I915_WRITE(aud_config, tmp);
> >> +
> >> +     DRM_DEBUG_DRIVER("ELD on pipe %c\n", pipe_name(pipe));
> >> +
> >> +     i = I915_READ(aud_cntl_st);
> >> +     i = (i >> 29) & DIP_PORT_SEL_MASK;              /* DIP_Port_Select, 0x1 =
> >> PortB */
> >> +     if (!i) {
> >> +             DRM_DEBUG_DRIVER("Audio directed to unknown port\n");
> >> +             /* operate blindly on all ports */
> >> +             eldv = AUDIO_ELD_VALID_A;
> >> +             eldv |= AUDIO_ELD_VALID_B;
> >> +             eldv |= AUDIO_ELD_VALID_C;
> >> +     } else {
> >> +             DRM_DEBUG_DRIVER("ELD on port %c\n", 'A' + i);
> >> +             eldv = AUDIO_ELD_VALID_A << ((i - 1) * 4);
> >> +     }
> 
> Again, if we handle the ELD_VALID bits on a transcoder basis, as above
> when enabling and
> disabling it, why are we doing it here differently, on a port basis?

A bit different. These bits[30:29] reflects which port is used to transmit DIP
data. It's configured in other register, see PIPE_DDI_FUNC_CTL, that
determines which DDI port would be combined with current pipe. I think it's
also transcoder basis. please note aud_cntl_st is also "pipe" based.

> This should read then just
> as above eldv = AUDIO_ELD_VALID_A << (pipe * 4);
> 
> As a sidenote I guess at the moment due to the bug you mentioned
> DIP_PORT_SEL will
> always read 0, hence the else branch never runs and we just enable
> blindly all valid bits.
> 
> --Imre

Re: [PATCH v7 3/4] drm/i915: Haswell HDMI audio initialization

[Imre Deak]

On Thu, Aug 16, 2012 at 6:45 AM, Wang Xingchao <xingchao.wang@intel.com> wrote:
> On Wed, Aug 15, 2012 at 08:05:14PM +0300, Imre Deak wrote:
>> On Wed, Aug 15, 2012 at 6:27 AM, Wang, Xingchao <xingchao.wang@intel.com> wrote:
>> [...]
>> >> +     I915_WRITE(aud_cntrl_st2, tmp);
>> >> +
>> >> +     /* Wait for 1 vertical blank */
>> >> +     intel_wait_for_vblank(dev, pipe);
>> >> +
>> >> +     /* Set ELD valid state */
>> >> +     tmp = I915_READ(aud_cntrl_st2);
>> >> +     DRM_DEBUG_DRIVER("HDMI audio: pin eld vld status=0x%8x\n", tmp);
>> >> +     tmp |= (AUDIO_ELD_VALID_A << (pipe * 4));
>> >> +     I915_WRITE(aud_cntrl_st2, tmp);
>> >> +     tmp = I915_READ(aud_cntrl_st2);
>> >> +     DRM_DEBUG_DRIVER("HDMI audio: eld vld status=0x%8x\n", tmp);
>> >> +
>> >> +     /* Enable HDMI mode */
>> >> +     tmp = I915_READ(aud_config);
>> >> +     DRM_DEBUG_DRIVER("HDMI audio: audio conf: 0x%8x\n", tmp);
>> >> +     /* clear N_programing_enable and N_value_index */
>> >> +     tmp &= ~(AUD_CONFIG_N_VALUE_INDEX |
>> >> AUD_CONFIG_N_PROG_ENABLE);
>> >> +     I915_WRITE(aud_config, tmp);
>> >> +
>> >> +     DRM_DEBUG_DRIVER("ELD on pipe %c\n", pipe_name(pipe));
>> >> +
>> >> +     i = I915_READ(aud_cntl_st);
>> >> +     i = (i >> 29) & DIP_PORT_SEL_MASK;              /* DIP_Port_Select, 0x1 =
>> >> PortB */
>> >> +     if (!i) {
>> >> +             DRM_DEBUG_DRIVER("Audio directed to unknown port\n");
>> >> +             /* operate blindly on all ports */
>> >> +             eldv = AUDIO_ELD_VALID_A;
>> >> +             eldv |= AUDIO_ELD_VALID_B;
>> >> +             eldv |= AUDIO_ELD_VALID_C;
>> >> +     } else {
>> >> +             DRM_DEBUG_DRIVER("ELD on port %c\n", 'A' + i);
>> >> +             eldv = AUDIO_ELD_VALID_A << ((i - 1) * 4);
>> >> +     }
>>
>> Again, if we handle the ELD_VALID bits on a transcoder basis, as above
>> when enabling and
>> disabling it, why are we doing it here differently, on a port basis?
>
> A bit different. These bits[30:29] reflects which port is used to transmit DIP
> data. It's configured in other register, see PIPE_DDI_FUNC_CTL, that
> determines which DDI port would be combined with current pipe. I think it's
> also transcoder basis. please note aud_cntl_st is also "pipe" based.

On new HW it shouldn't matter which port is used to transmit the DIP
data for the ELD configuration. Earlier in the code you have picked
the ELD_VALID_X bit based on the pipe:

tmp |= (AUDIO_ELD_VALID_A << (pipe * 4));

and written this to the AUD_PIN_ELD_CP_VLD register . Here you pick
the same bit based on the port:

eldv = AUDIO_ELD_VALID_A << ((i - 1) * 4);

and write this to the same AUD_PIN_ELD_CP_VLD register. The definition
from the spec for this register is the same though in both cases: the
ELD valid bit should be picked based on the transcoder, no matter what
port is used to transfer the data.

I cannot test this right now, since I don't have an HSW machine set up
here. Could you Wang give a try to the following diff on top of your
patch?

diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 7a3339a..0776f71 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5113,18 +5113,7 @@ static void haswell_write_eld(struct
drm_connector *connector,

 	DRM_DEBUG_DRIVER("ELD on pipe %c\n", pipe_name(pipe));

-	i = I915_READ(aud_cntl_st);
-	i = (i >> 29) & DIP_PORT_SEL_MASK;		/* DIP_Port_Select, 0x1 = PortB */
-	if (!i) {
-		DRM_DEBUG_DRIVER("Audio directed to unknown port\n");
-		/* operate blindly on all ports */
-		eldv = AUDIO_ELD_VALID_A;
-		eldv |= AUDIO_ELD_VALID_B;
-		eldv |= AUDIO_ELD_VALID_C;
-	} else {
-		DRM_DEBUG_DRIVER("ELD on port %c\n", 'A' + i);
-		eldv = AUDIO_ELD_VALID_A << ((i - 1) * 4);
-	}
+	eldv = AUDIO_ELD_VALID_A << (pipe * 4);

 	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT)) {
 		DRM_DEBUG_DRIVER("ELD: DisplayPort detected\n");


Thanks,
Imre

Re: [PATCH v7 3/4] drm/i915: Haswell HDMI audio initialization

[Wang, Xingchao]

> -----Original Message-----
> From: Imre Deak [mailto:imre.deak@gmail.com]
> Sent: Thursday, August 16, 2012 6:54 PM
> To: Wang, Xingchao
> Cc: daniel@ffwll.ch; intel-gfx@lists.freedesktop.org; przanoni@gmail.com
> Subject: Re: [PATCH v7 3/4] drm/i915: Haswell HDMI audio initialization
> 
> On Thu, Aug 16, 2012 at 6:45 AM, Wang Xingchao <xingchao.wang@intel.com>
> wrote:
> > On Wed, Aug 15, 2012 at 08:05:14PM +0300, Imre Deak wrote:
> >> On Wed, Aug 15, 2012 at 6:27 AM, Wang, Xingchao
> <xingchao.wang@intel.com> wrote:
> >> [...]
> >> >> +     I915_WRITE(aud_cntrl_st2, tmp);
> >> >> +
> >> >> +     /* Wait for 1 vertical blank */
> >> >> +     intel_wait_for_vblank(dev, pipe);
> >> >> +
> >> >> +     /* Set ELD valid state */
> >> >> +     tmp = I915_READ(aud_cntrl_st2);
> >> >> +     DRM_DEBUG_DRIVER("HDMI audio: pin eld vld status=0x%8x\n",
> tmp);
> >> >> +     tmp |= (AUDIO_ELD_VALID_A << (pipe * 4));
> >> >> +     I915_WRITE(aud_cntrl_st2, tmp);
> >> >> +     tmp = I915_READ(aud_cntrl_st2);
> >> >> +     DRM_DEBUG_DRIVER("HDMI audio: eld vld status=0x%8x\n",
> tmp);
> >> >> +
> >> >> +     /* Enable HDMI mode */
> >> >> +     tmp = I915_READ(aud_config);
> >> >> +     DRM_DEBUG_DRIVER("HDMI audio: audio conf: 0x%8x\n", tmp);
> >> >> +     /* clear N_programing_enable and N_value_index */
> >> >> +     tmp &= ~(AUD_CONFIG_N_VALUE_INDEX |
> >> >> AUD_CONFIG_N_PROG_ENABLE);
> >> >> +     I915_WRITE(aud_config, tmp);
> >> >> +
> >> >> +     DRM_DEBUG_DRIVER("ELD on pipe %c\n", pipe_name(pipe));
> >> >> +
> >> >> +     i = I915_READ(aud_cntl_st);
> >> >> +     i = (i >> 29) & DIP_PORT_SEL_MASK;              /*
> DIP_Port_Select, 0x1 =
> >> >> PortB */
> >> >> +     if (!i) {
> >> >> +             DRM_DEBUG_DRIVER("Audio directed to unknown
> port\n");
> >> >> +             /* operate blindly on all ports */
> >> >> +             eldv = AUDIO_ELD_VALID_A;
> >> >> +             eldv |= AUDIO_ELD_VALID_B;
> >> >> +             eldv |= AUDIO_ELD_VALID_C;
> >> >> +     } else {
> >> >> +             DRM_DEBUG_DRIVER("ELD on port %c\n", 'A' + i);
> >> >> +             eldv = AUDIO_ELD_VALID_A << ((i - 1) * 4);
> >> >> +     }
> >>
> >> Again, if we handle the ELD_VALID bits on a transcoder basis, as
> >> above when enabling and disabling it, why are we doing it here
> >> differently, on a port basis?
> >
> > A bit different. These bits[30:29] reflects which port is used to
> > transmit DIP data. It's configured in other register, see
> > PIPE_DDI_FUNC_CTL, that determines which DDI port would be combined
> > with current pipe. I think it's also transcoder basis. please note aud_cntl_st is
> also "pipe" based.
> 
> On new HW it shouldn't matter which port is used to transmit the DIP data for
> the ELD configuration. Earlier in the code you have picked the ELD_VALID_X bit
> based on the pipe:
> 
> tmp |= (AUDIO_ELD_VALID_A << (pipe * 4));
> 
> and written this to the AUD_PIN_ELD_CP_VLD register . Here you pick the
> same bit based on the port:
> 
> eldv = AUDIO_ELD_VALID_A << ((i - 1) * 4);
> 
> and write this to the same AUD_PIN_ELD_CP_VLD register. The definition from
> the spec for this register is the same though in both cases: the ELD valid bit
> should be picked based on the transcoder, no matter what port is used to
> transfer the data.

Thanks for clarification. As it's pipe/transcoder basis for ELD bits on Haswell, I agree just set the specific active transcoder,
For older hardware, we just left the code there, is that okay for you?

> 
> I cannot test this right now, since I don't have an HSW machine set up here.
> Could you Wang give a try to the following diff on top of your patch?

I tested your patch and it just works well as before. It's more clear now.
I will add your change to patch and send it to Daniel, is that okay?

Thanks a lot, Imre. :)
--xingchao

> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 7a3339a..0776f71 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5113,18 +5113,7 @@ static void haswell_write_eld(struct
> drm_connector *connector,
> 
>  	DRM_DEBUG_DRIVER("ELD on pipe %c\n", pipe_name(pipe));
> 
> -	i = I915_READ(aud_cntl_st);
> -	i = (i >> 29) & DIP_PORT_SEL_MASK;		/* DIP_Port_Select, 0x1 =
> PortB */
> -	if (!i) {
> -		DRM_DEBUG_DRIVER("Audio directed to unknown port\n");
> -		/* operate blindly on all ports */
> -		eldv = AUDIO_ELD_VALID_A;
> -		eldv |= AUDIO_ELD_VALID_B;
> -		eldv |= AUDIO_ELD_VALID_C;
> -	} else {
> -		DRM_DEBUG_DRIVER("ELD on port %c\n", 'A' + i);
> -		eldv = AUDIO_ELD_VALID_A << ((i - 1) * 4);
> -	}
> +	eldv = AUDIO_ELD_VALID_A << (pipe * 4);
> 
>  	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT)) {
>  		DRM_DEBUG_DRIVER("ELD: DisplayPort detected\n");
> 
> 
> Thanks,
> Imre

Re: [PATCH v7 3/4] drm/i915: Haswell HDMI audio initialization

[Imre Deak]

On Thu, Aug 16, 2012 at 4:23 PM, Wang, Xingchao <xingchao.wang@intel.com> wrote:
>
>
>> -----Original Message-----
>> From: Imre Deak [mailto:imre.deak@gmail.com]
>> Sent: Thursday, August 16, 2012 6:54 PM
>> To: Wang, Xingchao
>> Cc: daniel@ffwll.ch; intel-gfx@lists.freedesktop.org; przanoni@gmail.com
>> Subject: Re: [PATCH v7 3/4] drm/i915: Haswell HDMI audio initialization
>>
>> On Thu, Aug 16, 2012 at 6:45 AM, Wang Xingchao <xingchao.wang@intel.com>
>> wrote:
>> > On Wed, Aug 15, 2012 at 08:05:14PM +0300, Imre Deak wrote:
>> >> On Wed, Aug 15, 2012 at 6:27 AM, Wang, Xingchao
>> <xingchao.wang@intel.com> wrote:
>> >> [...]
>> >> >> +     I915_WRITE(aud_cntrl_st2, tmp);
>> >> >> +
>> >> >> +     /* Wait for 1 vertical blank */
>> >> >> +     intel_wait_for_vblank(dev, pipe);
>> >> >> +
>> >> >> +     /* Set ELD valid state */
>> >> >> +     tmp = I915_READ(aud_cntrl_st2);
>> >> >> +     DRM_DEBUG_DRIVER("HDMI audio: pin eld vld status=0x%8x\n",
>> tmp);
>> >> >> +     tmp |= (AUDIO_ELD_VALID_A << (pipe * 4));
>> >> >> +     I915_WRITE(aud_cntrl_st2, tmp);
>> >> >> +     tmp = I915_READ(aud_cntrl_st2);
>> >> >> +     DRM_DEBUG_DRIVER("HDMI audio: eld vld status=0x%8x\n",
>> tmp);
>> >> >> +
>> >> >> +     /* Enable HDMI mode */
>> >> >> +     tmp = I915_READ(aud_config);
>> >> >> +     DRM_DEBUG_DRIVER("HDMI audio: audio conf: 0x%8x\n", tmp);
>> >> >> +     /* clear N_programing_enable and N_value_index */
>> >> >> +     tmp &= ~(AUD_CONFIG_N_VALUE_INDEX |
>> >> >> AUD_CONFIG_N_PROG_ENABLE);
>> >> >> +     I915_WRITE(aud_config, tmp);
>> >> >> +
>> >> >> +     DRM_DEBUG_DRIVER("ELD on pipe %c\n", pipe_name(pipe));
>> >> >> +
>> >> >> +     i = I915_READ(aud_cntl_st);
>> >> >> +     i = (i >> 29) & DIP_PORT_SEL_MASK;              /*
>> DIP_Port_Select, 0x1 =
>> >> >> PortB */
>> >> >> +     if (!i) {
>> >> >> +             DRM_DEBUG_DRIVER("Audio directed to unknown
>> port\n");
>> >> >> +             /* operate blindly on all ports */
>> >> >> +             eldv = AUDIO_ELD_VALID_A;
>> >> >> +             eldv |= AUDIO_ELD_VALID_B;
>> >> >> +             eldv |= AUDIO_ELD_VALID_C;
>> >> >> +     } else {
>> >> >> +             DRM_DEBUG_DRIVER("ELD on port %c\n", 'A' + i);
>> >> >> +             eldv = AUDIO_ELD_VALID_A << ((i - 1) * 4);
>> >> >> +     }
>> >>
>> >> Again, if we handle the ELD_VALID bits on a transcoder basis, as
>> >> above when enabling and disabling it, why are we doing it here
>> >> differently, on a port basis?
>> >
>> > A bit different. These bits[30:29] reflects which port is used to
>> > transmit DIP data. It's configured in other register, see
>> > PIPE_DDI_FUNC_CTL, that determines which DDI port would be combined
>> > with current pipe. I think it's also transcoder basis. please note aud_cntl_st is
>> also "pipe" based.
>>
>> On new HW it shouldn't matter which port is used to transmit the DIP data for
>> the ELD configuration. Earlier in the code you have picked the ELD_VALID_X bit
>> based on the pipe:
>>
>> tmp |= (AUDIO_ELD_VALID_A << (pipe * 4));
>>
>> and written this to the AUD_PIN_ELD_CP_VLD register . Here you pick the
>> same bit based on the port:
>>
>> eldv = AUDIO_ELD_VALID_A << ((i - 1) * 4);
>>
>> and write this to the same AUD_PIN_ELD_CP_VLD register. The definition from
>> the spec for this register is the same though in both cases: the ELD valid bit
>> should be picked based on the transcoder, no matter what port is used to
>> transfer the data.
>
> Thanks for clarification. As it's pipe/transcoder basis for ELD bits on Haswell, I agree just set the specific active transcoder,
> For older hardware, we just left the code there, is that okay for you?

Yes, I presume then that all older HW versions (including
DevHSW:GT0:X0) are handled in another stub function.

>> I cannot test this right now, since I don't have an HSW machine set up here.
>> Could you Wang give a try to the following diff on top of your patch?
>
> I tested your patch and it just works well as before. It's more clear now.

Thanks for your explanations, it's also clearer for me now :)

> I will add your change to patch and send it to Daniel, is that okay?

Yes, it's ok, you can also add my r-b then.

--Imre