Re: [PATCH v5 3/4] media: chips-media: wave5: Use helpers to calculate bytesperline and sizeimage.

From: Hans Verkuil
Date: Sat Jun 15 2024 - 05:58:26 EST

Next message: syzbot: "[syzbot] [bcachefs?] INFO: task hung in __bch2_fs_stop"
Previous message: Furong Xu: "[PATCH net-next v4] net: stmmac: Enable TSO on VLANs"
In reply to: jackson . lee: "RE: [PATCH v5 3/4] media: chips-media: wave5: Use helpers to calculate bytesperline and sizeimage."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 14/06/2024 06:58, jackson.lee wrote:
> Hi Hans
>
>
>> -----Original Message-----
>> From: Hans Verkuil <hverkuil@xxxxxxxxx>
>> Sent: Thursday, June 13, 2024 7:29 PM
>> To: jackson.lee <jackson.lee@xxxxxxxxxxxxxxx>; mchehab@xxxxxxxxxx;
>> nicolas@xxxxxxxxxxxx; sebastian.fricke@xxxxxxxxxxxxx
>> Cc: linux-media@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Nas Chung
>> <nas.chung@xxxxxxxxxxxxxxx>; lafley.kim <lafley.kim@xxxxxxxxxxxxxxx>; b-
>> brnich@xxxxxx; Nicolas Dufresne <nicolas.dufresne@xxxxxxxxxxxxx>
>> Subject: Re: [PATCH v5 3/4] media: chips-media: wave5: Use helpers to
>> calculate bytesperline and sizeimage.
>>
>> On 11/06/2024 09:15, Jackson Lee wrote:
>>> From: "jackson.lee" <jackson.lee@xxxxxxxxxxxxxxx>
>>>
>>> Use v4l2-common helper functions to calculate bytesperline and
>>> sizeimage, instead of calculating in a wave5 driver directly.
>>>
>>> In case of raw(YUV) v4l2_pix_format, the wave5 driver updates
>>> v4l2_pix_format_mplane struct through v4l2_fill_pixfmt_mp() function.
>>>
>>> Encoder and Decoder need same bytesperline and sizeimage values for
>>> same v4l2_pix_format.
>>> So, a wave5_update_pix_fmt is refactored to support both together.
>>>
>>> Signed-off-by: Jackson.lee <jackson.lee@xxxxxxxxxxxxxxx>
>>> Signed-off-by: Nas Chung <nas.chung@xxxxxxxxxxxxxxx>
>>> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@xxxxxxxxxxxxx>
>>> ---
>>> .../platform/chips-media/wave5/wave5-helper.c | 24 ++
>>> .../platform/chips-media/wave5/wave5-helper.h | 5 +
>>> .../chips-media/wave5/wave5-vpu-dec.c | 296 ++++++------------
>>> .../chips-media/wave5/wave5-vpu-enc.c | 197 +++++-------
>>> .../platform/chips-media/wave5/wave5-vpu.h | 5 +-
>>> .../chips-media/wave5/wave5-vpuconfig.h | 27 +-
>>> 6 files changed, 235 insertions(+), 319 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/chips-media/wave5/wave5-helper.c
>>> b/drivers/media/platform/chips-media/wave5/wave5-helper.c
>>> index 7e0f34bfa5be..b20ab69cd341 100644
>>> --- a/drivers/media/platform/chips-media/wave5/wave5-helper.c
>>> +++ b/drivers/media/platform/chips-media/wave5/wave5-helper.c
>>> @@ -7,6 +7,8 @@
>>>
>>> #include "wave5-helper.h"
>>>
>>> +#define DEFAULT_BS_SIZE(width, height) ((width) * (height) / 8 * 3)
>>> +
>>> const char *state_to_str(enum vpu_instance_state state) {
>>> switch (state) {
>>> @@ -224,3 +226,25 @@ void wave5_return_bufs(struct vb2_queue *q, u32 state)
>>> v4l2_m2m_buf_done(vbuf, state);
>>> }
>>> }
>>> +
>>> +void wave5_update_pix_fmt(struct v4l2_pix_format_mplane *pix_mp,
>>> + int pix_fmt_type,
>>> + unsigned int width,
>>> + unsigned int height,
>>> + const struct v4l2_frmsize_stepwise *frmsize) {
>>> + v4l2_apply_frmsize_constraints(&width, &height, frmsize);
>>> +
>>> + if (pix_fmt_type == VPU_FMT_TYPE_CODEC) {
>>> + pix_mp->width = width;
>>> + pix_mp->height = height;
>>> + pix_mp->num_planes = 1;
>>> + pix_mp->plane_fmt[0].bytesperline = 0;
>>> + pix_mp->plane_fmt[0].sizeimage = max(DEFAULT_BS_SIZE(width,
>> height),
>>> + pix_mp->plane_fmt[0].sizeimage);
>>> + } else {
>>> + v4l2_fill_pixfmt_mp(pix_mp, pix_mp->pixelformat, width, height);
>>> + }
>>> + pix_mp->flags = 0;
>>> + pix_mp->field = V4L2_FIELD_NONE;
>>> +}
>>> diff --git a/drivers/media/platform/chips-media/wave5/wave5-helper.h
>>> b/drivers/media/platform/chips-media/wave5/wave5-helper.h
>>> index 6cee1c14d3ce..9937fce553fc 100644
>>> --- a/drivers/media/platform/chips-media/wave5/wave5-helper.h
>>> +++ b/drivers/media/platform/chips-media/wave5/wave5-helper.h
>>> @@ -28,4 +28,9 @@ const struct vpu_format
>> *wave5_find_vpu_fmt_by_idx(unsigned int idx,
>>> const struct vpu_format
>> fmt_list[MAX_FMTS]); enum wave_std
>>> wave5_to_vpu_std(unsigned int v4l2_pix_fmt, enum vpu_instance_type
>>> type); void wave5_return_bufs(struct vb2_queue *q, u32 state);
>>> +void wave5_update_pix_fmt(struct v4l2_pix_format_mplane *pix_mp,
>>> + int pix_fmt_type,
>>> + unsigned int width,
>>> + unsigned int height,
>>> + const struct v4l2_frmsize_stepwise *frmsize);
>>> #endif
>>> diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
>>> b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
>>> index 861a0664047c..f246c290ad6a 100644
>>> --- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
>>> +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
>>> @@ -11,111 +11,92 @@
>>> #define VPU_DEC_DEV_NAME "C&M Wave5 VPU decoder"
>>> #define VPU_DEC_DRV_NAME "wave5-dec"
>>>
>>> -#define DEFAULT_SRC_SIZE(width, height) ({ \
>>> - (width) * (height) / 8 * 3; \
>>> -})
>>> +static const struct v4l2_frmsize_stepwise dec_hevc_frmsize = {
>>> + .min_width = W5_MIN_DEC_PIC_8_WIDTH,
>>> + .max_width = W5_MAX_DEC_PIC_WIDTH,
>>> + .step_width = W5_DEC_CODEC_STEP_WIDTH,
>>> + .min_height = W5_MIN_DEC_PIC_8_HEIGHT,
>>> + .max_height = W5_MAX_DEC_PIC_HEIGHT,
>>> + .step_height = W5_DEC_CODEC_STEP_HEIGHT, };
>>> +
>>> +static const struct v4l2_frmsize_stepwise dec_h264_frmsize = {
>>> + .min_width = W5_MIN_DEC_PIC_32_WIDTH,
>>> + .max_width = W5_MAX_DEC_PIC_WIDTH,
>>> + .step_width = W5_DEC_CODEC_STEP_WIDTH,
>>> + .min_height = W5_MIN_DEC_PIC_32_HEIGHT,
>>> + .max_height = W5_MAX_DEC_PIC_HEIGHT,
>>> + .step_height = W5_DEC_CODEC_STEP_HEIGHT, };
>>> +
>>> +static const struct v4l2_frmsize_stepwise dec_raw_frmsize = {
>>> + .min_width = W5_MIN_DEC_PIC_8_WIDTH,
>>> + .max_width = W5_MAX_DEC_PIC_WIDTH,
>>> + .step_width = W5_DEC_RAW_STEP_WIDTH,
>>> + .min_height = W5_MIN_DEC_PIC_8_HEIGHT,
>>> + .max_height = W5_MAX_DEC_PIC_HEIGHT,
>>> + .step_height = W5_DEC_RAW_STEP_HEIGHT, };
>>>
>>> static const struct vpu_format dec_fmt_list[FMT_TYPES][MAX_FMTS] = {
>>> [VPU_FMT_TYPE_CODEC] = {
>>> {
>>> .v4l2_pix_fmt = V4L2_PIX_FMT_HEVC,
>>> - .max_width = 8192,
>>> - .min_width = 8,
>>> - .max_height = 4320,
>>> - .min_height = 8,
>>> + .v4l2_frmsize = &dec_hevc_frmsize,
>>> },
>>> {
>>> .v4l2_pix_fmt = V4L2_PIX_FMT_H264,
>>> - .max_width = 8192,
>>> - .min_width = 32,
>>> - .max_height = 4320,
>>> - .min_height = 32,
>>> + .v4l2_frmsize = &dec_h264_frmsize,
>>> },
>>> },
>>> [VPU_FMT_TYPE_RAW] = {
>>> {
>>> .v4l2_pix_fmt = V4L2_PIX_FMT_YUV420,
>>> - .max_width = 8192,
>>> - .min_width = 8,
>>> - .max_height = 4320,
>>> - .min_height = 8,
>>> + .v4l2_frmsize = &dec_raw_frmsize,
>>> },
>>> {
>>> .v4l2_pix_fmt = V4L2_PIX_FMT_NV12,
>>> - .max_width = 8192,
>>> - .min_width = 8,
>>> - .max_height = 4320,
>>> - .min_height = 8,
>>> + .v4l2_frmsize = &dec_raw_frmsize,
>>> },
>>> {
>>> .v4l2_pix_fmt = V4L2_PIX_FMT_NV21,
>>> - .max_width = 8192,
>>> - .min_width = 8,
>>> - .max_height = 4320,
>>> - .min_height = 8,
>>> + .v4l2_frmsize = &dec_raw_frmsize,
>>> },
>>> {
>>> .v4l2_pix_fmt = V4L2_PIX_FMT_YUV422P,
>>> - .max_width = 8192,
>>> - .min_width = 8,
>>> - .max_height = 4320,
>>> - .min_height = 8,
>>> + .v4l2_frmsize = &dec_raw_frmsize,
>>> },
>>> {
>>> .v4l2_pix_fmt = V4L2_PIX_FMT_NV16,
>>> - .max_width = 8192,
>>> - .min_width = 8,
>>> - .max_height = 4320,
>>> - .min_height = 8,
>>> + .v4l2_frmsize = &dec_raw_frmsize,
>>> },
>>> {
>>> .v4l2_pix_fmt = V4L2_PIX_FMT_NV61,
>>> - .max_width = 8192,
>>> - .min_width = 8,
>>> - .max_height = 4320,
>>> - .min_height = 8,
>>> + .v4l2_frmsize = &dec_raw_frmsize,
>>> },
>>> {
>>> .v4l2_pix_fmt = V4L2_PIX_FMT_YUV420M,
>>> - .max_width = 8192,
>>> - .min_width = 8,
>>> - .max_height = 4320,
>>> - .min_height = 8,
>>> + .v4l2_frmsize = &dec_raw_frmsize,
>>> },
>>> {
>>> .v4l2_pix_fmt = V4L2_PIX_FMT_NV12M,
>>> - .max_width = 8192,
>>> - .min_width = 8,
>>> - .max_height = 4320,
>>> - .min_height = 8,
>>> + .v4l2_frmsize = &dec_raw_frmsize,
>>> },
>>> {
>>> .v4l2_pix_fmt = V4L2_PIX_FMT_NV21M,
>>> - .max_width = 8192,
>>> - .min_width = 8,
>>> - .max_height = 4320,
>>> - .min_height = 8,
>>> + .v4l2_frmsize = &dec_raw_frmsize,
>>> },
>>> {
>>> .v4l2_pix_fmt = V4L2_PIX_FMT_YUV422M,
>>> - .max_width = 8192,
>>> - .min_width = 8,
>>> - .max_height = 4320,
>>> - .min_height = 8,
>>> + .v4l2_frmsize = &dec_raw_frmsize,
>>> },
>>> {
>>> .v4l2_pix_fmt = V4L2_PIX_FMT_NV16M,
>>> - .max_width = 8192,
>>> - .min_width = 8,
>>> - .max_height = 4320,
>>> - .min_height = 8,
>>> + .v4l2_frmsize = &dec_raw_frmsize,
>>> },
>>> {
>>> .v4l2_pix_fmt = V4L2_PIX_FMT_NV61M,
>>> - .max_width = 8192,
>>> - .min_width = 8,
>>> - .max_height = 4320,
>>> - .min_height = 8,
>>> + .v4l2_frmsize = &dec_raw_frmsize,
>>> },
>>> }
>>> };
>>> @@ -234,74 +215,6 @@ static void wave5_handle_src_buffer(struct
>> vpu_instance *inst, dma_addr_t rd_ptr
>>> inst->remaining_consumed_bytes = consumed_bytes; }
>>>
>>> -static void wave5_update_pix_fmt(struct v4l2_pix_format_mplane *pix_mp,
>> unsigned int width,
>>> - unsigned int height)
>>> -{
>>> - switch (pix_mp->pixelformat) {
>>> - case V4L2_PIX_FMT_YUV420:
>>> - case V4L2_PIX_FMT_NV12:
>>> - case V4L2_PIX_FMT_NV21:
>>> - pix_mp->width = round_up(width, 32);
>>> - pix_mp->height = round_up(height, 16);
>>> - pix_mp->plane_fmt[0].bytesperline = round_up(width, 32);
>>> - pix_mp->plane_fmt[0].sizeimage = width * height * 3 / 2;
>>> - break;
>>> - case V4L2_PIX_FMT_YUV422P:
>>> - case V4L2_PIX_FMT_NV16:
>>> - case V4L2_PIX_FMT_NV61:
>>> - pix_mp->width = round_up(width, 32);
>>> - pix_mp->height = round_up(height, 16);
>>> - pix_mp->plane_fmt[0].bytesperline = round_up(width, 32);
>>> - pix_mp->plane_fmt[0].sizeimage = width * height * 2;
>>> - break;
>>> - case V4L2_PIX_FMT_YUV420M:
>>> - pix_mp->width = round_up(width, 32);
>>> - pix_mp->height = round_up(height, 16);
>>> - pix_mp->plane_fmt[0].bytesperline = round_up(width, 32);
>>> - pix_mp->plane_fmt[0].sizeimage = width * height;
>>> - pix_mp->plane_fmt[1].bytesperline = round_up(width, 32) / 2;
>>> - pix_mp->plane_fmt[1].sizeimage = width * height / 4;
>>> - pix_mp->plane_fmt[2].bytesperline = round_up(width, 32) / 2;
>>> - pix_mp->plane_fmt[2].sizeimage = width * height / 4;
>>> - break;
>>> - case V4L2_PIX_FMT_NV12M:
>>> - case V4L2_PIX_FMT_NV21M:
>>> - pix_mp->width = round_up(width, 32);
>>> - pix_mp->height = round_up(height, 16);
>>> - pix_mp->plane_fmt[0].bytesperline = round_up(width, 32);
>>> - pix_mp->plane_fmt[0].sizeimage = width * height;
>>> - pix_mp->plane_fmt[1].bytesperline = round_up(width, 32);
>>> - pix_mp->plane_fmt[1].sizeimage = width * height / 2;
>>> - break;
>>> - case V4L2_PIX_FMT_YUV422M:
>>> - pix_mp->width = round_up(width, 32);
>>> - pix_mp->height = round_up(height, 16);
>>> - pix_mp->plane_fmt[0].bytesperline = round_up(width, 32);
>>> - pix_mp->plane_fmt[0].sizeimage = width * height;
>>> - pix_mp->plane_fmt[1].bytesperline = round_up(width, 32) / 2;
>>> - pix_mp->plane_fmt[1].sizeimage = width * height / 2;
>>> - pix_mp->plane_fmt[2].bytesperline = round_up(width, 32) / 2;
>>> - pix_mp->plane_fmt[2].sizeimage = width * height / 2;
>>> - break;
>>> - case V4L2_PIX_FMT_NV16M:
>>> - case V4L2_PIX_FMT_NV61M:
>>> - pix_mp->width = round_up(width, 32);
>>> - pix_mp->height = round_up(height, 16);
>>> - pix_mp->plane_fmt[0].bytesperline = round_up(width, 32);
>>> - pix_mp->plane_fmt[0].sizeimage = width * height;
>>> - pix_mp->plane_fmt[1].bytesperline = round_up(width, 32);
>>> - pix_mp->plane_fmt[1].sizeimage = width * height;
>>> - break;
>>> - default:
>>> - pix_mp->width = width;
>>> - pix_mp->height = height;
>>> - pix_mp->plane_fmt[0].bytesperline = 0;
>>> - pix_mp->plane_fmt[0].sizeimage = max(DEFAULT_SRC_SIZE(width,
>> height),
>>> - pix_mp->plane_fmt[0].sizeimage);
>>> - break;
>>> - }
>>> -}
>>> -
>>> static int start_decode(struct vpu_instance *inst, u32 *fail_res) {
>>> struct v4l2_m2m_ctx *m2m_ctx = inst->v4l2_fh.m2m_ctx; @@ -389,6
>>> +302,8 @@ static int handle_dynamic_resolution_change(struct vpu_instance
>> *inst)
>>> }
>>>
>>> if (p_dec_info->initial_info_obtained) {
>>> + const struct vpu_format *vpu_fmt;
>>> +
>>> inst->conf_win.left = initial_info->pic_crop_rect.left;
>>> inst->conf_win.top = initial_info->pic_crop_rect.top;
>>> inst->conf_win.width = initial_info->pic_width - @@ -396,10
>> +311,27
>>> @@ static int handle_dynamic_resolution_change(struct vpu_instance *inst)
>>> inst->conf_win.height = initial_info->pic_height -
>>> initial_info->pic_crop_rect.top -
>>> initial_info->pic_crop_rect.bottom;
>>>
>>> - wave5_update_pix_fmt(&inst->src_fmt, initial_info->pic_width,
>>> - initial_info->pic_height);
>>> - wave5_update_pix_fmt(&inst->dst_fmt, initial_info->pic_width,
>>> - initial_info->pic_height);
>>> + vpu_fmt = wave5_find_vpu_fmt(inst->src_fmt.pixelformat,
>>> + dec_fmt_list[VPU_FMT_TYPE_CODEC]);
>>> + if (!vpu_fmt)
>>> + return -EINVAL;
>>> +
>>> + wave5_update_pix_fmt(&inst->src_fmt,
>>> + VPU_FMT_TYPE_CODEC,
>>> + initial_info->pic_width,
>>> + initial_info->pic_height,
>>> + vpu_fmt->v4l2_frmsize);
>>> +
>>> + vpu_fmt = wave5_find_vpu_fmt(inst->dst_fmt.pixelformat,
>>> + dec_fmt_list[VPU_FMT_TYPE_RAW]);
>>> + if (!vpu_fmt)
>>> + return -EINVAL;
>>> +
>>> + wave5_update_pix_fmt(&inst->dst_fmt,
>>> + VPU_FMT_TYPE_RAW,
>>> + initial_info->pic_width,
>>> + initial_info->pic_height,
>>> + vpu_fmt->v4l2_frmsize);
>>> }
>>>
>>> v4l2_event_queue_fh(fh, &vpu_event_src_ch); @@ -545,15 +477,11 @@
>>> static int wave5_vpu_dec_enum_framesizes(struct file *f, void *fh, struct
>> v4l2_f
>>> vpu_fmt = wave5_find_vpu_fmt(fsize->pixel_format,
>> dec_fmt_list[VPU_FMT_TYPE_RAW]);
>>> if (!vpu_fmt)
>>> return -EINVAL;
>>> + return -ENOTTY;
>>
>> Huh? Where does this come from? It wasn't part of v4, and it doesn't make
>> sense either.
>>
>> It looks like a spurious test line that you forgot to remove.
>>
>> Regards,
>>
>> Hans
>
> Sorry for the confusion.
> It should be removed.
> Due to this code, we have been sharing incorrect v4l2-compliance results in the cover letter.
>
> To pass the v4l2-compliance test correctly, Can we change the following code
> to v6?

I think so, yes.

Regards,

Hans

>
>
>>
>>> }
>>>
>>> fsize->type = V4L2_FRMSIZE_TYPE_CONTINUOUS;
>>> - fsize->stepwise.min_width = vpu_fmt->min_width;
>>> - fsize->stepwise.max_width = vpu_fmt->max_width;
>>> - fsize->stepwise.step_width = 1;
>>> - fsize->stepwise.min_height = vpu_fmt->min_height;
>>> - fsize->stepwise.max_height = vpu_fmt->max_height;
>>> - fsize->stepwise.step_height = 1;
>>> + fsize->stepwise = *vpu_fmt->v4l2_frmsize;
>
> fsize->stepwise.min_width = vpu_fmt->v4l2_frmsize->min_width;
> fsize->stepwise.max_width = vpu_fmt->v4l2_frmsize->max_width;
> fsize->stepwise.step_width = W5_DEC_CODEC_STEP_WIDTH;
> fsize->stepwise.min_height = vpu_fmt->v4l2_frmsize->min_height;
> fsize->stepwise.max_height = vpu_fmt->v4l2_frmsize->max_height;
> fsize->stepwise.step_height = W5_DEC_CODEC_STEP_HEIGHT;
>
>
> thanks
> Jackson
>
>>>
>>> return 0;
>>> }
>>
>> Regards,
>>
>> Hans
>

Next message: syzbot: "[syzbot] [bcachefs?] INFO: task hung in __bch2_fs_stop"
Previous message: Furong Xu: "[PATCH net-next v4] net: stmmac: Enable TSO on VLANs"
In reply to: jackson . lee: "RE: [PATCH v5 3/4] media: chips-media: wave5: Use helpers to calculate bytesperline and sizeimage."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]