[v3,4/4] media: chips-media: wave5: Support YUV422 raw pixel-formats on the encoder.

Message ID 20240430013900.187-5-nas.chung@chipsnmedia.com (mailing list archive)
State Superseded
Delegated to: Sebastian Fricke
Headers
Series Add features to an existing driver |

Commit Message

Nas Chung April 30, 2024, 1:39 a.m. UTC
  From: "Jackson.lee" <jackson.lee@chipsnmedia.com>

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@chipsnmedia.com>
Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
---
 .../chips-media/wave5/wave5-vpu-enc.c         | 59 +++++++++++++++++--
 1 file changed, 54 insertions(+), 5 deletions(-)
  

Comments

Nicolas Dufresne May 1, 2024, 7:46 p.m. UTC | #1
Hi Nas,

Le mardi 30 avril 2024 à 10:39 +0900, Nas Chung a écrit :
> From: "Jackson.lee" <jackson.lee@chipsnmedia.com>
> 
> 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.

Same here, run check-patch, before sending your next version, it should tell you
that this message is not indented properly.

> 
> Signed-off-by: Jackson.lee <jackson.lee@chipsnmedia.com>
> Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
> ---
>  .../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 75d230df45f6..0d6bec4e28d1 100644
> --- a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> @@ -66,6 +66,24 @@ static const struct vpu_format enc_fmt_list[FMT_TYPES][MAX_FMTS] = {
>  			.v4l2_pix_fmt = V4L2_PIX_FMT_NV21M,
>  			.v4l2_frmsize = &enc_frmsize[VPU_FMT_TYPE_RAW],
>  		},
> +		{
> +			.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,
> +		},
>  	}
>  };
>  
> @@ -109,13 +127,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);
> +	u32 chroma_size = 0;
>  
>  	memset(&pic_param, 0, sizeof(struct enc_param));
>  	memset(&frame_buf, 0, sizeof(struct frame_buffer));
>  
> +	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;
> +

I'm still unhappy to see all the supported format having to be listed again
here, this is error prone and a maintenance burden. In general, what I would do
is (and this is simplified to the subset of format we support):

// might want to bug on that the info->pixel_encoding == V4L2_PIXEL_ENC_YUV
// if you believe some RGB or bayer formats could be added in the future and
// want the devs to notice. I've ignored fractional bytes-per-pixel values as
// we don't use that, but another bugon if there is a chance the firmware 
// will support more complex packing.

info = v4l2_format_info(inst->src_fmt.pixelformat);
if (info->mem_planes == 1) {
	luma_size = stride * inst->dst_fmt.height;
	chroma_size = luma_size * info->bpp[1] / (info->hdiv * info->vdiv)
} else {
	luma_size = inst->src_fmt.plane_fmt[0].sizeimage;
	chroma_size = inst->src_fmt.plane_fmt[1].sizeimage;
}

Or something similar that works ... (untested code above)

>  	dst_buf = v4l2_m2m_next_dst_buf(m2m_ctx);
>  	if (!dst_buf) {
>  		dev_dbg(inst->dev->dev, "%s: No destination buffer found\n", __func__);
> @@ -501,11 +536,15 @@ static int wave5_vpu_enc_s_fmt_out(struct file *file, void *fh, struct v4l2_form
>  	}
>  
>  	if (inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV12 ||
> -	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV12M) {
> +	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV12M ||
> +	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV16 ||
> +	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV16M) {
>  		inst->cbcr_interleave = true;
>  		inst->nv21 = false;
>  	} else if (inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV21 ||
> -		   inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV21M) {
> +		   inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV21M ||
> +		   inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV61 ||
> +		   inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV61M) {
>  		inst->cbcr_interleave = true;

This can be simplified to (avoiding enumerating formats):
 
	inst->cbcr_interleave = (info->comp_planes == 2) ? true : false;

>  		inst->nv21 = true;

Could be something to add into the info in the future, but for now this is list
of formats is needed. Would be a lot more efficient with a switch, but not a hot
path so not making this mandatory.

>  	} else {
> @@ -1102,6 +1141,16 @@ static void wave5_set_enc_openparam(struct enc_open_param *open_param,
>  	u32 num_ctu_row = ALIGN(inst->dst_fmt.height, 64) / 64;
>  	u32 num_mb_row = ALIGN(inst->dst_fmt.height, 16) / 16;
>  
> +	if (inst->src_fmt.pixelformat == V4L2_PIX_FMT_YUV422P ||
> +	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV16 ||
> +	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV61 ||
> +	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_YUV422M ||
> +	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV16M ||
> +	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV61M)
> +		open_param->src_format = FORMAT_422;
> +	else
> +		open_param->src_format = FORMAT_420;

Can be simplified to:

if (info->hdiv == 2 && info->vdiv == 2)
	open_param->src_format = FORMAT_422;
else if (info->hdiv == 2 && info->vdiv == 1)
	open_param->src_format = FORMAT_420;

> +
>  	open_param->wave_param.gop_preset_idx = PRESET_IDX_IPP_SINGLE;
>  	open_param->wave_param.hvs_qp_scale = 2;
>  	open_param->wave_param.hvs_max_delta_qp = 10;

regards,
Nicolas
  
Jackson.lee May 2, 2024, 4:55 a.m. UTC | #2
Hi Nicolas

> -----Original Message-----
> From: Nicolas Dufresne <nicolas@ndufresne.ca>
> Sent: Thursday, May 2, 2024 4:46 AM
> To: Nas Chung <nas.chung@chipsnmedia.com>; mchehab@kernel.org;
> sebastian.fricke@collabora.com
> Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> hverkuil@xs4all.nl; lafley.kim <lafley.kim@chipsnmedia.com>; b-brnich@ti.com;
> jackson.lee@chipnsmedia.com; jackson.lee <jackson.lee@chipsnmedia.com>
> Subject: Re: [PATCH v3 4/4] media: chips-media: wave5: Support YUV422 raw
> pixel-formats on the encoder.
> 
> Hi Nas,
> 
> Le mardi 30 avril 2024 à 10:39 +0900, Nas Chung a écrit :
> > From: "Jackson.lee" <jackson.lee@chipsnmedia.com>
> >
> > 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.
> 
> Same here, run check-patch, before sending your next version, it should tell
> you that this message is not indented properly.
> 
> >
> > Signed-off-by: Jackson.lee <jackson.lee@chipsnmedia.com>
> > Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
> > ---
> >  .../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 75d230df45f6..0d6bec4e28d1 100644
> > --- a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> > +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> > @@ -66,6 +66,24 @@ static const struct vpu_format
> enc_fmt_list[FMT_TYPES][MAX_FMTS] = {
> >  			.v4l2_pix_fmt = V4L2_PIX_FMT_NV21M,
> >  			.v4l2_frmsize = &enc_frmsize[VPU_FMT_TYPE_RAW],
> >  		},
> > +		{
> > +			.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,
> > +		},
> >  	}
> >  };
> >
> > @@ -109,13 +127,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);
> > +	u32 chroma_size = 0;
> >
> >  	memset(&pic_param, 0, sizeof(struct enc_param));
> >  	memset(&frame_buf, 0, sizeof(struct frame_buffer));
> >
> > +	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;
> > +
> 
> I'm still unhappy to see all the supported format having to be listed again
> here, this is error prone and a maintenance burden. In general, what I would
> do is (and this is simplified to the subset of format we support):
> 
> // might want to bug on that the info->pixel_encoding == V4L2_PIXEL_ENC_YUV
> // if you believe some RGB or bayer formats could be added in the future and
> // want the devs to notice. I've ignored fractional bytes-per-pixel values as
> // we don't use that, but another bugon if there is a chance the firmware //
> will support more complex packing.
> 
> info = v4l2_format_info(inst->src_fmt.pixelformat);
> if (info->mem_planes == 1) {
> 	luma_size = stride * inst->dst_fmt.height;
> 	chroma_size = luma_size * info->bpp[1] / (info->hdiv * info->vdiv) }
> else {
> 	luma_size = inst->src_fmt.plane_fmt[0].sizeimage;
> 	chroma_size = inst->src_fmt.plane_fmt[1].sizeimage;
> }
> 
> Or something similar that works ... (untested code above)
> 
> >  	dst_buf = v4l2_m2m_next_dst_buf(m2m_ctx);
> >  	if (!dst_buf) {
> >  		dev_dbg(inst->dev->dev, "%s: No destination buffer found\n",
> > __func__); @@ -501,11 +536,15 @@ static int wave5_vpu_enc_s_fmt_out(struct
> file *file, void *fh, struct v4l2_form
> >  	}
> >
> >  	if (inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV12 ||
> > -	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV12M) {
> > +	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV12M ||
> > +	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV16 ||
> > +	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV16M) {
> >  		inst->cbcr_interleave = true;
> >  		inst->nv21 = false;
> >  	} else if (inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV21 ||
> > -		   inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV21M) {
> > +		   inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV21M ||
> > +		   inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV61 ||
> > +		   inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV61M) {
> >  		inst->cbcr_interleave = true;
> 
> This can be simplified to (avoiding enumerating formats):
> 
> 	inst->cbcr_interleave = (info->comp_planes == 2) ? true : false;
> 
> >  		inst->nv21 = true;
> 
> Could be something to add into the info in the future, but for now this is
> list of formats is needed. Would be a lot more efficient with a switch, but
> not a hot path so not making this mandatory.
> 

Switch(pixel_format) {
Case V4L2_PIX_FMT_NV21:
Case V4L2_PIX_FMT_NV21M:
Case V4L2_PIX_FMT_NV61:
Case V4L2_PIX_FMT_NV61M:
	inst->nv21 = true;
Default:
	inst->nv21 = false;
}

What you are saying is that to set the nv21 variable(using switch) ?



> >  	} else {
> > @@ -1102,6 +1141,16 @@ static void wave5_set_enc_openparam(struct
> enc_open_param *open_param,
> >  	u32 num_ctu_row = ALIGN(inst->dst_fmt.height, 64) / 64;
> >  	u32 num_mb_row = ALIGN(inst->dst_fmt.height, 16) / 16;
> >
> > +	if (inst->src_fmt.pixelformat == V4L2_PIX_FMT_YUV422P ||
> > +	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV16 ||
> > +	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV61 ||
> > +	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_YUV422M ||
> > +	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV16M ||
> > +	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV61M)
> > +		open_param->src_format = FORMAT_422;
> > +	else
> > +		open_param->src_format = FORMAT_420;
> 
> Can be simplified to:
> 
> if (info->hdiv == 2 && info->vdiv == 2)
> 	open_param->src_format = FORMAT_422;
> else if (info->hdiv == 2 && info->vdiv == 1)
> 	open_param->src_format = FORMAT_420;
> 

I think the above codes should be changed like below.(we should swap the values of the src_format variable.)

 if (info->hdiv == 2 && info->vdiv == 2)
 	open_param->src_format = FORMAT_420;
 else if (info->hdiv == 2 && info->vdiv == 1)
 	open_param->src_format = FORMAT_422;





> > +
> >  	open_param->wave_param.gop_preset_idx = PRESET_IDX_IPP_SINGLE;
> >  	open_param->wave_param.hvs_qp_scale = 2;
> >  	open_param->wave_param.hvs_max_delta_qp = 10;
> 
> regards,
> Nicolas
  
Nicolas Dufresne May 14, 2024, 7:59 p.m. UTC | #3
Hi,

sorry for the delay

Le jeudi 02 mai 2024 à 04:55 +0000, jackson.lee a écrit :
> Hi Nicolas
> 
> > -----Original Message-----
> > From: Nicolas Dufresne <nicolas@ndufresne.ca>
> > Sent: Thursday, May 2, 2024 4:46 AM
> > To: Nas Chung <nas.chung@chipsnmedia.com>; mchehab@kernel.org;
> > sebastian.fricke@collabora.com
> > Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> > hverkuil@xs4all.nl; lafley.kim <lafley.kim@chipsnmedia.com>; b-brnich@ti.com;
> > jackson.lee@chipnsmedia.com; jackson.lee <jackson.lee@chipsnmedia.com>
> > Subject: Re: [PATCH v3 4/4] media: chips-media: wave5: Support YUV422 raw
> > pixel-formats on the encoder.
> > 
> > Hi Nas,
> > 
> > Le mardi 30 avril 2024 à 10:39 +0900, Nas Chung a écrit :
> > > From: "Jackson.lee" <jackson.lee@chipsnmedia.com>
> > > 
> > > 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.
> > 
> > Same here, run check-patch, before sending your next version, it should tell
> > you that this message is not indented properly.
> > 
> > > 
> > > Signed-off-by: Jackson.lee <jackson.lee@chipsnmedia.com>
> > > Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
> > > ---
> > >  .../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 75d230df45f6..0d6bec4e28d1 100644
> > > --- a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> > > +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> > > @@ -66,6 +66,24 @@ static const struct vpu_format
> > enc_fmt_list[FMT_TYPES][MAX_FMTS] = {
> > >  			.v4l2_pix_fmt = V4L2_PIX_FMT_NV21M,
> > >  			.v4l2_frmsize = &enc_frmsize[VPU_FMT_TYPE_RAW],
> > >  		},
> > > +		{
> > > +			.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,
> > > +		},
> > >  	}
> > >  };
> > > 
> > > @@ -109,13 +127,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);
> > > +	u32 chroma_size = 0;
> > > 
> > >  	memset(&pic_param, 0, sizeof(struct enc_param));
> > >  	memset(&frame_buf, 0, sizeof(struct frame_buffer));
> > > 
> > > +	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;
> > > +
> > 
> > I'm still unhappy to see all the supported format having to be listed again
> > here, this is error prone and a maintenance burden. In general, what I would
> > do is (and this is simplified to the subset of format we support):
> > 
> > // might want to bug on that the info->pixel_encoding == V4L2_PIXEL_ENC_YUV
> > // if you believe some RGB or bayer formats could be added in the future and
> > // want the devs to notice. I've ignored fractional bytes-per-pixel values as
> > // we don't use that, but another bugon if there is a chance the firmware //
> > will support more complex packing.
> > 
> > info = v4l2_format_info(inst->src_fmt.pixelformat);
> > if (info->mem_planes == 1) {
> > 	luma_size = stride * inst->dst_fmt.height;
> > 	chroma_size = luma_size * info->bpp[1] / (info->hdiv * info->vdiv) }
> > else {
> > 	luma_size = inst->src_fmt.plane_fmt[0].sizeimage;
> > 	chroma_size = inst->src_fmt.plane_fmt[1].sizeimage;
> > }
> > 
> > Or something similar that works ... (untested code above)
> > 
> > >  	dst_buf = v4l2_m2m_next_dst_buf(m2m_ctx);
> > >  	if (!dst_buf) {
> > >  		dev_dbg(inst->dev->dev, "%s: No destination buffer found\n",
> > > __func__); @@ -501,11 +536,15 @@ static int wave5_vpu_enc_s_fmt_out(struct
> > file *file, void *fh, struct v4l2_form
> > >  	}
> > > 
> > >  	if (inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV12 ||
> > > -	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV12M) {
> > > +	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV12M ||
> > > +	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV16 ||
> > > +	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV16M) {
> > >  		inst->cbcr_interleave = true;
> > >  		inst->nv21 = false;
> > >  	} else if (inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV21 ||
> > > -		   inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV21M) {
> > > +		   inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV21M ||
> > > +		   inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV61 ||
> > > +		   inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV61M) {
> > >  		inst->cbcr_interleave = true;
> > 
> > This can be simplified to (avoiding enumerating formats):
> > 
> > 	inst->cbcr_interleave = (info->comp_planes == 2) ? true : false;
> > 
> > >  		inst->nv21 = true;
> > 
> > Could be something to add into the info in the future, but for now this is
> > list of formats is needed. Would be a lot more efficient with a switch, but
> > not a hot path so not making this mandatory.
> > 
> 
> Switch(pixel_format) {
> Case V4L2_PIX_FMT_NV21:
> Case V4L2_PIX_FMT_NV21M:
> Case V4L2_PIX_FMT_NV61:
> Case V4L2_PIX_FMT_NV61M:
> 	inst->nv21 = true;
> Default:
> 	inst->nv21 = false;
> }
> 
> What you are saying is that to set the nv21 variable(using switch) ?

Yes, it seems easier to maintain to me.

> 
> 
> 
> > >  	} else {
> > > @@ -1102,6 +1141,16 @@ static void wave5_set_enc_openparam(struct
> > enc_open_param *open_param,
> > >  	u32 num_ctu_row = ALIGN(inst->dst_fmt.height, 64) / 64;
> > >  	u32 num_mb_row = ALIGN(inst->dst_fmt.height, 16) / 16;
> > > 
> > > +	if (inst->src_fmt.pixelformat == V4L2_PIX_FMT_YUV422P ||
> > > +	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV16 ||
> > > +	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV61 ||
> > > +	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_YUV422M ||
> > > +	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV16M ||
> > > +	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV61M)
> > > +		open_param->src_format = FORMAT_422;
> > > +	else
> > > +		open_param->src_format = FORMAT_420;
> > 
> > Can be simplified to:
> > 
> > if (info->hdiv == 2 && info->vdiv == 2)
> > 	open_param->src_format = FORMAT_422;
> > else if (info->hdiv == 2 && info->vdiv == 1)
> > 	open_param->src_format = FORMAT_420;
> > 
> 
> I think the above codes should be changed like below.(we should swap the values of the src_format variable.)
> 
>  if (info->hdiv == 2 && info->vdiv == 2)
>  	open_param->src_format = FORMAT_420;
>  else if (info->hdiv == 2 && info->vdiv == 1)
>  	open_param->src_format = FORMAT_422;
> 

Good catch.

> 
> > > +
> > >  	open_param->wave_param.gop_preset_idx = PRESET_IDX_IPP_SINGLE;
> > >  	open_param->wave_param.hvs_qp_scale = 2;
> > >  	open_param->wave_param.hvs_max_delta_qp = 10;
> > 
> > regards,
> > Nicolas
  

Patch

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 75d230df45f6..0d6bec4e28d1 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
@@ -66,6 +66,24 @@  static const struct vpu_format enc_fmt_list[FMT_TYPES][MAX_FMTS] = {
 			.v4l2_pix_fmt = V4L2_PIX_FMT_NV21M,
 			.v4l2_frmsize = &enc_frmsize[VPU_FMT_TYPE_RAW],
 		},
+		{
+			.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,
+		},
 	}
 };
 
@@ -109,13 +127,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);
+	u32 chroma_size = 0;
 
 	memset(&pic_param, 0, sizeof(struct enc_param));
 	memset(&frame_buf, 0, sizeof(struct frame_buffer));
 
+	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;
+
 	dst_buf = v4l2_m2m_next_dst_buf(m2m_ctx);
 	if (!dst_buf) {
 		dev_dbg(inst->dev->dev, "%s: No destination buffer found\n", __func__);
@@ -501,11 +536,15 @@  static int wave5_vpu_enc_s_fmt_out(struct file *file, void *fh, struct v4l2_form
 	}
 
 	if (inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV12 ||
-	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV12M) {
+	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV12M ||
+	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV16 ||
+	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV16M) {
 		inst->cbcr_interleave = true;
 		inst->nv21 = false;
 	} else if (inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV21 ||
-		   inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV21M) {
+		   inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV21M ||
+		   inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV61 ||
+		   inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV61M) {
 		inst->cbcr_interleave = true;
 		inst->nv21 = true;
 	} else {
@@ -1102,6 +1141,16 @@  static void wave5_set_enc_openparam(struct enc_open_param *open_param,
 	u32 num_ctu_row = ALIGN(inst->dst_fmt.height, 64) / 64;
 	u32 num_mb_row = ALIGN(inst->dst_fmt.height, 16) / 16;
 
+	if (inst->src_fmt.pixelformat == V4L2_PIX_FMT_YUV422P ||
+	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV16 ||
+	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV61 ||
+	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_YUV422M ||
+	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV16M ||
+	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV61M)
+		open_param->src_format = FORMAT_422;
+	else
+		open_param->src_format = FORMAT_420;
+
 	open_param->wave_param.gop_preset_idx = PRESET_IDX_IPP_SINGLE;
 	open_param->wave_param.hvs_qp_scale = 2;
 	open_param->wave_param.hvs_max_delta_qp = 10;