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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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