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

[Dmitry Baryshkov]

On Mon, Mar 16, 2026 at 12:01:48PM +0530, Dikshita Agarwal wrote:
> 
> 
> On 3/13/2026 2:55 PM, Dikshita Agarwal wrote:
> > 
> > 
> > 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@xxxxxxxxxxxxxxxx>
> >>>> ---
> >>>>  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.
> 
> Do you plan to implement above design in the next version of your series?

I've sent the platform_data patchset keeping the get_vpu_buffer_size in
place (as you suggested) because that is fine for the current supported
usecases. I would prefer if iris_firmware_desc comes as a part of your
Kodiak Gen1-vs-Gen2 patchset. I can send you a corresponding patch, if
you wan, or you can implement that on your own. What would be your
preference?

> 
> Thanks,
> Dikshita
> > 
> > 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.
> >>

-- 
With best wishes
Dmitry