Re: [PATCH v7 01/18] media: iris: Add Gen2 firmware autodetect and fallback

[Dmitry Baryshkov]

On Wed, 22 Jul 2026 at 01:15, Bryan O'Donoghue <bod@xxxxxxxxxx> wrote:
>
> On 09/07/2026 14:41, Dmitry Baryshkov wrote:
> > From: Dikshita Agarwal <dikshita.agarwal@xxxxxxxxxxxxxxxx>
> >
> > Some Iris platforms support both Gen1 and Gen2 HFI firmware images.
> > Update the firmware loading logic to handle this generically by
> > preferring Gen2 when available, while safely falling back to Gen1
> > when required.
> >
> > The firmware loading logic is updated with the following priority:
> > 1. Device Tree (`firmware-name`): If specified, load unconditionally.
> > 2. Gen2 default : If no DT override exists, select the Gen2 firmware
> >     descriptor when present and attempt to load the corresponding
> >     firmware image.
> > 3. Gen1 Fallback: If loading the Gen2 firmware fails and a Gen1
> >     descriptor is available, retry with the Gen1 firmware image.
> >
> > When a platform provides both Gen1 and Gen2 firmware descriptors and the
> > firmware is loaded via a DT override, the driver detects the
> > firmware generation at runtime before authentication by inspecting
> > the firmware data. The firmware is classified as Gen2 if the
> > QC_IMAGE_VERSION_STRING starts with "vfw" or matches the
> > "video-firmware.N.M" format with N >= 2.
> >
> > If a Gen1 firmware image is detected in this case, the driver switches
> > to the Gen1 firmware descriptor and associated platform data so that
> > the correct HFI implementation is used.
> >
> > This change makes firmware generation detection platform‑agnostic,
> > preserves DT overrides, prefers newer Gen2 firmware when available,
> > and maintains compatibility with platforms that only support Gen1.
> >
> > Signed-off-by: Dikshita Agarwal <dikshita.agarwal@xxxxxxxxxxxxxxxx>
> > Co-developed-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxxxxxxxx>
> > ---
> >   drivers/media/platform/qcom/iris/iris_core.c       |   2 +
> >   drivers/media/platform/qcom/iris/iris_firmware.c   | 146 ++++++++++++++++++---
> >   .../platform/qcom/iris/iris_platform_common.h      |   6 +-
> >   .../media/platform/qcom/iris/iris_platform_vpu2.c  |  11 +-
> >   .../media/platform/qcom/iris/iris_platform_vpu3x.c |  10 +-
> >   drivers/media/platform/qcom/iris/iris_probe.c      |   4 -
> >   drivers/media/platform/qcom/iris/iris_vidc.c       |   1 +
> >   7 files changed, 147 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/media/platform/qcom/iris/iris_core.c b/drivers/media/platform/qcom/iris/iris_core.c
> > index 52bf56e517f9..6dbe18be5b49 100644
> > --- a/drivers/media/platform/qcom/iris/iris_core.c
> > +++ b/drivers/media/platform/qcom/iris/iris_core.c
> > @@ -6,6 +6,7 @@
> >   #include <linux/pm_runtime.h>
> >
> >   #include "iris_core.h"
> > +#include "iris_ctrls.h"
> >   #include "iris_firmware.h"
> >   #include "iris_state.h"
> >   #include "iris_vpu_common.h"
> > @@ -79,6 +80,7 @@ int iris_core_init(struct iris_core *core)
> >               goto error_unload_fw;
> >
> >       core->iris_firmware_data->init_hfi_ops(core);
> > +     iris_session_init_caps(core);
> >
> >       ret = iris_hfi_core_init(core);
> >       if (ret)
> > diff --git a/drivers/media/platform/qcom/iris/iris_firmware.c b/drivers/media/platform/qcom/iris/iris_firmware.c
> > index 1a476146d758..a3aa41aa1e66 100644
> > --- a/drivers/media/platform/qcom/iris/iris_firmware.c
> > +++ b/drivers/media/platform/qcom/iris/iris_firmware.c
> > @@ -16,20 +16,138 @@
> >
> >   #define MAX_FIRMWARE_NAME_SIZE      128
> >
> > -static int iris_load_fw_to_memory(struct iris_core *core, const char *fw_name)
> > +/* Detect Gen2 firmware by scanning the blob for:
> > + *   QC_IMAGE_VERSION_STRING=<version>
> > + * and then checking:
> > + *   - version starts with "vfw", OR
> > + *   - version matches "video-firmware.N.M" with N >= 2
> > + */
> > +
> > +static bool iris_detect_gen2_from_fwdata(const u8 *data, size_t size)
> > +{
> > +     static const char *marker = "QC_IMAGE_VERSION_STRING=";
> > +     const size_t mlen = strlen(marker);
> > +     static const char *vfw = "vfw";
> > +     const size_t vfwlen = strlen(vfw);
> > +     static const char *vf = "video-firmware.";
> > +     const size_t vflen = strlen(vf);
> > +
> > +     for (size_t i = 0; i + mlen < size; i++) {
> > +             const char *found;
> > +
> > +             if (memcmp(data + i, marker, mlen))
> > +                     continue;
> > +
> > +             found = data + i + mlen;
> > +             size -= i + mlen;
> > +
> > +             /* vfw => Gen2 */
> > +             if (size > vfwlen && !memcmp(found, vfw, vfwlen))
> > +                     return true;
> > +
> > +             if (size < vflen ||
> > +                 memcmp(found, vf, vflen))
> > +                     return false;
> > +
> > +             found += vflen;
> > +             size -= vflen;
> > +
> > +             /*
> > +              * video-firmware.1.x is Gen1.
> > +              * video-firmware.2.x and video-firmware.10.x are Gen2.
> > +              */
> > +             return size >= 2 &&
> > +                     (*found >= '2' || (*found == '1' && found[1] != '.'));
> > +     }
> > +
> > +     return false;
>
> The logic here is much easier to read for me thanks for updating it.

And there will be a next iteration, as we need to check the version
string after qcom_mdt_load() loads all data segments.

>
> > +}
> > +
> > +static const struct firmware *iris_detect_firmware(struct iris_core *core,
> > +                                                const char **fw_name)
> > +{
> > +     const struct iris_firmware_desc *desc;
> > +     const struct firmware *firmware;
> > +     bool has_both_gens;
> > +     int ret;
> > +
> > +     *fw_name = NULL;
> > +     ret = of_property_read_string_index(dev_of_node(core->dev), "firmware-name", 0, fw_name);
> > +
> > +     /*
> > +      * A platform may support both Gen1 and Gen2 firmware; which one is used
> > +      * depends on the firmware image installed on the system, not on the
> > +      * hardware. That installed image does not change while the device is
> > +      * bound, so detect the generation only once and reuse the chosen
> > +      * descriptor on later core bring-ups (e.g. after a system error
> > +      * recovery). Besides avoiding the redundant probing, this ensures
> > +      * core->iris_firmware_desc and iris_firmware_data are published exactly
> > +      * once, before any session exists, so the lockless readers in the ioctl
> > +      * paths never observe a reassignment.
> > +      */
> > +     if (core->iris_firmware_desc) {
> > +             if (ret)
> > +                     *fw_name = core->iris_firmware_desc->fwname;
> > +             ret = request_firmware(&firmware, *fw_name, core->dev);
> > +             return ret ? ERR_PTR(ret) : firmware;
> > +     }
> > +
> > +     has_both_gens = core->iris_platform_data->firmware_desc_gen2 &&
> > +             core->iris_platform_data->firmware_desc_gen1;
> > +
> > +     if (core->iris_platform_data->firmware_desc_gen2)
> > +             desc = core->iris_platform_data->firmware_desc_gen2;
> > +     else if (core->iris_platform_data->firmware_desc_gen1)
> > +             desc = core->iris_platform_data->firmware_desc_gen1;
> > +     else
> > +             return ERR_PTR(-EINVAL);
> > +
> > +     if (ret) {
> > +             /* No firmware-name in DT: select by probing Gen2 then Gen1. */
> > +             *fw_name = desc->fwname;
> > +             if (has_both_gens)
> > +                     ret = firmware_request_nowarn(&firmware, *fw_name, core->dev);
> > +             else
> > +                     ret = request_firmware(&firmware, *fw_name, core->dev);
> > +             if (ret && has_both_gens) {
> > +                     desc = core->iris_platform_data->firmware_desc_gen1;
> > +                     *fw_name = desc->fwname;
> > +                     ret = request_firmware(&firmware, *fw_name, core->dev);
> > +             }
> > +             if (ret)
> > +                     return ERR_PTR(ret);
> > +     } else {
> > +             /* firmware-name given: load it and detect its generation. */
> > +             ret = request_firmware(&firmware, *fw_name, core->dev);
> > +             if (ret)
> > +                     return ERR_PTR(ret);
> > +
> > +             if (has_both_gens &&
> > +                 !iris_detect_gen2_from_fwdata((const u8 *)firmware->data, firmware->size)) {
>
> Is it really legitimate to parse the _entire_ firmware for this data -
> potentially megabytes ?
>
> Surely the first 4k or less of data is enough to know if a firmware
> string has been detected ?

I think we have discussed this already. No, it's not. The string is in
the middle of the image. 4k is just the ELF header and signatures.

>
> > +                     dev_info(core->dev, "Gen1 FW detected in %s\n", *fw_name);
> > +                     desc = core->iris_platform_data->firmware_desc_gen1;
> > +             }
> > +     }
> > +
> > +     /* Publish iris_firmware_data first, then iris_firmware_desc (the guard). */
> > +     core->iris_firmware_data = desc->firmware_data;
> > +     core->iris_firmware_desc = desc;
> > +
> > +     return firmware;
> > +}
> > +
> > +static int iris_load_fw_to_memory(struct iris_core *core)
> >   {
> >       const struct firmware *firmware = NULL;
> >       struct device *dev = core->dev;
> >       struct resource res;
> >       phys_addr_t mem_phys;
> > +     const char *fw_name;
> >       size_t res_size;
> >       ssize_t fw_size;
> >       void *mem_virt;
> >       int ret;
> >
> > -     if (strlen(fw_name) >= MAX_FIRMWARE_NAME_SIZE - 4)
> > -             return -EINVAL;
> > -
> >       ret = of_reserved_mem_region_to_resource(dev->of_node, 0, &res);
> >       if (ret)
> >               return ret;
> > @@ -37,9 +155,9 @@ static int iris_load_fw_to_memory(struct iris_core *core, const char *fw_name)
> >       mem_phys = res.start;
> >       res_size = resource_size(&res);
> >
> > -     ret = request_firmware(&firmware, fw_name, dev);
> > -     if (ret)
> > -             return ret;
> > +     firmware = iris_detect_firmware(core, &fw_name);
> > +     if (IS_ERR(firmware))
> > +             return PTR_ERR(firmware);
> >
> >       fw_size = qcom_mdt_get_size(firmware);
> >       if (fw_size < 0 || res_size < (size_t)fw_size) {
> > @@ -66,18 +184,12 @@ static int iris_load_fw_to_memory(struct iris_core *core, const char *fw_name)
> >   int iris_fw_load(struct iris_core *core)
> >   {
> >       const struct tz_cp_config *cp_config;
> > -     const char *fwpath = NULL;
> >       int i, ret;
> >
> > -     ret = of_property_read_string_index(core->dev->of_node, "firmware-name", 0,
> > -                                         &fwpath);
> > -     if (ret)
> > -             fwpath = core->iris_firmware_desc->fwname;
> > -
> > -     ret = iris_load_fw_to_memory(core, fwpath);
> > +     ret = iris_load_fw_to_memory(core);
> >       if (ret) {
> > -             dev_err(core->dev, "firmware download failed\n");
> > -             return -ENOMEM;
> > +             dev_err(core->dev, "firmware download failed %d\n", ret);
> > +             return ret;
> >       }
> >
> >       ret = qcom_scm_pas_auth_and_reset(IRIS_PAS_ID);
> > @@ -99,7 +211,7 @@ int iris_fw_load(struct iris_core *core)
> >               }
> >       }
> >
> > -     return ret;
> > +     return 0;
> >   }
> >
> >   int iris_fw_unload(struct iris_core *core)
> > diff --git a/drivers/media/platform/qcom/iris/iris_platform_common.h b/drivers/media/platform/qcom/iris/iris_platform_common.h
> > index c9256f2323dc..55a4fa356985 100644
> > --- a/drivers/media/platform/qcom/iris/iris_platform_common.h
> > +++ b/drivers/media/platform/qcom/iris/iris_platform_common.h
> > @@ -289,11 +289,7 @@ struct iris_firmware_desc {
> >   };
> >
> >   struct iris_platform_data {
> > -     /*
> > -      * XXX: replace with gen1 / gen2 pointers once we have platforms
> > -      * supporting both firmware kinds.
> > -      */
> > -     const struct iris_firmware_desc *firmware_desc;
> > +     const struct iris_firmware_desc *firmware_desc_gen1, *firmware_desc_gen2;
> >
> >       const struct vpu_ops *vpu_ops;
> >       const struct icc_info *icc_tbl;
> > diff --git a/drivers/media/platform/qcom/iris/iris_platform_vpu2.c b/drivers/media/platform/qcom/iris/iris_platform_vpu2.c
> > index 6e06a32822bb..961dce2e6aa9 100644
> > --- a/drivers/media/platform/qcom/iris/iris_platform_vpu2.c
> > +++ b/drivers/media/platform/qcom/iris/iris_platform_vpu2.c
> > @@ -22,6 +22,12 @@ static const struct iris_firmware_desc iris_vpu20_p1_gen1_desc = {
> >       .fwname = "qcom/vpu/vpu20_p1.mbn",
> >   };
> >
> > +static const struct iris_firmware_desc iris_vpu20_p1_gen2_s6_desc = {
> > +     .firmware_data = &iris_hfi_gen2_data,
> > +     .get_vpu_buffer_size = iris_vpu33_buf_size,
> > +     .fwname = "qcom/vpu/vpu20_p1_gen2_s6.mbn",
> > +};
> > +
> >   static const struct iris_firmware_desc iris_vpu20_p4_gen1_desc = {
> >       .firmware_data = &iris_hfi_gen1_data,
> >       .get_vpu_buffer_size = iris_vpu_buf_size,
> > @@ -65,7 +71,8 @@ static const struct tz_cp_config tz_cp_config_vpu2[] = {
> >   };
> >
> >   const struct iris_platform_data sc7280_data = {
> > -     .firmware_desc = &iris_vpu20_p1_gen1_desc,
> > +     .firmware_desc_gen1 = &iris_vpu20_p1_gen1_desc,
> > +     .firmware_desc_gen2 = &iris_vpu20_p1_gen2_s6_desc,
> >       .vpu_ops = &iris_vpu2_ops,
> >       .icc_tbl = iris_icc_info_vpu2,
> >       .icc_tbl_size = ARRAY_SIZE(iris_icc_info_vpu2),
> > @@ -94,7 +101,7 @@ const struct iris_platform_data sc7280_data = {
> >   };
> >
> >   const struct iris_platform_data sm8250_data = {
> > -     .firmware_desc = &iris_vpu20_p4_gen1_desc,
> > +     .firmware_desc_gen1 = &iris_vpu20_p4_gen1_desc,
> >       .vpu_ops = &iris_vpu2_ops,
> >       .icc_tbl = iris_icc_info_vpu2,
> >       .icc_tbl_size = ARRAY_SIZE(iris_icc_info_vpu2),
> > diff --git a/drivers/media/platform/qcom/iris/iris_platform_vpu3x.c b/drivers/media/platform/qcom/iris/iris_platform_vpu3x.c
> > index 2c63adbc5579..74626b35d9cb 100644
> > --- a/drivers/media/platform/qcom/iris/iris_platform_vpu3x.c
> > +++ b/drivers/media/platform/qcom/iris/iris_platform_vpu3x.c
> > @@ -90,7 +90,7 @@ static const struct tz_cp_config tz_cp_config_vpu3[] = {
> >    * - inst_caps to platform_inst_cap_qcs8300
> >    */
> >   const struct iris_platform_data qcs8300_data = {
> > -     .firmware_desc = &iris_vpu30_p4_s6_gen2_desc,
> > +     .firmware_desc_gen2 = &iris_vpu30_p4_s6_gen2_desc,
> >       .vpu_ops = &iris_vpu3_ops,
> >       .icc_tbl = iris_icc_info_vpu3x,
> >       .icc_tbl_size = ARRAY_SIZE(iris_icc_info_vpu3x),
> > @@ -119,7 +119,7 @@ const struct iris_platform_data qcs8300_data = {
> >   };
> >
> >   const struct iris_platform_data sm8550_data = {
> > -     .firmware_desc = &iris_vpu30_p4_gen2_desc,
> > +     .firmware_desc_gen2 = &iris_vpu30_p4_gen2_desc,
> >       .vpu_ops = &iris_vpu3_ops,
> >       .icc_tbl = iris_icc_info_vpu3x,
> >       .icc_tbl_size = ARRAY_SIZE(iris_icc_info_vpu3x),
> > @@ -154,7 +154,7 @@ const struct iris_platform_data sm8550_data = {
> >    * - controller_rst_tbl to sm8650_controller_reset_table
> >    */
> >   const struct iris_platform_data sm8650_data = {
> > -     .firmware_desc = &iris_vpu33_p4_gen2_desc,
> > +     .firmware_desc_gen2 = &iris_vpu33_p4_gen2_desc,
> >       .vpu_ops = &iris_vpu33_ops,
> >       .icc_tbl = iris_icc_info_vpu3x,
> >       .icc_tbl_size = ARRAY_SIZE(iris_icc_info_vpu3x),
> > @@ -185,7 +185,7 @@ const struct iris_platform_data sm8650_data = {
> >   };
> >
> >   const struct iris_platform_data sm8750_data = {
> > -     .firmware_desc = &iris_vpu35_p4_gen2_desc,
> > +     .firmware_desc_gen2 = &iris_vpu35_p4_gen2_desc,
> >       .vpu_ops = &iris_vpu35_ops,
> >       .icc_tbl = iris_icc_info_vpu3x,
> >       .icc_tbl_size = ARRAY_SIZE(iris_icc_info_vpu3x),
> > @@ -220,7 +220,7 @@ const struct iris_platform_data sm8750_data = {
> >    * - different num_vpp_pipe
> >    */
> >   const struct iris_platform_data x1p42100_data = {
> > -     .firmware_desc = &iris_vpu30_p1_gen2_desc,
> > +     .firmware_desc_gen2 = &iris_vpu30_p1_gen2_desc,
> >       .vpu_ops = &iris_vpu3_ops,
> >       .icc_tbl = iris_icc_info_vpu3x,
> >       .icc_tbl_size = ARRAY_SIZE(iris_icc_info_vpu3x),
> > diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c
> > index c2dcb50a2782..7fe31136df21 100644
> > --- a/drivers/media/platform/qcom/iris/iris_probe.c
> > +++ b/drivers/media/platform/qcom/iris/iris_probe.c
> > @@ -251,8 +251,6 @@ static int iris_probe(struct platform_device *pdev)
> >               return core->irq;
> >
> >       core->iris_platform_data = of_device_get_match_data(core->dev);
> > -     core->iris_firmware_desc = core->iris_platform_data->firmware_desc;
> > -     core->iris_firmware_data = core->iris_firmware_desc->firmware_data;
> >
> >       core->ubwc_cfg = qcom_ubwc_config_get_data();
> >       if (IS_ERR(core->ubwc_cfg))
> > @@ -271,8 +269,6 @@ static int iris_probe(struct platform_device *pdev)
> >       if (ret)
> >               return ret;
> >
> > -     iris_session_init_caps(core);
> > -
> >       ret = v4l2_device_register(dev, &core->v4l2_dev);
> >       if (ret)
> >               return ret;
> > diff --git a/drivers/media/platform/qcom/iris/iris_vidc.c b/drivers/media/platform/qcom/iris/iris_vidc.c
> > index 14d63dc76c9b..33edbc5cab8f 100644
> > --- a/drivers/media/platform/qcom/iris/iris_vidc.c
> > +++ b/drivers/media/platform/qcom/iris/iris_vidc.c
> > @@ -9,6 +9,7 @@
> >   #include <media/v4l2-mem2mem.h>
> >   #include <media/videobuf2-dma-contig.h>
> >
> > +#include "iris_ctrls.h"
> >   #include "iris_vidc.h"
> >   #include "iris_instance.h"
> >   #include "iris_vdec.h"
> >
> > --
> > 2.47.3
> >
> >
>


-- 
With best wishes
Dmitry