| Message ID | 1495007804-6133-8-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: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 {
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
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
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 {
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(-)