From: "Jackson.lee" <[email protected]>
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 <[email protected]>
Signed-off-by: Nas Chung <[email protected]>
---
.../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;
+
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;
--
2.43.0
Hi Nas,
Le mardi 30 avril 2024 à 10:39 +0900, Nas Chung a écrit :
> From: "Jackson.lee" <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Nas Chung <[email protected]>
> ---
> .../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
Hi Nicolas
> -----Original Message-----
> From: Nicolas Dufresne <[email protected]>
> Sent: Thursday, May 2, 2024 4:46 AM
> To: Nas Chung <[email protected]>; [email protected];
> [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; lafley.kim <[email protected]>; [email protected];
> [email protected]; jackson.lee <[email protected]>
> 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" <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Nas Chung <[email protected]>
> > ---
> > .../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