Re: [PATCH 00/20] Add support for HEVC and VP9 codecs in decoder
From: Vikash Garodia
Date: Wed Apr 09 2025 - 14:00:13 EST
On 4/9/2025 9:56 PM, Neil Armstrong wrote:
> On 09/04/2025 16:29, Bryan O'Donoghue wrote:
>> On 08/04/2025 16:54, Dikshita Agarwal wrote:
>>> 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
>>>
>
> <snip>
>
>>> ---
>>> 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,
>>
>> Assuming we merge Neils sm8650 stuff first, which I think we should merge
>> first, you'll have a subsequent build error to fix [1]
>
> I agree, it would be simpler, I prepared a fix to apply on top of this patchset.
Lets sort out the platform data handling. More so, when i see that the patch you
are adding more of 8650 specific data into 8550 file.
>
>>
>> https://git.codelinaro.org/bryan.odonoghue/kernel/-/tree/linaro/arm-laptop/wip/x1e80100-6.15-rc1-dell-inspiron14-camss-ov02c10-ov02e10-audio-iris?ref_type=heads
>>
>> Testing your series in isolation. I can confirm vp9 decodes also getting some
>> strange prinouts which we need to follow up to see if they exist with the
>> baseline driver [2].
>>
>> https://git.codelinaro.org/bryan.odonoghue/kernel/-/tree/linaro/arm-laptop/wip/x1e80100-6.15-rc1-dell-inspiron14-camss-ov02c10-ov02e10-audio-iris-20250408-iris-dec-hevc-vp9-v1-0-acd258778bd6@xxxxxxxxxxx?ref_type=heads
>>
>
> <snip>
>
>> [ 126.582170] qcom-iris aa00000.video-codec: session error received
>> 0x1000006: unknown
>> [ 126.582177] qcom-iris aa00000.video-codec: session error received
>> 0x4000004: invalid operation for current state
>
> With the following on top of the last SM8650 patchet + this patchset, I have the
> same HEVC errors on SM8650, but VP9 works fine:
> [ 115.185745] qcom-iris aa00000.video-codec: session error received 0x4000004:
> invalid operation for current state
> [ 115.221058] qcom-iris aa00000.video-codec: session error received 0x1000006:
> unknown
>
> ==========================================><==============================================
> diff --git a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c
> b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c
> index 65f3accc2fb2..7d5116528fca 100644
> --- a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c
> +++ b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c
> @@ -213,6 +213,22 @@ static void iris_set_sm8550_preset_registers(struct
> iris_core *core)
> writel(0x0, core->reg_base + 0xB0088);
> }
>
> +static void iris_set_sm8650_preset_registers(struct iris_core *core)
> +{
> + writel(0x0, core->reg_base + 0xB0088);
> + writel(0x33332222, core->reg_base + 0x13030);
> + writel(0x44444444, core->reg_base + 0x13034);
> + writel(0x1022, core->reg_base + 0x13038);
> + writel(0x0, core->reg_base + 0x13040);
> + writel(0xFFFF, core->reg_base + 0x13048);
> + writel(0x33332222, core->reg_base + 0x13430);
> + writel(0x44444444, core->reg_base + 0x13434);
> + writel(0x1022, core->reg_base + 0x13438);
> + writel(0x0, core->reg_base + 0x13440);
> + writel(0xFFFF, core->reg_base + 0x13448);
> + writel(0x99, core->reg_base + 0xA013C);
> +}
This is strange, h264 decoder does not need any of those while VP9 needed it to
work. I could see the same set of registers in downstream code, but cannot
recollect now on the need to add those.
Regards,
Vikash
> +
> static const struct icc_info sm8550_icc_table[] = {
> { "cpu-cfg", 1000, 1000 },
> { "video-mem", 1000, 15000000 },
> @@ -390,6 +406,7 @@ struct iris_platform_data sm8550_data = {
>
> /*
> * Shares most of SM8550 data except:
> + * - set_preset_registers to iris_set_sm8650_preset_registers
> * - vpu_ops to iris_vpu33_ops
> * - clk_rst_tbl to sm8650_clk_reset_table
> * - controller_rst_tbl to sm8650_controller_reset_table
> @@ -400,7 +417,7 @@ struct iris_platform_data sm8650_data = {
> .init_hfi_command_ops = iris_hfi_gen2_command_ops_init,
> .init_hfi_response_ops = iris_hfi_gen2_response_ops_init,
> .vpu_ops = &iris_vpu33_ops,
> - .set_preset_registers = iris_set_sm8550_preset_registers,
> + .set_preset_registers = iris_set_sm8650_preset_registers,
> .icc_tbl = sm8550_icc_table,
> .icc_tbl_size = ARRAY_SIZE(sm8550_icc_table),
> .clk_rst_tbl = sm8650_clk_reset_table,
> @@ -428,20 +445,34 @@ struct iris_platform_data sm8650_data = {
> .ubwc_config = &ubwc_config_sm8550,
> .num_vpp_pipe = 4,
> .max_session_count = 16,
> - .max_core_mbpf = ((8192 * 4352) / 256) * 2,
> - .input_config_params =
> - sm8550_vdec_input_config_params,
> - .input_config_params_size =
> - ARRAY_SIZE(sm8550_vdec_input_config_params),
> + .max_core_mbpf = NUM_MBS_8K * 2,
> + .input_config_params_default =
> + sm8550_vdec_input_config_params_default,
> + .input_config_params_default_size =
> + ARRAY_SIZE(sm8550_vdec_input_config_params_default),
> + .input_config_params_hevc =
> + sm8550_vdec_input_config_param_hevc,
> + .input_config_params_hevc_size =
> + ARRAY_SIZE(sm8550_vdec_input_config_param_hevc),
> + .input_config_params_vp9 =
> + sm8550_vdec_input_config_param_vp9,
> + .input_config_params_vp9_size =
> + ARRAY_SIZE(sm8550_vdec_input_config_param_vp9),
> .output_config_params =
> sm8550_vdec_output_config_params,
> .output_config_params_size =
> ARRAY_SIZE(sm8550_vdec_output_config_params),
> .dec_input_prop = sm8550_vdec_subscribe_input_properties,
> .dec_input_prop_size = ARRAY_SIZE(sm8550_vdec_subscribe_input_properties),
> - .dec_output_prop = sm8550_vdec_subscribe_output_properties,
> - .dec_output_prop_size = ARRAY_SIZE(sm8550_vdec_subscribe_output_properties),
> -
> + .dec_output_prop_avc = sm8550_vdec_subscribe_output_properties_avc,
> + .dec_output_prop_avc_size =
> + ARRAY_SIZE(sm8550_vdec_subscribe_output_properties_avc),
> + .dec_output_prop_hevc = sm8550_vdec_subscribe_output_properties_hevc,
> + .dec_output_prop_hevc_size =
> + ARRAY_SIZE(sm8550_vdec_subscribe_output_properties_hevc),
> + .dec_output_prop_vp9 = sm8550_vdec_subscribe_output_properties_vp9,
> + .dec_output_prop_vp9_size =
> + ARRAY_SIZE(sm8550_vdec_subscribe_output_properties_vp9),
> .dec_ip_int_buf_tbl = sm8550_dec_ip_int_buf_tbl,
> .dec_ip_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_ip_int_buf_tbl),
> .dec_op_int_buf_tbl = sm8550_dec_op_int_buf_tbl,
> ==========================================><==============================================
>
> Thanks,
> Neil