[2/2] drm/i915: Add 90/270 rotation for NV12 format.
diff mbox

Message ID 1431141767-29455-3-git-send-email-chandra.konduru@intel.com
State New
Headers show

Commit Message

Chandra Konduru May 9, 2015, 3:22 a.m. UTC
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(-)

Comments

Ville Syrjala May 11, 2015, 12:03 p.m. UTC | #1
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
Chandra Konduru May 11, 2015, 11:32 p.m. UTC | #2
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> Sent: Monday, May 11, 2015 5:03 AM
> To: Konduru, Chandra
> Cc: intel-gfx@lists.freedesktop.org; Vetter, Daniel; Syrjala, Ville
> Subject: Re: [Intel-gfx] [PATCH 2/2] drm/i915: Add 90/270 rotation for NV12
> format.
> 
> 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.

Hi Ville,
Per skl spec, it is expecting even lines aligned with 90/270 rotation not only
for NV12 but also for 422 formats. Perhaps we might have missed when 90/270
enabled for packed YUV formats.

> 
> >  	}
> >
> >  	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
Ville Syrjala May 12, 2015, 10:44 a.m. UTC | #3
On Mon, May 11, 2015 at 11:32:07PM +0000, Konduru, Chandra wrote:
> 
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > Sent: Monday, May 11, 2015 5:03 AM
> > To: Konduru, Chandra
> > Cc: intel-gfx@lists.freedesktop.org; Vetter, Daniel; Syrjala, Ville
> > Subject: Re: [Intel-gfx] [PATCH 2/2] drm/i915: Add 90/270 rotation for NV12
> > format.
> > 
> > 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.
> 
> Hi Ville,
> Per skl spec, it is expecting even lines aligned with 90/270 rotation not only
> for NV12 but also for 422 formats. Perhaps we might have missed when 90/270
> enabled for packed YUV formats.

The src coordinates are always in the fb orientation, so macropixels
appear in the src.x direction only. And when we do 90/270 rotation the
hardware Y offset comes from src.x coordinates.

The spec does seem a bit confused though; It claims the X offset must
always be even for YUV422+NV12, and the Y offset must be even when
rotated 90/270 degrees. I suspect the X offset text just didn't get
updated when 90/270 rotation was added.

Art, can you confirm?
Runyan, Arthur J May 12, 2015, 7:26 p.m. UTC | #4
I'll take a look.

-----Original Message-----
From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
Sent: Tuesday, May 12, 2015 3:44 AM
To: Konduru, Chandra
Cc: intel-gfx@lists.freedesktop.org; Vetter, Daniel; Syrjala, Ville; Jindal, Sonika; Runyan, Arthur J
Subject: Re: [Intel-gfx] [PATCH 2/2] drm/i915: Add 90/270 rotation for NV12 format.

On Mon, May 11, 2015 at 11:32:07PM +0000, Konduru, Chandra wrote:
> 
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > Sent: Monday, May 11, 2015 5:03 AM
> > To: Konduru, Chandra
> > Cc: intel-gfx@lists.freedesktop.org; Vetter, Daniel; Syrjala, Ville
> > Subject: Re: [Intel-gfx] [PATCH 2/2] drm/i915: Add 90/270 rotation for NV12
> > format.
> > 
> > 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.
> 
> Hi Ville,
> Per skl spec, it is expecting even lines aligned with 90/270 rotation not only
> for NV12 but also for 422 formats. Perhaps we might have missed when 90/270
> enabled for packed YUV formats.

The src coordinates are always in the fb orientation, so macropixels
appear in the src.x direction only. And when we do 90/270 rotation the
hardware Y offset comes from src.x coordinates.

The spec does seem a bit confused though; It claims the X offset must
always be even for YUV422+NV12, and the Y offset must be even when
rotated 90/270 degrees. I suspect the X offset text just didn't get
updated when 90/270 rotation was added.

Art, can you confirm?
Chandra Konduru May 15, 2015, 9:58 p.m. UTC | #5
> From: Runyan, Arthur J
> 
> I'll take a look.

Art, Any update to close on this?

[snip]

> > > > @@ -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.
> >
> > Hi Ville,
> > Per skl spec, it is expecting even lines aligned with 90/270 rotation
> > not only for NV12 but also for 422 formats. Perhaps we might have
> > missed when 90/270 enabled for packed YUV formats.
> 
> The src coordinates are always in the fb orientation, so macropixels appear in
> the src.x direction only. And when we do 90/270 rotation the hardware Y offset
> comes from src.x coordinates.
> 
> The spec does seem a bit confused though; It claims the X offset must always be
> even for YUV422+NV12, and the Y offset must be even when rotated 90/270
> degrees. I suspect the X offset text just didn't get updated when 90/270 rotation
> was added.
> 
> Art, can you confirm?
> 
> --
> Ville Syrjälä
> Intel OTC
Runyan, Arthur J May 18, 2015, 7:19 p.m. UTC | #6
The statement is correct - " the X offset must always be even for YUV422+NV12, and the Y offset must be even when rotated 90/270 degrees."

>From: Konduru, Chandra
>> From: Runyan, Arthur J
>>
>> I'll take a look.
>
>Art, Any update to close on this?
>
>[snip]
>
>> > > > @@ -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.
>> >
>> > Hi Ville,
>> > Per skl spec, it is expecting even lines aligned with 90/270 rotation
>> > not only for NV12 but also for 422 formats. Perhaps we might have
>> > missed when 90/270 enabled for packed YUV formats.
>>
>> The src coordinates are always in the fb orientation, so macropixels appear in
>> the src.x direction only. And when we do 90/270 rotation the hardware Y offset
>> comes from src.x coordinates.
>>
>> The spec does seem a bit confused though; It claims the X offset must always be
>> even for YUV422+NV12, and the Y offset must be even when rotated 90/270
>> degrees. I suspect the X offset text just didn't get updated when 90/270 rotation
>> was added.
>>
>> Art, can you confirm?
>>
>> --
>> Ville Syrjälä
>> Intel OTC
Chandra Konduru May 19, 2015, 11:12 p.m. UTC | #7
> -----Original Message-----
> From: Runyan, Arthur J
> Sent: Monday, May 18, 2015 12:19 PM
> To: Konduru, Chandra; Ville Syrjälä
> Cc: intel-gfx@lists.freedesktop.org; Vetter, Daniel; Syrjala, Ville; Jindal, Sonika
> Subject: RE: [Intel-gfx] [PATCH 2/2] drm/i915: Add 90/270 rotation for NV12
> format.
> 
> The statement is correct - " the X offset must always be even for YUV422+NV12,
> and the Y offset must be even when rotated 90/270 degrees."

Thanks Art.
Then below code to take care evenness for both X and Y offsets when YUV 90/270 
is ok.

Ville, with this, can you give R-b tag on the ones you reviewed? 

> 
> >From: Konduru, Chandra
> >> From: Runyan, Arthur J
> >>
> >> I'll take a look.
> >
> >Art, Any update to close on this?
> >
> >[snip]
> >
> >> > > > @@ -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.
> >> >
> >> > Hi Ville,
> >> > Per skl spec, it is expecting even lines aligned with 90/270 rotation
> >> > not only for NV12 but also for 422 formats. Perhaps we might have
> >> > missed when 90/270 enabled for packed YUV formats.
> >>
> >> The src coordinates are always in the fb orientation, so macropixels appear in
> >> the src.x direction only. And when we do 90/270 rotation the hardware Y
> offset
> >> comes from src.x coordinates.
> >>
> >> The spec does seem a bit confused though; It claims the X offset must always
> be
> >> even for YUV422+NV12, and the Y offset must be even when rotated 90/270
> >> degrees. I suspect the X offset text just didn't get updated when 90/270
> rotation
> >> was added.
> >>
> >> Art, can you confirm?
> >>
> >> --
> >> Ville Syrjälä
> >> Intel OTC
Ville Syrjala May 20, 2015, 12:27 p.m. UTC | #8
On Mon, May 18, 2015 at 07:19:19PM +0000, Runyan, Arthur J wrote:
> The statement is correct - " the X offset must always be even for YUV422+NV12, and the Y offset must be even when rotated 90/270 degrees."

Hmm. Can you elaborate a bit? I'm curious where this limitation comes
from.

> 
> >From: Konduru, Chandra
> >> From: Runyan, Arthur J
> >>
> >> I'll take a look.
> >
> >Art, Any update to close on this?
> >
> >[snip]
> >
> >> > > > @@ -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.
> >> >
> >> > Hi Ville,
> >> > Per skl spec, it is expecting even lines aligned with 90/270 rotation
> >> > not only for NV12 but also for 422 formats. Perhaps we might have
> >> > missed when 90/270 enabled for packed YUV formats.
> >>
> >> The src coordinates are always in the fb orientation, so macropixels appear in
> >> the src.x direction only. And when we do 90/270 rotation the hardware Y offset
> >> comes from src.x coordinates.
> >>
> >> The spec does seem a bit confused though; It claims the X offset must always be
> >> even for YUV422+NV12, and the Y offset must be even when rotated 90/270
> >> degrees. I suspect the X offset text just didn't get updated when 90/270 rotation
> >> was added.
> >>
> >> Art, can you confirm?
> >>
> >> --
> >> Ville Syrjälä
> >> Intel OTC
Runyan, Arthur J May 22, 2015, 6:07 p.m. UTC | #9
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]

>On Mon, May 18, 2015 at 07:19:19PM +0000, Runyan, Arthur J wrote:
>> The statement is correct - " the X offset must always be even for YUV422+NV12,
>and the Y offset must be even when rotated 90/270 degrees."
>
>Hmm. Can you elaborate a bit? I'm curious where this limitation comes
>from.
>
Ah, I get what you are asking now.   They don't both have to be even when roated.  The text should say " the X offset must be even for YUV422+NV12 ***when not rotated 90/270***, and the Y offset must be even when rotated 90/270 degrees."  I'll fix that text.
Ville Syrjala May 22, 2015, 6:28 p.m. UTC | #10
On Fri, May 22, 2015 at 06:07:36PM +0000, Runyan, Arthur J wrote:
> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> 
> >On Mon, May 18, 2015 at 07:19:19PM +0000, Runyan, Arthur J wrote:
> >> The statement is correct - " the X offset must always be even for YUV422+NV12,
> >and the Y offset must be even when rotated 90/270 degrees."
> >
> >Hmm. Can you elaborate a bit? I'm curious where this limitation comes
> >from.
> >
> Ah, I get what you are asking now.   They don't both have to be even when roated.  The text should say " the X offset must be even for YUV422+NV12 ***when not rotated 90/270***, and the Y offset must be even when rotated 90/270 degrees."  I'll fix that text.

Excellent. Thanks for confirming my hunch :)

Patch
diff mbox

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;
+		}
 	}
 
 	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;
+			}
 		}
 	}