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

[ykzhao]

On Mon, 2010-04-12 at 22:43 +0800, Adam Jackson 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.

As the sprite plane is not touched, it will be better to reserve the
corresponding sprite watermark. 

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

Very good point. Yes. It seems that the Ironlake can support up to
256X256 at 32bpp cursor. This is not considered in this patch. 


We can add another patch to calculate the cursor watermark.

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

LP3 is not supported on ironlake.

After consulting the hardware group, it is not worth setting up the LP2.
It is complex to set up the LP2 but the power saving is very small. (Not
more than 0.1W).

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

This is defined in the spec from hardware group. 

What Jesse said is right. Maybe we should add more comments about it.

> 
> - ajax