[Intel-gfx] [PATCH v6 3/4] drm/i915: Haswell HDMI audio initialization
Wang, Xingchao
xingchao.wang at intel.com
Wed Aug 15 04:46:14 CEST 2012
> -----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: [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 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: [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 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
More information about the Intel-gfx
mailing list