Re: [PATCH 4/5] drm/i915: Don't store panning coordinates as 16.16 fixed point

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Nov 17, 2014 at 06:10:38PM -0800, Matt Roper wrote:
> When using the universal plane interface, the source rectangle
> coordinates define the panning offset for the primary plane, which needs
> to be stored in crtc->{x,y}.  The original universal plane code
> negelected to set these panning offset fields, which was partially
> remedied in:
> 
>         commit ccc759dc2a0214fd8b65ed4ebe78050874a67f94
>         Author: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>         Date:   Wed Sep 24 14:20:22 2014 -0300
> 
>             drm/i915: Merge of visible and !visible paths for primary planes
> 
> However the plane source coordinates are provided in 16.16 fixed point
> format and the above commit forgot to convert back to integer
> coordinates before saving the values.  When we replace
> intel_pipe_set_base() with plane->funcs->update_plane() in a future
> patch, this bug becomes visible via the set_config entrypoint as well as
> update_plane.
> 
> Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> Testcase: igt/kms_plane
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8dbc0e9..1c1886e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11668,8 +11668,8 @@ intel_commit_primary_plane(struct drm_plane *plane,
>  	struct drm_rect *src = &state->src;
>  
>  	crtc->primary->fb = fb;
> -	crtc->x = src->x1;
> -	crtc->y = src->y1;
> +	crtc->x = src->x1 >> 16;
> +	crtc->y = src->y1 >> 16;

Oh I had the notion that we shift these down during the check phase.
But I see we only do that for sprites. We should pick one approach
and use it for all planes. I suppose keeping to the 16.16 format is
the better option in anticipation of the day when we have planes
which can do sub-pixel panning.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  
>  	intel_plane->crtc_x = state->orig_dst.x1;
>  	intel_plane->crtc_y = state->orig_dst.y1;
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx