[PATCH] media: rkvdec: Switch to using structs instead of writel

[Nicolas Dufresne]

Hi Jonas,

Le mardi 27 mai 2025 à 19:51 +0200, Jonas Karlman a écrit :
> Hi Detlev,
> 
> On 2025-05-27 17:00, Detlev Casanova wrote:
> > In an effort to merge the rkvdec2 driver [1] with this one, switch from
> > writel() calls to using structs to represent the register mappings.
> 
> Please wait with this until HEVC support has landed, now that H264
> 4:2:2/Hi10 finally have been merged I was hoping to be able to send a v2


Detlev communicated to me that this was meant to have the RFC tag. The intent
being to check with others if the usage of bitfield in structure for registers is
acceptable. I personally like it, and don't see any issue with it considering
the performance is not affected at all. The idea is not new, you can find
similar proposal from 2019.

https://lore.kernel.org/all/20190131031333.11905-2-ayaka@soulik.info/

> of the old HEVC series [3]. I was waiting on v6.16-rc1 before sending
> the series but can send it sooner if needed, [4] has current state of v2.

Please don't wait for that, just send as soon as your are happy with it.
Just based it on current media-committers/next branch, I will deal with the
rest.

> 
> H264 4:2:2/Hi10 and HEVC have been in the works for a few years now,
> would be nice to have it fully land before refactoring starts ;-)

My roadmap was that HEVC was not truly an unstaging dependency. I was a lot
more worried about concurrent decode, but I have a patch in queue now. So
unstaging is what I will prioritize once a patch is made. Meanwhile, better send
you patches soon.

> 
> [3] https://lore.kernel.org/linux-media/20231105233630.3927502-1-jonas@kwiboo.se
> [4] https://github.com/Kwiboo/linux-rockchip/commits/linuxtv-rkvdec-hevc-v2b/
> 
> > This is done in order to have all supported decoders use the same format
> > in the future and ease reading of the code.
> 
> Do you have any work-in-progress patches for this?
> 
> > Using structs also improves stability as the hardware is tested and
> > validated downstream using a similar method.
> > It was noticed, on decoders, that:
> >  - Some registers require to be writen in increasing order [2]
> >  - Some registers, even if unrelated, need to be written to their reset
> >    values (it was the case here for axi_ddr_[rw]data).
> > 
> > Using structs can also help improving performance later when, e.g.
> > multicore support is added on RK3588.
> 
> Are your referring to the linked-list feature (also present in e.g.
> RK3328) or just for multi-core purpose?

RK3588 multi-core design, but this is also needed if you want to use HW queues.
Multi-core is more pressing at the moment, the benefit of using HW queues will
have to be proven. It does not make a big difference for a single media.

Nicolas

> 
> Regards,
> Jonas
> 
> > Performance seems to be slightly improved, but at least, not made worse.
> > Running fluster's JVT-AVC_V1 test suite with GStreamer on the Radxa ROCK
> > PI 4 SE gives the following times:
> > 
> > Before this patch:
> > 
> > - --jobs 1: Ran 129/135 tests successfully               in 77.167 secs
> > - --jobs 6: Ran 129/135 tests successfully               in 23.046 secs
> > 
> > With this patch:
> > - --jobs 1: Ran 129/135 tests successfully               in 70.698 secs
> > - --jobs 6: Ran 129/135 tests successfully               in 22.917 secs
> > 
> > This also shows that the fluster score hasn't changed.
> > 
> > [1]: https://lore.kernel.org/all/20250325213303.826925-1-detlev.casanova@collabora.com/
> > [2]: https://lore.kernel.org/all/20200127143009.15677-5-andrzej.p@collabora.com/
> > 
> > Signed-off-by: Detlev Casanova <detlev.casanova at collabora.com>
> > ---
> >  drivers/staging/media/rkvdec/rkvdec-h264.c | 168 +++---
> >  drivers/staging/media/rkvdec/rkvdec-regs.h | 567 ++++++++++++++-------
> >  drivers/staging/media/rkvdec/rkvdec-vp9.c  | 239 ++++-----
> >  drivers/staging/media/rkvdec/rkvdec.c      |   1 -
> >  4 files changed, 559 insertions(+), 416 deletions(-)
> 
> [snip]