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

[Wang, Xingchao]

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

> 
> 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.
I will add your change to patch and send it to Daniel, is that okay?

Thanks a lot, Imre. :)
--xingchao

> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 7a3339a..0776f71 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5113,18 +5113,7 @@ static void haswell_write_eld(struct
> drm_connector *connector,
> 
>  	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);
> -	}
> +	eldv = AUDIO_ELD_VALID_A << (pipe * 4);
> 
>  	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT)) {
>  		DRM_DEBUG_DRIVER("ELD: DisplayPort detected\n");
> 
> 
> Thanks,
> Imre