| Message ID | 1345000265-6108-1-git-send-email-xingchao.wang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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)) {
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(-)