On Mon, 6 Jan 2025 at 20:52, Nicolas Dufresne <nicolas@xxxxxxxxxxxx> wrote: > > 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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> > > > --- > > > 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. The current DRM_FORMAT_MOD_BROADCOM_SAND128 taking the column height as the parameter is apparently contrary to how DRM modifiers are meant to be used. You're not allowed to have a genuine runtime parameter in there, and all potential values for parameters have to be listed out in the in_formats blob. I have a patch on [1] to add DRM_FORMAT_MOD_BROADCOM_SAND128(0) as a modifier which then takes the height passed in to addFB2 to be the column height. With luma and chroma now in independent buffers via the multi-planar API, that solves the problem, and we don't need a new modifier. I will submit that to dri-devel in the next week or so. I've pinged Igalia to sort the equivalent Mesa patch for me. Dave [1] https://github.com/6by9/linux/commits/rpi-6.12.y-hevc_dec patch "drm/vc4: Add algorithmic handling for SAND". I can't give a hash as I rebase that branch. > 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.. */ > > > > > >