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

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;