Re: [PATCH 07/26] drm/i915: add ValleyView clock gating init

[Ben Widawsky <ben@bwidawsk.net>]

On Thu, Mar 22, 2012 at 02:38:49PM -0700, Jesse Barnes wrote:
> Set the same bits as IVB plus a few others.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      |   18 +++++++++++++
>  drivers/gpu/drm/i915/intel_display.c |   46 ++++++++++++++++++++++++++++++++++
>  2 files changed, 64 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 1af17e0..86de215 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -578,6 +578,9 @@
>  #define   ECO_GATING_CX_ONLY	(1<<3)
>  #define   ECO_FLIP_DONE		(1<<0)
>  
> +#define CACHE_MODE_1		0x7004 /* IVB+ */
> +#define   PIXEL_SUBSPAN_COLLECT_OPT_DISABLE (1<<6)
> +
>  /* GEN6 interrupt control */
>  #define GEN6_RENDER_HWSTAM	0x2098
>  #define GEN6_RENDER_IMR		0x20a8

So bspec says this should be set for VLV and IVB. But in the series I
only see it defined and never set. So either set it, or kick it to the
curb. (I vote set it)

> @@ -3178,6 +3181,20 @@
>  #define  DISP_TILE_SURFACE_SWIZZLING	(1<<13)
>  #define  DISP_FBC_WM_DIS		(1<<15)
>  
> +/* GEN7 chicken */
> +#define GEN7_COMMON_SLICE_CHICKEN1		0x7010
> +# define GEN7_CSC1_RHWO_OPT_DISABLE_IN_RCC	((1<<10) | (1<<26))
> +
> +#define GEN7_L3CNTLREG1				0xB01C
> +#define  GEN7_WA_FOR_GEN7_L3_CONTROL			0x3C4FFF8C
> +
> +#define GEN7_L3_CHICKEN_MODE_REGISTER		0xB030
> +#define  GEN7_WA_L3_CHICKEN_MODE				0x20000000
> +
> +/* WaCatErrorRejectionIssue */
> +#define GEN7_SQ_CHICKEN_MBCUNIT_CONFIG		0x9030
> +#define  GEN7_SQ_CHICKEN_MBCUNIT_SQINTMOB	(1<<11)
> +
>  /* PCH */
>  
>  /* south display engine interrupt */

Note WaCatErrorRejectionIssue seems to apply for IVB as well.

> @@ -3781,6 +3798,7 @@
>  #define    GT_FIFO_NUM_RESERVED_ENTRIES		20
>  
>  #define GEN6_UCGCTL2				0x9404
> +# define GEN6_RCZUNIT_CLOCK_GATE_DISABLE		(1 << 13)
>  # define GEN6_RCPBUNIT_CLOCK_GATE_DISABLE		(1 << 12)
>  # define GEN6_RCCUNIT_CLOCK_GATE_DISABLE		(1 << 11)
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d72467f..22619c6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8483,6 +8483,50 @@ void gen6_update_ring_freq(struct drm_i915_private *dev_priv)
>  	mutex_unlock(&dev_priv->dev->struct_mutex);
>  }
>  
> +static void valleyview_init_clock_gating(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int pipe;
> +	uint32_t dspclk_gate = VRHUNIT_CLOCK_GATE_DISABLE;
> +
> +	I915_WRITE(PCH_DSPCLK_GATE_D, dspclk_gate);
> +
> +	I915_WRITE(WM3_LP_ILK, 0);
> +	I915_WRITE(WM2_LP_ILK, 0);
> +	I915_WRITE(WM1_LP_ILK, 0);
> +
> +	/* According to the spec, bit 13 (RCZUNIT) must be set on IVB.
> +	 * This implements the WaDisableRCZUnitClockGating workaround.
> +	 */
> +	I915_WRITE(GEN6_UCGCTL2, GEN6_RCZUNIT_CLOCK_GATE_DISABLE);
> +
> +	I915_WRITE(ILK_DSPCLK_GATE, IVB_VRHUNIT_CLK_GATE);
> +
> +	I915_WRITE(IVB_CHICKEN3,
> +		   CHICKEN3_DGMG_REQ_OUT_FIX_DISABLE |
> +		   CHICKEN3_DGMG_DONE_FIX_DISABLE);
> +
> +	/* Apply the WaDisableRHWOOptimizationForRenderHang workaround. */
> +	I915_WRITE(GEN7_COMMON_SLICE_CHICKEN1,
> +		   GEN7_CSC1_RHWO_OPT_DISABLE_IN_RCC);
> +
> +	/* WaApplyL3ControlAndL3ChickenMode requires those two on Ivy Bridge */
> +	I915_WRITE(GEN7_L3CNTLREG1, GEN7_WA_FOR_GEN7_L3_CONTROL);
> +	I915_WRITE(GEN7_L3_CHICKEN_MODE_REGISTER, GEN7_WA_L3_CHICKEN_MODE);
> +
> +	/* This is required by WaCatErrorRejectionIssue */
> +	I915_WRITE(GEN7_SQ_CHICKEN_MBCUNIT_CONFIG,
> +		   I915_READ(GEN7_SQ_CHICKEN_MBCUNIT_CONFIG) |
> +		   GEN7_SQ_CHICKEN_MBCUNIT_SQINTMOB);
> +
> +	for_each_pipe(pipe) {
> +		I915_WRITE(DSPCNTR(pipe),
> +			   I915_READ(DSPCNTR(pipe)) |
> +			   DISPPLANE_TRICKLE_FEED_DISABLE);
> +		intel_flush_display_plane(dev_priv, pipe);
> +	}
> +}
> +
>  static void ironlake_init_clock_gating(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -8990,6 +9034,8 @@ static void intel_init_display(struct drm_device *dev)
>  			dev_priv->display.update_wm = NULL;
>  	} else if (IS_VALLEYVIEW(dev)) {
>  		dev_priv->display.update_wm = valleyview_update_wm;
> +		dev_priv->display.init_clock_gating =
> +			valleyview_init_clock_gating;
>  	} else if (IS_PINEVIEW(dev)) {
>  		if (!intel_get_cxsr_latency(IS_PINEVIEW_G(dev),
>  					    dev_priv->is_ddr3,

You put valleyview_init_clock_gating above ironlake, when it should be
at least below gen6 (probably gen7, looking at the ordering of the rest
of the stuff). No comment on the actual content (important part) of the
patch yet.


Assuming you address my comments, this is:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>