> -----Original Message----- > From: Imre Deak [mailto:imre.deak at gmail.com] > Sent: Tuesday, August 14, 2012 8:36 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 > > 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. 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
