Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <46e128a7767c1c4db226b8a4cef0701a336a3766.camel@ndufresne.ca>
Subject: Re: [PATCH v4 1/4] media: add nv12_8l128 and nv12_10be_8l128 video
 format.
From:   Nicolas Dufresne <nicolas@ndufresne.ca>
To:     Tommaso Merciai <tommaso.merciai@amarulasolutions.com>,
        Ming Qian <ming.qian@nxp.com>
Cc:     mchehab@kernel.org, hverkuil-cisco@xs4all.nl, shawnguo@kernel.org,
        robh+dt@kernel.org, s.hauer@pengutronix.de, kernel@pengutronix.de,
        festevam@gmail.com, linux-imx@nxp.com, xiahong.bao@nxp.com,
        linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org
Date:   Tue, 16 Aug 2022 10:42:53 -0400
In-Reply-To: <20220811151853.GB6390@tom-ThinkPad-T14s-Gen-2i>
References: <cover.1660027440.git.ming.qian@nxp.com>
         <84842bffb432884a0fd84de96c6e64ee2273e511.1660027440.git.ming.qian@nxp.com>
         <20220811151853.GB6390@tom-ThinkPad-T14s-Gen-2i>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4 (3.44.4-1.fc36) 
MIME-Version: 1.0
Precedence: bulk

Le jeudi 11 ao=C3=BBt 2022 =C3=A0 17:18 +0200, Tommaso Merciai a =C3=A9crit=
=C2=A0:
> Hi Ming,
>=20
> On Tue, Aug 09, 2022 at 02:50:38PM +0800, Ming Qian wrote:
> > add contiguous nv12 tiled format nv12_8l128 and nv12_10be_8l128
> >=20
> > Signed-off-by: Ming Qian <ming.qian@nxp.com>
> > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > ---
> >  .../userspace-api/media/v4l/pixfmt-yuv-planar.rst         | 8 ++++++++
> >  drivers/media/v4l2-core/v4l2-ioctl.c                      | 2 ++
> >  include/uapi/linux/videodev2.h                            | 2 ++
> >  3 files changed, 12 insertions(+)
> >=20
> > diff --git a/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rs=
t b/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst
> > index 10b1feeb0b57..f1d5bb7b806d 100644
> > --- a/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst
> > +++ b/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst
> > @@ -273,7 +273,9 @@ of the luma plane.
> >  .. _V4L2-PIX-FMT-NV12-16L16:
> >  .. _V4L2-PIX-FMT-NV12-32L32:
> >  .. _V4L2-PIX-FMT-NV12M-8L128:
> > +.. _V4L2-PIX-FMT-NV12-8L128:
> >  .. _V4L2-PIX-FMT-NV12M-10BE-8L128:
> > +.. _V4L2-PIX-FMT-NV12-10BE-8L128:
> >  .. _V4L2-PIX-FMT-MM21:
> > =20
> >  Tiled NV12
> > @@ -319,6 +321,9 @@ pixels in 2D 8x128 tiles, and stores tiles linearly=
 in memory.
> >  The image height must be aligned to a multiple of 128.
> >  The layouts of the luma and chroma planes are identical.
> > =20
> > +``V4L2_PIX_FMT_NV12_8L128`` is similar to ``V4L2_PIX_FMT_NV12M_8L128``=
 but stores
> > +two planes in one memory.
> > +
>=20
> Don't know, maybe we need more details here?
>=20
> >  ``V4L2_PIX_FMT_NV12M_10BE_8L128`` is similar to ``V4L2_PIX_FMT_NV12M``=
 but stores
> >  10 bits pixels in 2D 8x128 tiles, and stores tiles linearly in memory.
> >  the data is arranged in big endian order.
> > @@ -334,6 +339,9 @@ byte 2: Y1(bits 3-0) Y2(bits 9-6)
> >  byte 3: Y2(bits 5-0) Y3(bits 9-8)
> >  byte 4: Y3(bits 7-0)
> > =20
> > +``V4L2_PIX_FMT_NV12_10BE_8L128`` is similar to ``V4L2_PIX_FMT_NV12M_10=
BE_8L128`` but stores
> > +two planes in one memory.
> > +
>=20
> here also?

Perhaps I'm too much into it, but it felt clear to me. Note that the duplic=
ation
of pixel formats based on the number of allocation is pretty unique to V4L2=
, but
its also generalized to all planar formats. So adding a lot of documentatio=
n for
each specific format will be heavy. Note that one improvement I see, would =
be
s/memory/allocation/ , or memory allocation.

>=20
> >  ``V4L2_PIX_FMT_MM21`` store luma pixel in 16x32 tiles, and chroma pixe=
ls
> >  in 16x16 tiles. The line stride must be aligned to a multiple of 16 an=
d the
> >  image height must be aligned to a multiple of 32. The number of luma a=
nd chroma
> > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-=
core/v4l2-ioctl.c
> > index c314025d977e..d973bd2ff750 100644
> > --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> > @@ -1444,7 +1444,9 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc =
*fmt)
> >  	case V4L2_META_FMT_VIVID:       descr =3D "Vivid Metadata"; break;
> >  	case V4L2_META_FMT_RK_ISP1_PARAMS:	descr =3D "Rockchip ISP1 3A Parame=
ters"; break;
> >  	case V4L2_META_FMT_RK_ISP1_STAT_3A:	descr =3D "Rockchip ISP1 3A Stati=
stics"; break;
> > +	case V4L2_PIX_FMT_NV12_8L128:	descr =3D "NV12 (8x128 Linear)"; break;
> >  	case V4L2_PIX_FMT_NV12M_8L128:	descr =3D "NV12M (8x128 Linear)"; brea=
k;
> > +	case V4L2_PIX_FMT_NV12_10BE_8L128:	descr =3D "10-bit NV12 (8x128 Line=
ar, BE)"; break;
> >  	case V4L2_PIX_FMT_NV12M_10BE_8L128:	descr =3D "10-bit NV12M (8x128 Li=
near, BE)"; break;
> > =20
> >  	default:
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videod=
ev2.h
> > index cd66e01ed3c3..64f16490dd2b 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -655,6 +655,8 @@ struct v4l2_pix_format {
> >  #define V4L2_PIX_FMT_NV12_16L16 v4l2_fourcc('H', 'M', '1', '2') /* 12 =
 Y/CbCr 4:2:0 16x16 tiles */
> >  #define V4L2_PIX_FMT_NV12_32L32 v4l2_fourcc('S', 'T', '1', '2') /* 12 =
 Y/CbCr 4:2:0 32x32 tiles */
> >  #define V4L2_PIX_FMT_P010_4L4 v4l2_fourcc('T', '0', '1', '0') /* 12  Y=
/CbCr 4:2:0 10-bit 4x4 macroblocks */
> > +#define V4L2_PIX_FMT_NV12_8L128       v4l2_fourcc('A', 'T', '1', '2') =
/* Y/CbCr 4:2:0 8x128 tiles */
> > +#define V4L2_PIX_FMT_NV12_10BE_8L128  v4l2_fourcc_be('A', 'X', '1', '2=
') /* Y/CbCr 4:2:0 10-bit 8x128 tiles */
> > =20
> >  /* Tiled YUV formats, non contiguous planes */
> >  #define V4L2_PIX_FMT_NV12MT  v4l2_fourcc('T', 'M', '1', '2') /* 12  Y/=
CbCr 4:2:0 64x32 tiles */
> > --=20
> > 2.37.1
> >=20
>=20
> For the other parts look's good to me.
> Reviewed-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
>=20
> Regards,
> Tommaso
>=20