Re: [PATCH 00/20] Add support for HEVC and VP9 codecs in decoder

From: Nicolas Dufresne
Date: Tue Apr 08 2025 - 14:38:07 EST

Next message: kernel test robot: "[tip:x86/boot] BUILD SUCCESS 4f2d1bbc2c92a32fd612e6c3b51832d5c1c3678e"
Previous message: mhkelley58: "[PATCH 3/3] fbdev: hyperv_fb: Fix mmap of framebuffers allocated using alloc_pages()"
In reply to: Dikshita Agarwal: "[PATCH 19/20] media: iris: Add internal buffer calculation for HEVC and VP9 decoders"
Next in thread: Bryan O'Donoghue: "Re: [PATCH 00/20] Add support for HEVC and VP9 codecs in decoder"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

This is for Bryan and Vikash to review first, but here's some initial feedback on that report.

Le mardi 08 avril 2025 à 21:24 +0530, Dikshita Agarwal a écrit :
> Hi All,
>
> This patch series adds initial support for the HEVC(H.265) and VP9
> codecs in iris decoder. The objective of this work is to extend the
> decoder's capabilities to handle HEVC and VP9 codec streams,
> including necessary format handling and buffer management.
> In addition, the series also includes a set of fixes to address issues
> identified during testing of these additional codecs.
>
> These patches also address the comments and feedback received from the
> RFC patches previously sent. I have made the necessary improvements
> based on the community's suggestions.
>
> Changes sinces RFC:
> - Added additional fixes to address issues identified during further
> testing.
> - Moved typo fix to a seperate patch [Neil]
> - Reordered the patches for better logical flow and clarity [Neil,
> Dmitry]
> - Added fixes tag wherever applicable [Neil, Dmitry]
> - Removed the default case in the switch statement for codecs [Bryan]
> - Replaced if-else statements with switch-case [Bryan]
> - Added comments for mbpf [Bryan]
> - RFC: https://lore.kernel.org/linux-media/20250305104335.3629945-1-quic_dikshita@xxxxxxxxxxx/
>
> These patches are tested on SM8250 and SM8550 with v4l2-ctl and
> Gstreamer for HEVC and VP9 decoders, at the same time ensured that
> the existing H264 decoder functionality remains uneffected.
>
> Note: 1 of the fluster compliance test is fixed with firmware [1]
> [1]: https://lore.kernel.org/linux-firmware/1a511921-446d-cdc4-0203-084c88a5dc1e@xxxxxxxxxxx/T/#u ;
>
> The result of fluster test on SM8550:
> 131/147 testcases passed while testing JCT-VC-HEVC_V1 with
> GStreamer-H.265-V4L2-Gst1.0.
> The failing test case:
> - 10 testcases failed due to unsupported 10 bit format.
>    - DBLK_A_MAIN10_VIXS_4
>    - INITQP_B_Main10_Sony_1
>    - TSUNEQBD_A_MAIN10_Technicolor_2
>    - WP_A_MAIN10_Toshiba_3
>    - WP_MAIN10_B_Toshiba_3
>    - WPP_A_ericsson_MAIN10_2
>    - WPP_B_ericsson_MAIN10_2
>    - WPP_C_ericsson_MAIN10_2
>    - WPP_E_ericsson_MAIN10_2
>    - WPP_F_ericsson_MAIN10_2
> - 4 testcase failed due to unsupported resolution
>    - PICSIZE_A_Bossen_1
>    - PICSIZE_B_Bossen_1
>    - WPP_D_ericsson_MAIN10_2
>    - WPP_D_ericsson_MAIN_2
> - 1 testcase failed as bitstream is invalid (this fails with reference
>    as well)
>    - RAP_B_Bossen_2

I was surprised of this comment, so I ran it myself through the
official JCT reference decoder.

[JCT-VC-HEVC_V1] (JCT-VT-H.265) RAP_B_Bossen_2 ... Success

I'm pretty sure this stream is valid. I personally care about these
reports since otherwise we cannot differentiate HW limitation, FW bugs
or V4L2 API limitation. The later is really something we want to know
about, since we can fix it. We cannot fix HW, and its up to the vendor
to fix their firmware.

This specific test is very nice test. It changes the coding
width/height but keeps the conf window the same (416x240).

Here's the coded size changes I could see:

Frame 0: 448 x 256
Frame 25: 416 x 240
Frame 70: 448 x 256

Each time, the driver must sent SRC_CHANGE, and if all went well, the
decoder should reallocate. Note that all this code in GStreamer is
recent, so it also a possibility, but something we can fix. I haven't
tried ffmpeg much, but same.

> - 1 testcase failed due to CRC mismatch
>    - RAP_A_docomo_6
>      Analysis - First few frames in this discarded by firmware and are
>      sent to driver with 0 filled length. Driver send such buffers to
>      client with timestamp 0 and payload set to 0 and
>      make buf state to VB2_BUF_STATE_ERROR. Such buffers should be
>      dropped by GST. But instead, the first frame displayed as green
>      frame and when a valid buffer is sent to client later with same 0
>      timestamp, its dropped, leading to CRC mismatch for first frame.

This looks like a firmware bug, even if GStreamer had drop them all,
the CRC would still miss-match.

Important question is if this worked on Venus firmware ? If so, it
sounds like it should be addressed.

>
> 235/305 testcases passed while testing VP9-TEST-VECTORS with
> GStreamer-VP9-V4L2-Gst1.0.
> The failing test case:
> - 64 testcases failed due to unsupported resolution
>    - vp90-2-02-size-08x08.webm
>    - vp90-2-02-size-08x10.webm
>    - vp90-2-02-size-08x16.webm
>    - vp90-2-02-size-08x18.webm
>    - vp90-2-02-size-08x32.webm
>    - vp90-2-02-size-08x34.webm
>    - vp90-2-02-size-08x64.webm
>    - vp90-2-02-size-08x66.webm
>    - vp90-2-02-size-10x08.webm
>    - vp90-2-02-size-10x10.webm
>    - vp90-2-02-size-10x16.webm
>    - vp90-2-02-size-10x18.webm
>    - vp90-2-02-size-10x32.webm
>    - vp90-2-02-size-10x34.webm
>    - vp90-2-02-size-10x64.webm
>    - vp90-2-02-size-10x66.webm
>    - vp90-2-02-size-16x08.webm
>    - vp90-2-02-size-16x10.webm
>    - vp90-2-02-size-16x16.webm
>    - vp90-2-02-size-16x18.webm
>    - vp90-2-02-size-16x32.webm
>    - vp90-2-02-size-16x34.webm
>    - vp90-2-02-size-16x64.webm
>    - vp90-2-02-size-16x66.webm
>    - vp90-2-02-size-18x08.webm
>    - vp90-2-02-size-18x10.webm
>    - vp90-2-02-size-18x16.webm
>    - vp90-2-02-size-18x18.webm
>    - vp90-2-02-size-18x32.webm
>    - vp90-2-02-size-18x34.webm
>    - vp90-2-02-size-18x64.webm
>    - vp90-2-02-size-18x66.webm
>    - vp90-2-02-size-32x08.webm
>    - vp90-2-02-size-32x10.webm
>    - vp90-2-02-size-32x16.webm
>    - vp90-2-02-size-32x18.webm
>    - vp90-2-02-size-32x32.webm
>    - vp90-2-02-size-32x34.webm
>    - vp90-2-02-size-32x64.webm
>    - vp90-2-02-size-32x66.webm
>    - vp90-2-02-size-34x08.webm
>    - vp90-2-02-size-34x10.webm
>    - vp90-2-02-size-34x16.webm
>    - vp90-2-02-size-34x18.webm
>    - vp90-2-02-size-34x32.webm
>    - vp90-2-02-size-34x34.webm
>    - vp90-2-02-size-34x64.webm
>    - vp90-2-02-size-34x66.webm
>    - vp90-2-02-size-64x08.webm
>    - vp90-2-02-size-64x10.webm
>    - vp90-2-02-size-64x16.webm
>    - vp90-2-02-size-64x18.webm
>    - vp90-2-02-size-64x32.webm
>    - vp90-2-02-size-64x34.webm
>    - vp90-2-02-size-64x64.webm
>    - vp90-2-02-size-64x66.webm
>    - vp90-2-02-size-66x08.webm
>    - vp90-2-02-size-66x10.webm
>    - vp90-2-02-size-66x16.webm
>    - vp90-2-02-size-66x18.webm
>    - vp90-2-02-size-66x32.webm
>    - vp90-2-02-size-66x34.webm
>    - vp90-2-02-size-66x64.webm
>    - vp90-2-02-size-66x66.webm
> - 2 testcases failed due to unsupported format
>    - vp91-2-04-yuv422.webm
>    - vp91-2-04-yuv444.webm
> - 1 testcase failed with CRC mismatch (fails with ref decoder as well)
>    - vp90-2-22-svc_1280x720_3.ivf

Not completely true:

[VP9-TEST-VECTORS] (libvpx-VP9) vp90-2-22-svc_1280x720_3.ivf ... Success

But fails with the GStreamer libvpx integration. Would you mind filling
an issue please ? There is clearly a GStreamer bug (not a reference
one), please document it as-such in the next report.

> - 2 testcase failed due to unsupported resolution after sequence change
>    - vp90-2-21-resize_inter_320x180_5_1-2.webm
>    - vp90-2-21-resize_inter_320x180_7_1-2.webm

Just be more precise, inter-frame resolution changes are not currently
supported in V4L2 stateful decoders. The concept and internals are
ready though. Basically, userpace would have to progressively re-
allocate the frames as they get dequeued, before being re-queued. That
is significant userspace modification, fine to not support it atm.

> - 1 testcase failed due to unsupported stream
>    - vp90-2-16-intra-only.webm
> Note: There is a timing issue with the clips having multiple resolution
> change. Where firmware returned all the buffers with previous sequence
> and has no buffer left to attach the LAST flag to. At the same time,
> client is not queueing any further buffers, so there is deadlock where
> client is waiting for LAST flag, while firmware doesn't have any
> capture buffer to attach LAST flag to.
> Ideally client should keep queueing the buffers on capture queue untill
> the LAST flag is received.

At least GStreamer do, so I slightly doubt your analyzes (happy to see
a bug report with relevant demonstration showing that !). I'll read it
as there is still bug with the resolution change draining process in
this driver.

>
> The result of fluster test on SM8250:
> 132/147 testcases passed while testing JCT-VC-HEVC_V1 with
> GStreamer-H.265-V4L2-Gst1.0.
> The failing test case:
> - 10 testcases failed due to unsupported 10 bit format.
>    - DBLK_A_MAIN10_VIXS_4
>    - INITQP_B_Main10_Sony_1
>    - TSUNEQBD_A_MAIN10_Technicolor_2
>    - WP_A_MAIN10_Toshiba_3
>    - WP_MAIN10_B_Toshiba_3
>    - WPP_A_ericsson_MAIN10_2
>    - WPP_B_ericsson_MAIN10_2
>    - WPP_C_ericsson_MAIN10_2
>    - WPP_E_ericsson_MAIN10_2
>    - WPP_F_ericsson_MAIN10_2
> - 4 testcase failed due to unsupported resolution
>    - PICSIZE_A_Bossen_1
>    - PICSIZE_B_Bossen_1
>    - WPP_D_ericsson_MAIN10_2
>    - WPP_D_ericsson_MAIN_2
> - 1 testcase failed as bitstream is invalid (this fails with reference
>    as well)
>    - RAP_B_Bossen_2
>
> 232/305 testcases passed while testing VP9-TEST-VECTORS with
> GStreamer-VP9-V4L2-Gst1.0.
> The failing test case:
> - 64 testcases failed due to unsupported resolution
>    - vp90-2-02-size-08x08.webm
>    - vp90-2-02-size-08x10.webm
>    - vp90-2-02-size-08x16.webm
>    - vp90-2-02-size-08x18.webm
>    - vp90-2-02-size-08x32.webm
>    - vp90-2-02-size-08x34.webm
>    - vp90-2-02-size-08x64.webm
>    - vp90-2-02-size-08x66.webm
>    - vp90-2-02-size-10x08.webm
>    - vp90-2-02-size-10x10.webm
>    - vp90-2-02-size-10x16.webm
>    - vp90-2-02-size-10x18.webm
>    - vp90-2-02-size-10x32.webm
>    - vp90-2-02-size-10x34.webm
>    - vp90-2-02-size-10x64.webm
>    - vp90-2-02-size-10x66.webm
>    - vp90-2-02-size-16x08.webm
>    - vp90-2-02-size-16x10.webm
>    - vp90-2-02-size-16x16.webm
>    - vp90-2-02-size-16x18.webm
>    - vp90-2-02-size-16x32.webm
>    - vp90-2-02-size-16x34.webm
>    - vp90-2-02-size-16x64.webm
>    - vp90-2-02-size-16x66.webm
>    - vp90-2-02-size-18x08.webm
>    - vp90-2-02-size-18x10.webm
>    - vp90-2-02-size-18x16.webm
>    - vp90-2-02-size-18x18.webm
>    - vp90-2-02-size-18x32.webm
>    - vp90-2-02-size-18x34.webm
>    - vp90-2-02-size-18x64.webm
>    - vp90-2-02-size-18x66.webm
>    - vp90-2-02-size-32x08.webm
>    - vp90-2-02-size-32x10.webm
>    - vp90-2-02-size-32x16.webm
>    - vp90-2-02-size-32x18.webm
>    - vp90-2-02-size-32x32.webm
>    - vp90-2-02-size-32x34.webm
>    - vp90-2-02-size-32x64.webm
>    - vp90-2-02-size-32x66.webm
>    - vp90-2-02-size-34x08.webm
>    - vp90-2-02-size-34x10.webm
>    - vp90-2-02-size-34x16.webm
>    - vp90-2-02-size-34x18.webm
>    - vp90-2-02-size-34x32.webm
>    - vp90-2-02-size-34x34.webm
>    - vp90-2-02-size-34x64.webm
>    - vp90-2-02-size-34x66.webm
>    - vp90-2-02-size-64x08.webm
>    - vp90-2-02-size-64x10.webm
>    - vp90-2-02-size-64x16.webm
>    - vp90-2-02-size-64x18.webm
>    - vp90-2-02-size-64x32.webm
>    - vp90-2-02-size-64x34.webm
>    - vp90-2-02-size-64x64.webm
>    - vp90-2-02-size-64x66.webm
>    - vp90-2-02-size-66x08.webm
>    - vp90-2-02-size-66x10.webm
>    - vp90-2-02-size-66x16.webm
>    - vp90-2-02-size-66x18.webm
>    - vp90-2-02-size-66x32.webm
>    - vp90-2-02-size-66x34.webm
>    - vp90-2-02-size-66x64.webm
>    - vp90-2-02-size-66x66.webm
> - 2 testcases failed due to unsupported format
>    - vp91-2-04-yuv422.webm
>    - vp91-2-04-yuv444.webm
> - 1 testcase failed with CRC mismatch (fails with ref decoder as well)
>    - vp90-2-22-svc_1280x720_3.ivf
> - 5 testcase failed due to unsupported resolution after sequence change
>    - vp90-2-21-resize_inter_320x180_5_1-2.webm
>    - vp90-2-21-resize_inter_320x180_7_1-2.webm
>    - vp90-2-21-resize_inter_320x240_5_1-2.webm
>    - vp90-2-21-resize_inter_320x240_7_1-2.webm
>    - vp90-2-18-resize.ivf
> - 1 testcase failed with CRC mismatch
>    - vp90-2-16-intra-only.webm
>    Analysis: First few frames are marked by firmware as NO_SHOW frame.
>    Driver make buf state to VB2_BUF_STATE_ERROR for such frames.
>    Such buffers should be dropped by GST. But instead, the first frame
>    is being displayed and when a valid buffer is sent to client later
>    with same timestamp, its dropped, leading to CRC mismatch for first
>    frame.

I checked this one, 3 first frame are "decode only" indeed. Though, by
returning these buffer with STATE_ERROR, you loose the ability to show
these frame later. GStreamer should be fixed to do internal ref frame
management, not silently drop these though.

What you can do instead, is leave these frames queued, and only mark
them done once you hit a matching show_existing_frame. It will fix this
specific stream (and most usage of show existing frame in the wild),
but show-existing frame will remain broken. The other solution is to
keep the reference frame internal, and output copies, but that imply
some HW design, and also cost quite a lot in bandwidth.

This test hits the known V4L2 queue limitation that it can only output
a frame once, and can only input a frame once (you cannot encode twice
the same frame notably). I open to suggestion how to fix those, but I
don't have any solutions for now.

p.s. a third solution is make the firmware stateless :-D

> Note: Same timing issue as observed on SM8550 is seen on SM8250 as
> well.
>
> Signed-off-by: Dikshita Agarwal <quic_dikshita@xxxxxxxxxxx>
> ---
> Dikshita Agarwal (20):
>       media: iris: Skip destroying internal buffer if not dequeued
>       media: iris: Update CAPTURE format info based on OUTPUT format
>       media: iris: Add handling for corrupt and drop frames
>       media: iris: Avoid updating frame size to firmware during reconfig
>       media: iris: Send V4L2_BUF_FLAG_ERROR for buffers with 0 filled length
>       media: iris: Add handling for no show frames
>       media: iris: Improve last flag handling
>       media: iris: Skip flush on first sequence change
>       media: iris: Prevent HFI queue writes when core is in deinit state
>       media: iris: Remove redundant buffer count check in stream off
>       media: iris: Remove deprecated property setting to firmware
>       media: iris: Fix missing function pointer initialization
>       media: iris: Fix NULL pointer dereference
>       media: iris: Fix typo in depth variable
>       media: iris: Add a comment to explain usage of MBPS
>       media: iris: Add HEVC and VP9 formats for decoder
>       media: iris: Add platform capabilities for HEVC and VP9 decoders
>       media: iris: Set mandatory properties for HEVC and VP9 decoders.
>       media: iris: Add internal buffer calculation for HEVC and VP9 decoders
>       media: iris: Add codec specific check for VP9 decoder drain handling
>
> drivers/media/platform/qcom/iris/iris_buffer.c     | 22 +-
> drivers/media/platform/qcom/iris/iris_ctrls.c      | 35 +-
> drivers/media/platform/qcom/iris/iris_hfi_common.h |   1 +
> .../platform/qcom/iris/iris_hfi_gen1_command.c     | 44 ++-
> .../platform/qcom/iris/iris_hfi_gen1_defines.h     |   5 +-
> .../platform/qcom/iris/iris_hfi_gen1_response.c    | 22 +-
> .../platform/qcom/iris/iris_hfi_gen2_command.c     | 143 +++++++-
> .../platform/qcom/iris/iris_hfi_gen2_defines.h     |   5 +
> .../platform/qcom/iris/iris_hfi_gen2_response.c    | 57 ++-
> drivers/media/platform/qcom/iris/iris_hfi_queue.c |   2 +-
> drivers/media/platform/qcom/iris/iris_instance.h   |   6 +
> .../platform/qcom/iris/iris_platform_common.h      | 28 +-
> .../platform/qcom/iris/iris_platform_sm8250.c      | 15 +-
> .../platform/qcom/iris/iris_platform_sm8550.c      | 143 +++++++-
> drivers/media/platform/qcom/iris/iris_vb2.c        |   3 +-
> drivers/media/platform/qcom/iris/iris_vdec.c       | 113 +++---
> drivers/media/platform/qcom/iris/iris_vdec.h       | 11 +
> drivers/media/platform/qcom/iris/iris_vidc.c       |   3 -
> drivers/media/platform/qcom/iris/iris_vpu_buffer.c | 397 ++++++++++++++++++++-
> drivers/media/platform/qcom/iris/iris_vpu_buffer.h | 46 ++-
> 20 files changed, 948 insertions(+), 153 deletions(-)
> ---
> base-commit: 7824b91d23e9f255f0e9d2acaa74265c9cac2e9c
> change-id: 20250402-iris-dec-hevc-vp9-2654a1fc4d0d
>
> Best regards,

Next message: kernel test robot: "[tip:x86/boot] BUILD SUCCESS 4f2d1bbc2c92a32fd612e6c3b51832d5c1c3678e"
Previous message: mhkelley58: "[PATCH 3/3] fbdev: hyperv_fb: Fix mmap of framebuffers allocated using alloc_pages()"
In reply to: Dikshita Agarwal: "[PATCH 19/20] media: iris: Add internal buffer calculation for HEVC and VP9 decoders"
Next in thread: Bryan O'Donoghue: "Re: [PATCH 00/20] Add support for HEVC and VP9 codecs in decoder"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]