Re: [PATCH RFC v5 0/8] media: qcom: iris: re-organize catalog & add support for SM8650
From: neil . armstrong
Date: Tue Apr 15 2025 - 04:29:09 EST
Hi,
On 14/04/2025 21:48, Vikash Garodia wrote:
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.
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@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,