Re: [PATCH v7 3/4] drm/i915: Haswell HDMI audio initialization

[Wang Xingchao <xingchao.wang@intel.com>]

On Wed, Aug 15, 2012 at 08:05:14PM +0300, Imre Deak wrote:
> On Wed, Aug 15, 2012 at 6:27 AM, Wang, Xingchao <xingchao.wang@intel.com> wrote:
> > Hi Daniel/Imre,
> >
> > This revised version changelog:
> > -  add " Wait for 1 vertical blank" after enable audio output port
> > -  configure pipe related transcoder instead of operate all transcoders blindly
> 
> Thanks for the explanation. I'd have still a couple of questions inlined:
> 
> >
> >> +
> >> +     /* 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);
> 
> The output bits are also transcoder specific, so why don't we enable
> only the required output?
> I.e. similarly as you do below tmp |= AUDIO_OUTPUT_ENABLE_A << (pipe *
> 4);. Otherwise
> this function will leave also unrelated outputs enabled which is not
> that nice power
> management-wise.

Thanks, makes sense. :)
I left the code without change because as the Audio enable sequence
description, there's no particular requirement to set the bit based on which
transcoder would be used. So i just enable *ALL* transcoders. Anyway your
suggestion makes sense, i should only enable the related transcoder. Wil make
that change in next version.
> 
> >> +     I915_WRITE(aud_cntrl_st2, tmp);
> >> +
> >> +     /* Wait for 1 vertical blank */
> >> +     intel_wait_for_vblank(dev, pipe);
> >> +
> >> +     /* 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 << (pipe * 4));
> >> +     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);
> >> +     }
> 
> Again, if we handle the ELD_VALID bits on a transcoder basis, as above
> when enabling and
> disabling it, why are we doing it here differently, on a port basis?

A bit different. These bits[30:29] reflects which port is used to transmit DIP
data. It's configured in other register, see PIPE_DDI_FUNC_CTL, that
determines which DDI port would be combined with current pipe. I think it's
also transcoder basis. please note aud_cntl_st is also "pipe" based.

> This should read then just
> as above eldv = AUDIO_ELD_VALID_A << (pipe * 4);
> 
> As a sidenote I guess at the moment due to the bug you mentioned
> DIP_PORT_SEL will
> always read 0, hence the else branch never runs and we just enable
> blindly all valid bits.
> 
> --Imre