[Intel-gfx] [PATCH 06/14] drm/i915: Keep sprite plane src rect in 16.16 format

[Matt Roper]

On Tue, Apr 07, 2015 at 03:28:39PM -0700, Chandra Konduru wrote:
> This patch keeps intel_plane_state->src rect back
> into 16.16 format.
> 
> v2:
> -sprite src rect to match primary format (Matt, Daniel)
> 
> Signed-off-by: Chandra Konduru <chandra.konduru at intel.com>

This looks good, and matches what we had discussed, but don't you also
need to add the corresponding unshift in intel_commit_sprite_plane()
when we actually pull the values out and make use of them?  The goal is
to keep the meaning of the structure fields consistent at all times
(16.16 fixed pt), but once we pull the values out of the structure, we
wind up passing them to functions that doing use fixed point, so we do
need to unshift at that point.

It looks like in patch #13 you do switch the low-level
skl_update_plane() to make use of 16.16 input parameters.  However any
commit that we bisect through between #6 and #13 is going to wind up
treating 16.16 values as integer values, which I assume will blow up.
Also, you haven't touched any of the other platforms (ilk, vlv, ivb,
etc.) so they're all still going to have problems.

I think the easiest short-term solution is to do the unshifting in
commit_plane and leave the hardware-specific programming functions
as-is.  Longer term, maybe it makes sense for a future patchset to
change the actual register-programming functions (foo_update_plane) so
that they take a plane_state directly and do their own unshifting?  In
that case we'd need to update them to do their own unshifting, but at
least we wouldn't have to pull all the values out in commit_plane, just
to pass them to these functions.


Matt

> ---
>  drivers/gpu/drm/i915/intel_sprite.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index ac4aa68..c05fb36 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1006,10 +1006,10 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  	}
>  
>  	if (state->visible) {
> -		src->x1 = src_x;
> -		src->x2 = src_x + src_w;
> -		src->y1 = src_y;
> -		src->y2 = src_y + src_h;
> +		src->x1 = src_x << 16;
> +		src->x2 = (src_x + src_w) << 16;
> +		src->y1 = src_y << 16;
> +		src->y2 = (src_y + src_h) << 16;
>  	}
>  
>  	dst->x1 = crtc_x;
> -- 
> 1.7.9.5
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795