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

[Dikshita Agarwal <quic_dikshita@quicinc.com>]

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
>>
>