Message-ID: <0c4fcd81-8e1f-4b4b-a345-c08caeb599c8@linaro.org>
Date: Tue, 15 Apr 2025 10:24:24 +0200
From: neil.armstrong@...aro.org
To: Vikash Garodia <quic_vgarodia@...cinc.com>,
 Dikshita Agarwal <quic_dikshita@...cinc.com>,
 Abhinav Kumar <quic_abhinavk@...cinc.com>,
 Mauro Carvalho Chehab <mchehab@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Philipp Zabel <p.zabel@...gutronix.de>
Cc: linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
 linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
 Bryan O'Donoghue <bryan.odonoghue@...aro.org>
Subject: Re: [PATCH RFC v5 0/8] media: qcom: iris: re-organize catalog & add
 support for SM8650

Hi,

On 14/04/2025 21:48, Vikash Garodia wrote:
> 
> On 4/14/2025 5:39 PM, neil.armstrong@...aro.org wrote:
>> Hi,
>>
>> On 14/04/2025 12:54, Vikash Garodia wrote:
>>> Hi Neil,
>>>
>>> On 4/14/2025 1:05 PM, Neil Armstrong wrote:
>>>> Hi Vikash, Dikshita,
>>>>
>>>> On 10/04/2025 18:29, Neil Armstrong wrote:
>>>>> Re-organize the platform support core into a gen1 catalog C file
>>>>> declaring common platform structure and include platform headers
>>>>> containing platform specific entries and iris_platform_data
>>>>> structure.
>>>>>
>>>>> The goal is to share most of the structure while having
>>>>> clear and separate per-SoC catalog files.
>>>>>
>>>>> The organization is based on the curent drm/msm dpu1 catalog
>>>>> entries.
>>>>
>>>> Any feedback on this patchset ?
>>> Myself and Dikshita went through the approach you are bringing here, let me
>>> update some context here:
>>> - sm8550, sm8650, sm8775p, qcs8300 are all irisv3, while qcs8300 is the scaled
>>> down variant i.e have 2 PIPE vs others having 4. Similarly there are other
>>> irisv3 having 1 pipe as well.
>>> - With above variations, firmware and instance caps would change for the variant
>>> SOCs.
>>> - Above these, few(less) bindings/connections specific delta would be there,
>>> like there is reset delta in sm8550 and sm8650.
>>>
>>> Given above, xxx_gen1.c and xxx_gen2.c can have all binding specific tables and
>>> SOC platform data, i.e sm8650_data (for sm8650). On top of this, individual SOC
>>> specific .c file can have any delta, from xxx_gen1/2.c) like reset table or
>>> preset register table, etc and export these delta structs in xxx_gen1.c or
>>> xxx_gen2.c.
>>>
>>> Going with above approach, sm8650.c would have only one reset table for now.
>>> Later if any delta is identified, the same can be added in it. All other common
>>> structs, can reside in xxx_gen2.c for now.
>>
>> Thanks for reviewing, but...
>> Sorry I don't understand what you and Dmitry are asking me...
>>
>> If I try really hard, you would like to have:
>>
>> iris_catalog_sm8550.c
>> - iris_set_sm8550_preset_registers
>> - sm8550_icc_table
>> - sm8550_clk_reset_table
>> - sm8550_bw_table_dec
>> - sm8550_pmdomain_table
>> - sm8550_opp_pd_table
>> - sm8550_clk_table
> Move or rename existing 8550.c as xxx_gen2.c. This is with the existing
> assumption that everything under 8550.c is common for all gen2 to come in future.
>>
>> iris_catalog_sm8650.c
>> - sm8650_clk_reset_table
>> - sm8650_controller_reset_table
> yes, since reset is the only delta.
>>
>> iris_catalog_gen2.c
>> - iris_hfi_gen2_command_ops_init
>> - iris_hfi_gen2_response_ops_init
>> ...
>> - sm8550_dec_op_int_buf_tbl
>>
>> and:
>> - struct iris_platform_data sm8550_data
>> - struct iris_platform_data sm8650_data
> all this goes to xxx_gen2.c as well.

Yeah so this is exactly my current approach, except it use .h files
for each SoC for simplicity.

> 
>> using data from iris_catalog_sm8550.c & iris_catalog_sm8550.c
>>
>> So this is basically what I _already_ propose except
>> you move data in separate .c files for no reasons,
>> please explain why you absolutely want distinct .c
>> files per SoC. We are no more in the 1990's and we camn
>> defintely have big .c files.
> Its not about the size of file alone, it is easy to understand later what would
> be the delta in the SOCs and what would common. For ex, just navigating through
> sm8650.c, anyone can comment that reset is the delta.

What's the problem with the current approach with .h file for each SoC ?

>>
>> And we still have a big issue, how to get the:
>> - ARRAY_SIZE(sm8550_clk_reset_table)
>> - ARRAY_SIZE(sm8550_bw_table_dec)
>> - ARRAY_SIZE(sm8550_pmdomain_table)
>> ...
>>
>> since they are declared in a separate .c file and you
>> need a compile-time const value to fill all the _size
>> attribute in iris_platform_data.
> I have not tries this, but isn't extern-ing the soc structs (in your case reset
> tables) into xxx_gen2.c enough here ? Also i think the tables you are pointing
> here, lies in the xxx_gen2.c only, so i am sure above ones would not be an issue
> at all. The only delta struct is reset table, lets see if extern helps.

No it doesn't, because I wrote C for the last 25 years, and I tried it already,
I also tried to export a const int with the table size, and it also doesn't work.

The 3 only ways are:
1) add defines with sizes for each table
2) add a NULL entry at the end of each table, and update all code using those tables
3) declare in the same scope, which is my current proposal

Neil

> 
> Regards,
> Vikash
>>
>> So I recall my goal, I just want to add sm8650 support,
>> and I'm not the owner of this driver, and I'm really happy
>> to help, but giving me random ideas to solve your problem
>> doesn't help us at all going forward.
>>
>> Neil
>>
>>>
>>> Regards,
>>> Vikash
>>>>
>>>> Thanks,
>>>> Neil
>>>>
>>>>>
>>>>> Add support for the IRIS accelerator for the SM8650
>>>>> platform, which uses the iris33 hardware.
>>>>>
>>>>> The vpu33 requires a different reset & poweroff sequence
>>>>> in order to properly get out of runtime suspend.
>>>>>
>>>>> Follow-up of [1]:
>>>>> https://lore.kernel.org/all/20250409-topic-sm8x50-iris-v10-v4-0-40e411594285@linaro.org/
>>>>>
>>>>> Signed-off-by: Neil Armstrong <neil.armstrong@...aro.org>
>>>>> ---
>>>>> Changes in v4:
>>>>> - Reorganized into catalog, rebased sm8650 support on top
>>>>> - Link to v4:
>>>>> https://lore.kernel.org/all/20250409-topic-sm8x50-iris-v10-v4-0-40e411594285@linaro.org
>>>>>
>>>>> Changes in v4:
>>>>> - collected tags
>>>>> - un-split power_off in vpu3x
>>>>> - removed useless function defines
>>>>> - added back vpu3x disappeared rename commit
>>>>> - Link to v3:
>>>>> https://lore.kernel.org/r/20250407-topic-sm8x50-iris-v10-v3-0-63569f6d04aa@linaro.org
>>>>>
>>>>> Changes in v3:
>>>>> - Collected review tags
>>>>> - Removed bulky reset_controller ops
>>>>> - Removed iris_vpu_power_off_controller split
>>>>> - Link to v2:
>>>>> https://lore.kernel.org/r/20250305-topic-sm8x50-iris-v10-v2-0-bd65a3fc099e@linaro.org
>>>>>
>>>>> Changes in v2:
>>>>> - Collected bindings review
>>>>> - Reworked rest handling by adding a secondary optional table to be used by
>>>>> controller poweroff
>>>>> - Reworked power_off_controller to be reused and extended by vpu33 support
>>>>> - Removed useless and unneeded vpu33 init
>>>>> - Moved vpu33 into vpu3x files to reuse code from vpu3
>>>>> - Moved sm8650 data table into sm8550
>>>>> - Link to v1:
>>>>> https://lore.kernel.org/r/20250225-topic-sm8x50-iris-v10-v1-0-128ef05d9665@linaro.org
>>>>>
>>>>> ---
>>>>> Neil Armstrong (8):
>>>>>          media: qcom: iris: move sm8250 to gen1 catalog
>>>>>          media: qcom: iris: move sm8550 to gen2 catalog
>>>>>          dt-bindings: media: qcom,sm8550-iris: document SM8650 IRIS accelerator
>>>>>          media: platform: qcom/iris: add power_off_controller to vpu_ops
>>>>>          media: platform: qcom/iris: introduce optional controller_rst_tbl
>>>>>          media: platform: qcom/iris: rename iris_vpu3 to iris_vpu3x
>>>>>          media: platform: qcom/iris: add support for vpu33
>>>>>          media: platform: qcom/iris: add sm8650 support
>>>>>
>>>>>     .../bindings/media/qcom,sm8550-iris.yaml           |  33 ++-
>>>>>     drivers/media/platform/qcom/iris/Makefile          |   6 +-
>>>>>     .../media/platform/qcom/iris/iris_catalog_gen1.c   |  83 +++++++
>>>>>     ...{iris_platform_sm8550.c => iris_catalog_gen2.c} |  85 +------
>>>>>     ...ris_platform_sm8250.c => iris_catalog_sm8250.h} |  80 +-----
>>>>>     .../media/platform/qcom/iris/iris_catalog_sm8550.h |  91 +++++++
>>>>>     .../media/platform/qcom/iris/iris_catalog_sm8650.h |  68 +++++
>>>>>     drivers/media/platform/qcom/iris/iris_core.h       |   1 +
>>>>>     .../platform/qcom/iris/iris_platform_common.h      |   3 +
>>>>>     drivers/media/platform/qcom/iris/iris_probe.c      |  43 +++-
>>>>>     drivers/media/platform/qcom/iris/iris_vpu2.c       |   1 +
>>>>>     drivers/media/platform/qcom/iris/iris_vpu3.c       | 122 ---------
>>>>>     drivers/media/platform/qcom/iris/iris_vpu3x.c      | 275
>>>>> +++++++++++++++++++++
>>>>>     drivers/media/platform/qcom/iris/iris_vpu_common.c |   4 +-
>>>>>     drivers/media/platform/qcom/iris/iris_vpu_common.h |   3 +
>>>>>     15 files changed, 598 insertions(+), 300 deletions(-)
>>>>> ---
>>>>> base-commit: 2bdde620f7f2bff2ff1cb7dc166859eaa0c78a7c
>>>>> change-id: 20250410-topic-sm8x50-upstream-iris-catalog-3e2e4a033d6f
>>>>>
>>>>> Best regards,
>>>>
>>

Powered by blists - more mailing lists