Message ID | 20250409-topic-sm8x50-iris-v10-v4-0-40e411594285@linaro.org |
---|---|
Headers | show |
Series | media: qcom: iris: add support for SM8650 | expand |
On 09/04/2025 18:57, Vikash Garodia wrote: > Hi Neil, > > On 4/9/2025 8:08 PM, Neil Armstrong wrote: >> Add support for the SM8650 platform by re-using the SM8550 >> definitions and using the vpu33 ops. >> >> The SM8650/vpu33 requires more reset lines, but the H.264 >> decoder capabilities are identical. >> >> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # x1e Dell >> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >> --- >> .../platform/qcom/iris/iris_platform_common.h | 1 + >> .../platform/qcom/iris/iris_platform_sm8550.c | 64 ++++++++++++++++++++++ >> drivers/media/platform/qcom/iris/iris_probe.c | 4 ++ >> 3 files changed, 69 insertions(+) >> >> diff --git a/drivers/media/platform/qcom/iris/iris_platform_common.h b/drivers/media/platform/qcom/iris/iris_platform_common.h >> index fdd40fd80178c4c66b37e392d07a0a62f492f108..6bc3a7975b04d612f6c89206eae95dac678695fc 100644 >> --- a/drivers/media/platform/qcom/iris/iris_platform_common.h >> +++ b/drivers/media/platform/qcom/iris/iris_platform_common.h >> @@ -35,6 +35,7 @@ enum pipe_type { >> >> extern struct iris_platform_data sm8250_data; >> extern struct iris_platform_data sm8550_data; >> +extern struct iris_platform_data sm8650_data; >> >> enum platform_clk_type { >> IRIS_AXI_CLK, >> diff --git a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >> index 35d278996c430f2856d0fe59586930061a271c3e..d0f8fa960d53367023e41bc5807ba3f8beae2efc 100644 >> --- a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >> +++ b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >> @@ -144,6 +144,10 @@ static const struct icc_info sm8550_icc_table[] = { >> >> static const char * const sm8550_clk_reset_table[] = { "bus" }; >> >> +static const char * const sm8650_clk_reset_table[] = { "bus", "core" }; >> + >> +static const char * const sm8650_controller_reset_table[] = { "xo" }; >> + >> static const struct bw_info sm8550_bw_table_dec[] = { >> { ((4096 * 2160) / 256) * 60, 1608000 }, >> { ((4096 * 2160) / 256) * 30, 826000 }, >> @@ -264,3 +268,63 @@ struct iris_platform_data sm8550_data = { >> .dec_op_int_buf_tbl = sm8550_dec_op_int_buf_tbl, >> .dec_op_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_op_int_buf_tbl), >> }; >> + >> +/* >> + * Shares most of SM8550 data except: >> + * - vpu_ops to iris_vpu33_ops >> + * - clk_rst_tbl to sm8650_clk_reset_table >> + * - controller_rst_tbl to sm8650_controller_reset_table >> + * - fwname to "qcom/vpu/vpu33_p4.mbn" >> + */ >> +struct iris_platform_data sm8650_data = { >> + .get_instance = iris_hfi_gen2_get_instance, >> + .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, >> + .icc_tbl = sm8550_icc_table, >> + .icc_tbl_size = ARRAY_SIZE(sm8550_icc_table), >> + .clk_rst_tbl = sm8650_clk_reset_table, >> + .clk_rst_tbl_size = ARRAY_SIZE(sm8650_clk_reset_table), >> + .controller_rst_tbl = sm8650_controller_reset_table, >> + .controller_rst_tbl_size = ARRAY_SIZE(sm8650_controller_reset_table), >> + .bw_tbl_dec = sm8550_bw_table_dec, >> + .bw_tbl_dec_size = ARRAY_SIZE(sm8550_bw_table_dec), >> + .pmdomain_tbl = sm8550_pmdomain_table, >> + .pmdomain_tbl_size = ARRAY_SIZE(sm8550_pmdomain_table), >> + .opp_pd_tbl = sm8550_opp_pd_table, >> + .opp_pd_tbl_size = ARRAY_SIZE(sm8550_opp_pd_table), >> + .clk_tbl = sm8550_clk_table, >> + .clk_tbl_size = ARRAY_SIZE(sm8550_clk_table), >> + /* Upper bound of DMA address range */ >> + .dma_mask = 0xe0000000 - 1, >> + .fwname = "qcom/vpu/vpu33_p4.mbn", >> + .pas_id = IRIS_PAS_ID, >> + .inst_caps = &platform_inst_cap_sm8550, >> + .inst_fw_caps = inst_fw_cap_sm8550, >> + .inst_fw_caps_size = ARRAY_SIZE(inst_fw_cap_sm8550), >> + .tz_cp_config_data = &tz_cp_config_sm8550, >> + .core_arch = VIDEO_ARCH_LX, >> + .hw_response_timeout = HW_RESPONSE_TIMEOUT_VALUE, >> + .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), >> + .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_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, >> + .dec_op_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_op_int_buf_tbl), >> +}; > While i was extending the data for QCS8300 (one another iris-v3 variant), i > realize that this file iris_platform_sm8550.c is getting dumped with all SOC > platform data. It would be a good idea at this point to split it into something > like this > 1. Introduce SOC specific c file and move the respective SOC platform data to > it, for ex, in this case sm8650_data > 2. Move the common structs from iris_platform_sm8550.c to > iris_platform_common.h. This way more SOCs getting added in future, can include > the common header to reuse them, otherwise it would end up using 8550.c for all > future SOC. > > Share your comments if you have any better approach to manage/re-use these > platform data considering more SOCs getting added. Right, yes the architecture is fine, but I don't feel iris_platform_common is the right place, perhaps we could introduce a platform_catalog.c where we could place all the common platform data and reuse them from the platform_<soc>.c files ? I can design prototype on top of this patchset as an RFC. Neil > > Regards, > Vikash > >> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c >> index 4f8bce6e2002bffee4c93dcaaf6e52bf4e40992e..7cd8650fbe9c09598670530103e3d5edf32953e7 100644 >> --- a/drivers/media/platform/qcom/iris/iris_probe.c >> +++ b/drivers/media/platform/qcom/iris/iris_probe.c >> @@ -345,6 +345,10 @@ static const struct of_device_id iris_dt_match[] = { >> .data = &sm8250_data, >> }, >> #endif >> + { >> + .compatible = "qcom,sm8650-iris", >> + .data = &sm8650_data, >> + }, >> { }, >> }; >> MODULE_DEVICE_TABLE(of, iris_dt_match); >>
On 4/10/2025 2:31 PM, Neil Armstrong wrote: > On 09/04/2025 18:57, Vikash Garodia wrote: >> Hi Neil, >> >> On 4/9/2025 8:08 PM, Neil Armstrong wrote: >>> Add support for the SM8650 platform by re-using the SM8550 >>> definitions and using the vpu33 ops. >>> >>> The SM8650/vpu33 requires more reset lines, but the H.264 >>> decoder capabilities are identical. >>> >>> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # x1e Dell >>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>> --- >>> .../platform/qcom/iris/iris_platform_common.h | 1 + >>> .../platform/qcom/iris/iris_platform_sm8550.c | 64 ++++++++++++++++++++++ >>> drivers/media/platform/qcom/iris/iris_probe.c | 4 ++ >>> 3 files changed, 69 insertions(+) >>> >>> diff --git a/drivers/media/platform/qcom/iris/iris_platform_common.h >>> b/drivers/media/platform/qcom/iris/iris_platform_common.h >>> index >>> fdd40fd80178c4c66b37e392d07a0a62f492f108..6bc3a7975b04d612f6c89206eae95dac678695fc 100644 >>> --- a/drivers/media/platform/qcom/iris/iris_platform_common.h >>> +++ b/drivers/media/platform/qcom/iris/iris_platform_common.h >>> @@ -35,6 +35,7 @@ enum pipe_type { >>> extern struct iris_platform_data sm8250_data; >>> extern struct iris_platform_data sm8550_data; >>> +extern struct iris_platform_data sm8650_data; >>> enum platform_clk_type { >>> IRIS_AXI_CLK, >>> diff --git a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>> b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>> index >>> 35d278996c430f2856d0fe59586930061a271c3e..d0f8fa960d53367023e41bc5807ba3f8beae2efc 100644 >>> --- a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>> +++ b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>> @@ -144,6 +144,10 @@ static const struct icc_info sm8550_icc_table[] = { >>> static const char * const sm8550_clk_reset_table[] = { "bus" }; >>> +static const char * const sm8650_clk_reset_table[] = { "bus", "core" }; >>> + >>> +static const char * const sm8650_controller_reset_table[] = { "xo" }; >>> + >>> static const struct bw_info sm8550_bw_table_dec[] = { >>> { ((4096 * 2160) / 256) * 60, 1608000 }, >>> { ((4096 * 2160) / 256) * 30, 826000 }, >>> @@ -264,3 +268,63 @@ struct iris_platform_data sm8550_data = { >>> .dec_op_int_buf_tbl = sm8550_dec_op_int_buf_tbl, >>> .dec_op_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_op_int_buf_tbl), >>> }; >>> + >>> +/* >>> + * Shares most of SM8550 data except: >>> + * - vpu_ops to iris_vpu33_ops >>> + * - clk_rst_tbl to sm8650_clk_reset_table >>> + * - controller_rst_tbl to sm8650_controller_reset_table >>> + * - fwname to "qcom/vpu/vpu33_p4.mbn" >>> + */ >>> +struct iris_platform_data sm8650_data = { >>> + .get_instance = iris_hfi_gen2_get_instance, >>> + .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, >>> + .icc_tbl = sm8550_icc_table, >>> + .icc_tbl_size = ARRAY_SIZE(sm8550_icc_table), >>> + .clk_rst_tbl = sm8650_clk_reset_table, >>> + .clk_rst_tbl_size = ARRAY_SIZE(sm8650_clk_reset_table), >>> + .controller_rst_tbl = sm8650_controller_reset_table, >>> + .controller_rst_tbl_size = ARRAY_SIZE(sm8650_controller_reset_table), >>> + .bw_tbl_dec = sm8550_bw_table_dec, >>> + .bw_tbl_dec_size = ARRAY_SIZE(sm8550_bw_table_dec), >>> + .pmdomain_tbl = sm8550_pmdomain_table, >>> + .pmdomain_tbl_size = ARRAY_SIZE(sm8550_pmdomain_table), >>> + .opp_pd_tbl = sm8550_opp_pd_table, >>> + .opp_pd_tbl_size = ARRAY_SIZE(sm8550_opp_pd_table), >>> + .clk_tbl = sm8550_clk_table, >>> + .clk_tbl_size = ARRAY_SIZE(sm8550_clk_table), >>> + /* Upper bound of DMA address range */ >>> + .dma_mask = 0xe0000000 - 1, >>> + .fwname = "qcom/vpu/vpu33_p4.mbn", >>> + .pas_id = IRIS_PAS_ID, >>> + .inst_caps = &platform_inst_cap_sm8550, >>> + .inst_fw_caps = inst_fw_cap_sm8550, >>> + .inst_fw_caps_size = ARRAY_SIZE(inst_fw_cap_sm8550), >>> + .tz_cp_config_data = &tz_cp_config_sm8550, >>> + .core_arch = VIDEO_ARCH_LX, >>> + .hw_response_timeout = HW_RESPONSE_TIMEOUT_VALUE, >>> + .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), >>> + .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_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, >>> + .dec_op_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_op_int_buf_tbl), >>> +}; >> While i was extending the data for QCS8300 (one another iris-v3 variant), i >> realize that this file iris_platform_sm8550.c is getting dumped with all SOC >> platform data. It would be a good idea at this point to split it into something >> like this >> 1. Introduce SOC specific c file and move the respective SOC platform data to >> it, for ex, in this case sm8650_data >> 2. Move the common structs from iris_platform_sm8550.c to >> iris_platform_common.h. This way more SOCs getting added in future, can include >> the common header to reuse them, otherwise it would end up using 8550.c for all >> future SOC. >> >> Share your comments if you have any better approach to manage/re-use these >> platform data considering more SOCs getting added. > > Right, yes the architecture is fine, but I don't feel iris_platform_common is > the right > place, perhaps we could introduce a platform_catalog.c where we could place all > the common > platform data and reuse them from the platform_<soc>.c files ? Common structs would certainly need to be part of a header which can be included. Where do you plan to keep common struct to be used across SOC specific file in your approach ? > > I can design prototype on top of this patchset as an RFC. I was thinking that the changes are not that big, and can be done in existing series though. Thanks, Vikash > > Neil > >> >> Regards, >> Vikash >> >>> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c >>> b/drivers/media/platform/qcom/iris/iris_probe.c >>> index >>> 4f8bce6e2002bffee4c93dcaaf6e52bf4e40992e..7cd8650fbe9c09598670530103e3d5edf32953e7 100644 >>> --- a/drivers/media/platform/qcom/iris/iris_probe.c >>> +++ b/drivers/media/platform/qcom/iris/iris_probe.c >>> @@ -345,6 +345,10 @@ static const struct of_device_id iris_dt_match[] = { >>> .data = &sm8250_data, >>> }, >>> #endif >>> + { >>> + .compatible = "qcom,sm8650-iris", >>> + .data = &sm8650_data, >>> + }, >>> { }, >>> }; >>> MODULE_DEVICE_TABLE(of, iris_dt_match); >>> >
On 4/10/2025 2:43 PM, Vikash Garodia wrote: > > On 4/10/2025 2:31 PM, Neil Armstrong wrote: >> On 09/04/2025 18:57, Vikash Garodia wrote: >>> Hi Neil, >>> >>> On 4/9/2025 8:08 PM, Neil Armstrong wrote: >>>> Add support for the SM8650 platform by re-using the SM8550 >>>> definitions and using the vpu33 ops. >>>> >>>> The SM8650/vpu33 requires more reset lines, but the H.264 >>>> decoder capabilities are identical. >>>> >>>> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # x1e Dell >>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>>> --- >>>> .../platform/qcom/iris/iris_platform_common.h | 1 + >>>> .../platform/qcom/iris/iris_platform_sm8550.c | 64 ++++++++++++++++++++++ >>>> drivers/media/platform/qcom/iris/iris_probe.c | 4 ++ >>>> 3 files changed, 69 insertions(+) >>>> >>>> diff --git a/drivers/media/platform/qcom/iris/iris_platform_common.h >>>> b/drivers/media/platform/qcom/iris/iris_platform_common.h >>>> index >>>> fdd40fd80178c4c66b37e392d07a0a62f492f108..6bc3a7975b04d612f6c89206eae95dac678695fc 100644 >>>> --- a/drivers/media/platform/qcom/iris/iris_platform_common.h >>>> +++ b/drivers/media/platform/qcom/iris/iris_platform_common.h >>>> @@ -35,6 +35,7 @@ enum pipe_type { >>>> extern struct iris_platform_data sm8250_data; >>>> extern struct iris_platform_data sm8550_data; >>>> +extern struct iris_platform_data sm8650_data; >>>> enum platform_clk_type { >>>> IRIS_AXI_CLK, >>>> diff --git a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>> b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>> index >>>> 35d278996c430f2856d0fe59586930061a271c3e..d0f8fa960d53367023e41bc5807ba3f8beae2efc 100644 >>>> --- a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>> +++ b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>> @@ -144,6 +144,10 @@ static const struct icc_info sm8550_icc_table[] = { >>>> static const char * const sm8550_clk_reset_table[] = { "bus" }; >>>> +static const char * const sm8650_clk_reset_table[] = { "bus", "core" }; >>>> + >>>> +static const char * const sm8650_controller_reset_table[] = { "xo" }; >>>> + >>>> static const struct bw_info sm8550_bw_table_dec[] = { >>>> { ((4096 * 2160) / 256) * 60, 1608000 }, >>>> { ((4096 * 2160) / 256) * 30, 826000 }, >>>> @@ -264,3 +268,63 @@ struct iris_platform_data sm8550_data = { >>>> .dec_op_int_buf_tbl = sm8550_dec_op_int_buf_tbl, >>>> .dec_op_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_op_int_buf_tbl), >>>> }; >>>> + >>>> +/* >>>> + * Shares most of SM8550 data except: >>>> + * - vpu_ops to iris_vpu33_ops >>>> + * - clk_rst_tbl to sm8650_clk_reset_table >>>> + * - controller_rst_tbl to sm8650_controller_reset_table >>>> + * - fwname to "qcom/vpu/vpu33_p4.mbn" >>>> + */ >>>> +struct iris_platform_data sm8650_data = { >>>> + .get_instance = iris_hfi_gen2_get_instance, >>>> + .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, >>>> + .icc_tbl = sm8550_icc_table, >>>> + .icc_tbl_size = ARRAY_SIZE(sm8550_icc_table), >>>> + .clk_rst_tbl = sm8650_clk_reset_table, >>>> + .clk_rst_tbl_size = ARRAY_SIZE(sm8650_clk_reset_table), >>>> + .controller_rst_tbl = sm8650_controller_reset_table, >>>> + .controller_rst_tbl_size = ARRAY_SIZE(sm8650_controller_reset_table), >>>> + .bw_tbl_dec = sm8550_bw_table_dec, >>>> + .bw_tbl_dec_size = ARRAY_SIZE(sm8550_bw_table_dec), >>>> + .pmdomain_tbl = sm8550_pmdomain_table, >>>> + .pmdomain_tbl_size = ARRAY_SIZE(sm8550_pmdomain_table), >>>> + .opp_pd_tbl = sm8550_opp_pd_table, >>>> + .opp_pd_tbl_size = ARRAY_SIZE(sm8550_opp_pd_table), >>>> + .clk_tbl = sm8550_clk_table, >>>> + .clk_tbl_size = ARRAY_SIZE(sm8550_clk_table), >>>> + /* Upper bound of DMA address range */ >>>> + .dma_mask = 0xe0000000 - 1, >>>> + .fwname = "qcom/vpu/vpu33_p4.mbn", >>>> + .pas_id = IRIS_PAS_ID, >>>> + .inst_caps = &platform_inst_cap_sm8550, >>>> + .inst_fw_caps = inst_fw_cap_sm8550, >>>> + .inst_fw_caps_size = ARRAY_SIZE(inst_fw_cap_sm8550), >>>> + .tz_cp_config_data = &tz_cp_config_sm8550, >>>> + .core_arch = VIDEO_ARCH_LX, >>>> + .hw_response_timeout = HW_RESPONSE_TIMEOUT_VALUE, >>>> + .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), >>>> + .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_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, >>>> + .dec_op_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_op_int_buf_tbl), >>>> +}; >>> While i was extending the data for QCS8300 (one another iris-v3 variant), i >>> realize that this file iris_platform_sm8550.c is getting dumped with all SOC >>> platform data. It would be a good idea at this point to split it into something >>> like this >>> 1. Introduce SOC specific c file and move the respective SOC platform data to >>> it, for ex, in this case sm8650_data >>> 2. Move the common structs from iris_platform_sm8550.c to >>> iris_platform_common.h. This way more SOCs getting added in future, can include >>> the common header to reuse them, otherwise it would end up using 8550.c for all >>> future SOC. >>> >>> Share your comments if you have any better approach to manage/re-use these >>> platform data considering more SOCs getting added. >> >> Right, yes the architecture is fine, but I don't feel iris_platform_common is >> the right >> place, perhaps we could introduce a platform_catalog.c where we could place all >> the common >> platform data and reuse them from the platform_<soc>.c files ? > Common structs would certainly need to be part of a header which can be > included. Where do you plan to keep common struct to be used across SOC specific > file in your approach ? >> >> I can design prototype on top of this patchset as an RFC. > I was thinking that the changes are not that big, and can be done in existing > series though. > For now, I think you can introduce a platform_sm8650.c as part of this series and use the common structure from platform_sm8550.c. Shouldn't be a big change. Later you can post a separate patch series to add platform_catalog.c and have common struct placed there which can be used across different SOC platform files. or other way is, Post a patch series to introduce platform_catalog.c with common struct and then rebase your 8650 series on top of it. Thanks, Dikshita > Thanks, > Vikash >> >> Neil >> >>> >>> Regards, >>> Vikash >>> >>>> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c >>>> b/drivers/media/platform/qcom/iris/iris_probe.c >>>> index >>>> 4f8bce6e2002bffee4c93dcaaf6e52bf4e40992e..7cd8650fbe9c09598670530103e3d5edf32953e7 100644 >>>> --- a/drivers/media/platform/qcom/iris/iris_probe.c >>>> +++ b/drivers/media/platform/qcom/iris/iris_probe.c >>>> @@ -345,6 +345,10 @@ static const struct of_device_id iris_dt_match[] = { >>>> .data = &sm8250_data, >>>> }, >>>> #endif >>>> + { >>>> + .compatible = "qcom,sm8650-iris", >>>> + .data = &sm8650_data, >>>> + }, >>>> { }, >>>> }; >>>> MODULE_DEVICE_TABLE(of, iris_dt_match); >>>> >>
On 10/04/2025 15:07, Dikshita Agarwal wrote: > > > On 4/10/2025 2:43 PM, Vikash Garodia wrote: >> >> On 4/10/2025 2:31 PM, Neil Armstrong wrote: >>> On 09/04/2025 18:57, Vikash Garodia wrote: >>>> Hi Neil, >>>> >>>> On 4/9/2025 8:08 PM, Neil Armstrong wrote: >>>>> Add support for the SM8650 platform by re-using the SM8550 >>>>> definitions and using the vpu33 ops. >>>>> >>>>> The SM8650/vpu33 requires more reset lines, but the H.264 >>>>> decoder capabilities are identical. >>>>> >>>>> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # x1e Dell >>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>>>> --- >>>>> .../platform/qcom/iris/iris_platform_common.h | 1 + >>>>> .../platform/qcom/iris/iris_platform_sm8550.c | 64 ++++++++++++++++++++++ >>>>> drivers/media/platform/qcom/iris/iris_probe.c | 4 ++ >>>>> 3 files changed, 69 insertions(+) >>>>> >>>>> diff --git a/drivers/media/platform/qcom/iris/iris_platform_common.h >>>>> b/drivers/media/platform/qcom/iris/iris_platform_common.h >>>>> index >>>>> fdd40fd80178c4c66b37e392d07a0a62f492f108..6bc3a7975b04d612f6c89206eae95dac678695fc 100644 >>>>> --- a/drivers/media/platform/qcom/iris/iris_platform_common.h >>>>> +++ b/drivers/media/platform/qcom/iris/iris_platform_common.h >>>>> @@ -35,6 +35,7 @@ enum pipe_type { >>>>> extern struct iris_platform_data sm8250_data; >>>>> extern struct iris_platform_data sm8550_data; >>>>> +extern struct iris_platform_data sm8650_data; >>>>> enum platform_clk_type { >>>>> IRIS_AXI_CLK, >>>>> diff --git a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>>> b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>>> index >>>>> 35d278996c430f2856d0fe59586930061a271c3e..d0f8fa960d53367023e41bc5807ba3f8beae2efc 100644 >>>>> --- a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>>> +++ b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>>> @@ -144,6 +144,10 @@ static const struct icc_info sm8550_icc_table[] = { >>>>> static const char * const sm8550_clk_reset_table[] = { "bus" }; >>>>> +static const char * const sm8650_clk_reset_table[] = { "bus", "core" }; >>>>> + >>>>> +static const char * const sm8650_controller_reset_table[] = { "xo" }; >>>>> + >>>>> static const struct bw_info sm8550_bw_table_dec[] = { >>>>> { ((4096 * 2160) / 256) * 60, 1608000 }, >>>>> { ((4096 * 2160) / 256) * 30, 826000 }, >>>>> @@ -264,3 +268,63 @@ struct iris_platform_data sm8550_data = { >>>>> .dec_op_int_buf_tbl = sm8550_dec_op_int_buf_tbl, >>>>> .dec_op_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_op_int_buf_tbl), >>>>> }; >>>>> + >>>>> +/* >>>>> + * Shares most of SM8550 data except: >>>>> + * - vpu_ops to iris_vpu33_ops >>>>> + * - clk_rst_tbl to sm8650_clk_reset_table >>>>> + * - controller_rst_tbl to sm8650_controller_reset_table >>>>> + * - fwname to "qcom/vpu/vpu33_p4.mbn" >>>>> + */ >>>>> +struct iris_platform_data sm8650_data = { >>>>> + .get_instance = iris_hfi_gen2_get_instance, >>>>> + .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, >>>>> + .icc_tbl = sm8550_icc_table, >>>>> + .icc_tbl_size = ARRAY_SIZE(sm8550_icc_table), >>>>> + .clk_rst_tbl = sm8650_clk_reset_table, >>>>> + .clk_rst_tbl_size = ARRAY_SIZE(sm8650_clk_reset_table), >>>>> + .controller_rst_tbl = sm8650_controller_reset_table, >>>>> + .controller_rst_tbl_size = ARRAY_SIZE(sm8650_controller_reset_table), >>>>> + .bw_tbl_dec = sm8550_bw_table_dec, >>>>> + .bw_tbl_dec_size = ARRAY_SIZE(sm8550_bw_table_dec), >>>>> + .pmdomain_tbl = sm8550_pmdomain_table, >>>>> + .pmdomain_tbl_size = ARRAY_SIZE(sm8550_pmdomain_table), >>>>> + .opp_pd_tbl = sm8550_opp_pd_table, >>>>> + .opp_pd_tbl_size = ARRAY_SIZE(sm8550_opp_pd_table), >>>>> + .clk_tbl = sm8550_clk_table, >>>>> + .clk_tbl_size = ARRAY_SIZE(sm8550_clk_table), >>>>> + /* Upper bound of DMA address range */ >>>>> + .dma_mask = 0xe0000000 - 1, >>>>> + .fwname = "qcom/vpu/vpu33_p4.mbn", >>>>> + .pas_id = IRIS_PAS_ID, >>>>> + .inst_caps = &platform_inst_cap_sm8550, >>>>> + .inst_fw_caps = inst_fw_cap_sm8550, >>>>> + .inst_fw_caps_size = ARRAY_SIZE(inst_fw_cap_sm8550), >>>>> + .tz_cp_config_data = &tz_cp_config_sm8550, >>>>> + .core_arch = VIDEO_ARCH_LX, >>>>> + .hw_response_timeout = HW_RESPONSE_TIMEOUT_VALUE, >>>>> + .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), >>>>> + .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_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, >>>>> + .dec_op_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_op_int_buf_tbl), >>>>> +}; >>>> While i was extending the data for QCS8300 (one another iris-v3 variant), i >>>> realize that this file iris_platform_sm8550.c is getting dumped with all SOC >>>> platform data. It would be a good idea at this point to split it into something >>>> like this >>>> 1. Introduce SOC specific c file and move the respective SOC platform data to >>>> it, for ex, in this case sm8650_data >>>> 2. Move the common structs from iris_platform_sm8550.c to >>>> iris_platform_common.h. This way more SOCs getting added in future, can include >>>> the common header to reuse them, otherwise it would end up using 8550.c for all >>>> future SOC. >>>> >>>> Share your comments if you have any better approach to manage/re-use these >>>> platform data considering more SOCs getting added. >>> >>> Right, yes the architecture is fine, but I don't feel iris_platform_common is >>> the right >>> place, perhaps we could introduce a platform_catalog.c where we could place all >>> the common >>> platform data and reuse them from the platform_<soc>.c files ? >> Common structs would certainly need to be part of a header which can be >> included. Where do you plan to keep common struct to be used across SOC specific >> file in your approach ? >>> >>> I can design prototype on top of this patchset as an RFC. >> I was thinking that the changes are not that big, and can be done in existing >> series though. >> > For now, I think you can introduce a platform_sm8650.c as part of this > series and use the common structure from platform_sm8550.c. > Shouldn't be a big change. I tried but I don't how to solve this, you need a build-time ARRAY_SIZE for the arrays, so they need to be in the same .c file to allow a build-time evaluation of ARRAY_SIZE. If he common tables are moved into a header they will be duplicated into both platform_sm8650 & platform_sm8550 objects which is not what we want. The solution would be to write all the platform tables & iris_platform_data into headers, with common headers, then include those into a platform_catalog.c like is done for the drm/msm/dpu1 catalog. Neil > > Later you can post a separate patch series to add platform_catalog.c and > have common struct placed there which can be used across different SOC > platform files. > > or other way is, > Post a patch series to introduce platform_catalog.c with common struct and > then rebase your 8650 series on top of it. > > Thanks, > Dikshita >> Thanks, >> Vikash >>> >>> Neil >>> >>>> >>>> Regards, >>>> Vikash >>>> >>>>> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c >>>>> b/drivers/media/platform/qcom/iris/iris_probe.c >>>>> index >>>>> 4f8bce6e2002bffee4c93dcaaf6e52bf4e40992e..7cd8650fbe9c09598670530103e3d5edf32953e7 100644 >>>>> --- a/drivers/media/platform/qcom/iris/iris_probe.c >>>>> +++ b/drivers/media/platform/qcom/iris/iris_probe.c >>>>> @@ -345,6 +345,10 @@ static const struct of_device_id iris_dt_match[] = { >>>>> .data = &sm8250_data, >>>>> }, >>>>> #endif >>>>> + { >>>>> + .compatible = "qcom,sm8650-iris", >>>>> + .data = &sm8650_data, >>>>> + }, >>>>> { }, >>>>> }; >>>>> MODULE_DEVICE_TABLE(of, iris_dt_match); >>>>> >>>
On 10/04/2025 16:40, neil.armstrong@linaro.org wrote: > On 10/04/2025 15:07, Dikshita Agarwal wrote: >> >> >> On 4/10/2025 2:43 PM, Vikash Garodia wrote: >>> >>> On 4/10/2025 2:31 PM, Neil Armstrong wrote: >>>> On 09/04/2025 18:57, Vikash Garodia wrote: >>>>> Hi Neil, >>>>> >>>>> On 4/9/2025 8:08 PM, Neil Armstrong wrote: >>>>>> Add support for the SM8650 platform by re-using the SM8550 >>>>>> definitions and using the vpu33 ops. >>>>>> >>>>>> The SM8650/vpu33 requires more reset lines, but the H.264 >>>>>> decoder capabilities are identical. >>>>>> >>>>>> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # x1e Dell >>>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>>>>> --- >>>>>> .../platform/qcom/iris/iris_platform_common.h | 1 + >>>>>> .../platform/qcom/iris/iris_platform_sm8550.c | 64 +++++++ >>>>>> +++++++++++++++ >>>>>> drivers/media/platform/qcom/iris/iris_probe.c | 4 ++ >>>>>> 3 files changed, 69 insertions(+) >>>>>> >>>>>> diff --git a/drivers/media/platform/qcom/iris/iris_platform_common.h >>>>>> b/drivers/media/platform/qcom/iris/iris_platform_common.h >>>>>> index >>>>>> fdd40fd80178c4c66b37e392d07a0a62f492f108..6bc3a7975b04d612f6c89206eae95dac678695fc 100644 >>>>>> --- a/drivers/media/platform/qcom/iris/iris_platform_common.h >>>>>> +++ b/drivers/media/platform/qcom/iris/iris_platform_common.h >>>>>> @@ -35,6 +35,7 @@ enum pipe_type { >>>>>> extern struct iris_platform_data sm8250_data; >>>>>> extern struct iris_platform_data sm8550_data; >>>>>> +extern struct iris_platform_data sm8650_data; >>>>>> enum platform_clk_type { >>>>>> IRIS_AXI_CLK, >>>>>> diff --git a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>>>> b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>>>> index >>>>>> 35d278996c430f2856d0fe59586930061a271c3e..d0f8fa960d53367023e41bc5807ba3f8beae2efc 100644 >>>>>> --- a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>>>> +++ b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>>>> @@ -144,6 +144,10 @@ static const struct icc_info >>>>>> sm8550_icc_table[] = { >>>>>> static const char * const sm8550_clk_reset_table[] = { "bus" }; >>>>>> +static const char * const sm8650_clk_reset_table[] = { "bus", >>>>>> "core" }; >>>>>> + >>>>>> +static const char * const sm8650_controller_reset_table[] = >>>>>> { "xo" }; >>>>>> + >>>>>> static const struct bw_info sm8550_bw_table_dec[] = { >>>>>> { ((4096 * 2160) / 256) * 60, 1608000 }, >>>>>> { ((4096 * 2160) / 256) * 30, 826000 }, >>>>>> @@ -264,3 +268,63 @@ struct iris_platform_data sm8550_data = { >>>>>> .dec_op_int_buf_tbl = sm8550_dec_op_int_buf_tbl, >>>>>> .dec_op_int_buf_tbl_size = >>>>>> ARRAY_SIZE(sm8550_dec_op_int_buf_tbl), >>>>>> }; >>>>>> + >>>>>> +/* >>>>>> + * Shares most of SM8550 data except: >>>>>> + * - vpu_ops to iris_vpu33_ops >>>>>> + * - clk_rst_tbl to sm8650_clk_reset_table >>>>>> + * - controller_rst_tbl to sm8650_controller_reset_table >>>>>> + * - fwname to "qcom/vpu/vpu33_p4.mbn" >>>>>> + */ >>>>>> +struct iris_platform_data sm8650_data = { >>>>>> + .get_instance = iris_hfi_gen2_get_instance, >>>>>> + .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, >>>>>> + .icc_tbl = sm8550_icc_table, >>>>>> + .icc_tbl_size = ARRAY_SIZE(sm8550_icc_table), >>>>>> + .clk_rst_tbl = sm8650_clk_reset_table, >>>>>> + .clk_rst_tbl_size = ARRAY_SIZE(sm8650_clk_reset_table), >>>>>> + .controller_rst_tbl = sm8650_controller_reset_table, >>>>>> + .controller_rst_tbl_size = >>>>>> ARRAY_SIZE(sm8650_controller_reset_table), >>>>>> + .bw_tbl_dec = sm8550_bw_table_dec, >>>>>> + .bw_tbl_dec_size = ARRAY_SIZE(sm8550_bw_table_dec), >>>>>> + .pmdomain_tbl = sm8550_pmdomain_table, >>>>>> + .pmdomain_tbl_size = ARRAY_SIZE(sm8550_pmdomain_table), >>>>>> + .opp_pd_tbl = sm8550_opp_pd_table, >>>>>> + .opp_pd_tbl_size = ARRAY_SIZE(sm8550_opp_pd_table), >>>>>> + .clk_tbl = sm8550_clk_table, >>>>>> + .clk_tbl_size = ARRAY_SIZE(sm8550_clk_table), >>>>>> + /* Upper bound of DMA address range */ >>>>>> + .dma_mask = 0xe0000000 - 1, >>>>>> + .fwname = "qcom/vpu/vpu33_p4.mbn", >>>>>> + .pas_id = IRIS_PAS_ID, >>>>>> + .inst_caps = &platform_inst_cap_sm8550, >>>>>> + .inst_fw_caps = inst_fw_cap_sm8550, >>>>>> + .inst_fw_caps_size = ARRAY_SIZE(inst_fw_cap_sm8550), >>>>>> + .tz_cp_config_data = &tz_cp_config_sm8550, >>>>>> + .core_arch = VIDEO_ARCH_LX, >>>>>> + .hw_response_timeout = HW_RESPONSE_TIMEOUT_VALUE, >>>>>> + .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), >>>>>> + .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_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, >>>>>> + .dec_op_int_buf_tbl_size = >>>>>> ARRAY_SIZE(sm8550_dec_op_int_buf_tbl), >>>>>> +}; >>>>> While i was extending the data for QCS8300 (one another iris-v3 >>>>> variant), i >>>>> realize that this file iris_platform_sm8550.c is getting dumped >>>>> with all SOC >>>>> platform data. It would be a good idea at this point to split it >>>>> into something >>>>> like this >>>>> 1. Introduce SOC specific c file and move the respective SOC >>>>> platform data to >>>>> it, for ex, in this case sm8650_data >>>>> 2. Move the common structs from iris_platform_sm8550.c to >>>>> iris_platform_common.h. This way more SOCs getting added in future, >>>>> can include >>>>> the common header to reuse them, otherwise it would end up using >>>>> 8550.c for all >>>>> future SOC. >>>>> >>>>> Share your comments if you have any better approach to manage/re- >>>>> use these >>>>> platform data considering more SOCs getting added. >>>> >>>> Right, yes the architecture is fine, but I don't feel >>>> iris_platform_common is >>>> the right >>>> place, perhaps we could introduce a platform_catalog.c where we >>>> could place all >>>> the common >>>> platform data and reuse them from the platform_<soc>.c files ? >>> Common structs would certainly need to be part of a header which can be >>> included. Where do you plan to keep common struct to be used across >>> SOC specific >>> file in your approach ? >>>> >>>> I can design prototype on top of this patchset as an RFC. >>> I was thinking that the changes are not that big, and can be done in >>> existing >>> series though. >>> >> For now, I think you can introduce a platform_sm8650.c as part of this >> series and use the common structure from platform_sm8550.c. >> Shouldn't be a big change. > > I tried but I don't how to solve this, you need a build-time ARRAY_SIZE for > the arrays, so they need to be in the same .c file to allow a build-time > evaluation of ARRAY_SIZE. If he common tables are moved into a header > they will be duplicated into both platform_sm8650 & platform_sm8550 objects > which is not what we want. > > The solution would be to write all the platform tables & iris_platform_data > into headers, with common headers, then include those into a > platform_catalog.c > like is done for the drm/msm/dpu1 catalog. > > Neil > >> >> Later you can post a separate patch series to add platform_catalog.c and >> have common struct placed there which can be used across different SOC >> platform files. >> >> or other way is, >> Post a patch series to introduce platform_catalog.c with common struct >> and >> then rebase your 8650 series on top of it. >> >> Thanks, >> Dikshita >>> Thanks, >>> Vikash >>>> >>>> Neil >>>> >>>>> >>>>> Regards, >>>>> Vikash >>>>> >>>>>> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c >>>>>> b/drivers/media/platform/qcom/iris/iris_probe.c >>>>>> index >>>>>> 4f8bce6e2002bffee4c93dcaaf6e52bf4e40992e..7cd8650fbe9c09598670530103e3d5edf32953e7 100644 >>>>>> --- a/drivers/media/platform/qcom/iris/iris_probe.c >>>>>> +++ b/drivers/media/platform/qcom/iris/iris_probe.c >>>>>> @@ -345,6 +345,10 @@ static const struct of_device_id >>>>>> iris_dt_match[] = { >>>>>> .data = &sm8250_data, >>>>>> }, >>>>>> #endif >>>>>> + { >>>>>> + .compatible = "qcom,sm8650-iris", >>>>>> + .data = &sm8650_data, >>>>>> + }, >>>>>> { }, >>>>>> }; >>>>>> MODULE_DEVICE_TABLE(of, iris_dt_match); >>>>>> >>>> > Eh as I read the suggestion about putting the structs - instantiating the structs in the header, I wondered if that would link but anyway. One way to solve this without going the dpu1 route right now is to rename the platform files iris_platform_sm8250.c -> iris_platform_common_hfi_gen1.c iris_platform_sm8550.c -> iris_platform_common_hfi_gen2.c The differentiation around HFI into "generations" instead of incrementing the already existing HFIXXX version is unfortunate. At least this way we have a repository of common HFI code located in each file, in expectation of HFI GEN3 whenever. --- bod
On 10/04/2025 18:06, Bryan O'Donoghue wrote: > On 10/04/2025 16:40, neil.armstrong@linaro.org wrote: >> On 10/04/2025 15:07, Dikshita Agarwal wrote: >>> >>> >>> On 4/10/2025 2:43 PM, Vikash Garodia wrote: >>>> >>>> On 4/10/2025 2:31 PM, Neil Armstrong wrote: >>>>> On 09/04/2025 18:57, Vikash Garodia wrote: >>>>>> Hi Neil, >>>>>> >>>>>> On 4/9/2025 8:08 PM, Neil Armstrong wrote: >>>>>>> Add support for the SM8650 platform by re-using the SM8550 >>>>>>> definitions and using the vpu33 ops. >>>>>>> >>>>>>> The SM8650/vpu33 requires more reset lines, but the H.264 >>>>>>> decoder capabilities are identical. >>>>>>> >>>>>>> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # x1e Dell >>>>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>>>>>> --- >>>>>>> .../platform/qcom/iris/iris_platform_common.h | 1 + >>>>>>> .../platform/qcom/iris/iris_platform_sm8550.c | 64 +++++++ +++++++++++++++ >>>>>>> drivers/media/platform/qcom/iris/iris_probe.c | 4 ++ >>>>>>> 3 files changed, 69 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/media/platform/qcom/iris/iris_platform_common.h >>>>>>> b/drivers/media/platform/qcom/iris/iris_platform_common.h >>>>>>> index >>>>>>> fdd40fd80178c4c66b37e392d07a0a62f492f108..6bc3a7975b04d612f6c89206eae95dac678695fc 100644 >>>>>>> --- a/drivers/media/platform/qcom/iris/iris_platform_common.h >>>>>>> +++ b/drivers/media/platform/qcom/iris/iris_platform_common.h >>>>>>> @@ -35,6 +35,7 @@ enum pipe_type { >>>>>>> extern struct iris_platform_data sm8250_data; >>>>>>> extern struct iris_platform_data sm8550_data; >>>>>>> +extern struct iris_platform_data sm8650_data; >>>>>>> enum platform_clk_type { >>>>>>> IRIS_AXI_CLK, >>>>>>> diff --git a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>>>>> b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>>>>> index >>>>>>> 35d278996c430f2856d0fe59586930061a271c3e..d0f8fa960d53367023e41bc5807ba3f8beae2efc 100644 >>>>>>> --- a/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>>>>> +++ b/drivers/media/platform/qcom/iris/iris_platform_sm8550.c >>>>>>> @@ -144,6 +144,10 @@ static const struct icc_info sm8550_icc_table[] = { >>>>>>> static const char * const sm8550_clk_reset_table[] = { "bus" }; >>>>>>> +static const char * const sm8650_clk_reset_table[] = { "bus", "core" }; >>>>>>> + >>>>>>> +static const char * const sm8650_controller_reset_table[] = { "xo" }; >>>>>>> + >>>>>>> static const struct bw_info sm8550_bw_table_dec[] = { >>>>>>> { ((4096 * 2160) / 256) * 60, 1608000 }, >>>>>>> { ((4096 * 2160) / 256) * 30, 826000 }, >>>>>>> @@ -264,3 +268,63 @@ struct iris_platform_data sm8550_data = { >>>>>>> .dec_op_int_buf_tbl = sm8550_dec_op_int_buf_tbl, >>>>>>> .dec_op_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_op_int_buf_tbl), >>>>>>> }; >>>>>>> + >>>>>>> +/* >>>>>>> + * Shares most of SM8550 data except: >>>>>>> + * - vpu_ops to iris_vpu33_ops >>>>>>> + * - clk_rst_tbl to sm8650_clk_reset_table >>>>>>> + * - controller_rst_tbl to sm8650_controller_reset_table >>>>>>> + * - fwname to "qcom/vpu/vpu33_p4.mbn" >>>>>>> + */ >>>>>>> +struct iris_platform_data sm8650_data = { >>>>>>> + .get_instance = iris_hfi_gen2_get_instance, >>>>>>> + .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, >>>>>>> + .icc_tbl = sm8550_icc_table, >>>>>>> + .icc_tbl_size = ARRAY_SIZE(sm8550_icc_table), >>>>>>> + .clk_rst_tbl = sm8650_clk_reset_table, >>>>>>> + .clk_rst_tbl_size = ARRAY_SIZE(sm8650_clk_reset_table), >>>>>>> + .controller_rst_tbl = sm8650_controller_reset_table, >>>>>>> + .controller_rst_tbl_size = ARRAY_SIZE(sm8650_controller_reset_table), >>>>>>> + .bw_tbl_dec = sm8550_bw_table_dec, >>>>>>> + .bw_tbl_dec_size = ARRAY_SIZE(sm8550_bw_table_dec), >>>>>>> + .pmdomain_tbl = sm8550_pmdomain_table, >>>>>>> + .pmdomain_tbl_size = ARRAY_SIZE(sm8550_pmdomain_table), >>>>>>> + .opp_pd_tbl = sm8550_opp_pd_table, >>>>>>> + .opp_pd_tbl_size = ARRAY_SIZE(sm8550_opp_pd_table), >>>>>>> + .clk_tbl = sm8550_clk_table, >>>>>>> + .clk_tbl_size = ARRAY_SIZE(sm8550_clk_table), >>>>>>> + /* Upper bound of DMA address range */ >>>>>>> + .dma_mask = 0xe0000000 - 1, >>>>>>> + .fwname = "qcom/vpu/vpu33_p4.mbn", >>>>>>> + .pas_id = IRIS_PAS_ID, >>>>>>> + .inst_caps = &platform_inst_cap_sm8550, >>>>>>> + .inst_fw_caps = inst_fw_cap_sm8550, >>>>>>> + .inst_fw_caps_size = ARRAY_SIZE(inst_fw_cap_sm8550), >>>>>>> + .tz_cp_config_data = &tz_cp_config_sm8550, >>>>>>> + .core_arch = VIDEO_ARCH_LX, >>>>>>> + .hw_response_timeout = HW_RESPONSE_TIMEOUT_VALUE, >>>>>>> + .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), >>>>>>> + .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_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, >>>>>>> + .dec_op_int_buf_tbl_size = ARRAY_SIZE(sm8550_dec_op_int_buf_tbl), >>>>>>> +}; >>>>>> While i was extending the data for QCS8300 (one another iris-v3 variant), i >>>>>> realize that this file iris_platform_sm8550.c is getting dumped with all SOC >>>>>> platform data. It would be a good idea at this point to split it into something >>>>>> like this >>>>>> 1. Introduce SOC specific c file and move the respective SOC platform data to >>>>>> it, for ex, in this case sm8650_data >>>>>> 2. Move the common structs from iris_platform_sm8550.c to >>>>>> iris_platform_common.h. This way more SOCs getting added in future, can include >>>>>> the common header to reuse them, otherwise it would end up using 8550.c for all >>>>>> future SOC. >>>>>> >>>>>> Share your comments if you have any better approach to manage/re- use these >>>>>> platform data considering more SOCs getting added. >>>>> >>>>> Right, yes the architecture is fine, but I don't feel iris_platform_common is >>>>> the right >>>>> place, perhaps we could introduce a platform_catalog.c where we could place all >>>>> the common >>>>> platform data and reuse them from the platform_<soc>.c files ? >>>> Common structs would certainly need to be part of a header which can be >>>> included. Where do you plan to keep common struct to be used across SOC specific >>>> file in your approach ? >>>>> >>>>> I can design prototype on top of this patchset as an RFC. >>>> I was thinking that the changes are not that big, and can be done in existing >>>> series though. >>>> >>> For now, I think you can introduce a platform_sm8650.c as part of this >>> series and use the common structure from platform_sm8550.c. >>> Shouldn't be a big change. >> >> I tried but I don't how to solve this, you need a build-time ARRAY_SIZE for >> the arrays, so they need to be in the same .c file to allow a build-time >> evaluation of ARRAY_SIZE. If he common tables are moved into a header >> they will be duplicated into both platform_sm8650 & platform_sm8550 objects >> which is not what we want. >> >> The solution would be to write all the platform tables & iris_platform_data >> into headers, with common headers, then include those into a platform_catalog.c >> like is done for the drm/msm/dpu1 catalog. >> >> Neil >> >>> >>> Later you can post a separate patch series to add platform_catalog.c and >>> have common struct placed there which can be used across different SOC >>> platform files. >>> >>> or other way is, >>> Post a patch series to introduce platform_catalog.c with common struct and >>> then rebase your 8650 series on top of it. >>> >>> Thanks, >>> Dikshita >>>> Thanks, >>>> Vikash >>>>> >>>>> Neil >>>>> >>>>>> >>>>>> Regards, >>>>>> Vikash >>>>>> >>>>>>> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c >>>>>>> b/drivers/media/platform/qcom/iris/iris_probe.c >>>>>>> index >>>>>>> 4f8bce6e2002bffee4c93dcaaf6e52bf4e40992e..7cd8650fbe9c09598670530103e3d5edf32953e7 100644 >>>>>>> --- a/drivers/media/platform/qcom/iris/iris_probe.c >>>>>>> +++ b/drivers/media/platform/qcom/iris/iris_probe.c >>>>>>> @@ -345,6 +345,10 @@ static const struct of_device_id iris_dt_match[] = { >>>>>>> .data = &sm8250_data, >>>>>>> }, >>>>>>> #endif >>>>>>> + { >>>>>>> + .compatible = "qcom,sm8650-iris", >>>>>>> + .data = &sm8650_data, >>>>>>> + }, >>>>>>> { }, >>>>>>> }; >>>>>>> MODULE_DEVICE_TABLE(of, iris_dt_match); >>>>>>> >>>>> >> > > Eh as I read the suggestion about putting the structs - instantiating the structs in the header, I wondered if that would link but anyway. > > One way to solve this without going the dpu1 route right now is to rename the platform files > > iris_platform_sm8250.c -> iris_platform_common_hfi_gen1.c > iris_platform_sm8550.c -> iris_platform_common_hfi_gen2.c > > The differentiation around HFI into "generations" instead of incrementing the already existing HFIXXX version is unfortunate. > > At least this way we have a repository of common HFI code located in each file, in expectation of HFI GEN3 whenever. Yeah I somehow did something like that, please review: https://lore.kernel.org/all/20250410-topic-sm8x50-upstream-iris-catalog-v5-0-44a431574c25@linaro.org/ Neil > > --- > bod
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. Based on the downstream implementation at: - https://git.codelinaro.org/clo/la/platform/vendor/opensource/video-driver/ branch video-kernel.lnx.4.0.r4-rel Signed-off-by: Neil Armstrong <neil.armstrong@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 (6): 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 | 2 +- drivers/media/platform/qcom/iris/iris_core.h | 1 + .../platform/qcom/iris/iris_platform_common.h | 3 + .../platform/qcom/iris/iris_platform_sm8550.c | 64 +++++ 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 + 11 files changed, 409 insertions(+), 142 deletions(-) --- base-commit: 0d6ed9e013fcc33da9676ed870454d2a014a5aee change-id: 20250225-topic-sm8x50-iris-v10-a219b8a8b477 Best regards,