Re: [PATCH] drm: rcar-du: Fix pitch handling for fully planar YUV formats

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Laurent,

On 06/08/2020 03:26, Laurent Pinchart wrote:
> When creating a frame buffer, the driver verifies that the pitches for
> the chroma planes match the luma plane. This is done incorrectly for
> fully planar YUV formats, without taking horizontal subsampling into
> account. Fix it.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c | 52 ++++++++++++++++++++++++++-
>  drivers/gpu/drm/rcar-du/rcar_du_kms.h |  1 +
>  2 files changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> index 482329102f19..2fda3734a57e 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -40,6 +40,7 @@ static const struct rcar_du_format_info rcar_du_format_infos[] = {
>  		.v4l2 = V4L2_PIX_FMT_RGB565,
>  		.bpp = 16,
>  		.planes = 1,
> +		.hsub = 1,
>  		.pnmr = PnMR_SPIM_TP | PnMR_DDDF_16BPP,
>  		.edf = PnDDCR4_EDF_NONE,
>  	}, {
> @@ -47,6 +48,7 @@ static const struct rcar_du_format_info rcar_du_format_infos[] = {
>  		.v4l2 = V4L2_PIX_FMT_ARGB555,
>  		.bpp = 16,
>  		.planes = 1,
> +		.hsub = 1,
>  		.pnmr = PnMR_SPIM_ALP | PnMR_DDDF_ARGB,
>  		.edf = PnDDCR4_EDF_NONE,
>  	}, {
> @@ -61,6 +63,7 @@ static const struct rcar_du_format_info rcar_du_format_infos[] = {
>  		.v4l2 = V4L2_PIX_FMT_XBGR32,
>  		.bpp = 32,
>  		.planes = 1,
> +		.hsub = 1,
>  		.pnmr = PnMR_SPIM_TP | PnMR_DDDF_16BPP,
>  		.edf = PnDDCR4_EDF_RGB888,
>  	}, {
> @@ -68,6 +71,7 @@ static const struct rcar_du_format_info rcar_du_format_infos[] = {
>  		.v4l2 = V4L2_PIX_FMT_ABGR32,
>  		.bpp = 32,
>  		.planes = 1,
> +		.hsub = 1,
>  		.pnmr = PnMR_SPIM_ALP | PnMR_DDDF_16BPP,
>  		.edf = PnDDCR4_EDF_ARGB8888,
>  	}, {
> @@ -75,6 +79,7 @@ static const struct rcar_du_format_info rcar_du_format_infos[] = {
>  		.v4l2 = V4L2_PIX_FMT_UYVY,
>  		.bpp = 16,
>  		.planes = 1,
> +		.hsub = 2,
>  		.pnmr = PnMR_SPIM_TP_OFF | PnMR_DDDF_YC,
>  		.edf = PnDDCR4_EDF_NONE,
>  	}, {
> @@ -82,6 +87,7 @@ static const struct rcar_du_format_info rcar_du_format_infos[] = {
>  		.v4l2 = V4L2_PIX_FMT_YUYV,
>  		.bpp = 16,
>  		.planes = 1,
> +		.hsub = 2,
>  		.pnmr = PnMR_SPIM_TP_OFF | PnMR_DDDF_YC,
>  		.edf = PnDDCR4_EDF_NONE,
>  	}, {
> @@ -89,6 +95,7 @@ static const struct rcar_du_format_info rcar_du_format_infos[] = {
>  		.v4l2 = V4L2_PIX_FMT_NV12M,
>  		.bpp = 12,
>  		.planes = 2,
> +		.hsub = 2,
>  		.pnmr = PnMR_SPIM_TP_OFF | PnMR_DDDF_YC,
>  		.edf = PnDDCR4_EDF_NONE,
>  	}, {
> @@ -96,6 +103,7 @@ static const struct rcar_du_format_info rcar_du_format_infos[] = {
>  		.v4l2 = V4L2_PIX_FMT_NV21M,
>  		.bpp = 12,
>  		.planes = 2,
> +		.hsub = 2,
>  		.pnmr = PnMR_SPIM_TP_OFF | PnMR_DDDF_YC,
>  		.edf = PnDDCR4_EDF_NONE,
>  	}, {
> @@ -103,6 +111,7 @@ static const struct rcar_du_format_info rcar_du_format_infos[] = {
>  		.v4l2 = V4L2_PIX_FMT_NV16M,
>  		.bpp = 16,
>  		.planes = 2,
> +		.hsub = 2,
>  		.pnmr = PnMR_SPIM_TP_OFF | PnMR_DDDF_YC,
>  		.edf = PnDDCR4_EDF_NONE,
>  	},
> @@ -115,156 +124,187 @@ static const struct rcar_du_format_info rcar_du_format_infos[] = {
>  		.v4l2 = V4L2_PIX_FMT_RGB332,
>  		.bpp = 8,
>  		.planes = 1,
> +		.hsub = 1,
>  	}, {
>  		.fourcc = DRM_FORMAT_ARGB4444,
>  		.v4l2 = V4L2_PIX_FMT_ARGB444,
>  		.bpp = 16,
>  		.planes = 1,
> +		.hsub = 1,
>  	}, {
>  		.fourcc = DRM_FORMAT_XRGB4444,
>  		.v4l2 = V4L2_PIX_FMT_XRGB444,
>  		.bpp = 16,
>  		.planes = 1,
> +		.hsub = 1,
>  	}, {
>  		.fourcc = DRM_FORMAT_RGBA4444,
>  		.v4l2 = V4L2_PIX_FMT_RGBA444,
>  		.bpp = 16,
>  		.planes = 1,
> +		.hsub = 1,
>  	}, {
>  		.fourcc = DRM_FORMAT_RGBX4444,
>  		.v4l2 = V4L2_PIX_FMT_RGBX444,
>  		.bpp = 16,
>  		.planes = 1,
> +		.hsub = 1,
>  	}, {
>  		.fourcc = DRM_FORMAT_ABGR4444,
>  		.v4l2 = V4L2_PIX_FMT_ABGR444,
>  		.bpp = 16,
>  		.planes = 1,
> +		.hsub = 1,
>  	}, {
>  		.fourcc = DRM_FORMAT_XBGR4444,
>  		.v4l2 = V4L2_PIX_FMT_XBGR444,
>  		.bpp = 16,
>  		.planes = 1,
> +		.hsub = 1,
>  	}, {
>  		.fourcc = DRM_FORMAT_BGRA4444,
>  		.v4l2 = V4L2_PIX_FMT_BGRA444,
>  		.bpp = 16,
>  		.planes = 1,
> +		.hsub = 1,
>  	}, {
>  		.fourcc = DRM_FORMAT_BGRX4444,
>  		.v4l2 = V4L2_PIX_FMT_BGRX444,
>  		.bpp = 16,
>  		.planes = 1,
> +		.hsub = 1,
>  	}, {
>  		.fourcc = DRM_FORMAT_RGBA5551,
>  		.v4l2 = V4L2_PIX_FMT_RGBA555,
>  		.bpp = 16,
>  		.planes = 1,
> +		.hsub = 1,
>  	}, {
>  		.fourcc = DRM_FORMAT_RGBX5551,
>  		.v4l2 = V4L2_PIX_FMT_RGBX555,
>  		.bpp = 16,
>  		.planes = 1,
> +		.hsub = 1,
>  	}, {
>  		.fourcc = DRM_FORMAT_ABGR1555,
>  		.v4l2 = V4L2_PIX_FMT_ABGR555,
>  		.bpp = 16,
>  		.planes = 1,
> +		.hsub = 1,
>  	}, {
>  		.fourcc = DRM_FORMAT_XBGR1555,
>  		.v4l2 = V4L2_PIX_FMT_XBGR555,
>  		.bpp = 16,
>  		.planes = 1,
> +		.hsub = 1,
>  	}, {
>  		.fourcc = DRM_FORMAT_BGRA5551,
>  		.v4l2 = V4L2_PIX_FMT_BGRA555,
>  		.bpp = 16,
>  		.planes = 1,
> +		.hsub = 1,
>  	}, {
>  		.fourcc = DRM_FORMAT_BGRX5551,
>  		.v4l2 = V4L2_PIX_FMT_BGRX555,
>  		.bpp = 16,
>  		.planes = 1,
> +		.hsub = 1,
>  	}, {
>  		.fourcc = DRM_FORMAT_BGR888,
>  		.v4l2 = V4L2_PIX_FMT_RGB24,
>  		.bpp = 24,
>  		.planes = 1,
> +		.hsub = 1,
>  	}, {
>  		.fourcc = DRM_FORMAT_RGB888,
>  		.v4l2 = V4L2_PIX_FMT_BGR24,
>  		.bpp = 24,
>  		.planes = 1,
> +		.hsub = 1,
>  	}, {
>  		.fourcc = DRM_FORMAT_RGBA8888,
>  		.v4l2 = V4L2_PIX_FMT_BGRA32,
>  		.bpp = 32,
>  		.planes = 1,
> +		.hsub = 1,
>  	}, {
>  		.fourcc = DRM_FORMAT_RGBX8888,
>  		.v4l2 = V4L2_PIX_FMT_BGRX32,
>  		.bpp = 32,
>  		.planes = 1,
> +		.hsub = 1,
>  	}, {
>  		.fourcc = DRM_FORMAT_ABGR8888,
>  		.v4l2 = V4L2_PIX_FMT_RGBA32,
>  		.bpp = 32,
>  		.planes = 1,
> +		.hsub = 1,
>  	}, {
>  		.fourcc = DRM_FORMAT_XBGR8888,
>  		.v4l2 = V4L2_PIX_FMT_RGBX32,
>  		.bpp = 32,
>  		.planes = 1,
> +		.hsub = 1,
>  	}, {
>  		.fourcc = DRM_FORMAT_BGRA8888,
>  		.v4l2 = V4L2_PIX_FMT_ARGB32,
>  		.bpp = 32,
>  		.planes = 1,
> +		.hsub = 1,
>  	}, {
>  		.fourcc = DRM_FORMAT_BGRX8888,
>  		.v4l2 = V4L2_PIX_FMT_XRGB32,
>  		.bpp = 32,
>  		.planes = 1,
> +		.hsub = 1,
>  	}, {
>  		.fourcc = DRM_FORMAT_YVYU,
>  		.v4l2 = V4L2_PIX_FMT_YVYU,
>  		.bpp = 16,
>  		.planes = 1,
> +		.hsub = 2,
>  	}, {
>  		.fourcc = DRM_FORMAT_NV61,
>  		.v4l2 = V4L2_PIX_FMT_NV61M,
>  		.bpp = 16,
>  		.planes = 2,
> +		.hsub = 2,
>  	}, {
>  		.fourcc = DRM_FORMAT_YUV420,
>  		.v4l2 = V4L2_PIX_FMT_YUV420M,
>  		.bpp = 12,
>  		.planes = 3,
> +		.hsub = 2,

I guess vertical subsampling is handled distinctly?
(perhaps the height of the plane or such?)

>  	}, {
>  		.fourcc = DRM_FORMAT_YVU420,
>  		.v4l2 = V4L2_PIX_FMT_YVU420M,
>  		.bpp = 12,
>  		.planes = 3,
> +		.hsub = 2,
>  	}, {
>  		.fourcc = DRM_FORMAT_YUV422,
>  		.v4l2 = V4L2_PIX_FMT_YUV422M,
>  		.bpp = 16,
>  		.planes = 3,
> +		.hsub = 2,
>  	}, {
>  		.fourcc = DRM_FORMAT_YVU422,
>  		.v4l2 = V4L2_PIX_FMT_YVU422M,
>  		.bpp = 16,
>  		.planes = 3,
> +		.hsub = 2,
>  	}, {
>  		.fourcc = DRM_FORMAT_YUV444,
>  		.v4l2 = V4L2_PIX_FMT_YUV444M,
>  		.bpp = 24,
>  		.planes = 3,
> +		.hsub = 1,
>  	}, {
>  		.fourcc = DRM_FORMAT_YVU444,
>  		.v4l2 = V4L2_PIX_FMT_YVU444M,
>  		.bpp = 24,
>  		.planes = 3,
> +		.hsub = 1,
>  	},
>  };
>  

I wonder when we can have a global/generic set of format tables so that
all of this isn't duplicated on a per-driver basis.


> @@ -311,6 +351,7 @@ rcar_du_fb_create(struct drm_device *dev, struct drm_file *file_priv,
>  {
>  	struct rcar_du_device *rcdu = dev->dev_private;
>  	const struct rcar_du_format_info *format;
> +	unsigned int chroma_pitch;
>  	unsigned int max_pitch;
>  	unsigned int align;
>  	unsigned int i;
> @@ -353,8 +394,17 @@ rcar_du_fb_create(struct drm_device *dev, struct drm_file *file_priv,
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> +	/*
> +	 * Calculate the chroma plane(s) pitch using the horizontal subsampling
> +	 * factor. For semi-planar formats, the U and V planes are combined, the
> +	 * pitch must thus be doubled.
> +	 */
> +	chroma_pitch = mode_cmd->pitches[0] / format->hsub;
> +	if (format->planes == 2)
> +		chroma_pitch *= 2;
> +
>  	for (i = 1; i < format->planes; ++i) {
> -		if (mode_cmd->pitches[i] != mode_cmd->pitches[0]) {
> +		if (mode_cmd->pitches[i] != chroma_pitch) {
>  			dev_dbg(dev->dev,
>  				"luma and chroma pitches do not match\n");

Is this statement still sufficient?
I'd perhaps say 'are not compatible' or 'are not correct'. - but its
only a debug print, so it really doesn't matter.

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>



>  			return ERR_PTR(-EINVAL);
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.h b/drivers/gpu/drm/rcar-du/rcar_du_kms.h
> index 0346504d8c59..8f5fff176754 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.h
> @@ -22,6 +22,7 @@ struct rcar_du_format_info {
>  	u32 v4l2;
>  	unsigned int bpp;
>  	unsigned int planes;
> +	unsigned int hsub;
>  	unsigned int pnmr;
>  	unsigned int edf;
>  };
> 

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



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux