[PATCH 1/2] drm/i915/skl: Allow universal planes to position

[PATCH 1/2] drm/i915/skl: Allow universal planes to position

[Sonika Jindal]

Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ceb2e61..f0bbc22 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane,
 	struct drm_rect *dest = &state->dst;
 	struct drm_rect *src = &state->src;
 	const struct drm_rect *clip = &state->clip;
+	bool can_position = false;
 	int ret;
 
 	crtc = crtc ? crtc : plane->crtc;
 	intel_crtc = to_intel_crtc(crtc);
 
+	if (INTEL_INFO(dev)->gen >= 9)
+		can_position = true;
+
 	ret = drm_plane_helper_check_update(plane, crtc, fb,
 					    src, dest, clip,
 					    DRM_PLANE_HELPER_NO_SCALING,
 					    DRM_PLANE_HELPER_NO_SCALING,
-					    false, true, &state->visible);
+					    can_position, true,
+					    &state->visible);
 	if (ret)
 		return ret;
 
-- 
1.7.10.4

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

[PATCH 2/2] drm/i915/skl: Support for 90/270 rotation

[Sonika Jindal]

v2: Moving creation of property in a function, checking for 90/270
rotation simultaneously (Chris)
Letting primary plane to be positioned
v3: Adding if/else for 90/270 and rest params programming, adding check for
pixel_format, some cleanup (review comments)
v4: Adding right pixel_formats, using src_* params instead of crtc_* for offset
and size programming (Ville)
v5: Rebased on -nightly and Tvrtko's series for gtt remapping.
v6: Rebased on -nightly (Tvrtko's series merged)
v7: Moving pixel_format check to intel_atomic_plane_check (Matt)

Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h           |    2 +
 drivers/gpu/drm/i915/intel_atomic_plane.c |   24 ++++++++
 drivers/gpu/drm/i915/intel_display.c      |   88 ++++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_drv.h          |    6 ++
 drivers/gpu/drm/i915/intel_sprite.c       |   52 ++++++++++++-----
 5 files changed, 131 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index b522eb6..564bbd5 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4854,7 +4854,9 @@ enum skl_disp_power_wells {
 #define   PLANE_CTL_ALPHA_HW_PREMULTIPLY	(  3 << 4)
 #define   PLANE_CTL_ROTATE_MASK			0x3
 #define   PLANE_CTL_ROTATE_0			0x0
+#define   PLANE_CTL_ROTATE_90			0x1
 #define   PLANE_CTL_ROTATE_180			0x2
+#define   PLANE_CTL_ROTATE_270			0x3
 #define _PLANE_STRIDE_1_A			0x70188
 #define _PLANE_STRIDE_2_A			0x70288
 #define _PLANE_STRIDE_3_A			0x70388
diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index 976b891..a27ee8c 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -162,6 +162,30 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 			(1 << drm_plane_index(plane));
 	}
 
+	if (state->fb && intel_rotation_90_or_270(state->rotation)) {
+		if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
+			state->fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
+			DRM_DEBUG_KMS("Y/Yf tiling required for 90/270!\n");
+			return -EINVAL;
+		}
+
+		/*
+		 * 90/270 is not allowed with RGB64 16:16:16:16,
+		 * RGB 16-bit 5:6:5, and Indexed 8-bit.
+		 * TBD: Add RGB64 case once its added in supported format list.
+		 */
+		switch (state->fb->pixel_format) {
+		case DRM_FORMAT_C8:
+		case DRM_FORMAT_RGB565:
+			DRM_DEBUG_KMS("Unsupported pixel format %s for 90/270!\n",
+					drm_get_format_name(state->fb->pixel_format));
+			return -EINVAL;
+
+		default:
+			break;
+		}
+	}
+
 	return intel_plane->check_plane(plane, intel_state);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f0bbc22..4de544c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2311,13 +2311,6 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb,
 	info->pitch = fb->pitches[0];
 	info->fb_modifier = fb->modifier[0];
 
-	if (!(info->fb_modifier == I915_FORMAT_MOD_Y_TILED ||
-	      info->fb_modifier == I915_FORMAT_MOD_Yf_TILED)) {
-		DRM_DEBUG_KMS(
-			      "Y or Yf tiling is needed for 90/270 rotation!\n");
-		return -EINVAL;
-	}
-
 	return 0;
 }
 
@@ -2919,8 +2912,12 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct drm_i915_gem_object *obj;
 	int pipe = intel_crtc->pipe;
-	u32 plane_ctl, stride_div;
+	u32 plane_ctl, stride_div, stride;
+	u32 tile_height, plane_offset, plane_size;
+	unsigned int rotation;
+	int x_offset, y_offset;
 	unsigned long surf_addr;
+	struct drm_plane *plane;
 
 	if (!intel_crtc->primary_enabled) {
 		I915_WRITE(PLANE_CTL(pipe, 0), 0);
@@ -2981,21 +2978,51 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
 	}
 
 	plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
-	if (crtc->primary->state->rotation == BIT(DRM_ROTATE_180))
+
+	plane = crtc->primary;
+	rotation = plane->state->rotation;
+	switch (rotation) {
+	case BIT(DRM_ROTATE_90):
+		plane_ctl |= PLANE_CTL_ROTATE_90;
+		break;
+
+	case BIT(DRM_ROTATE_180):
 		plane_ctl |= PLANE_CTL_ROTATE_180;
+		break;
+
+	case BIT(DRM_ROTATE_270):
+		plane_ctl |= PLANE_CTL_ROTATE_270;
+		break;
+	}
 
 	obj = intel_fb_obj(fb);
 	stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
 					       fb->pixel_format);
-	surf_addr = intel_plane_obj_offset(to_intel_plane(crtc->primary), obj);
+	surf_addr = intel_plane_obj_offset(to_intel_plane(plane), obj);
+
+	if (intel_rotation_90_or_270(rotation)) {
+		/* stride = Surface height in tiles */
+		tile_height = intel_tile_height(dev, fb->bits_per_pixel,
+						fb->modifier[0]);
+		stride = DIV_ROUND_UP(fb->height, tile_height);
+		x_offset = stride * tile_height - y - (plane->state->src_h >> 16);
+		y_offset = x;
+		plane_size = ((plane->state->src_w >> 16) - 1) << 16 |
+					((plane->state->src_h >> 16) - 1);
+	} else {
+		stride = fb->pitches[0] / stride_div;
+		x_offset = x;
+		y_offset = y;
+		plane_size = ((plane->state->src_h >> 16) - 1) << 16 |
+			((plane->state->src_w >> 16) - 1);
+	}
+	plane_offset = y_offset << 16 | x_offset;
 
 	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
 	I915_WRITE(PLANE_POS(pipe, 0), 0);
-	I915_WRITE(PLANE_OFFSET(pipe, 0), (y << 16) | x);
-	I915_WRITE(PLANE_SIZE(pipe, 0),
-		   (intel_crtc->config->pipe_src_h - 1) << 16 |
-		   (intel_crtc->config->pipe_src_w - 1));
-	I915_WRITE(PLANE_STRIDE(pipe, 0), fb->pitches[0] / stride_div);
+	I915_WRITE(PLANE_OFFSET(pipe, 0), plane_offset);
+	I915_WRITE(PLANE_SIZE(pipe, 0), plane_size);
+	I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
 	I915_WRITE(PLANE_SURF(pipe, 0), surf_addr);
 
 	POSTING_READ(PLANE_SURF(pipe, 0));
@@ -12406,23 +12433,32 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
 				 intel_primary_formats, num_formats,
 				 DRM_PLANE_TYPE_PRIMARY);
 
-	if (INTEL_INFO(dev)->gen >= 4) {
-		if (!dev->mode_config.rotation_property)
-			dev->mode_config.rotation_property =
-				drm_mode_create_rotation_property(dev,
-							BIT(DRM_ROTATE_0) |
-							BIT(DRM_ROTATE_180));
-		if (dev->mode_config.rotation_property)
-			drm_object_attach_property(&primary->base.base,
-				dev->mode_config.rotation_property,
-				state->base.rotation);
-	}
+	if (INTEL_INFO(dev)->gen >= 4)
+		intel_create_rotation_property(dev, primary);
 
 	drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs);
 
 	return &primary->base;
 }
 
+void intel_create_rotation_property(struct drm_device *dev, struct intel_plane *plane)
+{
+	if (!dev->mode_config.rotation_property) {
+		unsigned long flags = BIT(DRM_ROTATE_0) |
+			BIT(DRM_ROTATE_180);
+
+		if (INTEL_INFO(dev)->gen >= 9)
+			flags |= BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270);
+
+		dev->mode_config.rotation_property =
+			drm_mode_create_rotation_property(dev, flags);
+	}
+	if (dev->mode_config.rotation_property)
+		drm_object_attach_property(&plane->base.base,
+				dev->mode_config.rotation_property,
+				plane->base.state->rotation);
+}
+
 static int
 intel_check_cursor_plane(struct drm_plane *plane,
 			 struct intel_plane_state *state)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 811a1db..d32025a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -995,6 +995,12 @@ intel_rotation_90_or_270(unsigned int rotation)
 	return rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270));
 }
 
+unsigned int
+intel_tile_height(struct drm_device *dev, uint32_t bits_per_pixel,
+		  uint64_t fb_modifier);
+void intel_create_rotation_property(struct drm_device *dev,
+					struct intel_plane *plane);
+
 bool intel_wm_need_update(struct drm_plane *plane,
 			  struct drm_plane_state *state);
 
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index f41e872..83adc9b 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -190,10 +190,13 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	const int pipe = intel_plane->pipe;
 	const int plane = intel_plane->plane + 1;
-	u32 plane_ctl, stride_div;
+	u32 plane_ctl, stride_div, stride;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
 	const struct drm_intel_sprite_colorkey *key = &intel_plane->ckey;
 	unsigned long surf_addr;
+	u32 tile_height, plane_offset, plane_size;
+	unsigned int rotation;
+	int x_offset, y_offset;
 
 	plane_ctl = PLANE_CTL_ENABLE |
 		PLANE_CTL_PIPE_CSC_ENABLE;
@@ -254,8 +257,20 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 		MISSING_CASE(fb->modifier[0]);
 	}
 
-	if (drm_plane->state->rotation == BIT(DRM_ROTATE_180))
+	rotation = drm_plane->state->rotation;
+	switch (rotation) {
+	case BIT(DRM_ROTATE_90):
+		plane_ctl |= PLANE_CTL_ROTATE_90;
+		break;
+
+	case BIT(DRM_ROTATE_180):
 		plane_ctl |= PLANE_CTL_ROTATE_180;
+		break;
+
+	case BIT(DRM_ROTATE_270):
+		plane_ctl |= PLANE_CTL_ROTATE_270;
+		break;
+	}
 
 	intel_update_sprite_watermarks(drm_plane, crtc, src_w, src_h,
 				       pixel_size, true,
@@ -283,10 +298,26 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 
 	surf_addr = intel_plane_obj_offset(intel_plane, obj);
 
-	I915_WRITE(PLANE_OFFSET(pipe, plane), (y << 16) | x);
-	I915_WRITE(PLANE_STRIDE(pipe, plane), fb->pitches[0] / stride_div);
+	if (intel_rotation_90_or_270(rotation)) {
+		/* stride: Surface height in tiles */
+		tile_height = intel_tile_height(dev, fb->bits_per_pixel,
+							fb->modifier[0]);
+		stride = DIV_ROUND_UP(fb->height, tile_height);
+		plane_size = (src_w << 16) | src_h;
+		x_offset = stride * tile_height - y - (src_h + 1);
+		y_offset = x;
+	} else {
+		stride = fb->pitches[0] / stride_div;
+		plane_size = (src_h << 16) | src_w;
+		x_offset = x;
+		y_offset = y;
+	}
+	plane_offset = y_offset << 16 | x_offset;
+
+	I915_WRITE(PLANE_OFFSET(pipe, plane), plane_offset);
+	I915_WRITE(PLANE_STRIDE(pipe, plane), stride);
 	I915_WRITE(PLANE_POS(pipe, plane), (crtc_y << 16) | crtc_x);
-	I915_WRITE(PLANE_SIZE(pipe, plane), (crtc_h << 16) | crtc_w);
+	I915_WRITE(PLANE_SIZE(pipe, plane), plane_size);
 	I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl);
 	I915_WRITE(PLANE_SURF(pipe, plane), surf_addr);
 	POSTING_READ(PLANE_SURF(pipe, plane));
@@ -1310,16 +1341,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
 		goto out;
 	}
 
-	if (!dev->mode_config.rotation_property)
-		dev->mode_config.rotation_property =
-			drm_mode_create_rotation_property(dev,
-							  BIT(DRM_ROTATE_0) |
-							  BIT(DRM_ROTATE_180));
-
-	if (dev->mode_config.rotation_property)
-		drm_object_attach_property(&intel_plane->base.base,
-					   dev->mode_config.rotation_property,
-					   state->base.rotation);
+	intel_create_rotation_property(dev, intel_plane);
 
 	drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
 
-- 
1.7.10.4

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

Re: [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation

[Daniel Vetter]

On Fri, Apr 10, 2015 at 02:37:29PM +0530, Sonika Jindal wrote:
> v2: Moving creation of property in a function, checking for 90/270
> rotation simultaneously (Chris)
> Letting primary plane to be positioned
> v3: Adding if/else for 90/270 and rest params programming, adding check for
> pixel_format, some cleanup (review comments)
> v4: Adding right pixel_formats, using src_* params instead of crtc_* for offset
> and size programming (Ville)
> v5: Rebased on -nightly and Tvrtko's series for gtt remapping.
> v6: Rebased on -nightly (Tvrtko's series merged)
> v7: Moving pixel_format check to intel_atomic_plane_check (Matt)
> 
> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

Patches are missing the Testcase: tag, please add that. Also, are all the
igt committed or not yet? Pulled these two in meanwhile.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_reg.h           |    2 +
>  drivers/gpu/drm/i915/intel_atomic_plane.c |   24 ++++++++
>  drivers/gpu/drm/i915/intel_display.c      |   88 ++++++++++++++++++++---------
>  drivers/gpu/drm/i915/intel_drv.h          |    6 ++
>  drivers/gpu/drm/i915/intel_sprite.c       |   52 ++++++++++++-----
>  5 files changed, 131 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index b522eb6..564bbd5 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4854,7 +4854,9 @@ enum skl_disp_power_wells {
>  #define   PLANE_CTL_ALPHA_HW_PREMULTIPLY	(  3 << 4)
>  #define   PLANE_CTL_ROTATE_MASK			0x3
>  #define   PLANE_CTL_ROTATE_0			0x0
> +#define   PLANE_CTL_ROTATE_90			0x1
>  #define   PLANE_CTL_ROTATE_180			0x2
> +#define   PLANE_CTL_ROTATE_270			0x3
>  #define _PLANE_STRIDE_1_A			0x70188
>  #define _PLANE_STRIDE_2_A			0x70288
>  #define _PLANE_STRIDE_3_A			0x70388
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index 976b891..a27ee8c 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -162,6 +162,30 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>  			(1 << drm_plane_index(plane));
>  	}
>  
> +	if (state->fb && intel_rotation_90_or_270(state->rotation)) {
> +		if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
> +			state->fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
> +			DRM_DEBUG_KMS("Y/Yf tiling required for 90/270!\n");
> +			return -EINVAL;
> +		}
> +
> +		/*
> +		 * 90/270 is not allowed with RGB64 16:16:16:16,
> +		 * RGB 16-bit 5:6:5, and Indexed 8-bit.
> +		 * TBD: Add RGB64 case once its added in supported format list.
> +		 */
> +		switch (state->fb->pixel_format) {
> +		case DRM_FORMAT_C8:
> +		case DRM_FORMAT_RGB565:
> +			DRM_DEBUG_KMS("Unsupported pixel format %s for 90/270!\n",
> +					drm_get_format_name(state->fb->pixel_format));
> +			return -EINVAL;
> +
> +		default:
> +			break;
> +		}
> +	}
> +
>  	return intel_plane->check_plane(plane, intel_state);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f0bbc22..4de544c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2311,13 +2311,6 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb,
>  	info->pitch = fb->pitches[0];
>  	info->fb_modifier = fb->modifier[0];
>  
> -	if (!(info->fb_modifier == I915_FORMAT_MOD_Y_TILED ||
> -	      info->fb_modifier == I915_FORMAT_MOD_Yf_TILED)) {
> -		DRM_DEBUG_KMS(
> -			      "Y or Yf tiling is needed for 90/270 rotation!\n");
> -		return -EINVAL;
> -	}
> -
>  	return 0;
>  }
>  
> @@ -2919,8 +2912,12 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct drm_i915_gem_object *obj;
>  	int pipe = intel_crtc->pipe;
> -	u32 plane_ctl, stride_div;
> +	u32 plane_ctl, stride_div, stride;
> +	u32 tile_height, plane_offset, plane_size;
> +	unsigned int rotation;
> +	int x_offset, y_offset;
>  	unsigned long surf_addr;
> +	struct drm_plane *plane;
>  
>  	if (!intel_crtc->primary_enabled) {
>  		I915_WRITE(PLANE_CTL(pipe, 0), 0);
> @@ -2981,21 +2978,51 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  	}
>  
>  	plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
> -	if (crtc->primary->state->rotation == BIT(DRM_ROTATE_180))
> +
> +	plane = crtc->primary;
> +	rotation = plane->state->rotation;
> +	switch (rotation) {
> +	case BIT(DRM_ROTATE_90):
> +		plane_ctl |= PLANE_CTL_ROTATE_90;
> +		break;
> +
> +	case BIT(DRM_ROTATE_180):
>  		plane_ctl |= PLANE_CTL_ROTATE_180;
> +		break;
> +
> +	case BIT(DRM_ROTATE_270):
> +		plane_ctl |= PLANE_CTL_ROTATE_270;
> +		break;
> +	}
>  
>  	obj = intel_fb_obj(fb);
>  	stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
>  					       fb->pixel_format);
> -	surf_addr = intel_plane_obj_offset(to_intel_plane(crtc->primary), obj);
> +	surf_addr = intel_plane_obj_offset(to_intel_plane(plane), obj);
> +
> +	if (intel_rotation_90_or_270(rotation)) {
> +		/* stride = Surface height in tiles */
> +		tile_height = intel_tile_height(dev, fb->bits_per_pixel,
> +						fb->modifier[0]);
> +		stride = DIV_ROUND_UP(fb->height, tile_height);
> +		x_offset = stride * tile_height - y - (plane->state->src_h >> 16);
> +		y_offset = x;
> +		plane_size = ((plane->state->src_w >> 16) - 1) << 16 |
> +					((plane->state->src_h >> 16) - 1);
> +	} else {
> +		stride = fb->pitches[0] / stride_div;
> +		x_offset = x;
> +		y_offset = y;
> +		plane_size = ((plane->state->src_h >> 16) - 1) << 16 |
> +			((plane->state->src_w >> 16) - 1);
> +	}
> +	plane_offset = y_offset << 16 | x_offset;
>  
>  	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
>  	I915_WRITE(PLANE_POS(pipe, 0), 0);
> -	I915_WRITE(PLANE_OFFSET(pipe, 0), (y << 16) | x);
> -	I915_WRITE(PLANE_SIZE(pipe, 0),
> -		   (intel_crtc->config->pipe_src_h - 1) << 16 |
> -		   (intel_crtc->config->pipe_src_w - 1));
> -	I915_WRITE(PLANE_STRIDE(pipe, 0), fb->pitches[0] / stride_div);
> +	I915_WRITE(PLANE_OFFSET(pipe, 0), plane_offset);
> +	I915_WRITE(PLANE_SIZE(pipe, 0), plane_size);
> +	I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
>  	I915_WRITE(PLANE_SURF(pipe, 0), surf_addr);
>  
>  	POSTING_READ(PLANE_SURF(pipe, 0));
> @@ -12406,23 +12433,32 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>  				 intel_primary_formats, num_formats,
>  				 DRM_PLANE_TYPE_PRIMARY);
>  
> -	if (INTEL_INFO(dev)->gen >= 4) {
> -		if (!dev->mode_config.rotation_property)
> -			dev->mode_config.rotation_property =
> -				drm_mode_create_rotation_property(dev,
> -							BIT(DRM_ROTATE_0) |
> -							BIT(DRM_ROTATE_180));
> -		if (dev->mode_config.rotation_property)
> -			drm_object_attach_property(&primary->base.base,
> -				dev->mode_config.rotation_property,
> -				state->base.rotation);
> -	}
> +	if (INTEL_INFO(dev)->gen >= 4)
> +		intel_create_rotation_property(dev, primary);
>  
>  	drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs);
>  
>  	return &primary->base;
>  }
>  
> +void intel_create_rotation_property(struct drm_device *dev, struct intel_plane *plane)
> +{
> +	if (!dev->mode_config.rotation_property) {
> +		unsigned long flags = BIT(DRM_ROTATE_0) |
> +			BIT(DRM_ROTATE_180);
> +
> +		if (INTEL_INFO(dev)->gen >= 9)
> +			flags |= BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270);
> +
> +		dev->mode_config.rotation_property =
> +			drm_mode_create_rotation_property(dev, flags);
> +	}
> +	if (dev->mode_config.rotation_property)
> +		drm_object_attach_property(&plane->base.base,
> +				dev->mode_config.rotation_property,
> +				plane->base.state->rotation);
> +}
> +
>  static int
>  intel_check_cursor_plane(struct drm_plane *plane,
>  			 struct intel_plane_state *state)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 811a1db..d32025a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -995,6 +995,12 @@ intel_rotation_90_or_270(unsigned int rotation)
>  	return rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270));
>  }
>  
> +unsigned int
> +intel_tile_height(struct drm_device *dev, uint32_t bits_per_pixel,
> +		  uint64_t fb_modifier);
> +void intel_create_rotation_property(struct drm_device *dev,
> +					struct intel_plane *plane);
> +
>  bool intel_wm_need_update(struct drm_plane *plane,
>  			  struct drm_plane_state *state);
>  
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index f41e872..83adc9b 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -190,10 +190,13 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>  	const int pipe = intel_plane->pipe;
>  	const int plane = intel_plane->plane + 1;
> -	u32 plane_ctl, stride_div;
> +	u32 plane_ctl, stride_div, stride;
>  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
>  	const struct drm_intel_sprite_colorkey *key = &intel_plane->ckey;
>  	unsigned long surf_addr;
> +	u32 tile_height, plane_offset, plane_size;
> +	unsigned int rotation;
> +	int x_offset, y_offset;
>  
>  	plane_ctl = PLANE_CTL_ENABLE |
>  		PLANE_CTL_PIPE_CSC_ENABLE;
> @@ -254,8 +257,20 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  		MISSING_CASE(fb->modifier[0]);
>  	}
>  
> -	if (drm_plane->state->rotation == BIT(DRM_ROTATE_180))
> +	rotation = drm_plane->state->rotation;
> +	switch (rotation) {
> +	case BIT(DRM_ROTATE_90):
> +		plane_ctl |= PLANE_CTL_ROTATE_90;
> +		break;
> +
> +	case BIT(DRM_ROTATE_180):
>  		plane_ctl |= PLANE_CTL_ROTATE_180;
> +		break;
> +
> +	case BIT(DRM_ROTATE_270):
> +		plane_ctl |= PLANE_CTL_ROTATE_270;
> +		break;
> +	}
>  
>  	intel_update_sprite_watermarks(drm_plane, crtc, src_w, src_h,
>  				       pixel_size, true,
> @@ -283,10 +298,26 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  
>  	surf_addr = intel_plane_obj_offset(intel_plane, obj);
>  
> -	I915_WRITE(PLANE_OFFSET(pipe, plane), (y << 16) | x);
> -	I915_WRITE(PLANE_STRIDE(pipe, plane), fb->pitches[0] / stride_div);
> +	if (intel_rotation_90_or_270(rotation)) {
> +		/* stride: Surface height in tiles */
> +		tile_height = intel_tile_height(dev, fb->bits_per_pixel,
> +							fb->modifier[0]);
> +		stride = DIV_ROUND_UP(fb->height, tile_height);
> +		plane_size = (src_w << 16) | src_h;
> +		x_offset = stride * tile_height - y - (src_h + 1);
> +		y_offset = x;
> +	} else {
> +		stride = fb->pitches[0] / stride_div;
> +		plane_size = (src_h << 16) | src_w;
> +		x_offset = x;
> +		y_offset = y;
> +	}
> +	plane_offset = y_offset << 16 | x_offset;
> +
> +	I915_WRITE(PLANE_OFFSET(pipe, plane), plane_offset);
> +	I915_WRITE(PLANE_STRIDE(pipe, plane), stride);
>  	I915_WRITE(PLANE_POS(pipe, plane), (crtc_y << 16) | crtc_x);
> -	I915_WRITE(PLANE_SIZE(pipe, plane), (crtc_h << 16) | crtc_w);
> +	I915_WRITE(PLANE_SIZE(pipe, plane), plane_size);
>  	I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl);
>  	I915_WRITE(PLANE_SURF(pipe, plane), surf_addr);
>  	POSTING_READ(PLANE_SURF(pipe, plane));
> @@ -1310,16 +1341,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>  		goto out;
>  	}
>  
> -	if (!dev->mode_config.rotation_property)
> -		dev->mode_config.rotation_property =
> -			drm_mode_create_rotation_property(dev,
> -							  BIT(DRM_ROTATE_0) |
> -							  BIT(DRM_ROTATE_180));
> -
> -	if (dev->mode_config.rotation_property)
> -		drm_object_attach_property(&intel_plane->base.base,
> -					   dev->mode_config.rotation_property,
> -					   state->base.rotation);
> +	intel_create_rotation_property(dev, intel_plane);
>  
>  	drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
>  
> -- 
> 1.7.10.4
> 
> _______________________________________________
> 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

Re: [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation

[Ville Syrjälä]

On Fri, Apr 10, 2015 at 04:17:17PM +0200, Daniel Vetter wrote:
> On Fri, Apr 10, 2015 at 02:37:29PM +0530, Sonika Jindal wrote:
> > v2: Moving creation of property in a function, checking for 90/270
> > rotation simultaneously (Chris)
> > Letting primary plane to be positioned
> > v3: Adding if/else for 90/270 and rest params programming, adding check for
> > pixel_format, some cleanup (review comments)
> > v4: Adding right pixel_formats, using src_* params instead of crtc_* for offset
> > and size programming (Ville)
> > v5: Rebased on -nightly and Tvrtko's series for gtt remapping.
> > v6: Rebased on -nightly (Tvrtko's series merged)
> > v7: Moving pixel_format check to intel_atomic_plane_check (Matt)
> > 
> > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> > Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> 
> Patches are missing the Testcase: tag, please add that. Also, are all the
> igt committed or not yet? Pulled these two in meanwhile.

Things are going somewhat broken because you didn't apply my plane
state stuff. Hmm. Actually it sort of looks that it might work by luck
as long as the primary plane doesn't get clipped since this is bashing
the user state directly into the hardware registers and not the derived
state (ie. clipped coordinates).

Also I see my review comment about the 90 vs. 270 rotation mixup was
ignored at least.

> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h           |    2 +
> >  drivers/gpu/drm/i915/intel_atomic_plane.c |   24 ++++++++
> >  drivers/gpu/drm/i915/intel_display.c      |   88 ++++++++++++++++++++---------
> >  drivers/gpu/drm/i915/intel_drv.h          |    6 ++
> >  drivers/gpu/drm/i915/intel_sprite.c       |   52 ++++++++++++-----
> >  5 files changed, 131 insertions(+), 41 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index b522eb6..564bbd5 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4854,7 +4854,9 @@ enum skl_disp_power_wells {
> >  #define   PLANE_CTL_ALPHA_HW_PREMULTIPLY	(  3 << 4)
> >  #define   PLANE_CTL_ROTATE_MASK			0x3
> >  #define   PLANE_CTL_ROTATE_0			0x0
> > +#define   PLANE_CTL_ROTATE_90			0x1
> >  #define   PLANE_CTL_ROTATE_180			0x2
> > +#define   PLANE_CTL_ROTATE_270			0x3
> >  #define _PLANE_STRIDE_1_A			0x70188
> >  #define _PLANE_STRIDE_2_A			0x70288
> >  #define _PLANE_STRIDE_3_A			0x70388
> > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > index 976b891..a27ee8c 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > @@ -162,6 +162,30 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> >  			(1 << drm_plane_index(plane));
> >  	}
> >  
> > +	if (state->fb && intel_rotation_90_or_270(state->rotation)) {
> > +		if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
> > +			state->fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
> > +			DRM_DEBUG_KMS("Y/Yf tiling required for 90/270!\n");
> > +			return -EINVAL;
> > +		}
> > +
> > +		/*
> > +		 * 90/270 is not allowed with RGB64 16:16:16:16,
> > +		 * RGB 16-bit 5:6:5, and Indexed 8-bit.
> > +		 * TBD: Add RGB64 case once its added in supported format list.
> > +		 */
> > +		switch (state->fb->pixel_format) {
> > +		case DRM_FORMAT_C8:
> > +		case DRM_FORMAT_RGB565:
> > +			DRM_DEBUG_KMS("Unsupported pixel format %s for 90/270!\n",
> > +					drm_get_format_name(state->fb->pixel_format));
> > +			return -EINVAL;
> > +
> > +		default:
> > +			break;
> > +		}
> > +	}
> > +
> >  	return intel_plane->check_plane(plane, intel_state);
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index f0bbc22..4de544c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2311,13 +2311,6 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb,
> >  	info->pitch = fb->pitches[0];
> >  	info->fb_modifier = fb->modifier[0];
> >  
> > -	if (!(info->fb_modifier == I915_FORMAT_MOD_Y_TILED ||
> > -	      info->fb_modifier == I915_FORMAT_MOD_Yf_TILED)) {
> > -		DRM_DEBUG_KMS(
> > -			      "Y or Yf tiling is needed for 90/270 rotation!\n");
> > -		return -EINVAL;
> > -	}
> > -
> >  	return 0;
> >  }
> >  
> > @@ -2919,8 +2912,12 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	struct drm_i915_gem_object *obj;
> >  	int pipe = intel_crtc->pipe;
> > -	u32 plane_ctl, stride_div;
> > +	u32 plane_ctl, stride_div, stride;
> > +	u32 tile_height, plane_offset, plane_size;
> > +	unsigned int rotation;
> > +	int x_offset, y_offset;
> >  	unsigned long surf_addr;
> > +	struct drm_plane *plane;
> >  
> >  	if (!intel_crtc->primary_enabled) {
> >  		I915_WRITE(PLANE_CTL(pipe, 0), 0);
> > @@ -2981,21 +2978,51 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
> >  	}
> >  
> >  	plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
> > -	if (crtc->primary->state->rotation == BIT(DRM_ROTATE_180))
> > +
> > +	plane = crtc->primary;
> > +	rotation = plane->state->rotation;
> > +	switch (rotation) {
> > +	case BIT(DRM_ROTATE_90):
> > +		plane_ctl |= PLANE_CTL_ROTATE_90;
> > +		break;
> > +
> > +	case BIT(DRM_ROTATE_180):
> >  		plane_ctl |= PLANE_CTL_ROTATE_180;
> > +		break;
> > +
> > +	case BIT(DRM_ROTATE_270):
> > +		plane_ctl |= PLANE_CTL_ROTATE_270;
> > +		break;
> > +	}
> >  
> >  	obj = intel_fb_obj(fb);
> >  	stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
> >  					       fb->pixel_format);
> > -	surf_addr = intel_plane_obj_offset(to_intel_plane(crtc->primary), obj);
> > +	surf_addr = intel_plane_obj_offset(to_intel_plane(plane), obj);
> > +
> > +	if (intel_rotation_90_or_270(rotation)) {
> > +		/* stride = Surface height in tiles */
> > +		tile_height = intel_tile_height(dev, fb->bits_per_pixel,
> > +						fb->modifier[0]);
> > +		stride = DIV_ROUND_UP(fb->height, tile_height);
> > +		x_offset = stride * tile_height - y - (plane->state->src_h >> 16);
> > +		y_offset = x;
> > +		plane_size = ((plane->state->src_w >> 16) - 1) << 16 |
> > +					((plane->state->src_h >> 16) - 1);
> > +	} else {
> > +		stride = fb->pitches[0] / stride_div;
> > +		x_offset = x;
> > +		y_offset = y;
> > +		plane_size = ((plane->state->src_h >> 16) - 1) << 16 |
> > +			((plane->state->src_w >> 16) - 1);
> > +	}
> > +	plane_offset = y_offset << 16 | x_offset;
> >  
> >  	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
> >  	I915_WRITE(PLANE_POS(pipe, 0), 0);
> > -	I915_WRITE(PLANE_OFFSET(pipe, 0), (y << 16) | x);
> > -	I915_WRITE(PLANE_SIZE(pipe, 0),
> > -		   (intel_crtc->config->pipe_src_h - 1) << 16 |
> > -		   (intel_crtc->config->pipe_src_w - 1));
> > -	I915_WRITE(PLANE_STRIDE(pipe, 0), fb->pitches[0] / stride_div);
> > +	I915_WRITE(PLANE_OFFSET(pipe, 0), plane_offset);
> > +	I915_WRITE(PLANE_SIZE(pipe, 0), plane_size);
> > +	I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
> >  	I915_WRITE(PLANE_SURF(pipe, 0), surf_addr);
> >  
> >  	POSTING_READ(PLANE_SURF(pipe, 0));
> > @@ -12406,23 +12433,32 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> >  				 intel_primary_formats, num_formats,
> >  				 DRM_PLANE_TYPE_PRIMARY);
> >  
> > -	if (INTEL_INFO(dev)->gen >= 4) {
> > -		if (!dev->mode_config.rotation_property)
> > -			dev->mode_config.rotation_property =
> > -				drm_mode_create_rotation_property(dev,
> > -							BIT(DRM_ROTATE_0) |
> > -							BIT(DRM_ROTATE_180));
> > -		if (dev->mode_config.rotation_property)
> > -			drm_object_attach_property(&primary->base.base,
> > -				dev->mode_config.rotation_property,
> > -				state->base.rotation);
> > -	}
> > +	if (INTEL_INFO(dev)->gen >= 4)
> > +		intel_create_rotation_property(dev, primary);
> >  
> >  	drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs);
> >  
> >  	return &primary->base;
> >  }
> >  
> > +void intel_create_rotation_property(struct drm_device *dev, struct intel_plane *plane)
> > +{
> > +	if (!dev->mode_config.rotation_property) {
> > +		unsigned long flags = BIT(DRM_ROTATE_0) |
> > +			BIT(DRM_ROTATE_180);
> > +
> > +		if (INTEL_INFO(dev)->gen >= 9)
> > +			flags |= BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270);
> > +
> > +		dev->mode_config.rotation_property =
> > +			drm_mode_create_rotation_property(dev, flags);
> > +	}
> > +	if (dev->mode_config.rotation_property)
> > +		drm_object_attach_property(&plane->base.base,
> > +				dev->mode_config.rotation_property,
> > +				plane->base.state->rotation);
> > +}
> > +
> >  static int
> >  intel_check_cursor_plane(struct drm_plane *plane,
> >  			 struct intel_plane_state *state)
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 811a1db..d32025a 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -995,6 +995,12 @@ intel_rotation_90_or_270(unsigned int rotation)
> >  	return rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270));
> >  }
> >  
> > +unsigned int
> > +intel_tile_height(struct drm_device *dev, uint32_t bits_per_pixel,
> > +		  uint64_t fb_modifier);
> > +void intel_create_rotation_property(struct drm_device *dev,
> > +					struct intel_plane *plane);
> > +
> >  bool intel_wm_need_update(struct drm_plane *plane,
> >  			  struct drm_plane_state *state);
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index f41e872..83adc9b 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -190,10 +190,13 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
> >  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> >  	const int pipe = intel_plane->pipe;
> >  	const int plane = intel_plane->plane + 1;
> > -	u32 plane_ctl, stride_div;
> > +	u32 plane_ctl, stride_div, stride;
> >  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> >  	const struct drm_intel_sprite_colorkey *key = &intel_plane->ckey;
> >  	unsigned long surf_addr;
> > +	u32 tile_height, plane_offset, plane_size;
> > +	unsigned int rotation;
> > +	int x_offset, y_offset;
> >  
> >  	plane_ctl = PLANE_CTL_ENABLE |
> >  		PLANE_CTL_PIPE_CSC_ENABLE;
> > @@ -254,8 +257,20 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
> >  		MISSING_CASE(fb->modifier[0]);
> >  	}
> >  
> > -	if (drm_plane->state->rotation == BIT(DRM_ROTATE_180))
> > +	rotation = drm_plane->state->rotation;
> > +	switch (rotation) {
> > +	case BIT(DRM_ROTATE_90):
> > +		plane_ctl |= PLANE_CTL_ROTATE_90;
> > +		break;
> > +
> > +	case BIT(DRM_ROTATE_180):
> >  		plane_ctl |= PLANE_CTL_ROTATE_180;
> > +		break;
> > +
> > +	case BIT(DRM_ROTATE_270):
> > +		plane_ctl |= PLANE_CTL_ROTATE_270;
> > +		break;
> > +	}
> >  
> >  	intel_update_sprite_watermarks(drm_plane, crtc, src_w, src_h,
> >  				       pixel_size, true,
> > @@ -283,10 +298,26 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
> >  
> >  	surf_addr = intel_plane_obj_offset(intel_plane, obj);
> >  
> > -	I915_WRITE(PLANE_OFFSET(pipe, plane), (y << 16) | x);
> > -	I915_WRITE(PLANE_STRIDE(pipe, plane), fb->pitches[0] / stride_div);
> > +	if (intel_rotation_90_or_270(rotation)) {
> > +		/* stride: Surface height in tiles */
> > +		tile_height = intel_tile_height(dev, fb->bits_per_pixel,
> > +							fb->modifier[0]);
> > +		stride = DIV_ROUND_UP(fb->height, tile_height);
> > +		plane_size = (src_w << 16) | src_h;
> > +		x_offset = stride * tile_height - y - (src_h + 1);
> > +		y_offset = x;
> > +	} else {
> > +		stride = fb->pitches[0] / stride_div;
> > +		plane_size = (src_h << 16) | src_w;
> > +		x_offset = x;
> > +		y_offset = y;
> > +	}
> > +	plane_offset = y_offset << 16 | x_offset;
> > +
> > +	I915_WRITE(PLANE_OFFSET(pipe, plane), plane_offset);
> > +	I915_WRITE(PLANE_STRIDE(pipe, plane), stride);
> >  	I915_WRITE(PLANE_POS(pipe, plane), (crtc_y << 16) | crtc_x);
> > -	I915_WRITE(PLANE_SIZE(pipe, plane), (crtc_h << 16) | crtc_w);
> > +	I915_WRITE(PLANE_SIZE(pipe, plane), plane_size);
> >  	I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl);
> >  	I915_WRITE(PLANE_SURF(pipe, plane), surf_addr);
> >  	POSTING_READ(PLANE_SURF(pipe, plane));
> > @@ -1310,16 +1341,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> >  		goto out;
> >  	}
> >  
> > -	if (!dev->mode_config.rotation_property)
> > -		dev->mode_config.rotation_property =
> > -			drm_mode_create_rotation_property(dev,
> > -							  BIT(DRM_ROTATE_0) |
> > -							  BIT(DRM_ROTATE_180));
> > -
> > -	if (dev->mode_config.rotation_property)
> > -		drm_object_attach_property(&intel_plane->base.base,
> > -					   dev->mode_config.rotation_property,
> > -					   state->base.rotation);
> > +	intel_create_rotation_property(dev, intel_plane);
> >  
> >  	drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
> >  
> > -- 
> > 1.7.10.4
> > 
> > _______________________________________________
> > 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

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

Re: [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation

[Jindal, Sonika]

On 4/10/2015 8:14 PM, Ville Syrjälä wrote:
> On Fri, Apr 10, 2015 at 04:17:17PM +0200, Daniel Vetter wrote:
>> On Fri, Apr 10, 2015 at 02:37:29PM +0530, Sonika Jindal wrote:
>>> v2: Moving creation of property in a function, checking for 90/270
>>> rotation simultaneously (Chris)
>>> Letting primary plane to be positioned
>>> v3: Adding if/else for 90/270 and rest params programming, adding check for
>>> pixel_format, some cleanup (review comments)
>>> v4: Adding right pixel_formats, using src_* params instead of crtc_* for offset
>>> and size programming (Ville)
>>> v5: Rebased on -nightly and Tvrtko's series for gtt remapping.
>>> v6: Rebased on -nightly (Tvrtko's series merged)
>>> v7: Moving pixel_format check to intel_atomic_plane_check (Matt)
>>>
>>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
>>> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
>>
>> Patches are missing the Testcase: tag, please add that. Also, are all the
>> igt committed or not yet? Pulled these two in meanwhile.
>
> Things are going somewhat broken because you didn't apply my plane
> state stuff. Hmm. Actually it sort of looks that it might work by luck
> as long as the primary plane doesn't get clipped since this is bashing
> the user state directly into the hardware registers and not the derived
> state (ie. clipped coordinates).
>
I was hoping your changes get merged, but not sure why they are held up.

> Also I see my review comment about the 90 vs. 270 rotation mixup was
> ignored at least.
>
I never really got the to understand the need of reversing 90 and 270 :) 
The last discussion was not concluded, AFAIR.
Things look correct to me and work fine with the testcase also.
Is there something completely different which I am unable to get?

-Sonika

>> -Daniel
>>
>>> ---
>>>   drivers/gpu/drm/i915/i915_reg.h           |    2 +
>>>   drivers/gpu/drm/i915/intel_atomic_plane.c |   24 ++++++++
>>>   drivers/gpu/drm/i915/intel_display.c      |   88 ++++++++++++++++++++---------
>>>   drivers/gpu/drm/i915/intel_drv.h          |    6 ++
>>>   drivers/gpu/drm/i915/intel_sprite.c       |   52 ++++++++++++-----
>>>   5 files changed, 131 insertions(+), 41 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>> index b522eb6..564bbd5 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -4854,7 +4854,9 @@ enum skl_disp_power_wells {
>>>   #define   PLANE_CTL_ALPHA_HW_PREMULTIPLY	(  3 << 4)
>>>   #define   PLANE_CTL_ROTATE_MASK			0x3
>>>   #define   PLANE_CTL_ROTATE_0			0x0
>>> +#define   PLANE_CTL_ROTATE_90			0x1
>>>   #define   PLANE_CTL_ROTATE_180			0x2
>>> +#define   PLANE_CTL_ROTATE_270			0x3
>>>   #define _PLANE_STRIDE_1_A			0x70188
>>>   #define _PLANE_STRIDE_2_A			0x70288
>>>   #define _PLANE_STRIDE_3_A			0x70388
>>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
>>> index 976b891..a27ee8c 100644
>>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
>>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
>>> @@ -162,6 +162,30 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>>>   			(1 << drm_plane_index(plane));
>>>   	}
>>>
>>> +	if (state->fb && intel_rotation_90_or_270(state->rotation)) {
>>> +		if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
>>> +			state->fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
>>> +			DRM_DEBUG_KMS("Y/Yf tiling required for 90/270!\n");
>>> +			return -EINVAL;
>>> +		}
>>> +
>>> +		/*
>>> +		 * 90/270 is not allowed with RGB64 16:16:16:16,
>>> +		 * RGB 16-bit 5:6:5, and Indexed 8-bit.
>>> +		 * TBD: Add RGB64 case once its added in supported format list.
>>> +		 */
>>> +		switch (state->fb->pixel_format) {
>>> +		case DRM_FORMAT_C8:
>>> +		case DRM_FORMAT_RGB565:
>>> +			DRM_DEBUG_KMS("Unsupported pixel format %s for 90/270!\n",
>>> +					drm_get_format_name(state->fb->pixel_format));
>>> +			return -EINVAL;
>>> +
>>> +		default:
>>> +			break;
>>> +		}
>>> +	}
>>> +
>>>   	return intel_plane->check_plane(plane, intel_state);
>>>   }
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> index f0bbc22..4de544c 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -2311,13 +2311,6 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb,
>>>   	info->pitch = fb->pitches[0];
>>>   	info->fb_modifier = fb->modifier[0];
>>>
>>> -	if (!(info->fb_modifier == I915_FORMAT_MOD_Y_TILED ||
>>> -	      info->fb_modifier == I915_FORMAT_MOD_Yf_TILED)) {
>>> -		DRM_DEBUG_KMS(
>>> -			      "Y or Yf tiling is needed for 90/270 rotation!\n");
>>> -		return -EINVAL;
>>> -	}
>>> -
>>>   	return 0;
>>>   }
>>>
>>> @@ -2919,8 +2912,12 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>>>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>>   	struct drm_i915_gem_object *obj;
>>>   	int pipe = intel_crtc->pipe;
>>> -	u32 plane_ctl, stride_div;
>>> +	u32 plane_ctl, stride_div, stride;
>>> +	u32 tile_height, plane_offset, plane_size;
>>> +	unsigned int rotation;
>>> +	int x_offset, y_offset;
>>>   	unsigned long surf_addr;
>>> +	struct drm_plane *plane;
>>>
>>>   	if (!intel_crtc->primary_enabled) {
>>>   		I915_WRITE(PLANE_CTL(pipe, 0), 0);
>>> @@ -2981,21 +2978,51 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>>>   	}
>>>
>>>   	plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
>>> -	if (crtc->primary->state->rotation == BIT(DRM_ROTATE_180))
>>> +
>>> +	plane = crtc->primary;
>>> +	rotation = plane->state->rotation;
>>> +	switch (rotation) {
>>> +	case BIT(DRM_ROTATE_90):
>>> +		plane_ctl |= PLANE_CTL_ROTATE_90;
>>> +		break;
>>> +
>>> +	case BIT(DRM_ROTATE_180):
>>>   		plane_ctl |= PLANE_CTL_ROTATE_180;
>>> +		break;
>>> +
>>> +	case BIT(DRM_ROTATE_270):
>>> +		plane_ctl |= PLANE_CTL_ROTATE_270;
>>> +		break;
>>> +	}
>>>
>>>   	obj = intel_fb_obj(fb);
>>>   	stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
>>>   					       fb->pixel_format);
>>> -	surf_addr = intel_plane_obj_offset(to_intel_plane(crtc->primary), obj);
>>> +	surf_addr = intel_plane_obj_offset(to_intel_plane(plane), obj);
>>> +
>>> +	if (intel_rotation_90_or_270(rotation)) {
>>> +		/* stride = Surface height in tiles */
>>> +		tile_height = intel_tile_height(dev, fb->bits_per_pixel,
>>> +						fb->modifier[0]);
>>> +		stride = DIV_ROUND_UP(fb->height, tile_height);
>>> +		x_offset = stride * tile_height - y - (plane->state->src_h >> 16);
>>> +		y_offset = x;
>>> +		plane_size = ((plane->state->src_w >> 16) - 1) << 16 |
>>> +					((plane->state->src_h >> 16) - 1);
>>> +	} else {
>>> +		stride = fb->pitches[0] / stride_div;
>>> +		x_offset = x;
>>> +		y_offset = y;
>>> +		plane_size = ((plane->state->src_h >> 16) - 1) << 16 |
>>> +			((plane->state->src_w >> 16) - 1);
>>> +	}
>>> +	plane_offset = y_offset << 16 | x_offset;
>>>
>>>   	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
>>>   	I915_WRITE(PLANE_POS(pipe, 0), 0);
>>> -	I915_WRITE(PLANE_OFFSET(pipe, 0), (y << 16) | x);
>>> -	I915_WRITE(PLANE_SIZE(pipe, 0),
>>> -		   (intel_crtc->config->pipe_src_h - 1) << 16 |
>>> -		   (intel_crtc->config->pipe_src_w - 1));
>>> -	I915_WRITE(PLANE_STRIDE(pipe, 0), fb->pitches[0] / stride_div);
>>> +	I915_WRITE(PLANE_OFFSET(pipe, 0), plane_offset);
>>> +	I915_WRITE(PLANE_SIZE(pipe, 0), plane_size);
>>> +	I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
>>>   	I915_WRITE(PLANE_SURF(pipe, 0), surf_addr);
>>>
>>>   	POSTING_READ(PLANE_SURF(pipe, 0));
>>> @@ -12406,23 +12433,32 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>>>   				 intel_primary_formats, num_formats,
>>>   				 DRM_PLANE_TYPE_PRIMARY);
>>>
>>> -	if (INTEL_INFO(dev)->gen >= 4) {
>>> -		if (!dev->mode_config.rotation_property)
>>> -			dev->mode_config.rotation_property =
>>> -				drm_mode_create_rotation_property(dev,
>>> -							BIT(DRM_ROTATE_0) |
>>> -							BIT(DRM_ROTATE_180));
>>> -		if (dev->mode_config.rotation_property)
>>> -			drm_object_attach_property(&primary->base.base,
>>> -				dev->mode_config.rotation_property,
>>> -				state->base.rotation);
>>> -	}
>>> +	if (INTEL_INFO(dev)->gen >= 4)
>>> +		intel_create_rotation_property(dev, primary);
>>>
>>>   	drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs);
>>>
>>>   	return &primary->base;
>>>   }
>>>
>>> +void intel_create_rotation_property(struct drm_device *dev, struct intel_plane *plane)
>>> +{
>>> +	if (!dev->mode_config.rotation_property) {
>>> +		unsigned long flags = BIT(DRM_ROTATE_0) |
>>> +			BIT(DRM_ROTATE_180);
>>> +
>>> +		if (INTEL_INFO(dev)->gen >= 9)
>>> +			flags |= BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270);
>>> +
>>> +		dev->mode_config.rotation_property =
>>> +			drm_mode_create_rotation_property(dev, flags);
>>> +	}
>>> +	if (dev->mode_config.rotation_property)
>>> +		drm_object_attach_property(&plane->base.base,
>>> +				dev->mode_config.rotation_property,
>>> +				plane->base.state->rotation);
>>> +}
>>> +
>>>   static int
>>>   intel_check_cursor_plane(struct drm_plane *plane,
>>>   			 struct intel_plane_state *state)
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>> index 811a1db..d32025a 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -995,6 +995,12 @@ intel_rotation_90_or_270(unsigned int rotation)
>>>   	return rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270));
>>>   }
>>>
>>> +unsigned int
>>> +intel_tile_height(struct drm_device *dev, uint32_t bits_per_pixel,
>>> +		  uint64_t fb_modifier);
>>> +void intel_create_rotation_property(struct drm_device *dev,
>>> +					struct intel_plane *plane);
>>> +
>>>   bool intel_wm_need_update(struct drm_plane *plane,
>>>   			  struct drm_plane_state *state);
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>>> index f41e872..83adc9b 100644
>>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>>> @@ -190,10 +190,13 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>>>   	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>>>   	const int pipe = intel_plane->pipe;
>>>   	const int plane = intel_plane->plane + 1;
>>> -	u32 plane_ctl, stride_div;
>>> +	u32 plane_ctl, stride_div, stride;
>>>   	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
>>>   	const struct drm_intel_sprite_colorkey *key = &intel_plane->ckey;
>>>   	unsigned long surf_addr;
>>> +	u32 tile_height, plane_offset, plane_size;
>>> +	unsigned int rotation;
>>> +	int x_offset, y_offset;
>>>
>>>   	plane_ctl = PLANE_CTL_ENABLE |
>>>   		PLANE_CTL_PIPE_CSC_ENABLE;
>>> @@ -254,8 +257,20 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>>>   		MISSING_CASE(fb->modifier[0]);
>>>   	}
>>>
>>> -	if (drm_plane->state->rotation == BIT(DRM_ROTATE_180))
>>> +	rotation = drm_plane->state->rotation;
>>> +	switch (rotation) {
>>> +	case BIT(DRM_ROTATE_90):
>>> +		plane_ctl |= PLANE_CTL_ROTATE_90;
>>> +		break;
>>> +
>>> +	case BIT(DRM_ROTATE_180):
>>>   		plane_ctl |= PLANE_CTL_ROTATE_180;
>>> +		break;
>>> +
>>> +	case BIT(DRM_ROTATE_270):
>>> +		plane_ctl |= PLANE_CTL_ROTATE_270;
>>> +		break;
>>> +	}
>>>
>>>   	intel_update_sprite_watermarks(drm_plane, crtc, src_w, src_h,
>>>   				       pixel_size, true,
>>> @@ -283,10 +298,26 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>>>
>>>   	surf_addr = intel_plane_obj_offset(intel_plane, obj);
>>>
>>> -	I915_WRITE(PLANE_OFFSET(pipe, plane), (y << 16) | x);
>>> -	I915_WRITE(PLANE_STRIDE(pipe, plane), fb->pitches[0] / stride_div);
>>> +	if (intel_rotation_90_or_270(rotation)) {
>>> +		/* stride: Surface height in tiles */
>>> +		tile_height = intel_tile_height(dev, fb->bits_per_pixel,
>>> +							fb->modifier[0]);
>>> +		stride = DIV_ROUND_UP(fb->height, tile_height);
>>> +		plane_size = (src_w << 16) | src_h;
>>> +		x_offset = stride * tile_height - y - (src_h + 1);
>>> +		y_offset = x;
>>> +	} else {
>>> +		stride = fb->pitches[0] / stride_div;
>>> +		plane_size = (src_h << 16) | src_w;
>>> +		x_offset = x;
>>> +		y_offset = y;
>>> +	}
>>> +	plane_offset = y_offset << 16 | x_offset;
>>> +
>>> +	I915_WRITE(PLANE_OFFSET(pipe, plane), plane_offset);
>>> +	I915_WRITE(PLANE_STRIDE(pipe, plane), stride);
>>>   	I915_WRITE(PLANE_POS(pipe, plane), (crtc_y << 16) | crtc_x);
>>> -	I915_WRITE(PLANE_SIZE(pipe, plane), (crtc_h << 16) | crtc_w);
>>> +	I915_WRITE(PLANE_SIZE(pipe, plane), plane_size);
>>>   	I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl);
>>>   	I915_WRITE(PLANE_SURF(pipe, plane), surf_addr);
>>>   	POSTING_READ(PLANE_SURF(pipe, plane));
>>> @@ -1310,16 +1341,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>>>   		goto out;
>>>   	}
>>>
>>> -	if (!dev->mode_config.rotation_property)
>>> -		dev->mode_config.rotation_property =
>>> -			drm_mode_create_rotation_property(dev,
>>> -							  BIT(DRM_ROTATE_0) |
>>> -							  BIT(DRM_ROTATE_180));
>>> -
>>> -	if (dev->mode_config.rotation_property)
>>> -		drm_object_attach_property(&intel_plane->base.base,
>>> -					   dev->mode_config.rotation_property,
>>> -					   state->base.rotation);
>>> +	intel_create_rotation_property(dev, intel_plane);
>>>
>>>   	drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
>>>
>>> --
>>> 1.7.10.4
>>>
>>> _______________________________________________
>>> 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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation

[Ville Syrjälä]

On Mon, Apr 13, 2015 at 09:36:02AM +0530, Jindal, Sonika wrote:
> 
> 
> On 4/10/2015 8:14 PM, Ville Syrjälä wrote:
> > On Fri, Apr 10, 2015 at 04:17:17PM +0200, Daniel Vetter wrote:
> >> On Fri, Apr 10, 2015 at 02:37:29PM +0530, Sonika Jindal wrote:
> >>> v2: Moving creation of property in a function, checking for 90/270
> >>> rotation simultaneously (Chris)
> >>> Letting primary plane to be positioned
> >>> v3: Adding if/else for 90/270 and rest params programming, adding check for
> >>> pixel_format, some cleanup (review comments)
> >>> v4: Adding right pixel_formats, using src_* params instead of crtc_* for offset
> >>> and size programming (Ville)
> >>> v5: Rebased on -nightly and Tvrtko's series for gtt remapping.
> >>> v6: Rebased on -nightly (Tvrtko's series merged)
> >>> v7: Moving pixel_format check to intel_atomic_plane_check (Matt)
> >>>
> >>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> >>> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> >>
> >> Patches are missing the Testcase: tag, please add that. Also, are all the
> >> igt committed or not yet? Pulled these two in meanwhile.
> >
> > Things are going somewhat broken because you didn't apply my plane
> > state stuff. Hmm. Actually it sort of looks that it might work by luck
> > as long as the primary plane doesn't get clipped since this is bashing
> > the user state directly into the hardware registers and not the derived
> > state (ie. clipped coordinates).
> >
> I was hoping your changes get merged, but not sure why they are held up.
> 
> > Also I see my review comment about the 90 vs. 270 rotation mixup was
> > ignored at least.
> >
> I never really got the to understand the need of reversing 90 and 270 :) 
> The last discussion was not concluded, AFAIR.
> Things look correct to me and work fine with the testcase also.
> Is there something completely different which I am unable to get?

BSpec gives me the impression the hw rotation is cw, whereas the drm one
is ccw.

> 
> -Sonika
> 
> >> -Daniel
> >>
> >>> ---
> >>>   drivers/gpu/drm/i915/i915_reg.h           |    2 +
> >>>   drivers/gpu/drm/i915/intel_atomic_plane.c |   24 ++++++++
> >>>   drivers/gpu/drm/i915/intel_display.c      |   88 ++++++++++++++++++++---------
> >>>   drivers/gpu/drm/i915/intel_drv.h          |    6 ++
> >>>   drivers/gpu/drm/i915/intel_sprite.c       |   52 ++++++++++++-----
> >>>   5 files changed, 131 insertions(+), 41 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >>> index b522eb6..564bbd5 100644
> >>> --- a/drivers/gpu/drm/i915/i915_reg.h
> >>> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >>> @@ -4854,7 +4854,9 @@ enum skl_disp_power_wells {
> >>>   #define   PLANE_CTL_ALPHA_HW_PREMULTIPLY	(  3 << 4)
> >>>   #define   PLANE_CTL_ROTATE_MASK			0x3
> >>>   #define   PLANE_CTL_ROTATE_0			0x0
> >>> +#define   PLANE_CTL_ROTATE_90			0x1
> >>>   #define   PLANE_CTL_ROTATE_180			0x2
> >>> +#define   PLANE_CTL_ROTATE_270			0x3
> >>>   #define _PLANE_STRIDE_1_A			0x70188
> >>>   #define _PLANE_STRIDE_2_A			0x70288
> >>>   #define _PLANE_STRIDE_3_A			0x70388
> >>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> >>> index 976b891..a27ee8c 100644
> >>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> >>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> >>> @@ -162,6 +162,30 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> >>>   			(1 << drm_plane_index(plane));
> >>>   	}
> >>>
> >>> +	if (state->fb && intel_rotation_90_or_270(state->rotation)) {
> >>> +		if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
> >>> +			state->fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
> >>> +			DRM_DEBUG_KMS("Y/Yf tiling required for 90/270!\n");
> >>> +			return -EINVAL;
> >>> +		}
> >>> +
> >>> +		/*
> >>> +		 * 90/270 is not allowed with RGB64 16:16:16:16,
> >>> +		 * RGB 16-bit 5:6:5, and Indexed 8-bit.
> >>> +		 * TBD: Add RGB64 case once its added in supported format list.
> >>> +		 */
> >>> +		switch (state->fb->pixel_format) {
> >>> +		case DRM_FORMAT_C8:
> >>> +		case DRM_FORMAT_RGB565:
> >>> +			DRM_DEBUG_KMS("Unsupported pixel format %s for 90/270!\n",
> >>> +					drm_get_format_name(state->fb->pixel_format));
> >>> +			return -EINVAL;
> >>> +
> >>> +		default:
> >>> +			break;
> >>> +		}
> >>> +	}
> >>> +
> >>>   	return intel_plane->check_plane(plane, intel_state);
> >>>   }
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>> index f0bbc22..4de544c 100644
> >>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>> @@ -2311,13 +2311,6 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb,
> >>>   	info->pitch = fb->pitches[0];
> >>>   	info->fb_modifier = fb->modifier[0];
> >>>
> >>> -	if (!(info->fb_modifier == I915_FORMAT_MOD_Y_TILED ||
> >>> -	      info->fb_modifier == I915_FORMAT_MOD_Yf_TILED)) {
> >>> -		DRM_DEBUG_KMS(
> >>> -			      "Y or Yf tiling is needed for 90/270 rotation!\n");
> >>> -		return -EINVAL;
> >>> -	}
> >>> -
> >>>   	return 0;
> >>>   }
> >>>
> >>> @@ -2919,8 +2912,12 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
> >>>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >>>   	struct drm_i915_gem_object *obj;
> >>>   	int pipe = intel_crtc->pipe;
> >>> -	u32 plane_ctl, stride_div;
> >>> +	u32 plane_ctl, stride_div, stride;
> >>> +	u32 tile_height, plane_offset, plane_size;
> >>> +	unsigned int rotation;
> >>> +	int x_offset, y_offset;
> >>>   	unsigned long surf_addr;
> >>> +	struct drm_plane *plane;
> >>>
> >>>   	if (!intel_crtc->primary_enabled) {
> >>>   		I915_WRITE(PLANE_CTL(pipe, 0), 0);
> >>> @@ -2981,21 +2978,51 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
> >>>   	}
> >>>
> >>>   	plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
> >>> -	if (crtc->primary->state->rotation == BIT(DRM_ROTATE_180))
> >>> +
> >>> +	plane = crtc->primary;
> >>> +	rotation = plane->state->rotation;
> >>> +	switch (rotation) {
> >>> +	case BIT(DRM_ROTATE_90):
> >>> +		plane_ctl |= PLANE_CTL_ROTATE_90;
> >>> +		break;
> >>> +
> >>> +	case BIT(DRM_ROTATE_180):
> >>>   		plane_ctl |= PLANE_CTL_ROTATE_180;
> >>> +		break;
> >>> +
> >>> +	case BIT(DRM_ROTATE_270):
> >>> +		plane_ctl |= PLANE_CTL_ROTATE_270;
> >>> +		break;
> >>> +	}
> >>>
> >>>   	obj = intel_fb_obj(fb);
> >>>   	stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
> >>>   					       fb->pixel_format);
> >>> -	surf_addr = intel_plane_obj_offset(to_intel_plane(crtc->primary), obj);
> >>> +	surf_addr = intel_plane_obj_offset(to_intel_plane(plane), obj);
> >>> +
> >>> +	if (intel_rotation_90_or_270(rotation)) {
> >>> +		/* stride = Surface height in tiles */
> >>> +		tile_height = intel_tile_height(dev, fb->bits_per_pixel,
> >>> +						fb->modifier[0]);
> >>> +		stride = DIV_ROUND_UP(fb->height, tile_height);
> >>> +		x_offset = stride * tile_height - y - (plane->state->src_h >> 16);
> >>> +		y_offset = x;
> >>> +		plane_size = ((plane->state->src_w >> 16) - 1) << 16 |
> >>> +					((plane->state->src_h >> 16) - 1);
> >>> +	} else {
> >>> +		stride = fb->pitches[0] / stride_div;
> >>> +		x_offset = x;
> >>> +		y_offset = y;
> >>> +		plane_size = ((plane->state->src_h >> 16) - 1) << 16 |
> >>> +			((plane->state->src_w >> 16) - 1);
> >>> +	}
> >>> +	plane_offset = y_offset << 16 | x_offset;
> >>>
> >>>   	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
> >>>   	I915_WRITE(PLANE_POS(pipe, 0), 0);
> >>> -	I915_WRITE(PLANE_OFFSET(pipe, 0), (y << 16) | x);
> >>> -	I915_WRITE(PLANE_SIZE(pipe, 0),
> >>> -		   (intel_crtc->config->pipe_src_h - 1) << 16 |
> >>> -		   (intel_crtc->config->pipe_src_w - 1));
> >>> -	I915_WRITE(PLANE_STRIDE(pipe, 0), fb->pitches[0] / stride_div);
> >>> +	I915_WRITE(PLANE_OFFSET(pipe, 0), plane_offset);
> >>> +	I915_WRITE(PLANE_SIZE(pipe, 0), plane_size);
> >>> +	I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
> >>>   	I915_WRITE(PLANE_SURF(pipe, 0), surf_addr);
> >>>
> >>>   	POSTING_READ(PLANE_SURF(pipe, 0));
> >>> @@ -12406,23 +12433,32 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> >>>   				 intel_primary_formats, num_formats,
> >>>   				 DRM_PLANE_TYPE_PRIMARY);
> >>>
> >>> -	if (INTEL_INFO(dev)->gen >= 4) {
> >>> -		if (!dev->mode_config.rotation_property)
> >>> -			dev->mode_config.rotation_property =
> >>> -				drm_mode_create_rotation_property(dev,
> >>> -							BIT(DRM_ROTATE_0) |
> >>> -							BIT(DRM_ROTATE_180));
> >>> -		if (dev->mode_config.rotation_property)
> >>> -			drm_object_attach_property(&primary->base.base,
> >>> -				dev->mode_config.rotation_property,
> >>> -				state->base.rotation);
> >>> -	}
> >>> +	if (INTEL_INFO(dev)->gen >= 4)
> >>> +		intel_create_rotation_property(dev, primary);
> >>>
> >>>   	drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs);
> >>>
> >>>   	return &primary->base;
> >>>   }
> >>>
> >>> +void intel_create_rotation_property(struct drm_device *dev, struct intel_plane *plane)
> >>> +{
> >>> +	if (!dev->mode_config.rotation_property) {
> >>> +		unsigned long flags = BIT(DRM_ROTATE_0) |
> >>> +			BIT(DRM_ROTATE_180);
> >>> +
> >>> +		if (INTEL_INFO(dev)->gen >= 9)
> >>> +			flags |= BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270);
> >>> +
> >>> +		dev->mode_config.rotation_property =
> >>> +			drm_mode_create_rotation_property(dev, flags);
> >>> +	}
> >>> +	if (dev->mode_config.rotation_property)
> >>> +		drm_object_attach_property(&plane->base.base,
> >>> +				dev->mode_config.rotation_property,
> >>> +				plane->base.state->rotation);
> >>> +}
> >>> +
> >>>   static int
> >>>   intel_check_cursor_plane(struct drm_plane *plane,
> >>>   			 struct intel_plane_state *state)
> >>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >>> index 811a1db..d32025a 100644
> >>> --- a/drivers/gpu/drm/i915/intel_drv.h
> >>> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >>> @@ -995,6 +995,12 @@ intel_rotation_90_or_270(unsigned int rotation)
> >>>   	return rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270));
> >>>   }
> >>>
> >>> +unsigned int
> >>> +intel_tile_height(struct drm_device *dev, uint32_t bits_per_pixel,
> >>> +		  uint64_t fb_modifier);
> >>> +void intel_create_rotation_property(struct drm_device *dev,
> >>> +					struct intel_plane *plane);
> >>> +
> >>>   bool intel_wm_need_update(struct drm_plane *plane,
> >>>   			  struct drm_plane_state *state);
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> >>> index f41e872..83adc9b 100644
> >>> --- a/drivers/gpu/drm/i915/intel_sprite.c
> >>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> >>> @@ -190,10 +190,13 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
> >>>   	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> >>>   	const int pipe = intel_plane->pipe;
> >>>   	const int plane = intel_plane->plane + 1;
> >>> -	u32 plane_ctl, stride_div;
> >>> +	u32 plane_ctl, stride_div, stride;
> >>>   	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> >>>   	const struct drm_intel_sprite_colorkey *key = &intel_plane->ckey;
> >>>   	unsigned long surf_addr;
> >>> +	u32 tile_height, plane_offset, plane_size;
> >>> +	unsigned int rotation;
> >>> +	int x_offset, y_offset;
> >>>
> >>>   	plane_ctl = PLANE_CTL_ENABLE |
> >>>   		PLANE_CTL_PIPE_CSC_ENABLE;
> >>> @@ -254,8 +257,20 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
> >>>   		MISSING_CASE(fb->modifier[0]);
> >>>   	}
> >>>
> >>> -	if (drm_plane->state->rotation == BIT(DRM_ROTATE_180))
> >>> +	rotation = drm_plane->state->rotation;
> >>> +	switch (rotation) {
> >>> +	case BIT(DRM_ROTATE_90):
> >>> +		plane_ctl |= PLANE_CTL_ROTATE_90;
> >>> +		break;
> >>> +
> >>> +	case BIT(DRM_ROTATE_180):
> >>>   		plane_ctl |= PLANE_CTL_ROTATE_180;
> >>> +		break;
> >>> +
> >>> +	case BIT(DRM_ROTATE_270):
> >>> +		plane_ctl |= PLANE_CTL_ROTATE_270;
> >>> +		break;
> >>> +	}
> >>>
> >>>   	intel_update_sprite_watermarks(drm_plane, crtc, src_w, src_h,
> >>>   				       pixel_size, true,
> >>> @@ -283,10 +298,26 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
> >>>
> >>>   	surf_addr = intel_plane_obj_offset(intel_plane, obj);
> >>>
> >>> -	I915_WRITE(PLANE_OFFSET(pipe, plane), (y << 16) | x);
> >>> -	I915_WRITE(PLANE_STRIDE(pipe, plane), fb->pitches[0] / stride_div);
> >>> +	if (intel_rotation_90_or_270(rotation)) {
> >>> +		/* stride: Surface height in tiles */
> >>> +		tile_height = intel_tile_height(dev, fb->bits_per_pixel,
> >>> +							fb->modifier[0]);
> >>> +		stride = DIV_ROUND_UP(fb->height, tile_height);
> >>> +		plane_size = (src_w << 16) | src_h;
> >>> +		x_offset = stride * tile_height - y - (src_h + 1);
> >>> +		y_offset = x;
> >>> +	} else {
> >>> +		stride = fb->pitches[0] / stride_div;
> >>> +		plane_size = (src_h << 16) | src_w;
> >>> +		x_offset = x;
> >>> +		y_offset = y;
> >>> +	}
> >>> +	plane_offset = y_offset << 16 | x_offset;
> >>> +
> >>> +	I915_WRITE(PLANE_OFFSET(pipe, plane), plane_offset);
> >>> +	I915_WRITE(PLANE_STRIDE(pipe, plane), stride);
> >>>   	I915_WRITE(PLANE_POS(pipe, plane), (crtc_y << 16) | crtc_x);
> >>> -	I915_WRITE(PLANE_SIZE(pipe, plane), (crtc_h << 16) | crtc_w);
> >>> +	I915_WRITE(PLANE_SIZE(pipe, plane), plane_size);
> >>>   	I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl);
> >>>   	I915_WRITE(PLANE_SURF(pipe, plane), surf_addr);
> >>>   	POSTING_READ(PLANE_SURF(pipe, plane));
> >>> @@ -1310,16 +1341,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> >>>   		goto out;
> >>>   	}
> >>>
> >>> -	if (!dev->mode_config.rotation_property)
> >>> -		dev->mode_config.rotation_property =
> >>> -			drm_mode_create_rotation_property(dev,
> >>> -							  BIT(DRM_ROTATE_0) |
> >>> -							  BIT(DRM_ROTATE_180));
> >>> -
> >>> -	if (dev->mode_config.rotation_property)
> >>> -		drm_object_attach_property(&intel_plane->base.base,
> >>> -					   dev->mode_config.rotation_property,
> >>> -					   state->base.rotation);
> >>> +	intel_create_rotation_property(dev, intel_plane);
> >>>
> >>>   	drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
> >>>
> >>> --
> >>> 1.7.10.4
> >>>
> >>> _______________________________________________
> >>> 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
> >

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

Re: [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation

[Jindal, Sonika]

On 4/13/2015 3:40 PM, Ville Syrjälä wrote:
> On Mon, Apr 13, 2015 at 09:36:02AM +0530, Jindal, Sonika wrote:
>>
>>
>> On 4/10/2015 8:14 PM, Ville Syrjälä wrote:
>>> On Fri, Apr 10, 2015 at 04:17:17PM +0200, Daniel Vetter wrote:
>>>> On Fri, Apr 10, 2015 at 02:37:29PM +0530, Sonika Jindal wrote:
>>>>> v2: Moving creation of property in a function, checking for 90/270
>>>>> rotation simultaneously (Chris)
>>>>> Letting primary plane to be positioned
>>>>> v3: Adding if/else for 90/270 and rest params programming, adding check for
>>>>> pixel_format, some cleanup (review comments)
>>>>> v4: Adding right pixel_formats, using src_* params instead of crtc_* for offset
>>>>> and size programming (Ville)
>>>>> v5: Rebased on -nightly and Tvrtko's series for gtt remapping.
>>>>> v6: Rebased on -nightly (Tvrtko's series merged)
>>>>> v7: Moving pixel_format check to intel_atomic_plane_check (Matt)
>>>>>
>>>>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
>>>>> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
>>>>
>>>> Patches are missing the Testcase: tag, please add that. Also, are all the
>>>> igt committed or not yet? Pulled these two in meanwhile.
>>>
>>> Things are going somewhat broken because you didn't apply my plane
>>> state stuff. Hmm. Actually it sort of looks that it might work by luck
>>> as long as the primary plane doesn't get clipped since this is bashing
>>> the user state directly into the hardware registers and not the derived
>>> state (ie. clipped coordinates).
>>>
>> I was hoping your changes get merged, but not sure why they are held up.
>>
>>> Also I see my review comment about the 90 vs. 270 rotation mixup was
>>> ignored at least.
>>>
>> I never really got the to understand the need of reversing 90 and 270 :)
>> The last discussion was not concluded, AFAIR.
>> Things look correct to me and work fine with the testcase also.
>> Is there something completely different which I am unable to get?
>
> BSpec gives me the impression the hw rotation is cw, whereas the drm one
> is ccw.
>
Yes, HW rotation is cw as per bspec. In drm, where do we consider it as 
anti-cw? I assume it is cw because all the macros work as expected, ie cw.

>>
>> -Sonika
>>
>>>> -Daniel
>>>>
>>>>> ---
>>>>>    drivers/gpu/drm/i915/i915_reg.h           |    2 +
>>>>>    drivers/gpu/drm/i915/intel_atomic_plane.c |   24 ++++++++
>>>>>    drivers/gpu/drm/i915/intel_display.c      |   88 ++++++++++++++++++++---------
>>>>>    drivers/gpu/drm/i915/intel_drv.h          |    6 ++
>>>>>    drivers/gpu/drm/i915/intel_sprite.c       |   52 ++++++++++++-----
>>>>>    5 files changed, 131 insertions(+), 41 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>>>> index b522eb6..564bbd5 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>>>> @@ -4854,7 +4854,9 @@ enum skl_disp_power_wells {
>>>>>    #define   PLANE_CTL_ALPHA_HW_PREMULTIPLY	(  3 << 4)
>>>>>    #define   PLANE_CTL_ROTATE_MASK			0x3
>>>>>    #define   PLANE_CTL_ROTATE_0			0x0
>>>>> +#define   PLANE_CTL_ROTATE_90			0x1
>>>>>    #define   PLANE_CTL_ROTATE_180			0x2
>>>>> +#define   PLANE_CTL_ROTATE_270			0x3
>>>>>    #define _PLANE_STRIDE_1_A			0x70188
>>>>>    #define _PLANE_STRIDE_2_A			0x70288
>>>>>    #define _PLANE_STRIDE_3_A			0x70388
>>>>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
>>>>> index 976b891..a27ee8c 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
>>>>> @@ -162,6 +162,30 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>>>>>    			(1 << drm_plane_index(plane));
>>>>>    	}
>>>>>
>>>>> +	if (state->fb && intel_rotation_90_or_270(state->rotation)) {
>>>>> +		if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
>>>>> +			state->fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
>>>>> +			DRM_DEBUG_KMS("Y/Yf tiling required for 90/270!\n");
>>>>> +			return -EINVAL;
>>>>> +		}
>>>>> +
>>>>> +		/*
>>>>> +		 * 90/270 is not allowed with RGB64 16:16:16:16,
>>>>> +		 * RGB 16-bit 5:6:5, and Indexed 8-bit.
>>>>> +		 * TBD: Add RGB64 case once its added in supported format list.
>>>>> +		 */
>>>>> +		switch (state->fb->pixel_format) {
>>>>> +		case DRM_FORMAT_C8:
>>>>> +		case DRM_FORMAT_RGB565:
>>>>> +			DRM_DEBUG_KMS("Unsupported pixel format %s for 90/270!\n",
>>>>> +					drm_get_format_name(state->fb->pixel_format));
>>>>> +			return -EINVAL;
>>>>> +
>>>>> +		default:
>>>>> +			break;
>>>>> +		}
>>>>> +	}
>>>>> +
>>>>>    	return intel_plane->check_plane(plane, intel_state);
>>>>>    }
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>>> index f0bbc22..4de544c 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>>> @@ -2311,13 +2311,6 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb,
>>>>>    	info->pitch = fb->pitches[0];
>>>>>    	info->fb_modifier = fb->modifier[0];
>>>>>
>>>>> -	if (!(info->fb_modifier == I915_FORMAT_MOD_Y_TILED ||
>>>>> -	      info->fb_modifier == I915_FORMAT_MOD_Yf_TILED)) {
>>>>> -		DRM_DEBUG_KMS(
>>>>> -			      "Y or Yf tiling is needed for 90/270 rotation!\n");
>>>>> -		return -EINVAL;
>>>>> -	}
>>>>> -
>>>>>    	return 0;
>>>>>    }
>>>>>
>>>>> @@ -2919,8 +2912,12 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>>>>>    	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>>>>    	struct drm_i915_gem_object *obj;
>>>>>    	int pipe = intel_crtc->pipe;
>>>>> -	u32 plane_ctl, stride_div;
>>>>> +	u32 plane_ctl, stride_div, stride;
>>>>> +	u32 tile_height, plane_offset, plane_size;
>>>>> +	unsigned int rotation;
>>>>> +	int x_offset, y_offset;
>>>>>    	unsigned long surf_addr;
>>>>> +	struct drm_plane *plane;
>>>>>
>>>>>    	if (!intel_crtc->primary_enabled) {
>>>>>    		I915_WRITE(PLANE_CTL(pipe, 0), 0);
>>>>> @@ -2981,21 +2978,51 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>>>>>    	}
>>>>>
>>>>>    	plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
>>>>> -	if (crtc->primary->state->rotation == BIT(DRM_ROTATE_180))
>>>>> +
>>>>> +	plane = crtc->primary;
>>>>> +	rotation = plane->state->rotation;
>>>>> +	switch (rotation) {
>>>>> +	case BIT(DRM_ROTATE_90):
>>>>> +		plane_ctl |= PLANE_CTL_ROTATE_90;
>>>>> +		break;
>>>>> +
>>>>> +	case BIT(DRM_ROTATE_180):
>>>>>    		plane_ctl |= PLANE_CTL_ROTATE_180;
>>>>> +		break;
>>>>> +
>>>>> +	case BIT(DRM_ROTATE_270):
>>>>> +		plane_ctl |= PLANE_CTL_ROTATE_270;
>>>>> +		break;
>>>>> +	}
>>>>>
>>>>>    	obj = intel_fb_obj(fb);
>>>>>    	stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
>>>>>    					       fb->pixel_format);
>>>>> -	surf_addr = intel_plane_obj_offset(to_intel_plane(crtc->primary), obj);
>>>>> +	surf_addr = intel_plane_obj_offset(to_intel_plane(plane), obj);
>>>>> +
>>>>> +	if (intel_rotation_90_or_270(rotation)) {
>>>>> +		/* stride = Surface height in tiles */
>>>>> +		tile_height = intel_tile_height(dev, fb->bits_per_pixel,
>>>>> +						fb->modifier[0]);
>>>>> +		stride = DIV_ROUND_UP(fb->height, tile_height);
>>>>> +		x_offset = stride * tile_height - y - (plane->state->src_h >> 16);
>>>>> +		y_offset = x;
>>>>> +		plane_size = ((plane->state->src_w >> 16) - 1) << 16 |
>>>>> +					((plane->state->src_h >> 16) - 1);
>>>>> +	} else {
>>>>> +		stride = fb->pitches[0] / stride_div;
>>>>> +		x_offset = x;
>>>>> +		y_offset = y;
>>>>> +		plane_size = ((plane->state->src_h >> 16) - 1) << 16 |
>>>>> +			((plane->state->src_w >> 16) - 1);
>>>>> +	}
>>>>> +	plane_offset = y_offset << 16 | x_offset;
>>>>>
>>>>>    	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
>>>>>    	I915_WRITE(PLANE_POS(pipe, 0), 0);
>>>>> -	I915_WRITE(PLANE_OFFSET(pipe, 0), (y << 16) | x);
>>>>> -	I915_WRITE(PLANE_SIZE(pipe, 0),
>>>>> -		   (intel_crtc->config->pipe_src_h - 1) << 16 |
>>>>> -		   (intel_crtc->config->pipe_src_w - 1));
>>>>> -	I915_WRITE(PLANE_STRIDE(pipe, 0), fb->pitches[0] / stride_div);
>>>>> +	I915_WRITE(PLANE_OFFSET(pipe, 0), plane_offset);
>>>>> +	I915_WRITE(PLANE_SIZE(pipe, 0), plane_size);
>>>>> +	I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
>>>>>    	I915_WRITE(PLANE_SURF(pipe, 0), surf_addr);
>>>>>
>>>>>    	POSTING_READ(PLANE_SURF(pipe, 0));
>>>>> @@ -12406,23 +12433,32 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>>>>>    				 intel_primary_formats, num_formats,
>>>>>    				 DRM_PLANE_TYPE_PRIMARY);
>>>>>
>>>>> -	if (INTEL_INFO(dev)->gen >= 4) {
>>>>> -		if (!dev->mode_config.rotation_property)
>>>>> -			dev->mode_config.rotation_property =
>>>>> -				drm_mode_create_rotation_property(dev,
>>>>> -							BIT(DRM_ROTATE_0) |
>>>>> -							BIT(DRM_ROTATE_180));
>>>>> -		if (dev->mode_config.rotation_property)
>>>>> -			drm_object_attach_property(&primary->base.base,
>>>>> -				dev->mode_config.rotation_property,
>>>>> -				state->base.rotation);
>>>>> -	}
>>>>> +	if (INTEL_INFO(dev)->gen >= 4)
>>>>> +		intel_create_rotation_property(dev, primary);
>>>>>
>>>>>    	drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs);
>>>>>
>>>>>    	return &primary->base;
>>>>>    }
>>>>>
>>>>> +void intel_create_rotation_property(struct drm_device *dev, struct intel_plane *plane)
>>>>> +{
>>>>> +	if (!dev->mode_config.rotation_property) {
>>>>> +		unsigned long flags = BIT(DRM_ROTATE_0) |
>>>>> +			BIT(DRM_ROTATE_180);
>>>>> +
>>>>> +		if (INTEL_INFO(dev)->gen >= 9)
>>>>> +			flags |= BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270);
>>>>> +
>>>>> +		dev->mode_config.rotation_property =
>>>>> +			drm_mode_create_rotation_property(dev, flags);
>>>>> +	}
>>>>> +	if (dev->mode_config.rotation_property)
>>>>> +		drm_object_attach_property(&plane->base.base,
>>>>> +				dev->mode_config.rotation_property,
>>>>> +				plane->base.state->rotation);
>>>>> +}
>>>>> +
>>>>>    static int
>>>>>    intel_check_cursor_plane(struct drm_plane *plane,
>>>>>    			 struct intel_plane_state *state)
>>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>>>> index 811a1db..d32025a 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>>> @@ -995,6 +995,12 @@ intel_rotation_90_or_270(unsigned int rotation)
>>>>>    	return rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270));
>>>>>    }
>>>>>
>>>>> +unsigned int
>>>>> +intel_tile_height(struct drm_device *dev, uint32_t bits_per_pixel,
>>>>> +		  uint64_t fb_modifier);
>>>>> +void intel_create_rotation_property(struct drm_device *dev,
>>>>> +					struct intel_plane *plane);
>>>>> +
>>>>>    bool intel_wm_need_update(struct drm_plane *plane,
>>>>>    			  struct drm_plane_state *state);
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>>>>> index f41e872..83adc9b 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>>>>> @@ -190,10 +190,13 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>>>>>    	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>>>>>    	const int pipe = intel_plane->pipe;
>>>>>    	const int plane = intel_plane->plane + 1;
>>>>> -	u32 plane_ctl, stride_div;
>>>>> +	u32 plane_ctl, stride_div, stride;
>>>>>    	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
>>>>>    	const struct drm_intel_sprite_colorkey *key = &intel_plane->ckey;
>>>>>    	unsigned long surf_addr;
>>>>> +	u32 tile_height, plane_offset, plane_size;
>>>>> +	unsigned int rotation;
>>>>> +	int x_offset, y_offset;
>>>>>
>>>>>    	plane_ctl = PLANE_CTL_ENABLE |
>>>>>    		PLANE_CTL_PIPE_CSC_ENABLE;
>>>>> @@ -254,8 +257,20 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>>>>>    		MISSING_CASE(fb->modifier[0]);
>>>>>    	}
>>>>>
>>>>> -	if (drm_plane->state->rotation == BIT(DRM_ROTATE_180))
>>>>> +	rotation = drm_plane->state->rotation;
>>>>> +	switch (rotation) {
>>>>> +	case BIT(DRM_ROTATE_90):
>>>>> +		plane_ctl |= PLANE_CTL_ROTATE_90;
>>>>> +		break;
>>>>> +
>>>>> +	case BIT(DRM_ROTATE_180):
>>>>>    		plane_ctl |= PLANE_CTL_ROTATE_180;
>>>>> +		break;
>>>>> +
>>>>> +	case BIT(DRM_ROTATE_270):
>>>>> +		plane_ctl |= PLANE_CTL_ROTATE_270;
>>>>> +		break;
>>>>> +	}
>>>>>
>>>>>    	intel_update_sprite_watermarks(drm_plane, crtc, src_w, src_h,
>>>>>    				       pixel_size, true,
>>>>> @@ -283,10 +298,26 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>>>>>
>>>>>    	surf_addr = intel_plane_obj_offset(intel_plane, obj);
>>>>>
>>>>> -	I915_WRITE(PLANE_OFFSET(pipe, plane), (y << 16) | x);
>>>>> -	I915_WRITE(PLANE_STRIDE(pipe, plane), fb->pitches[0] / stride_div);
>>>>> +	if (intel_rotation_90_or_270(rotation)) {
>>>>> +		/* stride: Surface height in tiles */
>>>>> +		tile_height = intel_tile_height(dev, fb->bits_per_pixel,
>>>>> +							fb->modifier[0]);
>>>>> +		stride = DIV_ROUND_UP(fb->height, tile_height);
>>>>> +		plane_size = (src_w << 16) | src_h;
>>>>> +		x_offset = stride * tile_height - y - (src_h + 1);
>>>>> +		y_offset = x;
>>>>> +	} else {
>>>>> +		stride = fb->pitches[0] / stride_div;
>>>>> +		plane_size = (src_h << 16) | src_w;
>>>>> +		x_offset = x;
>>>>> +		y_offset = y;
>>>>> +	}
>>>>> +	plane_offset = y_offset << 16 | x_offset;
>>>>> +
>>>>> +	I915_WRITE(PLANE_OFFSET(pipe, plane), plane_offset);
>>>>> +	I915_WRITE(PLANE_STRIDE(pipe, plane), stride);
>>>>>    	I915_WRITE(PLANE_POS(pipe, plane), (crtc_y << 16) | crtc_x);
>>>>> -	I915_WRITE(PLANE_SIZE(pipe, plane), (crtc_h << 16) | crtc_w);
>>>>> +	I915_WRITE(PLANE_SIZE(pipe, plane), plane_size);
>>>>>    	I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl);
>>>>>    	I915_WRITE(PLANE_SURF(pipe, plane), surf_addr);
>>>>>    	POSTING_READ(PLANE_SURF(pipe, plane));
>>>>> @@ -1310,16 +1341,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>>>>>    		goto out;
>>>>>    	}
>>>>>
>>>>> -	if (!dev->mode_config.rotation_property)
>>>>> -		dev->mode_config.rotation_property =
>>>>> -			drm_mode_create_rotation_property(dev,
>>>>> -							  BIT(DRM_ROTATE_0) |
>>>>> -							  BIT(DRM_ROTATE_180));
>>>>> -
>>>>> -	if (dev->mode_config.rotation_property)
>>>>> -		drm_object_attach_property(&intel_plane->base.base,
>>>>> -					   dev->mode_config.rotation_property,
>>>>> -					   state->base.rotation);
>>>>> +	intel_create_rotation_property(dev, intel_plane);
>>>>>
>>>>>    	drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
>>>>>
>>>>> --
>>>>> 1.7.10.4
>>>>>
>>>>> _______________________________________________
>>>>> 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
>>>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation

[Ville Syrjälä]

On Mon, Apr 13, 2015 at 03:53:22PM +0530, Jindal, Sonika wrote:
> 
> 
> On 4/13/2015 3:40 PM, Ville Syrjälä wrote:
> > On Mon, Apr 13, 2015 at 09:36:02AM +0530, Jindal, Sonika wrote:
> >>
> >>
> >> On 4/10/2015 8:14 PM, Ville Syrjälä wrote:
> >>> On Fri, Apr 10, 2015 at 04:17:17PM +0200, Daniel Vetter wrote:
> >>>> On Fri, Apr 10, 2015 at 02:37:29PM +0530, Sonika Jindal wrote:
> >>>>> v2: Moving creation of property in a function, checking for 90/270
> >>>>> rotation simultaneously (Chris)
> >>>>> Letting primary plane to be positioned
> >>>>> v3: Adding if/else for 90/270 and rest params programming, adding check for
> >>>>> pixel_format, some cleanup (review comments)
> >>>>> v4: Adding right pixel_formats, using src_* params instead of crtc_* for offset
> >>>>> and size programming (Ville)
> >>>>> v5: Rebased on -nightly and Tvrtko's series for gtt remapping.
> >>>>> v6: Rebased on -nightly (Tvrtko's series merged)
> >>>>> v7: Moving pixel_format check to intel_atomic_plane_check (Matt)
> >>>>>
> >>>>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> >>>>> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> >>>>
> >>>> Patches are missing the Testcase: tag, please add that. Also, are all the
> >>>> igt committed or not yet? Pulled these two in meanwhile.
> >>>
> >>> Things are going somewhat broken because you didn't apply my plane
> >>> state stuff. Hmm. Actually it sort of looks that it might work by luck
> >>> as long as the primary plane doesn't get clipped since this is bashing
> >>> the user state directly into the hardware registers and not the derived
> >>> state (ie. clipped coordinates).
> >>>
> >> I was hoping your changes get merged, but not sure why they are held up.
> >>
> >>> Also I see my review comment about the 90 vs. 270 rotation mixup was
> >>> ignored at least.
> >>>
> >> I never really got the to understand the need of reversing 90 and 270 :)
> >> The last discussion was not concluded, AFAIR.
> >> Things look correct to me and work fine with the testcase also.
> >> Is there something completely different which I am unable to get?
> >
> > BSpec gives me the impression the hw rotation is cw, whereas the drm one
> > is ccw.
> >
> Yes, HW rotation is cw as per bspec. In drm, where do we consider it as 
> anti-cw? I assume it is cw because all the macros work as expected, ie cw.

The ccw rule was inherited from XRandR. I'm not sure what macros you
mean, but drm_rect_rotate() will certainly give you wrong results if
you assume cw.

> 
> >>
> >> -Sonika
> >>
> >>>> -Daniel
> >>>>
> >>>>> ---
> >>>>>    drivers/gpu/drm/i915/i915_reg.h           |    2 +
> >>>>>    drivers/gpu/drm/i915/intel_atomic_plane.c |   24 ++++++++
> >>>>>    drivers/gpu/drm/i915/intel_display.c      |   88 ++++++++++++++++++++---------
> >>>>>    drivers/gpu/drm/i915/intel_drv.h          |    6 ++
> >>>>>    drivers/gpu/drm/i915/intel_sprite.c       |   52 ++++++++++++-----
> >>>>>    5 files changed, 131 insertions(+), 41 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >>>>> index b522eb6..564bbd5 100644
> >>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
> >>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >>>>> @@ -4854,7 +4854,9 @@ enum skl_disp_power_wells {
> >>>>>    #define   PLANE_CTL_ALPHA_HW_PREMULTIPLY	(  3 << 4)
> >>>>>    #define   PLANE_CTL_ROTATE_MASK			0x3
> >>>>>    #define   PLANE_CTL_ROTATE_0			0x0
> >>>>> +#define   PLANE_CTL_ROTATE_90			0x1
> >>>>>    #define   PLANE_CTL_ROTATE_180			0x2
> >>>>> +#define   PLANE_CTL_ROTATE_270			0x3
> >>>>>    #define _PLANE_STRIDE_1_A			0x70188
> >>>>>    #define _PLANE_STRIDE_2_A			0x70288
> >>>>>    #define _PLANE_STRIDE_3_A			0x70388
> >>>>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> >>>>> index 976b891..a27ee8c 100644
> >>>>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> >>>>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> >>>>> @@ -162,6 +162,30 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> >>>>>    			(1 << drm_plane_index(plane));
> >>>>>    	}
> >>>>>
> >>>>> +	if (state->fb && intel_rotation_90_or_270(state->rotation)) {
> >>>>> +		if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
> >>>>> +			state->fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
> >>>>> +			DRM_DEBUG_KMS("Y/Yf tiling required for 90/270!\n");
> >>>>> +			return -EINVAL;
> >>>>> +		}
> >>>>> +
> >>>>> +		/*
> >>>>> +		 * 90/270 is not allowed with RGB64 16:16:16:16,
> >>>>> +		 * RGB 16-bit 5:6:5, and Indexed 8-bit.
> >>>>> +		 * TBD: Add RGB64 case once its added in supported format list.
> >>>>> +		 */
> >>>>> +		switch (state->fb->pixel_format) {
> >>>>> +		case DRM_FORMAT_C8:
> >>>>> +		case DRM_FORMAT_RGB565:
> >>>>> +			DRM_DEBUG_KMS("Unsupported pixel format %s for 90/270!\n",
> >>>>> +					drm_get_format_name(state->fb->pixel_format));
> >>>>> +			return -EINVAL;
> >>>>> +
> >>>>> +		default:
> >>>>> +			break;
> >>>>> +		}
> >>>>> +	}
> >>>>> +
> >>>>>    	return intel_plane->check_plane(plane, intel_state);
> >>>>>    }
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>>>> index f0bbc22..4de544c 100644
> >>>>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>>>> @@ -2311,13 +2311,6 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb,
> >>>>>    	info->pitch = fb->pitches[0];
> >>>>>    	info->fb_modifier = fb->modifier[0];
> >>>>>
> >>>>> -	if (!(info->fb_modifier == I915_FORMAT_MOD_Y_TILED ||
> >>>>> -	      info->fb_modifier == I915_FORMAT_MOD_Yf_TILED)) {
> >>>>> -		DRM_DEBUG_KMS(
> >>>>> -			      "Y or Yf tiling is needed for 90/270 rotation!\n");
> >>>>> -		return -EINVAL;
> >>>>> -	}
> >>>>> -
> >>>>>    	return 0;
> >>>>>    }
> >>>>>
> >>>>> @@ -2919,8 +2912,12 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
> >>>>>    	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >>>>>    	struct drm_i915_gem_object *obj;
> >>>>>    	int pipe = intel_crtc->pipe;
> >>>>> -	u32 plane_ctl, stride_div;
> >>>>> +	u32 plane_ctl, stride_div, stride;
> >>>>> +	u32 tile_height, plane_offset, plane_size;
> >>>>> +	unsigned int rotation;
> >>>>> +	int x_offset, y_offset;
> >>>>>    	unsigned long surf_addr;
> >>>>> +	struct drm_plane *plane;
> >>>>>
> >>>>>    	if (!intel_crtc->primary_enabled) {
> >>>>>    		I915_WRITE(PLANE_CTL(pipe, 0), 0);
> >>>>> @@ -2981,21 +2978,51 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
> >>>>>    	}
> >>>>>
> >>>>>    	plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
> >>>>> -	if (crtc->primary->state->rotation == BIT(DRM_ROTATE_180))
> >>>>> +
> >>>>> +	plane = crtc->primary;
> >>>>> +	rotation = plane->state->rotation;
> >>>>> +	switch (rotation) {
> >>>>> +	case BIT(DRM_ROTATE_90):
> >>>>> +		plane_ctl |= PLANE_CTL_ROTATE_90;
> >>>>> +		break;
> >>>>> +
> >>>>> +	case BIT(DRM_ROTATE_180):
> >>>>>    		plane_ctl |= PLANE_CTL_ROTATE_180;
> >>>>> +		break;
> >>>>> +
> >>>>> +	case BIT(DRM_ROTATE_270):
> >>>>> +		plane_ctl |= PLANE_CTL_ROTATE_270;
> >>>>> +		break;
> >>>>> +	}
> >>>>>
> >>>>>    	obj = intel_fb_obj(fb);
> >>>>>    	stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
> >>>>>    					       fb->pixel_format);
> >>>>> -	surf_addr = intel_plane_obj_offset(to_intel_plane(crtc->primary), obj);
> >>>>> +	surf_addr = intel_plane_obj_offset(to_intel_plane(plane), obj);
> >>>>> +
> >>>>> +	if (intel_rotation_90_or_270(rotation)) {
> >>>>> +		/* stride = Surface height in tiles */
> >>>>> +		tile_height = intel_tile_height(dev, fb->bits_per_pixel,
> >>>>> +						fb->modifier[0]);
> >>>>> +		stride = DIV_ROUND_UP(fb->height, tile_height);
> >>>>> +		x_offset = stride * tile_height - y - (plane->state->src_h >> 16);
> >>>>> +		y_offset = x;
> >>>>> +		plane_size = ((plane->state->src_w >> 16) - 1) << 16 |
> >>>>> +					((plane->state->src_h >> 16) - 1);
> >>>>> +	} else {
> >>>>> +		stride = fb->pitches[0] / stride_div;
> >>>>> +		x_offset = x;
> >>>>> +		y_offset = y;
> >>>>> +		plane_size = ((plane->state->src_h >> 16) - 1) << 16 |
> >>>>> +			((plane->state->src_w >> 16) - 1);
> >>>>> +	}
> >>>>> +	plane_offset = y_offset << 16 | x_offset;
> >>>>>
> >>>>>    	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
> >>>>>    	I915_WRITE(PLANE_POS(pipe, 0), 0);
> >>>>> -	I915_WRITE(PLANE_OFFSET(pipe, 0), (y << 16) | x);
> >>>>> -	I915_WRITE(PLANE_SIZE(pipe, 0),
> >>>>> -		   (intel_crtc->config->pipe_src_h - 1) << 16 |
> >>>>> -		   (intel_crtc->config->pipe_src_w - 1));
> >>>>> -	I915_WRITE(PLANE_STRIDE(pipe, 0), fb->pitches[0] / stride_div);
> >>>>> +	I915_WRITE(PLANE_OFFSET(pipe, 0), plane_offset);
> >>>>> +	I915_WRITE(PLANE_SIZE(pipe, 0), plane_size);
> >>>>> +	I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
> >>>>>    	I915_WRITE(PLANE_SURF(pipe, 0), surf_addr);
> >>>>>
> >>>>>    	POSTING_READ(PLANE_SURF(pipe, 0));
> >>>>> @@ -12406,23 +12433,32 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> >>>>>    				 intel_primary_formats, num_formats,
> >>>>>    				 DRM_PLANE_TYPE_PRIMARY);
> >>>>>
> >>>>> -	if (INTEL_INFO(dev)->gen >= 4) {
> >>>>> -		if (!dev->mode_config.rotation_property)
> >>>>> -			dev->mode_config.rotation_property =
> >>>>> -				drm_mode_create_rotation_property(dev,
> >>>>> -							BIT(DRM_ROTATE_0) |
> >>>>> -							BIT(DRM_ROTATE_180));
> >>>>> -		if (dev->mode_config.rotation_property)
> >>>>> -			drm_object_attach_property(&primary->base.base,
> >>>>> -				dev->mode_config.rotation_property,
> >>>>> -				state->base.rotation);
> >>>>> -	}
> >>>>> +	if (INTEL_INFO(dev)->gen >= 4)
> >>>>> +		intel_create_rotation_property(dev, primary);
> >>>>>
> >>>>>    	drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs);
> >>>>>
> >>>>>    	return &primary->base;
> >>>>>    }
> >>>>>
> >>>>> +void intel_create_rotation_property(struct drm_device *dev, struct intel_plane *plane)
> >>>>> +{
> >>>>> +	if (!dev->mode_config.rotation_property) {
> >>>>> +		unsigned long flags = BIT(DRM_ROTATE_0) |
> >>>>> +			BIT(DRM_ROTATE_180);
> >>>>> +
> >>>>> +		if (INTEL_INFO(dev)->gen >= 9)
> >>>>> +			flags |= BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270);
> >>>>> +
> >>>>> +		dev->mode_config.rotation_property =
> >>>>> +			drm_mode_create_rotation_property(dev, flags);
> >>>>> +	}
> >>>>> +	if (dev->mode_config.rotation_property)
> >>>>> +		drm_object_attach_property(&plane->base.base,
> >>>>> +				dev->mode_config.rotation_property,
> >>>>> +				plane->base.state->rotation);
> >>>>> +}
> >>>>> +
> >>>>>    static int
> >>>>>    intel_check_cursor_plane(struct drm_plane *plane,
> >>>>>    			 struct intel_plane_state *state)
> >>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >>>>> index 811a1db..d32025a 100644
> >>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
> >>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >>>>> @@ -995,6 +995,12 @@ intel_rotation_90_or_270(unsigned int rotation)
> >>>>>    	return rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270));
> >>>>>    }
> >>>>>
> >>>>> +unsigned int
> >>>>> +intel_tile_height(struct drm_device *dev, uint32_t bits_per_pixel,
> >>>>> +		  uint64_t fb_modifier);
> >>>>> +void intel_create_rotation_property(struct drm_device *dev,
> >>>>> +					struct intel_plane *plane);
> >>>>> +
> >>>>>    bool intel_wm_need_update(struct drm_plane *plane,
> >>>>>    			  struct drm_plane_state *state);
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> >>>>> index f41e872..83adc9b 100644
> >>>>> --- a/drivers/gpu/drm/i915/intel_sprite.c
> >>>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> >>>>> @@ -190,10 +190,13 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
> >>>>>    	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> >>>>>    	const int pipe = intel_plane->pipe;
> >>>>>    	const int plane = intel_plane->plane + 1;
> >>>>> -	u32 plane_ctl, stride_div;
> >>>>> +	u32 plane_ctl, stride_div, stride;
> >>>>>    	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> >>>>>    	const struct drm_intel_sprite_colorkey *key = &intel_plane->ckey;
> >>>>>    	unsigned long surf_addr;
> >>>>> +	u32 tile_height, plane_offset, plane_size;
> >>>>> +	unsigned int rotation;
> >>>>> +	int x_offset, y_offset;
> >>>>>
> >>>>>    	plane_ctl = PLANE_CTL_ENABLE |
> >>>>>    		PLANE_CTL_PIPE_CSC_ENABLE;
> >>>>> @@ -254,8 +257,20 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
> >>>>>    		MISSING_CASE(fb->modifier[0]);
> >>>>>    	}
> >>>>>
> >>>>> -	if (drm_plane->state->rotation == BIT(DRM_ROTATE_180))
> >>>>> +	rotation = drm_plane->state->rotation;
> >>>>> +	switch (rotation) {
> >>>>> +	case BIT(DRM_ROTATE_90):
> >>>>> +		plane_ctl |= PLANE_CTL_ROTATE_90;
> >>>>> +		break;
> >>>>> +
> >>>>> +	case BIT(DRM_ROTATE_180):
> >>>>>    		plane_ctl |= PLANE_CTL_ROTATE_180;
> >>>>> +		break;
> >>>>> +
> >>>>> +	case BIT(DRM_ROTATE_270):
> >>>>> +		plane_ctl |= PLANE_CTL_ROTATE_270;
> >>>>> +		break;
> >>>>> +	}
> >>>>>
> >>>>>    	intel_update_sprite_watermarks(drm_plane, crtc, src_w, src_h,
> >>>>>    				       pixel_size, true,
> >>>>> @@ -283,10 +298,26 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
> >>>>>
> >>>>>    	surf_addr = intel_plane_obj_offset(intel_plane, obj);
> >>>>>
> >>>>> -	I915_WRITE(PLANE_OFFSET(pipe, plane), (y << 16) | x);
> >>>>> -	I915_WRITE(PLANE_STRIDE(pipe, plane), fb->pitches[0] / stride_div);
> >>>>> +	if (intel_rotation_90_or_270(rotation)) {
> >>>>> +		/* stride: Surface height in tiles */
> >>>>> +		tile_height = intel_tile_height(dev, fb->bits_per_pixel,
> >>>>> +							fb->modifier[0]);
> >>>>> +		stride = DIV_ROUND_UP(fb->height, tile_height);
> >>>>> +		plane_size = (src_w << 16) | src_h;
> >>>>> +		x_offset = stride * tile_height - y - (src_h + 1);
> >>>>> +		y_offset = x;
> >>>>> +	} else {
> >>>>> +		stride = fb->pitches[0] / stride_div;
> >>>>> +		plane_size = (src_h << 16) | src_w;
> >>>>> +		x_offset = x;
> >>>>> +		y_offset = y;
> >>>>> +	}
> >>>>> +	plane_offset = y_offset << 16 | x_offset;
> >>>>> +
> >>>>> +	I915_WRITE(PLANE_OFFSET(pipe, plane), plane_offset);
> >>>>> +	I915_WRITE(PLANE_STRIDE(pipe, plane), stride);
> >>>>>    	I915_WRITE(PLANE_POS(pipe, plane), (crtc_y << 16) | crtc_x);
> >>>>> -	I915_WRITE(PLANE_SIZE(pipe, plane), (crtc_h << 16) | crtc_w);
> >>>>> +	I915_WRITE(PLANE_SIZE(pipe, plane), plane_size);
> >>>>>    	I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl);
> >>>>>    	I915_WRITE(PLANE_SURF(pipe, plane), surf_addr);
> >>>>>    	POSTING_READ(PLANE_SURF(pipe, plane));
> >>>>> @@ -1310,16 +1341,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> >>>>>    		goto out;
> >>>>>    	}
> >>>>>
> >>>>> -	if (!dev->mode_config.rotation_property)
> >>>>> -		dev->mode_config.rotation_property =
> >>>>> -			drm_mode_create_rotation_property(dev,
> >>>>> -							  BIT(DRM_ROTATE_0) |
> >>>>> -							  BIT(DRM_ROTATE_180));
> >>>>> -
> >>>>> -	if (dev->mode_config.rotation_property)
> >>>>> -		drm_object_attach_property(&intel_plane->base.base,
> >>>>> -					   dev->mode_config.rotation_property,
> >>>>> -					   state->base.rotation);
> >>>>> +	intel_create_rotation_property(dev, intel_plane);
> >>>>>
> >>>>>    	drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
> >>>>>
> >>>>> --
> >>>>> 1.7.10.4
> >>>>>
> >>>>> _______________________________________________
> >>>>> 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
> >>>
> >

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

Re: [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation

[Matt Roper]

On Mon, Apr 13, 2015 at 01:49:22PM +0300, Ville Syrjälä wrote:
> On Mon, Apr 13, 2015 at 03:53:22PM +0530, Jindal, Sonika wrote:
> > 
> > 
> > On 4/13/2015 3:40 PM, Ville Syrjälä wrote:
> > > On Mon, Apr 13, 2015 at 09:36:02AM +0530, Jindal, Sonika wrote:
> > >>
> > >>
> > >> On 4/10/2015 8:14 PM, Ville Syrjälä wrote:
> > >>> On Fri, Apr 10, 2015 at 04:17:17PM +0200, Daniel Vetter wrote:
> > >>>> On Fri, Apr 10, 2015 at 02:37:29PM +0530, Sonika Jindal wrote:
> > >>>>> v2: Moving creation of property in a function, checking for 90/270
> > >>>>> rotation simultaneously (Chris)
> > >>>>> Letting primary plane to be positioned
> > >>>>> v3: Adding if/else for 90/270 and rest params programming, adding check for
> > >>>>> pixel_format, some cleanup (review comments)
> > >>>>> v4: Adding right pixel_formats, using src_* params instead of crtc_* for offset
> > >>>>> and size programming (Ville)
> > >>>>> v5: Rebased on -nightly and Tvrtko's series for gtt remapping.
> > >>>>> v6: Rebased on -nightly (Tvrtko's series merged)
> > >>>>> v7: Moving pixel_format check to intel_atomic_plane_check (Matt)
> > >>>>>
> > >>>>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> > >>>>> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> > >>>>
> > >>>> Patches are missing the Testcase: tag, please add that. Also, are all the
> > >>>> igt committed or not yet? Pulled these two in meanwhile.
> > >>>
> > >>> Things are going somewhat broken because you didn't apply my plane
> > >>> state stuff. Hmm. Actually it sort of looks that it might work by luck
> > >>> as long as the primary plane doesn't get clipped since this is bashing
> > >>> the user state directly into the hardware registers and not the derived
> > >>> state (ie. clipped coordinates).
> > >>>
> > >> I was hoping your changes get merged, but not sure why they are held up.
> > >>
> > >>> Also I see my review comment about the 90 vs. 270 rotation mixup was
> > >>> ignored at least.
> > >>>
> > >> I never really got the to understand the need of reversing 90 and 270 :)
> > >> The last discussion was not concluded, AFAIR.
> > >> Things look correct to me and work fine with the testcase also.
> > >> Is there something completely different which I am unable to get?
> > >
> > > BSpec gives me the impression the hw rotation is cw, whereas the drm one
> > > is ccw.
> > >
> > Yes, HW rotation is cw as per bspec. In drm, where do we consider it as 
> > anti-cw? I assume it is cw because all the macros work as expected, ie cw.
> 
> The ccw rule was inherited from XRandR. I'm not sure what macros you
> mean, but drm_rect_rotate() will certainly give you wrong results if
> you assume cw.

It seems like this information needs to be added to the property section
of the DRM DocBook; continuing to follow XRandR probably makes sense if
that's what's agreed on, but there's no indication of that design
philosophy in the actual DRM documentation today, so we're in danger of
having different driver authors use conflicting interpretations.

90/270 rotation is already supported by existing drivers (omap for
several years now and atmel-hlcdc just recently).  I think it's too late
to try to redefine the meaning of rotation property values that are
already in active use, so we probably need to check whether omap is
using a cw or ccw scheme and follow (and document!) that.


Matt

> 
> > 
> > >>
> > >> -Sonika
> > >>
> > >>>> -Daniel
> > >>>>
> > >>>>> ---
> > >>>>>    drivers/gpu/drm/i915/i915_reg.h           |    2 +
> > >>>>>    drivers/gpu/drm/i915/intel_atomic_plane.c |   24 ++++++++
> > >>>>>    drivers/gpu/drm/i915/intel_display.c      |   88 ++++++++++++++++++++---------
> > >>>>>    drivers/gpu/drm/i915/intel_drv.h          |    6 ++
> > >>>>>    drivers/gpu/drm/i915/intel_sprite.c       |   52 ++++++++++++-----
> > >>>>>    5 files changed, 131 insertions(+), 41 deletions(-)
> > >>>>>
> > >>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > >>>>> index b522eb6..564bbd5 100644
> > >>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
> > >>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
> > >>>>> @@ -4854,7 +4854,9 @@ enum skl_disp_power_wells {
> > >>>>>    #define   PLANE_CTL_ALPHA_HW_PREMULTIPLY	(  3 << 4)
> > >>>>>    #define   PLANE_CTL_ROTATE_MASK			0x3
> > >>>>>    #define   PLANE_CTL_ROTATE_0			0x0
> > >>>>> +#define   PLANE_CTL_ROTATE_90			0x1
> > >>>>>    #define   PLANE_CTL_ROTATE_180			0x2
> > >>>>> +#define   PLANE_CTL_ROTATE_270			0x3
> > >>>>>    #define _PLANE_STRIDE_1_A			0x70188
> > >>>>>    #define _PLANE_STRIDE_2_A			0x70288
> > >>>>>    #define _PLANE_STRIDE_3_A			0x70388
> > >>>>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > >>>>> index 976b891..a27ee8c 100644
> > >>>>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> > >>>>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> > >>>>> @@ -162,6 +162,30 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> > >>>>>    			(1 << drm_plane_index(plane));
> > >>>>>    	}
> > >>>>>
> > >>>>> +	if (state->fb && intel_rotation_90_or_270(state->rotation)) {
> > >>>>> +		if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
> > >>>>> +			state->fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
> > >>>>> +			DRM_DEBUG_KMS("Y/Yf tiling required for 90/270!\n");
> > >>>>> +			return -EINVAL;
> > >>>>> +		}
> > >>>>> +
> > >>>>> +		/*
> > >>>>> +		 * 90/270 is not allowed with RGB64 16:16:16:16,
> > >>>>> +		 * RGB 16-bit 5:6:5, and Indexed 8-bit.
> > >>>>> +		 * TBD: Add RGB64 case once its added in supported format list.
> > >>>>> +		 */
> > >>>>> +		switch (state->fb->pixel_format) {
> > >>>>> +		case DRM_FORMAT_C8:
> > >>>>> +		case DRM_FORMAT_RGB565:
> > >>>>> +			DRM_DEBUG_KMS("Unsupported pixel format %s for 90/270!\n",
> > >>>>> +					drm_get_format_name(state->fb->pixel_format));
> > >>>>> +			return -EINVAL;
> > >>>>> +
> > >>>>> +		default:
> > >>>>> +			break;
> > >>>>> +		}
> > >>>>> +	}
> > >>>>> +
> > >>>>>    	return intel_plane->check_plane(plane, intel_state);
> > >>>>>    }
> > >>>>>
> > >>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > >>>>> index f0bbc22..4de544c 100644
> > >>>>> --- a/drivers/gpu/drm/i915/intel_display.c
> > >>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
> > >>>>> @@ -2311,13 +2311,6 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb,
> > >>>>>    	info->pitch = fb->pitches[0];
> > >>>>>    	info->fb_modifier = fb->modifier[0];
> > >>>>>
> > >>>>> -	if (!(info->fb_modifier == I915_FORMAT_MOD_Y_TILED ||
> > >>>>> -	      info->fb_modifier == I915_FORMAT_MOD_Yf_TILED)) {
> > >>>>> -		DRM_DEBUG_KMS(
> > >>>>> -			      "Y or Yf tiling is needed for 90/270 rotation!\n");
> > >>>>> -		return -EINVAL;
> > >>>>> -	}
> > >>>>> -
> > >>>>>    	return 0;
> > >>>>>    }
> > >>>>>
> > >>>>> @@ -2919,8 +2912,12 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
> > >>>>>    	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > >>>>>    	struct drm_i915_gem_object *obj;
> > >>>>>    	int pipe = intel_crtc->pipe;
> > >>>>> -	u32 plane_ctl, stride_div;
> > >>>>> +	u32 plane_ctl, stride_div, stride;
> > >>>>> +	u32 tile_height, plane_offset, plane_size;
> > >>>>> +	unsigned int rotation;
> > >>>>> +	int x_offset, y_offset;
> > >>>>>    	unsigned long surf_addr;
> > >>>>> +	struct drm_plane *plane;
> > >>>>>
> > >>>>>    	if (!intel_crtc->primary_enabled) {
> > >>>>>    		I915_WRITE(PLANE_CTL(pipe, 0), 0);
> > >>>>> @@ -2981,21 +2978,51 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
> > >>>>>    	}
> > >>>>>
> > >>>>>    	plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
> > >>>>> -	if (crtc->primary->state->rotation == BIT(DRM_ROTATE_180))
> > >>>>> +
> > >>>>> +	plane = crtc->primary;
> > >>>>> +	rotation = plane->state->rotation;
> > >>>>> +	switch (rotation) {
> > >>>>> +	case BIT(DRM_ROTATE_90):
> > >>>>> +		plane_ctl |= PLANE_CTL_ROTATE_90;
> > >>>>> +		break;
> > >>>>> +
> > >>>>> +	case BIT(DRM_ROTATE_180):
> > >>>>>    		plane_ctl |= PLANE_CTL_ROTATE_180;
> > >>>>> +		break;
> > >>>>> +
> > >>>>> +	case BIT(DRM_ROTATE_270):
> > >>>>> +		plane_ctl |= PLANE_CTL_ROTATE_270;
> > >>>>> +		break;
> > >>>>> +	}
> > >>>>>
> > >>>>>    	obj = intel_fb_obj(fb);
> > >>>>>    	stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
> > >>>>>    					       fb->pixel_format);
> > >>>>> -	surf_addr = intel_plane_obj_offset(to_intel_plane(crtc->primary), obj);
> > >>>>> +	surf_addr = intel_plane_obj_offset(to_intel_plane(plane), obj);
> > >>>>> +
> > >>>>> +	if (intel_rotation_90_or_270(rotation)) {
> > >>>>> +		/* stride = Surface height in tiles */
> > >>>>> +		tile_height = intel_tile_height(dev, fb->bits_per_pixel,
> > >>>>> +						fb->modifier[0]);
> > >>>>> +		stride = DIV_ROUND_UP(fb->height, tile_height);
> > >>>>> +		x_offset = stride * tile_height - y - (plane->state->src_h >> 16);
> > >>>>> +		y_offset = x;
> > >>>>> +		plane_size = ((plane->state->src_w >> 16) - 1) << 16 |
> > >>>>> +					((plane->state->src_h >> 16) - 1);
> > >>>>> +	} else {
> > >>>>> +		stride = fb->pitches[0] / stride_div;
> > >>>>> +		x_offset = x;
> > >>>>> +		y_offset = y;
> > >>>>> +		plane_size = ((plane->state->src_h >> 16) - 1) << 16 |
> > >>>>> +			((plane->state->src_w >> 16) - 1);
> > >>>>> +	}
> > >>>>> +	plane_offset = y_offset << 16 | x_offset;
> > >>>>>
> > >>>>>    	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
> > >>>>>    	I915_WRITE(PLANE_POS(pipe, 0), 0);
> > >>>>> -	I915_WRITE(PLANE_OFFSET(pipe, 0), (y << 16) | x);
> > >>>>> -	I915_WRITE(PLANE_SIZE(pipe, 0),
> > >>>>> -		   (intel_crtc->config->pipe_src_h - 1) << 16 |
> > >>>>> -		   (intel_crtc->config->pipe_src_w - 1));
> > >>>>> -	I915_WRITE(PLANE_STRIDE(pipe, 0), fb->pitches[0] / stride_div);
> > >>>>> +	I915_WRITE(PLANE_OFFSET(pipe, 0), plane_offset);
> > >>>>> +	I915_WRITE(PLANE_SIZE(pipe, 0), plane_size);
> > >>>>> +	I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
> > >>>>>    	I915_WRITE(PLANE_SURF(pipe, 0), surf_addr);
> > >>>>>
> > >>>>>    	POSTING_READ(PLANE_SURF(pipe, 0));
> > >>>>> @@ -12406,23 +12433,32 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> > >>>>>    				 intel_primary_formats, num_formats,
> > >>>>>    				 DRM_PLANE_TYPE_PRIMARY);
> > >>>>>
> > >>>>> -	if (INTEL_INFO(dev)->gen >= 4) {
> > >>>>> -		if (!dev->mode_config.rotation_property)
> > >>>>> -			dev->mode_config.rotation_property =
> > >>>>> -				drm_mode_create_rotation_property(dev,
> > >>>>> -							BIT(DRM_ROTATE_0) |
> > >>>>> -							BIT(DRM_ROTATE_180));
> > >>>>> -		if (dev->mode_config.rotation_property)
> > >>>>> -			drm_object_attach_property(&primary->base.base,
> > >>>>> -				dev->mode_config.rotation_property,
> > >>>>> -				state->base.rotation);
> > >>>>> -	}
> > >>>>> +	if (INTEL_INFO(dev)->gen >= 4)
> > >>>>> +		intel_create_rotation_property(dev, primary);
> > >>>>>
> > >>>>>    	drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs);
> > >>>>>
> > >>>>>    	return &primary->base;
> > >>>>>    }
> > >>>>>
> > >>>>> +void intel_create_rotation_property(struct drm_device *dev, struct intel_plane *plane)
> > >>>>> +{
> > >>>>> +	if (!dev->mode_config.rotation_property) {
> > >>>>> +		unsigned long flags = BIT(DRM_ROTATE_0) |
> > >>>>> +			BIT(DRM_ROTATE_180);
> > >>>>> +
> > >>>>> +		if (INTEL_INFO(dev)->gen >= 9)
> > >>>>> +			flags |= BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270);
> > >>>>> +
> > >>>>> +		dev->mode_config.rotation_property =
> > >>>>> +			drm_mode_create_rotation_property(dev, flags);
> > >>>>> +	}
> > >>>>> +	if (dev->mode_config.rotation_property)
> > >>>>> +		drm_object_attach_property(&plane->base.base,
> > >>>>> +				dev->mode_config.rotation_property,
> > >>>>> +				plane->base.state->rotation);
> > >>>>> +}
> > >>>>> +
> > >>>>>    static int
> > >>>>>    intel_check_cursor_plane(struct drm_plane *plane,
> > >>>>>    			 struct intel_plane_state *state)
> > >>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > >>>>> index 811a1db..d32025a 100644
> > >>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
> > >>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
> > >>>>> @@ -995,6 +995,12 @@ intel_rotation_90_or_270(unsigned int rotation)
> > >>>>>    	return rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270));
> > >>>>>    }
> > >>>>>
> > >>>>> +unsigned int
> > >>>>> +intel_tile_height(struct drm_device *dev, uint32_t bits_per_pixel,
> > >>>>> +		  uint64_t fb_modifier);
> > >>>>> +void intel_create_rotation_property(struct drm_device *dev,
> > >>>>> +					struct intel_plane *plane);
> > >>>>> +
> > >>>>>    bool intel_wm_need_update(struct drm_plane *plane,
> > >>>>>    			  struct drm_plane_state *state);
> > >>>>>
> > >>>>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > >>>>> index f41e872..83adc9b 100644
> > >>>>> --- a/drivers/gpu/drm/i915/intel_sprite.c
> > >>>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > >>>>> @@ -190,10 +190,13 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
> > >>>>>    	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> > >>>>>    	const int pipe = intel_plane->pipe;
> > >>>>>    	const int plane = intel_plane->plane + 1;
> > >>>>> -	u32 plane_ctl, stride_div;
> > >>>>> +	u32 plane_ctl, stride_div, stride;
> > >>>>>    	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> > >>>>>    	const struct drm_intel_sprite_colorkey *key = &intel_plane->ckey;
> > >>>>>    	unsigned long surf_addr;
> > >>>>> +	u32 tile_height, plane_offset, plane_size;
> > >>>>> +	unsigned int rotation;
> > >>>>> +	int x_offset, y_offset;
> > >>>>>
> > >>>>>    	plane_ctl = PLANE_CTL_ENABLE |
> > >>>>>    		PLANE_CTL_PIPE_CSC_ENABLE;
> > >>>>> @@ -254,8 +257,20 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
> > >>>>>    		MISSING_CASE(fb->modifier[0]);
> > >>>>>    	}
> > >>>>>
> > >>>>> -	if (drm_plane->state->rotation == BIT(DRM_ROTATE_180))
> > >>>>> +	rotation = drm_plane->state->rotation;
> > >>>>> +	switch (rotation) {
> > >>>>> +	case BIT(DRM_ROTATE_90):
> > >>>>> +		plane_ctl |= PLANE_CTL_ROTATE_90;
> > >>>>> +		break;
> > >>>>> +
> > >>>>> +	case BIT(DRM_ROTATE_180):
> > >>>>>    		plane_ctl |= PLANE_CTL_ROTATE_180;
> > >>>>> +		break;
> > >>>>> +
> > >>>>> +	case BIT(DRM_ROTATE_270):
> > >>>>> +		plane_ctl |= PLANE_CTL_ROTATE_270;
> > >>>>> +		break;
> > >>>>> +	}
> > >>>>>
> > >>>>>    	intel_update_sprite_watermarks(drm_plane, crtc, src_w, src_h,
> > >>>>>    				       pixel_size, true,
> > >>>>> @@ -283,10 +298,26 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
> > >>>>>
> > >>>>>    	surf_addr = intel_plane_obj_offset(intel_plane, obj);
> > >>>>>
> > >>>>> -	I915_WRITE(PLANE_OFFSET(pipe, plane), (y << 16) | x);
> > >>>>> -	I915_WRITE(PLANE_STRIDE(pipe, plane), fb->pitches[0] / stride_div);
> > >>>>> +	if (intel_rotation_90_or_270(rotation)) {
> > >>>>> +		/* stride: Surface height in tiles */
> > >>>>> +		tile_height = intel_tile_height(dev, fb->bits_per_pixel,
> > >>>>> +							fb->modifier[0]);
> > >>>>> +		stride = DIV_ROUND_UP(fb->height, tile_height);
> > >>>>> +		plane_size = (src_w << 16) | src_h;
> > >>>>> +		x_offset = stride * tile_height - y - (src_h + 1);
> > >>>>> +		y_offset = x;
> > >>>>> +	} else {
> > >>>>> +		stride = fb->pitches[0] / stride_div;
> > >>>>> +		plane_size = (src_h << 16) | src_w;
> > >>>>> +		x_offset = x;
> > >>>>> +		y_offset = y;
> > >>>>> +	}
> > >>>>> +	plane_offset = y_offset << 16 | x_offset;
> > >>>>> +
> > >>>>> +	I915_WRITE(PLANE_OFFSET(pipe, plane), plane_offset);
> > >>>>> +	I915_WRITE(PLANE_STRIDE(pipe, plane), stride);
> > >>>>>    	I915_WRITE(PLANE_POS(pipe, plane), (crtc_y << 16) | crtc_x);
> > >>>>> -	I915_WRITE(PLANE_SIZE(pipe, plane), (crtc_h << 16) | crtc_w);
> > >>>>> +	I915_WRITE(PLANE_SIZE(pipe, plane), plane_size);
> > >>>>>    	I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl);
> > >>>>>    	I915_WRITE(PLANE_SURF(pipe, plane), surf_addr);
> > >>>>>    	POSTING_READ(PLANE_SURF(pipe, plane));
> > >>>>> @@ -1310,16 +1341,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> > >>>>>    		goto out;
> > >>>>>    	}
> > >>>>>
> > >>>>> -	if (!dev->mode_config.rotation_property)
> > >>>>> -		dev->mode_config.rotation_property =
> > >>>>> -			drm_mode_create_rotation_property(dev,
> > >>>>> -							  BIT(DRM_ROTATE_0) |
> > >>>>> -							  BIT(DRM_ROTATE_180));
> > >>>>> -
> > >>>>> -	if (dev->mode_config.rotation_property)
> > >>>>> -		drm_object_attach_property(&intel_plane->base.base,
> > >>>>> -					   dev->mode_config.rotation_property,
> > >>>>> -					   state->base.rotation);
> > >>>>> +	intel_create_rotation_property(dev, intel_plane);
> > >>>>>
> > >>>>>    	drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
> > >>>>>
> > >>>>> --
> > >>>>> 1.7.10.4
> > >>>>>
> > >>>>> _______________________________________________
> > >>>>> 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
> > >>>
> > >
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation

[Jindal, Sonika]

On 4/14/2015 5:09 AM, Matt Roper wrote:
> On Mon, Apr 13, 2015 at 01:49:22PM +0300, Ville Syrjälä wrote:
>> On Mon, Apr 13, 2015 at 03:53:22PM +0530, Jindal, Sonika wrote:
>>>
>>>
>>> On 4/13/2015 3:40 PM, Ville Syrjälä wrote:
>>>> On Mon, Apr 13, 2015 at 09:36:02AM +0530, Jindal, Sonika wrote:
>>>>>
>>>>>
>>>>> On 4/10/2015 8:14 PM, Ville Syrjälä wrote:
>>>>>> On Fri, Apr 10, 2015 at 04:17:17PM +0200, Daniel Vetter wrote:
>>>>>>> On Fri, Apr 10, 2015 at 02:37:29PM +0530, Sonika Jindal wrote:
>>>>>>>> v2: Moving creation of property in a function, checking for 90/270
>>>>>>>> rotation simultaneously (Chris)
>>>>>>>> Letting primary plane to be positioned
>>>>>>>> v3: Adding if/else for 90/270 and rest params programming, adding check for
>>>>>>>> pixel_format, some cleanup (review comments)
>>>>>>>> v4: Adding right pixel_formats, using src_* params instead of crtc_* for offset
>>>>>>>> and size programming (Ville)
>>>>>>>> v5: Rebased on -nightly and Tvrtko's series for gtt remapping.
>>>>>>>> v6: Rebased on -nightly (Tvrtko's series merged)
>>>>>>>> v7: Moving pixel_format check to intel_atomic_plane_check (Matt)
>>>>>>>>
>>>>>>>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
>>>>>>>> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
>>>>>>>
>>>>>>> Patches are missing the Testcase: tag, please add that. Also, are all the
>>>>>>> igt committed or not yet? Pulled these two in meanwhile.
>>>>>>
>>>>>> Things are going somewhat broken because you didn't apply my plane
>>>>>> state stuff. Hmm. Actually it sort of looks that it might work by luck
>>>>>> as long as the primary plane doesn't get clipped since this is bashing
>>>>>> the user state directly into the hardware registers and not the derived
>>>>>> state (ie. clipped coordinates).
>>>>>>
>>>>> I was hoping your changes get merged, but not sure why they are held up.
>>>>>
>>>>>> Also I see my review comment about the 90 vs. 270 rotation mixup was
>>>>>> ignored at least.
>>>>>>
>>>>> I never really got the to understand the need of reversing 90 and 270 :)
>>>>> The last discussion was not concluded, AFAIR.
>>>>> Things look correct to me and work fine with the testcase also.
>>>>> Is there something completely different which I am unable to get?
>>>>
>>>> BSpec gives me the impression the hw rotation is cw, whereas the drm one
>>>> is ccw.
>>>>
>>> Yes, HW rotation is cw as per bspec. In drm, where do we consider it as
>>> anti-cw? I assume it is cw because all the macros work as expected, ie cw.
>>
>> The ccw rule was inherited from XRandR. I'm not sure what macros you
>> mean, but drm_rect_rotate() will certainly give you wrong results if
>> you assume cw.
>
> It seems like this information needs to be added to the property section
> of the DRM DocBook; continuing to follow XRandR probably makes sense if
> that's what's agreed on, but there's no indication of that design
> philosophy in the actual DRM documentation today, so we're in danger of
> having different driver authors use conflicting interpretations.
>
Ok, I will create a DocBook patch with description for 90/270 rotation 
(Anyways 90/270 rotation is not added in the rotation property in 
documentation). Will there be any other place for this or i915's 
rotation property?

Also, I will send a patch to flip 90/270 in i915. I am just worried, 
that it will look confusing to check for DRM_ROTATE_90 and use 
PLANE_CTL_ROTATE_270 for programming. I will add a comment though.
Sounds good?

-Sonika
> 90/270 rotation is already supported by existing drivers (omap for
> several years now and atmel-hlcdc just recently).  I think it's too late
> to try to redefine the meaning of rotation property values that are
> already in active use, so we probably need to check whether omap is
> using a cw or ccw scheme and follow (and document!) that.
>
>
> Matt
>
>>
>>>
>>>>>
>>>>> -Sonika
>>>>>
>>>>>>> -Daniel
>>>>>>>
>>>>>>>> ---
>>>>>>>>     drivers/gpu/drm/i915/i915_reg.h           |    2 +
>>>>>>>>     drivers/gpu/drm/i915/intel_atomic_plane.c |   24 ++++++++
>>>>>>>>     drivers/gpu/drm/i915/intel_display.c      |   88 ++++++++++++++++++++---------
>>>>>>>>     drivers/gpu/drm/i915/intel_drv.h          |    6 ++
>>>>>>>>     drivers/gpu/drm/i915/intel_sprite.c       |   52 ++++++++++++-----
>>>>>>>>     5 files changed, 131 insertions(+), 41 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>>>>>>> index b522eb6..564bbd5 100644
>>>>>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>>>>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>>>>>>> @@ -4854,7 +4854,9 @@ enum skl_disp_power_wells {
>>>>>>>>     #define   PLANE_CTL_ALPHA_HW_PREMULTIPLY	(  3 << 4)
>>>>>>>>     #define   PLANE_CTL_ROTATE_MASK			0x3
>>>>>>>>     #define   PLANE_CTL_ROTATE_0			0x0
>>>>>>>> +#define   PLANE_CTL_ROTATE_90			0x1
>>>>>>>>     #define   PLANE_CTL_ROTATE_180			0x2
>>>>>>>> +#define   PLANE_CTL_ROTATE_270			0x3
>>>>>>>>     #define _PLANE_STRIDE_1_A			0x70188
>>>>>>>>     #define _PLANE_STRIDE_2_A			0x70288
>>>>>>>>     #define _PLANE_STRIDE_3_A			0x70388
>>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
>>>>>>>> index 976b891..a27ee8c 100644
>>>>>>>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
>>>>>>>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
>>>>>>>> @@ -162,6 +162,30 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>>>>>>>>     			(1 << drm_plane_index(plane));
>>>>>>>>     	}
>>>>>>>>
>>>>>>>> +	if (state->fb && intel_rotation_90_or_270(state->rotation)) {
>>>>>>>> +		if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
>>>>>>>> +			state->fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
>>>>>>>> +			DRM_DEBUG_KMS("Y/Yf tiling required for 90/270!\n");
>>>>>>>> +			return -EINVAL;
>>>>>>>> +		}
>>>>>>>> +
>>>>>>>> +		/*
>>>>>>>> +		 * 90/270 is not allowed with RGB64 16:16:16:16,
>>>>>>>> +		 * RGB 16-bit 5:6:5, and Indexed 8-bit.
>>>>>>>> +		 * TBD: Add RGB64 case once its added in supported format list.
>>>>>>>> +		 */
>>>>>>>> +		switch (state->fb->pixel_format) {
>>>>>>>> +		case DRM_FORMAT_C8:
>>>>>>>> +		case DRM_FORMAT_RGB565:
>>>>>>>> +			DRM_DEBUG_KMS("Unsupported pixel format %s for 90/270!\n",
>>>>>>>> +					drm_get_format_name(state->fb->pixel_format));
>>>>>>>> +			return -EINVAL;
>>>>>>>> +
>>>>>>>> +		default:
>>>>>>>> +			break;
>>>>>>>> +		}
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>>     	return intel_plane->check_plane(plane, intel_state);
>>>>>>>>     }
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>>>>>> index f0bbc22..4de544c 100644
>>>>>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>>>>>> @@ -2311,13 +2311,6 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb,
>>>>>>>>     	info->pitch = fb->pitches[0];
>>>>>>>>     	info->fb_modifier = fb->modifier[0];
>>>>>>>>
>>>>>>>> -	if (!(info->fb_modifier == I915_FORMAT_MOD_Y_TILED ||
>>>>>>>> -	      info->fb_modifier == I915_FORMAT_MOD_Yf_TILED)) {
>>>>>>>> -		DRM_DEBUG_KMS(
>>>>>>>> -			      "Y or Yf tiling is needed for 90/270 rotation!\n");
>>>>>>>> -		return -EINVAL;
>>>>>>>> -	}
>>>>>>>> -
>>>>>>>>     	return 0;
>>>>>>>>     }
>>>>>>>>
>>>>>>>> @@ -2919,8 +2912,12 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>>>>>>>>     	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>>>>>>>     	struct drm_i915_gem_object *obj;
>>>>>>>>     	int pipe = intel_crtc->pipe;
>>>>>>>> -	u32 plane_ctl, stride_div;
>>>>>>>> +	u32 plane_ctl, stride_div, stride;
>>>>>>>> +	u32 tile_height, plane_offset, plane_size;
>>>>>>>> +	unsigned int rotation;
>>>>>>>> +	int x_offset, y_offset;
>>>>>>>>     	unsigned long surf_addr;
>>>>>>>> +	struct drm_plane *plane;
>>>>>>>>
>>>>>>>>     	if (!intel_crtc->primary_enabled) {
>>>>>>>>     		I915_WRITE(PLANE_CTL(pipe, 0), 0);
>>>>>>>> @@ -2981,21 +2978,51 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>>>>>>>>     	}
>>>>>>>>
>>>>>>>>     	plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
>>>>>>>> -	if (crtc->primary->state->rotation == BIT(DRM_ROTATE_180))
>>>>>>>> +
>>>>>>>> +	plane = crtc->primary;
>>>>>>>> +	rotation = plane->state->rotation;
>>>>>>>> +	switch (rotation) {
>>>>>>>> +	case BIT(DRM_ROTATE_90):
>>>>>>>> +		plane_ctl |= PLANE_CTL_ROTATE_90;
>>>>>>>> +		break;
>>>>>>>> +
>>>>>>>> +	case BIT(DRM_ROTATE_180):
>>>>>>>>     		plane_ctl |= PLANE_CTL_ROTATE_180;
>>>>>>>> +		break;
>>>>>>>> +
>>>>>>>> +	case BIT(DRM_ROTATE_270):
>>>>>>>> +		plane_ctl |= PLANE_CTL_ROTATE_270;
>>>>>>>> +		break;
>>>>>>>> +	}
>>>>>>>>
>>>>>>>>     	obj = intel_fb_obj(fb);
>>>>>>>>     	stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
>>>>>>>>     					       fb->pixel_format);
>>>>>>>> -	surf_addr = intel_plane_obj_offset(to_intel_plane(crtc->primary), obj);
>>>>>>>> +	surf_addr = intel_plane_obj_offset(to_intel_plane(plane), obj);
>>>>>>>> +
>>>>>>>> +	if (intel_rotation_90_or_270(rotation)) {
>>>>>>>> +		/* stride = Surface height in tiles */
>>>>>>>> +		tile_height = intel_tile_height(dev, fb->bits_per_pixel,
>>>>>>>> +						fb->modifier[0]);
>>>>>>>> +		stride = DIV_ROUND_UP(fb->height, tile_height);
>>>>>>>> +		x_offset = stride * tile_height - y - (plane->state->src_h >> 16);
>>>>>>>> +		y_offset = x;
>>>>>>>> +		plane_size = ((plane->state->src_w >> 16) - 1) << 16 |
>>>>>>>> +					((plane->state->src_h >> 16) - 1);
>>>>>>>> +	} else {
>>>>>>>> +		stride = fb->pitches[0] / stride_div;
>>>>>>>> +		x_offset = x;
>>>>>>>> +		y_offset = y;
>>>>>>>> +		plane_size = ((plane->state->src_h >> 16) - 1) << 16 |
>>>>>>>> +			((plane->state->src_w >> 16) - 1);
>>>>>>>> +	}
>>>>>>>> +	plane_offset = y_offset << 16 | x_offset;
>>>>>>>>
>>>>>>>>     	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
>>>>>>>>     	I915_WRITE(PLANE_POS(pipe, 0), 0);
>>>>>>>> -	I915_WRITE(PLANE_OFFSET(pipe, 0), (y << 16) | x);
>>>>>>>> -	I915_WRITE(PLANE_SIZE(pipe, 0),
>>>>>>>> -		   (intel_crtc->config->pipe_src_h - 1) << 16 |
>>>>>>>> -		   (intel_crtc->config->pipe_src_w - 1));
>>>>>>>> -	I915_WRITE(PLANE_STRIDE(pipe, 0), fb->pitches[0] / stride_div);
>>>>>>>> +	I915_WRITE(PLANE_OFFSET(pipe, 0), plane_offset);
>>>>>>>> +	I915_WRITE(PLANE_SIZE(pipe, 0), plane_size);
>>>>>>>> +	I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
>>>>>>>>     	I915_WRITE(PLANE_SURF(pipe, 0), surf_addr);
>>>>>>>>
>>>>>>>>     	POSTING_READ(PLANE_SURF(pipe, 0));
>>>>>>>> @@ -12406,23 +12433,32 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>>>>>>>>     				 intel_primary_formats, num_formats,
>>>>>>>>     				 DRM_PLANE_TYPE_PRIMARY);
>>>>>>>>
>>>>>>>> -	if (INTEL_INFO(dev)->gen >= 4) {
>>>>>>>> -		if (!dev->mode_config.rotation_property)
>>>>>>>> -			dev->mode_config.rotation_property =
>>>>>>>> -				drm_mode_create_rotation_property(dev,
>>>>>>>> -							BIT(DRM_ROTATE_0) |
>>>>>>>> -							BIT(DRM_ROTATE_180));
>>>>>>>> -		if (dev->mode_config.rotation_property)
>>>>>>>> -			drm_object_attach_property(&primary->base.base,
>>>>>>>> -				dev->mode_config.rotation_property,
>>>>>>>> -				state->base.rotation);
>>>>>>>> -	}
>>>>>>>> +	if (INTEL_INFO(dev)->gen >= 4)
>>>>>>>> +		intel_create_rotation_property(dev, primary);
>>>>>>>>
>>>>>>>>     	drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs);
>>>>>>>>
>>>>>>>>     	return &primary->base;
>>>>>>>>     }
>>>>>>>>
>>>>>>>> +void intel_create_rotation_property(struct drm_device *dev, struct intel_plane *plane)
>>>>>>>> +{
>>>>>>>> +	if (!dev->mode_config.rotation_property) {
>>>>>>>> +		unsigned long flags = BIT(DRM_ROTATE_0) |
>>>>>>>> +			BIT(DRM_ROTATE_180);
>>>>>>>> +
>>>>>>>> +		if (INTEL_INFO(dev)->gen >= 9)
>>>>>>>> +			flags |= BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270);
>>>>>>>> +
>>>>>>>> +		dev->mode_config.rotation_property =
>>>>>>>> +			drm_mode_create_rotation_property(dev, flags);
>>>>>>>> +	}
>>>>>>>> +	if (dev->mode_config.rotation_property)
>>>>>>>> +		drm_object_attach_property(&plane->base.base,
>>>>>>>> +				dev->mode_config.rotation_property,
>>>>>>>> +				plane->base.state->rotation);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>     static int
>>>>>>>>     intel_check_cursor_plane(struct drm_plane *plane,
>>>>>>>>     			 struct intel_plane_state *state)
>>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>>>>>>> index 811a1db..d32025a 100644
>>>>>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>>>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>>>>>> @@ -995,6 +995,12 @@ intel_rotation_90_or_270(unsigned int rotation)
>>>>>>>>     	return rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270));
>>>>>>>>     }
>>>>>>>>
>>>>>>>> +unsigned int
>>>>>>>> +intel_tile_height(struct drm_device *dev, uint32_t bits_per_pixel,
>>>>>>>> +		  uint64_t fb_modifier);
>>>>>>>> +void intel_create_rotation_property(struct drm_device *dev,
>>>>>>>> +					struct intel_plane *plane);
>>>>>>>> +
>>>>>>>>     bool intel_wm_need_update(struct drm_plane *plane,
>>>>>>>>     			  struct drm_plane_state *state);
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>>>>>>>> index f41e872..83adc9b 100644
>>>>>>>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>>>>>>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>>>>>>>> @@ -190,10 +190,13 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>>>>>>>>     	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>>>>>>>>     	const int pipe = intel_plane->pipe;
>>>>>>>>     	const int plane = intel_plane->plane + 1;
>>>>>>>> -	u32 plane_ctl, stride_div;
>>>>>>>> +	u32 plane_ctl, stride_div, stride;
>>>>>>>>     	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
>>>>>>>>     	const struct drm_intel_sprite_colorkey *key = &intel_plane->ckey;
>>>>>>>>     	unsigned long surf_addr;
>>>>>>>> +	u32 tile_height, plane_offset, plane_size;
>>>>>>>> +	unsigned int rotation;
>>>>>>>> +	int x_offset, y_offset;
>>>>>>>>
>>>>>>>>     	plane_ctl = PLANE_CTL_ENABLE |
>>>>>>>>     		PLANE_CTL_PIPE_CSC_ENABLE;
>>>>>>>> @@ -254,8 +257,20 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>>>>>>>>     		MISSING_CASE(fb->modifier[0]);
>>>>>>>>     	}
>>>>>>>>
>>>>>>>> -	if (drm_plane->state->rotation == BIT(DRM_ROTATE_180))
>>>>>>>> +	rotation = drm_plane->state->rotation;
>>>>>>>> +	switch (rotation) {
>>>>>>>> +	case BIT(DRM_ROTATE_90):
>>>>>>>> +		plane_ctl |= PLANE_CTL_ROTATE_90;
>>>>>>>> +		break;
>>>>>>>> +
>>>>>>>> +	case BIT(DRM_ROTATE_180):
>>>>>>>>     		plane_ctl |= PLANE_CTL_ROTATE_180;
>>>>>>>> +		break;
>>>>>>>> +
>>>>>>>> +	case BIT(DRM_ROTATE_270):
>>>>>>>> +		plane_ctl |= PLANE_CTL_ROTATE_270;
>>>>>>>> +		break;
>>>>>>>> +	}
>>>>>>>>
>>>>>>>>     	intel_update_sprite_watermarks(drm_plane, crtc, src_w, src_h,
>>>>>>>>     				       pixel_size, true,
>>>>>>>> @@ -283,10 +298,26 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>>>>>>>>
>>>>>>>>     	surf_addr = intel_plane_obj_offset(intel_plane, obj);
>>>>>>>>
>>>>>>>> -	I915_WRITE(PLANE_OFFSET(pipe, plane), (y << 16) | x);
>>>>>>>> -	I915_WRITE(PLANE_STRIDE(pipe, plane), fb->pitches[0] / stride_div);
>>>>>>>> +	if (intel_rotation_90_or_270(rotation)) {
>>>>>>>> +		/* stride: Surface height in tiles */
>>>>>>>> +		tile_height = intel_tile_height(dev, fb->bits_per_pixel,
>>>>>>>> +							fb->modifier[0]);
>>>>>>>> +		stride = DIV_ROUND_UP(fb->height, tile_height);
>>>>>>>> +		plane_size = (src_w << 16) | src_h;
>>>>>>>> +		x_offset = stride * tile_height - y - (src_h + 1);
>>>>>>>> +		y_offset = x;
>>>>>>>> +	} else {
>>>>>>>> +		stride = fb->pitches[0] / stride_div;
>>>>>>>> +		plane_size = (src_h << 16) | src_w;
>>>>>>>> +		x_offset = x;
>>>>>>>> +		y_offset = y;
>>>>>>>> +	}
>>>>>>>> +	plane_offset = y_offset << 16 | x_offset;
>>>>>>>> +
>>>>>>>> +	I915_WRITE(PLANE_OFFSET(pipe, plane), plane_offset);
>>>>>>>> +	I915_WRITE(PLANE_STRIDE(pipe, plane), stride);
>>>>>>>>     	I915_WRITE(PLANE_POS(pipe, plane), (crtc_y << 16) | crtc_x);
>>>>>>>> -	I915_WRITE(PLANE_SIZE(pipe, plane), (crtc_h << 16) | crtc_w);
>>>>>>>> +	I915_WRITE(PLANE_SIZE(pipe, plane), plane_size);
>>>>>>>>     	I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl);
>>>>>>>>     	I915_WRITE(PLANE_SURF(pipe, plane), surf_addr);
>>>>>>>>     	POSTING_READ(PLANE_SURF(pipe, plane));
>>>>>>>> @@ -1310,16 +1341,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>>>>>>>>     		goto out;
>>>>>>>>     	}
>>>>>>>>
>>>>>>>> -	if (!dev->mode_config.rotation_property)
>>>>>>>> -		dev->mode_config.rotation_property =
>>>>>>>> -			drm_mode_create_rotation_property(dev,
>>>>>>>> -							  BIT(DRM_ROTATE_0) |
>>>>>>>> -							  BIT(DRM_ROTATE_180));
>>>>>>>> -
>>>>>>>> -	if (dev->mode_config.rotation_property)
>>>>>>>> -		drm_object_attach_property(&intel_plane->base.base,
>>>>>>>> -					   dev->mode_config.rotation_property,
>>>>>>>> -					   state->base.rotation);
>>>>>>>> +	intel_create_rotation_property(dev, intel_plane);
>>>>>>>>
>>>>>>>>     	drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
>>>>>>>>
>>>>>>>> --
>>>>>>>> 1.7.10.4
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> 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
>>>>>>
>>>>
>>
>> --
>> Ville Syrjälä
>> Intel OTC
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation

[Daniel Vetter]

On Tue, Apr 14, 2015 at 05:49:40PM +0530, Jindal, Sonika wrote:
> 
> 
> On 4/14/2015 5:09 AM, Matt Roper wrote:
> >On Mon, Apr 13, 2015 at 01:49:22PM +0300, Ville Syrjälä wrote:
> >>On Mon, Apr 13, 2015 at 03:53:22PM +0530, Jindal, Sonika wrote:
> >>>
> >>>
> >>>On 4/13/2015 3:40 PM, Ville Syrjälä wrote:
> >>>>On Mon, Apr 13, 2015 at 09:36:02AM +0530, Jindal, Sonika wrote:
> >>>>>
> >>>>>
> >>>>>On 4/10/2015 8:14 PM, Ville Syrjälä wrote:
> >>>>>>On Fri, Apr 10, 2015 at 04:17:17PM +0200, Daniel Vetter wrote:
> >>>>>>>On Fri, Apr 10, 2015 at 02:37:29PM +0530, Sonika Jindal wrote:
> >>>>>>>>v2: Moving creation of property in a function, checking for 90/270
> >>>>>>>>rotation simultaneously (Chris)
> >>>>>>>>Letting primary plane to be positioned
> >>>>>>>>v3: Adding if/else for 90/270 and rest params programming, adding check for
> >>>>>>>>pixel_format, some cleanup (review comments)
> >>>>>>>>v4: Adding right pixel_formats, using src_* params instead of crtc_* for offset
> >>>>>>>>and size programming (Ville)
> >>>>>>>>v5: Rebased on -nightly and Tvrtko's series for gtt remapping.
> >>>>>>>>v6: Rebased on -nightly (Tvrtko's series merged)
> >>>>>>>>v7: Moving pixel_format check to intel_atomic_plane_check (Matt)
> >>>>>>>>
> >>>>>>>>Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> >>>>>>>>Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> >>>>>>>
> >>>>>>>Patches are missing the Testcase: tag, please add that. Also, are all the
> >>>>>>>igt committed or not yet? Pulled these two in meanwhile.
> >>>>>>
> >>>>>>Things are going somewhat broken because you didn't apply my plane
> >>>>>>state stuff. Hmm. Actually it sort of looks that it might work by luck
> >>>>>>as long as the primary plane doesn't get clipped since this is bashing
> >>>>>>the user state directly into the hardware registers and not the derived
> >>>>>>state (ie. clipped coordinates).
> >>>>>>
> >>>>>I was hoping your changes get merged, but not sure why they are held up.
> >>>>>
> >>>>>>Also I see my review comment about the 90 vs. 270 rotation mixup was
> >>>>>>ignored at least.
> >>>>>>
> >>>>>I never really got the to understand the need of reversing 90 and 270 :)
> >>>>>The last discussion was not concluded, AFAIR.
> >>>>>Things look correct to me and work fine with the testcase also.
> >>>>>Is there something completely different which I am unable to get?
> >>>>
> >>>>BSpec gives me the impression the hw rotation is cw, whereas the drm one
> >>>>is ccw.
> >>>>
> >>>Yes, HW rotation is cw as per bspec. In drm, where do we consider it as
> >>>anti-cw? I assume it is cw because all the macros work as expected, ie cw.
> >>
> >>The ccw rule was inherited from XRandR. I'm not sure what macros you
> >>mean, but drm_rect_rotate() will certainly give you wrong results if
> >>you assume cw.
> >
> >It seems like this information needs to be added to the property section
> >of the DRM DocBook; continuing to follow XRandR probably makes sense if
> >that's what's agreed on, but there's no indication of that design
> >philosophy in the actual DRM documentation today, so we're in danger of
> >having different driver authors use conflicting interpretations.
> >
> Ok, I will create a DocBook patch with description for 90/270 rotation
> (Anyways 90/270 rotation is not added in the rotation property in
> documentation). Will there be any other place for this or i915's rotation
> property?
> 
> Also, I will send a patch to flip 90/270 in i915. I am just worried, that it
> will look confusing to check for DRM_ROTATE_90 and use PLANE_CTL_ROTATE_270
> for programming. I will add a comment though.

Just add a comment stating that drm is ccw (to stay compatible with
Xrandr) and i915 hw is cw. That should explain it all.

There's other places where the linux kernel and intel hw people disagree
on definitions, it just happens.
-Daniel
-- 
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

[PATCH 1/2] drm/i915: Swapping 90 and 270 to be compliant with Xrandr

[Sonika Jindal]

Since DRM_ROTATE is counter clockwise (which is compliant with Xrandr),
and HW rotation is clockwise, swapping 90/270 to work as expected from
userspace.

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    8 ++++++--
 drivers/gpu/drm/i915/intel_sprite.c  |    8 ++++++--
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 97922fb..0d7da3f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3039,8 +3039,12 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
 	plane = crtc->primary;
 	rotation = plane->state->rotation;
 	switch (rotation) {
+	/*
+	 * DRM_ROTATE_ is counter clockwise to stay compatible with Xrandr
+	 * while i915 HW rotation is clockwise, thats why this swapping.
+	 */
 	case BIT(DRM_ROTATE_90):
-		plane_ctl |= PLANE_CTL_ROTATE_90;
+		plane_ctl |= PLANE_CTL_ROTATE_270;
 		break;
 
 	case BIT(DRM_ROTATE_180):
@@ -3048,7 +3052,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
 		break;
 
 	case BIT(DRM_ROTATE_270):
-		plane_ctl |= PLANE_CTL_ROTATE_270;
+		plane_ctl |= PLANE_CTL_ROTATE_90;
 		break;
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index e3d41c0..765095e 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -259,8 +259,12 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 
 	rotation = drm_plane->state->rotation;
 	switch (rotation) {
+	/*
+	 * DRM_ROTATE_ is counter clockwise to stay compatible with Xrandr
+	 * while i915 HW rotation is clockwise, thats why this swapping.
+	 */
 	case BIT(DRM_ROTATE_90):
-		plane_ctl |= PLANE_CTL_ROTATE_90;
+		plane_ctl |= PLANE_CTL_ROTATE_270;
 		break;
 
 	case BIT(DRM_ROTATE_180):
@@ -268,7 +272,7 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 		break;
 
 	case BIT(DRM_ROTATE_270):
-		plane_ctl |= PLANE_CTL_ROTATE_270;
+		plane_ctl |= PLANE_CTL_ROTATE_90;
 		break;
 	}
 
-- 
1.7.10.4

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

[PATCH 2/2] Documentation/drm: Update rotation property with 90/270 and description

[Sonika Jindal]

Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
---
 Documentation/DocBook/drm.tmpl |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index f4976cd..266d50a 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -2853,9 +2853,12 @@ void intel_crt_init(struct drm_device *dev)
 	<td rowspan="1" valign="top" >Plane</td>
 	<td valign="top" >“rotation”</td>
 	<td valign="top" >BITMASK</td>
-	<td valign="top" >{ 0, "rotate-0" }, { 2, "rotate-180" }</td>
+	<td valign="top" >{ 0, "rotate-0" }, { 1, "rotate-90" },
+				{ 2, "rotate-180" }, { 3, "rotate-270" }</td>
 	<td valign="top" >Plane</td>
-	<td valign="top" >TBD</td>
+	<td valign="top" >To set plane HW rotation. This rotation property does
+		the plane rotation in counter clockwise direction which is
+		inline with the way XRandr works.</td>
 	</tr>
 	<tr>
 	<td rowspan="17" valign="top" >SDVO-TV</td>
-- 
1.7.10.4

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

Re: [PATCH 2/2] Documentation/drm: Update rotation property with 90/270 and description

[Daniel Vetter]

On Wed, Apr 15, 2015 at 03:35:08PM +0530, Sonika Jindal wrote:
> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> ---
>  Documentation/DocBook/drm.tmpl |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index f4976cd..266d50a 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -2853,9 +2853,12 @@ void intel_crt_init(struct drm_device *dev)
>  	<td rowspan="1" valign="top" >Plane</td>
>  	<td valign="top" >“rotation”</td>
>  	<td valign="top" >BITMASK</td>
> -	<td valign="top" >{ 0, "rotate-0" }, { 2, "rotate-180" }</td>
> +	<td valign="top" >{ 0, "rotate-0" }, { 1, "rotate-90" },
> +				{ 2, "rotate-180" }, { 3, "rotate-270" }</td>
>  	<td valign="top" >Plane</td>
> -	<td valign="top" >TBD</td>
> +	<td valign="top" >To set plane HW rotation. This rotation property does
> +		the plane rotation in counter clockwise direction which is
> +		inline with the way XRandr works.</td>

Since this touches shared code can you please resend this patch with
dri-devel added to cc? BKM is to add a Cc: dri-devel ... line to the sob
section of the patch, then git send-email will automatically pick it up.
-Daniel

>  	</tr>
>  	<tr>
>  	<td rowspan="17" valign="top" >SDVO-TV</td>
> -- 
> 1.7.10.4
> 
> _______________________________________________
> 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

Re: [PATCH 2/2] Documentation/drm: Update rotation property with 90/270 and description

[Jindal, Sonika]

On 4/15/2015 3:57 PM, Daniel Vetter wrote:
> On Wed, Apr 15, 2015 at 03:35:08PM +0530, Sonika Jindal wrote:
>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
>> ---
>>   Documentation/DocBook/drm.tmpl |    7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
>> index f4976cd..266d50a 100644
>> --- a/Documentation/DocBook/drm.tmpl
>> +++ b/Documentation/DocBook/drm.tmpl
>> @@ -2853,9 +2853,12 @@ void intel_crt_init(struct drm_device *dev)
>>   	<td rowspan="1" valign="top" >Plane</td>
>>   	<td valign="top" >“rotation”</td>
>>   	<td valign="top" >BITMASK</td>
>> -	<td valign="top" >{ 0, "rotate-0" }, { 2, "rotate-180" }</td>
>> +	<td valign="top" >{ 0, "rotate-0" }, { 1, "rotate-90" },
>> +				{ 2, "rotate-180" }, { 3, "rotate-270" }</td>
>>   	<td valign="top" >Plane</td>
>> -	<td valign="top" >TBD</td>
>> +	<td valign="top" >To set plane HW rotation. This rotation property does
>> +		the plane rotation in counter clockwise direction which is
>> +		inline with the way XRandr works.</td>
>
> Since this touches shared code can you please resend this patch with
> dri-devel added to cc? BKM is to add a Cc: dri-devel ... line to the sob
> section of the patch, then git send-email will automatically pick it up.
> -Daniel
>
But I am changing the description for only the rotation property for i915.
>>   	</tr>
>>   	<tr>
>>   	<td rowspan="17" valign="top" >SDVO-TV</td>
>> --
>> 1.7.10.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] Documentation/drm: Update rotation property with 90/270 and description

[Daniel Vetter]

On Wed, Apr 15, 2015 at 03:59:51PM +0530, Jindal, Sonika wrote:
> 
> 
> On 4/15/2015 3:57 PM, Daniel Vetter wrote:
> >On Wed, Apr 15, 2015 at 03:35:08PM +0530, Sonika Jindal wrote:
> >>Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> >>---
> >>  Documentation/DocBook/drm.tmpl |    7 +++++--
> >>  1 file changed, 5 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> >>index f4976cd..266d50a 100644
> >>--- a/Documentation/DocBook/drm.tmpl
> >>+++ b/Documentation/DocBook/drm.tmpl
> >>@@ -2853,9 +2853,12 @@ void intel_crt_init(struct drm_device *dev)
> >>  	<td rowspan="1" valign="top" >Plane</td>
> >>  	<td valign="top" >“rotation”</td>
> >>  	<td valign="top" >BITMASK</td>
> >>-	<td valign="top" >{ 0, "rotate-0" }, { 2, "rotate-180" }</td>
> >>+	<td valign="top" >{ 0, "rotate-0" }, { 1, "rotate-90" },
> >>+				{ 2, "rotate-180" }, { 3, "rotate-270" }</td>
> >>  	<td valign="top" >Plane</td>
> >>-	<td valign="top" >TBD</td>
> >>+	<td valign="top" >To set plane HW rotation. This rotation property does
> >>+		the plane rotation in counter clockwise direction which is
> >>+		inline with the way XRandr works.</td>
> >
> >Since this touches shared code can you please resend this patch with
> >dri-devel added to cc? BKM is to add a Cc: dri-devel ... line to the sob
> >section of the patch, then git send-email will automatically pick it up.
> >-Daniel
> >
> But I am changing the description for only the rotation property for i915.

Property names are supposed to be somewhat standardized across all
drivers, and omapdrm already supports rotation. This is way we have this
shared table, so that other driver authors know what's going on. Please
resend.
-Daniel
-- 
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

[PATCH] Documentation/drm: Update rotation property with 90/270 and description

[Sonika Jindal]

Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
---
 Documentation/DocBook/drm.tmpl |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index f4976cd..266d50a 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -2853,9 +2853,12 @@ void intel_crt_init(struct drm_device *dev)
 	<td rowspan="1" valign="top" >Plane</td>
 	<td valign="top" >“rotation”</td>
 	<td valign="top" >BITMASK</td>
-	<td valign="top" >{ 0, "rotate-0" }, { 2, "rotate-180" }</td>
+	<td valign="top" >{ 0, "rotate-0" }, { 1, "rotate-90" },
+				{ 2, "rotate-180" }, { 3, "rotate-270" }</td>
 	<td valign="top" >Plane</td>
-	<td valign="top" >TBD</td>
+	<td valign="top" >To set plane HW rotation. This rotation property does
+		the plane rotation in counter clockwise direction which is
+		inline with the way XRandr works.</td>
 	</tr>
 	<tr>
 	<td rowspan="17" valign="top" >SDVO-TV</td>
-- 
1.7.10.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] Documentation/drm: Update rotation property with 90/270 and description

[Ville Syrjälä]

On Wed, Apr 15, 2015 at 04:05:19PM +0530, Sonika Jindal wrote:
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> ---
>  Documentation/DocBook/drm.tmpl |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index f4976cd..266d50a 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -2853,9 +2853,12 @@ void intel_crt_init(struct drm_device *dev)
>  	<td rowspan="1" valign="top" >Plane</td>
>  	<td valign="top" >“rotation”</td>
>  	<td valign="top" >BITMASK</td>
> -	<td valign="top" >{ 0, "rotate-0" }, { 2, "rotate-180" }</td>
> +	<td valign="top" >{ 0, "rotate-0" }, { 1, "rotate-90" },
> +				{ 2, "rotate-180" }, { 3, "rotate-270" }</td>
>  	<td valign="top" >Plane</td>
> -	<td valign="top" >TBD</td>
> +	<td valign="top" >To set plane HW rotation. This rotation property does
> +		the plane rotation in counter clockwise direction which is
> +		inline with the way XRandr works.</td>

I would suggest moving the thing to the generci props section since we
have omap and i915 both supporting it.

As for the description, we should also document the reflect flags.

I might write it as something like this:
"rotate-0,rotate-90,rotate-180,rotate-270 rotate the image by the
specified amount in degrees in a counter clockwise direction.
reflect-x,reflect-y reflect the image along the specified axis,
prior to rotation."

>  	</tr>
>  	<tr>
>  	<td rowspan="17" valign="top" >SDVO-TV</td>
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [Intel-gfx] [PATCH] Documentation/drm: Update rotation property with 90/270 and description

[Jindal, Sonika]

On 5/12/2015 6:20 PM, Ville Syrjälä wrote:
> On Wed, Apr 15, 2015 at 04:05:19PM +0530, Sonika Jindal wrote:
>> Cc: dri-devel@lists.freedesktop.org
>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
>> ---
>>   Documentation/DocBook/drm.tmpl |    7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
>> index f4976cd..266d50a 100644
>> --- a/Documentation/DocBook/drm.tmpl
>> +++ b/Documentation/DocBook/drm.tmpl
>> @@ -2853,9 +2853,12 @@ void intel_crt_init(struct drm_device *dev)
>>   	<td rowspan="1" valign="top" >Plane</td>
>>   	<td valign="top" >“rotation”</td>
>>   	<td valign="top" >BITMASK</td>
>> -	<td valign="top" >{ 0, "rotate-0" }, { 2, "rotate-180" }</td>
>> +	<td valign="top" >{ 0, "rotate-0" }, { 1, "rotate-90" },
>> +				{ 2, "rotate-180" }, { 3, "rotate-270" }</td>
>>   	<td valign="top" >Plane</td>
>> -	<td valign="top" >TBD</td>
>> +	<td valign="top" >To set plane HW rotation. This rotation property does
>> +		the plane rotation in counter clockwise direction which is
>> +		inline with the way XRandr works.</td>
>
> I would suggest moving the thing to the generci props section since we
> have omap and i915 both supporting it.
You mean in DRM properties section?
Right now, OMAP section also has rotation property. I will remove it 
from OMAP section as well if you think drm is the better place.

>
> As for the description, we should also document the reflect flags.
>
> I might write it as something like this:
> "rotate-0,rotate-90,rotate-180,rotate-270 rotate the image by the
> specified amount in degrees in a counter clockwise direction.
> reflect-x,reflect-y reflect the image along the specified axis,
> prior to rotation."
>
>>   	</tr>
>>   	<tr>
>>   	<td rowspan="17" valign="top" >SDVO-TV</td>
>> --
>> 1.7.10.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] Documentation/drm: Update rotation property with 90/270 and description

[Jindal, Sonika]

On 5/13/2015 9:57 AM, Jindal, Sonika wrote:
>
>
> On 5/12/2015 6:20 PM, Ville Syrjälä wrote:
>> On Wed, Apr 15, 2015 at 04:05:19PM +0530, Sonika Jindal wrote:
>>> Cc: dri-devel@lists.freedesktop.org
>>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
>>> ---
>>>   Documentation/DocBook/drm.tmpl |    7 +++++--
>>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/DocBook/drm.tmpl
>>> b/Documentation/DocBook/drm.tmpl
>>> index f4976cd..266d50a 100644
>>> --- a/Documentation/DocBook/drm.tmpl
>>> +++ b/Documentation/DocBook/drm.tmpl
>>> @@ -2853,9 +2853,12 @@ void intel_crt_init(struct drm_device *dev)
>>>       <td rowspan="1" valign="top" >Plane</td>
>>>       <td valign="top" >“rotation”</td>
>>>       <td valign="top" >BITMASK</td>
>>> -    <td valign="top" >{ 0, "rotate-0" }, { 2, "rotate-180" }</td>
>>> +    <td valign="top" >{ 0, "rotate-0" }, { 1, "rotate-90" },
>>> +                { 2, "rotate-180" }, { 3, "rotate-270" }</td>
>>>       <td valign="top" >Plane</td>
>>> -    <td valign="top" >TBD</td>
>>> +    <td valign="top" >To set plane HW rotation. This rotation
>>> property does
>>> +        the plane rotation in counter clockwise direction which is
>>> +        inline with the way XRandr works.</td>
>>
>> I would suggest moving the thing to the generci props section since we
>> have omap and i915 both supporting it.
> You mean in DRM properties section?
> Right now, OMAP section also has rotation property. I will remove it
> from OMAP section as well if you think drm is the better place.
>
>>
>> As for the description, we should also document the reflect flags.
>>
Also, i915 doesn't support reflect flags. Only rotation is supported there.
For the "generic" part, you meant just moving to the generic group in 
i915 property section?
>> I might write it as something like this:
>> "rotate-0,rotate-90,rotate-180,rotate-270 rotate the image by the
>> specified amount in degrees in a counter clockwise direction.
>> reflect-x,reflect-y reflect the image along the specified axis,
>> prior to rotation."
>>
>>>       </tr>
>>>       <tr>
>>>       <td rowspan="17" valign="top" >SDVO-TV</td>
>>> --
>>> 1.7.10.4
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] Documentation/drm: Update rotation property with 90/270 and description

[Jindal, Sonika]

On 5/13/2015 9:57 AM, Jindal, Sonika wrote:
>
>
> On 5/12/2015 6:20 PM, Ville Syrjälä wrote:
>> On Wed, Apr 15, 2015 at 04:05:19PM +0530, Sonika Jindal wrote:
>>> Cc: dri-devel@lists.freedesktop.org
>>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
>>> ---
>>>   Documentation/DocBook/drm.tmpl |    7 +++++--
>>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/DocBook/drm.tmpl
>>> b/Documentation/DocBook/drm.tmpl
>>> index f4976cd..266d50a 100644
>>> --- a/Documentation/DocBook/drm.tmpl
>>> +++ b/Documentation/DocBook/drm.tmpl
>>> @@ -2853,9 +2853,12 @@ void intel_crt_init(struct drm_device *dev)
>>>       <td rowspan="1" valign="top" >Plane</td>
>>>       <td valign="top" >“rotation”</td>
>>>       <td valign="top" >BITMASK</td>
>>> -    <td valign="top" >{ 0, "rotate-0" }, { 2, "rotate-180" }</td>
>>> +    <td valign="top" >{ 0, "rotate-0" }, { 1, "rotate-90" },
>>> +                { 2, "rotate-180" }, { 3, "rotate-270" }</td>
>>>       <td valign="top" >Plane</td>
>>> -    <td valign="top" >TBD</td>
>>> +    <td valign="top" >To set plane HW rotation. This rotation
>>> property does
>>> +        the plane rotation in counter clockwise direction which is
>>> +        inline with the way XRandr works.</td>
>>
>> I would suggest moving the thing to the generci props section since we
>> have omap and i915 both supporting it.
> You mean in DRM properties section?
> Right now, OMAP section also has rotation property. I will remove it
> from OMAP section as well if you think drm is the better place.
>
>>
>> As for the description, we should also document the reflect flags.
>>
i915 doesn't support reflect flags. It only create rotation property 
with rotation flags.

For "generic" section, you mean moving to generic group of properties in 
i915 only right?
>> I might write it as something like this:
>> "rotate-0,rotate-90,rotate-180,rotate-270 rotate the image by the
>> specified amount in degrees in a counter clockwise direction.
>> reflect-x,reflect-y reflect the image along the specified axis,
>> prior to rotation."
>>
>>>       </tr>
>>>       <tr>
>>>       <td rowspan="17" valign="top" >SDVO-TV</td>
>>> --
>>> 1.7.10.4
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] Documentation/drm: Update rotation property with 90/270 and description

[Ville Syrjälä]

On Wed, May 20, 2015 at 01:19:34PM +0530, Jindal, Sonika wrote:
> 
> 
> On 5/13/2015 9:57 AM, Jindal, Sonika wrote:
> >
> >
> > On 5/12/2015 6:20 PM, Ville Syrjälä wrote:
> >> On Wed, Apr 15, 2015 at 04:05:19PM +0530, Sonika Jindal wrote:
> >>> Cc: dri-devel@lists.freedesktop.org
> >>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> >>> ---
> >>>   Documentation/DocBook/drm.tmpl |    7 +++++--
> >>>   1 file changed, 5 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/Documentation/DocBook/drm.tmpl
> >>> b/Documentation/DocBook/drm.tmpl
> >>> index f4976cd..266d50a 100644
> >>> --- a/Documentation/DocBook/drm.tmpl
> >>> +++ b/Documentation/DocBook/drm.tmpl
> >>> @@ -2853,9 +2853,12 @@ void intel_crt_init(struct drm_device *dev)
> >>>       <td rowspan="1" valign="top" >Plane</td>
> >>>       <td valign="top" >“rotation”</td>
> >>>       <td valign="top" >BITMASK</td>
> >>> -    <td valign="top" >{ 0, "rotate-0" }, { 2, "rotate-180" }</td>
> >>> +    <td valign="top" >{ 0, "rotate-0" }, { 1, "rotate-90" },
> >>> +                { 2, "rotate-180" }, { 3, "rotate-270" }</td>
> >>>       <td valign="top" >Plane</td>
> >>> -    <td valign="top" >TBD</td>
> >>> +    <td valign="top" >To set plane HW rotation. This rotation
> >>> property does
> >>> +        the plane rotation in counter clockwise direction which is
> >>> +        inline with the way XRandr works.</td>
> >>
> >> I would suggest moving the thing to the generci props section since we
> >> have omap and i915 both supporting it.
> > You mean in DRM properties section?
> > Right now, OMAP section also has rotation property. I will remove it
> > from OMAP section as well if you think drm is the better place.
> >
> >>
> >> As for the description, we should also document the reflect flags.
> >>
> i915 doesn't support reflect flags.

It will as soon as I (or someone else) find a bit of time to implement
it.

> It only create rotation property 
> with rotation flags.
> 
> For "generic" section, you mean moving to generic group of properties in 
> i915 only right?

I meant the entire thing. I don't think we should care too much about
which actual bits each driver supports since that can even depend on
which plane is used in some specific platform. So we should just
document all the possible bits, and if you want you could add a note
stating that a specific driver/platform/plane may not support all of
them.

> >> I might write it as something like this:
> >> "rotate-0,rotate-90,rotate-180,rotate-270 rotate the image by the
> >> specified amount in degrees in a counter clockwise direction.
> >> reflect-x,reflect-y reflect the image along the specified axis,
> >> prior to rotation."
> >>
> >>>       </tr>
> >>>       <tr>
> >>>       <td rowspan="17" valign="top" >SDVO-TV</td>
> >>> --
> >>> 1.7.10.4
> >>>
> >>> _______________________________________________
> >>> 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

[PATCH] Documentation/drm: Update rotation property

[Sonika Jindal]

Moving rotation property to "Drm" and removing from i915 and omap.
Also, adding description to the property

Cc: DRI Development <dri-devel@lists.freedesktop.org>
Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
---
 Documentation/DocBook/drm.tmpl |   41 ++++++++++++++++++----------------------
 1 file changed, 18 insertions(+), 23 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 109fde8..c0312cb 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -2585,7 +2585,22 @@ void intel_crt_init(struct drm_device *dev)
 	<td valign="top" >Description/Restrictions</td>
 	</tr>
 	<tr>
-	<td rowspan="36" valign="top" >DRM</td>
+	<td rowspan="37" valign="top" >DRM</td>
+	<td valign="top" >Generic</td>
+	<td valign="top" >“rotation”</td>
+	<td valign="top" >BITMASK</td>
+	<td valign="top" >{ 0, "rotate-0" },
+	{ 1, "rotate-90" },
+	{ 2, "rotate-180" },
+	{ 3, "rotate-270" },
+	{ 4, "reflect-x" },
+	{ 5, "reflect-y" }</td>
+	<td valign="top" >CRTC, Plane</td>
+	<td valign="top" >rotate-(degrees) rotates the image by the specified amount in degrees
+	in counter clockwise direction. reflect-x and reflect-y reflects the
+	image along the specified axis prior to rotation</td>
+	</tr>
+	<tr>
 	<td rowspan="5" valign="top" >Connector</td>
 	<td valign="top" >“EDID”</td>
 	<td valign="top" >BLOB | IMMUTABLE</td>
@@ -2846,7 +2861,7 @@ void intel_crt_init(struct drm_device *dev)
 	<td valign="top" >TBD</td>
 	</tr>
 	<tr>
-	<td rowspan="21" valign="top" >i915</td>
+	<td rowspan="20" valign="top" >i915</td>
 	<td rowspan="2" valign="top" >Generic</td>
 	<td valign="top" >"Broadcast RGB"</td>
 	<td valign="top" >ENUM</td>
@@ -2862,14 +2877,6 @@ void intel_crt_init(struct drm_device *dev)
 	<td valign="top" >TBD</td>
 	</tr>
 	<tr>
-	<td rowspan="1" valign="top" >Plane</td>
-	<td valign="top" >“rotation”</td>
-	<td valign="top" >BITMASK</td>
-	<td valign="top" >{ 0, "rotate-0" }, { 2, "rotate-180" }</td>
-	<td valign="top" >Plane</td>
-	<td valign="top" >TBD</td>
-	</tr>
-	<tr>
 	<td rowspan="17" valign="top" >SDVO-TV</td>
 	<td valign="top" >“mode”</td>
 	<td valign="top" >ENUM</td>
@@ -3377,19 +3384,7 @@ void intel_crt_init(struct drm_device *dev)
 	</tr>
 	<tr>
 	<td rowspan="2" valign="top" >omap</td>
-	<td rowspan="2" valign="top" >Generic</td>
-	<td valign="top" >“rotation”</td>
-	<td valign="top" >BITMASK</td>
-	<td valign="top" >{ 0, "rotate-0" },
-	{ 1, "rotate-90" },
-	{ 2, "rotate-180" },
-	{ 3, "rotate-270" },
-	{ 4, "reflect-x" },
-	{ 5, "reflect-y" }</td>
-	<td valign="top" >CRTC, Plane</td>
-	<td valign="top" >TBD</td>
-	</tr>
-	<tr>
+	<td valign="top" >Generic</td>
 	<td valign="top" >“zorder”</td>
 	<td valign="top" >RANGE</td>
 	<td valign="top" >Min=0, Max=3</td>
-- 
1.7.10.4

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

Re: [Intel-gfx] [PATCH] Documentation/drm: Update rotation property

[Daniel Vetter]

On Thu, May 28, 2015 at 04:35:07PM +0530, Sonika Jindal wrote:
> Moving rotation property to "Drm" and removing from i915 and omap.
> Also, adding description to the property
> 
> Cc: DRI Development <dri-devel@lists.freedesktop.org>
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>

Applied to drm-misc, thanks.
-Daniel

> ---
>  Documentation/DocBook/drm.tmpl |   41 ++++++++++++++++++----------------------
>  1 file changed, 18 insertions(+), 23 deletions(-)
> 
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index 109fde8..c0312cb 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -2585,7 +2585,22 @@ void intel_crt_init(struct drm_device *dev)
>  	<td valign="top" >Description/Restrictions</td>
>  	</tr>
>  	<tr>
> -	<td rowspan="36" valign="top" >DRM</td>
> +	<td rowspan="37" valign="top" >DRM</td>
> +	<td valign="top" >Generic</td>
> +	<td valign="top" >“rotation”</td>
> +	<td valign="top" >BITMASK</td>
> +	<td valign="top" >{ 0, "rotate-0" },
> +	{ 1, "rotate-90" },
> +	{ 2, "rotate-180" },
> +	{ 3, "rotate-270" },
> +	{ 4, "reflect-x" },
> +	{ 5, "reflect-y" }</td>
> +	<td valign="top" >CRTC, Plane</td>
> +	<td valign="top" >rotate-(degrees) rotates the image by the specified amount in degrees
> +	in counter clockwise direction. reflect-x and reflect-y reflects the
> +	image along the specified axis prior to rotation</td>
> +	</tr>
> +	<tr>
>  	<td rowspan="5" valign="top" >Connector</td>
>  	<td valign="top" >“EDID”</td>
>  	<td valign="top" >BLOB | IMMUTABLE</td>
> @@ -2846,7 +2861,7 @@ void intel_crt_init(struct drm_device *dev)
>  	<td valign="top" >TBD</td>
>  	</tr>
>  	<tr>
> -	<td rowspan="21" valign="top" >i915</td>
> +	<td rowspan="20" valign="top" >i915</td>
>  	<td rowspan="2" valign="top" >Generic</td>
>  	<td valign="top" >"Broadcast RGB"</td>
>  	<td valign="top" >ENUM</td>
> @@ -2862,14 +2877,6 @@ void intel_crt_init(struct drm_device *dev)
>  	<td valign="top" >TBD</td>
>  	</tr>
>  	<tr>
> -	<td rowspan="1" valign="top" >Plane</td>
> -	<td valign="top" >“rotation”</td>
> -	<td valign="top" >BITMASK</td>
> -	<td valign="top" >{ 0, "rotate-0" }, { 2, "rotate-180" }</td>
> -	<td valign="top" >Plane</td>
> -	<td valign="top" >TBD</td>
> -	</tr>
> -	<tr>
>  	<td rowspan="17" valign="top" >SDVO-TV</td>
>  	<td valign="top" >“mode”</td>
>  	<td valign="top" >ENUM</td>
> @@ -3377,19 +3384,7 @@ void intel_crt_init(struct drm_device *dev)
>  	</tr>
>  	<tr>
>  	<td rowspan="2" valign="top" >omap</td>
> -	<td rowspan="2" valign="top" >Generic</td>
> -	<td valign="top" >“rotation”</td>
> -	<td valign="top" >BITMASK</td>
> -	<td valign="top" >{ 0, "rotate-0" },
> -	{ 1, "rotate-90" },
> -	{ 2, "rotate-180" },
> -	{ 3, "rotate-270" },
> -	{ 4, "reflect-x" },
> -	{ 5, "reflect-y" }</td>
> -	<td valign="top" >CRTC, Plane</td>
> -	<td valign="top" >TBD</td>
> -	</tr>
> -	<tr>
> +	<td valign="top" >Generic</td>
>  	<td valign="top" >“zorder”</td>
>  	<td valign="top" >RANGE</td>
>  	<td valign="top" >Min=0, Max=3</td>
> -- 
> 1.7.10.4
> 
> _______________________________________________
> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/i915: Swapping 90 and 270 to be compliant with Xrandr

[Ville Syrjälä]

On Wed, Apr 15, 2015 at 03:35:07PM +0530, Sonika Jindal wrote:
> Since DRM_ROTATE is counter clockwise (which is compliant with Xrandr),
> and HW rotation is clockwise, swapping 90/270 to work as expected from
> userspace.
> 
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>

Looks like this needs a rebase, but other than that this is

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

> ---
>  drivers/gpu/drm/i915/intel_display.c |    8 ++++++--
>  drivers/gpu/drm/i915/intel_sprite.c  |    8 ++++++--
>  2 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 97922fb..0d7da3f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3039,8 +3039,12 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  	plane = crtc->primary;
>  	rotation = plane->state->rotation;
>  	switch (rotation) {
> +	/*
> +	 * DRM_ROTATE_ is counter clockwise to stay compatible with Xrandr
> +	 * while i915 HW rotation is clockwise, thats why this swapping.
> +	 */
>  	case BIT(DRM_ROTATE_90):
> -		plane_ctl |= PLANE_CTL_ROTATE_90;
> +		plane_ctl |= PLANE_CTL_ROTATE_270;
>  		break;
>  
>  	case BIT(DRM_ROTATE_180):
> @@ -3048,7 +3052,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  		break;
>  
>  	case BIT(DRM_ROTATE_270):
> -		plane_ctl |= PLANE_CTL_ROTATE_270;
> +		plane_ctl |= PLANE_CTL_ROTATE_90;
>  		break;
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index e3d41c0..765095e 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -259,8 +259,12 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  
>  	rotation = drm_plane->state->rotation;
>  	switch (rotation) {
> +	/*
> +	 * DRM_ROTATE_ is counter clockwise to stay compatible with Xrandr
> +	 * while i915 HW rotation is clockwise, thats why this swapping.
> +	 */
>  	case BIT(DRM_ROTATE_90):
> -		plane_ctl |= PLANE_CTL_ROTATE_90;
> +		plane_ctl |= PLANE_CTL_ROTATE_270;
>  		break;
>  
>  	case BIT(DRM_ROTATE_180):
> @@ -268,7 +272,7 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  		break;
>  
>  	case BIT(DRM_ROTATE_270):
> -		plane_ctl |= PLANE_CTL_ROTATE_270;
> +		plane_ctl |= PLANE_CTL_ROTATE_90;
>  		break;
>  	}
>  
> -- 
> 1.7.10.4

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

[PATCH] drm/i915: Swapping 90 and 270 to be compliant with Xrandr

[Sonika Jindal]

Since DRM_ROTATE is counter clockwise (which is compliant with Xrandr),
and HW rotation is clockwise, swapping 90/270 to work as expected from
userspace.

v2: Rebased

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9d2d6fb..a583422 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3010,12 +3010,16 @@ u32 skl_plane_ctl_rotation(unsigned int rotation)
 	switch (rotation) {
 	case BIT(DRM_ROTATE_0):
 		break;
+	/*
+	 * DRM_ROTATE_ is counter clockwise to stay compatible with Xrandr
+	 * while i915 HW rotation is clockwise, thats why this swapping.
+	 */
 	case BIT(DRM_ROTATE_90):
-		return PLANE_CTL_ROTATE_90;
+		return PLANE_CTL_ROTATE_270;
 	case BIT(DRM_ROTATE_180):
 		return PLANE_CTL_ROTATE_180;
 	case BIT(DRM_ROTATE_270):
-		return PLANE_CTL_ROTATE_270;
+		return PLANE_CTL_ROTATE_90;
 	default:
 		MISSING_CASE(rotation);
 	}
-- 
1.7.10.4

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

Re: [PATCH] drm/i915: Swapping 90 and 270 to be compliant with Xrandr

[Daniel Vetter]

On Wed, May 20, 2015 at 01:40:48PM +0530, Sonika Jindal wrote:
> Since DRM_ROTATE is counter clockwise (which is compliant with Xrandr),
> and HW rotation is clockwise, swapping 90/270 to work as expected from
> userspace.
> 
> v2: Rebased
> 
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I've fixed the summary to make it clear this is skl-only and applied the
patch to dinq, thanks.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c |    8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9d2d6fb..a583422 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3010,12 +3010,16 @@ u32 skl_plane_ctl_rotation(unsigned int rotation)
>  	switch (rotation) {
>  	case BIT(DRM_ROTATE_0):
>  		break;
> +	/*
> +	 * DRM_ROTATE_ is counter clockwise to stay compatible with Xrandr
> +	 * while i915 HW rotation is clockwise, thats why this swapping.
> +	 */
>  	case BIT(DRM_ROTATE_90):
> -		return PLANE_CTL_ROTATE_90;
> +		return PLANE_CTL_ROTATE_270;
>  	case BIT(DRM_ROTATE_180):
>  		return PLANE_CTL_ROTATE_180;
>  	case BIT(DRM_ROTATE_270):
> -		return PLANE_CTL_ROTATE_270;
> +		return PLANE_CTL_ROTATE_90;
>  	default:
>  		MISSING_CASE(rotation);
>  	}
> -- 
> 1.7.10.4
> 
> _______________________________________________
> 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

Re: [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation

[Damien Lespiau]

On Mon, Apr 13, 2015 at 03:53:22PM +0530, Jindal, Sonika wrote:
> >>I never really got the to understand the need of reversing 90 and 270 :)
> >>The last discussion was not concluded, AFAIR.
> >>Things look correct to me and work fine with the testcase also.
> >>Is there something completely different which I am unable to get?
> >
> >BSpec gives me the impression the hw rotation is cw, whereas the drm one
> >is ccw.
> >
> Yes, HW rotation is cw as per bspec. In drm, where do we consider it as
> anti-cw? I assume it is cw because all the macros work as expected, ie cw.

The DRM is quite a direct implementation of the X semantics:

http://cgit.freedesktop.org/xorg/proto/randrproto/tree/randrproto.txt

"In Randr, the coordinate system is rotated in a counter-clockwise direction
relative to the normal orientation"

This matches the usual orientation of the trigonometric circle.

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

Re: [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation

[Jindal, Sonika]

On 4/10/2015 7:47 PM, Daniel Vetter wrote:
> On Fri, Apr 10, 2015 at 02:37:29PM +0530, Sonika Jindal wrote:
>> v2: Moving creation of property in a function, checking for 90/270
>> rotation simultaneously (Chris)
>> Letting primary plane to be positioned
>> v3: Adding if/else for 90/270 and rest params programming, adding check for
>> pixel_format, some cleanup (review comments)
>> v4: Adding right pixel_formats, using src_* params instead of crtc_* for offset
>> and size programming (Ville)
>> v5: Rebased on -nightly and Tvrtko's series for gtt remapping.
>> v6: Rebased on -nightly (Tvrtko's series merged)
>> v7: Moving pixel_format check to intel_atomic_plane_check (Matt)
>>
>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
>> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
>
> Patches are missing the Testcase: tag, please add that. Also, are all the
> igt committed or not yet? Pulled these two in meanwhile.
> -Daniel
The kms_rotation_crc is updated for 90/270 tests and is posted on the 
mailing list. Not sure if an r-b is a must for tests too?
-Sonika
>
>> ---
>>   drivers/gpu/drm/i915/i915_reg.h           |    2 +
>>   drivers/gpu/drm/i915/intel_atomic_plane.c |   24 ++++++++
>>   drivers/gpu/drm/i915/intel_display.c      |   88 ++++++++++++++++++++---------
>>   drivers/gpu/drm/i915/intel_drv.h          |    6 ++
>>   drivers/gpu/drm/i915/intel_sprite.c       |   52 ++++++++++++-----
>>   5 files changed, 131 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index b522eb6..564bbd5 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -4854,7 +4854,9 @@ enum skl_disp_power_wells {
>>   #define   PLANE_CTL_ALPHA_HW_PREMULTIPLY	(  3 << 4)
>>   #define   PLANE_CTL_ROTATE_MASK			0x3
>>   #define   PLANE_CTL_ROTATE_0			0x0
>> +#define   PLANE_CTL_ROTATE_90			0x1
>>   #define   PLANE_CTL_ROTATE_180			0x2
>> +#define   PLANE_CTL_ROTATE_270			0x3
>>   #define _PLANE_STRIDE_1_A			0x70188
>>   #define _PLANE_STRIDE_2_A			0x70288
>>   #define _PLANE_STRIDE_3_A			0x70388
>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
>> index 976b891..a27ee8c 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
>> @@ -162,6 +162,30 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>>   			(1 << drm_plane_index(plane));
>>   	}
>>
>> +	if (state->fb && intel_rotation_90_or_270(state->rotation)) {
>> +		if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
>> +			state->fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
>> +			DRM_DEBUG_KMS("Y/Yf tiling required for 90/270!\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +		/*
>> +		 * 90/270 is not allowed with RGB64 16:16:16:16,
>> +		 * RGB 16-bit 5:6:5, and Indexed 8-bit.
>> +		 * TBD: Add RGB64 case once its added in supported format list.
>> +		 */
>> +		switch (state->fb->pixel_format) {
>> +		case DRM_FORMAT_C8:
>> +		case DRM_FORMAT_RGB565:
>> +			DRM_DEBUG_KMS("Unsupported pixel format %s for 90/270!\n",
>> +					drm_get_format_name(state->fb->pixel_format));
>> +			return -EINVAL;
>> +
>> +		default:
>> +			break;
>> +		}
>> +	}
>> +
>>   	return intel_plane->check_plane(plane, intel_state);
>>   }
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index f0bbc22..4de544c 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -2311,13 +2311,6 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb,
>>   	info->pitch = fb->pitches[0];
>>   	info->fb_modifier = fb->modifier[0];
>>
>> -	if (!(info->fb_modifier == I915_FORMAT_MOD_Y_TILED ||
>> -	      info->fb_modifier == I915_FORMAT_MOD_Yf_TILED)) {
>> -		DRM_DEBUG_KMS(
>> -			      "Y or Yf tiling is needed for 90/270 rotation!\n");
>> -		return -EINVAL;
>> -	}
>> -
>>   	return 0;
>>   }
>>
>> @@ -2919,8 +2912,12 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>   	struct drm_i915_gem_object *obj;
>>   	int pipe = intel_crtc->pipe;
>> -	u32 plane_ctl, stride_div;
>> +	u32 plane_ctl, stride_div, stride;
>> +	u32 tile_height, plane_offset, plane_size;
>> +	unsigned int rotation;
>> +	int x_offset, y_offset;
>>   	unsigned long surf_addr;
>> +	struct drm_plane *plane;
>>
>>   	if (!intel_crtc->primary_enabled) {
>>   		I915_WRITE(PLANE_CTL(pipe, 0), 0);
>> @@ -2981,21 +2978,51 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>>   	}
>>
>>   	plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
>> -	if (crtc->primary->state->rotation == BIT(DRM_ROTATE_180))
>> +
>> +	plane = crtc->primary;
>> +	rotation = plane->state->rotation;
>> +	switch (rotation) {
>> +	case BIT(DRM_ROTATE_90):
>> +		plane_ctl |= PLANE_CTL_ROTATE_90;
>> +		break;
>> +
>> +	case BIT(DRM_ROTATE_180):
>>   		plane_ctl |= PLANE_CTL_ROTATE_180;
>> +		break;
>> +
>> +	case BIT(DRM_ROTATE_270):
>> +		plane_ctl |= PLANE_CTL_ROTATE_270;
>> +		break;
>> +	}
>>
>>   	obj = intel_fb_obj(fb);
>>   	stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
>>   					       fb->pixel_format);
>> -	surf_addr = intel_plane_obj_offset(to_intel_plane(crtc->primary), obj);
>> +	surf_addr = intel_plane_obj_offset(to_intel_plane(plane), obj);
>> +
>> +	if (intel_rotation_90_or_270(rotation)) {
>> +		/* stride = Surface height in tiles */
>> +		tile_height = intel_tile_height(dev, fb->bits_per_pixel,
>> +						fb->modifier[0]);
>> +		stride = DIV_ROUND_UP(fb->height, tile_height);
>> +		x_offset = stride * tile_height - y - (plane->state->src_h >> 16);
>> +		y_offset = x;
>> +		plane_size = ((plane->state->src_w >> 16) - 1) << 16 |
>> +					((plane->state->src_h >> 16) - 1);
>> +	} else {
>> +		stride = fb->pitches[0] / stride_div;
>> +		x_offset = x;
>> +		y_offset = y;
>> +		plane_size = ((plane->state->src_h >> 16) - 1) << 16 |
>> +			((plane->state->src_w >> 16) - 1);
>> +	}
>> +	plane_offset = y_offset << 16 | x_offset;
>>
>>   	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
>>   	I915_WRITE(PLANE_POS(pipe, 0), 0);
>> -	I915_WRITE(PLANE_OFFSET(pipe, 0), (y << 16) | x);
>> -	I915_WRITE(PLANE_SIZE(pipe, 0),
>> -		   (intel_crtc->config->pipe_src_h - 1) << 16 |
>> -		   (intel_crtc->config->pipe_src_w - 1));
>> -	I915_WRITE(PLANE_STRIDE(pipe, 0), fb->pitches[0] / stride_div);
>> +	I915_WRITE(PLANE_OFFSET(pipe, 0), plane_offset);
>> +	I915_WRITE(PLANE_SIZE(pipe, 0), plane_size);
>> +	I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
>>   	I915_WRITE(PLANE_SURF(pipe, 0), surf_addr);
>>
>>   	POSTING_READ(PLANE_SURF(pipe, 0));
>> @@ -12406,23 +12433,32 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>>   				 intel_primary_formats, num_formats,
>>   				 DRM_PLANE_TYPE_PRIMARY);
>>
>> -	if (INTEL_INFO(dev)->gen >= 4) {
>> -		if (!dev->mode_config.rotation_property)
>> -			dev->mode_config.rotation_property =
>> -				drm_mode_create_rotation_property(dev,
>> -							BIT(DRM_ROTATE_0) |
>> -							BIT(DRM_ROTATE_180));
>> -		if (dev->mode_config.rotation_property)
>> -			drm_object_attach_property(&primary->base.base,
>> -				dev->mode_config.rotation_property,
>> -				state->base.rotation);
>> -	}
>> +	if (INTEL_INFO(dev)->gen >= 4)
>> +		intel_create_rotation_property(dev, primary);
>>
>>   	drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs);
>>
>>   	return &primary->base;
>>   }
>>
>> +void intel_create_rotation_property(struct drm_device *dev, struct intel_plane *plane)
>> +{
>> +	if (!dev->mode_config.rotation_property) {
>> +		unsigned long flags = BIT(DRM_ROTATE_0) |
>> +			BIT(DRM_ROTATE_180);
>> +
>> +		if (INTEL_INFO(dev)->gen >= 9)
>> +			flags |= BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270);
>> +
>> +		dev->mode_config.rotation_property =
>> +			drm_mode_create_rotation_property(dev, flags);
>> +	}
>> +	if (dev->mode_config.rotation_property)
>> +		drm_object_attach_property(&plane->base.base,
>> +				dev->mode_config.rotation_property,
>> +				plane->base.state->rotation);
>> +}
>> +
>>   static int
>>   intel_check_cursor_plane(struct drm_plane *plane,
>>   			 struct intel_plane_state *state)
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 811a1db..d32025a 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -995,6 +995,12 @@ intel_rotation_90_or_270(unsigned int rotation)
>>   	return rotation & (BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_270));
>>   }
>>
>> +unsigned int
>> +intel_tile_height(struct drm_device *dev, uint32_t bits_per_pixel,
>> +		  uint64_t fb_modifier);
>> +void intel_create_rotation_property(struct drm_device *dev,
>> +					struct intel_plane *plane);
>> +
>>   bool intel_wm_need_update(struct drm_plane *plane,
>>   			  struct drm_plane_state *state);
>>
>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>> index f41e872..83adc9b 100644
>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> @@ -190,10 +190,13 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>>   	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>>   	const int pipe = intel_plane->pipe;
>>   	const int plane = intel_plane->plane + 1;
>> -	u32 plane_ctl, stride_div;
>> +	u32 plane_ctl, stride_div, stride;
>>   	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
>>   	const struct drm_intel_sprite_colorkey *key = &intel_plane->ckey;
>>   	unsigned long surf_addr;
>> +	u32 tile_height, plane_offset, plane_size;
>> +	unsigned int rotation;
>> +	int x_offset, y_offset;
>>
>>   	plane_ctl = PLANE_CTL_ENABLE |
>>   		PLANE_CTL_PIPE_CSC_ENABLE;
>> @@ -254,8 +257,20 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>>   		MISSING_CASE(fb->modifier[0]);
>>   	}
>>
>> -	if (drm_plane->state->rotation == BIT(DRM_ROTATE_180))
>> +	rotation = drm_plane->state->rotation;
>> +	switch (rotation) {
>> +	case BIT(DRM_ROTATE_90):
>> +		plane_ctl |= PLANE_CTL_ROTATE_90;
>> +		break;
>> +
>> +	case BIT(DRM_ROTATE_180):
>>   		plane_ctl |= PLANE_CTL_ROTATE_180;
>> +		break;
>> +
>> +	case BIT(DRM_ROTATE_270):
>> +		plane_ctl |= PLANE_CTL_ROTATE_270;
>> +		break;
>> +	}
>>
>>   	intel_update_sprite_watermarks(drm_plane, crtc, src_w, src_h,
>>   				       pixel_size, true,
>> @@ -283,10 +298,26 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>>
>>   	surf_addr = intel_plane_obj_offset(intel_plane, obj);
>>
>> -	I915_WRITE(PLANE_OFFSET(pipe, plane), (y << 16) | x);
>> -	I915_WRITE(PLANE_STRIDE(pipe, plane), fb->pitches[0] / stride_div);
>> +	if (intel_rotation_90_or_270(rotation)) {
>> +		/* stride: Surface height in tiles */
>> +		tile_height = intel_tile_height(dev, fb->bits_per_pixel,
>> +							fb->modifier[0]);
>> +		stride = DIV_ROUND_UP(fb->height, tile_height);
>> +		plane_size = (src_w << 16) | src_h;
>> +		x_offset = stride * tile_height - y - (src_h + 1);
>> +		y_offset = x;
>> +	} else {
>> +		stride = fb->pitches[0] / stride_div;
>> +		plane_size = (src_h << 16) | src_w;
>> +		x_offset = x;
>> +		y_offset = y;
>> +	}
>> +	plane_offset = y_offset << 16 | x_offset;
>> +
>> +	I915_WRITE(PLANE_OFFSET(pipe, plane), plane_offset);
>> +	I915_WRITE(PLANE_STRIDE(pipe, plane), stride);
>>   	I915_WRITE(PLANE_POS(pipe, plane), (crtc_y << 16) | crtc_x);
>> -	I915_WRITE(PLANE_SIZE(pipe, plane), (crtc_h << 16) | crtc_w);
>> +	I915_WRITE(PLANE_SIZE(pipe, plane), plane_size);
>>   	I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl);
>>   	I915_WRITE(PLANE_SURF(pipe, plane), surf_addr);
>>   	POSTING_READ(PLANE_SURF(pipe, plane));
>> @@ -1310,16 +1341,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>>   		goto out;
>>   	}
>>
>> -	if (!dev->mode_config.rotation_property)
>> -		dev->mode_config.rotation_property =
>> -			drm_mode_create_rotation_property(dev,
>> -							  BIT(DRM_ROTATE_0) |
>> -							  BIT(DRM_ROTATE_180));
>> -
>> -	if (dev->mode_config.rotation_property)
>> -		drm_object_attach_property(&intel_plane->base.base,
>> -					   dev->mode_config.rotation_property,
>> -					   state->base.rotation);
>> +	intel_create_rotation_property(dev, intel_plane);
>>
>>   	drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
>>
>> --
>> 1.7.10.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6170
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  270/270              270/270
ILK                                  303/303              303/303
SNB                 -21              304/304              283/304
IVB                                  337/337              337/337
BYT                 -1              287/287              286/287
HSW                                  361/361              361/361
BDW                                  309/309              309/309
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 SNB  igt@kms_cursor_crc@cursor-size-change      NSPT(2)PASS(1)      NSPT(2)
 SNB  igt@kms_mmio_vs_cs_flip@setcrtc_vs_cs_flip      NSPT(2)PASS(1)      NSPT(2)
 SNB  igt@kms_mmio_vs_cs_flip@setplane_vs_cs_flip      NSPT(2)PASS(1)      NSPT(2)
 SNB  igt@kms_rotation_crc@primary-rotation      NSPT(2)PASS(1)      NSPT(2)
 SNB  igt@kms_rotation_crc@sprite-rotation      NSPT(2)PASS(3)      NSPT(2)
 SNB  igt@pm_rpm@cursor      NSPT(2)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@cursor-dpms      NSPT(2)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@dpms-mode-unset-non-lpsp      NSPT(2)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@dpms-non-lpsp      NSPT(1)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@drm-resources-equal      NSPT(1)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@fences      NSPT(1)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@fences-dpms      NSPT(1)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@gem-execbuf      NSPT(1)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@gem-mmap-cpu      NSPT(1)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@gem-mmap-gtt      NSPT(1)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@gem-pread      NSPT(1)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@i2c      NSPT(1)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@modeset-non-lpsp      NSPT(1)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@modeset-non-lpsp-stress-no-wait      NSPT(1)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@pci-d3-state      NSPT(1)PASS(1)      NSPT(2)
 SNB  igt@pm_rpm@rte      NSPT(1)PASS(1)      NSPT(2)
*BYT  igt@gem_exec_bad_domains@conflicting-write-domain      PASS(2)      FAIL(1)PASS(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915/skl: Allow universal planes to position

[Tvrtko Ursulin]

On 10/04/15 10:07, Sonika Jindal wrote:
> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c |    7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ceb2e61..f0bbc22 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane,
>   	struct drm_rect *dest = &state->dst;
>   	struct drm_rect *src = &state->src;
>   	const struct drm_rect *clip = &state->clip;
> +	bool can_position = false;
>   	int ret;
>
>   	crtc = crtc ? crtc : plane->crtc;
>   	intel_crtc = to_intel_crtc(crtc);
>
> +	if (INTEL_INFO(dev)->gen >= 9)
> +		can_position = true;
> +
>   	ret = drm_plane_helper_check_update(plane, crtc, fb,
>   					    src, dest, clip,
>   					    DRM_PLANE_HELPER_NO_SCALING,
>   					    DRM_PLANE_HELPER_NO_SCALING,
> -					    false, true, &state->visible);
> +					    can_position, true,
> +					    &state->visible);
>   	if (ret)
>   		return ret;
>
>

I have discovered today that, while this allows SetCrtc and SetPlane 
ioctls to work with frame buffers which do not cover the plane, page 
flips are not that lucky and fail roughly with:

[drm:drm_crtc_check_viewport] Invalid fb size 1080x1080 for CRTC 
viewport 1920x1080+0+0.

I have posted a quick IGT exerciser for this as "kms_rotation_crc: 
Excercise page flips with 90 degree rotation". May not be that great but 
shows the failure.

I am not that hot on meddling with this code, nor do I feel competent to 
even try on my own at least. :/ Maybe just because the atomic and plane 
related rewrites have been going on for so long, and have multiple 
people involved, it all sounds pretty scary and fragile.

But I think some sort of plan on how to fix this could be in order?

Regards,

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

Re: [PATCH 1/2] drm/i915/skl: Allow universal planes to position

[Matt Roper]

On Tue, Oct 06, 2015 at 02:32:47PM +0100, Tvrtko Ursulin wrote:
> 
> On 10/04/15 10:07, Sonika Jindal wrote:
> >Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> >Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> >---
> >  drivers/gpu/drm/i915/intel_display.c |    7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >index ceb2e61..f0bbc22 100644
> >--- a/drivers/gpu/drm/i915/intel_display.c
> >+++ b/drivers/gpu/drm/i915/intel_display.c
> >@@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane,
> >  	struct drm_rect *dest = &state->dst;
> >  	struct drm_rect *src = &state->src;
> >  	const struct drm_rect *clip = &state->clip;
> >+	bool can_position = false;
> >  	int ret;
> >
> >  	crtc = crtc ? crtc : plane->crtc;
> >  	intel_crtc = to_intel_crtc(crtc);
> >
> >+	if (INTEL_INFO(dev)->gen >= 9)
> >+		can_position = true;
> >+
> >  	ret = drm_plane_helper_check_update(plane, crtc, fb,
> >  					    src, dest, clip,
> >  					    DRM_PLANE_HELPER_NO_SCALING,
> >  					    DRM_PLANE_HELPER_NO_SCALING,
> >-					    false, true, &state->visible);
> >+					    can_position, true,
> >+					    &state->visible);
> >  	if (ret)
> >  		return ret;
> >
> >
> 
> I have discovered today that, while this allows SetCrtc and SetPlane
> ioctls to work with frame buffers which do not cover the plane, page
> flips are not that lucky and fail roughly with:
> 
> [drm:drm_crtc_check_viewport] Invalid fb size 1080x1080 for CRTC
> viewport 1920x1080+0+0.

Maybe I'm misunderstanding your explanation, but a framebuffer is always
required to fill/cover the plane scanning out of it.  What this patch is
supposed to be allowing is for the primary plane to not cover the entire
CRTC (since that's something that only became possible for Intel
hardware on the gen9+ platforms).  I.e., the primary plane is now
allowed to positioned and resized to cover a subset of the CRTC area,
just like "sprite" planes have always been able to.

If you've got a 1080x1080 framebuffer, then it's legal to have a
1080x1080 primary plane while running in 1920x1080 mode on SKL/BXT.
However it is not legal to size the primary plane as 1920x1080 and use
this same 1080x1080 framebuffer with any of our interfaces (setplane,
setcrtc, pageflip, or atomic).

Are you using ioctls/libdrm directly or are you using igt_kms helpers?
IIRC, the IGT helpers will try to be extra helpful and automatically
size the plane to match the framebuffer (unless you override that
behavior), so that might be what's causing the confusion here.


Matt

> 
> I have posted a quick IGT exerciser for this as "kms_rotation_crc:
> Excercise page flips with 90 degree rotation". May not be that great
> but shows the failure.
> 
> I am not that hot on meddling with this code, nor do I feel
> competent to even try on my own at least. :/ Maybe just because the
> atomic and plane related rewrites have been going on for so long,
> and have multiple people involved, it all sounds pretty scary and
> fragile.
> 
> But I think some sort of plan on how to fix this could be in order?
> 
> Regards,
> 
> Tvrtko

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

Re: [PATCH 1/2] drm/i915/skl: Allow universal planes to position

[Ville Syrjälä]

On Tue, Oct 06, 2015 at 07:29:54AM -0700, Matt Roper wrote:
> On Tue, Oct 06, 2015 at 02:32:47PM +0100, Tvrtko Ursulin wrote:
> > 
> > On 10/04/15 10:07, Sonika Jindal wrote:
> > >Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> > >Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> > >---
> > >  drivers/gpu/drm/i915/intel_display.c |    7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > >diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > >index ceb2e61..f0bbc22 100644
> > >--- a/drivers/gpu/drm/i915/intel_display.c
> > >+++ b/drivers/gpu/drm/i915/intel_display.c
> > >@@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane,
> > >  	struct drm_rect *dest = &state->dst;
> > >  	struct drm_rect *src = &state->src;
> > >  	const struct drm_rect *clip = &state->clip;
> > >+	bool can_position = false;
> > >  	int ret;
> > >
> > >  	crtc = crtc ? crtc : plane->crtc;
> > >  	intel_crtc = to_intel_crtc(crtc);
> > >
> > >+	if (INTEL_INFO(dev)->gen >= 9)
> > >+		can_position = true;
> > >+
> > >  	ret = drm_plane_helper_check_update(plane, crtc, fb,
> > >  					    src, dest, clip,
> > >  					    DRM_PLANE_HELPER_NO_SCALING,
> > >  					    DRM_PLANE_HELPER_NO_SCALING,
> > >-					    false, true, &state->visible);
> > >+					    can_position, true,
> > >+					    &state->visible);
> > >  	if (ret)
> > >  		return ret;
> > >
> > >
> > 
> > I have discovered today that, while this allows SetCrtc and SetPlane
> > ioctls to work with frame buffers which do not cover the plane, page
> > flips are not that lucky and fail roughly with:
> > 
> > [drm:drm_crtc_check_viewport] Invalid fb size 1080x1080 for CRTC
> > viewport 1920x1080+0+0.
> 
> Maybe I'm misunderstanding your explanation, but a framebuffer is always
> required to fill/cover the plane scanning out of it.  What this patch is
> supposed to be allowing is for the primary plane to not cover the entire
> CRTC (since that's something that only became possible for Intel
> hardware on the gen9+ platforms).  I.e., the primary plane is now
> allowed to positioned and resized to cover a subset of the CRTC area,
> just like "sprite" planes have always been able to.
> 
> If you've got a 1080x1080 framebuffer, then it's legal to have a
> 1080x1080 primary plane while running in 1920x1080 mode on SKL/BXT.
> However it is not legal to size the primary plane as 1920x1080 and use
> this same 1080x1080 framebuffer with any of our interfaces (setplane,
> setcrtc, pageflip, or atomic).
> 
> Are you using ioctls/libdrm directly or are you using igt_kms helpers?
> IIRC, the IGT helpers will try to be extra helpful and automatically
> size the plane to match the framebuffer (unless you override that
> behavior), so that might be what's causing the confusion here.

The problem is clear as day in drm_mode_page_flip_ioctl():
ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y, &crtc->mode, fb);
if (ret)
	goto out;

The fix should be easy; just extract the current src coordinates from
the plane state and check those against the new fb size. And then hope
that the plane state is really up to date.

And I'm sure rotated cases will go boom in some other ways. Probably
we should just switch over to using the full plane update for mmio
flips to fix it.

> 
> 
> Matt
> 
> > 
> > I have posted a quick IGT exerciser for this as "kms_rotation_crc:
> > Excercise page flips with 90 degree rotation". May not be that great
> > but shows the failure.
> > 
> > I am not that hot on meddling with this code, nor do I feel
> > competent to even try on my own at least. :/ Maybe just because the
> > atomic and plane related rewrites have been going on for so long,
> > and have multiple people involved, it all sounds pretty scary and
> > fragile.
> > 
> > But I think some sort of plan on how to fix this could be in order?
> > 
> > Regards,
> > 
> > Tvrtko
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

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

Re: [PATCH 1/2] drm/i915/skl: Allow universal planes to position

[Matt Roper]

On Tue, Oct 06, 2015 at 05:42:42PM +0300, Ville Syrjälä wrote:
> On Tue, Oct 06, 2015 at 07:29:54AM -0700, Matt Roper wrote:
> > On Tue, Oct 06, 2015 at 02:32:47PM +0100, Tvrtko Ursulin wrote:
> > > 
> > > On 10/04/15 10:07, Sonika Jindal wrote:
> > > >Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> > > >Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> > > >---
> > > >  drivers/gpu/drm/i915/intel_display.c |    7 ++++++-
> > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > >
> > > >diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > >index ceb2e61..f0bbc22 100644
> > > >--- a/drivers/gpu/drm/i915/intel_display.c
> > > >+++ b/drivers/gpu/drm/i915/intel_display.c
> > > >@@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane,
> > > >  	struct drm_rect *dest = &state->dst;
> > > >  	struct drm_rect *src = &state->src;
> > > >  	const struct drm_rect *clip = &state->clip;
> > > >+	bool can_position = false;
> > > >  	int ret;
> > > >
> > > >  	crtc = crtc ? crtc : plane->crtc;
> > > >  	intel_crtc = to_intel_crtc(crtc);
> > > >
> > > >+	if (INTEL_INFO(dev)->gen >= 9)
> > > >+		can_position = true;
> > > >+
> > > >  	ret = drm_plane_helper_check_update(plane, crtc, fb,
> > > >  					    src, dest, clip,
> > > >  					    DRM_PLANE_HELPER_NO_SCALING,
> > > >  					    DRM_PLANE_HELPER_NO_SCALING,
> > > >-					    false, true, &state->visible);
> > > >+					    can_position, true,
> > > >+					    &state->visible);
> > > >  	if (ret)
> > > >  		return ret;
> > > >
> > > >
> > > 
> > > I have discovered today that, while this allows SetCrtc and SetPlane
> > > ioctls to work with frame buffers which do not cover the plane, page
> > > flips are not that lucky and fail roughly with:
> > > 
> > > [drm:drm_crtc_check_viewport] Invalid fb size 1080x1080 for CRTC
> > > viewport 1920x1080+0+0.
> > 
> > Maybe I'm misunderstanding your explanation, but a framebuffer is always
> > required to fill/cover the plane scanning out of it.  What this patch is
> > supposed to be allowing is for the primary plane to not cover the entire
> > CRTC (since that's something that only became possible for Intel
> > hardware on the gen9+ platforms).  I.e., the primary plane is now
> > allowed to positioned and resized to cover a subset of the CRTC area,
> > just like "sprite" planes have always been able to.
> > 
> > If you've got a 1080x1080 framebuffer, then it's legal to have a
> > 1080x1080 primary plane while running in 1920x1080 mode on SKL/BXT.
> > However it is not legal to size the primary plane as 1920x1080 and use
> > this same 1080x1080 framebuffer with any of our interfaces (setplane,
> > setcrtc, pageflip, or atomic).
> > 
> > Are you using ioctls/libdrm directly or are you using igt_kms helpers?
> > IIRC, the IGT helpers will try to be extra helpful and automatically
> > size the plane to match the framebuffer (unless you override that
> > behavior), so that might be what's causing the confusion here.
> 
> The problem is clear as day in drm_mode_page_flip_ioctl():
> ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y, &crtc->mode, fb);
> if (ret)
> 	goto out;
> 
> The fix should be easy; just extract the current src coordinates from
> the plane state and check those against the new fb size. And then hope
> that the plane state is really up to date.

Yep, that's the conclusion we came to once Tvrtko explained what he was
seeing on IRC.  I'm not sure whether non-atomic drivers have enough
state setup by the default helpers to work properly.  Worst case we'll
just assume that a non-atomic driver won't support primary plane
windowing (since none have in the past) and fall back to looking at the
mode for legacy non-atomic drivers.

> 
> And I'm sure rotated cases will go boom in some other ways. Probably
> we should just switch over to using the full plane update for mmio
> flips to fix it.

Yeah; the core looks at a drm_plane->invert_dimensions field that's only
set by omap.  That should probably be updated to look at the state's
rotation on atomic-capable drivers.


Matt

> 
> > 
> > 
> > Matt
> > 
> > > 
> > > I have posted a quick IGT exerciser for this as "kms_rotation_crc:
> > > Excercise page flips with 90 degree rotation". May not be that great
> > > but shows the failure.
> > > 
> > > I am not that hot on meddling with this code, nor do I feel
> > > competent to even try on my own at least. :/ Maybe just because the
> > > atomic and plane related rewrites have been going on for so long,
> > > and have multiple people involved, it all sounds pretty scary and
> > > fragile.
> > > 
> > > But I think some sort of plan on how to fix this could be in order?
> > > 
> > > Regards,
> > > 
> > > Tvrtko
> > 
> > -- 
> > Matt Roper
> > Graphics Software Engineer
> > IoTG Platform Enabling & Development
> > Intel Corporation
> > (916) 356-2795
> 
> -- 
> Ville Syrjälä
> Intel OTC

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

Re: [PATCH 1/2] drm/i915/skl: Allow universal planes to position

[Ville Syrjälä]

On Tue, Oct 06, 2015 at 08:16:19AM -0700, Matt Roper wrote:
> On Tue, Oct 06, 2015 at 05:42:42PM +0300, Ville Syrjälä wrote:
> > On Tue, Oct 06, 2015 at 07:29:54AM -0700, Matt Roper wrote:
> > > On Tue, Oct 06, 2015 at 02:32:47PM +0100, Tvrtko Ursulin wrote:
> > > > 
> > > > On 10/04/15 10:07, Sonika Jindal wrote:
> > > > >Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> > > > >Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> > > > >---
> > > > >  drivers/gpu/drm/i915/intel_display.c |    7 ++++++-
> > > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > > >
> > > > >diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > >index ceb2e61..f0bbc22 100644
> > > > >--- a/drivers/gpu/drm/i915/intel_display.c
> > > > >+++ b/drivers/gpu/drm/i915/intel_display.c
> > > > >@@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane,
> > > > >  	struct drm_rect *dest = &state->dst;
> > > > >  	struct drm_rect *src = &state->src;
> > > > >  	const struct drm_rect *clip = &state->clip;
> > > > >+	bool can_position = false;
> > > > >  	int ret;
> > > > >
> > > > >  	crtc = crtc ? crtc : plane->crtc;
> > > > >  	intel_crtc = to_intel_crtc(crtc);
> > > > >
> > > > >+	if (INTEL_INFO(dev)->gen >= 9)
> > > > >+		can_position = true;
> > > > >+
> > > > >  	ret = drm_plane_helper_check_update(plane, crtc, fb,
> > > > >  					    src, dest, clip,
> > > > >  					    DRM_PLANE_HELPER_NO_SCALING,
> > > > >  					    DRM_PLANE_HELPER_NO_SCALING,
> > > > >-					    false, true, &state->visible);
> > > > >+					    can_position, true,
> > > > >+					    &state->visible);
> > > > >  	if (ret)
> > > > >  		return ret;
> > > > >
> > > > >
> > > > 
> > > > I have discovered today that, while this allows SetCrtc and SetPlane
> > > > ioctls to work with frame buffers which do not cover the plane, page
> > > > flips are not that lucky and fail roughly with:
> > > > 
> > > > [drm:drm_crtc_check_viewport] Invalid fb size 1080x1080 for CRTC
> > > > viewport 1920x1080+0+0.
> > > 
> > > Maybe I'm misunderstanding your explanation, but a framebuffer is always
> > > required to fill/cover the plane scanning out of it.  What this patch is
> > > supposed to be allowing is for the primary plane to not cover the entire
> > > CRTC (since that's something that only became possible for Intel
> > > hardware on the gen9+ platforms).  I.e., the primary plane is now
> > > allowed to positioned and resized to cover a subset of the CRTC area,
> > > just like "sprite" planes have always been able to.
> > > 
> > > If you've got a 1080x1080 framebuffer, then it's legal to have a
> > > 1080x1080 primary plane while running in 1920x1080 mode on SKL/BXT.
> > > However it is not legal to size the primary plane as 1920x1080 and use
> > > this same 1080x1080 framebuffer with any of our interfaces (setplane,
> > > setcrtc, pageflip, or atomic).
> > > 
> > > Are you using ioctls/libdrm directly or are you using igt_kms helpers?
> > > IIRC, the IGT helpers will try to be extra helpful and automatically
> > > size the plane to match the framebuffer (unless you override that
> > > behavior), so that might be what's causing the confusion here.
> > 
> > The problem is clear as day in drm_mode_page_flip_ioctl():
> > ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y, &crtc->mode, fb);
> > if (ret)
> > 	goto out;
> > 
> > The fix should be easy; just extract the current src coordinates from
> > the plane state and check those against the new fb size. And then hope
> > that the plane state is really up to date.
> 
> Yep, that's the conclusion we came to once Tvrtko explained what he was
> seeing on IRC.  I'm not sure whether non-atomic drivers have enough
> state setup by the default helpers to work properly.  Worst case we'll
> just assume that a non-atomic driver won't support primary plane
> windowing (since none have in the past) and fall back to looking at the
> mode for legacy non-atomic drivers.
> 
> > 
> > And I'm sure rotated cases will go boom in some other ways. Probably
> > we should just switch over to using the full plane update for mmio
> > flips to fix it.
> 
> Yeah; the core looks at a drm_plane->invert_dimensions field that's only
> set by omap.  That should probably be updated to look at the state's
> rotation on atomic-capable drivers.

We can just look at the src coordinates. Those always match the fb
orientation.

> 
> 
> Matt
> 
> > 
> > > 
> > > 
> > > Matt
> > > 
> > > > 
> > > > I have posted a quick IGT exerciser for this as "kms_rotation_crc:
> > > > Excercise page flips with 90 degree rotation". May not be that great
> > > > but shows the failure.
> > > > 
> > > > I am not that hot on meddling with this code, nor do I feel
> > > > competent to even try on my own at least. :/ Maybe just because the
> > > > atomic and plane related rewrites have been going on for so long,
> > > > and have multiple people involved, it all sounds pretty scary and
> > > > fragile.
> > > > 
> > > > But I think some sort of plan on how to fix this could be in order?
> > > > 
> > > > Regards,
> > > > 
> > > > Tvrtko
> > > 
> > > -- 
> > > Matt Roper
> > > Graphics Software Engineer
> > > IoTG Platform Enabling & Development
> > > Intel Corporation
> > > (916) 356-2795
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

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

Re: [PATCH 1/2] drm/i915/skl: Allow universal planes to position

[Daniel Vetter]

On Tue, Oct 06, 2015 at 07:28:10PM +0300, Ville Syrjälä wrote:
> On Tue, Oct 06, 2015 at 08:16:19AM -0700, Matt Roper wrote:
> > On Tue, Oct 06, 2015 at 05:42:42PM +0300, Ville Syrjälä wrote:
> > > On Tue, Oct 06, 2015 at 07:29:54AM -0700, Matt Roper wrote:
> > > > On Tue, Oct 06, 2015 at 02:32:47PM +0100, Tvrtko Ursulin wrote:
> > > > > 
> > > > > On 10/04/15 10:07, Sonika Jindal wrote:
> > > > > >Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> > > > > >Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> > > > > >---
> > > > > >  drivers/gpu/drm/i915/intel_display.c |    7 ++++++-
> > > > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > > > >
> > > > > >diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > > >index ceb2e61..f0bbc22 100644
> > > > > >--- a/drivers/gpu/drm/i915/intel_display.c
> > > > > >+++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > >@@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane,
> > > > > >  	struct drm_rect *dest = &state->dst;
> > > > > >  	struct drm_rect *src = &state->src;
> > > > > >  	const struct drm_rect *clip = &state->clip;
> > > > > >+	bool can_position = false;
> > > > > >  	int ret;
> > > > > >
> > > > > >  	crtc = crtc ? crtc : plane->crtc;
> > > > > >  	intel_crtc = to_intel_crtc(crtc);
> > > > > >
> > > > > >+	if (INTEL_INFO(dev)->gen >= 9)
> > > > > >+		can_position = true;
> > > > > >+
> > > > > >  	ret = drm_plane_helper_check_update(plane, crtc, fb,
> > > > > >  					    src, dest, clip,
> > > > > >  					    DRM_PLANE_HELPER_NO_SCALING,
> > > > > >  					    DRM_PLANE_HELPER_NO_SCALING,
> > > > > >-					    false, true, &state->visible);
> > > > > >+					    can_position, true,
> > > > > >+					    &state->visible);
> > > > > >  	if (ret)
> > > > > >  		return ret;
> > > > > >
> > > > > >
> > > > > 
> > > > > I have discovered today that, while this allows SetCrtc and SetPlane
> > > > > ioctls to work with frame buffers which do not cover the plane, page
> > > > > flips are not that lucky and fail roughly with:
> > > > > 
> > > > > [drm:drm_crtc_check_viewport] Invalid fb size 1080x1080 for CRTC
> > > > > viewport 1920x1080+0+0.
> > > > 
> > > > Maybe I'm misunderstanding your explanation, but a framebuffer is always
> > > > required to fill/cover the plane scanning out of it.  What this patch is
> > > > supposed to be allowing is for the primary plane to not cover the entire
> > > > CRTC (since that's something that only became possible for Intel
> > > > hardware on the gen9+ platforms).  I.e., the primary plane is now
> > > > allowed to positioned and resized to cover a subset of the CRTC area,
> > > > just like "sprite" planes have always been able to.
> > > > 
> > > > If you've got a 1080x1080 framebuffer, then it's legal to have a
> > > > 1080x1080 primary plane while running in 1920x1080 mode on SKL/BXT.
> > > > However it is not legal to size the primary plane as 1920x1080 and use
> > > > this same 1080x1080 framebuffer with any of our interfaces (setplane,
> > > > setcrtc, pageflip, or atomic).
> > > > 
> > > > Are you using ioctls/libdrm directly or are you using igt_kms helpers?
> > > > IIRC, the IGT helpers will try to be extra helpful and automatically
> > > > size the plane to match the framebuffer (unless you override that
> > > > behavior), so that might be what's causing the confusion here.
> > > 
> > > The problem is clear as day in drm_mode_page_flip_ioctl():
> > > ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y, &crtc->mode, fb);
> > > if (ret)
> > > 	goto out;
> > > 
> > > The fix should be easy; just extract the current src coordinates from
> > > the plane state and check those against the new fb size. And then hope
> > > that the plane state is really up to date.
> > 
> > Yep, that's the conclusion we came to once Tvrtko explained what he was
> > seeing on IRC.  I'm not sure whether non-atomic drivers have enough
> > state setup by the default helpers to work properly.  Worst case we'll
> > just assume that a non-atomic driver won't support primary plane
> > windowing (since none have in the past) and fall back to looking at the
> > mode for legacy non-atomic drivers.
> > 
> > > 
> > > And I'm sure rotated cases will go boom in some other ways. Probably
> > > we should just switch over to using the full plane update for mmio
> > > flips to fix it.
> > 
> > Yeah; the core looks at a drm_plane->invert_dimensions field that's only
> > set by omap.  That should probably be updated to look at the state's
> > rotation on atomic-capable drivers.
> 
> We can just look at the src coordinates. Those always match the fb
> orientation.

Can we just not bother with legacy pageflips on rotated planes? setplane
works and once you rotate it kinda gets nasty anyway.

The problem I see is that with legacy pageflip we also need to hack up
something that doesn't look at plane->state for legacy and all that for a
grand total of about 2 drivers, both getting converted to atomic.
-Daniel

> 
> > 
> > 
> > Matt
> > 
> > > 
> > > > 
> > > > 
> > > > Matt
> > > > 
> > > > > 
> > > > > I have posted a quick IGT exerciser for this as "kms_rotation_crc:
> > > > > Excercise page flips with 90 degree rotation". May not be that great
> > > > > but shows the failure.
> > > > > 
> > > > > I am not that hot on meddling with this code, nor do I feel
> > > > > competent to even try on my own at least. :/ Maybe just because the
> > > > > atomic and plane related rewrites have been going on for so long,
> > > > > and have multiple people involved, it all sounds pretty scary and
> > > > > fragile.
> > > > > 
> > > > > But I think some sort of plan on how to fix this could be in order?
> > > > > 
> > > > > Regards,
> > > > > 
> > > > > Tvrtko
> > > > 
> > > > -- 
> > > > Matt Roper
> > > > Graphics Software Engineer
> > > > IoTG Platform Enabling & Development
> > > > Intel Corporation
> > > > (916) 356-2795
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel OTC
> > 
> > -- 
> > Matt Roper
> > Graphics Software Engineer
> > IoTG Platform Enabling & Development
> > Intel Corporation
> > (916) 356-2795
> 
> -- 
> Ville Syrjälä
> Intel OTC

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

Re: [PATCH 1/2] drm/i915/skl: Allow universal planes to position

[Tvrtko Ursulin]

On 07/10/15 15:19, Daniel Vetter wrote:
> On Tue, Oct 06, 2015 at 07:28:10PM +0300, Ville Syrjälä wrote:
>> On Tue, Oct 06, 2015 at 08:16:19AM -0700, Matt Roper wrote:
>>> On Tue, Oct 06, 2015 at 05:42:42PM +0300, Ville Syrjälä wrote:
>>>> On Tue, Oct 06, 2015 at 07:29:54AM -0700, Matt Roper wrote:
>>>>> On Tue, Oct 06, 2015 at 02:32:47PM +0100, Tvrtko Ursulin wrote:
>>>>>>
>>>>>> On 10/04/15 10:07, Sonika Jindal wrote:
>>>>>>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
>>>>>>> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
>>>>>>> ---
>>>>>>>   drivers/gpu/drm/i915/intel_display.c |    7 ++++++-
>>>>>>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>>>>> index ceb2e61..f0bbc22 100644
>>>>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>>>>> @@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct drm_plane *plane,
>>>>>>>   	struct drm_rect *dest = &state->dst;
>>>>>>>   	struct drm_rect *src = &state->src;
>>>>>>>   	const struct drm_rect *clip = &state->clip;
>>>>>>> +	bool can_position = false;
>>>>>>>   	int ret;
>>>>>>>
>>>>>>>   	crtc = crtc ? crtc : plane->crtc;
>>>>>>>   	intel_crtc = to_intel_crtc(crtc);
>>>>>>>
>>>>>>> +	if (INTEL_INFO(dev)->gen >= 9)
>>>>>>> +		can_position = true;
>>>>>>> +
>>>>>>>   	ret = drm_plane_helper_check_update(plane, crtc, fb,
>>>>>>>   					    src, dest, clip,
>>>>>>>   					    DRM_PLANE_HELPER_NO_SCALING,
>>>>>>>   					    DRM_PLANE_HELPER_NO_SCALING,
>>>>>>> -					    false, true, &state->visible);
>>>>>>> +					    can_position, true,
>>>>>>> +					    &state->visible);
>>>>>>>   	if (ret)
>>>>>>>   		return ret;
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> I have discovered today that, while this allows SetCrtc and SetPlane
>>>>>> ioctls to work with frame buffers which do not cover the plane, page
>>>>>> flips are not that lucky and fail roughly with:
>>>>>>
>>>>>> [drm:drm_crtc_check_viewport] Invalid fb size 1080x1080 for CRTC
>>>>>> viewport 1920x1080+0+0.
>>>>>
>>>>> Maybe I'm misunderstanding your explanation, but a framebuffer is always
>>>>> required to fill/cover the plane scanning out of it.  What this patch is
>>>>> supposed to be allowing is for the primary plane to not cover the entire
>>>>> CRTC (since that's something that only became possible for Intel
>>>>> hardware on the gen9+ platforms).  I.e., the primary plane is now
>>>>> allowed to positioned and resized to cover a subset of the CRTC area,
>>>>> just like "sprite" planes have always been able to.
>>>>>
>>>>> If you've got a 1080x1080 framebuffer, then it's legal to have a
>>>>> 1080x1080 primary plane while running in 1920x1080 mode on SKL/BXT.
>>>>> However it is not legal to size the primary plane as 1920x1080 and use
>>>>> this same 1080x1080 framebuffer with any of our interfaces (setplane,
>>>>> setcrtc, pageflip, or atomic).
>>>>>
>>>>> Are you using ioctls/libdrm directly or are you using igt_kms helpers?
>>>>> IIRC, the IGT helpers will try to be extra helpful and automatically
>>>>> size the plane to match the framebuffer (unless you override that
>>>>> behavior), so that might be what's causing the confusion here.
>>>>
>>>> The problem is clear as day in drm_mode_page_flip_ioctl():
>>>> ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y, &crtc->mode, fb);
>>>> if (ret)
>>>> 	goto out;
>>>>
>>>> The fix should be easy; just extract the current src coordinates from
>>>> the plane state and check those against the new fb size. And then hope
>>>> that the plane state is really up to date.
>>>
>>> Yep, that's the conclusion we came to once Tvrtko explained what he was
>>> seeing on IRC.  I'm not sure whether non-atomic drivers have enough
>>> state setup by the default helpers to work properly.  Worst case we'll
>>> just assume that a non-atomic driver won't support primary plane
>>> windowing (since none have in the past) and fall back to looking at the
>>> mode for legacy non-atomic drivers.
>>>
>>>>
>>>> And I'm sure rotated cases will go boom in some other ways. Probably
>>>> we should just switch over to using the full plane update for mmio
>>>> flips to fix it.
>>>
>>> Yeah; the core looks at a drm_plane->invert_dimensions field that's only
>>> set by omap.  That should probably be updated to look at the state's
>>> rotation on atomic-capable drivers.
>>
>> We can just look at the src coordinates. Those always match the fb
>> orientation.
>
> Can we just not bother with legacy pageflips on rotated planes? setplane
> works and once you rotate it kinda gets nasty anyway.

I don't know - thought it is simple enough to make it work so why not? 
Just " [PATCH] drm/i915: Consider plane rotation when calculating stride 
in skl_do_mmio_flip" I posted, plus Matt's "[PATCH] drm: Check fb 
against plane size rather than CRTC mode for pageflip​" to allow smaller 
than mode planes.

> The problem I see is that with legacy pageflip we also need to hack up
> something that doesn't look at plane->state for legacy and all that for a
> grand total of about 2 drivers, both getting converted to atomic.

I'll leave the legacy/atomic/etc considerations to the experts. :)

Regards,

Tvrtko



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

Re: [PATCH 1/2] drm/i915/skl: Allow universal planes to position

[Tvrtko Ursulin]

On 08/10/15 09:58, Tvrtko Ursulin wrote:
> On 07/10/15 15:19, Daniel Vetter wrote:
>> On Tue, Oct 06, 2015 at 07:28:10PM +0300, Ville Syrjälä wrote:
>>> On Tue, Oct 06, 2015 at 08:16:19AM -0700, Matt Roper wrote:
>>>> On Tue, Oct 06, 2015 at 05:42:42PM +0300, Ville Syrjälä wrote:
>>>>> On Tue, Oct 06, 2015 at 07:29:54AM -0700, Matt Roper wrote:
>>>>>> On Tue, Oct 06, 2015 at 02:32:47PM +0100, Tvrtko Ursulin wrote:
>>>>>>>
>>>>>>> On 10/04/15 10:07, Sonika Jindal wrote:
>>>>>>>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
>>>>>>>> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
>>>>>>>> ---
>>>>>>>>   drivers/gpu/drm/i915/intel_display.c |    7 ++++++-
>>>>>>>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>>>>>>>> b/drivers/gpu/drm/i915/intel_display.c
>>>>>>>> index ceb2e61..f0bbc22 100644
>>>>>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>>>>>> @@ -12150,16 +12150,21 @@ intel_check_primary_plane(struct
>>>>>>>> drm_plane *plane,
>>>>>>>>       struct drm_rect *dest = &state->dst;
>>>>>>>>       struct drm_rect *src = &state->src;
>>>>>>>>       const struct drm_rect *clip = &state->clip;
>>>>>>>> +    bool can_position = false;
>>>>>>>>       int ret;
>>>>>>>>
>>>>>>>>       crtc = crtc ? crtc : plane->crtc;
>>>>>>>>       intel_crtc = to_intel_crtc(crtc);
>>>>>>>>
>>>>>>>> +    if (INTEL_INFO(dev)->gen >= 9)
>>>>>>>> +        can_position = true;
>>>>>>>> +
>>>>>>>>       ret = drm_plane_helper_check_update(plane, crtc, fb,
>>>>>>>>                           src, dest, clip,
>>>>>>>>                           DRM_PLANE_HELPER_NO_SCALING,
>>>>>>>>                           DRM_PLANE_HELPER_NO_SCALING,
>>>>>>>> -                        false, true, &state->visible);
>>>>>>>> +                        can_position, true,
>>>>>>>> +                        &state->visible);
>>>>>>>>       if (ret)
>>>>>>>>           return ret;
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> I have discovered today that, while this allows SetCrtc and SetPlane
>>>>>>> ioctls to work with frame buffers which do not cover the plane, page
>>>>>>> flips are not that lucky and fail roughly with:
>>>>>>>
>>>>>>> [drm:drm_crtc_check_viewport] Invalid fb size 1080x1080 for CRTC
>>>>>>> viewport 1920x1080+0+0.
>>>>>>
>>>>>> Maybe I'm misunderstanding your explanation, but a framebuffer is
>>>>>> always
>>>>>> required to fill/cover the plane scanning out of it.  What this
>>>>>> patch is
>>>>>> supposed to be allowing is for the primary plane to not cover the
>>>>>> entire
>>>>>> CRTC (since that's something that only became possible for Intel
>>>>>> hardware on the gen9+ platforms).  I.e., the primary plane is now
>>>>>> allowed to positioned and resized to cover a subset of the CRTC area,
>>>>>> just like "sprite" planes have always been able to.
>>>>>>
>>>>>> If you've got a 1080x1080 framebuffer, then it's legal to have a
>>>>>> 1080x1080 primary plane while running in 1920x1080 mode on SKL/BXT.
>>>>>> However it is not legal to size the primary plane as 1920x1080 and
>>>>>> use
>>>>>> this same 1080x1080 framebuffer with any of our interfaces (setplane,
>>>>>> setcrtc, pageflip, or atomic).
>>>>>>
>>>>>> Are you using ioctls/libdrm directly or are you using igt_kms
>>>>>> helpers?
>>>>>> IIRC, the IGT helpers will try to be extra helpful and automatically
>>>>>> size the plane to match the framebuffer (unless you override that
>>>>>> behavior), so that might be what's causing the confusion here.
>>>>>
>>>>> The problem is clear as day in drm_mode_page_flip_ioctl():
>>>>> ret = drm_crtc_check_viewport(crtc, crtc->x, crtc->y, &crtc->mode,
>>>>> fb);
>>>>> if (ret)
>>>>>     goto out;
>>>>>
>>>>> The fix should be easy; just extract the current src coordinates from
>>>>> the plane state and check those against the new fb size. And then hope
>>>>> that the plane state is really up to date.
>>>>
>>>> Yep, that's the conclusion we came to once Tvrtko explained what he was
>>>> seeing on IRC.  I'm not sure whether non-atomic drivers have enough
>>>> state setup by the default helpers to work properly.  Worst case we'll
>>>> just assume that a non-atomic driver won't support primary plane
>>>> windowing (since none have in the past) and fall back to looking at the
>>>> mode for legacy non-atomic drivers.
>>>>
>>>>>
>>>>> And I'm sure rotated cases will go boom in some other ways. Probably
>>>>> we should just switch over to using the full plane update for mmio
>>>>> flips to fix it.
>>>>
>>>> Yeah; the core looks at a drm_plane->invert_dimensions field that's
>>>> only
>>>> set by omap.  That should probably be updated to look at the state's
>>>> rotation on atomic-capable drivers.
>>>
>>> We can just look at the src coordinates. Those always match the fb
>>> orientation.
>>
>> Can we just not bother with legacy pageflips on rotated planes? setplane
>> works and once you rotate it kinda gets nasty anyway.
>
> I don't know - thought it is simple enough to make it work so why not?
> Just " [PATCH] drm/i915: Consider plane rotation when calculating stride
> in skl_do_mmio_flip" I posted, plus Matt's "[PATCH] drm: Check fb
> against plane size rather than CRTC mode for pageflip​" to allow smaller
> than mode planes.
>
>> The problem I see is that with legacy pageflip we also need to hack up
>> something that doesn't look at plane->state for legacy and all that for a
>> grand total of about 2 drivers, both getting converted to atomic.
>
> I'll leave the legacy/atomic/etc considerations to the experts. :)

Are we sure any efforts to support rotation in legacy page flips is not 
worth it?

So far there were three patches for this: Plane programming fix (very 
simple) and an IGT test case (simple as well) from me, and a sub-crtc 
size plane support from Matt.

It kind of remained hanging a bit so I think it would be good to make a 
definitive decision.

Regards,

Tvrtko

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