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 1:37 PM, Dmitry Baryshkov wrote:
> On Fri, Mar 13, 2026 at 01:19:21PM +0530, Dikshita Agarwal wrote:
> 
> I'm sorry, I've refreshed the series before receiving this email. I will
> send new iteration after settling the discussion here.
> 
>> 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.
> 
> Okay, so how do we solve the SC7280 Gen1 vs Gen2 situation? I can keep
> the function pointer in struct iris_platform_data for now, letting you
> sort it out in your series.

Thanks! that is SC7280 problem, since code evolved due to additional
features and other things, we might need to increase the vpu2 buffer size
to accommodate both Ge1 and Gen2 requirement, I will check that and address
in my series.

> 
>>
>> <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?
> 
> That would fail because then each device will have to gain its own
> struct iris_firmware_data.
> 
> But... Maybe we can do something as simple as:
> 
> struct iris_firmware_desc {
> 	const char *fwname;
> 	u32 (*get_vpu_buffer_size)(struct iris_inst *inst, enum iris_buffer_type buffer_type);
> 	bool (*checK_fw_match)(u8 *buf, size_t size);
> 	const struct iris_firmware_data *data;
> };
> 
> and then
> 
> struct iris_platform_data {
> 	struct iris_firmware_desc *gen1, *gen2;
> 	// .. the rest as usual;
> };
> 
> 
> struct iris_core {
> 	u32 (*get_vpu_buffer_size)(struct iris_inst *inst, enum iris_buffer_type buffer_type);
> 	const struct iris_firmware_data *data;
> 	// ... the rest as expected
> };
> 
> During first open the driver will try loading firmware from DT and
> identifying it using the check_fw_match() callback. If DT doesn't have
> firmware-name the driver will try loading gen2 and, if not found, gen1.
> When firmware loading succeeds, it will set the pointer and the callback
> in iris_core, settling the interface between the driver and the
> firmware.
> 
> WDYT?

This looks good to me. It handles the SC7280 Gen1 vs Gen2 buffer size
differences as well.

Thanks,
Dikshita

> 
>>> -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.
> 
> Yes. I'm not sure, if there is any difference between params / caps as
> implremented by the firmware for those generations.
>