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

[ykzhao]

On Thu, 2009-12-03 at 01:32 +0800, Jesse Barnes wrote:
> 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.)

The BIOS will write some default watermark for display plane A/B/C. If
we don't touch it, it should also work when it is in non self-refresh
state.

But according to the spec the value should be as recommended in the high
priority bandwidth analysis spreadsheet.
So on the g4x platform we will calculate the watermark for display plane
A/B when two display planes are used.

Of course I agree with what you said.It is unnecessary to copy them on
g4x platform.

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

Sorry that I don't received this doc.

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

The calculate_wm function will return the different result with the
old-refresh calculation. 

Do you mean that the extra buffering is the guard size?

Thanks.
>