RE: [RESEND PATCH v2 4/4] media: chips-media: wave5: Support YUV422 raw pixel-formats on the encoder.

[jackson . lee]

Hi Nicolas

> -----Original Message-----
> From: Nicolas Dufresne <nicolas@xxxxxxxxxxxx>
> Sent: Tuesday, April 23, 2024 12:49 AM
> To: jackson.lee <jackson.lee@xxxxxxxxxxxxxxx>; mchehab@xxxxxxxxxx;
> sebastian.fricke@xxxxxxxxxxxxx
> Cc: linux-media@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> hverkuil@xxxxxxxxx; Nas Chung <nas.chung@xxxxxxxxxxxxxxx>; lafley.kim
> <lafley.kim@xxxxxxxxxxxxxxx>; b-brnich@xxxxxx
> Subject: Re: [RESEND PATCH v2 4/4] media: chips-media: wave5: Support YUV422
> raw pixel-formats on the encoder.
> 
> Hi Jackson,
> 
> Le lundi 22 avril 2024 à 04:30 +0000, jackson.lee a écrit :
> > Hi Nicolas
> >
> >
> > > -----Original Message-----
> > > From: Nicolas Dufresne <nicolas@xxxxxxxxxxxx>
> > > Sent: Friday, April 19, 2024 6:06 AM
> > > To: jackson.lee <jackson.lee@xxxxxxxxxxxxxxx>; mchehab@xxxxxxxxxx;
> > > sebastian.fricke@xxxxxxxxxxxxx
> > > Cc: linux-media@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > > hverkuil@xxxxxxxxx; Nas Chung <nas.chung@xxxxxxxxxxxxxxx>;
> > > lafley.kim <lafley.kim@xxxxxxxxxxxxxxx>; b-brnich@xxxxxx
> > > Subject: Re: [RESEND PATCH v2 4/4] media: chips-media: wave5:
> > > Support YUV422 raw pixel-formats on the encoder.
> > >
> > > Le lundi 11 mars 2024 à 19:56 +0900, jackson.lee a écrit :
> > > > From: "Jackson.lee" <jackson.lee@xxxxxxxxxxxxxxx>
> > > >
> > > > Add support for the YUV422P, NV16, NV61, YUV422M, NV16M, NV61M raw
> > > > pixel-
> > > formats to the Wave5 encoder.
> > > > All these formats have a chroma subsampling ratio of 4:2:2 and
> > > > therefore
> > > require a new image size calculation as the driver previously only
> > > handled a ratio of 4:2:0.
> > > >
> > > > Signed-off-by: Jackson.lee <jackson.lee@xxxxxxxxxxxxxxx>
> > > > Signed-off-by: Nas Chung <nas.chung@xxxxxxxxxxxxxxx>
> > > > ---
> > > >  .../chips-media/wave5/wave5-vpu-enc.c         | 59 +++++++++++++++++--
> > > >  1 file changed, 54 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git
> > > > a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> > > > b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> > > > index 5a32bb138158..77657f63a169 100644
> > > > --- a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> > > > +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> > > > @@ -39,6 +39,24 @@ static const struct vpu_format
> > > enc_fmt_list[FMT_TYPES][MAX_FMTS] = {
> > > >  		{
> > > >  			.v4l2_pix_fmt = V4L2_PIX_FMT_NV21M,
> > > >  		},
> > > > +		{
> > > > +			.v4l2_pix_fmt = V4L2_PIX_FMT_YUV422P,
> > > > +		},
> > > > +		{
> > > > +			.v4l2_pix_fmt = V4L2_PIX_FMT_NV16,
> > > > +		},
> > > > +		{
> > > > +			.v4l2_pix_fmt = V4L2_PIX_FMT_NV61,
> > > > +		},
> > > > +		{
> > > > +			.v4l2_pix_fmt = V4L2_PIX_FMT_YUV422M,
> > > > +		},
> > > > +		{
> > > > +			.v4l2_pix_fmt = V4L2_PIX_FMT_NV16M,
> > > > +		},
> > > > +		{
> > > > +			.v4l2_pix_fmt = V4L2_PIX_FMT_NV61M,
> > > > +		},
> > > >  	}
> > > >  };
> > > >
> > > > @@ -101,13 +119,30 @@ static int start_encode(struct vpu_instance
> > > > *inst,
> > > u32 *fail_res)
> > > >  	struct vb2_v4l2_buffer *dst_buf;
> > > >  	struct frame_buffer frame_buf;
> > > >  	struct enc_param pic_param;
> > > > -	u32 stride = ALIGN(inst->dst_fmt.width, 32);
> > > > -	u32 luma_size = (stride * inst->dst_fmt.height);
> > > > -	u32 chroma_size = ((stride / 2) * (inst->dst_fmt.height / 2));
> > > > +	u32 stride = inst->src_fmt.plane_fmt[0].bytesperline;
> > > > +	u32 luma_size = (stride * inst->src_fmt.height);

I reviewed the above code, again. The code would be proper.
Because in case of NV12 color space, it has one planar, therefor the YUV value is in the same linear memory.
To calculate luma_size, we need the above code.


> > > > +	u32 chroma_size = 0;
> > >
> > > The helper introduced in previous patch also calculate sizeimage for
> > > each planes, so no need for this code anymore.
> >
> > Your comment means the below code?
> >
> > 	u32 luma_size = inst->src_fmt.plane_fmt[0].sizeimage
> > 	u32 chroma_size = inst->src_fmt.plane_fmt[1].sizeimage
> >
> >
> > >
> > > >
> > > >  	memset(&pic_param, 0, sizeof(struct enc_param));
> > > >  	memset(&frame_buf, 0, sizeof(struct frame_buffer));
> > > >
> >
> > The below code could be removed.
> >
> > > > +	if (inst->src_fmt.pixelformat == V4L2_PIX_FMT_YUV420 ||
> > > > +	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_YUV420M)
> > > > +		chroma_size = luma_size / 4;
> > > > +	else if (inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV12 ||
> > > > +		 inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV21 ||
> > > > +		 inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV12M ||
> > > > +		 inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV21M)
> > > > +		chroma_size = luma_size / 2;
> > > > +	else if (inst->src_fmt.pixelformat == V4L2_PIX_FMT_YUV422P ||
> > > > +		 inst->src_fmt.pixelformat == V4L2_PIX_FMT_YUV422M)
> > > > +		chroma_size = luma_size / 2;
> > > > +	else if (inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV16 ||
> > > > +		 inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV61 ||
> > > > +		 inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV16M ||
> > > > +		 inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV61M)
> > > > +		chroma_size = luma_size;
> > > > +
> >
> > Is That right?
> 
> Yes, using the src_fmt seems accurate for the encoder.
> 
> cheers,
> Nicolas