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

2012-08-16 Thread Imre Deak 
On Thu, Aug 16, 2012 at 6:45 AM, Wang Xingchao xingchao.w...@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.w...@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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2012-08-16 Thread Wang, Xingchao 


 -Original Message-
 From: Imre Deak [mailto:imre.d...@gmail.com]
 Sent: Thursday, August 16, 2012 6:54 PM
 To: Wang, Xingchao
 Cc: dan...@ffwll.ch; intel-gfx@lists.freedesktop.org; przan...@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.w...@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.w...@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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2012-08-16 Thread Imre Deak 
On Thu, Aug 16, 2012 at 4:23 PM, Wang, Xingchao xingchao.w...@intel.com wrote:


 -Original Message-
 From: Imre Deak [mailto:imre.d...@gmail.com]
 Sent: Thursday, August 16, 2012 6:54 PM
 To: Wang, Xingchao
 Cc: dan...@ffwll.ch; intel-gfx@lists.freedesktop.org; przan...@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.w...@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.w...@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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2012-08-15 Thread Imre Deak 
On Wed, Aug 15, 2012 at 6:27 AM, Wang, Xingchao xingchao.w...@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: dan...@ffwll.ch; imre.d...@gmail.com
 Cc: intel-gfx@lists.freedesktop.org; przan...@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.w...@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_A0x65050
  #define   HSW_AUD_EDID_DATA_B0x65150
 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org

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

2012-08-15 Thread 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.w...@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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2012-08-14 Thread 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: dan...@ffwll.ch; imre.d...@gmail.com
 Cc: intel-gfx@lists.freedesktop.org; przan...@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.w...@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_A0x65050
  #define   HSW_AUD_EDID_DATA_B0x65150
 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 =