Le mercredi 22 novembre 2023 à 16:23 +0100, Hans Verkuil a écrit : > On 09/11/2023 18:44, Jonas Karlman wrote: > > Hi Hans, > > > > On 2023-11-07 14:49, Hans Verkuil wrote: > > > Hi Jonas, > > > > > > On 06/11/2023 00:36, Jonas Karlman wrote: > > > > This series add a HEVC backend to the Rockchip Video Decoder driver. > > > > > > > > A version of this HEVC backend has been in use by the LibreELEC distro > > > > for the past 3+ years [1]. It was initially created based on a copy of > > > > the H264 backend, unstable HEVC uAPI controls and a cabac table + scaling > > > > matrix functions shamelessly copied 1:1 from the Rockchip mpp library. > > > > > > > > It has since then been extended to use the stable HEVC uAPI controls and > > > > improved opon e.g. to include support for rk3288 and fix decoding issues > > > > by Alex Bee and Nicolas Dufresne. > > > > > > > > The version submitted in this series is based on the code currently used > > > > by the LibreELEC distro, excluding hard/soft reset, and with cabac table > > > > and scaling matrix functions picked from Sebastian Fricke prior series > > > > to add a HEVC backend [2]. > > > > > > > > Big thanks to Alex Bee, Nicolas Dufresne and Sebastian Fricke for making > > > > this series possible! > > > > > > I ran this series through smatch and found these two issues: > > > > > > drivers/staging/media/rkvdec/rkvdec-hevc.c: In function 'transpose_and_flatten_matrices': > > > drivers/staging/media/rkvdec/rkvdec-hevc.c:429:83: warning: variable 'new_value' set but not used [-Wunused-but-set-variable] > > > 429 | int i, j, row, x_offset, matrix_offset, rot_index, y_offset, matrix_size, new_value; > > > | ^~~~~~~~~ > > > drivers/staging/media/rkvdec/rkvdec-hevc.c:756 rkvdec_hevc_run_preamble() error: we previously assumed 'ctrl' could be null (see line 755) > > > > Thanks, will fix in v2. > > > > > > > > Also, this series drops the HEVC part from the TODO file, but > > > I wonder if the last remaining item is still valid: > > > > > > * Evaluate introducing a helper to consolidate duplicated > > > code in rkvdec_request_validate and cedrus_request_validate. > > > The helper needs to the driver private data associated with > > > the videobuf2 queue, from a media request. > > > > > > It doesn't look like there is much duplicate code at all. It is certainly not > > > something that prevents this driver from moving out of staging. > > > > I agree, if this is still valid it is not something that should prevent > > this driver from moving out of staging. > > > > There is however one remaining feature/issue that is not listed in TODO. > > > > In certain situations the hw block may need a reset after there has been > > a decoding error, especially after a hevc decoding error. Decoding can > > typically be re-started with successful result after a short pm > > autosuspend timeout. > > > > We do have a soft/hard reset implementation in LibreELEC-distro, > > excluded it from this series because reset is typically not needed, and > > I think the reset code need some rework to be upstream ready. > > > > Would missing hard reset support be a reason for keeping this driver > > in staging? > > I think Nicolas would be better placed to comment on that. If it is not > considered a blocker, shouldn't there at least be a patch adding comments > at the relevant place describing this issue? That way it is at least > documented as a known issue. I *think* that Sebastian have identified how to detect when the self reset is not sufficient (and I guess the same may exist in LibreELEC tree). As the failure is not fatal, and it will recover if you let the codec idle for a little, I don't have any argument to make this a hard blocker. But before we unstated with this known issue, I'd like to see this well documented, and ideally see a kernel warning whenever that state is reached. >From there, it will be easy to justify a more invasive change to reset the entire block. It may also help identify when getting user reports of issues. Nicolas > > Regards, > > Hans > > > > > Regards, > > Jonas > > > > > > > > Regards, > > > > > > Hans > > > > > > > > > > > Patch 1 add the new HEVC backend. > > > > Patch 2-3 add variants support to the driver. > > > > Patch 4 add support for a rk3288 variant. > > > > Patch 5 add a rk3328 variant to work around hw quirks. > > > > Patch 6-7 add device tree node for rk3288. > > > > Patch 8-9 extend vdec node reg size to include cache/perf registers. > > > > > > > > This was tested on a ROCK Pi 4 (RK3399) and Rock64 (RK3328): > > > > > > > > v4l2-compliance 1.24.1, 64 bits, 64-bit time_t > > > > ... > > > > Total for rkvdec device /dev/video1: 46, Succeeded: 46, Failed: 0, Warnings: 0 > > > > > > > > Running test suite JCT-VC-HEVC_V1 with decoder FFmpeg-H.265-V4L2-request > > > > ... > > > > Ran 135/147 tests successfully > > > > > > > > Running test suite JCT-VC-MV-HEVC with decoder FFmpeg-H.265-V4L2-request > > > > ... > > > > Ran 9/9 tests successfully > > > > > > > > And on a TinkerBoard (RK3288): > > > > > > > > v4l2-compliance 1.24.1, 32 bits, 32-bit time_t > > > > ... > > > > Total for rkvdec device /dev/video3: 47, Succeeded: 47, Failed: 0, Warnings: 0 > > > > > > > > Running test suite JCT-VC-HEVC_V1 with decoder FFmpeg-H.265-V4L2-request > > > > ... > > > > Ran 137/147 tests successfully > > > > > > > > Running test suite JCT-VC-MV-HEVC with decoder FFmpeg-H.265-V4L2-request > > > > ... > > > > Ran 9/9 tests successfully > > > > > > > > This series depend on the following series: > > > > - media: rkvdec: Add H.264 High 10 and 4:2:2 profile support [3] > > > > > > > > To fully runtime test this series you need above series and ffmpeg > > > > patches from [4], this series and its depends is also available at [5]. > > > > > > > > Full summary of fluster run can be found at [6]. > > > > > > > > [1] https://github.com/LibreELEC/LibreELEC.tv/blob/master/projects/Rockchip/patches/linux/default/linux-2000-v4l2-wip-rkvdec-hevc.patch > > > > [2] https://lore.kernel.org/linux-media/20230101-patch-series-v2-6-2-rc1-v2-0-fa1897efac14@xxxxxxxxxxxxx/ > > > > [3] https://lore.kernel.org/linux-media/20231105165521.3592037-1-jonas@xxxxxxxxx/ > > > > [4] https://github.com/Kwiboo/FFmpeg/commits/v4l2-request-n6.1-dev/ > > > > [5] https://github.com/Kwiboo/linux-rockchip/commits/linuxtv-rkvdec-hevc-v1/ > > > > [6] https://gist.github.com/Kwiboo/4c0ed87774dede44ce6838451a1ec93d > > > > > > > > Regards, > > > > Jonas > > > > > > > > Alex Bee (5): > > > > media: rkvdec: Add variants support > > > > media: rkvdec: Add RK3288 variant > > > > media: rkvdec: Disable QoS for HEVC and VP9 on RK3328 > > > > ARM: dts: rockchip: Add vdec node for RK3288 > > > > arm64: dts: rockchip: Expand reg size of vdec node for RK3399 > > > > > > > > Jonas Karlman (4): > > > > media: rkvdec: Add HEVC backend > > > > media: rkvdec: Implement capability filtering > > > > media: dt-bindings: rockchip,vdec: Add RK3288 compatible > > > > arm64: dts: rockchip: Expand reg size of vdec node for RK3328 > > > > > > > > .../bindings/media/rockchip,vdec.yaml | 4 +- > > > > arch/arm/boot/dts/rockchip/rk3288.dtsi | 17 +- > > > > arch/arm64/boot/dts/rockchip/rk3328.dtsi | 2 +- > > > > arch/arm64/boot/dts/rockchip/rk3399.dtsi | 6 +- > > > > drivers/staging/media/rkvdec/Makefile | 2 +- > > > > drivers/staging/media/rkvdec/TODO | 7 - > > > > .../staging/media/rkvdec/rkvdec-hevc-data.c | 1848 +++++++++++++++++ > > > > drivers/staging/media/rkvdec/rkvdec-hevc.c | 823 ++++++++ > > > > drivers/staging/media/rkvdec/rkvdec-regs.h | 3 + > > > > drivers/staging/media/rkvdec/rkvdec-vp9.c | 10 + > > > > drivers/staging/media/rkvdec/rkvdec.c | 180 +- > > > > drivers/staging/media/rkvdec/rkvdec.h | 15 + > > > > 12 files changed, 2886 insertions(+), 31 deletions(-) > > > > create mode 100644 drivers/staging/media/rkvdec/rkvdec-hevc-data.c > > > > create mode 100644 drivers/staging/media/rkvdec/rkvdec-hevc.c > > > > > > > > > > >
