Re: [PATCH v2] drm/i915/hsw: Add display Audio codec disable sequence for Haswell

[Ben Widawsky <ben@bwidawsk.net>]

On Thu, Aug 29, 2013 at 07:50:20PM -0400, mengdong.lin@intel.com wrote:
> From: Mukesh <mukeshx.arora@intel.com>
> 
> The code defines a new function intel_disable_audio() to implement the
> entire audio codec disable sequence, called by intel_disable_ddi() in
> DDI port disablement. This audio codec disbale sequence is implemented
> as per the recommendation of the Bspec.
> 
> [Patch modified by Mendong to apply to both HDMI and DP port.]
> 
> Signed-off-by: Mukesh Arora <mukeshx.arora@intel.com>
> Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index b042ee5..2946fe7 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1088,6 +1088,45 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
>  	I915_WRITE(PORT_CLK_SEL(port), PORT_CLK_SEL_NONE);
>  }
>  
> +/* audio codec disable sequence, as per Bspec */
> +void intel_disable_audio(struct intel_encoder *intel_encoder)
> +{
> +	int type = intel_encoder->type;
> +	struct drm_encoder *encoder = &intel_encoder->base;
> +	struct drm_device *dev = encoder->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
> +	int pipe = intel_crtc->pipe;
> +	int aud_config = HSW_AUD_CFG(pipe);
> +	u32 temp;
> +

I don't know what it means, but where is the : "Disable sample
fabrication" step?

> +	/* disable timestamps */
> +	temp = I915_READ(aud_config);
> +	if (type == INTEL_OUTPUT_HDMI)
> +		temp &= ~AUD_CONFIG_N_VALUE_INDEX;
> +	else if (type == INTEL_OUTPUT_DISPLAYPORT)
> +		temp |= AUD_CONFIG_N_VALUE_INDEX;
> +	else
> +		return;
> +	temp |= AUD_CONFIG_N_PROG_ENABLE;
> +	temp &= ~(AUD_CONFIG_UPPER_N_VALUE|AUD_CONFIG_LOWER_N_VALUE);
> +	I915_WRITE(aud_config, temp);
> +
> +	/* Disable ELDV and ELD buffer */
> +	temp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
> +	temp &= ~(AUDIO_ELD_VALID_A << (pipe * 4));
> +	I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, temp);
> +

POSTING_READ or a comment explaining why you don't need one.

> +	/* Wait for 2 vertical blanks */
> +	intel_wait_for_vblank(dev, pipe);
> +	intel_wait_for_vblank(dev, pipe);
> +
> +	/* Disable audio PD. This is optional as per Bspec.  */
Please add the comment when software might want to skip this step (it
seems relevant to me for future programming)
> +	temp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
> +	temp &= ~(AUDIO_OUTPUT_ENABLE_A << (pipe * 4));
> +	I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, temp);
> +}
> +

The bspec isn't clear about any implied waits in the steps, but it's
probably safer to add a POSTING_READ after each I915_WRITE, unless you
specifically know the HW doesn't need the last write to have landed.

With all comments addressed, it's:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

>  static void intel_enable_ddi(struct intel_encoder *intel_encoder)
>  {
>  	struct drm_encoder *encoder = &intel_encoder->base;
> @@ -1132,18 +1171,10 @@ 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;
>  
> -	if (intel_crtc->eld_vld && type != INTEL_OUTPUT_EDP) {
> -		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);
> -	}
> +	if (intel_crtc->eld_vld && type != INTEL_OUTPUT_EDP)
> +		intel_disable_audio(intel_encoder);
>  
>  	if (type == INTEL_OUTPUT_EDP) {
>  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);

This hunk looks fine.

-- 
Ben Widawsky, Intel Open Source Technology Center