Re: [PATCH RFC v5 0/8] media: qcom: iris: re-organize catalog & add support for SM8650

[Vikash Garodia]

On 4/14/2025 5:39 PM, neil.armstrong@xxxxxxxxxx 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.

> 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.
> 
> 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.

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@xxxxxxxxxx/
>>>>
>>>> Signed-off-by: Neil Armstrong <neil.armstrong@xxxxxxxxxx>
>>>> ---
>>>> 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@xxxxxxxxxx
>>>>
>>>> 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@xxxxxxxxxx
>>>>
>>>> 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@xxxxxxxxxx
>>>>
>>>> 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@xxxxxxxxxx
>>>>
>>>> ---
>>>> 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,
>>>
>