[Intel-gfx] [RFC PATCH 1/2] drm/i915: Calculate self-refresh watermark by using one unique algorithm on g4x/965/9xx platform

[Jesse Barnes]

On Wed,  2 Dec 2009 15:57:42 +0800
yakui.zhao at intel.com wrote:

> From: Zhao Yakui <yakui.zhao at intel.com>
> 
> Use one unique algorithm to calucate the self-refresh watermark on
> g4x/965/9xx platform, which is done by calling the function of
> intel_calculate_wm. In this function it will calculate the requried
> minimum fifo size firstly and then get the expected watermark.
> 
> We will configure the cursor self-refresh watermark on 965 platform.
> 
> Signed-off-by: Zhao Yakui <yakui.zhao at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  119
> ++++++++++++++++----------------- 1 files changed, 58 insertions(+),
> 61 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c index 65b76ff..c87974d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2191,6 +2191,12 @@ struct intel_watermark_params {
>  	unsigned long default_wm;
>  	unsigned long guard_size;
>  	unsigned long cacheline_size;
> +	/* indicate whether the required entry is treated as the
> watermark.
> +	 * If it is 1, it will use the required entries to set the
> watermark.
> +	 * Otherwise we will subtract the required entry from the
> total size
> +	 * to get the watermark.
> +	 */
> +	int	watermark_flag;
>  };

Nice, I think this is better than adding an extra arg to the calc_wm
function.

You just need one space here though between 'int' and 'watermark_flag'.

>  	/* Create copies of the base settings for each pipe */
>  	planea_params = planeb_params = g4x_wm_info;

Just noticed that since we don't adjust the plane A/B FIFO sizes, we
can get rid of the per-plane structs for g4x I think?  Would clean
things up a little.  (And if you can think of a better way of handling
the FIFO sizing for pre-Cantiga, feel free to clean that up too... the
"modify struct copy" always bugged me a little.)

> +	planeb_wm = intel_calculate_wm(planeb_clock, &planeb_params,
> +					pixel_size, latency_ns);
> +
> +	/* the cursor watermark should also be calculated by using
> the function
> +	 * of intel_calculate_wm.
> +	 * But as we have no proper latency setting and total fifo
> size
> +	 * for cursor, now the fixed value is used.
> +	 * The cursor self-refresh watermark is 32.
> +	 * The cursor A/B watermark is 16.
> +	 * If it is incorrect, please fix me.
> +	 */
>  	cursora_wm = cursorb_wm = 16;
>  	cursor_sr = 32;

I think the watermark calc doc I sent has some guidance on these, if
you've double checked you can remove the "fix me".

> +	cursor_srwm = 16;
>  	/* Calc sr entries for one plane configs */
>  	if (sr_hdisplay && (!planea_clock || !planeb_clock)) {
>  		/* self-refresh has much higher latency */
>  		const static int sr_latency_ns = 12000;
>  
>  		sr_clock = planea_clock ? planea_clock :
> planeb_clock;
> -		line_time_us = ((sr_hdisplay * 1000) / sr_clock);
> -
> -		/* Use ns/us then divide to preserve precision */
> -		sr_entries = (((sr_latency_ns / line_time_us) + 1) *
> -			      pixel_size * sr_hdisplay) / 1000;
> -		sr_entries = roundup(sr_entries /
> I915_FIFO_LINE_SIZE, 1);
> -		DRM_DEBUG("self-refresh entries: %d\n", sr_entries);
> -		srwm = I945_FIFO_SIZE - sr_entries;
> -		if (srwm < 0)
> -			srwm = 1;
> -		srwm &= 0x3f;
> +
> +		srwm = intel_calculate_wm(sr_clock, &i945_wm_info,
> +						pixel_size,
> sr_latency_ns);
> +		DRM_DEBUG_KMS("self-refresh watermark: %d\n", srwm);
> +
>  		I915_WRITE(FW_BLC_SELF, FW_BLC_SELF_EN);
>  	}

Have you confirmed that the calculate_wm function gives you the same
results as the old self-refresh calculation?  The doc says self-refresh
needs special calculations in addition to some extra buffering in the
large FIFO case; we need to preserve that.

-- 
Jesse Barnes, Intel Open Source Technology Center