[Intel-gfx] [PATCH] drm/i915: Fix SKL+ 90/270 degree rotated plane coordinate computation

[Paulo Zanoni]

Em Seg, 2016-10-24 às 19:13 +0300, ville.syrjala at linux.intel.com
escreveu:
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> 
> Pass the framebuffer size in .16 fixed point coordinates to
> drm_rect_rotate() since that's what the source coordinates are as
> well
> at this stage. We used to do this part of the computation in integer
> coordinates, but that got changed when moving the computation to
> happen in the check phase of the operation. Unfortunately I forgot
> to shift up the fb width and height appropriately.
> 
> With the bogus size we ended up with some negative fb offset, which
> when
> added to the vma offset caused out scanout to start at an offset
> earlier
> than we inteded. Eg. when testing on my SKL I saw a row of incorrect
> tiles at the top of my screen.

I already mentioned this while reviewing another patch from another
author, but let's throw the idea again: how about adding a specific
16.16 type in order to prevent these silent failures when mixing them
with integers?

struct (or union) int_fixed_16_16 {
	uint32_t number;
}

And them some nice macros for explicit casting to/from int.

I see include/drm/fixed.h even has a 20_12 type...

I could even volunteer to do this if there's some positive feedback
upfront, but feel free to do this too in case you want.

We're humans and we're going to make the same "mix normal integers with
16.16 integers" mistake again and again and again, while the compiler
could really help us if the types were not plain integers.

Thoughts?

> 
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Sivakumar Thulasimani <sivakumar.thulasimani at intel.com>
> Cc: drm-intel-fixes at lists.freedesktop.org
> Fixes: b63a16f6cd89 ("drm/i915: Compute display surface offset in the
> plane check hook for SKL+")
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 5a036999487b..c783f884f85d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2985,7 +2985,8 @@ int skl_check_plane_surface(struct
> intel_plane_state *plane_state)
>  	/* Rotate src coordinates to match rotated GTT view */
>  	if (drm_rotation_90_or_270(rotation))
>  		drm_rect_rotate(&plane_state->base.src,
> -				fb->width, fb->height,
> DRM_ROTATE_270);
> +				fb->width << 16, fb->height << 16,
> +				DRM_ROTATE_270);
>  
>  	/*
>  	 * Handle the AUX surface first since