Re: [PATCH v4 1/4] media: add nv12_8l128 and nv12_10be_8l128 video format.

[Nicolas Dufresne]

Le jeudi 11 août 2022 à 17:18 +0200, Tommaso Merciai a écrit :
> Hi Ming,
> 
> On Tue, Aug 09, 2022 at 02:50:38PM +0800, Ming Qian wrote:
> > add contiguous nv12 tiled format nv12_8l128 and nv12_10be_8l128
> > 
> > Signed-off-by: Ming Qian <ming.qian@xxxxxxx>
> > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@xxxxxxxxxxxxx>
> > ---
> >  .../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(+)
> > 
> > diff --git a/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst 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:
> >  
> >  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.
> >  
> > +``V4L2_PIX_FMT_NV12_8L128`` is similar to ``V4L2_PIX_FMT_NV12M_8L128`` but stores
> > +two planes in one memory.
> > +
> 
> Don't know, maybe we need more details here?
> 
> >  ``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)
> >  
> > +``V4L2_PIX_FMT_NV12_10BE_8L128`` is similar to ``V4L2_PIX_FMT_NV12M_10BE_8L128`` but stores
> > +two planes in one memory.
> > +
> 
> here also?

Perhaps I'm too much into it, but it felt clear to me. Note that the duplication
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 documentation for
each specific format will be heavy. Note that one improvement I see, would be
s/memory/allocation/ , or memory allocation.

> 
> >  ``V4L2_PIX_FMT_MM21`` store luma pixel in 16x32 tiles, and chroma pixels
> >  in 16x16 tiles. The line stride must be aligned to a multiple of 16 and the
> >  image height must be aligned to a multiple of 32. The number of luma and 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 = "Vivid Metadata"; break;
> >  	case V4L2_META_FMT_RK_ISP1_PARAMS:	descr = "Rockchip ISP1 3A Parameters"; break;
> >  	case V4L2_META_FMT_RK_ISP1_STAT_3A:	descr = "Rockchip ISP1 3A Statistics"; break;
> > +	case V4L2_PIX_FMT_NV12_8L128:	descr = "NV12 (8x128 Linear)"; break;
> >  	case V4L2_PIX_FMT_NV12M_8L128:	descr = "NV12M (8x128 Linear)"; break;
> > +	case V4L2_PIX_FMT_NV12_10BE_8L128:	descr = "10-bit NV12 (8x128 Linear, BE)"; break;
> >  	case V4L2_PIX_FMT_NV12M_10BE_8L128:	descr = "10-bit NV12M (8x128 Linear, BE)"; break;
> >  
> >  	default:
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.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 */
> >  
> >  /* 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 */
> > -- 
> > 2.37.1
> > 
> 
> For the other parts look's good to me.
> Reviewed-by: Tommaso Merciai <tommaso.merciai@xxxxxxxxxxxxxxxxxxxx>
> 
> Regards,
> Tommaso
>