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)

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)

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;

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

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

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