| Message ID | 20240430013900.187-5-nas.chung@chipsnmedia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add features to an existing driver | expand |
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
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
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
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;