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

[Imre Deak <imre.deak@gmail.com>]

On Thu, Aug 16, 2012 at 6:45 AM, Wang Xingchao <xingchao.wang@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@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.

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?

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