| Message ID | 20250128080429.3911091-2-quic_dikshita@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Add helper module to select video driver | expand |
On Tue, Jan 28, 2025 at 01:34:28PM +0530, Dikshita Agarwal wrote: > Introduce a helper module with a kernel param to select between > venus and iris drivers for platforms supported by both drivers. > > Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com> > --- > drivers/media/platform/qcom/Makefile | 1 + > drivers/media/platform/qcom/iris/iris_core.h | 1 + > drivers/media/platform/qcom/iris/iris_probe.c | 3 + > drivers/media/platform/qcom/venus/core.c | 5 ++ > .../platform/qcom/video_drv_helper/Makefile | 4 ++ > .../qcom/video_drv_helper/video_drv_helper.c | 70 +++++++++++++++++++ > .../qcom/video_drv_helper/video_drv_helper.h | 11 +++ > 7 files changed, 95 insertions(+) > create mode 100644 drivers/media/platform/qcom/video_drv_helper/Makefile > create mode 100644 drivers/media/platform/qcom/video_drv_helper/video_drv_helper.c > create mode 100644 drivers/media/platform/qcom/video_drv_helper/video_drv_helper.h > > diff --git a/drivers/media/platform/qcom/Makefile b/drivers/media/platform/qcom/Makefile > index ea2221a202c0..15accde3bd67 100644 > --- a/drivers/media/platform/qcom/Makefile > +++ b/drivers/media/platform/qcom/Makefile > @@ -2,3 +2,4 @@ > obj-y += camss/ > obj-y += iris/ > obj-y += venus/ > +obj-y += video_drv_helper/ > diff --git a/drivers/media/platform/qcom/iris/iris_core.h b/drivers/media/platform/qcom/iris/iris_core.h > index 37fb4919fecc..7108e751ff88 100644 > --- a/drivers/media/platform/qcom/iris/iris_core.h > +++ b/drivers/media/platform/qcom/iris/iris_core.h > @@ -107,5 +107,6 @@ struct iris_core { > > int iris_core_init(struct iris_core *core); > void iris_core_deinit(struct iris_core *core); > +extern bool video_drv_should_bind(struct device *dev, bool is_iris_driver); s/extern //g > > #endif > diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c > index 954cc7c0cc97..276461ade811 100644 > --- a/drivers/media/platform/qcom/iris/iris_probe.c > +++ b/drivers/media/platform/qcom/iris/iris_probe.c > @@ -196,6 +196,9 @@ static int iris_probe(struct platform_device *pdev) > u64 dma_mask; > int ret; > > + if (!video_drv_should_bind(&pdev->dev, true)) > + return -ENODEV; > + > core = devm_kzalloc(&pdev->dev, sizeof(*core), GFP_KERNEL); > if (!core) > return -ENOMEM; > diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c > index 77d48578ecd2..b38be7812efe 100644 > --- a/drivers/media/platform/qcom/venus/core.c > +++ b/drivers/media/platform/qcom/venus/core.c > @@ -369,12 +369,17 @@ static int venus_add_dynamic_nodes(struct venus_core *core) > static void venus_remove_dynamic_nodes(struct venus_core *core) {} > #endif > > +extern bool video_drv_should_bind(struct device *dev, bool is_iris_driver); Use #include instead. > + > static int venus_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > struct venus_core *core; > int ret; > > + if (!video_drv_should_bind(&pdev->dev, false)) > + return -ENODEV; > + > core = devm_kzalloc(dev, sizeof(*core), GFP_KERNEL); > if (!core) > return -ENOMEM; > diff --git a/drivers/media/platform/qcom/video_drv_helper/Makefile b/drivers/media/platform/qcom/video_drv_helper/Makefile > new file mode 100644 > index 000000000000..82567e0392fb > --- /dev/null > +++ b/drivers/media/platform/qcom/video_drv_helper/Makefile > @@ -0,0 +1,4 @@ > +# Makefile for Video driver helper > + > +obj-m := video_drv_helper.o Always built as a module? And what if iris or venus are built into the kernel? Add a normal Kconfig symbol, tristate, no Kconfig string. Use depends on IRIS && VENUS (and maybe default y) to let it be built only if both drivers are enabled. > + > diff --git a/drivers/media/platform/qcom/video_drv_helper/video_drv_helper.c b/drivers/media/platform/qcom/video_drv_helper/video_drv_helper.c > new file mode 100644 > index 000000000000..9009c2906e54 > --- /dev/null > +++ b/drivers/media/platform/qcom/video_drv_helper/video_drv_helper.c > @@ -0,0 +1,70 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#include <linux/device.h> > +#include <linux/module.h> > +#include <linux/of.h> > + > +#include "video_drv_helper.h" > + > +/* The venus driver supports only hfi gen1 to communicate with the firmware while > + * the iris driver supports both hfi gen1 and hfi gen2. > + * The support of hfi gen1 is added to the iris driver with the intention that > + * it can support old gen1 interface based firmware, while enabling gen2 based future SOCs. > + * With this, the plan is to migrate older SOCs from venus to iris. > + * As of now, since the iris driver supports only entry level features and doesn't have > + * feature parity with the venus driver, a runtime-selection is provided to user via > + * module parameter 'prefer_venus' to select the driver. > + * This selection is available only for the SoCs which are supported by both venus > + * and iris eg: SM8250. > + * When the feature parity is achieved, the plan is to switch the default to point to > + * the iris driver, then gradually start removing platforms from venus. > + * Hardware supported by only venus - 8916, 8996, SDM660, SDM845, SC7180, SC7280 > + * Hardware supported by only iris - SM8550 > + * Hardware supported by both venus and iris - SM8250 > + */ > + > +#if !IS_REACHABLE(CONFIG_VIDEO_QCOM_VENUS) || !IS_REACHABLE(CONFIG_VIDEO_QCOM_IRIS) > +bool video_drv_should_bind(struct device *dev, bool is_iris_driver) > +{ > + /* If just a single driver is enabled, use it no matter what */ > + return true; > +} > + > +#else Move the stub funtion to header. > +static bool prefer_venus = true; > +MODULE_PARM_DESC(prefer_venus, "Select whether venus or iris driver should be preferred"); > +module_param(prefer_venus, bool, 0444); > + > +/* list all platforms supported by both venus and iris drivers */ > +static const char *const venus_to_iris_migration[] = { > + "qcom,sm8250-venus", > + NULL, > +}; > + > +bool video_drv_should_bind(struct device *dev, bool is_iris_driver) The prefix is too broad, but maybe its fine. > +{ > + if (of_device_compatible_match(dev->of_node, venus_to_iris_migration)) > + return prefer_venus ? !is_iris_driver : is_iris_driver; > + > + return true; > +} > +EXPORT_SYMBOL_GPL(video_drv_should_bind); > +#endif > + > +static int __init video_drv_helper_init(void) > +{ > + return 0; > +} > + > +static void __exit video_drv_helper_exit(void) > +{ > +} > + > +module_init(video_drv_helper_init); > +module_exit(video_drv_helper_exit); No need for this, please drop. > + > +MODULE_DESCRIPTION("A video driver helper module"); > +MODULE_LICENSE("GPL"); > diff --git a/drivers/media/platform/qcom/video_drv_helper/video_drv_helper.h b/drivers/media/platform/qcom/video_drv_helper/video_drv_helper.h > new file mode 100644 > index 000000000000..6d835227fec2 > --- /dev/null > +++ b/drivers/media/platform/qcom/video_drv_helper/video_drv_helper.h > @@ -0,0 +1,11 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#ifndef __VIDEO_DRV_HELPER_H__ > +#define __VIDEO_DRV_HELPER_H__ > + > +bool video_drv_should_bind(struct device *dev, bool is_iris_driver); > + > +#endif > -- > 2.34.1 >
On 1/28/2025 9:44 PM, Dmitry Baryshkov wrote: > On Tue, Jan 28, 2025 at 01:34:28PM +0530, Dikshita Agarwal wrote: >> Introduce a helper module with a kernel param to select between >> venus and iris drivers for platforms supported by both drivers. >> >> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com> >> --- >> drivers/media/platform/qcom/Makefile | 1 + >> drivers/media/platform/qcom/iris/iris_core.h | 1 + >> drivers/media/platform/qcom/iris/iris_probe.c | 3 + >> drivers/media/platform/qcom/venus/core.c | 5 ++ >> .../platform/qcom/video_drv_helper/Makefile | 4 ++ >> .../qcom/video_drv_helper/video_drv_helper.c | 70 +++++++++++++++++++ >> .../qcom/video_drv_helper/video_drv_helper.h | 11 +++ >> 7 files changed, 95 insertions(+) >> create mode 100644 drivers/media/platform/qcom/video_drv_helper/Makefile >> create mode 100644 drivers/media/platform/qcom/video_drv_helper/video_drv_helper.c >> create mode 100644 drivers/media/platform/qcom/video_drv_helper/video_drv_helper.h >> >> diff --git a/drivers/media/platform/qcom/Makefile b/drivers/media/platform/qcom/Makefile >> index ea2221a202c0..15accde3bd67 100644 >> --- a/drivers/media/platform/qcom/Makefile >> +++ b/drivers/media/platform/qcom/Makefile >> @@ -2,3 +2,4 @@ >> obj-y += camss/ >> obj-y += iris/ >> obj-y += venus/ >> +obj-y += video_drv_helper/ >> diff --git a/drivers/media/platform/qcom/iris/iris_core.h b/drivers/media/platform/qcom/iris/iris_core.h >> index 37fb4919fecc..7108e751ff88 100644 >> --- a/drivers/media/platform/qcom/iris/iris_core.h >> +++ b/drivers/media/platform/qcom/iris/iris_core.h >> @@ -107,5 +107,6 @@ struct iris_core { >> >> int iris_core_init(struct iris_core *core); >> void iris_core_deinit(struct iris_core *core); >> +extern bool video_drv_should_bind(struct device *dev, bool is_iris_driver); > > s/extern //g > >> >> #endif >> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c >> index 954cc7c0cc97..276461ade811 100644 >> --- a/drivers/media/platform/qcom/iris/iris_probe.c >> +++ b/drivers/media/platform/qcom/iris/iris_probe.c >> @@ -196,6 +196,9 @@ static int iris_probe(struct platform_device *pdev) >> u64 dma_mask; >> int ret; >> >> + if (!video_drv_should_bind(&pdev->dev, true)) >> + return -ENODEV; >> + >> core = devm_kzalloc(&pdev->dev, sizeof(*core), GFP_KERNEL); >> if (!core) >> return -ENOMEM; >> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c >> index 77d48578ecd2..b38be7812efe 100644 >> --- a/drivers/media/platform/qcom/venus/core.c >> +++ b/drivers/media/platform/qcom/venus/core.c >> @@ -369,12 +369,17 @@ static int venus_add_dynamic_nodes(struct venus_core *core) >> static void venus_remove_dynamic_nodes(struct venus_core *core) {} >> #endif >> >> +extern bool video_drv_should_bind(struct device *dev, bool is_iris_driver); > > Use #include instead. > >> + >> static int venus_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> struct venus_core *core; >> int ret; >> >> + if (!video_drv_should_bind(&pdev->dev, false)) >> + return -ENODEV; >> + >> core = devm_kzalloc(dev, sizeof(*core), GFP_KERNEL); >> if (!core) >> return -ENOMEM; >> diff --git a/drivers/media/platform/qcom/video_drv_helper/Makefile b/drivers/media/platform/qcom/video_drv_helper/Makefile >> new file mode 100644 >> index 000000000000..82567e0392fb >> --- /dev/null >> +++ b/drivers/media/platform/qcom/video_drv_helper/Makefile >> @@ -0,0 +1,4 @@ >> +# Makefile for Video driver helper >> + >> +obj-m := video_drv_helper.o > > Always built as a module? And what if iris or venus are built into the > kernel? iris and venus are always built as module, and if we are adding the dependency of this module on IRIS && VENUS then this can't be Y I think. > Add a normal Kconfig symbol, tristate, no Kconfig string. Use depends on > IRIS && VENUS (and maybe default y) to let it be built only if both > drivers are enabled. > >> + >> diff --git a/drivers/media/platform/qcom/video_drv_helper/video_drv_helper.c b/drivers/media/platform/qcom/video_drv_helper/video_drv_helper.c >> new file mode 100644 >> index 000000000000..9009c2906e54 >> --- /dev/null >> +++ b/drivers/media/platform/qcom/video_drv_helper/video_drv_helper.c >> @@ -0,0 +1,70 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved. >> + */ >> + >> +#include <linux/device.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> + >> +#include "video_drv_helper.h" >> + >> +/* The venus driver supports only hfi gen1 to communicate with the firmware while >> + * the iris driver supports both hfi gen1 and hfi gen2. >> + * The support of hfi gen1 is added to the iris driver with the intention that >> + * it can support old gen1 interface based firmware, while enabling gen2 based future SOCs. >> + * With this, the plan is to migrate older SOCs from venus to iris. >> + * As of now, since the iris driver supports only entry level features and doesn't have >> + * feature parity with the venus driver, a runtime-selection is provided to user via >> + * module parameter 'prefer_venus' to select the driver. >> + * This selection is available only for the SoCs which are supported by both venus >> + * and iris eg: SM8250. >> + * When the feature parity is achieved, the plan is to switch the default to point to >> + * the iris driver, then gradually start removing platforms from venus. >> + * Hardware supported by only venus - 8916, 8996, SDM660, SDM845, SC7180, SC7280 >> + * Hardware supported by only iris - SM8550 >> + * Hardware supported by both venus and iris - SM8250 >> + */ >> + >> +#if !IS_REACHABLE(CONFIG_VIDEO_QCOM_VENUS) || !IS_REACHABLE(CONFIG_VIDEO_QCOM_IRIS) >> +bool video_drv_should_bind(struct device *dev, bool is_iris_driver) >> +{ >> + /* If just a single driver is enabled, use it no matter what */ >> + return true; >> +} >> + >> +#else > > Move the stub funtion to header. > >> +static bool prefer_venus = true; >> +MODULE_PARM_DESC(prefer_venus, "Select whether venus or iris driver should be preferred"); >> +module_param(prefer_venus, bool, 0444); >> + >> +/* list all platforms supported by both venus and iris drivers */ >> +static const char *const venus_to_iris_migration[] = { >> + "qcom,sm8250-venus", >> + NULL, >> +}; >> + >> +bool video_drv_should_bind(struct device *dev, bool is_iris_driver) > > The prefix is too broad, but maybe its fine. > >> +{ >> + if (of_device_compatible_match(dev->of_node, venus_to_iris_migration)) >> + return prefer_venus ? !is_iris_driver : is_iris_driver; >> + >> + return true; >> +} >> +EXPORT_SYMBOL_GPL(video_drv_should_bind); >> +#endif >> + >> +static int __init video_drv_helper_init(void) >> +{ >> + return 0; >> +} >> + >> +static void __exit video_drv_helper_exit(void) >> +{ >> +} >> + >> +module_init(video_drv_helper_init); >> +module_exit(video_drv_helper_exit); > > No need for this, please drop. > >> + >> +MODULE_DESCRIPTION("A video driver helper module"); >> +MODULE_LICENSE("GPL"); >> diff --git a/drivers/media/platform/qcom/video_drv_helper/video_drv_helper.h b/drivers/media/platform/qcom/video_drv_helper/video_drv_helper.h >> new file mode 100644 >> index 000000000000..6d835227fec2 >> --- /dev/null >> +++ b/drivers/media/platform/qcom/video_drv_helper/video_drv_helper.h >> @@ -0,0 +1,11 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> +/* >> + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved. >> + */ >> + >> +#ifndef __VIDEO_DRV_HELPER_H__ >> +#define __VIDEO_DRV_HELPER_H__ >> + >> +bool video_drv_should_bind(struct device *dev, bool is_iris_driver); >> + >> +#endif >> -- >> 2.34.1 >> >
On 28/01/2025 09:04, Dikshita Agarwal wrote: > Introduce a helper module with a kernel param to select between > venus and iris drivers for platforms supported by both drivers. > > Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com> > --- > drivers/media/platform/qcom/Makefile | 1 + > drivers/media/platform/qcom/iris/iris_core.h | 1 + > drivers/media/platform/qcom/iris/iris_probe.c | 3 + > drivers/media/platform/qcom/venus/core.c | 5 ++ > .../platform/qcom/video_drv_helper/Makefile | 4 ++ > .../qcom/video_drv_helper/video_drv_helper.c | 70 +++++++++++++++++++ > .../qcom/video_drv_helper/video_drv_helper.h | 11 +++ > 7 files changed, 95 insertions(+) > create mode 100644 drivers/media/platform/qcom/video_drv_helper/Makefile > create mode 100644 drivers/media/platform/qcom/video_drv_helper/video_drv_helper.c > create mode 100644 drivers/media/platform/qcom/video_drv_helper/video_drv_helper.h > > diff --git a/drivers/media/platform/qcom/Makefile b/drivers/media/platform/qcom/Makefile > index ea2221a202c0..15accde3bd67 100644 > --- a/drivers/media/platform/qcom/Makefile > +++ b/drivers/media/platform/qcom/Makefile > @@ -2,3 +2,4 @@ > obj-y += camss/ > obj-y += iris/ > obj-y += venus/ > +obj-y += video_drv_helper/ > diff --git a/drivers/media/platform/qcom/iris/iris_core.h b/drivers/media/platform/qcom/iris/iris_core.h > index 37fb4919fecc..7108e751ff88 100644 > --- a/drivers/media/platform/qcom/iris/iris_core.h > +++ b/drivers/media/platform/qcom/iris/iris_core.h > @@ -107,5 +107,6 @@ struct iris_core { > > int iris_core_init(struct iris_core *core); > void iris_core_deinit(struct iris_core *core); > +extern bool video_drv_should_bind(struct device *dev, bool is_iris_driver); > > #endif > diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c > index 954cc7c0cc97..276461ade811 100644 > --- a/drivers/media/platform/qcom/iris/iris_probe.c > +++ b/drivers/media/platform/qcom/iris/iris_probe.c > @@ -196,6 +196,9 @@ static int iris_probe(struct platform_device *pdev) > u64 dma_mask; > int ret; > > + if (!video_drv_should_bind(&pdev->dev, true)) > + return -ENODEV; Wouldn't it mark the probe as failed and cause dmesg regressions? > + > core = devm_kzalloc(&pdev->dev, sizeof(*core), GFP_KERNEL); > if (!core) > return -ENOMEM; > diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c > index 77d48578ecd2..b38be7812efe 100644 > --- a/drivers/media/platform/qcom/venus/core.c > +++ b/drivers/media/platform/qcom/venus/core.c > @@ -369,12 +369,17 @@ static int venus_add_dynamic_nodes(struct venus_core *core) > static void venus_remove_dynamic_nodes(struct venus_core *core) {} > #endif > > +extern bool video_drv_should_bind(struct device *dev, bool is_iris_driver); You just defined it in the header. Why is this here? > + > static int venus_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > struct venus_core *core; > int ret; > > + if (!video_drv_should_bind(&pdev->dev, false)) > + return -ENODEV; Same problems - d,esg regression. > + > core = devm_kzalloc(dev, sizeof(*core), GFP_KERNEL); > if (!core) > return -ENOMEM; > diff --git a/drivers/media/platform/qcom/video_drv_helper/Makefile b/drivers/media/platform/qcom/video_drv_helper/Makefile > new file mode 100644 > index 000000000000..82567e0392fb > --- /dev/null > +++ b/drivers/media/platform/qcom/video_drv_helper/Makefile > @@ -0,0 +1,4 @@ Missing SPDX > +# Makefile for Video driver helper > + > +obj-m := video_drv_helper.o > + > diff --git a/drivers/media/platform/qcom/video_drv_helper/video_drv_helper.c b/drivers/media/platform/qcom/video_drv_helper/video_drv_helper.c > new file mode 100644 > index 000000000000..9009c2906e54 > --- /dev/null > +++ b/drivers/media/platform/qcom/video_drv_helper/video_drv_helper.c > @@ -0,0 +1,70 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#include <linux/device.h> > +#include <linux/module.h> > +#include <linux/of.h> > + > +#include "video_drv_helper.h" > + > +/* The venus driver supports only hfi gen1 to communicate with the firmware while Use Linux generic coding style comment, not netdev. > + * the iris driver supports both hfi gen1 and hfi gen2. > + * The support of hfi gen1 is added to the iris driver with the intention that > + * it can support old gen1 interface based firmware, while enabling gen2 based future SOCs. > + * With this, the plan is to migrate older SOCs from venus to iris. > + * As of now, since the iris driver supports only entry level features and doesn't have > + * feature parity with the venus driver, a runtime-selection is provided to user via > + * module parameter 'prefer_venus' to select the driver. > + * This selection is available only for the SoCs which are supported by both venus > + * and iris eg: SM8250. > + * When the feature parity is achieved, the plan is to switch the default to point to > + * the iris driver, then gradually start removing platforms from venus. > + * Hardware supported by only venus - 8916, 8996, SDM660, SDM845, SC7180, SC7280 > + * Hardware supported by only iris - SM8550 > + * Hardware supported by both venus and iris - SM8250 > + */ > + > +#if !IS_REACHABLE(CONFIG_VIDEO_QCOM_VENUS) || !IS_REACHABLE(CONFIG_VIDEO_QCOM_IRIS) > +bool video_drv_should_bind(struct device *dev, bool is_iris_driver) > +{ > + /* If just a single driver is enabled, use it no matter what */ > + return true; > +} > + > +#else > +static bool prefer_venus = true; > +MODULE_PARM_DESC(prefer_venus, "Select whether venus or iris driver should be preferred"); > +module_param(prefer_venus, bool, 0444); The choice of driver is by module blacklisting, not by failing probes. I don't understand why this patchset is needed and neither commit msg nor above longer code comment explain me that. Just blacklist the module. Best regards, Krzysztof
On Wed, Jan 29, 2025 at 03:23:22PM +0530, Dikshita Agarwal wrote: > > > On 1/28/2025 9:44 PM, Dmitry Baryshkov wrote: > > On Tue, Jan 28, 2025 at 01:34:28PM +0530, Dikshita Agarwal wrote: > >> Introduce a helper module with a kernel param to select between > >> venus and iris drivers for platforms supported by both drivers. > >> > >> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com> > >> --- > >> drivers/media/platform/qcom/Makefile | 1 + > >> drivers/media/platform/qcom/iris/iris_core.h | 1 + > >> drivers/media/platform/qcom/iris/iris_probe.c | 3 + > >> drivers/media/platform/qcom/venus/core.c | 5 ++ > >> .../platform/qcom/video_drv_helper/Makefile | 4 ++ > >> .../qcom/video_drv_helper/video_drv_helper.c | 70 +++++++++++++++++++ > >> .../qcom/video_drv_helper/video_drv_helper.h | 11 +++ > >> 7 files changed, 95 insertions(+) > >> create mode 100644 drivers/media/platform/qcom/video_drv_helper/Makefile > >> create mode 100644 drivers/media/platform/qcom/video_drv_helper/video_drv_helper.c > >> create mode 100644 drivers/media/platform/qcom/video_drv_helper/video_drv_helper.h > >> > >> diff --git a/drivers/media/platform/qcom/Makefile b/drivers/media/platform/qcom/Makefile > >> index ea2221a202c0..15accde3bd67 100644 > >> --- a/drivers/media/platform/qcom/Makefile > >> +++ b/drivers/media/platform/qcom/Makefile > >> @@ -2,3 +2,4 @@ > >> obj-y += camss/ > >> obj-y += iris/ > >> obj-y += venus/ > >> +obj-y += video_drv_helper/ > >> diff --git a/drivers/media/platform/qcom/iris/iris_core.h b/drivers/media/platform/qcom/iris/iris_core.h > >> index 37fb4919fecc..7108e751ff88 100644 > >> --- a/drivers/media/platform/qcom/iris/iris_core.h > >> +++ b/drivers/media/platform/qcom/iris/iris_core.h > >> @@ -107,5 +107,6 @@ struct iris_core { > >> > >> int iris_core_init(struct iris_core *core); > >> void iris_core_deinit(struct iris_core *core); > >> +extern bool video_drv_should_bind(struct device *dev, bool is_iris_driver); > > > > s/extern //g > > > >> > >> #endif > >> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c > >> index 954cc7c0cc97..276461ade811 100644 > >> --- a/drivers/media/platform/qcom/iris/iris_probe.c > >> +++ b/drivers/media/platform/qcom/iris/iris_probe.c > >> @@ -196,6 +196,9 @@ static int iris_probe(struct platform_device *pdev) > >> u64 dma_mask; > >> int ret; > >> > >> + if (!video_drv_should_bind(&pdev->dev, true)) > >> + return -ENODEV; > >> + > >> core = devm_kzalloc(&pdev->dev, sizeof(*core), GFP_KERNEL); > >> if (!core) > >> return -ENOMEM; > >> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c > >> index 77d48578ecd2..b38be7812efe 100644 > >> --- a/drivers/media/platform/qcom/venus/core.c > >> +++ b/drivers/media/platform/qcom/venus/core.c > >> @@ -369,12 +369,17 @@ static int venus_add_dynamic_nodes(struct venus_core *core) > >> static void venus_remove_dynamic_nodes(struct venus_core *core) {} > >> #endif > >> > >> +extern bool video_drv_should_bind(struct device *dev, bool is_iris_driver); > > > > Use #include instead. > > > >> + > >> static int venus_probe(struct platform_device *pdev) > >> { > >> struct device *dev = &pdev->dev; > >> struct venus_core *core; > >> int ret; > >> > >> + if (!video_drv_should_bind(&pdev->dev, false)) > >> + return -ENODEV; > >> + > >> core = devm_kzalloc(dev, sizeof(*core), GFP_KERNEL); > >> if (!core) > >> return -ENOMEM; > >> diff --git a/drivers/media/platform/qcom/video_drv_helper/Makefile b/drivers/media/platform/qcom/video_drv_helper/Makefile > >> new file mode 100644 > >> index 000000000000..82567e0392fb > >> --- /dev/null > >> +++ b/drivers/media/platform/qcom/video_drv_helper/Makefile > >> @@ -0,0 +1,4 @@ > >> +# Makefile for Video driver helper > >> + > >> +obj-m := video_drv_helper.o > > > > Always built as a module? And what if iris or venus are built into the > > kernel? > iris and venus are always built as module, This is not correct. > and if we are adding the > dependency of this module on IRIS && VENUS then this can't be Y I think. It surely can. Moreover, if one doesn't enable both Iris and Venus, this module is completely unnecessary. > > Add a normal Kconfig symbol, tristate, no Kconfig string. Use depends on > > IRIS && VENUS (and maybe default y) to let it be built only if both > > drivers are enabled.
On 1/29/2025 2:44 AM, Krzysztof Kozlowski wrote: > On 28/01/2025 09:04, Dikshita Agarwal wrote: >> Introduce a helper module with a kernel param to select between >> venus and iris drivers for platforms supported by both drivers. >> >> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com> >> --- >> drivers/media/platform/qcom/Makefile | 1 + >> drivers/media/platform/qcom/iris/iris_core.h | 1 + >> drivers/media/platform/qcom/iris/iris_probe.c | 3 + >> drivers/media/platform/qcom/venus/core.c | 5 ++ >> .../platform/qcom/video_drv_helper/Makefile | 4 ++ >> .../qcom/video_drv_helper/video_drv_helper.c | 70 +++++++++++++++++++ >> .../qcom/video_drv_helper/video_drv_helper.h | 11 +++ >> 7 files changed, 95 insertions(+) >> create mode 100644 drivers/media/platform/qcom/video_drv_helper/Makefile >> create mode 100644 drivers/media/platform/qcom/video_drv_helper/video_drv_helper.c >> create mode 100644 drivers/media/platform/qcom/video_drv_helper/video_drv_helper.h >> >> diff --git a/drivers/media/platform/qcom/Makefile b/drivers/media/platform/qcom/Makefile >> index ea2221a202c0..15accde3bd67 100644 >> --- a/drivers/media/platform/qcom/Makefile >> +++ b/drivers/media/platform/qcom/Makefile >> @@ -2,3 +2,4 @@ >> obj-y += camss/ >> obj-y += iris/ >> obj-y += venus/ >> +obj-y += video_drv_helper/ >> diff --git a/drivers/media/platform/qcom/iris/iris_core.h b/drivers/media/platform/qcom/iris/iris_core.h >> index 37fb4919fecc..7108e751ff88 100644 >> --- a/drivers/media/platform/qcom/iris/iris_core.h >> +++ b/drivers/media/platform/qcom/iris/iris_core.h >> @@ -107,5 +107,6 @@ struct iris_core { >> >> int iris_core_init(struct iris_core *core); >> void iris_core_deinit(struct iris_core *core); >> +extern bool video_drv_should_bind(struct device *dev, bool is_iris_driver); >> >> #endif >> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c >> index 954cc7c0cc97..276461ade811 100644 >> --- a/drivers/media/platform/qcom/iris/iris_probe.c >> +++ b/drivers/media/platform/qcom/iris/iris_probe.c >> @@ -196,6 +196,9 @@ static int iris_probe(struct platform_device *pdev) >> u64 dma_mask; >> int ret; >> >> + if (!video_drv_should_bind(&pdev->dev, true)) >> + return -ENODEV; > > Wouldn't it mark the probe as failed and cause dmesg regressions? > >> + >> core = devm_kzalloc(&pdev->dev, sizeof(*core), GFP_KERNEL); >> if (!core) >> return -ENOMEM; >> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c >> index 77d48578ecd2..b38be7812efe 100644 >> --- a/drivers/media/platform/qcom/venus/core.c >> +++ b/drivers/media/platform/qcom/venus/core.c >> @@ -369,12 +369,17 @@ static int venus_add_dynamic_nodes(struct venus_core *core) >> static void venus_remove_dynamic_nodes(struct venus_core *core) {} >> #endif >> >> +extern bool video_drv_should_bind(struct device *dev, bool is_iris_driver); > > You just defined it in the header. Why is this here? > >> + >> static int venus_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> struct venus_core *core; >> int ret; >> >> + if (!video_drv_should_bind(&pdev->dev, false)) >> + return -ENODEV; > > Same problems - d,esg regression. > >> + >> core = devm_kzalloc(dev, sizeof(*core), GFP_KERNEL); >> if (!core) >> return -ENOMEM; >> diff --git a/drivers/media/platform/qcom/video_drv_helper/Makefile b/drivers/media/platform/qcom/video_drv_helper/Makefile >> new file mode 100644 >> index 000000000000..82567e0392fb >> --- /dev/null >> +++ b/drivers/media/platform/qcom/video_drv_helper/Makefile >> @@ -0,0 +1,4 @@ > > Missing SPDX > >> +# Makefile for Video driver helper >> + >> +obj-m := video_drv_helper.o >> + >> diff --git a/drivers/media/platform/qcom/video_drv_helper/video_drv_helper.c b/drivers/media/platform/qcom/video_drv_helper/video_drv_helper.c >> new file mode 100644 >> index 000000000000..9009c2906e54 >> --- /dev/null >> +++ b/drivers/media/platform/qcom/video_drv_helper/video_drv_helper.c >> @@ -0,0 +1,70 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved. >> + */ >> + >> +#include <linux/device.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> + >> +#include "video_drv_helper.h" >> + >> +/* The venus driver supports only hfi gen1 to communicate with the firmware while > > Use Linux generic coding style comment, not netdev. > >> + * the iris driver supports both hfi gen1 and hfi gen2. >> + * The support of hfi gen1 is added to the iris driver with the intention that >> + * it can support old gen1 interface based firmware, while enabling gen2 based future SOCs. >> + * With this, the plan is to migrate older SOCs from venus to iris. >> + * As of now, since the iris driver supports only entry level features and doesn't have >> + * feature parity with the venus driver, a runtime-selection is provided to user via >> + * module parameter 'prefer_venus' to select the driver. >> + * This selection is available only for the SoCs which are supported by both venus >> + * and iris eg: SM8250. >> + * When the feature parity is achieved, the plan is to switch the default to point to >> + * the iris driver, then gradually start removing platforms from venus. >> + * Hardware supported by only venus - 8916, 8996, SDM660, SDM845, SC7180, SC7280 >> + * Hardware supported by only iris - SM8550 >> + * Hardware supported by both venus and iris - SM8250 >> + */ >> + >> +#if !IS_REACHABLE(CONFIG_VIDEO_QCOM_VENUS) || !IS_REACHABLE(CONFIG_VIDEO_QCOM_IRIS) >> +bool video_drv_should_bind(struct device *dev, bool is_iris_driver) >> +{ >> + /* If just a single driver is enabled, use it no matter what */ >> + return true; >> +} >> + >> +#else >> +static bool prefer_venus = true; >> +MODULE_PARM_DESC(prefer_venus, "Select whether venus or iris driver should be preferred"); >> +module_param(prefer_venus, bool, 0444); > > > The choice of driver is by module blacklisting, not by failing probes. > > I don't understand why this patchset is needed and neither commit msg > nor above longer code comment explain me that. Just blacklist the module. > > Best regards, > Krzysztof Summarizing the discussion with myself, Krzysztof and Dmitry: 1) module blacklisting solution will not be ideal if users want to have both venus and iris or either of them built-in 2) with current approach, one of the probes (either venus or iris) will certainly fail as video_drv_should_bind() will fail for one of them. This can be considered as a regression and should not happen. Solution: If the user prefers iris driver and iris driver has not probed yet, and if venus tries to probe ahead of iris we keep -EDEFERing till iris probes and succeeds. Vice-versa when the preference is venus as well.
On Fri, Jan 31, 2025 at 10:44:28AM -0800, Abhinav Kumar wrote: > On 1/29/2025 2:44 AM, Krzysztof Kozlowski wrote: > > On 28/01/2025 09:04, Dikshita Agarwal wrote: > >> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c > >> index 954cc7c0cc97..276461ade811 100644 > >> --- a/drivers/media/platform/qcom/iris/iris_probe.c > >> +++ b/drivers/media/platform/qcom/iris/iris_probe.c > >> @@ -196,6 +196,9 @@ static int iris_probe(struct platform_device *pdev) > >> u64 dma_mask; > >> int ret; > >> > >> + if (!video_drv_should_bind(&pdev->dev, true)) > >> + return -ENODEV; > > > > Wouldn't it mark the probe as failed and cause dmesg regressions? No, this is perfectly fine. Probe can return -ENODEV and driver core will continue with any further matches. > >> +#if !IS_REACHABLE(CONFIG_VIDEO_QCOM_VENUS) || !IS_REACHABLE(CONFIG_VIDEO_QCOM_IRIS) > >> +bool video_drv_should_bind(struct device *dev, bool is_iris_driver) > >> +{ > >> + /* If just a single driver is enabled, use it no matter what */ > >> + return true; > >> +} > >> + > >> +#else > >> +static bool prefer_venus = true; > >> +MODULE_PARM_DESC(prefer_venus, "Select whether venus or iris driver should be preferred"); > >> +module_param(prefer_venus, bool, 0444); > > > > > > The choice of driver is by module blacklisting, not by failing probes. > > > > I don't understand why this patchset is needed and neither commit msg > > nor above longer code comment explain me that. Just blacklist the module. > Summarizing the discussion with myself, Krzysztof and Dmitry: > > 1) module blacklisting solution will not be ideal if users want to have > both venus and iris or either of them built-in Module blacklisting is not the way to go, you shouldn't have two drivers racing to bind to the same device ever. > 2) with current approach, one of the probes (either venus or iris) will > certainly fail as video_drv_should_bind() will fail for one of them. > This can be considered as a regression and should not happen. How can that be a regression? One driver must fail to probe (see above). > Solution: If the user prefers iris driver and iris driver has not probed > yet, and if venus tries to probe ahead of iris we keep -EDEFERing till > iris probes and succeeds. Vice-versa when the preference is venus as well. This sounds wrong too. Look, first you guys need to explain *why* you want to have two drivers for the same hardware (not just to me, in the commit message and cover letter). That's something that really should never be the case and would need to be motivated properly. Second, if the reasons for keeping both drivers are deemed justifiable, you need to come up with mechanism for only binding one of them. I already told you that module parameters is not the way to go here (and the msm drm driver's abuse of module parameters is not a good precedent here). If this is a transitional thing (which it must be), then just add a Kconfig symbol to determine which driver should probe. That's good enough for evaluating whatever needs to be evaluated, and doesn't depend on adding anti-patterns like module parameters (and helper modules for them). Keep it simple. Johan
On Mon, Feb 03, 2025 at 09:22:51AM +0100, Johan Hovold wrote: > On Fri, Jan 31, 2025 at 10:44:28AM -0800, Abhinav Kumar wrote: > > On 1/29/2025 2:44 AM, Krzysztof Kozlowski wrote: > > > On 28/01/2025 09:04, Dikshita Agarwal wrote: > > > >> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c > > >> index 954cc7c0cc97..276461ade811 100644 > > >> --- a/drivers/media/platform/qcom/iris/iris_probe.c > > >> +++ b/drivers/media/platform/qcom/iris/iris_probe.c > > >> @@ -196,6 +196,9 @@ static int iris_probe(struct platform_device *pdev) > > >> u64 dma_mask; > > >> int ret; > > >> > > >> + if (!video_drv_should_bind(&pdev->dev, true)) > > >> + return -ENODEV; > > > > > > Wouldn't it mark the probe as failed and cause dmesg regressions? > > No, this is perfectly fine. Probe can return -ENODEV and driver core > will continue with any further matches. > > > >> +#if !IS_REACHABLE(CONFIG_VIDEO_QCOM_VENUS) || !IS_REACHABLE(CONFIG_VIDEO_QCOM_IRIS) > > >> +bool video_drv_should_bind(struct device *dev, bool is_iris_driver) > > >> +{ > > >> + /* If just a single driver is enabled, use it no matter what */ > > >> + return true; > > >> +} > > >> + > > >> +#else > > >> +static bool prefer_venus = true; > > >> +MODULE_PARM_DESC(prefer_venus, "Select whether venus or iris driver should be preferred"); > > >> +module_param(prefer_venus, bool, 0444); > > > > > > > > > The choice of driver is by module blacklisting, not by failing probes. > > > > > > I don't understand why this patchset is needed and neither commit msg > > > nor above longer code comment explain me that. Just blacklist the module. > > > Summarizing the discussion with myself, Krzysztof and Dmitry: > > > > 1) module blacklisting solution will not be ideal if users want to have > > both venus and iris or either of them built-in > > Module blacklisting is not the way to go, you shouldn't have two drivers > racing to bind to the same device ever. > > > 2) with current approach, one of the probes (either venus or iris) will > > certainly fail as video_drv_should_bind() will fail for one of them. > > This can be considered as a regression and should not happen. > > How can that be a regression? One driver must fail to probe (see above). I also don't think that it's a regression. I think Krzysztof was concerned about the 'failed to bind' messages in dmesg. > > Solution: If the user prefers iris driver and iris driver has not probed > > yet, and if venus tries to probe ahead of iris we keep -EDEFERing till > > iris probes and succeeds. Vice-versa when the preference is venus as well. > > This sounds wrong too. > > Look, first you guys need to explain *why* you want to have two drivers > for the same hardware (not just to me, in the commit message and cover > letter). > > That's something that really should never be the case and would need to > be motivated properly. I think it has been written several time (not sure about this commit): to provide a way for a migration path _while_ people are working on Iris features. Being able to A/B test Venus and Iris drivers and to report possible regressions or lack of those on the common platforms supported by those (sm8250 at this moment). > Second, if the reasons for keeping both drivers are deemed justifiable, > you need to come up with mechanism for only binding one of them. > > I already told you that module parameters is not the way to go here (and > the msm drm driver's abuse of module parameters is not a good precedent > here). > > If this is a transitional thing (which it must be), then just add a > Kconfig symbol to determine which driver should probe. That's good > enough for evaluating whatever needs to be evaluated, and doesn't > depend on adding anti-patterns like module parameters (and helper > modules for them). No, Kconfig complicates A/B testing. What you usually want to do is to boot a kernel, then go over a bunch of files testing that they work with both drivers. Kconfig implies booting abother kernel, etc. > > Keep it simple. > > Johan
On 03/02/2025 16:16, Dmitry Baryshkov wrote: > On Mon, Feb 03, 2025 at 09:22:51AM +0100, Johan Hovold wrote: >> On Fri, Jan 31, 2025 at 10:44:28AM -0800, Abhinav Kumar wrote: >>> On 1/29/2025 2:44 AM, Krzysztof Kozlowski wrote: >>>> On 28/01/2025 09:04, Dikshita Agarwal wrote: >> >>>>> diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c >>>>> index 954cc7c0cc97..276461ade811 100644 >>>>> --- a/drivers/media/platform/qcom/iris/iris_probe.c >>>>> +++ b/drivers/media/platform/qcom/iris/iris_probe.c >>>>> @@ -196,6 +196,9 @@ static int iris_probe(struct platform_device *pdev) >>>>> u64 dma_mask; >>>>> int ret; >>>>> >>>>> + if (!video_drv_should_bind(&pdev->dev, true)) >>>>> + return -ENODEV; >>>> >>>> Wouldn't it mark the probe as failed and cause dmesg regressions? >> >> No, this is perfectly fine. Probe can return -ENODEV and driver core >> will continue with any further matches. >> >>>>> +#if !IS_REACHABLE(CONFIG_VIDEO_QCOM_VENUS) || !IS_REACHABLE(CONFIG_VIDEO_QCOM_IRIS) >>>>> +bool video_drv_should_bind(struct device *dev, bool is_iris_driver) >>>>> +{ >>>>> + /* If just a single driver is enabled, use it no matter what */ >>>>> + return true; >>>>> +} >>>>> + >>>>> +#else >>>>> +static bool prefer_venus = true; >>>>> +MODULE_PARM_DESC(prefer_venus, "Select whether venus or iris driver should be preferred"); >>>>> +module_param(prefer_venus, bool, 0444); >>>> >>>> >>>> The choice of driver is by module blacklisting, not by failing probes. >>>> >>>> I don't understand why this patchset is needed and neither commit msg >>>> nor above longer code comment explain me that. Just blacklist the module. >> >>> Summarizing the discussion with myself, Krzysztof and Dmitry: >>> >>> 1) module blacklisting solution will not be ideal if users want to have >>> both venus and iris or either of them built-in >> >> Module blacklisting is not the way to go, you shouldn't have two drivers >> racing to bind to the same device ever. >> >>> 2) with current approach, one of the probes (either venus or iris) will >>> certainly fail as video_drv_should_bind() will fail for one of them. >>> This can be considered as a regression and should not happen. >> >> How can that be a regression? One driver must fail to probe (see above). > > I also don't think that it's a regression. I think Krzysztof was > concerned about the 'failed to bind' messages in dmesg. I never used word "regression" alone. I said "dmesg regression", which means you have error in logs or any system facility which provides you self-information about device probe history. I don't remember if -ENODEV leads to any printks, so maybe I am wrong here, but regardless normal and expected operation of a driver should never result in -ERRNO, except deferred probe of course. Best regards, Krzysztof
On Mon, Feb 03, 2025 at 05:16:50PM +0200, Dmitry Baryshkov wrote: > On Mon, Feb 03, 2025 at 09:22:51AM +0100, Johan Hovold wrote: > > On Fri, Jan 31, 2025 at 10:44:28AM -0800, Abhinav Kumar wrote: > > > Solution: If the user prefers iris driver and iris driver has not probed > > > yet, and if venus tries to probe ahead of iris we keep -EDEFERing till > > > iris probes and succeeds. Vice-versa when the preference is venus as well. > > > > This sounds wrong too. > > > > Look, first you guys need to explain *why* you want to have two drivers > > for the same hardware (not just to me, in the commit message and cover > > letter). > > > > That's something that really should never be the case and would need to > > be motivated properly. > > I think it has been written several time (not sure about this commit): > to provide a way for a migration path _while_ people are working on Iris > features. Being able to A/B test Venus and Iris drivers and to report > possible regressions or lack of those on the common platforms supported > by those (sm8250 at this moment). You don't need a module parameter for that. And we're still waiting to hear the answers to all of Hans' questions. I still haven't seen anyone explaining why any of this is needed. You could have just continued letting Venus support 8250 so presumably there is some benefit in using Iris instead. Which? And is that potential benefit important enough to not just wait until Iris is up to par feature wise? I'm sure you have some answers, but you need to provide them as part of the series. > > Second, if the reasons for keeping both drivers are deemed justifiable, > > you need to come up with mechanism for only binding one of them. > > > > I already told you that module parameters is not the way to go here (and > > the msm drm driver's abuse of module parameters is not a good precedent > > here). > > > > If this is a transitional thing (which it must be), then just add a > > Kconfig symbol to determine which driver should probe. That's good > > enough for evaluating whatever needs to be evaluated, and doesn't > > depend on adding anti-patterns like module parameters (and helper > > modules for them). > > No, Kconfig complicates A/B testing. What you usually want to do is to > boot a kernel, then go over a bunch of files testing that they work with > both drivers. Kconfig implies booting abother kernel, etc. No, I'm pretty sure you'd even want to reboot in between so as not to rely on state left behind by the other driver. Unbinding and rebinding drivers is not part of any normal work flow expect possibly during development. And a developer can easily compare Venus and Iris for 8250 without a module parameter too. Johan
On Mon, Feb 03, 2025 at 05:34:20PM +0100, Krzysztof Kozlowski wrote: > On 03/02/2025 16:16, Dmitry Baryshkov wrote: > > On Mon, Feb 03, 2025 at 09:22:51AM +0100, Johan Hovold wrote: > >> On Fri, Jan 31, 2025 at 10:44:28AM -0800, Abhinav Kumar wrote: > >>> On 1/29/2025 2:44 AM, Krzysztof Kozlowski wrote: > >>>> On 28/01/2025 09:04, Dikshita Agarwal wrote: > >>>>> @@ -196,6 +196,9 @@ static int iris_probe(struct platform_device *pdev) > >>>>> u64 dma_mask; > >>>>> int ret; > >>>>> > >>>>> + if (!video_drv_should_bind(&pdev->dev, true)) > >>>>> + return -ENODEV; > >>>> > >>>> Wouldn't it mark the probe as failed and cause dmesg regressions? > >> > >> No, this is perfectly fine. Probe can return -ENODEV and driver core > >> will continue with any further matches. > >>> 2) with current approach, one of the probes (either venus or iris) will > >>> certainly fail as video_drv_should_bind() will fail for one of them. > >>> This can be considered as a regression and should not happen. > >> > >> How can that be a regression? One driver must fail to probe (see above). > > > > I also don't think that it's a regression. I think Krzysztof was > > concerned about the 'failed to bind' messages in dmesg. > > I never used word "regression" alone. I said "dmesg regression", which > means you have error in logs or any system facility which provides you > self-information about device probe history. I don't remember if -ENODEV > leads to any printks, so maybe I am wrong here, but regardless normal > and expected operation of a driver should never result in -ERRNO, except > deferred probe of course. A probe function returning -ENODEV is not an error and is used to signal that the driver should not bind to the device it has just probed. So that part is perfectly fine here as I mentioned above. Johan
On Tue, Feb 04, 2025 at 10:31:49AM +0100, Johan Hovold wrote: > On Mon, Feb 03, 2025 at 05:16:50PM +0200, Dmitry Baryshkov wrote: > > On Mon, Feb 03, 2025 at 09:22:51AM +0100, Johan Hovold wrote: > > > On Fri, Jan 31, 2025 at 10:44:28AM -0800, Abhinav Kumar wrote: > > > > > Solution: If the user prefers iris driver and iris driver has not probed > > > > yet, and if venus tries to probe ahead of iris we keep -EDEFERing till > > > > iris probes and succeeds. Vice-versa when the preference is venus as well. > > > > > > This sounds wrong too. > > > > > > Look, first you guys need to explain *why* you want to have two drivers > > > for the same hardware (not just to me, in the commit message and cover > > > letter). > > > > > > That's something that really should never be the case and would need to > > > be motivated properly. > > > > I think it has been written several time (not sure about this commit): > > to provide a way for a migration path _while_ people are working on Iris > > features. Being able to A/B test Venus and Iris drivers and to report > > possible regressions or lack of those on the common platforms supported > > by those (sm8250 at this moment). > > You don't need a module parameter for that. > > And we're still waiting to hear the answers to all of Hans' questions. I > still haven't seen anyone explaining why any of this is needed. You > could have just continued letting Venus support 8250 so presumably there > is some benefit in using Iris instead. Which? And is that potential > benefit important enough to not just wait until Iris is up to par > feature wise? Because that's exactly opposite of what Iris developers are trying to do: SM8250 and SM8550 belong to two different generations of the FW interface. By supporting both of them in the Iris driver the developers can verify that the internal driver abstractions are good enough. It has been discussed in one of the threads (or in telcos) related to the first iterations of this driver. We definitely don't want to end up in Venus situation, where the abstractions were added afterwards and in some cases they are not the best ones. The plan is to use sm8250 as a "bridge" between two drivers, verifying that they are on par during development, then drop sm8250 from Venus driver. Then move sc7280 support too, drop all HFD_VERSION_6XX support from Venus, cleanup the remnants. I think most of that information should have been a part of the main Iris series. If it is not, please comment there, so that those commit messages can be updated. > > I'm sure you have some answers, but you need to provide them as part of > the series. > > > > Second, if the reasons for keeping both drivers are deemed justifiable, > > > you need to come up with mechanism for only binding one of them. > > > > > > I already told you that module parameters is not the way to go here (and > > > the msm drm driver's abuse of module parameters is not a good precedent > > > here). > > > > > > If this is a transitional thing (which it must be), then just add a > > > Kconfig symbol to determine which driver should probe. That's good > > > enough for evaluating whatever needs to be evaluated, and doesn't > > > depend on adding anti-patterns like module parameters (and helper > > > modules for them). > > > > No, Kconfig complicates A/B testing. What you usually want to do is to > > boot a kernel, then go over a bunch of files testing that they work with > > both drivers. Kconfig implies booting abother kernel, etc. > > No, I'm pretty sure you'd even want to reboot in between so as not to > rely on state left behind by the other driver. As a quick test I'd definitely do not want to reboot. Both drivers completely shut down the firmware running on the core, so there is no intermediate state left between driver probes. > Unbinding and rebinding drivers is not part of any normal work flow > expect possibly during development. And a developer can easily compare > Venus and Iris for 8250 without a module parameter too. Yes, we are talking about development. And yes, modparam helps. If you'd like to do two separate kernel builds, that's fine.
On 2/4/2025 3:01 PM, Johan Hovold wrote: > On Mon, Feb 03, 2025 at 05:16:50PM +0200, Dmitry Baryshkov wrote: >> On Mon, Feb 03, 2025 at 09:22:51AM +0100, Johan Hovold wrote: >>> On Fri, Jan 31, 2025 at 10:44:28AM -0800, Abhinav Kumar wrote: > >>>> Solution: If the user prefers iris driver and iris driver has not probed >>>> yet, and if venus tries to probe ahead of iris we keep -EDEFERing till >>>> iris probes and succeeds. Vice-versa when the preference is venus as well. >>> >>> This sounds wrong too. >>> >>> Look, first you guys need to explain *why* you want to have two drivers >>> for the same hardware (not just to me, in the commit message and cover >>> letter). >>> >>> That's something that really should never be the case and would need to >>> be motivated properly. >> >> I think it has been written several time (not sure about this commit): >> to provide a way for a migration path _while_ people are working on Iris >> features. Being able to A/B test Venus and Iris drivers and to report >> possible regressions or lack of those on the common platforms supported >> by those (sm8250 at this moment). > > You don't need a module parameter for that. > > And we're still waiting to hear the answers to all of Hans' questions. I > still haven't seen anyone explaining why any of this is needed. You > could have just continued letting Venus support 8250 so presumably there > is some benefit in using Iris instead. Which? And is that potential > benefit important enough to not just wait until Iris is up to par > feature wise? They are documented in this RFC [1] as comments, and would be added in v10 as we conclude the ongoing discussion to handle 2 drivers during the transition phase. [1] https://patchwork.kernel.org/project/linux-media/patch/20250128080429.3911091-2-quic_dikshita@quicinc.com/ Regards, Vikash > > I'm sure you have some answers, but you need to provide them as part of > the series. > >>> Second, if the reasons for keeping both drivers are deemed justifiable, >>> you need to come up with mechanism for only binding one of them. >>> >>> I already told you that module parameters is not the way to go here (and >>> the msm drm driver's abuse of module parameters is not a good precedent >>> here). >>> >>> If this is a transitional thing (which it must be), then just add a >>> Kconfig symbol to determine which driver should probe. That's good >>> enough for evaluating whatever needs to be evaluated, and doesn't >>> depend on adding anti-patterns like module parameters (and helper >>> modules for them). >> >> No, Kconfig complicates A/B testing. What you usually want to do is to >> boot a kernel, then go over a bunch of files testing that they work with >> both drivers. Kconfig implies booting abother kernel, etc. > > No, I'm pretty sure you'd even want to reboot in between so as not to > rely on state left behind by the other driver. > > Unbinding and rebinding drivers is not part of any normal work flow > expect possibly during development. And a developer can easily compare > Venus and Iris for 8250 without a module parameter too. > > Johan
On Tue, Feb 04, 2025 at 04:55:58PM +0200, Dmitry Baryshkov wrote: > On Tue, Feb 04, 2025 at 10:31:49AM +0100, Johan Hovold wrote: > > On Mon, Feb 03, 2025 at 05:16:50PM +0200, Dmitry Baryshkov wrote: > > > On Mon, Feb 03, 2025 at 09:22:51AM +0100, Johan Hovold wrote: > > > > On Fri, Jan 31, 2025 at 10:44:28AM -0800, Abhinav Kumar wrote: > > And we're still waiting to hear the answers to all of Hans' questions. I > > still haven't seen anyone explaining why any of this is needed. You > > could have just continued letting Venus support 8250 so presumably there > > is some benefit in using Iris instead. Which? And is that potential > > benefit important enough to not just wait until Iris is up to par > > feature wise? > > Because that's exactly opposite of what Iris developers are trying to > do: SM8250 and SM8550 belong to two different generations of the FW > interface. By supporting both of them in the Iris driver the developers > can verify that the internal driver abstractions are good enough. It has > been discussed in one of the threads (or in telcos) related to the first > iterations of this driver. We definitely don't want to end up in Venus > situation, where the abstractions were added afterwards and in some > cases they are not the best ones. Ok, but as I've said a number of times already, information like this needs to be included in the cover letter and commit message of what is posted to the list. Maintainers and reviewers obviously have no idea what you discussed in some internal teleconference and can't be expected to remember or dig this out from some old email thread either. > The plan is to use sm8250 as a "bridge" between two drivers, verifying > that they are on par during development, then drop sm8250 from Venus > driver. Then move sc7280 support too, drop all HFD_VERSION_6XX support > from Venus, cleanup the remnants. Ok, but venus would still remain for some older hardware? It's just the "hfi gen1" ones that would move to the iris eventually? > I think most of that information should have been a part of the main > Iris series. If it is not, please comment there, so that those commit > messages can be updated. Unfortunately it was not, which I also pointed in my comments to the Iris series. > > I'm sure you have some answers, but you need to provide them as part of > > the series. > > Unbinding and rebinding drivers is not part of any normal work flow > > expect possibly during development. And a developer can easily compare > > Venus and Iris for 8250 without a module parameter too. > > Yes, we are talking about development. And yes, modparam helps. If you'd > like to do two separate kernel builds, that's fine. Please just motivate why you think this is needed as part of the submission. And make sure that the implementation is sane (e.g. not some random probe defer indefinitely thing). Like I said, having two drivers for the same hardware is normally not something that is acceptable, and this would need to be a transitional thing as we both agree. One way to guarantee that is to not expose it to regular users until it is ready (e.g. a Kconfig hidden behind CONFIG_EXPERT or similar). Otherwise, I fear you'll end up supporting both forever (with at least one of them bitrotting behind that module parameter over time). Johan
On Tue, Feb 04, 2025 at 08:38:56PM +0530, Vikash Garodia wrote: > On 2/4/2025 3:01 PM, Johan Hovold wrote: > > And we're still waiting to hear the answers to all of Hans' questions. I > > still haven't seen anyone explaining why any of this is needed. You > > could have just continued letting Venus support 8250 so presumably there > > is some benefit in using Iris instead. Which? And is that potential > > benefit important enough to not just wait until Iris is up to par > > feature wise? > They are documented in this RFC [1] as comments, and would be added in v10 as we > conclude the ongoing discussion to handle 2 drivers during the transition phase. Ah, I forgot about the comment. Most of that should probably go in the cover letter and commit messages of v10 instead (and with a more condensed comment in the code), but it does indeed seem to provide some answers. > [1] > https://patchwork.kernel.org/project/linux-media/patch/20250128080429.3911091-2-quic_dikshita@quicinc.com/ Johan
On Tue, Feb 04, 2025 at 05:00:31PM +0100, Johan Hovold wrote: > On Tue, Feb 04, 2025 at 04:55:58PM +0200, Dmitry Baryshkov wrote: > > On Tue, Feb 04, 2025 at 10:31:49AM +0100, Johan Hovold wrote: > > > On Mon, Feb 03, 2025 at 05:16:50PM +0200, Dmitry Baryshkov wrote: > > > > On Mon, Feb 03, 2025 at 09:22:51AM +0100, Johan Hovold wrote: > > > > > On Fri, Jan 31, 2025 at 10:44:28AM -0800, Abhinav Kumar wrote: > > > > And we're still waiting to hear the answers to all of Hans' questions. I > > > still haven't seen anyone explaining why any of this is needed. You > > > could have just continued letting Venus support 8250 so presumably there > > > is some benefit in using Iris instead. Which? And is that potential > > > benefit important enough to not just wait until Iris is up to par > > > feature wise? > > > > Because that's exactly opposite of what Iris developers are trying to > > do: SM8250 and SM8550 belong to two different generations of the FW > > interface. By supporting both of them in the Iris driver the developers > > can verify that the internal driver abstractions are good enough. It has > > been discussed in one of the threads (or in telcos) related to the first > > iterations of this driver. We definitely don't want to end up in Venus > > situation, where the abstractions were added afterwards and in some > > cases they are not the best ones. > > Ok, but as I've said a number of times already, information like this > needs to be included in the cover letter and commit message of what is > posted to the list. > > Maintainers and reviewers obviously have no idea what you discussed in > some internal teleconference and can't be expected to remember or dig > this out from some old email thread either. > > > The plan is to use sm8250 as a "bridge" between two drivers, verifying > > that they are on par during development, then drop sm8250 from Venus > > driver. Then move sc7280 support too, drop all HFD_VERSION_6XX support > > from Venus, cleanup the remnants. > > Ok, but venus would still remain for some older hardware? It's just the > "hfi gen1" ones that would move to the iris eventually? Yes. At least for the foreseable future. Nobody has explored an option of moving older hardware to the Iris driver. > > > I think most of that information should have been a part of the main > > Iris series. If it is not, please comment there, so that those commit > > messages can be updated. > > Unfortunately it was not, which I also pointed in my comments to the > Iris series. > > > > I'm sure you have some answers, but you need to provide them as part of > > > the series. > > > > Unbinding and rebinding drivers is not part of any normal work flow > > > expect possibly during development. And a developer can easily compare > > > Venus and Iris for 8250 without a module parameter too. > > > > Yes, we are talking about development. And yes, modparam helps. If you'd > > like to do two separate kernel builds, that's fine. > > Please just motivate why you think this is needed as part of the > submission. And make sure that the implementation is sane (e.g. not some > random probe defer indefinitely thing). > > Like I said, having two drivers for the same hardware is normally not > something that is acceptable, and this would need to be a transitional > thing as we both agree. One way to guarantee that is to not expose it to > regular users until it is ready (e.g. a Kconfig hidden behind > CONFIG_EXPERT or similar). Otherwise, I fear you'll end up supporting > both forever (with at least one of them bitrotting behind that module > parameter over time). I think I'm fine with hiding IRIS behind CONFIG_EXPERT, might be a good idea.
On 2/5/2025 4:39 AM, Dmitry Baryshkov wrote: > On Tue, Feb 04, 2025 at 05:00:31PM +0100, Johan Hovold wrote: >> On Tue, Feb 04, 2025 at 04:55:58PM +0200, Dmitry Baryshkov wrote: >>> On Tue, Feb 04, 2025 at 10:31:49AM +0100, Johan Hovold wrote: >>>> On Mon, Feb 03, 2025 at 05:16:50PM +0200, Dmitry Baryshkov wrote: >>>>> On Mon, Feb 03, 2025 at 09:22:51AM +0100, Johan Hovold wrote: >>>>>> On Fri, Jan 31, 2025 at 10:44:28AM -0800, Abhinav Kumar wrote: >> >>>> And we're still waiting to hear the answers to all of Hans' questions. I >>>> still haven't seen anyone explaining why any of this is needed. You >>>> could have just continued letting Venus support 8250 so presumably there >>>> is some benefit in using Iris instead. Which? And is that potential >>>> benefit important enough to not just wait until Iris is up to par >>>> feature wise? >>> >>> Because that's exactly opposite of what Iris developers are trying to >>> do: SM8250 and SM8550 belong to two different generations of the FW >>> interface. By supporting both of them in the Iris driver the developers >>> can verify that the internal driver abstractions are good enough. It has >>> been discussed in one of the threads (or in telcos) related to the first >>> iterations of this driver. We definitely don't want to end up in Venus >>> situation, where the abstractions were added afterwards and in some >>> cases they are not the best ones. >> >> Ok, but as I've said a number of times already, information like this >> needs to be included in the cover letter and commit message of what is >> posted to the list. >> >> Maintainers and reviewers obviously have no idea what you discussed in >> some internal teleconference and can't be expected to remember or dig >> this out from some old email thread either. >> >>> The plan is to use sm8250 as a "bridge" between two drivers, verifying >>> that they are on par during development, then drop sm8250 from Venus >>> driver. Then move sc7280 support too, drop all HFD_VERSION_6XX support >>> from Venus, cleanup the remnants. >> >> Ok, but venus would still remain for some older hardware? It's just the >> "hfi gen1" ones that would move to the iris eventually? > > Yes. At least for the foreseable future. Nobody has explored an option > of moving older hardware to the Iris driver. > >> >>> I think most of that information should have been a part of the main >>> Iris series. If it is not, please comment there, so that those commit >>> messages can be updated. >> >> Unfortunately it was not, which I also pointed in my comments to the >> Iris series. >> >>>> I'm sure you have some answers, but you need to provide them as part of >>>> the series. >> >>>> Unbinding and rebinding drivers is not part of any normal work flow >>>> expect possibly during development. And a developer can easily compare >>>> Venus and Iris for 8250 without a module parameter too. >>> >>> Yes, we are talking about development. And yes, modparam helps. If you'd >>> like to do two separate kernel builds, that's fine. >> >> Please just motivate why you think this is needed as part of the >> submission. And make sure that the implementation is sane (e.g. not some >> random probe defer indefinitely thing). >> >> Like I said, having two drivers for the same hardware is normally not >> something that is acceptable, and this would need to be a transitional >> thing as we both agree. One way to guarantee that is to not expose it to >> regular users until it is ready (e.g. a Kconfig hidden behind >> CONFIG_EXPERT or similar). Otherwise, I fear you'll end up supporting >> both forever (with at least one of them bitrotting behind that module >> parameter over time). > > I think I'm fine with hiding IRIS behind CONFIG_EXPERT, might be a good > idea. Are you suggesting to add a dependency on CONFIG_EXPERT for IRIS driver? Something like: config VIDEO_QCOM_IRIS tristate "Qualcomm iris V4L2 decoder driver" depends on EXPERT This will impact the enablement of iris driver on SM8550 as well. And will this also be needed to be captured in cover letter? Thanks, Dikshita >
On Wed, Feb 05, 2025 at 11:47:12AM +0530, Dikshita Agarwal wrote: > On 2/5/2025 4:39 AM, Dmitry Baryshkov wrote: > > On Tue, Feb 04, 2025 at 05:00:31PM +0100, Johan Hovold wrote: > >> On Tue, Feb 04, 2025 at 04:55:58PM +0200, Dmitry Baryshkov wrote: > >>> On Tue, Feb 04, 2025 at 10:31:49AM +0100, Johan Hovold wrote: > >>>> Unbinding and rebinding drivers is not part of any normal work flow > >>>> expect possibly during development. And a developer can easily compare > >>>> Venus and Iris for 8250 without a module parameter too. > >>> > >>> Yes, we are talking about development. And yes, modparam helps. If you'd > >>> like to do two separate kernel builds, that's fine. > >> > >> Please just motivate why you think this is needed as part of the > >> submission. And make sure that the implementation is sane (e.g. not some > >> random probe defer indefinitely thing). > >> > >> Like I said, having two drivers for the same hardware is normally not > >> something that is acceptable, and this would need to be a transitional > >> thing as we both agree. One way to guarantee that is to not expose it to > >> regular users until it is ready (e.g. a Kconfig hidden behind > >> CONFIG_EXPERT or similar). Otherwise, I fear you'll end up supporting > >> both forever (with at least one of them bitrotting behind that module > >> parameter over time). > > > > I think I'm fine with hiding IRIS behind CONFIG_EXPERT, might be a good > > idea. I was only thinking about the (experimental) support for hardware already supported by venus (i.e. initially 8250) and possibly the module parameter if you think that is essential. I can imagine the feature set becoming non-overlapping overtime so that you end up with some users depending on venus and some depending on iris for the same hardware if you expose iris support prematurely. > Are you suggesting to add a dependency on CONFIG_EXPERT for IRIS driver? > Something like: > config VIDEO_QCOM_IRIS > tristate "Qualcomm iris V4L2 decoder driver" > depends on EXPERT > > This will impact the enablement of iris driver on SM8550 as well. Right, it would, and that is not necessarily right. > And will this also be needed to be captured in cover letter? Yes, whatever solution you end up with need to be described and motivated in the cover letter (and commit messages) so that the reasoning can be evaluated. Johan
diff --git a/drivers/media/platform/qcom/Makefile b/drivers/media/platform/qcom/Makefile index ea2221a202c0..15accde3bd67 100644 --- a/drivers/media/platform/qcom/Makefile +++ b/drivers/media/platform/qcom/Makefile @@ -2,3 +2,4 @@ obj-y += camss/ obj-y += iris/ obj-y += venus/ +obj-y += video_drv_helper/ diff --git a/drivers/media/platform/qcom/iris/iris_core.h b/drivers/media/platform/qcom/iris/iris_core.h index 37fb4919fecc..7108e751ff88 100644 --- a/drivers/media/platform/qcom/iris/iris_core.h +++ b/drivers/media/platform/qcom/iris/iris_core.h @@ -107,5 +107,6 @@ struct iris_core { int iris_core_init(struct iris_core *core); void iris_core_deinit(struct iris_core *core); +extern bool video_drv_should_bind(struct device *dev, bool is_iris_driver); #endif diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c index 954cc7c0cc97..276461ade811 100644 --- a/drivers/media/platform/qcom/iris/iris_probe.c +++ b/drivers/media/platform/qcom/iris/iris_probe.c @@ -196,6 +196,9 @@ static int iris_probe(struct platform_device *pdev) u64 dma_mask; int ret; + if (!video_drv_should_bind(&pdev->dev, true)) + return -ENODEV; + core = devm_kzalloc(&pdev->dev, sizeof(*core), GFP_KERNEL); if (!core) return -ENOMEM; diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c index 77d48578ecd2..b38be7812efe 100644 --- a/drivers/media/platform/qcom/venus/core.c +++ b/drivers/media/platform/qcom/venus/core.c @@ -369,12 +369,17 @@ static int venus_add_dynamic_nodes(struct venus_core *core) static void venus_remove_dynamic_nodes(struct venus_core *core) {} #endif +extern bool video_drv_should_bind(struct device *dev, bool is_iris_driver); + static int venus_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; struct venus_core *core; int ret; + if (!video_drv_should_bind(&pdev->dev, false)) + return -ENODEV; + core = devm_kzalloc(dev, sizeof(*core), GFP_KERNEL); if (!core) return -ENOMEM; diff --git a/drivers/media/platform/qcom/video_drv_helper/Makefile b/drivers/media/platform/qcom/video_drv_helper/Makefile new file mode 100644 index 000000000000..82567e0392fb --- /dev/null +++ b/drivers/media/platform/qcom/video_drv_helper/Makefile @@ -0,0 +1,4 @@ +# Makefile for Video driver helper + +obj-m := video_drv_helper.o + diff --git a/drivers/media/platform/qcom/video_drv_helper/video_drv_helper.c b/drivers/media/platform/qcom/video_drv_helper/video_drv_helper.c new file mode 100644 index 000000000000..9009c2906e54 --- /dev/null +++ b/drivers/media/platform/qcom/video_drv_helper/video_drv_helper.c @@ -0,0 +1,70 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved. + */ + +#include <linux/device.h> +#include <linux/module.h> +#include <linux/of.h> + +#include "video_drv_helper.h" + +/* The venus driver supports only hfi gen1 to communicate with the firmware while + * the iris driver supports both hfi gen1 and hfi gen2. + * The support of hfi gen1 is added to the iris driver with the intention that + * it can support old gen1 interface based firmware, while enabling gen2 based future SOCs. + * With this, the plan is to migrate older SOCs from venus to iris. + * As of now, since the iris driver supports only entry level features and doesn't have + * feature parity with the venus driver, a runtime-selection is provided to user via + * module parameter 'prefer_venus' to select the driver. + * This selection is available only for the SoCs which are supported by both venus + * and iris eg: SM8250. + * When the feature parity is achieved, the plan is to switch the default to point to + * the iris driver, then gradually start removing platforms from venus. + * Hardware supported by only venus - 8916, 8996, SDM660, SDM845, SC7180, SC7280 + * Hardware supported by only iris - SM8550 + * Hardware supported by both venus and iris - SM8250 + */ + +#if !IS_REACHABLE(CONFIG_VIDEO_QCOM_VENUS) || !IS_REACHABLE(CONFIG_VIDEO_QCOM_IRIS) +bool video_drv_should_bind(struct device *dev, bool is_iris_driver) +{ + /* If just a single driver is enabled, use it no matter what */ + return true; +} + +#else +static bool prefer_venus = true; +MODULE_PARM_DESC(prefer_venus, "Select whether venus or iris driver should be preferred"); +module_param(prefer_venus, bool, 0444); + +/* list all platforms supported by both venus and iris drivers */ +static const char *const venus_to_iris_migration[] = { + "qcom,sm8250-venus", + NULL, +}; + +bool video_drv_should_bind(struct device *dev, bool is_iris_driver) +{ + if (of_device_compatible_match(dev->of_node, venus_to_iris_migration)) + return prefer_venus ? !is_iris_driver : is_iris_driver; + + return true; +} +EXPORT_SYMBOL_GPL(video_drv_should_bind); +#endif + +static int __init video_drv_helper_init(void) +{ + return 0; +} + +static void __exit video_drv_helper_exit(void) +{ +} + +module_init(video_drv_helper_init); +module_exit(video_drv_helper_exit); + +MODULE_DESCRIPTION("A video driver helper module"); +MODULE_LICENSE("GPL"); diff --git a/drivers/media/platform/qcom/video_drv_helper/video_drv_helper.h b/drivers/media/platform/qcom/video_drv_helper/video_drv_helper.h new file mode 100644 index 000000000000..6d835227fec2 --- /dev/null +++ b/drivers/media/platform/qcom/video_drv_helper/video_drv_helper.h @@ -0,0 +1,11 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved. + */ + +#ifndef __VIDEO_DRV_HELPER_H__ +#define __VIDEO_DRV_HELPER_H__ + +bool video_drv_should_bind(struct device *dev, bool is_iris_driver); + +#endif
Introduce a helper module with a kernel param to select between venus and iris drivers for platforms supported by both drivers. Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com> --- drivers/media/platform/qcom/Makefile | 1 + drivers/media/platform/qcom/iris/iris_core.h | 1 + drivers/media/platform/qcom/iris/iris_probe.c | 3 + drivers/media/platform/qcom/venus/core.c | 5 ++ .../platform/qcom/video_drv_helper/Makefile | 4 ++ .../qcom/video_drv_helper/video_drv_helper.c | 70 +++++++++++++++++++ .../qcom/video_drv_helper/video_drv_helper.h | 11 +++ 7 files changed, 95 insertions(+) create mode 100644 drivers/media/platform/qcom/video_drv_helper/Makefile create mode 100644 drivers/media/platform/qcom/video_drv_helper/video_drv_helper.c create mode 100644 drivers/media/platform/qcom/video_drv_helper/video_drv_helper.h