Hello Nicolas
Thanks for your review, we will review them.
> -----Original Message-----
> From: Nicolas Dufresne <nicolas@xxxxxxxxxxxx>
> Sent: Thursday, February 8, 2024 2:55 AM
> To: jackson.lee <jackson.lee@xxxxxxxxxxxxxxx>; mchehab@xxxxxxxxxx; linux-
> media@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Nas Chung
> <nas.chung@xxxxxxxxxxxxxxx>
> Cc: lafley.kim <lafley.kim@xxxxxxxxxxxxxxx>; b-brnich@xxxxxx
> Subject: Re: [RESEND PATCH v0 1/5] wave5 : Support yuv422 input format for
> encoder.
>
> Hi Jackson,
>
> Le mercredi 31 janvier 2024 à 10:30 +0900, jackson.lee a écrit :
> > Encoder supports the following formats.
> > YUV422P, NV16, NV61, NV16M, NV61M
> >
> > Signed-off-by: Jackson Lee <jackson.lee@xxxxxxxxxxxxxxx>
> > Signed-off-by: Nas Chung <nas.chung@xxxxxxxxxxxxxxx>
> > ---
> > .../chips-media/wave5/wave5-vpu-enc.c | 79 ++++++++++++++++++-
> > 1 file changed, 76 insertions(+), 3 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 f29cfa3af94a..0cb5bfb67258 100644
> > --- a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> > +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> > @@ -70,6 +70,41 @@ static const struct vpu_format
> enc_fmt_list[FMT_TYPES][MAX_FMTS] = {
> > .max_height = W5_MAX_ENC_PIC_HEIGHT,
> > .min_height = W5_MIN_ENC_PIC_HEIGHT,
> > },
> > + {
> > + .v4l2_pix_fmt = V4L2_PIX_FMT_YUV422P,
> > + .max_width = W5_MAX_ENC_PIC_WIDTH,
> > + .min_width = W5_MIN_ENC_PIC_WIDTH,
> > + .max_height = W5_MAX_ENC_PIC_HEIGHT,
> > + .min_height = W5_MIN_ENC_PIC_HEIGHT,
> > + },
>
> During upstreaming, we discussed the lack of usage of v4l2-common in this
> driver and agreed that future updates such as this one should first port
> the driver to use the common helpers instead.
>
> This implies dropping this custom made structure in favour of
> v4l2_frmsize_stepwise structure. Unlike yours, you can encoded the needed
> padding, allowing to encode this in one place instead of spreading it
> across numerous formulas in the code.
>
> With this information, you will be able to use:
>
> v4l2_apply_frmsize_constraints()
> v4l2_fill_pixfmt_mp()
>
> To adjust your dimensions to padded dimensions and compute your
> bytesperline
> (stride) and sizeimage. You can of course increase the size image after
> this call. You can have a look at rkvdec driver as an example.
>
> Please port existing set of pixel formats support, and then add the new
> pixel formats. This should remove about 3/4 of this patch and remove that
> huge risk of miss-computing a size.
>
> > + {
> > + .v4l2_pix_fmt = V4L2_PIX_FMT_NV16,
> > + .max_width = W5_MAX_ENC_PIC_WIDTH,
> > + .min_width = W5_MIN_ENC_PIC_WIDTH,
> > + .max_height = W5_MAX_ENC_PIC_HEIGHT,
> > + .min_height = W5_MIN_ENC_PIC_HEIGHT,
> > + },
> > + {
> > + .v4l2_pix_fmt = V4L2_PIX_FMT_NV61,
> > + .max_width = W5_MAX_ENC_PIC_WIDTH,
> > + .min_width = W5_MIN_ENC_PIC_WIDTH,
> > + .max_height = W5_MAX_ENC_PIC_HEIGHT,
> > + .min_height = W5_MIN_ENC_PIC_HEIGHT,
> > + },
> > + {
> > + .v4l2_pix_fmt = V4L2_PIX_FMT_NV16M,
> > + .max_width = W5_MAX_ENC_PIC_WIDTH,
> > + .min_width = W5_MIN_ENC_PIC_WIDTH,
> > + .max_height = W5_MAX_ENC_PIC_HEIGHT,
> > + .min_height = W5_MIN_ENC_PIC_HEIGHT,
> > + },
> > + {
> > + .v4l2_pix_fmt = V4L2_PIX_FMT_NV61M,
> > + .max_width = W5_MAX_ENC_PIC_WIDTH,
> > + .min_width = W5_MIN_ENC_PIC_WIDTH,
> > + .max_height = W5_MAX_ENC_PIC_HEIGHT,
> > + .min_height = W5_MIN_ENC_PIC_HEIGHT,
> > + },
> > }
> > };
> >
> > @@ -136,6 +171,23 @@ static void wave5_update_pix_fmt(struct
> v4l2_pix_format_mplane *pix_mp, unsigned
> > pix_mp->plane_fmt[1].bytesperline = round_up(width, 32);
> > pix_mp->plane_fmt[1].sizeimage = round_up(width, 32) *
> height / 2;
> > break;
> > + case V4L2_PIX_FMT_YUV422P:
> > + case V4L2_PIX_FMT_NV16:
> > + case V4L2_PIX_FMT_NV61:
> > + pix_mp->width = width;
> > + pix_mp->height = height;
> > + pix_mp->plane_fmt[0].bytesperline = round_up(width, 32);
> > + pix_mp->plane_fmt[0].sizeimage = round_up(width, 32) *
> height * 2;
> > + break;
> > + case V4L2_PIX_FMT_NV16M:
> > + case V4L2_PIX_FMT_NV61M:
> > + pix_mp->width = width;
> > + pix_mp->height = height;
> > + pix_mp->plane_fmt[0].bytesperline = round_up(width, 32);
> > + pix_mp->plane_fmt[0].sizeimage = round_up(width, 32) *
> height;
> > + pix_mp->plane_fmt[1].bytesperline = round_up(width, 32);
> > + pix_mp->plane_fmt[1].sizeimage = round_up(width, 32) *
> height;
> > + break;
> > default:
> > pix_mp->width = width;
> > pix_mp->height = height;
> > @@ -155,11 +207,19 @@ static int start_encode(struct vpu_instance *inst,
> u32 *fail_res)
> > 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 chroma_size;
> >
> > 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 = ((stride / 2) * (inst->dst_fmt.height / 2));
> > + else if (inst->src_fmt.pixelformat == V4L2_PIX_FMT_YUV422P)
> > + chroma_size = ((stride) * (inst->dst_fmt.height / 2));
> > + else
> > + chroma_size = 0;
> > +
> > dst_buf = v4l2_m2m_next_dst_buf(m2m_ctx);
> > if (!dst_buf) {
> > dev_dbg(inst->dev->dev, "%s: No destination buffer found\n",
> > __func__); @@ -550,11 +610,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 {
> > @@ -1132,6 +1196,15 @@ 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_NV16 ||
> > + inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV61 ||
> > + inst->src_fmt.pixelformat == V4L2_PIX_FMT_YUV422P ||
> > + 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;
