| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d43d0b56-83e0-4567-a99a-6611a146954e@linaro.org>
Date: Thu, 10 Apr 2025 09:20:12 +0200
From: neil.armstrong@...aro.org
To: Vikash Garodia <quic_vgarodia@...cinc.com>,
Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
Dikshita Agarwal <quic_dikshita@...cinc.com>,
Abhinav Kumar <quic_abhinavk@...cinc.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Stefan Schmidt <stefan.schmidt@...aro.org>, Hans Verkuil
<hverkuil@...all.nl>, Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konradybcio@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>
Cc: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
linux-media@...r.kernel.org, linux-arm-msm@...r.kernel.org,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
stable@...r.kernel.org, Dan Carpenter <dan.carpenter@...aro.org>
Subject: Re: [PATCH 00/20] Add support for HEVC and VP9 codecs in decoder
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@quicinc.com/
>>>>
>>>> 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@quicinc.com/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()
>>
>>>
>>> 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@quicinc.com?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
Powered by blists - more mailing lists