| Message ID | 1495007804-6133-4-git-send-email-tomi.valkeinen@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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;
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
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;
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(-)