Re: [PATCH 06/20] drm/i915: Convert primary plane 16.16 values to regular ints

[Matt Roper <matthew.d.roper@intel.com>]

On Wed, Apr 01, 2015 at 07:59:35PM -0700, Chandra Konduru wrote:
> This patch converts intel_plane_state->src rect from 16.16
> values into regular ints.
> 
> This approach aligns with sprite_plane_state->src rects
> which are already in regular ints.
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>

You're not touching cursor state here, so I guess it stays in 16.16 form
always?  Does it need to be updated to behave the same way?

Looking at our sprite code today, it treats intel_state->src as 16.16
for most of the check function, then re-writes it as integer pixels near
the end, which I guess matches the type of change you're doing here.  I
still find this pretty confusing that our structure is sometimes
interpreted in one way and other times interpreted a different way.

Personally I think it would be less error-prone if we just treated src
as 16.16 always, but if you to keep the current logic which changes the
meaning at the end of the check() stage, can you add some comments to
struct intel_plane_state clarifying that?

With some extra comments to avoid future confusion, you can consider this
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>


Matt

> ---
>  drivers/gpu/drm/i915/intel_display.c |   13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ee71bde..dddbe11 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12590,6 +12590,15 @@ intel_check_primary_plane(struct drm_plane *plane,
>  			intel_crtc->atomic.update_wm = true;
>  	}
>  
> +	/*
> +	 * Hardware doesn't handle subpixel coordinates.
> +	 * Adjust to (macro)pixel boundary.
> +	 */
> +	src->x1 >>= 16;
> +	src->x2 >>= 16;
> +	src->y1 >>= 16;
> +	src->y2 >>= 16;
> +
>  	return 0;
>  }
>  
> @@ -12608,8 +12617,8 @@ intel_commit_primary_plane(struct drm_plane *plane,
>  	intel_crtc = to_intel_crtc(crtc);
>  
>  	plane->fb = fb;
> -	crtc->x = src->x1 >> 16;
> -	crtc->y = src->y1 >> 16;
> +	crtc->x = src->x1;
> +	crtc->y = src->y1;
>  
>  	if (intel_crtc->active) {
>  		if (state->visible) {
> -- 
> 1.7.9.5
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx