[Intel-gfx] [PATCH 4/6] drm/i915: Redefine the fifo size on 965/g4x and the function of intel_calculate_wm

[Zhao, Yakui]

Thanks for the review.

-----Original Message-----
From: Eric Anholt [mailto:eric at anholt.net] 
Sent: Saturday, January 16, 2010 6:02 AM
To: Zhao, Yakui
Cc: intel-gfx at lists.freedesktop.org; Zhao, Yakui
Subject: Re: [PATCH 4/6] drm/i915: Redefine the fifo size on 965/g4x and the function of intel_calculate_wm

On Wed, 13 Jan 2010 22:10:53 +0800, yakui.zhao at intel.com wrote:
> From: Zhao Yakui <yakui.zhao at intel.com>
> 
> According to the spec we use the incorrect fifo size on 965/g4x 
> platform to calculate the self-refresh watermark for display 
> plane/cursor. So redefine the fifo size on 965/g4x platform to calculate the self-refresh watermark.
> 
> At the same time this patch also redefines the function of 
> intel_calculate_wm so that it can be used on g4x/965/9xx/pineviw 
> platform. It contains the following parts:
>    1. Add a watermark flag to indicate whether the required fifo size 
> is directly used as the watermark. This is required on g4x platform.
>    2. we add another new method to calculate the watermark, which uses 
> the following formula to calcuate the required size.
>     >(Trunc(required_latency / line_time) + 1) * pixel_size * surface_width
>    The argument of calc_flags can decide which method is used to 
> calculate the watermark.
> 
>    3. Consider the difference between the total fifo size and the size 
> of 8 whole lines when calculating the watermark for display plane.
> 
> Signed-off-by: Zhao Yakui <yakui.zhao at intel.com>
> Reviewd-by: Jesse Barnes <jbarnes at virtuousgeek.org>

For these remaining patches, I'm not clear which branch you're intending them for, because they don't make clear what they're fixing.  Is it "things don't quite match the spec and let's play it safe" or "people's laptops are broken and this fixes them?"  

[Yakui]: The issue is that the things we used don't match the spec. 

Now, for this particular patch since I'm looking at it, what is the 8 lines thing about?  There's this comment:

> + * At the same time the difference between FIFO size and 8 lines size 
> + is also
> + * considered when calculating the watermark for display plane.

I can easily see in the code that some difference between "FIFO size and
8 lines [of display] is also considered when calculating the watermark", but that doesn't enlighten me at all as to why 8 lines of display is important and needs to be considered.  If a comment is necessary, it should explain to the reader why the code is there doing what it's doing, not what the code is doing.

[Yakui]: This comes from the spec. 
Thanks for the advice. I will pay attention to the unnecessary comment.

> +	if (wm->watermark_flag) {
> +		wm_size = sr_entries + wm->guard_size;
> +	} else {
> +		wm_size = wm->fifo_size - (sr_entries + wm->guard_size);
> +	}

If watermark_flag isn't a set of flags but a boolean of "register uses entry count", let's call it watermark_uses_entries, and comment the alternative.  Or follow nice API design as cworth teaches us and make it a 2-value enum where we pass one of {WATERMARK_USES_ENTRY_COUNT, WATERMARK_USES_OFFSET} in.  This patch kind of does this with the #defines for int wm_type -- enums are great for this kind of stuff, because you get type checking.


I'm really uncomfortable with this patch, because right from the commit message there's giant warning flags that this is at least 3 commits all smashed into one.  A sensible approach to this set of changes would seem to be:

[Yakui]: Do you mean that we divide this patch into several small patches? 

1) commit that fixes DSPFW_CURSORA_MASK usage.  (this seems like an
   obvious for-linus fix that should be Cc:ed to stable along with the
   other one for CURSORA).

2) commit that adds enum wm_type argument and new calculation of
   entries_required.  Explain in the commit what's better -- what
   systems it fixes, for example.

3) commit that adds the 8-lines adjustment for WM_TYPE_PLANE.  Explain
   in the commit *why* this adjustment is needed.

4) commit that adds the enum wm_register_type argument for the 965/g4x
   vs pre-965 difference, treats
   WATERMARK_USES_OFFSET/WATERMARK_USES_ENTRY_COUNT differently, and
   uses it for 965/g4x.  I suspect this one is a drm-intel-next change?