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@xxxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/rcar-du/rcar_du_kms.c | 52 ++++++++++++++++++++++++++-
> > drivers/gpu/drm/rcar-du/rcar_du_kms.h | 1 +
> > 2 files changed, 52 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > index 482329102f19..2fda3734a57e 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > @@ -40,6 +40,7 @@ static const struct rcar_du_format_info rcar_du_format_infos[] = {
> > .v4l2 = V4L2_PIX_FMT_RGB565,
> > .bpp = 16,
> > .planes = 1,
> > + .hsub = 1,
> > .pnmr = PnMR_SPIM_TP | PnMR_DDDF_16BPP,
> > .edf = PnDDCR4_EDF_NONE,
> > }, {
> > @@ -47,6 +48,7 @@ static const struct rcar_du_format_info rcar_du_format_infos[] = {
> > .v4l2 = V4L2_PIX_FMT_ARGB555,
> > .bpp = 16,
> > .planes = 1,
> > + .hsub = 1,
> > .pnmr = PnMR_SPIM_ALP | PnMR_DDDF_ARGB,
> > .edf = PnDDCR4_EDF_NONE,
> > }, {
> > @@ -61,6 +63,7 @@ static const struct rcar_du_format_info rcar_du_format_infos[] = {
> > .v4l2 = V4L2_PIX_FMT_XBGR32,
> > .bpp = 32,
> > .planes = 1,
> > + .hsub = 1,
> > .pnmr = PnMR_SPIM_TP | PnMR_DDDF_16BPP,
> > .edf = PnDDCR4_EDF_RGB888,
> > }, {
> > @@ -68,6 +71,7 @@ static const struct rcar_du_format_info rcar_du_format_infos[] = {
> > .v4l2 = V4L2_PIX_FMT_ABGR32,
> > .bpp = 32,
> > .planes = 1,
> > + .hsub = 1,
> > .pnmr = PnMR_SPIM_ALP | PnMR_DDDF_16BPP,
> > .edf = PnDDCR4_EDF_ARGB8888,
> > }, {
> > @@ -75,6 +79,7 @@ static const struct rcar_du_format_info rcar_du_format_infos[] = {
> > .v4l2 = V4L2_PIX_FMT_UYVY,
> > .bpp = 16,
> > .planes = 1,
> > + .hsub = 2,
> > .pnmr = PnMR_SPIM_TP_OFF | PnMR_DDDF_YC,
> > .edf = PnDDCR4_EDF_NONE,
> > }, {
> > @@ -82,6 +87,7 @@ static const struct rcar_du_format_info rcar_du_format_infos[] = {
> > .v4l2 = V4L2_PIX_FMT_YUYV,
> > .bpp = 16,
> > .planes = 1,
> > + .hsub = 2,
> > .pnmr = PnMR_SPIM_TP_OFF | PnMR_DDDF_YC,
> > .edf = PnDDCR4_EDF_NONE,
> > }, {
> > @@ -89,6 +95,7 @@ static const struct rcar_du_format_info rcar_du_format_infos[] = {
> > .v4l2 = V4L2_PIX_FMT_NV12M,
> > .bpp = 12,
> > .planes = 2,
> > + .hsub = 2,
> > .pnmr = PnMR_SPIM_TP_OFF | PnMR_DDDF_YC,
> > .edf = PnDDCR4_EDF_NONE,
> > }, {
> > @@ -96,6 +103,7 @@ static const struct rcar_du_format_info rcar_du_format_infos[] = {
> > .v4l2 = V4L2_PIX_FMT_NV21M,
> > .bpp = 12,
> > .planes = 2,
> > + .hsub = 2,
> > .pnmr = PnMR_SPIM_TP_OFF | PnMR_DDDF_YC,
> > .edf = PnDDCR4_EDF_NONE,
> > }, {
> > @@ -103,6 +111,7 @@ static const struct rcar_du_format_info rcar_du_format_infos[] = {
> > .v4l2 = V4L2_PIX_FMT_NV16M,
> > .bpp = 16,
> > .planes = 2,
> > + .hsub = 2,
> > .pnmr = PnMR_SPIM_TP_OFF | PnMR_DDDF_YC,
> > .edf = PnDDCR4_EDF_NONE,
> > },
> > @@ -115,156 +124,187 @@ static const struct rcar_du_format_info rcar_du_format_infos[] = {
> > .v4l2 = V4L2_PIX_FMT_RGB332,
> > .bpp = 8,
> > .planes = 1,
> > + .hsub = 1,
> > }, {
> > .fourcc = DRM_FORMAT_ARGB4444,
> > .v4l2 = V4L2_PIX_FMT_ARGB444,
> > .bpp = 16,
> > .planes = 1,
> > + .hsub = 1,
> > }, {
> > .fourcc = DRM_FORMAT_XRGB4444,
> > .v4l2 = V4L2_PIX_FMT_XRGB444,
> > .bpp = 16,
> > .planes = 1,
> > + .hsub = 1,
> > }, {
> > .fourcc = DRM_FORMAT_RGBA4444,
> > .v4l2 = V4L2_PIX_FMT_RGBA444,
> > .bpp = 16,
> > .planes = 1,
> > + .hsub = 1,
> > }, {
> > .fourcc = DRM_FORMAT_RGBX4444,
> > .v4l2 = V4L2_PIX_FMT_RGBX444,
> > .bpp = 16,
> > .planes = 1,
> > + .hsub = 1,
> > }, {
> > .fourcc = DRM_FORMAT_ABGR4444,
> > .v4l2 = V4L2_PIX_FMT_ABGR444,
> > .bpp = 16,
> > .planes = 1,
> > + .hsub = 1,
> > }, {
> > .fourcc = DRM_FORMAT_XBGR4444,
> > .v4l2 = V4L2_PIX_FMT_XBGR444,
> > .bpp = 16,
> > .planes = 1,
> > + .hsub = 1,
> > }, {
> > .fourcc = DRM_FORMAT_BGRA4444,
> > .v4l2 = V4L2_PIX_FMT_BGRA444,
> > .bpp = 16,
> > .planes = 1,
> > + .hsub = 1,
> > }, {
> > .fourcc = DRM_FORMAT_BGRX4444,
> > .v4l2 = V4L2_PIX_FMT_BGRX444,
> > .bpp = 16,
> > .planes = 1,
> > + .hsub = 1,
> > }, {
> > .fourcc = DRM_FORMAT_RGBA5551,
> > .v4l2 = V4L2_PIX_FMT_RGBA555,
> > .bpp = 16,
> > .planes = 1,
> > + .hsub = 1,
> > }, {
> > .fourcc = DRM_FORMAT_RGBX5551,
> > .v4l2 = V4L2_PIX_FMT_RGBX555,
> > .bpp = 16,
> > .planes = 1,
> > + .hsub = 1,
> > }, {
> > .fourcc = DRM_FORMAT_ABGR1555,
> > .v4l2 = V4L2_PIX_FMT_ABGR555,
> > .bpp = 16,
> > .planes = 1,
> > + .hsub = 1,
> > }, {
> > .fourcc = DRM_FORMAT_XBGR1555,
> > .v4l2 = V4L2_PIX_FMT_XBGR555,
> > .bpp = 16,
> > .planes = 1,
> > + .hsub = 1,
> > }, {
> > .fourcc = DRM_FORMAT_BGRA5551,
> > .v4l2 = V4L2_PIX_FMT_BGRA555,
> > .bpp = 16,
> > .planes = 1,
> > + .hsub = 1,
> > }, {
> > .fourcc = DRM_FORMAT_BGRX5551,
> > .v4l2 = V4L2_PIX_FMT_BGRX555,
> > .bpp = 16,
> > .planes = 1,
> > + .hsub = 1,
> > }, {
> > .fourcc = DRM_FORMAT_BGR888,
> > .v4l2 = V4L2_PIX_FMT_RGB24,
> > .bpp = 24,
> > .planes = 1,
> > + .hsub = 1,
> > }, {
> > .fourcc = DRM_FORMAT_RGB888,
> > .v4l2 = V4L2_PIX_FMT_BGR24,
> > .bpp = 24,
> > .planes = 1,
> > + .hsub = 1,
> > }, {
> > .fourcc = DRM_FORMAT_RGBA8888,
> > .v4l2 = V4L2_PIX_FMT_BGRA32,
> > .bpp = 32,
> > .planes = 1,
> > + .hsub = 1,
> > }, {
> > .fourcc = DRM_FORMAT_RGBX8888,
> > .v4l2 = V4L2_PIX_FMT_BGRX32,
> > .bpp = 32,
> > .planes = 1,
> > + .hsub = 1,
> > }, {
> > .fourcc = DRM_FORMAT_ABGR8888,
> > .v4l2 = V4L2_PIX_FMT_RGBA32,
> > .bpp = 32,
> > .planes = 1,
> > + .hsub = 1,
> > }, {
> > .fourcc = DRM_FORMAT_XBGR8888,
> > .v4l2 = V4L2_PIX_FMT_RGBX32,
> > .bpp = 32,
> > .planes = 1,
> > + .hsub = 1,
> > }, {
> > .fourcc = DRM_FORMAT_BGRA8888,
> > .v4l2 = V4L2_PIX_FMT_ARGB32,
> > .bpp = 32,
> > .planes = 1,
> > + .hsub = 1,
> > }, {
> > .fourcc = DRM_FORMAT_BGRX8888,
> > .v4l2 = V4L2_PIX_FMT_XRGB32,
> > .bpp = 32,
> > .planes = 1,
> > + .hsub = 1,
> > }, {
> > .fourcc = DRM_FORMAT_YVYU,
> > .v4l2 = V4L2_PIX_FMT_YVYU,
> > .bpp = 16,
> > .planes = 1,
> > + .hsub = 2,
> > }, {
> > .fourcc = DRM_FORMAT_NV61,
> > .v4l2 = V4L2_PIX_FMT_NV61M,
> > .bpp = 16,
> > .planes = 2,
> > + .hsub = 2,
> > }, {
> > .fourcc = DRM_FORMAT_YUV420,
> > .v4l2 = V4L2_PIX_FMT_YUV420M,
> > .bpp = 12,
> > .planes = 3,
> > + .hsub = 2,
>
> I guess vertical subsampling is handled distinctly?
> (perhaps the height of the plane or such?)
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@xxxxxxxxxxxxxxxx>
>
> > return ERR_PTR(-EINVAL);
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.h b/drivers/gpu/drm/rcar-du/rcar_du_kms.h
> > index 0346504d8c59..8f5fff176754 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.h
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.h
> > @@ -22,6 +22,7 @@ struct rcar_du_format_info {
> > u32 v4l2;
> > unsigned int bpp;
> > unsigned int planes;
> > + unsigned int hsub;
> > unsigned int pnmr;
> > unsigned int edf;
> > };
--
Regards,
Laurent Pinchart
