Re: [PATCH 3/7] media: ioctl: Add pixel formats NV12MT_COL128 and NV12MT_10_COL128

[Nicolas Dufresne <nicolas@ndufresne.ca>]

Le jeudi 02 janvier 2025 à 12:52 +0000, Dave Stevenson a écrit :
> Hi Robert
> 
> Resending this reply as replying from my phone before the Christmas
> break sent it as HTML :-(
> 
> On Sat, 21 Dec 2024 at 17:38, Robert Mader <robert.mader@collabora.com> wrote:
> > 
> > Hi, thanks for the patch.
> > 
> > On 20.12.24 17:21, Dave Stevenson wrote:
> > 
> > Add V4L2_PIXFMT_NV12MT_COL128 and V4L2_PIXFMT_NV12MT_10_COL128
> > to describe the Raspberry Pi HEVC decoder NV12 multiplanar formats.
> > 
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >  drivers/media/v4l2-core/v4l2-ioctl.c | 2 ++
> >  include/uapi/linux/videodev2.h       | 5 +++++
> >  2 files changed, 7 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> > index 0304daa8471d..e510e375a871 100644
> > --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> > @@ -1377,7 +1377,9 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
> >   case V4L2_PIX_FMT_NV16M: descr = "Y/UV 4:2:2 (N-C)"; break;
> >   case V4L2_PIX_FMT_NV61M: descr = "Y/VU 4:2:2 (N-C)"; break;
> >   case V4L2_PIX_FMT_NV12MT: descr = "Y/UV 4:2:0 (64x32 MB, N-C)"; break;
> > + case V4L2_PIX_FMT_NV12MT_COL128: descr = "Y/CbCr 4:2:0 (128b cols)"; break;
> >   case V4L2_PIX_FMT_NV12MT_16X16: descr = "Y/UV 4:2:0 (16x16 MB, N-C)"; break;
> > + case V4L2_PIX_FMT_NV12MT_10_COL128: descr = "10-bit Y/CbCr 4:2:0 (128b cols)"; break;
> >   case V4L2_PIX_FMT_P012M: descr = "12-bit Y/UV 4:2:0 (N-C)"; break;
> >   case V4L2_PIX_FMT_YUV420M: descr = "Planar YUV 4:2:0 (N-C)"; break;
> >   case V4L2_PIX_FMT_YVU420M: descr = "Planar YVU 4:2:0 (N-C)"; break;
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index e7c4dce39007..f8f97aa6a616 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -687,6 +687,11 @@ struct v4l2_pix_format {
> >  #define V4L2_PIX_FMT_NV12MT_16X16 v4l2_fourcc('V', 'M', '1', '2') /* 12  Y/CbCr 4:2:0 16x16 tiles */
> >  #define V4L2_PIX_FMT_NV12M_8L128      v4l2_fourcc('N', 'A', '1', '2') /* Y/CbCr 4:2:0 8x128 tiles */
> >  #define V4L2_PIX_FMT_NV12M_10BE_8L128 v4l2_fourcc_be('N', 'T', '1', '2') /* Y/CbCr 4:2:0 10-bit 8x128 tiles */
> > +#define V4L2_PIX_FMT_NV12MT_COL128 v4l2_fourcc('N', 'c', '1', '2') /* 12  Y/CbCr 4:2:0 128 pixel wide column */
> > +#define V4L2_PIX_FMT_NV12MT_10_COL128 v4l2_fourcc('N', 'c', '3', '0')
> > 
> > Should these be upper-case Cs instead? So they compatible with the previously used downstream values?
> 
> No, this is deliberate.
> 
> Downstream was using a single planar format, with extra complexity for
> determining the chroma offset per column, and weird handling required
> on bytesperline.
> Having had discussions, switching to a multiplanar format (hence MT in
> the name) removes those complexities and means we don't need to do
> anything weird in the v4l2 format definitions.
> 
> Reusing NC12 and NC30 fourccs will give us grief over backwards
> compatibility, hence lower case for the new version.
> 
> I have a patch on [1] that adds back in NC12 and NC30 for downstream
> just so we don't break existing users, but see no point in upstreaming
> that.

Yes, I think its fair to avoid incompatibility there. Are there matching Mesa
patches coming, since without that we are back to square one, where the format
remains unusable. NC12 have a matching (mainline) DRM_FORMAT_NV12 
DRM_FORMAT_MOD_BROADCOM_SAND128 pair. I believe a new modifier is needed and
will serve both Nc12/30.

Nicolas

> 
> > P.S.: Coincidentally Gstreamer just landed support for NC12 in https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/7355 and there is also a link to experimental NC30 patches. So happy to see this series appear upstream :)
> 
> Ooh, nice. I'll give it a test when I'm back in the office.
> 
> Dave
> 
> [1] https://github.com/6by9/linux/commits/rpi-6.12.y-hevc_dec/
> 
> > + /* Y/CbCr 4:2:0 10bpc, 3x10 packed as 4 bytes in
> > + * a 128 bytes / 96 pixel wide column */
> > +
> > 
> >  /* Bayer formats - see http://www.siliconimaging.com/RGB%20Bayer.htm */
> >  #define V4L2_PIX_FMT_SBGGR8  v4l2_fourcc('B', 'A', '8', '1') /*  8  BGBG.. GRGR.. */
> > 
>