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 at 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
