* [PATCH] drm: rcar-du: Fix pitch handling for fully planar YUV formats @ 2020-08-06 2:26 ` Laurent Pinchart 0 siblings, 0 replies; 16+ messages in thread From: Laurent Pinchart @ 2020-08-06 2:26 UTC (permalink / raw) To: dri-devel; +Cc: linux-renesas-soc, Kieran Bingham 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@ideasonboard.com> --- 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, }, { .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, }, }; @@ -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"); 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, Laurent Pinchart ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH] drm: rcar-du: Fix pitch handling for fully planar YUV formats @ 2020-08-06 2:26 ` Laurent Pinchart 0 siblings, 0 replies; 16+ messages in thread From: Laurent Pinchart @ 2020-08-06 2:26 UTC (permalink / raw) To: dri-devel; +Cc: linux-renesas-soc, Kieran Bingham 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@ideasonboard.com> --- 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, }, { .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, }, }; @@ -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"); 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, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] drm: rcar-du: Fix pitch handling for fully planar YUV formats 2020-08-06 2:26 ` Laurent Pinchart @ 2020-09-08 15:42 ` Kieran Bingham -1 siblings, 0 replies; 16+ messages in thread From: Kieran Bingham @ 2020-09-08 15:42 UTC (permalink / raw) To: Laurent Pinchart, dri-devel; +Cc: linux-renesas-soc 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@ideasonboard.com> > --- > 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@ideasonboard.com> > 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm: rcar-du: Fix pitch handling for fully planar YUV formats @ 2020-09-08 15:42 ` Kieran Bingham 0 siblings, 0 replies; 16+ messages in thread From: Kieran Bingham @ 2020-09-08 15:42 UTC (permalink / raw) To: Laurent Pinchart, dri-devel; +Cc: linux-renesas-soc 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@ideasonboard.com> > --- > 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@ideasonboard.com> > 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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm: rcar-du: Fix pitch handling for fully planar YUV formats 2020-09-08 15:42 ` Kieran Bingham @ 2020-09-08 15:52 ` Laurent Pinchart -1 siblings, 0 replies; 16+ messages in thread From: Laurent Pinchart @ 2020-09-08 15:52 UTC (permalink / raw) To: Kieran Bingham; +Cc: Laurent Pinchart, dri-devel, linux-renesas-soc Hi Kieran, On Tue, Sep 08, 2020 at 04:42:58PM +0100, Kieran Bingham wrote: > 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@ideasonboard.com> > > --- > > 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?) Vertical subsampling doesn't affect the pitch, so there's no specific constraint there. > > }, { > > .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. Note that this table also contains register values, so at least that part will need to be kept. For the rest, do you mean a 4CC library that would be shared between DRM/KMS and V4L2 ? That's a great idea. Too bad it has been shot down when patches were submitted :-S > > @@ -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. I like "are not compatible", I'll switch to that. > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > > 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, Laurent Pinchart ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm: rcar-du: Fix pitch handling for fully planar YUV formats @ 2020-09-08 15:52 ` Laurent Pinchart 0 siblings, 0 replies; 16+ messages in thread From: Laurent Pinchart @ 2020-09-08 15:52 UTC (permalink / raw) To: Kieran Bingham; +Cc: linux-renesas-soc, Laurent Pinchart, dri-devel Hi Kieran, On Tue, Sep 08, 2020 at 04:42:58PM +0100, Kieran Bingham wrote: > 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@ideasonboard.com> > > --- > > 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?) Vertical subsampling doesn't affect the pitch, so there's no specific constraint there. > > }, { > > .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. Note that this table also contains register values, so at least that part will need to be kept. For the rest, do you mean a 4CC library that would be shared between DRM/KMS and V4L2 ? That's a great idea. Too bad it has been shot down when patches were submitted :-S > > @@ -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. I like "are not compatible", I'll switch to that. > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > > 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, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm: rcar-du: Fix pitch handling for fully planar YUV formats 2020-09-08 15:52 ` Laurent Pinchart @ 2020-09-08 16:05 ` Kieran Bingham -1 siblings, 0 replies; 16+ messages in thread From: Kieran Bingham @ 2020-09-08 16:05 UTC (permalink / raw) To: Laurent Pinchart; +Cc: Laurent Pinchart, dri-devel, linux-renesas-soc Hi Laurent, On 08/09/2020 16:52, Laurent Pinchart wrote: > Hi Kieran, > > On Tue, Sep 08, 2020 at 04:42:58PM +0100, Kieran Bingham wrote: >> 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@ideasonboard.com> >>> --- >>> 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?) > > Vertical subsampling doesn't affect the pitch, so there's no specific > constraint there. Good point, I wrongly assumed there would be a need to validate that specifically. But the height covers it. > >>> }, { >>> .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. > > Note that this table also contains register values, so at least that > part will need to be kept. For the rest, do you mean a 4CC library that Yes, the driver specific mappings of course need to be driver specific. > would be shared between DRM/KMS and V4L2 ? That's a great idea. Too bad > it has been shot down when patches were submitted :-S /o\ ... It just seems like so much data replication that must be used by many drivers. Even without mapping the DRM/V4L2 fourccs - even a common table in each subsystem would be beneficial wouldn't it? I mean - RCar-DU isn't the only device that needs to know how many planes DRM_FORMAT_YUV422 has, or what horizontal subsampling it uses? Anyway, that's not an issue with this patch, it just seems glaring to me that these entries are common across all hardware that use them ... (the bpp/planes/subsampling of course, not the hardware specific registers). >>> @@ -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. > > I like "are not compatible", I'll switch to that. > >> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> >> >>> 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm: rcar-du: Fix pitch handling for fully planar YUV formats @ 2020-09-08 16:05 ` Kieran Bingham 0 siblings, 0 replies; 16+ messages in thread From: Kieran Bingham @ 2020-09-08 16:05 UTC (permalink / raw) To: Laurent Pinchart; +Cc: linux-renesas-soc, Laurent Pinchart, dri-devel Hi Laurent, On 08/09/2020 16:52, Laurent Pinchart wrote: > Hi Kieran, > > On Tue, Sep 08, 2020 at 04:42:58PM +0100, Kieran Bingham wrote: >> 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@ideasonboard.com> >>> --- >>> 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?) > > Vertical subsampling doesn't affect the pitch, so there's no specific > constraint there. Good point, I wrongly assumed there would be a need to validate that specifically. But the height covers it. > >>> }, { >>> .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. > > Note that this table also contains register values, so at least that > part will need to be kept. For the rest, do you mean a 4CC library that Yes, the driver specific mappings of course need to be driver specific. > would be shared between DRM/KMS and V4L2 ? That's a great idea. Too bad > it has been shot down when patches were submitted :-S /o\ ... It just seems like so much data replication that must be used by many drivers. Even without mapping the DRM/V4L2 fourccs - even a common table in each subsystem would be beneficial wouldn't it? I mean - RCar-DU isn't the only device that needs to know how many planes DRM_FORMAT_YUV422 has, or what horizontal subsampling it uses? Anyway, that's not an issue with this patch, it just seems glaring to me that these entries are common across all hardware that use them ... (the bpp/planes/subsampling of course, not the hardware specific registers). >>> @@ -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. > > I like "are not compatible", I'll switch to that. > >> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> >> >>> 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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm: rcar-du: Fix pitch handling for fully planar YUV formats 2020-09-08 16:05 ` Kieran Bingham @ 2020-09-09 12:08 ` Ville Syrjälä -1 siblings, 0 replies; 16+ messages in thread From: Ville Syrjälä @ 2020-09-09 12:08 UTC (permalink / raw) To: Kieran Bingham Cc: Laurent Pinchart, linux-renesas-soc, Laurent Pinchart, dri-devel On Tue, Sep 08, 2020 at 05:05:48PM +0100, Kieran Bingham wrote: > Hi Laurent, > > On 08/09/2020 16:52, Laurent Pinchart wrote: > > Hi Kieran, > > > > On Tue, Sep 08, 2020 at 04:42:58PM +0100, Kieran Bingham wrote: > >> 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@ideasonboard.com> > >>> --- <snip> > >>> }, { > >>> .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. > > > > Note that this table also contains register values, so at least that > > part will need to be kept. For the rest, do you mean a 4CC library that > > Yes, the driver specific mappings of course need to be driver specific. > > > > would be shared between DRM/KMS and V4L2 ? That's a great idea. Too bad > > it has been shot down when patches were submitted :-S > > > /o\ ... It just seems like so much data replication that must be used > by many drivers. > > Even without mapping the DRM/V4L2 fourccs - even a common table in each > subsystem would be beneficial wouldn't it? > > I mean - RCar-DU isn't the only device that needs to know how many > planes DRM_FORMAT_YUV422 has, or what horizontal subsampling it uses? > > Anyway, that's not an issue with this patch, it just seems glaring to me > that these entries are common across all hardware that use them ... > > (the bpp/planes/subsampling of course, not the hardware specific registers). See drm_format_info() & co. -- Ville Syrjälä Intel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm: rcar-du: Fix pitch handling for fully planar YUV formats @ 2020-09-09 12:08 ` Ville Syrjälä 0 siblings, 0 replies; 16+ messages in thread From: Ville Syrjälä @ 2020-09-09 12:08 UTC (permalink / raw) To: Kieran Bingham Cc: linux-renesas-soc, Laurent Pinchart, Laurent Pinchart, dri-devel On Tue, Sep 08, 2020 at 05:05:48PM +0100, Kieran Bingham wrote: > Hi Laurent, > > On 08/09/2020 16:52, Laurent Pinchart wrote: > > Hi Kieran, > > > > On Tue, Sep 08, 2020 at 04:42:58PM +0100, Kieran Bingham wrote: > >> 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@ideasonboard.com> > >>> --- <snip> > >>> }, { > >>> .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. > > > > Note that this table also contains register values, so at least that > > part will need to be kept. For the rest, do you mean a 4CC library that > > Yes, the driver specific mappings of course need to be driver specific. > > > > would be shared between DRM/KMS and V4L2 ? That's a great idea. Too bad > > it has been shot down when patches were submitted :-S > > > /o\ ... It just seems like so much data replication that must be used > by many drivers. > > Even without mapping the DRM/V4L2 fourccs - even a common table in each > subsystem would be beneficial wouldn't it? > > I mean - RCar-DU isn't the only device that needs to know how many > planes DRM_FORMAT_YUV422 has, or what horizontal subsampling it uses? > > Anyway, that's not an issue with this patch, it just seems glaring to me > that these entries are common across all hardware that use them ... > > (the bpp/planes/subsampling of course, not the hardware specific registers). See drm_format_info() & co. -- Ville Syrjälä Intel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm: rcar-du: Fix pitch handling for fully planar YUV formats 2020-09-09 12:08 ` Ville Syrjälä @ 2020-09-09 16:06 ` Kieran Bingham -1 siblings, 0 replies; 16+ messages in thread From: Kieran Bingham @ 2020-09-09 16:06 UTC (permalink / raw) To: Ville Syrjälä Cc: Laurent Pinchart, linux-renesas-soc, Laurent Pinchart, dri-devel Hi Ville, Laurent, On 09/09/2020 13:08, Ville Syrjälä wrote: > On Tue, Sep 08, 2020 at 05:05:48PM +0100, Kieran Bingham wrote: >> Hi Laurent, >> >> On 08/09/2020 16:52, Laurent Pinchart wrote: >>> Hi Kieran, >>> >>> On Tue, Sep 08, 2020 at 04:42:58PM +0100, Kieran Bingham wrote: >>>> 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@ideasonboard.com> >>>>> --- > <snip> >>>>> }, { >>>>> .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. >>> >>> Note that this table also contains register values, so at least that >>> part will need to be kept. For the rest, do you mean a 4CC library that >> >> Yes, the driver specific mappings of course need to be driver specific. >> >> >>> would be shared between DRM/KMS and V4L2 ? That's a great idea. Too bad >>> it has been shot down when patches were submitted :-S >> >> >> /o\ ... It just seems like so much data replication that must be used >> by many drivers. >> >> Even without mapping the DRM/V4L2 fourccs - even a common table in each >> subsystem would be beneficial wouldn't it? >> >> I mean - RCar-DU isn't the only device that needs to know how many >> planes DRM_FORMAT_YUV422 has, or what horizontal subsampling it uses? >> >> Anyway, that's not an issue with this patch, it just seems glaring to me >> that these entries are common across all hardware that use them ... >> >> (the bpp/planes/subsampling of course, not the hardware specific registers). > > See drm_format_info() & co. Aha perfect, That's what I was looking for. I'm glad to see that's common (at least for the DRM parts). The question is - why aren't we using it in RCar-DU. Laurent, would you see any issue in obtaining the struct drm_format_info rather than re-encoding all the data in these tables? And if not - would you prefer to convert on top of this patch, or preceding this patch? Or simply prefer to keep the existing tables ? Given that this fixes a bug - I'd say getting this patch in now is probably best. -- Regards -- Kieran ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm: rcar-du: Fix pitch handling for fully planar YUV formats @ 2020-09-09 16:06 ` Kieran Bingham 0 siblings, 0 replies; 16+ messages in thread From: Kieran Bingham @ 2020-09-09 16:06 UTC (permalink / raw) To: Ville Syrjälä Cc: linux-renesas-soc, Laurent Pinchart, Laurent Pinchart, dri-devel Hi Ville, Laurent, On 09/09/2020 13:08, Ville Syrjälä wrote: > On Tue, Sep 08, 2020 at 05:05:48PM +0100, Kieran Bingham wrote: >> Hi Laurent, >> >> On 08/09/2020 16:52, Laurent Pinchart wrote: >>> Hi Kieran, >>> >>> On Tue, Sep 08, 2020 at 04:42:58PM +0100, Kieran Bingham wrote: >>>> 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@ideasonboard.com> >>>>> --- > <snip> >>>>> }, { >>>>> .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. >>> >>> Note that this table also contains register values, so at least that >>> part will need to be kept. For the rest, do you mean a 4CC library that >> >> Yes, the driver specific mappings of course need to be driver specific. >> >> >>> would be shared between DRM/KMS and V4L2 ? That's a great idea. Too bad >>> it has been shot down when patches were submitted :-S >> >> >> /o\ ... It just seems like so much data replication that must be used >> by many drivers. >> >> Even without mapping the DRM/V4L2 fourccs - even a common table in each >> subsystem would be beneficial wouldn't it? >> >> I mean - RCar-DU isn't the only device that needs to know how many >> planes DRM_FORMAT_YUV422 has, or what horizontal subsampling it uses? >> >> Anyway, that's not an issue with this patch, it just seems glaring to me >> that these entries are common across all hardware that use them ... >> >> (the bpp/planes/subsampling of course, not the hardware specific registers). > > See drm_format_info() & co. Aha perfect, That's what I was looking for. I'm glad to see that's common (at least for the DRM parts). The question is - why aren't we using it in RCar-DU. Laurent, would you see any issue in obtaining the struct drm_format_info rather than re-encoding all the data in these tables? And if not - would you prefer to convert on top of this patch, or preceding this patch? Or simply prefer to keep the existing tables ? Given that this fixes a bug - I'd say getting this patch in now is probably best. -- Regards -- Kieran _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm: rcar-du: Fix pitch handling for fully planar YUV formats 2020-09-09 16:06 ` Kieran Bingham @ 2020-09-09 16:17 ` Laurent Pinchart -1 siblings, 0 replies; 16+ messages in thread From: Laurent Pinchart @ 2020-09-09 16:17 UTC (permalink / raw) To: Kieran Bingham Cc: Ville Syrjälä, linux-renesas-soc, Laurent Pinchart, dri-devel Hi Kieran, On Wed, Sep 09, 2020 at 05:06:01PM +0100, Kieran Bingham wrote: > On 09/09/2020 13:08, Ville Syrjälä wrote: > > On Tue, Sep 08, 2020 at 05:05:48PM +0100, Kieran Bingham wrote: > >> On 08/09/2020 16:52, Laurent Pinchart wrote: > >>> On Tue, Sep 08, 2020 at 04:42:58PM +0100, Kieran Bingham wrote: > >>>> 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@ideasonboard.com> > >>>>> --- > > <snip> > >>>>> }, { > >>>>> .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. > >>> > >>> Note that this table also contains register values, so at least that > >>> part will need to be kept. For the rest, do you mean a 4CC library that > >> > >> Yes, the driver specific mappings of course need to be driver specific. > >> > >>> would be shared between DRM/KMS and V4L2 ? That's a great idea. Too bad > >>> it has been shot down when patches were submitted :-S > >> > >> /o\ ... It just seems like so much data replication that must be used > >> by many drivers. > >> > >> Even without mapping the DRM/V4L2 fourccs - even a common table in each > >> subsystem would be beneficial wouldn't it? > >> > >> I mean - RCar-DU isn't the only device that needs to know how many > >> planes DRM_FORMAT_YUV422 has, or what horizontal subsampling it uses? > >> > >> Anyway, that's not an issue with this patch, it just seems glaring to me > >> that these entries are common across all hardware that use them ... > >> > >> (the bpp/planes/subsampling of course, not the hardware specific registers). > > > > See drm_format_info() & co. > > Aha perfect, That's what I was looking for. > I'm glad to see that's common (at least for the DRM parts). > > The question is - why aren't we using it in RCar-DU. > > Laurent, would you see any issue in obtaining the struct drm_format_info > rather than re-encoding all the data in these tables? > > And if not - would you prefer to convert on top of this patch, or > preceding this patch? > > Or simply prefer to keep the existing tables ? > > Given that this fixes a bug - I'd say getting this patch in now is > probably best. I'd apply refactoring on top, if desired. You would have to keep the existing table for the mapping to V4L2 (although this could be moved to drm_format_info if desired), as well as for the register value. And that would then lead to a double lookup operation. That's the part that bothers me the most. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm: rcar-du: Fix pitch handling for fully planar YUV formats @ 2020-09-09 16:17 ` Laurent Pinchart 0 siblings, 0 replies; 16+ messages in thread From: Laurent Pinchart @ 2020-09-09 16:17 UTC (permalink / raw) To: Kieran Bingham; +Cc: linux-renesas-soc, Laurent Pinchart, dri-devel Hi Kieran, On Wed, Sep 09, 2020 at 05:06:01PM +0100, Kieran Bingham wrote: > On 09/09/2020 13:08, Ville Syrjälä wrote: > > On Tue, Sep 08, 2020 at 05:05:48PM +0100, Kieran Bingham wrote: > >> On 08/09/2020 16:52, Laurent Pinchart wrote: > >>> On Tue, Sep 08, 2020 at 04:42:58PM +0100, Kieran Bingham wrote: > >>>> 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@ideasonboard.com> > >>>>> --- > > <snip> > >>>>> }, { > >>>>> .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. > >>> > >>> Note that this table also contains register values, so at least that > >>> part will need to be kept. For the rest, do you mean a 4CC library that > >> > >> Yes, the driver specific mappings of course need to be driver specific. > >> > >>> would be shared between DRM/KMS and V4L2 ? That's a great idea. Too bad > >>> it has been shot down when patches were submitted :-S > >> > >> /o\ ... It just seems like so much data replication that must be used > >> by many drivers. > >> > >> Even without mapping the DRM/V4L2 fourccs - even a common table in each > >> subsystem would be beneficial wouldn't it? > >> > >> I mean - RCar-DU isn't the only device that needs to know how many > >> planes DRM_FORMAT_YUV422 has, or what horizontal subsampling it uses? > >> > >> Anyway, that's not an issue with this patch, it just seems glaring to me > >> that these entries are common across all hardware that use them ... > >> > >> (the bpp/planes/subsampling of course, not the hardware specific registers). > > > > See drm_format_info() & co. > > Aha perfect, That's what I was looking for. > I'm glad to see that's common (at least for the DRM parts). > > The question is - why aren't we using it in RCar-DU. > > Laurent, would you see any issue in obtaining the struct drm_format_info > rather than re-encoding all the data in these tables? > > And if not - would you prefer to convert on top of this patch, or > preceding this patch? > > Or simply prefer to keep the existing tables ? > > Given that this fixes a bug - I'd say getting this patch in now is > probably best. I'd apply refactoring on top, if desired. You would have to keep the existing table for the mapping to V4L2 (although this could be moved to drm_format_info if desired), as well as for the register value. And that would then lead to a double lookup operation. That's the part that bothers me the most. -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm: rcar-du: Fix pitch handling for fully planar YUV formats 2020-09-08 16:05 ` Kieran Bingham @ 2020-09-09 12:17 ` Daniel Vetter -1 siblings, 0 replies; 16+ messages in thread From: Daniel Vetter @ 2020-09-09 12:17 UTC (permalink / raw) To: Kieran Bingham Cc: Laurent Pinchart, open list:DRM DRIVERS FOR RENESAS, Laurent Pinchart, dri-devel On Tue, Sep 8, 2020 at 6:05 PM Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Hi Laurent, > > On 08/09/2020 16:52, Laurent Pinchart wrote: > > Hi Kieran, > > > > On Tue, Sep 08, 2020 at 04:42:58PM +0100, Kieran Bingham wrote: > >> 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@ideasonboard.com> > >>> --- > >>> 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?) > > > > Vertical subsampling doesn't affect the pitch, so there's no specific > > constraint there. > > Good point, I wrongly assumed there would be a need to validate that > specifically. > > But the height covers it. > > > > >>> }, { > >>> .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. > > > > Note that this table also contains register values, so at least that > > part will need to be kept. For the rest, do you mean a 4CC library that > > Yes, the driver specific mappings of course need to be driver specific. > > > > would be shared between DRM/KMS and V4L2 ? That's a great idea. Too bad > > it has been shot down when patches were submitted :-S > > > /o\ ... It just seems like so much data replication that must be used > by many drivers. Just to clarify: It wasn't shot down categorically, but I shot down the specific approach of first sharing code, and then trying to align the subsystems somehow later on with lots of hand-waving. The trouble with format enumerations that the devil's always in the details, so you have to absolutely align first, and then share code second, or it'll just become a complete mess. E.g. even in the drm world we lots of overlapping enumerations because they come from different standard bodies. There's a bit of a push to fully standardize on drm_fourcc.h codes at least in the low-level stuff, but e.g. vk/gl will always have their own. So yeah if you want to share format code with v4l, step 1 is to figure out how to establish common definitions. And not as in "we'll use the same numbers", but with stuff like userspace in gstreamer and everywhere assuming that they match perfectly (like we do between mesa and drm-modesetting drivers), with all that implies. And we're not even close to such a world. Cheers, Daniel > Even without mapping the DRM/V4L2 fourccs - even a common table in each > subsystem would be beneficial wouldn't it? > > I mean - RCar-DU isn't the only device that needs to know how many > planes DRM_FORMAT_YUV422 has, or what horizontal subsampling it uses? > > Anyway, that's not an issue with this patch, it just seems glaring to me > that these entries are common across all hardware that use them ... > > (the bpp/planes/subsampling of course, not the hardware specific registers). > > > > >>> @@ -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. > > > > I like "are not compatible", I'll switch to that. > > > >> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > >> > >>> 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm: rcar-du: Fix pitch handling for fully planar YUV formats @ 2020-09-09 12:17 ` Daniel Vetter 0 siblings, 0 replies; 16+ messages in thread From: Daniel Vetter @ 2020-09-09 12:17 UTC (permalink / raw) To: Kieran Bingham Cc: open list:DRM DRIVERS FOR RENESAS, Laurent Pinchart, Laurent Pinchart, dri-devel On Tue, Sep 8, 2020 at 6:05 PM Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Hi Laurent, > > On 08/09/2020 16:52, Laurent Pinchart wrote: > > Hi Kieran, > > > > On Tue, Sep 08, 2020 at 04:42:58PM +0100, Kieran Bingham wrote: > >> 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@ideasonboard.com> > >>> --- > >>> 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?) > > > > Vertical subsampling doesn't affect the pitch, so there's no specific > > constraint there. > > Good point, I wrongly assumed there would be a need to validate that > specifically. > > But the height covers it. > > > > >>> }, { > >>> .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. > > > > Note that this table also contains register values, so at least that > > part will need to be kept. For the rest, do you mean a 4CC library that > > Yes, the driver specific mappings of course need to be driver specific. > > > > would be shared between DRM/KMS and V4L2 ? That's a great idea. Too bad > > it has been shot down when patches were submitted :-S > > > /o\ ... It just seems like so much data replication that must be used > by many drivers. Just to clarify: It wasn't shot down categorically, but I shot down the specific approach of first sharing code, and then trying to align the subsystems somehow later on with lots of hand-waving. The trouble with format enumerations that the devil's always in the details, so you have to absolutely align first, and then share code second, or it'll just become a complete mess. E.g. even in the drm world we lots of overlapping enumerations because they come from different standard bodies. There's a bit of a push to fully standardize on drm_fourcc.h codes at least in the low-level stuff, but e.g. vk/gl will always have their own. So yeah if you want to share format code with v4l, step 1 is to figure out how to establish common definitions. And not as in "we'll use the same numbers", but with stuff like userspace in gstreamer and everywhere assuming that they match perfectly (like we do between mesa and drm-modesetting drivers), with all that implies. And we're not even close to such a world. Cheers, Daniel > Even without mapping the DRM/V4L2 fourccs - even a common table in each > subsystem would be beneficial wouldn't it? > > I mean - RCar-DU isn't the only device that needs to know how many > planes DRM_FORMAT_YUV422 has, or what horizontal subsampling it uses? > > Anyway, that's not an issue with this patch, it just seems glaring to me > that these entries are common across all hardware that use them ... > > (the bpp/planes/subsampling of course, not the hardware specific registers). > > > > >>> @@ -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. > > > > I like "are not compatible", I'll switch to that. > > > >> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > >> > >>> 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2020-09-09 16:50 UTC | newest] Thread overview: 16+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-08-06 2:26 [PATCH] drm: rcar-du: Fix pitch handling for fully planar YUV formats Laurent Pinchart 2020-08-06 2:26 ` Laurent Pinchart 2020-09-08 15:42 ` Kieran Bingham 2020-09-08 15:42 ` Kieran Bingham 2020-09-08 15:52 ` Laurent Pinchart 2020-09-08 15:52 ` Laurent Pinchart 2020-09-08 16:05 ` Kieran Bingham 2020-09-08 16:05 ` Kieran Bingham 2020-09-09 12:08 ` Ville Syrjälä 2020-09-09 12:08 ` Ville Syrjälä 2020-09-09 16:06 ` Kieran Bingham 2020-09-09 16:06 ` Kieran Bingham 2020-09-09 16:17 ` Laurent Pinchart 2020-09-09 16:17 ` Laurent Pinchart 2020-09-09 12:17 ` Daniel Vetter 2020-09-09 12:17 ` Daniel Vetter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.