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

[Lin, Mengdong]

Hi Ben,

Thank you very much for the review. I'm sorry I missed the feedback for quite a long time.
Please see comments below ...

> -----Original Message-----
> From: Ben Widawsky [mailto:ben at bwidawsk.net]
> Sent: Thursday, September 05, 2013 1:03 AM
> To: Lin, Mengdong
> Cc: intel-gfx at lists.freedesktop.org; Arora, MukeshX
> Subject: Re: [Intel-gfx] [PATCH v2] drm/i915/hsw: Add display Audio codec
> disable sequence for Haswell
> 
> On Thu, Aug 29, 2013 at 07:50:20PM -0400, mengdong.lin at intel.com wrote:
> > From: Mukesh <mukeshx.arora at 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 at intel.com>
> > Signed-off-by: Mengdong Lin <mengdong.lin at 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?

No, there is no "Disable sample fabrication" step, because sample fabrication is never enabled.
I'll add a comment in code about this.

> > +	/* 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.

I'll add POST_READ.

> > +	/* 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 at bwidawsk.net>

Okay. Thanks!
Mengdong

> >  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