[Intel-gfx] [PATCH v7 3/4] drm/i915: Haswell HDMI audio initialization

[Imre Deak]

On Thu, Aug 16, 2012 at 4:23 PM, Wang, Xingchao <xingchao.wang at intel.com> wrote:
>
>
>> -----Original Message-----
>> From: Imre Deak [mailto:imre.deak at gmail.com]
>> Sent: Thursday, August 16, 2012 6:54 PM
>> To: Wang, Xingchao
>> Cc: daniel at ffwll.ch; intel-gfx at lists.freedesktop.org; przanoni at gmail.com
>> Subject: Re: [PATCH v7 3/4] drm/i915: Haswell HDMI audio initialization
>>
>> On Thu, Aug 16, 2012 at 6:45 AM, Wang Xingchao <xingchao.wang at intel.com>
>> wrote:
>> > 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:
>> >> [...]
>> >> >> +     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.
>>
>> On new HW it shouldn't matter which port is used to transmit the DIP data for
>> the ELD configuration. Earlier in the code you have picked the ELD_VALID_X bit
>> based on the pipe:
>>
>> tmp |= (AUDIO_ELD_VALID_A << (pipe * 4));
>>
>> and written this to the AUD_PIN_ELD_CP_VLD register . Here you pick the
>> same bit based on the port:
>>
>> eldv = AUDIO_ELD_VALID_A << ((i - 1) * 4);
>>
>> and write this to the same AUD_PIN_ELD_CP_VLD register. The definition from
>> the spec for this register is the same though in both cases: the ELD valid bit
>> should be picked based on the transcoder, no matter what port is used to
>> transfer the data.
>
> Thanks for clarification. As it's pipe/transcoder basis for ELD bits on Haswell, I agree just set the specific active transcoder,
> For older hardware, we just left the code there, is that okay for you?

Yes, I presume then that all older HW versions (including
DevHSW:GT0:X0) are handled in another stub function.

>> I cannot test this right now, since I don't have an HSW machine set up here.
>> Could you Wang give a try to the following diff on top of your patch?
>
> I tested your patch and it just works well as before. It's more clear now.

Thanks for your explanations, it's also clearer for me now :)

> I will add your change to patch and send it to Daniel, is that okay?

Yes, it's ok, you can also add my r-b then.

--Imre