Re: [PATCH 6/7] drm/omap: fix YUV422 rotation with TILER

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

Hi Tomi,

Thank you for the patch.

On Wednesday 17 May 2017 10:56:43 Tomi Valkeinen wrote:
> TILER rotation with YUV422 pixelformats does not work at the moment. All
> other pixel formats work, because the pixelformat's pixel size is equal
> to tiler unit size (e.g. XR24's pixel size is 32 bits, and the TILER
> unit size that has to be used is 32 bits).
> 
> For YUV422 formats this is not the case, as the TILER unit size has to
> be 32 bits, but the pixel size is 16 bits. The end result is OCP errors
> and sync losts.
> 
> This patch adds the code to adjust the variables for YUV422 formats.
> 
> We could make the code more generic by passing around the pixel format,
> rotation type, angle and the tiler unit size, which would allow us to do
> calculations without special case for YUV422. However, this would make
> the code more complex, and at least for now this is much more easier to
> handle with these two special cases for YUV422.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/dispc.c | 20 ++++++++++++++++++--
>  drivers/gpu/drm/omapdrm/omap_fb.c   | 14 ++++++++++++++
>  2 files changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c
> b/drivers/gpu/drm/omapdrm/dss/dispc.c index a25db6e25165..80c75e5913cb
> 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> @@ -1917,7 +1917,8 @@ static s32 pixinc(int pixels, u8 ps)
>  static void calc_offset(u16 screen_width, u16 width,
>  		u32 fourcc, bool fieldmode,
>  		unsigned int field_offset, unsigned *offset0, unsigned 
*offset1,
> -		s32 *row_inc, s32 *pix_inc, int x_predecim, int y_predecim)
> +		s32 *row_inc, s32 *pix_inc, int x_predecim, int y_predecim,
> +		enum omap_dss_rotation_type rotation_type, u8 rotation)
>  {
>  	u8 ps;
> 
> @@ -1925,6 +1926,20 @@ static void calc_offset(u16 screen_width, u16 width,
> 
>  	DSSDBG("scrw %d, width %d\n", screen_width, width);
> 
> +	if (rotation_type == OMAP_DSS_ROT_TILER &&
> +	    (fourcc == DRM_FORMAT_UYVY || fourcc == DRM_FORMAT_YUYV) &&
> +	    drm_rotation_90_or_270(rotation)) {
> +		/*
> +		 * HACK: ROW_INC needs to be calculated with TILER units.
> +		 * We get such 'screen_width' that multiplying it with the
> +		 * YUV422 pixel size gives the correct TILER container width.
> +		 * However, 'width' is in pixels and multiplying it with 
YUV422
> +		 * pixel size gives incorrect result. We thus multiply it here
> +		 * with 2 to match the 32 bit TILER unit size.
> +		 */
> +		width *= 2;
> +	}
> +
>  	/*
>  	 * field 0 = even field = bottom field
>  	 * field 1 = odd field = top field
> @@ -2473,7 +2488,8 @@ static int dispc_ovl_setup_common(enum omap_plane_id
> plane, calc_offset(screen_width, frame_width,
>  			fourcc, fieldmode, field_offset,
>  			&offset0, &offset1, &row_inc, &pix_inc,
> -			x_predecim, y_predecim);
> +			x_predecim, y_predecim,
> +			rotation_type, rotation);
> 
>  	DSSDBG("offset0 %u, offset1 %u, row_inc %d, pix_inc %d\n",
>  			offset0, offset1, row_inc, pix_inc);
> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c
> 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.

> +			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.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  		info->screen_width  = omap_gem_tiled_stride(plane->bo, 
orient);
>  	} else {
>  		switch (state->rotation & DRM_ROTATE_MASK) {

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel