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

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

 



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.. */
> > >
> >
>





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux