Re: [PATCH v3] drm/i915: rewrite hsw/bdw audio codec enable/disable sequences

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 30 Oct 2014, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> On Tue, Oct 28, 2014 at 5:03 AM, Jani Nikula <jani.nikula@intel.com> wrote:
>> There's some serious confusion regarding ELD valid bit that gets set and
>> cleared back and forth etc. Rewrite it all based on the documented audio
>> codec enable/disable sequences.
>>
>> v3: replace vblank wait with a comment
>>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_audio.c | 110 ++++++++++++++++---------------------
>>  1 file changed, 46 insertions(+), 64 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
>> index 821514c95229..2e7d42878b9d 100644
>> --- a/drivers/gpu/drm/i915/intel_audio.c
>> +++ b/drivers/gpu/drm/i915/intel_audio.c
>> @@ -132,14 +132,26 @@ static void g4x_audio_codec_enable(struct drm_connector *connector,
>>
>>  static void hsw_audio_codec_disable(struct intel_encoder *encoder)
>>  {
>> -       struct drm_device *dev = encoder->base.dev;
>> -       struct drm_i915_private *dev_priv = dev->dev_private;
>> -       struct drm_crtc *crtc = encoder->base.crtc;
>> -       enum pipe pipe = to_intel_crtc(crtc)->pipe;
>> +       struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
>> +       struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
>> +       enum pipe pipe = intel_crtc->pipe;
>>         uint32_t tmp;
>>
>> +       DRM_DEBUG_KMS("Disable audio codec on pipe %c\n", pipe_name(pipe));
>> +
>> +       /* Disable timestamps */
>> +       tmp = I915_READ(HSW_AUD_CFG(pipe));
>> +       tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
>> +       tmp |= AUD_CONFIG_N_PROG_ENABLE;
>> +       tmp &= ~AUD_CONFIG_UPPER_N_MASK;
>> +       tmp &= ~AUD_CONFIG_LOWER_N_MASK;
>> +       if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT))
>> +               tmp |= AUD_CONFIG_N_VALUE_INDEX;
>> +       I915_WRITE(HSW_AUD_CFG(pipe), tmp);
>> +
>> +       /* Invalidate ELD */
>>         tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
>> -       tmp &= ~((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) << (pipe * 4));
>> +       tmp &= ~(AUDIO_ELD_VALID_A << (pipe * 4));
>>         I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
>>  }
>>
>> @@ -149,77 +161,47 @@ static void hsw_audio_codec_enable(struct drm_connector *connector,
>>  {
>>         struct drm_i915_private *dev_priv = connector->dev->dev_private;
>>         struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
>> -       uint8_t *eld = connector->eld;
>> -       uint32_t eldv;
>> +       enum pipe pipe = intel_crtc->pipe;
>> +       const uint8_t *eld = connector->eld;
>>         uint32_t tmp;
>>         int len, i;
>> -       enum pipe pipe = intel_crtc->pipe;
>> -       enum port port;
>> -       int hdmiw_hdmiedid = HSW_AUD_EDID_DATA(pipe);
>> -       int aud_cntl_st = HSW_AUD_DIP_ELD_CTRL(pipe);
>> -       int aud_config = HSW_AUD_CFG(pipe);
>> -       int aud_cntrl_st2 = HSW_AUD_PIN_ELD_CP_VLD;
>>
>> -       /* Audio output enable */
>> -       DRM_DEBUG_DRIVER("HDMI audio: enable codec\n");
>> -       tmp = I915_READ(aud_cntrl_st2);
>> -       tmp |= (AUDIO_OUTPUT_ENABLE_A << (pipe * 4));
>> -       I915_WRITE(aud_cntrl_st2, tmp);
>> -       POSTING_READ(aud_cntrl_st2);
>> -
>> -       /* Set ELD valid state */
>> -       tmp = I915_READ(aud_cntrl_st2);
>> -       DRM_DEBUG_DRIVER("HDMI audio: pin eld vld status=0x%08x\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%08x\n", tmp);
>> -
>> -       /* Enable HDMI mode */
>> -       tmp = I915_READ(aud_config);
>> -       DRM_DEBUG_DRIVER("HDMI audio: audio conf: 0x%08x\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_KMS("Enable audio codec on pipe %c, %u bytes ELD\n",
>> +                     pipe_name(pipe), eld[2]);
>>
>> -       DRM_DEBUG_DRIVER("ELD on pipe %c\n", pipe_name(pipe));
>> -
>> -       eldv = AUDIO_ELD_VALID_A << (pipe * 4);
>> -
>> -       if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT))
>> -               I915_WRITE(aud_config, AUD_CONFIG_N_VALUE_INDEX); /* 0x1 = DP */
>> -       else
>> -               I915_WRITE(aud_config, audio_config_hdmi_pixel_clock(mode));
>> -
>> -       if (intel_eld_uptodate(connector,
>> -                              aud_cntrl_st2, eldv,
>> -                              aud_cntl_st, IBX_ELD_ADDRESS_MASK,
>> -                              hdmiw_hdmiedid))
>> -               return;
>> +       /* Enable audio presence detect, invalidate ELD */
>> +       tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
>> +       tmp |= AUDIO_OUTPUT_ENABLE_A << (pipe * 4);
>> +       tmp &= ~(AUDIO_ELD_VALID_A << (pipe * 4));
>> +       I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
>>
>> -       tmp = I915_READ(aud_cntrl_st2);
>> -       tmp &= ~eldv;
>> -       I915_WRITE(aud_cntrl_st2, tmp);
>> +       /* XXX: vblank wait here */
>
> the warns doesn't tell us we are still doing this too soon?

We have drm_crtc_vblank_off very early in the disable sequence, and
drm_crtc_vblank_on very late in the enable sequence. Especially
early/late since

commit 4b3a9526fc3228e74011b88f58088336acd2c9e2
Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
Date:   Thu Aug 14 22:04:37 2014 +0300

    drm/i915: Move vblank enable earlier and disable later

All of the audio codec enable/disable stuff, like most of mode set, is
done with vblank disabled.

Since

commit 44bd93a3d367913d883be6abba9a6e51a53c4e90
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Fri Jul 25 23:36:44 2014 +0200

    drm/i915: Use generic vblank wait

we started using drm_wait_one_vblank instead of directly checking the
registers in intel_wait_for_vblank, and this specifically disallows its
use during mode set (and IIRC we've seen bug reports about that too).

I don't know what the correct answer should be here. I've ensured the
audio codec enable doesn't happen until after the port/pipe are up and
running, but I still can't vblank wait.

>
>>
>> -       tmp = I915_READ(aud_cntl_st);
>> +       /* Reset ELD write address */
>> +       tmp = I915_READ(HSW_AUD_DIP_ELD_CTRL(pipe));
>>         tmp &= ~IBX_ELD_ADDRESS_MASK;
>> -       I915_WRITE(aud_cntl_st, tmp);
>> -       port = (tmp >> 29) & DIP_PORT_SEL_MASK;         /* DIP_Port_Select, 0x1 = PortB */
>> -       DRM_DEBUG_DRIVER("port num:%d\n", port);
>> +       I915_WRITE(HSW_AUD_DIP_ELD_CTRL(pipe), tmp);
>
> This  set isn't required by spec. At least not on audio enable codec step.
> Where does it come from?

The "ELD access address" is the the dword offset in the ELD buffer to be
accessed when HSW_AUD_EDID_DATA is read/written. If you want to write
the whole ELD, you need to reset the start address to zero first...

>
>>
>> -       len = min_t(int, eld[2], 21);   /* 84 bytes of hw ELD buffer */
>> -       DRM_DEBUG_DRIVER("ELD size %d\n", len);
>> +       /* Up to 84 bytes of hw ELD buffer */
>> +       len = min_t(int, eld[2], 21);
>>         for (i = 0; i < len; i++)
>> -               I915_WRITE(hdmiw_hdmiedid, *((uint32_t *)eld + i));
>> -
>> -       tmp = I915_READ(aud_cntrl_st2);
>> -       tmp |= eldv;
>> -       I915_WRITE(aud_cntrl_st2, tmp);
>> +               I915_WRITE(HSW_AUD_EDID_DATA(pipe), *((uint32_t *)eld + i));
>
> This edid set isn't at audio codec enable sequence as well.

...and you have to write the whole ELD because "Load ELD buffer and
Enable ELDV" is there in the codec enable sequence!

BR,
Jani.

>
>>
>> -       /* XXX: Transitional */
>> +       /* ELD valid */
>>         tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
>> -       tmp |= ((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) << (pipe * 4));
>> +       tmp |= AUDIO_ELD_VALID_A << (pipe * 4);
>>         I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
>> +
>> +       /* Enable timestamps */
>> +       tmp = I915_READ(HSW_AUD_CFG(pipe));
>> +       tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
>> +       tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
>> +       tmp &= ~AUD_CONFIG_PIXEL_CLOCK_HDMI_MASK;
>> +       if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT))
>> +               tmp |= AUD_CONFIG_N_VALUE_INDEX;
>> +       else
>> +               tmp |= audio_config_hdmi_pixel_clock(mode);
>> +       I915_WRITE(HSW_AUD_CFG(pipe), tmp);
>>  }
>>
>>  static void ilk_audio_codec_enable(struct drm_connector *connector,
>> --
>> 2.1.1
>>
>
>
>
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx