| Message ID | CAMDDuasForm3D7iGt4aei2wMakHq5kxvXB5Fn=UK0HE0yizxcw@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: Imre Deak [mailto:imre.deak@gmail.com] > Sent: Thursday, August 16, 2012 6:54 PM > To: Wang, Xingchao > Cc: daniel@ffwll.ch; intel-gfx@lists.freedesktop.org; przanoni@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@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. 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
On Thu, Aug 16, 2012 at 4:23 PM, Wang, Xingchao <xingchao.wang@intel.com> wrote: > > >> -----Original Message----- >> From: Imre Deak [mailto:imre.deak@gmail.com] >> Sent: Thursday, August 16, 2012 6:54 PM >> To: Wang, Xingchao >> Cc: daniel@ffwll.ch; intel-gfx@lists.freedesktop.org; przanoni@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@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. > > 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
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)) {