[Intel-gfx] [PATCH 2/2] drm/i915: Add the support of memory self-refresh on Ironlake

[Jesse Barnes]

On Mon, 12 Apr 2010 10:43:04 -0400
Adam Jackson <ajax at redhat.com> wrote:

> On Mon, 2010-04-12 at 17:09 +0800, Zhenyu Wang wrote:
> > Update the self-refresh watermark for display plane/cursor and enable
> > the memory self-refresh on Ironlake. The watermark is also updated for
> > the active display plane.
> > 
> > More than 1W idle power is saved on one Ironlake laptop after enabling
> > memory self-refresh.
> 
> Looks good at a quick read, though I haven't tested it yet.
> 
> > +	/* Calculate and update the watermark for plane A */
> > +	if (planea_clock) {
> > +		entries_required = ((planea_clock / 1000) * pixel_size *
> > +				     ILK_LP0_PLANE_LATENCY) / 1000;
> > +		entries_required = DIV_ROUND_UP(entries_required,
> > +				   ironlake_display_wm_info.cacheline_size);
> > +		planea_wm = entries_required +
> > +			    ironlake_display_wm_info.guard_size;
> > +
> > +		if (planea_wm > (int)ironlake_display_wm_info.max_wm)
> > +			planea_wm = ironlake_display_wm_info.max_wm;
> > +
> > +		cursora_wm = 16;
> > +		reg_value = I915_READ(WM0_PIPEA_ILK);
> > +		reg_value &= ~(WM0_PIPE_PLANE_MASK | WM0_PIPE_CURSOR_MASK);
> > +		reg_value |= (planea_wm << WM0_PIPE_PLANE_SHIFT) |
> > +			     (cursora_wm & WM0_PIPE_CURSOR_MASK);
> > +		I915_WRITE(WM0_PIPEA_ILK, reg_value);
> > +		DRM_DEBUG_KMS("FIFO watermarks For pipe A - plane %d, "
> > +				"cursor: %d\n", planea_wm, cursora_wm);
> > +	}
> 
> This doesn't adjust the video sprite plane watermark.  I suppose it
> doesn't really matter, since we're not enabling it yet, but for clarity
> I'd also mask off WM0_PIPE_SPRITE_MASK.
> 
> Also, the cursor watermark is a magic number.  I assume it's derived
> from the hardware cursor size.  In which case, it'd be nice to show the
> derivation; Ironlake can do hardware cursors up to 256x256, it'd be nice
> to have that work automatically when and if we enable that in the
> driver.
> 
> > +		/* configure watermark and enable self-refresh */
> > +		reg_value = I915_READ(WM1_LP_ILK);
> > +		reg_value &= ~(WM1_LP_LATENCY_MASK | WM1_LP_SR_MASK |
> > +			       WM1_LP_CURSOR_MASK);
> > +		reg_value |= WM1_LP_SR_EN |
> > +			     (ilk_sr_latency << WM1_LP_LATENCY_SHIFT) |
> > +			     (sr_wm << WM1_LP_SR_SHIFT) | cursor_wm;
> > +
> > +		I915_WRITE(WM1_LP_ILK, reg_value);
> > +		DRM_DEBUG_KMS("self-refresh watermark: display plane %d "
> > +				"cursor %d\n", sr_wm, cursor_wm);
> 
> What are the conditions for entering LP2 or LP3 state, and is it worth
> trying to set them up too?
> 
> > +		/*
> > +		 * According to the spec the following bits should be set in
> > +		 * order to enable memory self-refresh
> > +		 * The bit 22/21 of 0x42004
> > +		 * The bit 5 of 0x42020
> > +		 * The bit 15 of 0x45000
> > +		 */
> > +		if (IS_IRONLAKE(dev)) {
> > +			I915_WRITE(ILK_DISPLAY_CHICKEN2,
> > +					(I915_READ(ILK_DISPLAY_CHICKEN2) |
> > +					ILK_DPARB_GATE | ILK_VSDPFD_FULL));
> 
> I can only hope this is named "chicken" in allusion to:
> 
> http://en.wikipedia.org/wiki/Chicken_(game)
> 
> I certainly can't find any reference to it in the documentation.

We should probably add a comment about that.  The chicken bits
generally control whether new features are enabled in hw, so-called
because the designers were too chicken to just enable them
unconditionally. :)

-- 
Jesse Barnes, Intel Open Source Technology Center