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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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?


> 
> >  	}
> >
> >  	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





[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux