Re: [PATCH 00/20] Add support for HEVC and VP9 codecs in decoder
From: Vikash Garodia
Date: Thu Apr 10 2025 - 03:32:45 EST
On 4/10/2025 12:50 PM, neil.armstrong@xxxxxxxxxx wrote:
> On 09/04/2025 19:59, Vikash Garodia wrote:
>>
>>
>> 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.
>
> Not really, I only add iris_set_sm8650_preset_registers()
You might have added iris_set_sm8650_preset_registers in this patch, but the
list have already grown enough which demand for separate SOC specific platform
data file. Better done now than keeping it for later.
Thanks,
Vikash
>
>>>
>>>>
>>>> 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.
>
> Yes this is why I added it only to enable support for HEVC and VP9, before we were
> using the iris_set_sm8550_preset_registers().
>
>>
>> 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
>