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

[Eric Anholt]

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?"  

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.

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

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?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20100115/e1d0a7f1/attachment.sig>