Re: [RFC PATCH v10 1/2] media: iris: introduce helper module to select video driver

Dmitry Baryshkov
2025-01-29T11:27:36Z

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.

--
With best wishes
Dmitry