| Message ID | 20250128080429.3911091-1-quic_dikshita@quicinc.com |
|---|---|
| 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 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.