Re: [PATCH v4 7/8] media: qcom: iris: split firmware_data from raw platform data

[Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>]

On 3/13/2026 9:00 AM, Dmitry Baryshkov wrote:
> Having firmware-related fields in platform data results in the tying
> platform data to the HFI firmware data rather than the actual hardware.
> For example, SM8450 uses Gen2 firmware, so currently its platform data
> should be placed next to the other gen2 platforms, although it has the
> VPU2.0 core, similar to the one found on SM8250 and SC7280 and so the
> hardware-specific platform data is also close to those devices.
> 
> Split firmware data to a separate struct, separating hardware-related
> data from the firmware interfaces.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> ---
>  drivers/media/platform/qcom/iris/iris_buffer.c     |  84 +++----
>  drivers/media/platform/qcom/iris/iris_core.h       |   1 +
>  drivers/media/platform/qcom/iris/iris_ctrls.c      |   8 +-
>  .../platform/qcom/iris/iris_hfi_gen1_command.c     |  10 +-
>  .../platform/qcom/iris/iris_hfi_gen2_command.c     |  66 ++---
>  .../platform/qcom/iris/iris_platform_common.h      |  79 +++---
>  .../media/platform/qcom/iris/iris_platform_gen1.c  |  68 +++---
>  .../media/platform/qcom/iris/iris_platform_gen2.c  | 268 +++++++--------------
>  drivers/media/platform/qcom/iris/iris_probe.c      |   3 +-
>  drivers/media/platform/qcom/iris/iris_vidc.c       |  10 +-
>  10 files changed, 246 insertions(+), 351 deletions(-)
> 

<snip>

> diff --git a/drivers/media/platform/qcom/iris/iris_platform_common.h b/drivers/media/platform/qcom/iris/iris_platform_common.h
> index d1daef2d874b..1a870fec4f31 100644
> --- a/drivers/media/platform/qcom/iris/iris_platform_common.h
> +++ b/drivers/media/platform/qcom/iris/iris_platform_common.h
> @@ -201,45 +201,16 @@ enum platform_pm_domain_type {
>  	IRIS_APV_HW_POWER_DOMAIN,
>  };
>  
> -struct iris_platform_data {
> +struct iris_firmware_data {
>  	void (*init_hfi_ops)(struct iris_core *core);
> +
>  	u32 (*get_vpu_buffer_size)(struct iris_inst *inst, enum iris_buffer_type buffer_type);

I still don't think it's right to keep vpu_buffer_size in firmware data as
this would change mostly for every new VPU variant.

The buffer sizing logic depends on VPU generation (vpu2, vpu3, vpu33,
vpu35) / SoC constraints, not on whether the HFI is Gen1 vs Gen2.

<snip>

> diff --git a/drivers/media/platform/qcom/iris/iris_platform_gen2.c b/drivers/media/platform/qcom/iris/iris_platform_gen2.c
> index 10a972f96cbe..a83f6910f8b7 100644
> --- a/drivers/media/platform/qcom/iris/iris_platform_gen2.c
> +++ b/drivers/media/platform/qcom/iris/iris_platform_gen2.c
> @@ -906,41 +906,15 @@ static const u32 sm8550_enc_op_int_buf_tbl[] = {
>  	BUF_SCRATCH_2,
>  };
>  
> -const struct iris_platform_data sm8550_data = {
> +const struct iris_firmware_data iris_hfi_gen2_data = {
>  	.init_hfi_ops = iris_hfi_gen2_sys_ops_init,
>  	.get_vpu_buffer_size = iris_vpu_buf_size,
> -	.vpu_ops = &iris_vpu3_ops,
> -	.icc_tbl = sm8550_icc_table,
> -	.icc_tbl_size = ARRAY_SIZE(sm8550_icc_table),
> -	.clk_rst_tbl = sm8550_clk_reset_table,
> -	.clk_rst_tbl_size = ARRAY_SIZE(sm8550_clk_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),
> -	.opp_clk_tbl = sm8550_opp_clk_table,
> -	/* Upper bound of DMA address range */
> -	.dma_mask = 0xe0000000 - 1,
> -	.fwname = "qcom/vpu/vpu30_p4.mbn",

Should fw_name be in firmware_data? as this can be change based on HFI
versions?

> -	.inst_iris_fmts = platform_fmts_sm8550_dec,
> -	.inst_iris_fmts_size = ARRAY_SIZE(platform_fmts_sm8550_dec),
> -	.inst_caps = &platform_inst_cap_sm8550,
> +
>  	.inst_fw_caps_dec = inst_fw_cap_sm8550_dec,
>  	.inst_fw_caps_dec_size = ARRAY_SIZE(inst_fw_cap_sm8550_dec),
>  	.inst_fw_caps_enc = inst_fw_cap_sm8550_enc,
>  	.inst_fw_caps_enc_size = ARRAY_SIZE(inst_fw_cap_sm8550_enc),
> -	.tz_cp_config_data = tz_cp_config_sm8550,
> -	.tz_cp_config_data_size = ARRAY_SIZE(tz_cp_config_sm8550),
> -	.core_arch = VIDEO_ARCH_LX,
> -	.hw_response_timeout = HW_RESPONSE_TIMEOUT_VALUE,
> -	.num_vpp_pipe = 4,
> -	.max_session_count = 16,
> -	.max_core_mbpf = NUM_MBS_8K * 2,
> -	.max_core_mbps = ((7680 * 4320) / 256) * 60,
> +
>  	.dec_input_config_params_default =
>  		sm8550_vdec_input_config_params_default,
>  	.dec_input_config_params_default_size =
> @@ -997,50 +971,15 @@ const struct iris_platform_data sm8550_data = {
>  	.enc_op_int_buf_tbl_size = ARRAY_SIZE(sm8550_enc_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"
> - */
> -const struct iris_platform_data sm8650_data = {
> +const struct iris_firmware_data iris_hfi_gen2_vpu33_data = {

This proves my above point.

iris_hfi_gen2_data and iris_hfi_gen2_vpu33_data become identical except for
get_vpu_buffer_size, which forces us to create multiple “firmware_data”
variants just to carry a hardware-specific difference.

Also, it will scale poorly going forward. When we introduce vpu4 /
vpu5–based platforms, we would need to add more copies of essentially the
same HFI Gen2 firmware_data, differing only in the buffer sizing callback.

Thanks,
Dikshita

>  	.init_hfi_ops = iris_hfi_gen2_sys_ops_init,
>  	.get_vpu_buffer_size = iris_vpu33_buf_size,
> -	.vpu_ops = &iris_vpu33_ops,
> -	.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),
> -	.opp_clk_tbl = sm8550_opp_clk_table,
> -	/* Upper bound of DMA address range */
> -	.dma_mask = 0xe0000000 - 1,
> -	.fwname = "qcom/vpu/vpu33_p4.mbn",
> -	.inst_iris_fmts = platform_fmts_sm8550_dec,
> -	.inst_iris_fmts_size = ARRAY_SIZE(platform_fmts_sm8550_dec),
> -	.inst_caps = &platform_inst_cap_sm8550,
> +
>  	.inst_fw_caps_dec = inst_fw_cap_sm8550_dec,
>  	.inst_fw_caps_dec_size = ARRAY_SIZE(inst_fw_cap_sm8550_dec),
>  	.inst_fw_caps_enc = inst_fw_cap_sm8550_enc,
>  	.inst_fw_caps_enc_size = ARRAY_SIZE(inst_fw_cap_sm8550_enc),
> -	.tz_cp_config_data = tz_cp_config_sm8550,
> -	.tz_cp_config_data_size = ARRAY_SIZE(tz_cp_config_sm8550),
> -	.core_arch = VIDEO_ARCH_LX,
> -	.hw_response_timeout = HW_RESPONSE_TIMEOUT_VALUE,
> -	.num_vpp_pipe = 4,
> -	.max_session_count = 16,
> -	.max_core_mbpf = NUM_MBS_8K * 2,
> -	.max_core_mbps = ((7680 * 4320) / 256) * 60,
> +
>  	.dec_input_config_params_default =
>  		sm8550_vdec_input_config_params_default,
>  	.dec_input_config_params_default_size =
> @@ -1097,9 +1036,81 @@ const struct iris_platform_data sm8650_data = {
>  	.enc_op_int_buf_tbl_size = ARRAY_SIZE(sm8550_enc_op_int_buf_tbl),
>  };
>