Re: [PATCH v2 2/3] media: verisilicon: add WebP decoding support

[Hugues FRUCHET <hugues.fruchet@xxxxxxxxxxx>]

Hi Nicolas,

On 11/21/24 15:20, Nicolas Dufresne wrote:
> Hi Hugues,
>
> Le jeudi 21 novembre 2024 à 11:07 +0100, Hugues FRUCHET a écrit :
> > Hi Nicolas,
> >
> > On 11/20/24 15:25, Nicolas Dufresne wrote:
> > > Le mercredi 20 novembre 2024 à 12:01 +0100, Hugues Fruchet a écrit :
> > > > Add WebP picture decoding support to VP8 stateless decoder.
> > > >
> > > > Signed-off-by: Hugues Fruchet <hugues.fruchet@xxxxxxxxxxx>
> > > > ---
> > > >    .../media/platform/verisilicon/hantro_g1_regs.h |  1 +
> > > >    .../platform/verisilicon/hantro_g1_vp8_dec.c    | 14 ++++++++++++++
> > > >    .../media/platform/verisilicon/hantro_v4l2.c    |  2 ++
> > > >    .../platform/verisilicon/stm32mp25_vpu_hw.c     | 17 +++++++++++++++--
> > > >    4 files changed, 32 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/media/platform/verisilicon/hantro_g1_regs.h b/drivers/media/platform/verisilicon/hantro_g1_regs.h
> > > > index c623b3b0be18..e7d4db788e57 100644
> > > > --- a/drivers/media/platform/verisilicon/hantro_g1_regs.h
> > > > +++ b/drivers/media/platform/verisilicon/hantro_g1_regs.h
> > > > @@ -232,6 +232,7 @@
> > > >    #define     G1_REG_DEC_CTRL7_DCT7_START_BIT(x)		(((x) & 0x3f) << 0)
> > > >    #define G1_REG_ADDR_STR					0x030
> > > >    #define G1_REG_ADDR_DST					0x034
> > > > +#define G1_REG_ADDR_DST_CHROMA				0x038
> > > >    #define G1_REG_ADDR_REF(i)				(0x038 + ((i) * 0x4))
> > > >    #define     G1_REG_ADDR_REF_FIELD_E			BIT(1)
> > > >    #define     G1_REG_ADDR_REF_TOPC_E			BIT(0)
> > > > diff --git a/drivers/media/platform/verisilicon/hantro_g1_vp8_dec.c b/drivers/media/platform/verisilicon/hantro_g1_vp8_dec.c
> > > > index 851eb67f19f5..c83ee6f5edc8 100644
> > > > --- a/drivers/media/platform/verisilicon/hantro_g1_vp8_dec.c
> > > > +++ b/drivers/media/platform/verisilicon/hantro_g1_vp8_dec.c
> > > > @@ -307,6 +307,12 @@ static void cfg_parts(struct hantro_ctx *ctx,
> > > >    			   G1_REG_DEC_CTRL3_STREAM_LEN(dct_part_total_len),
> > > >    			   G1_REG_DEC_CTRL3);
> > > >    
> > > > +	if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_WEBP_FRAME)
> > > > +		vdpu_write_relaxed(vpu,
> > > > +				   G1_REG_DEC_CTRL3_STREAM_LEN_EXT
> > > > +					(dct_part_total_len >> 24),
> > > > +				   G1_REG_DEC_CTRL3);
> > > > +
> > > >    	/* DCT partitions base address */
> > > >    	for (i = 0; i < hdr->num_dct_parts; i++) {
> > > >    		u32 byte_offset = dct_part_offset + dct_size_part_size + count;
> > > > @@ -427,6 +433,12 @@ static void cfg_buffers(struct hantro_ctx *ctx,
> > > >    
> > > >    	dst_dma = hantro_get_dec_buf_addr(ctx, &vb2_dst->vb2_buf);
> > > >    	vdpu_write_relaxed(vpu, dst_dma, G1_REG_ADDR_DST);
> > > > +
> > > > +	if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_WEBP_FRAME)
> > > > +		vdpu_write_relaxed(vpu, dst_dma +
> > > > +				   ctx->dst_fmt.plane_fmt[0].bytesperline *
> > > > +				   ctx->dst_fmt.height,
> > > > +				   G1_REG_ADDR_DST_CHROMA);
> > > >    }
> > > >    
> > > >    int hantro_g1_vp8_dec_run(struct hantro_ctx *ctx)
> > > > @@ -471,6 +483,8 @@ int hantro_g1_vp8_dec_run(struct hantro_ctx *ctx)
> > > >    		reg |= G1_REG_DEC_CTRL0_SKIP_MODE;
> > > >    	if (hdr->lf.level == 0)
> > > >    		reg |= G1_REG_DEC_CTRL0_FILTERING_DIS;
> > > > +	if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_WEBP_FRAME)
> > > > +		reg |= G1_REG_DEC_CTRL0_WEBP_E;
> > > >    	vdpu_write_relaxed(vpu, reg, G1_REG_DEC_CTRL0);
> > > >    
> > > >    	/* Frame dimensions */
> > > > diff --git a/drivers/media/platform/verisilicon/hantro_v4l2.c b/drivers/media/platform/verisilicon/hantro_v4l2.c
> > > > index 2513adfbd825..7075b2ba1ec2 100644
> > > > --- a/drivers/media/platform/verisilicon/hantro_v4l2.c
> > > > +++ b/drivers/media/platform/verisilicon/hantro_v4l2.c
> > > > @@ -470,6 +470,7 @@ hantro_update_requires_request(struct hantro_ctx *ctx, u32 fourcc)
> > > >    		break;
> > > >    	case V4L2_PIX_FMT_MPEG2_SLICE:
> > > >    	case V4L2_PIX_FMT_VP8_FRAME:
> > > > +	case V4L2_PIX_FMT_WEBP_FRAME:
> > > >    	case V4L2_PIX_FMT_H264_SLICE:
> > > >    	case V4L2_PIX_FMT_HEVC_SLICE:
> > > >    	case V4L2_PIX_FMT_VP9_FRAME:
> > > > @@ -492,6 +493,7 @@ hantro_update_requires_hold_capture_buf(struct hantro_ctx *ctx, u32 fourcc)
> > > >    	case V4L2_PIX_FMT_JPEG:
> > > >    	case V4L2_PIX_FMT_MPEG2_SLICE:
> > > >    	case V4L2_PIX_FMT_VP8_FRAME:
> > > > +	case V4L2_PIX_FMT_WEBP_FRAME:
> > > >    	case V4L2_PIX_FMT_HEVC_SLICE:
> > > >    	case V4L2_PIX_FMT_VP9_FRAME:
> > > >    		vq->subsystem_flags &= ~(VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF);
> > > > diff --git a/drivers/media/platform/verisilicon/stm32mp25_vpu_hw.c b/drivers/media/platform/verisilicon/stm32mp25_vpu_hw.c
> > > > index 833821120b20..48d6912c3bab 100644
> > > > --- a/drivers/media/platform/verisilicon/stm32mp25_vpu_hw.c
> > > > +++ b/drivers/media/platform/verisilicon/stm32mp25_vpu_hw.c
> > > > @@ -22,10 +22,10 @@ static const struct hantro_fmt stm32mp25_vdec_fmts[] = {
> > > >    		.codec_mode = HANTRO_MODE_NONE,
> > > >    		.frmsize = {
> > > >    			.min_width = FMT_MIN_WIDTH,
> > > > -			.max_width = FMT_FHD_WIDTH,
> > > > +			.max_width = FMT_4K_WIDTH,
> > > >    			.step_width = MB_DIM,
> > > >    			.min_height = FMT_MIN_HEIGHT,
> > > > -			.max_height = FMT_FHD_HEIGHT,
> > > > +			.max_height = FMT_4K_HEIGHT,
> > >
> > > I'm a little surprised of this change, since this is modifying VP8_FRAME, while
> > > we should instead introduce WEBP_FRAME.
> >
> > This is the resolution of the YUV output of decoder, not the WebP input,
> > and because of lack of post-processor, the output is not scaled, so can
> > go up to 4K with WebP.
> > Before WebP introduction, the maximum output resolution was FHD for all
> > codecs. Now WebP allows up to 4K but FHD constraint remains for
> > H264/VP8. I don't see real problems because VP8/H264 compressed inputs
> > are well limited to FHD and only WebP allows 4K...
>
> Good point. Would you mind adding a justification for this change within the
> commit message in v3 ?

v3 already sent but I'll note that for the future v4.
Could you test & ack the 4K support on your platform having 
post-processor support ?

> > > >    			.step_height = MB_DIM,
> > > >    		},
> > > >    	},
> > > > @@ -68,6 +68,19 @@ static const struct hantro_fmt stm32mp25_venc_fmts[] = {
> > > >    		.codec_mode = HANTRO_MODE_NONE,
> > > >    		.enc_fmt = ROCKCHIP_VPU_ENC_FMT_YUV420SP,
> > > >    	},
> > > > +	{
> > > > +		.fourcc = V4L2_PIX_FMT_WEBP_FRAME,
> > > > +		.codec_mode = HANTRO_MODE_VP8_DEC,
> > > > +		.max_depth = 2,
> > > > +		.frmsize = {
> > > > +			.min_width = FMT_MIN_WIDTH,
> > > > +			.max_width = FMT_4K_WIDTH,
> > > > +			.step_width = MB_DIM,
> > > > +			.min_height = FMT_MIN_HEIGHT,
> > > > +			.max_height = FMT_4K_HEIGHT,
> > > > +			.step_height = MB_DIM,
> > > > +		},
> > > > +	},
> > >
> > > This is venc_fmt (encoder), this shouldn't be there.
> >
> > All apologizes for this rebase issue, it is of course part of
> > stm32mp25_vdec_fmts.
>
> Ack, let's get this right in v3 :-D
>
> > > >    	{
> > > >    		.fourcc = V4L2_PIX_FMT_YUYV,
> > > >    		.codec_mode = HANTRO_MODE_NONE,
> >
> > BR,
> > Hugues.