Re: [PATCH 2/2] drm/i915: Add 90/270 rotation for NV12 format.

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

On Fri, May 08, 2015 at 08:22:47PM -0700, Chandra Konduru wrote:
> Adding NV12 90/270 rotation support for primary and sprite planes.
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   23 ++++++++++++++++-------
>  drivers/gpu/drm/i915/intel_sprite.c  |   32 +++++++++++++++++++++++++-------
>  2 files changed, 41 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c385a3b..77d7f69 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3105,7 +3105,8 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  	int src_x = 0, src_y = 0, src_w = 0, src_h = 0;
>  	int dst_x = 0, dst_y = 0, dst_w = 0, dst_h = 0;
>  	int scaler_id = -1;
> -	u32 aux_dist = 0, aux_x_offset = 0, aux_y_offset = 0, aux_stride = 0;
> +	unsigned long aux_dist = 0;
> +	u32 aux_x_offset = 0, aux_y_offset = 0, aux_stride = 0;
>  	u32 tile_row_adjustment = 0;
>  
>  	plane_state = to_intel_plane_state(plane->state);
> @@ -3163,12 +3164,16 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  		x_offset = stride * tile_height - y - src_h;
>  		y_offset = x;
>  		plane_size = (src_w - 1) << 16 | (src_h - 1);
> -		/*
> -		 * TBD: For NV12 90/270 rotation, Y and UV subplanes should
> -		 * be treated as separate surfaces and GTT remapping for
> -		 * rotation should be done separately for each subplane.
> -		 * Enable support once seperate remappings are available.
> -		 */
> +
> +		if (fb->pixel_format == DRM_FORMAT_NV12) {
> +			u32 uv_tile_height = intel_tile_height(dev, fb->pixel_format,
> +						fb->modifier[0], 1);
> +			aux_stride = DIV_ROUND_UP(fb->height / 2, uv_tile_height);
> +			aux_dist = intel_plane_obj_offset(to_intel_plane(plane), obj, 1) -
> +				surf_addr;
> +			aux_x_offset = aux_stride * uv_tile_height - y / 2 - fb->height / 2;
> +			aux_y_offset = x / 2;
> +		}
>  	} else {
>  		stride = fb->pitches[0] / stride_div;
>  		x_offset = x;
> @@ -13144,6 +13149,10 @@ intel_check_primary_plane(struct drm_plane *plane,
>  	if (fb && format_is_yuv(fb->pixel_format)) {
>  		src->x1 &= ~0x10000;
>  		src->x2 &= ~0x10000;
> +		if (intel_rotation_90_or_270(state->base.rotation)) {
> +			src->y1 &= ~0x10000;
> +			src->y2 &= ~0x10000;
> +		}

This feels fishy. Why do we need to make the Y coordinates even? The
reson for making the X coordinates even is to make them macropixel
aligned, but there are no macropixels in the Y direction so this
doesn't make much sense to me.

>  	}
>  
>  	if (INTEL_INFO(dev)->gen >= 9) {
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index e23fe8e..d4ce7eb 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -190,7 +190,8 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  	int x_offset, y_offset;
>  	struct intel_crtc_state *crtc_state = to_intel_crtc(crtc)->config;
>  	int scaler_id;
> -	u32 aux_dist = 0, aux_x_offset = 0, aux_y_offset = 0, aux_stride = 0;
> +	unsigned long aux_dist = 0;
> +	u32 aux_x_offset = 0, aux_y_offset = 0, aux_stride = 0;
>  	u32 tile_row_adjustment = 0;
>  
>  	plane_ctl = PLANE_CTL_ENABLE |
> @@ -239,12 +240,14 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  		x_offset = stride * tile_height - y - (src_h + 1);
>  		y_offset = x;
>  
> -		/*
> -		 * TBD: For NV12 90/270 rotation, Y and UV subplanes should
> -		 * be treated as separate surfaces and GTT remapping for
> -		 * rotation should be done separately for each subplane.
> -		 * Enable support once seperate remappings are available.
> -		 */
> +		if (fb->pixel_format == DRM_FORMAT_NV12) {
> +			u32 uv_tile_height = intel_tile_height(dev, fb->pixel_format,
> +						fb->modifier[0], 1);
> +			aux_stride = DIV_ROUND_UP(fb->height / 2, uv_tile_height);
> +			aux_dist = intel_plane_obj_offset(intel_plane, obj, 1) - surf_addr;
> +			aux_x_offset = aux_stride * uv_tile_height - y / 2 - fb->height / 2;
> +			aux_y_offset = x / 2;
> +		}
>  	} else {
>  		stride = fb->pitches[0] / stride_div;
>  		plane_size = (src_h << 16) | src_w;
> @@ -909,6 +912,21 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  
>  			if (crtc_w == 0)
>  				state->visible = false;
> +
> +			if (intel_rotation_90_or_270(state->base.rotation)) {
> +				src_y &= ~1;
> +				src_h &= ~1;
> +
> +				/*
> +				 * Must keep src and dst the
> +				 * same if we can't scale.
> +				 */
> +				if (!intel_plane->can_scale)
> +					crtc_h &= ~1;
> +
> +				if (crtc_h == 0)
> +					state->visible = false;
> +			}

Same here.

>  		}
>  	}
>  
> -- 
> 1.7.9.5
> 
> _______________________________________________
> 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