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@xxxxxxxxxxxx] > Sent: Thursday, September 05, 2013 1:03 AM > To: Lin, Mengdong > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Arora, MukeshX > Subject: Re: [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@xxxxxxxxx wrote: > > From: Mukesh <mukeshx.arora@xxxxxxxxx> > > > > 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@xxxxxxxxx> > > Signed-off-by: Mengdong Lin <mengdong.lin@xxxxxxxxx> > > > > 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@xxxxxxxxxxxx> 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
