Re: [PATCH v2 08/18] drm/i915: introduce intel_audio_codec_{enable, disable}

Rodrigo Vivi
2014-10-27T18:21:28Z

Good!
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

On Mon, Oct 27, 2014 at 7:26 AM, Jani Nikula <jani.nikula@intel.com> wrote:
> Introduce functions to enable/disable the audio codec, incorporating the
> ELD setup within enable. The disable is initially limited to HSW,
> covering exactly what was done previously.
>
> The only functional difference is that ELD valid is no longer set if
> there is no connector with ELD, which should be the right thing to do
> anyway. Otherwise the sequence remains the same, with warts and all, in
> preparation for applying more sanity.
>
> v2: add kernel doc.
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 7 ++--
> drivers/gpu/drm/i915/intel_audio.c | 83 +++++++++++++++++++++++++++++---------
> drivers/gpu/drm/i915/intel_ddi.c | 17 +-------
> drivers/gpu/drm/i915/intel_dp.c | 2 +-
> drivers/gpu/drm/i915/intel_drv.h | 3 +-
> drivers/gpu/drm/i915/intel_hdmi.c | 2 +-
> 6 files changed, 73 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6a73803482cb..0344fd561789 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -483,9 +483,10 @@ struct drm_i915_display_funcs {
> void (*crtc_enable)(struct drm_crtc *crtc);
> void (*crtc_disable)(struct drm_crtc *crtc);
> void (*off)(struct drm_crtc *crtc);
> - void (*write_eld)(struct drm_connector *connector,
> - struct intel_encoder *encoder,
> - struct drm_display_mode *mode);
> + void (*audio_codec_enable)(struct drm_connector *connector,
> + struct intel_encoder *encoder,
> + struct drm_display_mode *mode);
> + void (*audio_codec_disable)(struct intel_encoder *encoder);
> void (*fdi_link_train)(struct drm_crtc *crtc);
> void (*init_clock_gating)(struct drm_device *dev);
> int (*queue_flip)(struct drm_device *dev, struct drm_crtc *crtc,
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index b9d3a143dd8c..c38c62eaebad 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -96,9 +96,9 @@ static bool intel_eld_uptodate(struct drm_connector *connector,
> return true;
> }
>
> -static void g4x_write_eld(struct drm_connector *connector,
> - struct intel_encoder *encoder,
> - struct drm_display_mode *mode)
> +static void g4x_audio_codec_enable(struct drm_connector *connector,
> + struct intel_encoder *encoder,
> + struct drm_display_mode *mode)
> {
> struct drm_i915_private *dev_priv = connector->dev->dev_private;
> uint8_t *eld = connector->eld;
> @@ -136,9 +136,22 @@ static void g4x_write_eld(struct drm_connector *connector,
> I915_WRITE(G4X_AUD_CNTL_ST, tmp);
> }
>
> -static void haswell_write_eld(struct drm_connector *connector,
> - struct intel_encoder *encoder,
> - struct drm_display_mode *mode)
> +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;
> + uint32_t tmp;
> +
> + tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
> + tmp &= ~((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) << (pipe * 4));
> + I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
> +}
> +
> +static void hsw_audio_codec_enable(struct drm_connector *connector,
> + struct intel_encoder *encoder,
> + struct drm_display_mode *mode)
> {
> struct drm_i915_private *dev_priv = connector->dev->dev_private;
> struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> @@ -211,11 +224,16 @@ static void haswell_write_eld(struct drm_connector *connector,
> tmp = I915_READ(aud_cntrl_st2);
> tmp |= eldv;
> I915_WRITE(aud_cntrl_st2, tmp);
> +
> + /* XXX: Transitional */
> + tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
> + tmp |= ((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) << (pipe * 4));
> + I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
> }
>
> -static void ironlake_write_eld(struct drm_connector *connector,
> - struct intel_encoder *encoder,
> - struct drm_display_mode *mode)
> +static void ilk_audio_codec_enable(struct drm_connector *connector,
> + struct intel_encoder *encoder,
> + struct drm_display_mode *mode)
> {
> struct drm_i915_private *dev_priv = connector->dev->dev_private;
> struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> @@ -303,7 +321,14 @@ static void ironlake_write_eld(struct drm_connector *connector,
> I915_WRITE(aud_cntrl_st2, tmp);
> }
>
> -void intel_write_eld(struct intel_encoder *intel_encoder)
> +/**
> + * intel_audio_codec_enable - Enable the audio codec for HD audio
> + * @intel_encoder: encoder on which to enable audio
> + *
> + * The enable sequences may only be performed after enabling the transcoder and
> + * port, and after completed link training.
> + */
> +void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
> {
> struct drm_encoder *encoder = &intel_encoder->base;
> struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
> @@ -329,8 +354,24 @@ void intel_write_eld(struct intel_encoder *intel_encoder)
>
> connector->eld[6] = drm_av_sync_delay(connector, mode) / 2;
>
> - if (dev_priv->display.write_eld)
> - dev_priv->display.write_eld(connector, intel_encoder, mode);
> + if (dev_priv->display.audio_codec_enable)
> + dev_priv->display.audio_codec_enable(connector, intel_encoder, mode);
> +}
> +
> +/**
> + * intel_audio_codec_disable - Disable the audio codec for HD audio
> + * @encoder: encoder on which to disable audio
> + *
> + * The disable sequences must be performed before disabling the transcoder or
> + * port.
> + */
> +void intel_audio_codec_disable(struct intel_encoder *encoder)
> +{
> + struct drm_device *dev = encoder->base.dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> +
> + if (dev_priv->display.audio_codec_disable)
> + dev_priv->display.audio_codec_disable(encoder);
> }
>
> /**
> @@ -341,12 +382,14 @@ void intel_init_audio(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
>
> - if (IS_G4X(dev))
> - dev_priv->display.write_eld = g4x_write_eld;
> - else if (IS_VALLEYVIEW(dev))
> - dev_priv->display.write_eld = ironlake_write_eld;
> - else if (IS_HASWELL(dev) || INTEL_INFO(dev)->gen >= 8)
> - dev_priv->display.write_eld = haswell_write_eld;
> - else if (HAS_PCH_SPLIT(dev))
> - dev_priv->display.write_eld = ironlake_write_eld;
> + if (IS_G4X(dev)) {
> + dev_priv->display.audio_codec_enable = g4x_audio_codec_enable;
> + } else if (IS_VALLEYVIEW(dev)) {
> + dev_priv->display.audio_codec_enable = ilk_audio_codec_enable;
> + } else if (IS_HASWELL(dev) || INTEL_INFO(dev)->gen >= 8) {
> + dev_priv->display.audio_codec_enable = hsw_audio_codec_enable;
> + dev_priv->display.audio_codec_disable = hsw_audio_codec_disable;
> + } else if (HAS_PCH_SPLIT(dev)) {
> + dev_priv->display.audio_codec_enable = ilk_audio_codec_enable;
> + }
> }
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 56e7cb1ddc75..b182b9b80461 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1186,12 +1186,10 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder)
> struct drm_encoder *encoder = &intel_encoder->base;
> struct drm_crtc *crtc = encoder->crtc;
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - int pipe = intel_crtc->pipe;
> struct drm_device *dev = encoder->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> enum port port = intel_ddi_get_encoder_port(intel_encoder);
> int type = intel_encoder->type;
> - uint32_t tmp;
>
> if (type == INTEL_OUTPUT_HDMI) {
> struct intel_digital_port *intel_dig_port =
> @@ -1216,11 +1214,7 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder)
>
> if (intel_crtc->config.has_audio) {
> intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
> - intel_write_eld(intel_encoder);
> -
> - tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
> - tmp |= ((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) << (pipe * 4));
> - I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
> + intel_audio_codec_enable(intel_encoder);
> }
> }
>
> @@ -1229,19 +1223,12 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
> struct drm_encoder *encoder = &intel_encoder->base;
> struct drm_crtc *crtc = encoder->crtc;
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - int pipe = intel_crtc->pipe;
> int type = intel_encoder->type;
> struct drm_device *dev = encoder->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> - uint32_t tmp;
>
> - /* We can't touch HSW_AUD_PIN_ELD_CP_VLD uncionditionally because this
> - * register is part of the power well on Haswell. */
> if (intel_crtc->config.has_audio) {
> - tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
> - tmp &= ~((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) <<
> - (pipe * 4));
> - I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
> + intel_audio_codec_disable(intel_encoder);
> intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO);
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index eafe9f598a03..299c606a9b3c 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1250,7 +1250,7 @@ static void intel_dp_prepare(struct intel_encoder *encoder)
> DRM_DEBUG_DRIVER("Enabling DP audio on pipe %c\n",
> pipe_name(crtc->pipe));
> intel_dp->DP |= DP_AUDIO_OUTPUT_ENABLE;
> - intel_write_eld(encoder);
> + intel_audio_codec_enable(encoder);
> }
>
> /* Split out the IBX/CPU vs CPT settings */
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index bf72a9201a15..e4deb9cbc6bc 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -850,7 +850,8 @@ void intel_fb_obj_flush(struct drm_i915_gem_object *obj, bool retire);
>
> /* intel_audio.c */
> void intel_init_audio(struct drm_device *dev);
> -void intel_write_eld(struct intel_encoder *encoder);
> +void intel_audio_codec_enable(struct intel_encoder *encoder);
> +void intel_audio_codec_disable(struct intel_encoder *encoder);
>
> /* intel_display.c */
> const char *intel_output_name(int output);
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 07b5ebd65d41..f29026a1157d 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -666,7 +666,7 @@ static void intel_hdmi_prepare(struct intel_encoder *encoder)
> DRM_DEBUG_DRIVER("Enabling HDMI audio on pipe %c\n",
> pipe_name(crtc->pipe));
> hdmi_val |= SDVO_AUDIO_ENABLE;
> - intel_write_eld(encoder);
> + intel_audio_codec_enable(encoder);
> }
>
> if (HAS_PCH_CPT(dev))
> --
> 2.1.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br