Re: [PATCH v2 5/7] media: rkvdec: Disable QoS for HEVC and VP9 on RK3328

[Nicolas Dufresne <nicolas.dufresne@collabora.com>]

[-- Attachment #1: Type: text/plain, Size: 5587 bytes --]

Le mardi 12 août 2025 à 01:08 +0200, Jonas Karlman a écrit :
> Hi Nicolas,
> 
> Missed some comments in my last mail.
> 
> On 8/11/2025 11:25 PM, Nicolas Dufresne wrote:
> > Le dimanche 10 août 2025 à 21:24 +0000, Jonas Karlman a écrit :
> > > From: Alex Bee <knaerzche@gmail.com>
> > > 
> > > The RK3328 VDEC has a HW quirk that require QoS to be disabled when HEVC
> > > or VP9 is decoded, otherwise the decoded picture may become corrupted.
> > > 
> > > Add a RK3328 variant with a quirk flag to disable QoS when before
> > > decoding is started.
> > > 
> > > Signed-off-by: Alex Bee <knaerzche@gmail.com>
> > > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > > ---
> > > Changes in v2:
> > > - No change
> > > ---
> > >  drivers/media/platform/rockchip/rkvdec/rkvdec-hevc.c |  9 +++++++++
> > >  drivers/media/platform/rockchip/rkvdec/rkvdec-regs.h |  2 ++
> > >  drivers/media/platform/rockchip/rkvdec/rkvdec-vp9.c  | 10 ++++++++++
> > >  drivers/media/platform/rockchip/rkvdec/rkvdec.c      | 12 ++++++++++++
> > >  drivers/media/platform/rockchip/rkvdec/rkvdec.h      |  4 ++++
> > >  5 files changed, 37 insertions(+)
> > > 
> > > diff --git a/drivers/media/platform/rockchip/rkvdec/rkvdec-hevc.c
> > > b/drivers/media/platform/rockchip/rkvdec/rkvdec-hevc.c
> > > index 1994ea24f0be..f8bb8c4264f7 100644
> > > --- a/drivers/media/platform/rockchip/rkvdec/rkvdec-hevc.c
> > > +++ b/drivers/media/platform/rockchip/rkvdec/rkvdec-hevc.c
> > > @@ -789,6 +789,15 @@ static int rkvdec_hevc_run(struct rkvdec_ctx *ctx)
> > >  	writel(1, rkvdec->regs + RKVDEC_REG_PREF_LUMA_CACHE_COMMAND);
> > >  	writel(1, rkvdec->regs + RKVDEC_REG_PREF_CHR_CACHE_COMMAND);
> > >  
> > > +	if (rkvdec->quirks & RKVDEC_QUIRK_DISABLE_QOS) {
> > > +		u32 reg;
> > > +
> > > +		reg = readl(rkvdec->regs + RKVDEC_REG_QOS_CTRL);
> > > +		reg |= 0xFFFF;
> > > +		reg &= ~BIT(12);
> > 
> > I wonder if there is a better way to express that, if not, a comment for
> > future
> > readers would be nice. If read it will, we keep the upper 16bit, and
> > replaced
> > the lower bits with 0xEFFF (all bits set except 12) ? I'd rather not spend
> > time
> > thinking if I walk by this code again.
> 
> Vendor kernel use following comment to describe the purpose of this [1]:
> 
>   HW defeat workaround: VP9 and H.265 power save optimization cause
>   decoding corruption, disable optimization here.
> 
> From the TRM we can see following for rkvdec_swreg99_qos_ctrl:
> 
>   27:26 sw_axi_wr_hurry_level
>     00: hurry off 
>     01~11: hurry level 
>   25:24 sw_axi_rd_hurry_level
>     00: hurry off 
>     01~11: hurry level 
>   23:16 sw_bus2mc_buffer_qos_level
>     range is: 0~255
>     the value is means that left space <=
>     sw_bus2mc_buffer_qos_level, it will give hurry
>   15:0 swreg_block_gating_e
> 
> So yes this set swreg_block_gating_e to 0xEFFF. Possible this configure
> hw to not auto gate most internal clocks?
> 
> Could add a comment and possible use something like following:
> 
>   reg &= GENMASK(31, 16);
>   reg |= 0xEFFF;

Thanks for the information, I think this form is somewhat nicer indeed, and a
little comment, its fine to say that the QOS bits are undocumented.

Nicolas

> 
> [1]
> https://github.com/Kwiboo/linux-rockchip/blob/linux-6.1-stan-rkr6.1/drivers/video/rockchip/mpp/mpp_rkvdec.c#L857-L867
> 
> > 
> > > +		writel(reg, rkvdec->regs + RKVDEC_REG_QOS_CTRL);
> > > +	}
> > > +
> > >  	/* Start decoding! */
> > >  	reg = (run.pps->flags & V4L2_HEVC_PPS_FLAG_TILES_ENABLED) ?
> > >  		0 : RKVDEC_WR_DDR_ALIGN_EN;
> > > diff --git a/drivers/media/platform/rockchip/rkvdec/rkvdec-regs.h
> > > b/drivers/media/platform/rockchip/rkvdec/rkvdec-regs.h
> > > index 540c8bdf24e4..c627b6b6f53a 100644
> > > --- a/drivers/media/platform/rockchip/rkvdec/rkvdec-regs.h
> > > +++ b/drivers/media/platform/rockchip/rkvdec/rkvdec-regs.h
> > > @@ -219,6 +219,8 @@
> > >  #define RKVDEC_REG_H264_ERR_E				0x134
> > >  #define RKVDEC_H264_ERR_EN_HIGHBITS(x)			((x) &
> > > 0x3fffffff)
> > >  
> > > +#define RKVDEC_REG_QOS_CTRL				0x18C
> > > +
> > >  #define RKVDEC_REG_PREF_LUMA_CACHE_COMMAND		0x410
> > >  #define RKVDEC_REG_PREF_CHR_CACHE_COMMAND		0x450
> > >  
> > > diff --git a/drivers/media/platform/rockchip/rkvdec/rkvdec-vp9.c
> > > b/drivers/media/platform/rockchip/rkvdec/rkvdec-vp9.c
> > > index 0e7e16f20eeb..cadb9d592308 100644
> > > --- a/drivers/media/platform/rockchip/rkvdec/rkvdec-vp9.c
> > > +++ b/drivers/media/platform/rockchip/rkvdec/rkvdec-vp9.c
> > > @@ -824,6 +824,16 @@ static int rkvdec_vp9_run(struct rkvdec_ctx *ctx)
> > >  	writel(1, rkvdec->regs + RKVDEC_REG_PREF_CHR_CACHE_COMMAND);
> > >  
> > >  	writel(0xe, rkvdec->regs + RKVDEC_REG_STRMD_ERR_EN);
> > > +
> > > +	if (rkvdec->quirks & RKVDEC_QUIRK_DISABLE_QOS) {
> > > +		u32 reg;
> > > +
> > > +		reg = readl(rkvdec->regs + RKVDEC_REG_QOS_CTRL);
> > > +		reg |= 0xFFFF;
> > > +		reg &= ~BIT(12);
> > > +		writel(reg, rkvdec->regs + RKVDEC_REG_QOS_CTRL);
> > 
> > Can we deduplicate that ?
> 
> Guess so, any suggestion on how to best do that?
> 
> One possible way that comes to mind:
> 
>   if (rkvdec->quirks & RKVDEC_QUIRK_DISABLE_QOS)
> 	rkvdec_quirk_disable_qos(rkvdec);
> 
> > 
> > > +	}
> > > +
> > >  	/* Start decoding! */
> > >  	writel(RKVDEC_INTERRUPT_DEC_E | RKVDEC_CONFIG_DEC_CLK_GATE_E |
> > >  	       RKVDEC_TIMEOUT_E | RKVDEC_BUF_EMPTY_E,
> 
> [snip]

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]