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 ? > > > + > > + /* 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 at intel.com Thanks your review. :) --xingchao
