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

[neil . armstrong]

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

iris_catalog_sm8650.c
- sm8650_clk_reset_table
- sm8650_controller_reset_table

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

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.

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,