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

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

On Wed, 29 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
>
> Why did you remove the vblank wait? seemed correct by spec you
> pointed... at least looking to BDW one...

The problem is, we can't do that call in modeset path without getting a
warning, IIUC this is since we moved to the drm vblank wait
call. Daniel?

BR,
Jani.




>
>
>>
>> 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 */
>>
>> -       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);
>>
>> -       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));
>>
>> -       /* 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

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