All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] Add NV12 90/270 support for skl planes
@ 2015-05-09  3:22 Chandra Konduru
  2015-05-09  3:22 ` [PATCH 1/2] drm/i915: call intel_tile_height with correct parameter Chandra Konduru
  2015-05-09  3:22 ` [PATCH 2/2] drm/i915: Add 90/270 rotation for NV12 format Chandra Konduru
  0 siblings, 2 replies; 15+ messages in thread
From: Chandra Konduru @ 2015-05-09  3:22 UTC (permalink / raw)
  To: intel-gfx; +Cc: daniel.vetter, ville.syrjala

First attempt to enabling 90/270 rotation for NV12 format using Tvrtko's
recent rotated mapping for NV12.

Calling intel_plane_obj_offset(plane, obj, 1) is causing kernel NULL
pointer reference. Sending early out early for Tvrtko to work on the
issue.

Very first NV12 flip with 90/270 rotation giving rotated image but
UV isn't there because of above issue. Also subsequent flips are causing
crashes and lockups.

Pending work:
 * few changes are need to DDB calcuations for NV12 90/270.

Chandra Konduru (2):
  drm/i915: call intel_tile_height with correct parameter
  drm/i915: Add 90/270 rotation for NV12 format.

 drivers/gpu/drm/i915/intel_display.c |   25 +++++++++++++++++--------
 drivers/gpu/drm/i915/intel_sprite.c  |   34 ++++++++++++++++++++++++++--------
 2 files changed, 43 insertions(+), 16 deletions(-)

-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/2] drm/i915: call intel_tile_height with correct parameter
  2015-05-09  3:22 [PATCH 0/2] Add NV12 90/270 support for skl planes Chandra Konduru
@ 2015-05-09  3:22 ` Chandra Konduru
  2015-05-11  9:41   ` Tvrtko Ursulin
  2015-05-11 10:28   ` Daniel Vetter
  2015-05-09  3:22 ` [PATCH 2/2] drm/i915: Add 90/270 rotation for NV12 format Chandra Konduru
  1 sibling, 2 replies; 15+ messages in thread
From: Chandra Konduru @ 2015-05-09  3:22 UTC (permalink / raw)
  To: intel-gfx; +Cc: daniel.vetter, ville.syrjala

In skylake update plane functions, intel_tile_height() is called with
bits_per_pixel instead of pixel_format. Correcting it.

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    2 +-
 drivers/gpu/drm/i915/intel_sprite.c  |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 66c78b6..c385a3b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3157,7 +3157,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
 
 	if (intel_rotation_90_or_270(rotation)) {
 		/* stride = Surface height in tiles */
-		tile_height = intel_tile_height(dev, fb->bits_per_pixel,
+		tile_height = intel_tile_height(dev, fb->pixel_format,
 						fb->modifier[0], 0);
 		stride = DIV_ROUND_UP(fb->height, tile_height);
 		x_offset = stride * tile_height - y - src_h;
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index fc1505b7..e23fe8e 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -232,7 +232,7 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 
 	if (intel_rotation_90_or_270(rotation)) {
 		/* stride: Surface height in tiles */
-		tile_height = intel_tile_height(dev, fb->bits_per_pixel,
+		tile_height = intel_tile_height(dev, fb->pixel_format,
 						fb->modifier[0], 0);
 		stride = DIV_ROUND_UP(fb->height, tile_height);
 		plane_size = (src_w << 16) | src_h;
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/2] drm/i915: Add 90/270 rotation for NV12 format.
  2015-05-09  3:22 [PATCH 0/2] Add NV12 90/270 support for skl planes Chandra Konduru
  2015-05-09  3:22 ` [PATCH 1/2] drm/i915: call intel_tile_height with correct parameter Chandra Konduru
@ 2015-05-09  3:22 ` Chandra Konduru
  2015-05-11 12:03   ` Ville Syrjälä
  1 sibling, 1 reply; 15+ messages in thread
From: Chandra Konduru @ 2015-05-09  3:22 UTC (permalink / raw)
  To: intel-gfx; +Cc: daniel.vetter, ville.syrjala

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] drm/i915: call intel_tile_height with correct parameter
  2015-05-09  3:22 ` [PATCH 1/2] drm/i915: call intel_tile_height with correct parameter Chandra Konduru
@ 2015-05-11  9:41   ` Tvrtko Ursulin
  2015-05-11 10:28   ` Daniel Vetter
  1 sibling, 0 replies; 15+ messages in thread
From: Tvrtko Ursulin @ 2015-05-11  9:41 UTC (permalink / raw)
  To: Chandra Konduru, intel-gfx; +Cc: daniel.vetter, ville.syrjala


On 05/09/2015 04:22 AM, Chandra Konduru wrote:
> In skylake update plane functions, intel_tile_height() is called with
> bits_per_pixel instead of pixel_format. Correcting it.
>
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c |    2 +-
>   drivers/gpu/drm/i915/intel_sprite.c  |    2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

This BTW fixes a bug in:

commit 3b7a5119b5d2def1161226a4c6a643db537dff7e
Author: Sonika Jindal <sonika.jindal@intel.com>
Date:   Fri Apr 10 14:37:29 2015 +0530

     drm/i915/skl: Support for 90/270 rotation

I've also posted last week "drm/i915: Remove duplicated 
intel_tile_height declaration" which fixes one part of the fallout, but 
didn't notice this as well.

So I think both of these fixes can be merged early.

A bit intriguing is how passing in bits per pixel instead of pixel 
format and things still worked. Don't see at the moment how tile height 
could have been correct for 90/270 as exercised from kms_rotation_crc.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] drm/i915: call intel_tile_height with correct parameter
  2015-05-09  3:22 ` [PATCH 1/2] drm/i915: call intel_tile_height with correct parameter Chandra Konduru
  2015-05-11  9:41   ` Tvrtko Ursulin
@ 2015-05-11 10:28   ` Daniel Vetter
  1 sibling, 0 replies; 15+ messages in thread
From: Daniel Vetter @ 2015-05-11 10:28 UTC (permalink / raw)
  To: Chandra Konduru; +Cc: daniel.vetter, intel-gfx, ville.syrjala

On Fri, May 08, 2015 at 08:22:46PM -0700, Chandra Konduru wrote:
> In skylake update plane functions, intel_tile_height() is called with
> bits_per_pixel instead of pixel_format. Correcting it.
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
Queued for -next, thanks for the patch.
> ---
>  drivers/gpu/drm/i915/intel_display.c |    2 +-
>  drivers/gpu/drm/i915/intel_sprite.c  |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 66c78b6..c385a3b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3157,7 +3157,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  
>  	if (intel_rotation_90_or_270(rotation)) {
>  		/* stride = Surface height in tiles */
> -		tile_height = intel_tile_height(dev, fb->bits_per_pixel,
> +		tile_height = intel_tile_height(dev, fb->pixel_format,
>  						fb->modifier[0], 0);
>  		stride = DIV_ROUND_UP(fb->height, tile_height);
>  		x_offset = stride * tile_height - y - src_h;
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index fc1505b7..e23fe8e 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -232,7 +232,7 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  
>  	if (intel_rotation_90_or_270(rotation)) {
>  		/* stride: Surface height in tiles */
> -		tile_height = intel_tile_height(dev, fb->bits_per_pixel,
> +		tile_height = intel_tile_height(dev, fb->pixel_format,
>  						fb->modifier[0], 0);

Alignment isn't properly patched up here, continuatino should line up with
the opening (. I've fixed this while applying.
-Daniel

>  		stride = DIV_ROUND_UP(fb->height, tile_height);
>  		plane_size = (src_w << 16) | src_h;
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] drm/i915: Add 90/270 rotation for NV12 format.
  2015-05-09  3:22 ` [PATCH 2/2] drm/i915: Add 90/270 rotation for NV12 format Chandra Konduru
@ 2015-05-11 12:03   ` Ville Syrjälä
  2015-05-11 23:32     ` Konduru, Chandra
  0 siblings, 1 reply; 15+ messages in thread
From: Ville Syrjälä @ 2015-05-11 12:03 UTC (permalink / raw)
  To: Chandra Konduru; +Cc: daniel.vetter, intel-gfx, ville.syrjala

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] drm/i915: Add 90/270 rotation for NV12 format.
  2015-05-11 12:03   ` Ville Syrjälä
@ 2015-05-11 23:32     ` Konduru, Chandra
  2015-05-12 10:44       ` Ville Syrjälä
  0 siblings, 1 reply; 15+ messages in thread
From: Konduru, Chandra @ 2015-05-11 23:32 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: Vetter, Daniel, intel-gfx, Syrjala, Ville


> -----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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] drm/i915: Add 90/270 rotation for NV12 format.
  2015-05-11 23:32     ` Konduru, Chandra
@ 2015-05-12 10:44       ` Ville Syrjälä
  2015-05-12 19:26         ` Runyan, Arthur J
  0 siblings, 1 reply; 15+ messages in thread
From: Ville Syrjälä @ 2015-05-12 10:44 UTC (permalink / raw)
  To: Konduru, Chandra
  Cc: Vetter, Daniel, intel-gfx, Runyan, Arthur J, Syrjala, Ville

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?

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] drm/i915: Add 90/270 rotation for NV12 format.
  2015-05-12 10:44       ` Ville Syrjälä
@ 2015-05-12 19:26         ` Runyan, Arthur J
  2015-05-15 21:58           ` Konduru, Chandra
  0 siblings, 1 reply; 15+ messages in thread
From: Runyan, Arthur J @ 2015-05-12 19:26 UTC (permalink / raw)
  To: Ville Syrjälä, Konduru, Chandra
  Cc: Vetter, Daniel, intel-gfx, Syrjala, Ville

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?

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] drm/i915: Add 90/270 rotation for NV12 format.
  2015-05-12 19:26         ` Runyan, Arthur J
@ 2015-05-15 21:58           ` Konduru, Chandra
  2015-05-18 19:19             ` Runyan, Arthur J
  0 siblings, 1 reply; 15+ messages in thread
From: Konduru, Chandra @ 2015-05-15 21:58 UTC (permalink / raw)
  To: Runyan, Arthur J, Ville Syrjälä
  Cc: Vetter, Daniel, intel-gfx, Syrjala, Ville


> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] drm/i915: Add 90/270 rotation for NV12 format.
  2015-05-15 21:58           ` Konduru, Chandra
@ 2015-05-18 19:19             ` Runyan, Arthur J
  2015-05-19 23:12               ` Konduru, Chandra
  2015-05-20 12:27               ` Ville Syrjälä
  0 siblings, 2 replies; 15+ messages in thread
From: Runyan, Arthur J @ 2015-05-18 19:19 UTC (permalink / raw)
  To: Konduru, Chandra, Ville Syrjälä
  Cc: Vetter, Daniel, intel-gfx, Syrjala, Ville

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] drm/i915: Add 90/270 rotation for NV12 format.
  2015-05-18 19:19             ` Runyan, Arthur J
@ 2015-05-19 23:12               ` Konduru, Chandra
  2015-05-20 12:27               ` Ville Syrjälä
  1 sibling, 0 replies; 15+ messages in thread
From: Konduru, Chandra @ 2015-05-19 23:12 UTC (permalink / raw)
  To: Runyan, Arthur J, Ville Syrjälä
  Cc: Vetter, Daniel, intel-gfx, Syrjala, Ville



> -----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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] drm/i915: Add 90/270 rotation for NV12 format.
  2015-05-18 19:19             ` Runyan, Arthur J
  2015-05-19 23:12               ` Konduru, Chandra
@ 2015-05-20 12:27               ` Ville Syrjälä
  2015-05-22 18:07                 ` Runyan, Arthur J
  1 sibling, 1 reply; 15+ messages in thread
From: Ville Syrjälä @ 2015-05-20 12:27 UTC (permalink / raw)
  To: Runyan, Arthur J; +Cc: Vetter, Daniel, intel-gfx, Syrjala, Ville

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

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] drm/i915: Add 90/270 rotation for NV12 format.
  2015-05-20 12:27               ` Ville Syrjälä
@ 2015-05-22 18:07                 ` Runyan, Arthur J
  2015-05-22 18:28                   ` Ville Syrjälä
  0 siblings, 1 reply; 15+ messages in thread
From: Runyan, Arthur J @ 2015-05-22 18:07 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: Vetter, Daniel, intel-gfx, Syrjala, Ville

>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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] drm/i915: Add 90/270 rotation for NV12 format.
  2015-05-22 18:07                 ` Runyan, Arthur J
@ 2015-05-22 18:28                   ` Ville Syrjälä
  0 siblings, 0 replies; 15+ messages in thread
From: Ville Syrjälä @ 2015-05-22 18:28 UTC (permalink / raw)
  To: Runyan, Arthur J; +Cc: Vetter, Daniel, intel-gfx, Syrjala, Ville

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 :)

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2015-05-22 18:28 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-09  3:22 [PATCH 0/2] Add NV12 90/270 support for skl planes Chandra Konduru
2015-05-09  3:22 ` [PATCH 1/2] drm/i915: call intel_tile_height with correct parameter Chandra Konduru
2015-05-11  9:41   ` Tvrtko Ursulin
2015-05-11 10:28   ` Daniel Vetter
2015-05-09  3:22 ` [PATCH 2/2] drm/i915: Add 90/270 rotation for NV12 format Chandra Konduru
2015-05-11 12:03   ` Ville Syrjälä
2015-05-11 23:32     ` Konduru, Chandra
2015-05-12 10:44       ` Ville Syrjälä
2015-05-12 19:26         ` Runyan, Arthur J
2015-05-15 21:58           ` Konduru, Chandra
2015-05-18 19:19             ` Runyan, Arthur J
2015-05-19 23:12               ` Konduru, Chandra
2015-05-20 12:27               ` Ville Syrjälä
2015-05-22 18:07                 ` Runyan, Arthur J
2015-05-22 18:28                   ` Ville Syrjälä

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.