| Message ID | 1344502338-28371-4-git-send-email-xingchao.wang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, couple of comments inlined: On Thu, Aug 9, 2012 at 11:52 AM, Wang Xingchao <xingchao.wang@intel.com> wrote: > 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 | 94 +++++++++++++++++++++++++++++++++- > 2 files changed, 94 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..be0950d 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -5067,6 +5067,98 @@ 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; > + 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); uint32_t for tmp would be better, in case you happen to right shift tmp with bit31 set. > + tmp |= (AUDIO_OUTPUT_ENABLE_A | AUDIO_OUTPUT_ENABLE_B | AUDIO_OUTPUT_ENABLE_C); > + I915_WRITE(aud_cntrl_st2, tmp); I don't know much about HDMI, but according to the spec you'd need to wait for one vblank here. At least a comment why we don't do it would be nice. > + > + /* 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 | AUDIO_ELD_VALID_B | AUDIO_ELD_VALID_C); > + 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); This is confusing. Reading the spec I undersand i=1 means port B, but in that case we'll set eldv to AUDIO_ELD_VALID_A. Is it intentional? Other than the above, for the patchset: Reviewed-by: imre.deak@intel.com
HI Deak, > -----Original Message----- > From: Imre Deak [mailto:imre.deak@gmail.com] > Sent: Friday, August 10, 2012 9:15 PM > To: Wang, Xingchao > Cc: daniel@ffwll.ch; przanoni@gmail.com; intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH v6 3/4] drm/i915: Haswell HDMI audio > initialization > > Hi, > > couple of comments inlined: > > On Thu, Aug 9, 2012 at 11:52 AM, Wang Xingchao <xingchao.wang@intel.com> > wrote: > > 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 | 94 > +++++++++++++++++++++++++++++++++- > > 2 files changed, 94 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..be0950d 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -5067,6 +5067,98 @@ 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; > > + 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); > > uint32_t for tmp would be better, in case you happen to right shift tmp with > bit31 set. Agree, will change it to u32 type in next version. > > > + tmp |= (AUDIO_OUTPUT_ENABLE_A | AUDIO_OUTPUT_ENABLE_B > | AUDIO_OUTPUT_ENABLE_C); > > + I915_WRITE(aud_cntrl_st2, tmp); > > I don't know much about HDMI, but according to the spec you'd need to wait > for one vblank here. At least a comment why we don't do it would be nice. Yeah, according to audio programming sequence, wait 1 vertical blank here. I left it empty here because I did not found the proper api and did not meet issue during test. Is intel_wait_for_vblank() the proper one I should call ? > > > + > > + /* 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 | AUDIO_ELD_VALID_B | > AUDIO_ELD_VALID_C); > > + 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); > > This is confusing. Reading the spec I undersand i=1 means port B, but in that > case we'll set eldv to AUDIO_ELD_VALID_A. Is it intentional? Bit 0 is for ELD Valid A in Project DevHSW and for ELD Valid B for HSW-X0, and HSW-X0 is old one which doesnot need to support it in new code. > > Other than the above, for the patchset: > > Reviewed-by: imre.deak@intel.com Thanks your review. :) --xingchao
On Fri, Aug 10, 2012 at 5:52 PM, Wang, Xingchao <xingchao.wang@intel.com> wrote: > HI Deak, > >> -----Original Message----- >> From: Imre Deak [mailto:imre.deak@gmail.com] >> Sent: Friday, August 10, 2012 9:15 PM >> To: Wang, Xingchao >> Cc: daniel@ffwll.ch; przanoni@gmail.com; intel-gfx@lists.freedesktop.org >> Subject: Re: [Intel-gfx] [PATCH v6 3/4] drm/i915: Haswell HDMI audio >> initialization >> >> Hi, >> >> couple of comments inlined: >> >> On Thu, Aug 9, 2012 at 11:52 AM, Wang Xingchao <xingchao.wang@intel.com> >> wrote: >> > 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 | 94 >> +++++++++++++++++++++++++++++++++- >> > 2 files changed, 94 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..be0950d 100644 >> > --- a/drivers/gpu/drm/i915/intel_display.c >> > +++ b/drivers/gpu/drm/i915/intel_display.c >> > @@ -5067,6 +5067,98 @@ 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; >> > + 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); >> >> uint32_t for tmp would be better, in case you happen to right shift tmp with >> bit31 set. > > Agree, will change it to u32 type in next version. > >> >> > + tmp |= (AUDIO_OUTPUT_ENABLE_A | AUDIO_OUTPUT_ENABLE_B >> | AUDIO_OUTPUT_ENABLE_C); >> > + I915_WRITE(aud_cntrl_st2, tmp); >> >> I don't know much about HDMI, but according to the spec you'd need to wait >> for one vblank here. At least a comment why we don't do it would be nice. > > Yeah, according to audio programming sequence, wait 1 vertical blank here. > I left it empty here because I did not found the proper api and did not meet issue during test. > Is intel_wait_for_vblank() the proper one I should call ? Yes, although it will work only in non-atomic context. Also the relevant pipe needs to be enabled otherwise you'll get a timeout. >> > + >> > + /* 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 | AUDIO_ELD_VALID_B | >> AUDIO_ELD_VALID_C); >> > + 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); >> >> This is confusing. Reading the spec I undersand i=1 means port B, but in that >> case we'll set eldv to AUDIO_ELD_VALID_A. Is it intentional? > > Bit 0 is for ELD Valid A in Project DevHSW and for ELD Valid B for HSW-X0, and HSW-X0 is old one which > doesnot need to support it in new code. Yes, I noticed this bit field renaming through HW versions, but I still don't see how the above code works. So according to the devHSW register definition the above code will select one of the following values for AUD_PIN_ELD_CP_VLD valid bits based on AUD_DIP_ELD_CTRL_ST[DIP port select]: 0 [Reserved]: AUDIO_ELD_VALID_A | AUDIO_ELD_VALID_B | AUDIO_ELD_VALID_C 1 [Port B]: AUDIO_ELD_VALID_A 2 [Port C]: AUDIO_ELD_VALID_B 3 [Port D]: AUDIO_ELD_VALID_C Could you explain why the code uses AUDIO_ELD_VALID_A for 'Port B' for instance. --Imre
> -----Original Message----- > From: Imre Deak [mailto:imre.deak@gmail.com] > Sent: Tuesday, August 14, 2012 8:36 PM > To: Wang, Xingchao > Cc: daniel@ffwll.ch; przanoni@gmail.com; intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH v6 3/4] drm/i915: Haswell HDMI audio > initialization > > On Fri, Aug 10, 2012 at 5:52 PM, Wang, Xingchao <xingchao.wang@intel.com> > wrote: > > HI Deak, > > > >> -----Original Message----- > >> From: Imre Deak [mailto:imre.deak@gmail.com] > >> Sent: Friday, August 10, 2012 9:15 PM > >> To: Wang, Xingchao > >> Cc: daniel@ffwll.ch; przanoni@gmail.com; > >> intel-gfx@lists.freedesktop.org > >> Subject: Re: [Intel-gfx] [PATCH v6 3/4] drm/i915: Haswell HDMI audio > >> initialization > >> > >> Hi, > >> > >> couple of comments inlined: > >> > >> On Thu, Aug 9, 2012 at 11:52 AM, Wang Xingchao > >> <xingchao.wang@intel.com> > >> wrote: > >> > 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 | 94 > >> +++++++++++++++++++++++++++++++++- > >> > 2 files changed, 94 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..be0950d 100644 > >> > --- a/drivers/gpu/drm/i915/intel_display.c > >> > +++ b/drivers/gpu/drm/i915/intel_display.c > >> > @@ -5067,6 +5067,98 @@ 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; > >> > + 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); > >> > >> uint32_t for tmp would be better, in case you happen to right shift > >> tmp with > >> bit31 set. > > > > Agree, will change it to u32 type in next version. > > > >> > >> > + tmp |= (AUDIO_OUTPUT_ENABLE_A | > AUDIO_OUTPUT_ENABLE_B > >> | AUDIO_OUTPUT_ENABLE_C); > >> > + I915_WRITE(aud_cntrl_st2, tmp); > >> > >> I don't know much about HDMI, but according to the spec you'd need to > >> wait for one vblank here. At least a comment why we don't do it would be > nice. > > > > Yeah, according to audio programming sequence, wait 1 vertical blank here. > > I left it empty here because I did not found the proper api and did not meet > issue during test. > > Is intel_wait_for_vblank() the proper one I should call ? > > Yes, although it will work only in non-atomic context. Also the relevant pipe > needs to be enabled otherwise you'll get a timeout. Okay, I made the change. Haswell_write_eld() is called after ironlake_crtc_enable() which pipe was enabled there, so I think it's safe. > > >> > + > >> > + /* 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 | AUDIO_ELD_VALID_B | > >> AUDIO_ELD_VALID_C); > >> > + 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); > >> > >> This is confusing. Reading the spec I undersand i=1 means port B, but > >> in that case we'll set eldv to AUDIO_ELD_VALID_A. Is it intentional? > > > > Bit 0 is for ELD Valid A in Project DevHSW and for ELD Valid B for > > HSW-X0, and HSW-X0 is old one which doesnot need to support it in new > code. > > Yes, I noticed this bit field renaming through HW versions, but I still don't see > how the above code works. > So according to the devHSW register definition the above code will select one of > the following values for AUD_PIN_ELD_CP_VLD valid bits based on > AUD_DIP_ELD_CTRL_ST[DIP port select]: > > 0 [Reserved]: AUDIO_ELD_VALID_A | AUDIO_ELD_VALID_B | > AUDIO_ELD_VALID_C > 1 [Port B]: AUDIO_ELD_VALID_A > 2 [Port C]: AUDIO_ELD_VALID_B > 3 [Port D]: AUDIO_ELD_VALID_C > > Could you explain why the code uses AUDIO_ELD_VALID_A for 'Port B' for > instance. For DevHSW-X0(old), ELD and CP ready bit are port based, which match your above description. ((TranscoderA was portB, TranscoderB was portC and TranscoderC was portD) But for DevHSW-A0+: "Display software sets these bits as part of enabling the respective audio-enabled digital device/transcoder. To support DP MST, these bits are transcoder based and harfware will use it appropriately to send the status to the audio driver using device widgets." So we use AUDIO_ELD_VALID_A just means it's for Transcoder A, not 'Port B'. In the code, I just operate blindly on all ports because there's a bug to read out current selected DIP port from AUD_DIP_ELD_CTRL_ST(bits 29:30), it's the same issue as Ivybridge, these bits are always 00b(reserved). As a workround I could enable pipe related transcoder and set the right ELD/CP accordingly. Thanks your review, Imre. --xingchao
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..be0950d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5067,6 +5067,98 @@ 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; + 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); + + /* 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 | AUDIO_ELD_VALID_B | AUDIO_ELD_VALID_C); + 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 +7133,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 | 94 +++++++++++++++++++++++++++++++++- 2 files changed, 94 insertions(+), 1 deletion(-)