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