[PATCH 0/7] drm/omap: rotation fixes and cleanups

[PATCH 0/7] drm/omap: rotation fixes and cleanups

[Tomi Valkeinen]

Hi,

This series cleans up rotation related code and fixes the major bugs with
rotation with YUV422 pixelformats. This series is based on the earlier
"drm/omap: misc cleanups and pixel format change" series.

There are still some smaller bugs with the rotation when used with scaling,
which I will continue on debugging.

 Tomi

Tomi Valkeinen (7):
  drm/omap: add drm_rotation_to_tiler helper()
  drm/omap: remove omap_drm_win
  drm/omap: use DRM_ROTATE_* instead of OMAP_DSS_ROT_*
  drm/omap: DRM_REFLECT_* instead of mirror boolean
  drm/omap: pass rotation to dispc
  drm/omap: fix YUV422 rotation with TILER
  drm/omap: fix YUV422 90/270 rotation with mirroring

 drivers/gpu/drm/omapdrm/dss/dispc.c   |  94 ++++++++++++++++--------------
 drivers/gpu/drm/omapdrm/dss/omapdss.h |  10 ----
 drivers/gpu/drm/omapdrm/omap_drv.h    |  11 +---
 drivers/gpu/drm/omapdrm/omap_fb.c     | 105 +++++++++++++++++++++-------------
 drivers/gpu/drm/omapdrm/omap_plane.c  |  28 +--------
 5 files changed, 119 insertions(+), 129 deletions(-)

-- 
2.7.4

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

[PATCH 1/7] drm/omap: add drm_rotation_to_tiler helper()

[Tomi Valkeinen]

Add a helper function to convert DRM rotation to TILER rotation.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_fb.c | 56 ++++++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
index c565f5734a53..4fc5db5d2d29 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.c
+++ b/drivers/gpu/drm/omapdrm/omap_fb.c
@@ -122,6 +122,36 @@ bool omap_framebuffer_supports_rotation(struct drm_framebuffer *fb)
 	return omap_gem_flags(plane->bo) & OMAP_BO_TILED;
 }
 
+/* Note: DRM rotates counter-clockwise, TILER & DSS rotates clockwise */
+static uint32_t drm_rotation_to_tiler(unsigned int drm_rot)
+{
+	uint32_t orient;
+
+	switch (drm_rot & DRM_ROTATE_MASK) {
+	default:
+	case DRM_ROTATE_0:
+		orient = 0;
+		break;
+	case DRM_ROTATE_90:
+		orient = MASK_XY_FLIP | MASK_X_INVERT;
+		break;
+	case DRM_ROTATE_180:
+		orient = MASK_X_INVERT | MASK_Y_INVERT;
+		break;
+	case DRM_ROTATE_270:
+		orient = MASK_XY_FLIP | MASK_Y_INVERT;
+		break;
+	}
+
+	if (drm_rot & DRM_REFLECT_X)
+		orient ^= MASK_X_INVERT;
+
+	if (drm_rot & DRM_REFLECT_Y)
+		orient ^= MASK_Y_INVERT;
+
+	return orient;
+}
+
 /* update ovl info for scanout, handles cases of multi-planar fb's, etc.
  */
 void omap_framebuffer_update_scanout(struct drm_framebuffer *fb,
@@ -148,31 +178,7 @@ void omap_framebuffer_update_scanout(struct drm_framebuffer *fb,
 		uint32_t w = win->src_w;
 		uint32_t h = win->src_h;
 
-		switch (win->rotation & DRM_ROTATE_MASK) {
-		default:
-			dev_err(fb->dev->dev, "invalid rotation: %02x",
-					(uint32_t)win->rotation);
-			/* fallthru to default to no rotation */
-		case 0:
-		case DRM_ROTATE_0:
-			orient = 0;
-			break;
-		case DRM_ROTATE_90:
-			orient = MASK_XY_FLIP | MASK_X_INVERT;
-			break;
-		case DRM_ROTATE_180:
-			orient = MASK_X_INVERT | MASK_Y_INVERT;
-			break;
-		case DRM_ROTATE_270:
-			orient = MASK_XY_FLIP | MASK_Y_INVERT;
-			break;
-		}
-
-		if (win->rotation & DRM_REFLECT_X)
-			orient ^= MASK_X_INVERT;
-
-		if (win->rotation & DRM_REFLECT_Y)
-			orient ^= MASK_Y_INVERT;
+		orient = drm_rotation_to_tiler(win->rotation);
 
 		/* adjust x,y offset for flip/invert: */
 		if (orient & MASK_XY_FLIP)
-- 
2.7.4

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

Re: [PATCH 1/7] drm/omap: add drm_rotation_to_tiler helper()

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Wednesday 17 May 2017 10:56:38 Tomi Valkeinen wrote:
> Add a helper function to convert DRM rotation to TILER rotation.

You could mention here that the patch drops an error message that could never 
be printed as the DRM core guarantees that the rotation value is valid.

Apart from that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_fb.c | 56 ++++++++++++++++++++----------------
>  1 file changed, 31 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c
> b/drivers/gpu/drm/omapdrm/omap_fb.c index c565f5734a53..4fc5db5d2d29 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fb.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
> @@ -122,6 +122,36 @@ bool omap_framebuffer_supports_rotation(struct
> drm_framebuffer *fb) return omap_gem_flags(plane->bo) & OMAP_BO_TILED;
>  }
> 
> +/* Note: DRM rotates counter-clockwise, TILER & DSS rotates clockwise */
> +static uint32_t drm_rotation_to_tiler(unsigned int drm_rot)
> +{
> +	uint32_t orient;
> +
> +	switch (drm_rot & DRM_ROTATE_MASK) {
> +	default:
> +	case DRM_ROTATE_0:
> +		orient = 0;
> +		break;
> +	case DRM_ROTATE_90:
> +		orient = MASK_XY_FLIP | MASK_X_INVERT;
> +		break;
> +	case DRM_ROTATE_180:
> +		orient = MASK_X_INVERT | MASK_Y_INVERT;
> +		break;
> +	case DRM_ROTATE_270:
> +		orient = MASK_XY_FLIP | MASK_Y_INVERT;
> +		break;
> +	}
> +
> +	if (drm_rot & DRM_REFLECT_X)
> +		orient ^= MASK_X_INVERT;
> +
> +	if (drm_rot & DRM_REFLECT_Y)
> +		orient ^= MASK_Y_INVERT;
> +
> +	return orient;
> +}
> +
>  /* update ovl info for scanout, handles cases of multi-planar fb's, etc.
>   */
>  void omap_framebuffer_update_scanout(struct drm_framebuffer *fb,
> @@ -148,31 +178,7 @@ void omap_framebuffer_update_scanout(struct
> drm_framebuffer *fb, uint32_t w = win->src_w;
>  		uint32_t h = win->src_h;
> 
> -		switch (win->rotation & DRM_ROTATE_MASK) {
> -		default:
> -			dev_err(fb->dev->dev, "invalid rotation: %02x",
> -					(uint32_t)win->rotation);
> -			/* fallthru to default to no rotation */
> -		case 0:
> -		case DRM_ROTATE_0:
> -			orient = 0;
> -			break;
> -		case DRM_ROTATE_90:
> -			orient = MASK_XY_FLIP | MASK_X_INVERT;
> -			break;
> -		case DRM_ROTATE_180:
> -			orient = MASK_X_INVERT | MASK_Y_INVERT;
> -			break;
> -		case DRM_ROTATE_270:
> -			orient = MASK_XY_FLIP | MASK_Y_INVERT;
> -			break;
> -		}
> -
> -		if (win->rotation & DRM_REFLECT_X)
> -			orient ^= MASK_X_INVERT;
> -
> -		if (win->rotation & DRM_REFLECT_Y)
> -			orient ^= MASK_Y_INVERT;
> +		orient = drm_rotation_to_tiler(win->rotation);
> 
>  		/* adjust x,y offset for flip/invert: */
>  		if (orient & MASK_XY_FLIP)

-- 
Regards,

Laurent Pinchart

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

[PATCH 2/7] drm/omap: remove omap_drm_win

[Tomi Valkeinen]

struct omap_drm_window is only used to pass plane setup data to
omap_framebuffer_update_scanout(). This can as well be accomplished by
just passing the DRM state.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.h   | 11 +----------
 drivers/gpu/drm/omapdrm/omap_fb.c    | 35 ++++++++++++++++++-----------------
 drivers/gpu/drm/omapdrm/omap_plane.c | 25 +------------------------
 3 files changed, 20 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index ca087a993909..4bd1e9070b31 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -38,15 +38,6 @@
 
 struct omap_drm_usergart;
 
-/* parameters which describe (unrotated) coordinates of scanout within a fb: */
-struct omap_drm_window {
-	uint32_t rotation;
-	int32_t  crtc_x, crtc_y;		/* signed because can be offscreen */
-	uint32_t crtc_w, crtc_h;
-	uint32_t src_x, src_y;
-	uint32_t src_w, src_h;
-};
-
 /* For KMS code that needs to wait for a certain # of IRQs:
  */
 struct omap_irq_wait;
@@ -157,7 +148,7 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
 int omap_framebuffer_pin(struct drm_framebuffer *fb);
 void omap_framebuffer_unpin(struct drm_framebuffer *fb);
 void omap_framebuffer_update_scanout(struct drm_framebuffer *fb,
-		struct omap_drm_window *win, struct omap_overlay_info *info);
+		struct drm_plane_state *state, struct omap_overlay_info *info);
 struct drm_connector *omap_framebuffer_get_next_connector(
 		struct drm_framebuffer *fb, struct drm_connector *from);
 bool omap_framebuffer_supports_rotation(struct drm_framebuffer *fb);
diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
index 4fc5db5d2d29..b7e7038cd2ce 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.c
+++ b/drivers/gpu/drm/omapdrm/omap_fb.c
@@ -155,7 +155,7 @@ static uint32_t drm_rotation_to_tiler(unsigned int drm_rot)
 /* update ovl info for scanout, handles cases of multi-planar fb's, etc.
  */
 void omap_framebuffer_update_scanout(struct drm_framebuffer *fb,
-		struct omap_drm_window *win, struct omap_overlay_info *info)
+		struct drm_plane_state *state, struct omap_overlay_info *info)
 {
 	struct omap_framebuffer *omap_fb = to_omap_framebuffer(fb);
 	const struct drm_format_info *format = omap_fb->format;
@@ -164,25 +164,27 @@ void omap_framebuffer_update_scanout(struct drm_framebuffer *fb,
 
 	info->fourcc = fb->format->format;
 
-	info->pos_x      = win->crtc_x;
-	info->pos_y      = win->crtc_y;
-	info->out_width  = win->crtc_w;
-	info->out_height = win->crtc_h;
-	info->width      = win->src_w;
-	info->height     = win->src_h;
+	info->pos_x      = state->crtc_x;
+	info->pos_y      = state->crtc_y;
+	info->out_width  = state->crtc_w;
+	info->out_height = state->crtc_h;
+	info->width      = state->src_w >> 16;
+	info->height     = state->src_h >> 16;
 
-	x = win->src_x;
-	y = win->src_y;
+	/* DSS driver wants the w & h in rotated orientation */
+	if (drm_rotation_90_or_270(state->rotation))
+		swap(info->width, info->height);
+
+	x = state->src_x >> 16;
+	y = state->src_y >> 16;
 
 	if (omap_gem_flags(plane->bo) & OMAP_BO_TILED) {
-		uint32_t w = win->src_w;
-		uint32_t h = win->src_h;
+		uint32_t w = state->src_w >> 16;
+		uint32_t h = state->src_h >> 16;
 
-		orient = drm_rotation_to_tiler(win->rotation);
+		orient = drm_rotation_to_tiler(state->rotation);
 
 		/* adjust x,y offset for flip/invert: */
-		if (orient & MASK_XY_FLIP)
-			swap(w, h);
 		if (orient & MASK_Y_INVERT)
 			y += h - 1;
 		if (orient & MASK_X_INVERT)
@@ -193,7 +195,7 @@ void omap_framebuffer_update_scanout(struct drm_framebuffer *fb,
 		info->rotation_type = OMAP_DSS_ROT_TILER;
 		info->screen_width  = omap_gem_tiled_stride(plane->bo, orient);
 	} else {
-		switch (win->rotation & DRM_ROTATE_MASK) {
+		switch (state->rotation & DRM_ROTATE_MASK) {
 		case 0:
 		case DRM_ROTATE_0:
 			/* OK */
@@ -202,8 +204,7 @@ void omap_framebuffer_update_scanout(struct drm_framebuffer *fb,
 		default:
 			dev_warn(fb->dev->dev,
 				"rotation '%d' ignored for non-tiled fb\n",
-				win->rotation);
-			win->rotation = 0;
+				state->rotation);
 			break;
 		}
 
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
index 7c8525c21df8..08644326f7eb 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -59,7 +59,6 @@ static void omap_plane_atomic_update(struct drm_plane *plane,
 	struct omap_plane *omap_plane = to_omap_plane(plane);
 	struct drm_plane_state *state = plane->state;
 	struct omap_overlay_info info;
-	struct omap_drm_window win;
 	int ret;
 
 	DBG("%s, crtc=%p fb=%p", omap_plane->name, state->crtc, state->fb);
@@ -71,30 +70,8 @@ static void omap_plane_atomic_update(struct drm_plane *plane,
 	info.mirror = 0;
 	info.zorder = state->zpos;
 
-	memset(&win, 0, sizeof(win));
-	win.rotation = state->rotation;
-	win.crtc_x = state->crtc_x;
-	win.crtc_y = state->crtc_y;
-	win.crtc_w = state->crtc_w;
-	win.crtc_h = state->crtc_h;
-
-	/*
-	 * src values are in Q16 fixed point, convert to integer.
-	 * omap_framebuffer_update_scanout() takes adjusted src.
-	 */
-	win.src_x = state->src_x >> 16;
-	win.src_y = state->src_y >> 16;
-
-	if (drm_rotation_90_or_270(state->rotation)) {
-		win.src_w = state->src_h >> 16;
-		win.src_h = state->src_w >> 16;
-	} else {
-		win.src_w = state->src_w >> 16;
-		win.src_h = state->src_h >> 16;
-	}
-
 	/* update scanout: */
-	omap_framebuffer_update_scanout(state->fb, &win, &info);
+	omap_framebuffer_update_scanout(state->fb, state, &info);
 
 	DBG("%dx%d -> %dx%d (%d)", info.width, info.height,
 			info.out_width, info.out_height,
-- 
2.7.4

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

Re: [PATCH 2/7] drm/omap: remove omap_drm_win

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Wednesday 17 May 2017 10:56:39 Tomi Valkeinen wrote:
> struct omap_drm_window is only used to pass plane setup data to
> omap_framebuffer_update_scanout(). This can as well be accomplished by
> just passing the DRM state.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_drv.h   | 11 +----------
>  drivers/gpu/drm/omapdrm/omap_fb.c    | 35 +++++++++++++++-----------------
>  drivers/gpu/drm/omapdrm/omap_plane.c | 25 +------------------------
>  3 files changed, 20 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h
> b/drivers/gpu/drm/omapdrm/omap_drv.h index ca087a993909..4bd1e9070b31
> 100644

[snip]

> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c
> b/drivers/gpu/drm/omapdrm/omap_fb.c index 4fc5db5d2d29..b7e7038cd2ce 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fb.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c

[snip]

> @@ -164,25 +164,27 @@ void omap_framebuffer_update_scanout(struct
> drm_framebuffer *fb,
> 
>  	info->fourcc = fb->format->format;
> 
> -	info->pos_x      = win->crtc_x;
> -	info->pos_y      = win->crtc_y;
> -	info->out_width  = win->crtc_w;
> -	info->out_height = win->crtc_h;
> -	info->width      = win->src_w;
> -	info->height     = win->src_h;
> +	info->pos_x      = state->crtc_x;
> +	info->pos_y      = state->crtc_y;
> +	info->out_width  = state->crtc_w;
> +	info->out_height = state->crtc_h;
> +	info->width      = state->src_w >> 16;
> +	info->height     = state->src_h >> 16;
> 
> -	x = win->src_x;
> -	y = win->src_y;
> +	/* DSS driver wants the w & h in rotated orientation */
> +	if (drm_rotation_90_or_270(state->rotation))
> +		swap(info->width, info->height);
> +
> +	x = state->src_x >> 16;
> +	y = state->src_y >> 16;
> 
>  	if (omap_gem_flags(plane->bo) & OMAP_BO_TILED) {
> -		uint32_t w = win->src_w;
> -		uint32_t h = win->src_h;
> +		uint32_t w = state->src_w >> 16;
> +		uint32_t h = state->src_h >> 16;
> 
> -		orient = drm_rotation_to_tiler(win->rotation);
> +		orient = drm_rotation_to_tiler(state->rotation);
> 
>  		/* adjust x,y offset for flip/invert: */

s/flip\///

Apart from that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> -		if (orient & MASK_XY_FLIP)
> -			swap(w, h);
>  		if (orient & MASK_Y_INVERT)
>  			y += h - 1;
>  		if (orient & MASK_X_INVERT)

-- 
Regards,

Laurent Pinchart

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

[PATCH 3/7] drm/omap: use DRM_ROTATE_* instead of OMAP_DSS_ROT_*

[Tomi Valkeinen]

At the moment the dispc driver uses a custom enum for rotation. Change
it to use the DRM's DRM_ROTATE_*.

Note that mirroring is at the moment handled as a separate boolean in
the dispc driver, so we only use the DRM_ROTATE_* values.

Note, DSS HW uses clockwise rotation, DRM counter-clockwise.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/dispc.c   | 46 +++++++++++++++++------------------
 drivers/gpu/drm/omapdrm/dss/omapdss.h |  8 ------
 drivers/gpu/drm/omapdrm/omap_plane.c  |  2 +-
 3 files changed, 23 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
index 7ccbcfc1d011..612170a96bdd 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -41,6 +41,7 @@
 #include <linux/of.h>
 #include <linux/component.h>
 #include <drm/drm_fourcc.h>
+#include <drm/drm_blend.h>
 
 #include "omapdss.h"
 #include "dss.h"
@@ -1600,22 +1601,20 @@ static void dispc_ovl_set_accu_uv(enum omap_plane_id plane,
 		{  0, 1, 0, 1, -1, 1, 0, 1 },
 	};
 
-	switch (rotation) {
-	case OMAP_DSS_ROT_0:
+	switch (rotation & DRM_ROTATE_MASK) {
+	default:
+	case DRM_ROTATE_0:
 		idx = 0;
 		break;
-	case OMAP_DSS_ROT_90:
+	case DRM_ROTATE_270:
 		idx = 1;
 		break;
-	case OMAP_DSS_ROT_180:
+	case DRM_ROTATE_180:
 		idx = 2;
 		break;
-	case OMAP_DSS_ROT_270:
+	case DRM_ROTATE_90:
 		idx = 3;
 		break;
-	default:
-		BUG();
-		return;
 	}
 
 	switch (fourcc) {
@@ -1742,8 +1741,7 @@ static void dispc_ovl_set_scaling_uv(enum omap_plane_id plane,
 	case DRM_FORMAT_YUYV:
 	case DRM_FORMAT_UYVY:
 		/* For YUV422 with 90/270 rotation, we don't upsample chroma */
-		if (rotation == OMAP_DSS_ROT_0 ||
-				rotation == OMAP_DSS_ROT_180) {
+		if (!drm_rotation_90_or_270(rotation)) {
 			if (chroma_upscale)
 				/* UV is subsampled by 2 horizontally */
 				orig_width >>= 1;
@@ -1753,7 +1751,7 @@ static void dispc_ovl_set_scaling_uv(enum omap_plane_id plane,
 		}
 
 		/* must use FIR for YUV422 if rotated */
-		if (rotation != OMAP_DSS_ROT_0)
+		if (rotation != DRM_ROTATE_0)
 			scale_x = scale_y = true;
 
 		break;
@@ -1815,38 +1813,38 @@ static void dispc_ovl_set_rotation_attrs(enum omap_plane_id plane, u8 rotation,
 	if (fourcc == DRM_FORMAT_YUYV || fourcc == DRM_FORMAT_UYVY) {
 
 		if (mirroring) {
-			switch (rotation) {
-			case OMAP_DSS_ROT_0:
+			switch (rotation & DRM_ROTATE_MASK) {
+			case DRM_ROTATE_0:
 				vidrot = 2;
 				break;
-			case OMAP_DSS_ROT_90:
+			case DRM_ROTATE_270:
 				vidrot = 1;
 				break;
-			case OMAP_DSS_ROT_180:
+			case DRM_ROTATE_180:
 				vidrot = 0;
 				break;
-			case OMAP_DSS_ROT_270:
+			case DRM_ROTATE_90:
 				vidrot = 3;
 				break;
 			}
 		} else {
-			switch (rotation) {
-			case OMAP_DSS_ROT_0:
+			switch (rotation & DRM_ROTATE_MASK) {
+			case DRM_ROTATE_0:
 				vidrot = 0;
 				break;
-			case OMAP_DSS_ROT_90:
+			case DRM_ROTATE_270:
 				vidrot = 1;
 				break;
-			case OMAP_DSS_ROT_180:
+			case DRM_ROTATE_180:
 				vidrot = 2;
 				break;
-			case OMAP_DSS_ROT_270:
+			case DRM_ROTATE_90:
 				vidrot = 3;
 				break;
 			}
 		}
 
-		if (rotation == OMAP_DSS_ROT_90 || rotation == OMAP_DSS_ROT_270)
+		if (drm_rotation_90_or_270(rotation))
 			row_repeat = true;
 		else
 			row_repeat = false;
@@ -1869,7 +1867,7 @@ static void dispc_ovl_set_rotation_attrs(enum omap_plane_id plane, u8 rotation,
 		bool doublestride =
 			fourcc == DRM_FORMAT_NV12 &&
 			rotation_type == OMAP_DSS_ROT_TILER &&
-			(rotation == OMAP_DSS_ROT_0 || rotation == OMAP_DSS_ROT_180);
+			!drm_rotation_90_or_270(rotation);
 
 		/* DOUBLESTRIDE */
 		REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), doublestride, 22, 22);
@@ -3916,7 +3914,7 @@ static const struct dispc_errata_i734_data {
 		.screen_width = 1,
 		.width = 1, .height = 1,
 		.fourcc = DRM_FORMAT_XRGB8888,
-		.rotation = OMAP_DSS_ROT_0,
+		.rotation = DRM_ROTATE_0,
 		.rotation_type = OMAP_DSS_ROT_NONE,
 		.mirror = 0,
 		.pos_x = 0, .pos_y = 0,
diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
index b4bd070bab33..daf792496882 100644
--- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
+++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
@@ -145,14 +145,6 @@ enum omap_dss_rotation_type {
 	OMAP_DSS_ROT_TILER	= 1 << 0,
 };
 
-/* clockwise rotation angle */
-enum omap_dss_rotation_angle {
-	OMAP_DSS_ROT_0   = 0,
-	OMAP_DSS_ROT_90  = 1,
-	OMAP_DSS_ROT_180 = 2,
-	OMAP_DSS_ROT_270 = 3,
-};
-
 enum omap_overlay_caps {
 	OMAP_DSS_OVL_CAP_SCALE = 1 << 0,
 	OMAP_DSS_OVL_CAP_GLOBAL_ALPHA = 1 << 1,
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
index 08644326f7eb..0ea97aa15c19 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -65,7 +65,7 @@ static void omap_plane_atomic_update(struct drm_plane *plane,
 
 	memset(&info, 0, sizeof(info));
 	info.rotation_type = OMAP_DSS_ROT_NONE;
-	info.rotation = OMAP_DSS_ROT_0;
+	info.rotation = DRM_ROTATE_0;
 	info.global_alpha = 0xff;
 	info.mirror = 0;
 	info.zorder = state->zpos;
-- 
2.7.4

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

Re: [PATCH 3/7] drm/omap: use DRM_ROTATE_* instead of OMAP_DSS_ROT_*

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Wednesday 17 May 2017 10:56:40 Tomi Valkeinen wrote:
> At the moment the dispc driver uses a custom enum for rotation. Change
> it to use the DRM's DRM_ROTATE_*.
> 
> Note that mirroring is at the moment handled as a separate boolean in
> the dispc driver, so we only use the DRM_ROTATE_* values.
> 
> Note, DSS HW uses clockwise rotation, DRM counter-clockwise.

I've tried to review this patch by checking how the driver converts from DRM 
rotation to DSS rotation, but unless I'm mistaken the only entry point to the 
DSS where rotation is passed is through the .ovl_setup() operation, and info-
>rotation doesn't seem to ever be set to a non-zero value. Am I missing 
something or is the rotation code in DSS actually not needed ?

It's hard to review if the driver does the right thing by checking how input 
values are handled before and after the patch when the only input value ever 
set is 0 :-) However, I see no issue in the patch itself, so

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/dispc.c   | 46 +++++++++++++++----------------
>  drivers/gpu/drm/omapdrm/dss/omapdss.h |  8 ------
>  drivers/gpu/drm/omapdrm/omap_plane.c  |  2 +-
>  3 files changed, 23 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c
> b/drivers/gpu/drm/omapdrm/dss/dispc.c index 7ccbcfc1d011..612170a96bdd
> 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> @@ -41,6 +41,7 @@
>  #include <linux/of.h>
>  #include <linux/component.h>
>  #include <drm/drm_fourcc.h>
> +#include <drm/drm_blend.h>
> 
>  #include "omapdss.h"
>  #include "dss.h"
> @@ -1600,22 +1601,20 @@ static void dispc_ovl_set_accu_uv(enum omap_plane_id
> plane, {  0, 1, 0, 1, -1, 1, 0, 1 },
>  	};
> 
> -	switch (rotation) {
> -	case OMAP_DSS_ROT_0:
> +	switch (rotation & DRM_ROTATE_MASK) {
> +	default:
> +	case DRM_ROTATE_0:
>  		idx = 0;
>  		break;
> -	case OMAP_DSS_ROT_90:
> +	case DRM_ROTATE_270:
>  		idx = 1;
>  		break;
> -	case OMAP_DSS_ROT_180:
> +	case DRM_ROTATE_180:
>  		idx = 2;
>  		break;
> -	case OMAP_DSS_ROT_270:
> +	case DRM_ROTATE_90:
>  		idx = 3;
>  		break;
> -	default:
> -		BUG();
> -		return;
>  	}
> 
>  	switch (fourcc) {
> @@ -1742,8 +1741,7 @@ static void dispc_ovl_set_scaling_uv(enum
> omap_plane_id plane, case DRM_FORMAT_YUYV:
>  	case DRM_FORMAT_UYVY:
>  		/* For YUV422 with 90/270 rotation, we don't upsample chroma 
*/
> -		if (rotation == OMAP_DSS_ROT_0 ||
> -				rotation == OMAP_DSS_ROT_180) {
> +		if (!drm_rotation_90_or_270(rotation)) {
>  			if (chroma_upscale)
>  				/* UV is subsampled by 2 horizontally */
>  				orig_width >>= 1;
> @@ -1753,7 +1751,7 @@ static void dispc_ovl_set_scaling_uv(enum
> omap_plane_id plane, }
> 
>  		/* must use FIR for YUV422 if rotated */
> -		if (rotation != OMAP_DSS_ROT_0)
> +		if (rotation != DRM_ROTATE_0)
>  			scale_x = scale_y = true;
> 
>  		break;
> @@ -1815,38 +1813,38 @@ static void dispc_ovl_set_rotation_attrs(enum
> omap_plane_id plane, u8 rotation, if (fourcc == DRM_FORMAT_YUYV || fourcc
> == DRM_FORMAT_UYVY) {
> 
>  		if (mirroring) {
> -			switch (rotation) {
> -			case OMAP_DSS_ROT_0:
> +			switch (rotation & DRM_ROTATE_MASK) {
> +			case DRM_ROTATE_0:
>  				vidrot = 2;
>  				break;
> -			case OMAP_DSS_ROT_90:
> +			case DRM_ROTATE_270:
>  				vidrot = 1;
>  				break;
> -			case OMAP_DSS_ROT_180:
> +			case DRM_ROTATE_180:
>  				vidrot = 0;
>  				break;
> -			case OMAP_DSS_ROT_270:
> +			case DRM_ROTATE_90:
>  				vidrot = 3;
>  				break;
>  			}
>  		} else {
> -			switch (rotation) {
> -			case OMAP_DSS_ROT_0:
> +			switch (rotation & DRM_ROTATE_MASK) {
> +			case DRM_ROTATE_0:
>  				vidrot = 0;
>  				break;
> -			case OMAP_DSS_ROT_90:
> +			case DRM_ROTATE_270:
>  				vidrot = 1;
>  				break;
> -			case OMAP_DSS_ROT_180:
> +			case DRM_ROTATE_180:
>  				vidrot = 2;
>  				break;
> -			case OMAP_DSS_ROT_270:
> +			case DRM_ROTATE_90:
>  				vidrot = 3;
>  				break;
>  			}
>  		}
> 
> -		if (rotation == OMAP_DSS_ROT_90 || rotation == 
OMAP_DSS_ROT_270)
> +		if (drm_rotation_90_or_270(rotation))
>  			row_repeat = true;
>  		else
>  			row_repeat = false;
> @@ -1869,7 +1867,7 @@ static void dispc_ovl_set_rotation_attrs(enum
> omap_plane_id plane, u8 rotation, bool doublestride =
>  			fourcc == DRM_FORMAT_NV12 &&
>  			rotation_type == OMAP_DSS_ROT_TILER &&
> -			(rotation == OMAP_DSS_ROT_0 || rotation == 
OMAP_DSS_ROT_180);
> +			!drm_rotation_90_or_270(rotation);
> 
>  		/* DOUBLESTRIDE */
>  		REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), doublestride, 22, 
22);
> @@ -3916,7 +3914,7 @@ static const struct dispc_errata_i734_data {
>  		.screen_width = 1,
>  		.width = 1, .height = 1,
>  		.fourcc = DRM_FORMAT_XRGB8888,
> -		.rotation = OMAP_DSS_ROT_0,
> +		.rotation = DRM_ROTATE_0,
>  		.rotation_type = OMAP_DSS_ROT_NONE,
>  		.mirror = 0,
>  		.pos_x = 0, .pos_y = 0,
> diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> b/drivers/gpu/drm/omapdrm/dss/omapdss.h index b4bd070bab33..daf792496882
> 100644
> --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> @@ -145,14 +145,6 @@ enum omap_dss_rotation_type {
>  	OMAP_DSS_ROT_TILER	= 1 << 0,
>  };
> 
> -/* clockwise rotation angle */
> -enum omap_dss_rotation_angle {
> -	OMAP_DSS_ROT_0   = 0,
> -	OMAP_DSS_ROT_90  = 1,
> -	OMAP_DSS_ROT_180 = 2,
> -	OMAP_DSS_ROT_270 = 3,
> -};
> -
>  enum omap_overlay_caps {
>  	OMAP_DSS_OVL_CAP_SCALE = 1 << 0,
>  	OMAP_DSS_OVL_CAP_GLOBAL_ALPHA = 1 << 1,
> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c
> b/drivers/gpu/drm/omapdrm/omap_plane.c index 08644326f7eb..0ea97aa15c19
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> @@ -65,7 +65,7 @@ static void omap_plane_atomic_update(struct drm_plane
> *plane,
> 
>  	memset(&info, 0, sizeof(info));
>  	info.rotation_type = OMAP_DSS_ROT_NONE;
> -	info.rotation = OMAP_DSS_ROT_0;
> +	info.rotation = DRM_ROTATE_0;
>  	info.global_alpha = 0xff;
>  	info.mirror = 0;
>  	info.zorder = state->zpos;

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 3/7] drm/omap: use DRM_ROTATE_* instead of OMAP_DSS_ROT_*

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 1547 bytes --]

On 23/05/17 16:07, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Wednesday 17 May 2017 10:56:40 Tomi Valkeinen wrote:
>> At the moment the dispc driver uses a custom enum for rotation. Change
>> it to use the DRM's DRM_ROTATE_*.
>>
>> Note that mirroring is at the moment handled as a separate boolean in
>> the dispc driver, so we only use the DRM_ROTATE_* values.
>>
>> Note, DSS HW uses clockwise rotation, DRM counter-clockwise.
> 
> I've tried to review this patch by checking how the driver converts from DRM 
> rotation to DSS rotation, but unless I'm mistaken the only entry point to the 
> DSS where rotation is passed is through the .ovl_setup() operation, and info-
>> rotation doesn't seem to ever be set to a non-zero value. Am I missing 
> something or is the rotation code in DSS actually not needed ?
> 
> It's hard to review if the driver does the right thing by checking how input 
> values are handled before and after the patch when the only input value ever 
> set is 0 :-) However, I see no issue in the patch itself, so

With tiler, in many cases (RGB) the DSS doesn't have to care about the
angle. But with YUV422 (in the following patches) we do need that
information in the DSS.

DMA and VRFB rotation used the rotation value, if I recall right.

Also, omapdrm was broken and never passed the angle (fixed in later patch).

So, for this patch, please ignore the all the oddness with rotation
code, just look at changing the custom enum to drm's =).

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

[PATCH 4/7] drm/omap: DRM_REFLECT_* instead of mirror boolean

[Tomi Valkeinen]

Change dispc driver to use the DRM_REFLECT flags instead of a mirror
boolean.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/dispc.c   | 24 ++++++++++--------------
 drivers/gpu/drm/omapdrm/dss/omapdss.h |  2 --
 drivers/gpu/drm/omapdrm/omap_plane.c  |  1 -
 3 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
index 612170a96bdd..a25db6e25165 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -1804,15 +1804,14 @@ static void dispc_ovl_set_scaling(enum omap_plane_id plane,
 }
 
 static void dispc_ovl_set_rotation_attrs(enum omap_plane_id plane, u8 rotation,
-		enum omap_dss_rotation_type rotation_type,
-		bool mirroring, u32 fourcc)
+		enum omap_dss_rotation_type rotation_type, u32 fourcc)
 {
 	bool row_repeat = false;
 	int vidrot = 0;
 
 	if (fourcc == DRM_FORMAT_YUYV || fourcc == DRM_FORMAT_UYVY) {
 
-		if (mirroring) {
+		if (rotation & DRM_REFLECT_X) {
 			switch (rotation & DRM_ROTATE_MASK) {
 			case DRM_ROTATE_0:
 				vidrot = 2;
@@ -2367,7 +2366,7 @@ static int dispc_ovl_setup_common(enum omap_plane_id plane,
 		enum omap_overlay_caps caps, u32 paddr, u32 p_uv_addr,
 		u16 screen_width, int pos_x, int pos_y, u16 width, u16 height,
 		u16 out_width, u16 out_height, u32 fourcc,
-		u8 rotation, bool mirror, u8 zorder, u8 pre_mult_alpha,
+		u8 rotation, u8 zorder, u8 pre_mult_alpha,
 		u8 global_alpha, enum omap_dss_rotation_type rotation_type,
 		bool replication, const struct videomode *vm,
 		bool mem_to_mem)
@@ -2515,8 +2514,7 @@ static int dispc_ovl_setup_common(enum omap_plane_id plane,
 		dispc_ovl_set_vid_color_conv(plane, cconv);
 	}
 
-	dispc_ovl_set_rotation_attrs(plane, rotation, rotation_type, mirror,
-			fourcc);
+	dispc_ovl_set_rotation_attrs(plane, rotation, rotation_type, fourcc);
 
 	dispc_ovl_set_zorder(plane, caps, zorder);
 	dispc_ovl_set_pre_mult_alpha(plane, caps, pre_mult_alpha);
@@ -2537,17 +2535,17 @@ static int dispc_ovl_setup(enum omap_plane_id plane,
 	const bool replication = true;
 
 	DSSDBG("dispc_ovl_setup %d, pa %pad, pa_uv %pad, sw %d, %d,%d, %dx%d ->"
-		" %dx%d, cmode %x, rot %d, mir %d, chan %d repl %d\n",
+		" %dx%d, cmode %x, rot %d, chan %d repl %d\n",
 		plane, &oi->paddr, &oi->p_uv_addr, oi->screen_width, oi->pos_x,
 		oi->pos_y, oi->width, oi->height, oi->out_width, oi->out_height,
-		oi->fourcc, oi->rotation, oi->mirror, channel, replication);
+		oi->fourcc, oi->rotation, channel, replication);
 
 	dispc_ovl_set_channel_out(plane, channel);
 
 	r = dispc_ovl_setup_common(plane, caps, oi->paddr, oi->p_uv_addr,
 		oi->screen_width, oi->pos_x, oi->pos_y, oi->width, oi->height,
 		oi->out_width, oi->out_height, oi->fourcc, oi->rotation,
-		oi->mirror, oi->zorder, oi->pre_mult_alpha, oi->global_alpha,
+		oi->zorder, oi->pre_mult_alpha, oi->global_alpha,
 		oi->rotation_type, replication, vm, mem_to_mem);
 
 	return r;
@@ -2569,13 +2567,12 @@ int dispc_wb_setup(const struct omap_dss_writeback_info *wi,
 		OMAP_DSS_OVL_CAP_SCALE | OMAP_DSS_OVL_CAP_PRE_MULT_ALPHA;
 
 	DSSDBG("dispc_wb_setup, pa %x, pa_uv %x, %d,%d -> %dx%d, cmode %x, "
-		"rot %d, mir %d\n", wi->paddr, wi->p_uv_addr, in_width,
-		in_height, wi->width, wi->height, wi->fourcc, wi->rotation,
-		wi->mirror);
+		"rot %d\n", wi->paddr, wi->p_uv_addr, in_width,
+		in_height, wi->width, wi->height, wi->fourcc, wi->rotation);
 
 	r = dispc_ovl_setup_common(plane, caps, wi->paddr, wi->p_uv_addr,
 		wi->buf_width, pos_x, pos_y, in_width, in_height, wi->width,
-		wi->height, wi->fourcc, wi->rotation, wi->mirror, zorder,
+		wi->height, wi->fourcc, wi->rotation, zorder,
 		wi->pre_mult_alpha, global_alpha, wi->rotation_type,
 		replication, vm, mem_to_mem);
 
@@ -3916,7 +3913,6 @@ static const struct dispc_errata_i734_data {
 		.fourcc = DRM_FORMAT_XRGB8888,
 		.rotation = DRM_ROTATE_0,
 		.rotation_type = OMAP_DSS_ROT_NONE,
-		.mirror = 0,
 		.pos_x = 0, .pos_y = 0,
 		.out_width = 0, .out_height = 0,
 		.global_alpha = 0xff,
diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
index daf792496882..e9d6b72eb69e 100644
--- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
+++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
@@ -259,7 +259,6 @@ struct omap_overlay_info {
 	u32 fourcc;
 	u8 rotation;
 	enum omap_dss_rotation_type rotation_type;
-	bool mirror;
 
 	u16 pos_x;
 	u16 pos_y;
@@ -307,7 +306,6 @@ struct omap_dss_writeback_info {
 	u32 fourcc;
 	u8 rotation;
 	enum omap_dss_rotation_type rotation_type;
-	bool mirror;
 	u8 pre_mult_alpha;
 };
 
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
index 0ea97aa15c19..9fe97c71763f 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -67,7 +67,6 @@ static void omap_plane_atomic_update(struct drm_plane *plane,
 	info.rotation_type = OMAP_DSS_ROT_NONE;
 	info.rotation = DRM_ROTATE_0;
 	info.global_alpha = 0xff;
-	info.mirror = 0;
 	info.zorder = state->zpos;
 
 	/* update scanout: */
-- 
2.7.4

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

Re: [PATCH 4/7] drm/omap: DRM_REFLECT_* instead of mirror boolean

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Wednesday 17 May 2017 10:56:41 Tomi Valkeinen wrote:
> Change dispc driver to use the DRM_REFLECT flags instead of a mirror
> boolean.

Patch 3/7 has

		/* must use FIR for YUV422 if rotated */
		if (rotation != DRM_ROTATE_0)
			scale_x = scale_y = true;

Shouldn't it be (rotation & DRM_ROTATE_MASK) != DRM_ROTATE_0 now ?

Apart from that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/dispc.c   | 24 ++++++++++--------------
>  drivers/gpu/drm/omapdrm/dss/omapdss.h |  2 --
>  drivers/gpu/drm/omapdrm/omap_plane.c  |  1 -
>  3 files changed, 10 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c
> b/drivers/gpu/drm/omapdrm/dss/dispc.c index 612170a96bdd..a25db6e25165
> 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> @@ -1804,15 +1804,14 @@ static void dispc_ovl_set_scaling(enum omap_plane_id
> plane, }
> 
>  static void dispc_ovl_set_rotation_attrs(enum omap_plane_id plane, u8
> rotation, -		enum omap_dss_rotation_type rotation_type,
> -		bool mirroring, u32 fourcc)
> +		enum omap_dss_rotation_type rotation_type, u32 fourcc)
>  {
>  	bool row_repeat = false;
>  	int vidrot = 0;
> 
>  	if (fourcc == DRM_FORMAT_YUYV || fourcc == DRM_FORMAT_UYVY) {
> 
> -		if (mirroring) {
> +		if (rotation & DRM_REFLECT_X) {
>  			switch (rotation & DRM_ROTATE_MASK) {
>  			case DRM_ROTATE_0:
>  				vidrot = 2;
> @@ -2367,7 +2366,7 @@ static int dispc_ovl_setup_common(enum omap_plane_id
> plane, enum omap_overlay_caps caps, u32 paddr, u32 p_uv_addr,
>  		u16 screen_width, int pos_x, int pos_y, u16 width, u16 height,
>  		u16 out_width, u16 out_height, u32 fourcc,
> -		u8 rotation, bool mirror, u8 zorder, u8 pre_mult_alpha,
> +		u8 rotation, u8 zorder, u8 pre_mult_alpha,
>  		u8 global_alpha, enum omap_dss_rotation_type rotation_type,
>  		bool replication, const struct videomode *vm,
>  		bool mem_to_mem)
> @@ -2515,8 +2514,7 @@ static int dispc_ovl_setup_common(enum omap_plane_id
> plane, dispc_ovl_set_vid_color_conv(plane, cconv);
>  	}
> 
> -	dispc_ovl_set_rotation_attrs(plane, rotation, rotation_type, mirror,
> -			fourcc);
> +	dispc_ovl_set_rotation_attrs(plane, rotation, rotation_type, fourcc);
> 
>  	dispc_ovl_set_zorder(plane, caps, zorder);
>  	dispc_ovl_set_pre_mult_alpha(plane, caps, pre_mult_alpha);
> @@ -2537,17 +2535,17 @@ static int dispc_ovl_setup(enum omap_plane_id plane,
> const bool replication = true;
> 
>  	DSSDBG("dispc_ovl_setup %d, pa %pad, pa_uv %pad, sw %d, %d,%d, %dx%d -
>"
> -		" %dx%d, cmode %x, rot %d, mir %d, chan %d repl %d\n",
> +		" %dx%d, cmode %x, rot %d, chan %d repl %d\n",
>  		plane, &oi->paddr, &oi->p_uv_addr, oi->screen_width, oi-
>pos_x,
>  		oi->pos_y, oi->width, oi->height, oi->out_width, oi-
>out_height,
> -		oi->fourcc, oi->rotation, oi->mirror, channel, replication);
> +		oi->fourcc, oi->rotation, channel, replication);
> 
>  	dispc_ovl_set_channel_out(plane, channel);
> 
>  	r = dispc_ovl_setup_common(plane, caps, oi->paddr, oi->p_uv_addr,
>  		oi->screen_width, oi->pos_x, oi->pos_y, oi->width, oi->height,
>  		oi->out_width, oi->out_height, oi->fourcc, oi->rotation,
> -		oi->mirror, oi->zorder, oi->pre_mult_alpha, oi->global_alpha,
> +		oi->zorder, oi->pre_mult_alpha, oi->global_alpha,
>  		oi->rotation_type, replication, vm, mem_to_mem);
> 
>  	return r;
> @@ -2569,13 +2567,12 @@ int dispc_wb_setup(const struct
> omap_dss_writeback_info *wi, OMAP_DSS_OVL_CAP_SCALE |
> OMAP_DSS_OVL_CAP_PRE_MULT_ALPHA;
> 
>  	DSSDBG("dispc_wb_setup, pa %x, pa_uv %x, %d,%d -> %dx%d, cmode %x, "
> -		"rot %d, mir %d\n", wi->paddr, wi->p_uv_addr, in_width,
> -		in_height, wi->width, wi->height, wi->fourcc, wi->rotation,
> -		wi->mirror);
> +		"rot %d\n", wi->paddr, wi->p_uv_addr, in_width,
> +		in_height, wi->width, wi->height, wi->fourcc, wi->rotation);
> 
>  	r = dispc_ovl_setup_common(plane, caps, wi->paddr, wi->p_uv_addr,
>  		wi->buf_width, pos_x, pos_y, in_width, in_height, wi->width,
> -		wi->height, wi->fourcc, wi->rotation, wi->mirror, zorder,
> +		wi->height, wi->fourcc, wi->rotation, zorder,
>  		wi->pre_mult_alpha, global_alpha, wi->rotation_type,
>  		replication, vm, mem_to_mem);
> 
> @@ -3916,7 +3913,6 @@ static const struct dispc_errata_i734_data {
>  		.fourcc = DRM_FORMAT_XRGB8888,
>  		.rotation = DRM_ROTATE_0,
>  		.rotation_type = OMAP_DSS_ROT_NONE,
> -		.mirror = 0,
>  		.pos_x = 0, .pos_y = 0,
>  		.out_width = 0, .out_height = 0,
>  		.global_alpha = 0xff,
> diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> b/drivers/gpu/drm/omapdrm/dss/omapdss.h index daf792496882..e9d6b72eb69e
> 100644
> --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> @@ -259,7 +259,6 @@ struct omap_overlay_info {
>  	u32 fourcc;
>  	u8 rotation;
>  	enum omap_dss_rotation_type rotation_type;
> -	bool mirror;
> 
>  	u16 pos_x;
>  	u16 pos_y;
> @@ -307,7 +306,6 @@ struct omap_dss_writeback_info {
>  	u32 fourcc;
>  	u8 rotation;
>  	enum omap_dss_rotation_type rotation_type;
> -	bool mirror;
>  	u8 pre_mult_alpha;
>  };
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c
> b/drivers/gpu/drm/omapdrm/omap_plane.c index 0ea97aa15c19..9fe97c71763f
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> @@ -67,7 +67,6 @@ static void omap_plane_atomic_update(struct drm_plane
> *plane, info.rotation_type = OMAP_DSS_ROT_NONE;
>  	info.rotation = DRM_ROTATE_0;
>  	info.global_alpha = 0xff;
> -	info.mirror = 0;
>  	info.zorder = state->zpos;
> 
>  	/* update scanout: */

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 4/7] drm/omap: DRM_REFLECT_* instead of mirror boolean

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 491 bytes --]

On 23/05/17 16:15, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Wednesday 17 May 2017 10:56:41 Tomi Valkeinen wrote:
>> Change dispc driver to use the DRM_REFLECT flags instead of a mirror
>> boolean.
> 
> Patch 3/7 has
> 
> 		/* must use FIR for YUV422 if rotated */
> 		if (rotation != DRM_ROTATE_0)
> 			scale_x = scale_y = true;
> 
> Shouldn't it be (rotation & DRM_ROTATE_MASK) != DRM_ROTATE_0 now ?

Yep, I'll fix that. Thanks!

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

[PATCH 5/7] drm/omap: pass rotation to dispc

[Tomi Valkeinen]

The omapdrm driver has not passed the rotation value to the dispc
driver. This doesn't affect RGB formats, but YUV formats don't work
without dispc knowing the orientation.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_fb.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
index b7e7038cd2ce..bd05976fc20b 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.c
+++ b/drivers/gpu/drm/omapdrm/omap_fb.c
@@ -193,6 +193,7 @@ void omap_framebuffer_update_scanout(struct drm_framebuffer *fb,
 		omap_gem_rotated_dma_addr(plane->bo, orient, x, y,
 					  &info->paddr);
 		info->rotation_type = OMAP_DSS_ROT_TILER;
+		info->rotation = state->rotation ?: DRM_ROTATE_0;
 		info->screen_width  = omap_gem_tiled_stride(plane->bo, orient);
 	} else {
 		switch (state->rotation & DRM_ROTATE_MASK) {
@@ -210,6 +211,7 @@ void omap_framebuffer_update_scanout(struct drm_framebuffer *fb,
 
 		info->paddr         = get_linear_addr(plane, format, 0, x, y);
 		info->rotation_type = OMAP_DSS_ROT_NONE;
+		info->rotation      = DRM_ROTATE_0;
 		info->screen_width  = plane->pitch;
 	}
 
-- 
2.7.4

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

Re: [PATCH 5/7] drm/omap: pass rotation to dispc

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Wednesday 17 May 2017 10:56:42 Tomi Valkeinen wrote:
> The omapdrm driver has not passed the rotation value to the dispc
> driver. This doesn't affect RGB formats, but YUV formats don't work
> without dispc knowing the orientation.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

I assume you've tested this patch series with TILER rotation, right ? If so,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/omapdrm/omap_fb.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c
> b/drivers/gpu/drm/omapdrm/omap_fb.c index b7e7038cd2ce..bd05976fc20b 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fb.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
> @@ -193,6 +193,7 @@ void omap_framebuffer_update_scanout(struct
> drm_framebuffer *fb, omap_gem_rotated_dma_addr(plane->bo, orient, x, y,
>  					  &info->paddr);
>  		info->rotation_type = OMAP_DSS_ROT_TILER;
> +		info->rotation = state->rotation ?: DRM_ROTATE_0;
>  		info->screen_width  = omap_gem_tiled_stride(plane->bo, 
orient);
>  	} else {
>  		switch (state->rotation & DRM_ROTATE_MASK) {
> @@ -210,6 +211,7 @@ void omap_framebuffer_update_scanout(struct
> drm_framebuffer *fb,
> 
>  		info->paddr         = get_linear_addr(plane, format, 0, x, y);
>  		info->rotation_type = OMAP_DSS_ROT_NONE;
> +		info->rotation      = DRM_ROTATE_0;
>  		info->screen_width  = plane->pitch;
>  	}

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 5/7] drm/omap: pass rotation to dispc

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 562 bytes --]



On 23/05/17 17:06, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Wednesday 17 May 2017 10:56:42 Tomi Valkeinen wrote:
>> The omapdrm driver has not passed the rotation value to the dispc
>> driver. This doesn't affect RGB formats, but YUV formats don't work
>> without dispc knowing the orientation.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
> I assume you've tested this patch series with TILER rotation, right ? If so,

For RGB formats, yes. YUV formats need the following patches.

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

[PATCH 6/7] drm/omap: fix YUV422 rotation with TILER

[Tomi Valkeinen]

TILER rotation with YUV422 pixelformats does not work at the moment. All
other pixel formats work, because the pixelformat's pixel size is equal
to tiler unit size (e.g. XR24's pixel size is 32 bits, and the TILER
unit size that has to be used is 32 bits).

For YUV422 formats this is not the case, as the TILER unit size has to
be 32 bits, but the pixel size is 16 bits. The end result is OCP errors
and sync losts.

This patch adds the code to adjust the variables for YUV422 formats.

We could make the code more generic by passing around the pixel format,
rotation type, angle and the tiler unit size, which would allow us to do
calculations without special case for YUV422. However, this would make
the code more complex, and at least for now this is much more easier to
handle with these two special cases for YUV422.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/dispc.c | 20 ++++++++++++++++++--
 drivers/gpu/drm/omapdrm/omap_fb.c   | 14 ++++++++++++++
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
index a25db6e25165..80c75e5913cb 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -1917,7 +1917,8 @@ static s32 pixinc(int pixels, u8 ps)
 static void calc_offset(u16 screen_width, u16 width,
 		u32 fourcc, bool fieldmode,
 		unsigned int field_offset, unsigned *offset0, unsigned *offset1,
-		s32 *row_inc, s32 *pix_inc, int x_predecim, int y_predecim)
+		s32 *row_inc, s32 *pix_inc, int x_predecim, int y_predecim,
+		enum omap_dss_rotation_type rotation_type, u8 rotation)
 {
 	u8 ps;
 
@@ -1925,6 +1926,20 @@ static void calc_offset(u16 screen_width, u16 width,
 
 	DSSDBG("scrw %d, width %d\n", screen_width, width);
 
+	if (rotation_type == OMAP_DSS_ROT_TILER &&
+	    (fourcc == DRM_FORMAT_UYVY || fourcc == DRM_FORMAT_YUYV) &&
+	    drm_rotation_90_or_270(rotation)) {
+		/*
+		 * HACK: ROW_INC needs to be calculated with TILER units.
+		 * We get such 'screen_width' that multiplying it with the
+		 * YUV422 pixel size gives the correct TILER container width.
+		 * However, 'width' is in pixels and multiplying it with YUV422
+		 * pixel size gives incorrect result. We thus multiply it here
+		 * with 2 to match the 32 bit TILER unit size.
+		 */
+		width *= 2;
+	}
+
 	/*
 	 * field 0 = even field = bottom field
 	 * field 1 = odd field = top field
@@ -2473,7 +2488,8 @@ static int dispc_ovl_setup_common(enum omap_plane_id plane,
 	calc_offset(screen_width, frame_width,
 			fourcc, fieldmode, field_offset,
 			&offset0, &offset1, &row_inc, &pix_inc,
-			x_predecim, y_predecim);
+			x_predecim, y_predecim,
+			rotation_type, rotation);
 
 	DSSDBG("offset0 %u, offset1 %u, row_inc %d, pix_inc %d\n",
 			offset0, offset1, row_inc, pix_inc);
diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
index bd05976fc20b..e5cc13799e73 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.c
+++ b/drivers/gpu/drm/omapdrm/omap_fb.c
@@ -184,16 +184,30 @@ void omap_framebuffer_update_scanout(struct drm_framebuffer *fb,
 
 		orient = drm_rotation_to_tiler(state->rotation);
 
+		/*
+		 * omap_gem_rotated_paddr() wants the x & y in tiler units.
+		 * Usually tiler unit size is the same as the pixel size, except
+		 * for YUV422 formats, for which the tiler unit size is 32 bits
+		 * and pixel size is 16 bits.
+		 */
+		if (fb->format->format == DRM_FORMAT_UYVY ||
+				fb->format->format == DRM_FORMAT_YUYV) {
+			x /= 2;
+			w /= 2;
+		}
+
 		/* adjust x,y offset for flip/invert: */
 		if (orient & MASK_Y_INVERT)
 			y += h - 1;
 		if (orient & MASK_X_INVERT)
 			x += w - 1;
 
+		/* Note: x and y are in TILER units, not pixels */
 		omap_gem_rotated_dma_addr(plane->bo, orient, x, y,
 					  &info->paddr);
 		info->rotation_type = OMAP_DSS_ROT_TILER;
 		info->rotation = state->rotation ?: DRM_ROTATE_0;
+		/* Note: stride in TILER units, not pixels */
 		info->screen_width  = omap_gem_tiled_stride(plane->bo, orient);
 	} else {
 		switch (state->rotation & DRM_ROTATE_MASK) {
-- 
2.7.4

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

Re: [PATCH 6/7] drm/omap: fix YUV422 rotation with TILER

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Wednesday 17 May 2017 10:56:43 Tomi Valkeinen wrote:
> TILER rotation with YUV422 pixelformats does not work at the moment. All
> other pixel formats work, because the pixelformat's pixel size is equal
> to tiler unit size (e.g. XR24's pixel size is 32 bits, and the TILER
> unit size that has to be used is 32 bits).
> 
> For YUV422 formats this is not the case, as the TILER unit size has to
> be 32 bits, but the pixel size is 16 bits. The end result is OCP errors
> and sync losts.
> 
> This patch adds the code to adjust the variables for YUV422 formats.
> 
> We could make the code more generic by passing around the pixel format,
> rotation type, angle and the tiler unit size, which would allow us to do
> calculations without special case for YUV422. However, this would make
> the code more complex, and at least for now this is much more easier to
> handle with these two special cases for YUV422.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/dispc.c | 20 ++++++++++++++++++--
>  drivers/gpu/drm/omapdrm/omap_fb.c   | 14 ++++++++++++++
>  2 files changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c
> b/drivers/gpu/drm/omapdrm/dss/dispc.c index a25db6e25165..80c75e5913cb
> 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> @@ -1917,7 +1917,8 @@ static s32 pixinc(int pixels, u8 ps)
>  static void calc_offset(u16 screen_width, u16 width,
>  		u32 fourcc, bool fieldmode,
>  		unsigned int field_offset, unsigned *offset0, unsigned 
*offset1,
> -		s32 *row_inc, s32 *pix_inc, int x_predecim, int y_predecim)
> +		s32 *row_inc, s32 *pix_inc, int x_predecim, int y_predecim,
> +		enum omap_dss_rotation_type rotation_type, u8 rotation)
>  {
>  	u8 ps;
> 
> @@ -1925,6 +1926,20 @@ static void calc_offset(u16 screen_width, u16 width,
> 
>  	DSSDBG("scrw %d, width %d\n", screen_width, width);
> 
> +	if (rotation_type == OMAP_DSS_ROT_TILER &&
> +	    (fourcc == DRM_FORMAT_UYVY || fourcc == DRM_FORMAT_YUYV) &&
> +	    drm_rotation_90_or_270(rotation)) {
> +		/*
> +		 * HACK: ROW_INC needs to be calculated with TILER units.
> +		 * We get such 'screen_width' that multiplying it with the
> +		 * YUV422 pixel size gives the correct TILER container width.
> +		 * However, 'width' is in pixels and multiplying it with 
YUV422
> +		 * pixel size gives incorrect result. We thus multiply it here
> +		 * with 2 to match the 32 bit TILER unit size.
> +		 */
> +		width *= 2;
> +	}
> +
>  	/*
>  	 * field 0 = even field = bottom field
>  	 * field 1 = odd field = top field
> @@ -2473,7 +2488,8 @@ static int dispc_ovl_setup_common(enum omap_plane_id
> plane, calc_offset(screen_width, frame_width,
>  			fourcc, fieldmode, field_offset,
>  			&offset0, &offset1, &row_inc, &pix_inc,
> -			x_predecim, y_predecim);
> +			x_predecim, y_predecim,
> +			rotation_type, rotation);
> 
>  	DSSDBG("offset0 %u, offset1 %u, row_inc %d, pix_inc %d\n",
>  			offset0, offset1, row_inc, pix_inc);
> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c
> b/drivers/gpu/drm/omapdrm/omap_fb.c index bd05976fc20b..e5cc13799e73 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fb.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
> @@ -184,16 +184,30 @@ void omap_framebuffer_update_scanout(struct
> drm_framebuffer *fb,
> 
>  		orient = drm_rotation_to_tiler(state->rotation);
> 
> +		/*
> +		 * omap_gem_rotated_paddr() wants the x & y in tiler units.
> +		 * Usually tiler unit size is the same as the pixel size, 
except
> +		 * for YUV422 formats, for which the tiler unit size is 32 
bits
> +		 * and pixel size is 16 bits.
> +		 */
> +		if (fb->format->format == DRM_FORMAT_UYVY ||
> +				fb->format->format == DRM_FORMAT_YUYV) {

That's a very peculiar indentation.

> +			x /= 2;
> +			w /= 2;
> +		}
> +
>  		/* adjust x,y offset for flip/invert: */
>  		if (orient & MASK_Y_INVERT)
>  			y += h - 1;
>  		if (orient & MASK_X_INVERT)
>  			x += w - 1;
> 
> +		/* Note: x and y are in TILER units, not pixels */
>  		omap_gem_rotated_dma_addr(plane->bo, orient, x, y,
>  					  &info->paddr);
>  		info->rotation_type = OMAP_DSS_ROT_TILER;
>  		info->rotation = state->rotation ?: DRM_ROTATE_0;
> +		/* Note: stride in TILER units, not pixels */

Nitpicking, I would have combined the two comments.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  		info->screen_width  = omap_gem_tiled_stride(plane->bo, 
orient);
>  	} else {
>  		switch (state->rotation & DRM_ROTATE_MASK) {

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 6/7] drm/omap: fix YUV422 rotation with TILER

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 1695 bytes --]

On 24/05/17 09:44, Laurent Pinchart wrote:

>> b/drivers/gpu/drm/omapdrm/omap_fb.c index bd05976fc20b..e5cc13799e73 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_fb.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
>> @@ -184,16 +184,30 @@ void omap_framebuffer_update_scanout(struct
>> drm_framebuffer *fb,
>>
>>  		orient = drm_rotation_to_tiler(state->rotation);
>>
>> +		/*
>> +		 * omap_gem_rotated_paddr() wants the x & y in tiler units.
>> +		 * Usually tiler unit size is the same as the pixel size, 
> except
>> +		 * for YUV422 formats, for which the tiler unit size is 32 
> bits
>> +		 * and pixel size is 16 bits.
>> +		 */
>> +		if (fb->format->format == DRM_FORMAT_UYVY ||
>> +				fb->format->format == DRM_FORMAT_YUYV) {
> 
> That's a very peculiar indentation.

Well, not really if you don't want to mix tabs and spaces. If there was
just one tab on the second line, it would align with the lines below,
making it confusing.

> 
>> +			x /= 2;
>> +			w /= 2;
>> +		}
>> +
>>  		/* adjust x,y offset for flip/invert: */
>>  		if (orient & MASK_Y_INVERT)
>>  			y += h - 1;
>>  		if (orient & MASK_X_INVERT)
>>  			x += w - 1;
>>
>> +		/* Note: x and y are in TILER units, not pixels */
>>  		omap_gem_rotated_dma_addr(plane->bo, orient, x, y,
>>  					  &info->paddr);
>>  		info->rotation_type = OMAP_DSS_ROT_TILER;
>>  		info->rotation = state->rotation ?: DRM_ROTATE_0;
>> +		/* Note: stride in TILER units, not pixels */
> 
> Nitpicking, I would have combined the two comments.

Perhaps... I found myself mixing up pixels and tiler units all the time,
so I wanted to highlight the fact in the places where it's mixed up.

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH 6/7] drm/omap: fix YUV422 rotation with TILER

[Laurent Pinchart]

Hi Tomi,

On Wednesday 24 May 2017 09:50:49 Tomi Valkeinen wrote:
> On 24/05/17 09:44, Laurent Pinchart wrote:
> >> b/drivers/gpu/drm/omapdrm/omap_fb.c index bd05976fc20b..e5cc13799e73
> >> 100644
> >> --- a/drivers/gpu/drm/omapdrm/omap_fb.c
> >> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
> >> @@ -184,16 +184,30 @@ void omap_framebuffer_update_scanout(struct
> >> drm_framebuffer *fb,
> >> 
> >>  		orient = drm_rotation_to_tiler(state->rotation);
> >> 
> >> +		/*
> >> +		 * omap_gem_rotated_paddr() wants the x & y in tiler units.
> >> +		 * Usually tiler unit size is the same as the pixel size,
> >> except
> >> +		 * for YUV422 formats, for which the tiler unit size is 32
> >> bits
> >> +		 * and pixel size is 16 bits.
> >> +		 */
> >> +		if (fb->format->format == DRM_FORMAT_UYVY ||
> >> +				fb->format->format == DRM_FORMAT_YUYV) {
> > 
> > That's a very peculiar indentation.
> 
> Well, not really if you don't want to mix tabs and spaces. If there was
> just one tab on the second line, it would align with the lines below,
> making it confusing.

You mix tabs and places in other patches in this series ;-)

> >> +			x /= 2;
> >> +			w /= 2;
> >> +		}
> >> +
> >>  		/* adjust x,y offset for flip/invert: */
> >>  		if (orient & MASK_Y_INVERT)
> >>  			y += h - 1;
> >>  		
> >>  			x += w - 1;
> >> 
> >> +		/* Note: x and y are in TILER units, not pixels */
> >>  		omap_gem_rotated_dma_addr(plane->bo, orient, x, y,
> >>  					  &info->paddr);
> >>  		info->rotation_type = OMAP_DSS_ROT_TILER;
> >>  		info->rotation = state->rotation ?: DRM_ROTATE_0;
> >> +		/* Note: stride in TILER units, not pixels */
> > 
> > Nitpicking, I would have combined the two comments.
> 
> Perhaps... I found myself mixing up pixels and tiler units all the time,
> so I wanted to highlight the fact in the places where it's mixed up.

-- 
Regards,

Laurent Pinchart

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

[PATCH 7/7] drm/omap: fix YUV422 90/270 rotation with mirroring

[Tomi Valkeinen]

When rotating 90/270 + mirroring with YUV422, the end result will have
adjacent pixels swapped. The problem is that
dispc_ovl_set_rotation_attrs() has wrong rotation values for these
cases.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/dispc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
index 80c75e5913cb..7261f87b2a5b 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -1817,13 +1817,13 @@ static void dispc_ovl_set_rotation_attrs(enum omap_plane_id plane, u8 rotation,
 				vidrot = 2;
 				break;
 			case DRM_ROTATE_270:
-				vidrot = 1;
+				vidrot = 3;
 				break;
 			case DRM_ROTATE_180:
 				vidrot = 0;
 				break;
 			case DRM_ROTATE_90:
-				vidrot = 3;
+				vidrot = 1;
 				break;
 			}
 		} else {
-- 
2.7.4

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

Re: [PATCH 7/7] drm/omap: fix YUV422 90/270 rotation with mirroring

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Wednesday 17 May 2017 10:56:44 Tomi Valkeinen wrote:
> When rotating 90/270 + mirroring with YUV422, the end result will have
> adjacent pixels swapped. The problem is that
> dispc_ovl_set_rotation_attrs() has wrong rotation values for these
> cases.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/dispc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c
> b/drivers/gpu/drm/omapdrm/dss/dispc.c index 80c75e5913cb..7261f87b2a5b
> 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> @@ -1817,13 +1817,13 @@ static void dispc_ovl_set_rotation_attrs(enum
> omap_plane_id plane, u8 rotation, vidrot = 2;
>  				break;
>  			case DRM_ROTATE_270:
> -				vidrot = 1;
> +				vidrot = 3;
>  				break;
>  			case DRM_ROTATE_180:
>  				vidrot = 0;
>  				break;
>  			case DRM_ROTATE_90:
> -				vidrot = 3;
> +				vidrot = 1;

How about ordering the cases in 0, 90, 180, 270 order ? That would look 
cleaner for both the case label and the vidrot value. I would actually have 
done so in the patch where you replaced OMAP_DSS_ROT_* with DRM_ROTATE_*.

Apart from this,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  				break;
>  			}
>  		} else {

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 7/7] drm/omap: fix YUV422 90/270 rotation with mirroring

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 1719 bytes --]



On 24/05/17 09:46, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Wednesday 17 May 2017 10:56:44 Tomi Valkeinen wrote:
>> When rotating 90/270 + mirroring with YUV422, the end result will have
>> adjacent pixels swapped. The problem is that
>> dispc_ovl_set_rotation_attrs() has wrong rotation values for these
>> cases.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>  drivers/gpu/drm/omapdrm/dss/dispc.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c
>> b/drivers/gpu/drm/omapdrm/dss/dispc.c index 80c75e5913cb..7261f87b2a5b
>> 100644
>> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
>> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
>> @@ -1817,13 +1817,13 @@ static void dispc_ovl_set_rotation_attrs(enum
>> omap_plane_id plane, u8 rotation, vidrot = 2;
>>  				break;
>>  			case DRM_ROTATE_270:
>> -				vidrot = 1;
>> +				vidrot = 3;
>>  				break;
>>  			case DRM_ROTATE_180:
>>  				vidrot = 0;
>>  				break;
>>  			case DRM_ROTATE_90:
>> -				vidrot = 3;
>> +				vidrot = 1;
> 
> How about ordering the cases in 0, 90, 180, 270 order ? That would look 
> cleaner for both the case label and the vidrot value. I would actually have 
> done so in the patch where you replaced OMAP_DSS_ROT_* with DRM_ROTATE_*.

I thought about it, and I kind of agree. But... DSS rotates the other
way, so the cases are now in DSS's 0, 90, 180, 270 order. And if I'd
change the order, then the vidrot values for non-DRM_REFLECT_X (i.e. the
"normal) case would be in strange order.

I should probably add comments there that the DSS rotation is the other way.

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH 7/7] drm/omap: fix YUV422 90/270 rotation with mirroring

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 1494 bytes --]



On 24/05/17 09:46, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Wednesday 17 May 2017 10:56:44 Tomi Valkeinen wrote:
>> When rotating 90/270 + mirroring with YUV422, the end result will have
>> adjacent pixels swapped. The problem is that
>> dispc_ovl_set_rotation_attrs() has wrong rotation values for these
>> cases.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>  drivers/gpu/drm/omapdrm/dss/dispc.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c
>> b/drivers/gpu/drm/omapdrm/dss/dispc.c index 80c75e5913cb..7261f87b2a5b
>> 100644
>> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
>> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
>> @@ -1817,13 +1817,13 @@ static void dispc_ovl_set_rotation_attrs(enum
>> omap_plane_id plane, u8 rotation, vidrot = 2;
>>  				break;
>>  			case DRM_ROTATE_270:
>> -				vidrot = 1;
>> +				vidrot = 3;
>>  				break;
>>  			case DRM_ROTATE_180:
>>  				vidrot = 0;
>>  				break;
>>  			case DRM_ROTATE_90:
>> -				vidrot = 3;
>> +				vidrot = 1;
> 
> How about ordering the cases in 0, 90, 180, 270 order ? That would look 
> cleaner for both the case label and the vidrot value. I would actually have 
> done so in the patch where you replaced OMAP_DSS_ROT_* with DRM_ROTATE_*.

I now change the order in the patch you mention, and added comments
highlighting the different clockwiseness.

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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