Re: [PATCH v2 1/7] media: rkvdec: Add HEVC backend

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

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

Hi,

Le mercredi 03 septembre 2025 à 09:28 +0200, Jonas Karlman a écrit :
> Hi Nicolas,
> 
> On 8/29/2025 10:22 PM, Nicolas Dufresne wrote:
> > Le dimanche 10 août 2025 à 21:24 +0000, Jonas Karlman a écrit :
> > > The Rockchip VDEC supports the HEVC codec with the Main and Main10
> > > Profile up to Level 5.1 High tier: 4096x2304@60 fps.
> > > 
> > > Add the backend for HEVC format to the decoder.
> > > 
> > > Signed-off-by: Alex Bee <knaerzche@gmail.com>
> > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > > Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> > > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > 
> > Re-reading myself, most of my comments were off or really "nitty". So let's move
> > forward and spare you the v3. Detlev is happy to rebase and work on top of your
> > series, so let's help everyone getting better RK codec support.
> > 
> > Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > 
> > Just to be transparent, during testing, it was notice that some concurrent
> > decoding resulted into failures. I tested Detlev port to structure registers,
> > and it didn't change anything (so probably not a stalled state, or one that we
> > control). This could easily be a HW issue with older chip. Since you have used
> > this for years without major issue reported, I happy to move on.
> 
> Thanks, I found some minor changes compared to the LibreELEC version
> that I am running some new tests on, plan to send out a v3 as soon as
> testing completes later today.

Ok, I will be patient then.

> 
> In LibreELEC version we enable some error detection,
> 
> 	// sw_cabac_error_e - cabac error enable
> 	writel(0xfdfffffd, rkvdec->regs + RKVDEC_REG_STRMD_ERR_EN);
> 	// slice end error enable = BIT(28)
> 	// frame end error enable = BIT(29)
> 	writel(0x30000000, rkvdec->regs + RKVDEC_REG_H264_ERR_E);
> 
> and in this series it was fully disabled to closer match H264/VP9:
> 
> 	writel(0, rkvdec->regs + RKVDEC_REG_STRMD_ERR_EN);
> 	writel(0, rkvdec->regs + RKVDEC_REG_H264_ERR_E);

In error detection mode, the IP will emit an IRQ on the first error. It does not
self reset, this is just to let you read the macroblock index that wasn't
decoded properly. The problem is that we don't know how to resume it. That lead
to HW lockup and timeout on packet lost during RTP streaming tests. It is likely
best to keep this off. It is meant to be used to do concealment of lost
macroblock in software.

> 
> There is also an extra memset(0, ...) in rkvdec_hevc_start:
> 
> 	memset(priv_tbl, 0, sizeof(*priv_tbl));
> 
> This should not really be needed and was removed in this series.

Ack.

> 
> Still unclear if any of these will result in a changed behavior. Enable
> of cabac/slice end/frame end error could possible activate some more
> states when block issue a self-reset, but I am only guessing.
> 
> One thing to note for the flaky tests is that when they fail, they
> typically just end up with a different consistent checksum. I have not
> done any visual inspection of those frames, but will extract each frame
> and compare them both bitwise and visually.

Thanks, looking forward v3. Please note the fh changes, which I needed to
manually apply here.

Nicolas

> 
> Regards,
> Jonas
> 
> > 
> > regards,
> > Nicolas
> > 
> > > ---
> > > Changes in v2:
> > > - Use new_value in transpose_and_flatten_matrices()
> > > - Add NULL check for ctrl->new_elems in rkvdec_hevc_run_preamble()
> > > - Set RKVDEC_WR_DDR_ALIGN_EN for RK3328
> > > ---
> > >  .../media/platform/rockchip/rkvdec/Makefile   |    2 +-
> > >  .../rockchip/rkvdec/rkvdec-hevc-data.c        | 1848 +++++++++++++++++
> > >  .../platform/rockchip/rkvdec/rkvdec-hevc.c    |  817 ++++++++
> > >  .../platform/rockchip/rkvdec/rkvdec-regs.h    |    2 +
> > >  .../media/platform/rockchip/rkvdec/rkvdec.c   |   76 +
> > >  .../media/platform/rockchip/rkvdec/rkvdec.h   |    1 +
> > >  6 files changed, 2745 insertions(+), 1 deletion(-)
> > >  create mode 100644 drivers/media/platform/rockchip/rkvdec/rkvdec-hevc-data.c
> > >  create mode 100644 drivers/media/platform/rockchip/rkvdec/rkvdec-hevc.c
> 
> [snip]

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