On Fri, Aug 10, 2012 at 5:52 PM, Wang, Xingchao <xingchao.wang at intel.com> wrote: > HI Deak, > >> -----Original Message----- >> From: Imre Deak [mailto:imre.deak at gmail.com] >> Sent: Friday, August 10, 2012 9:15 PM >> To: Wang, Xingchao >> Cc: daniel at ffwll.ch; przanoni at gmail.com; intel-gfx at lists.freedesktop.org >> Subject: Re: [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 at 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 at 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
