[RFC PATCH v10 0/2] Add helper module to select video driver

[RFC PATCH v10 0/2] Add helper module to select video driver

[Dikshita Agarwal]

This is a RFC patch series from the original iris driver [1].
The first patch introduces a helper module to select between venus and iris
driver for plarform supported by both drivers.
The second patch enables the probe of SM8250 with iris driver.

This is just a RFC series, will send a proper v10 of iris driver
including these two patches, once everyone is OK with the changes.

[1]: https://lore.kernel.org/linux-media/20241212-qcom-video-iris-v9-0-e8c2c6bd4041@quicinc.com/

Dikshita Agarwal (2):
  media: iris: introduce helper module to select video driver
  media: iris: enable video driver probe of SM8250 SoC

 drivers/media/platform/qcom/Makefile          |   1 +
 drivers/media/platform/qcom/iris/Makefile     |   1 +
 drivers/media/platform/qcom/iris/iris_core.h  |   1 +
 .../platform/qcom/iris/iris_platform_common.h |   1 +
 .../platform/qcom/iris/iris_platform_sm8250.c | 148 ++++++++++++++++++
 drivers/media/platform/qcom/iris/iris_probe.c |   7 +
 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 ++
 10 files changed, 249 insertions(+)
 create mode 100644 drivers/media/platform/qcom/iris/iris_platform_sm8250.c
 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

-- 
2.34.1

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

[Dikshita Agarwal]

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;
+
 	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
-- 
2.34.1

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

[Dmitry Baryshkov]

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
> 

-- 
With best wishes
Dmitry

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

[Dikshita Agarwal]

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

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

[Dmitry Baryshkov]

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

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

[Krzysztof Kozlowski]

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

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

[Abhinav Kumar]

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.

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

[Johan Hovold]

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

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

[Dmitry Baryshkov]

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

-- 
With best wishes
Dmitry

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

[Krzysztof Kozlowski]

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

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

[Johan Hovold]

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

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

[Johan Hovold]

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

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

[Dmitry Baryshkov]

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.

-- 
With best wishes
Dmitry

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

[Johan Hovold]

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

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

[Dmitry Baryshkov]

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.

-- 
With best wishes
Dmitry

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

[Dikshita Agarwal]

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
> 

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

[Johan Hovold]

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

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

[Vikash Garodia]

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

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

[Johan Hovold]

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

[RFC PATCH v10 2/2] media: iris: enable video driver probe of SM8250 SoC

[Dikshita Agarwal]

Initialize the platform data and enable video driver probe of SM8250
SoC.

Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
---
 drivers/media/platform/qcom/iris/Makefile     |   1 +
 .../platform/qcom/iris/iris_platform_common.h |   1 +
 .../platform/qcom/iris/iris_platform_sm8250.c | 148 ++++++++++++++++++
 drivers/media/platform/qcom/iris/iris_probe.c |   4 +
 4 files changed, 154 insertions(+)
 create mode 100644 drivers/media/platform/qcom/iris/iris_platform_sm8250.c

diff --git a/drivers/media/platform/qcom/iris/Makefile b/drivers/media/platform/qcom/iris/Makefile
index ca31db847273..a746681e03cd 100644
--- a/drivers/media/platform/qcom/iris/Makefile
+++ b/drivers/media/platform/qcom/iris/Makefile
@@ -9,6 +9,7 @@ iris-objs += iris_buffer.o \
              iris_hfi_gen2_packet.o \
              iris_hfi_gen2_response.o \
              iris_hfi_queue.o \
+             iris_platform_sm8250.o \
              iris_platform_sm8550.o \
              iris_power.o \
              iris_probe.o \
diff --git a/drivers/media/platform/qcom/iris/iris_platform_common.h b/drivers/media/platform/qcom/iris/iris_platform_common.h
index 189dd081ad0a..f6b15d2805fb 100644
--- a/drivers/media/platform/qcom/iris/iris_platform_common.h
+++ b/drivers/media/platform/qcom/iris/iris_platform_common.h
@@ -33,6 +33,7 @@ enum pipe_type {
 	PIPE_4 = 4,
 };
 
+extern struct iris_platform_data sm8250_data;
 extern struct iris_platform_data sm8550_data;
 
 enum platform_clk_type {
diff --git a/drivers/media/platform/qcom/iris/iris_platform_sm8250.c b/drivers/media/platform/qcom/iris/iris_platform_sm8250.c
new file mode 100644
index 000000000000..360be452c1fa
--- /dev/null
+++ b/drivers/media/platform/qcom/iris/iris_platform_sm8250.c
@@ -0,0 +1,148 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include "iris_core.h"
+#include "iris_ctrls.h"
+#include "iris_platform_common.h"
+#include "iris_resources.h"
+#include "iris_hfi_gen1.h"
+#include "iris_hfi_gen1_defines.h"
+#include "iris_vpu_common.h"
+
+static struct platform_inst_fw_cap inst_fw_cap_sm8250[] = {
+	{
+		.cap_id = PIPE,
+		.min = PIPE_1,
+		.max = PIPE_4,
+		.step_or_mask = 1,
+		.value = PIPE_4,
+		.hfi_id = HFI_PROPERTY_PARAM_WORK_ROUTE,
+		.set = iris_set_pipe,
+	},
+	{
+		.cap_id = STAGE,
+		.min = STAGE_1,
+		.max = STAGE_2,
+		.step_or_mask = 1,
+		.value = STAGE_2,
+		.hfi_id = HFI_PROPERTY_PARAM_WORK_MODE,
+		.set = iris_set_stage,
+	},
+	{
+		.cap_id = DEBLOCK,
+		.min = 0,
+		.max = 1,
+		.step_or_mask = 1,
+		.value = 0,
+		.hfi_id = HFI_PROPERTY_CONFIG_VDEC_POST_LOOP_DEBLOCKER,
+		.set = iris_set_u32,
+	},
+};
+
+static struct platform_inst_caps platform_inst_cap_sm8250 = {
+	.min_frame_width = 128,
+	.max_frame_width = 8192,
+	.min_frame_height = 128,
+	.max_frame_height = 8192,
+	.max_mbpf = 138240,
+	.mb_cycles_vsp = 25,
+	.mb_cycles_vpp = 200,
+};
+
+static void iris_set_sm8250_preset_registers(struct iris_core *core)
+{
+	writel(0x0, core->reg_base + 0xB0088);
+}
+
+static const struct icc_info sm8250_icc_table[] = {
+	{ "cpu-cfg",    1000, 1000     },
+	{ "video-mem",  1000, 15000000 },
+};
+
+static const char * const sm8250_clk_reset_table[] = { "bus", "core" };
+
+static const struct bw_info sm8250_bw_table_dec[] = {
+	{ ((4096 * 2160) / 256) * 60, 2403000 },
+	{ ((4096 * 2160) / 256) * 30, 1224000 },
+	{ ((1920 * 1080) / 256) * 60,  812000 },
+	{ ((1920 * 1080) / 256) * 30,  416000 },
+};
+
+static const char * const sm8250_pmdomain_table[] = { "venus", "vcodec0" };
+
+static const char * const sm8250_opp_pd_table[] = { "mx" };
+
+static const struct platform_clk_data sm8250_clk_table[] = {
+	{IRIS_AXI_CLK,  "iface"        },
+	{IRIS_CTRL_CLK, "core"         },
+	{IRIS_HW_CLK,   "vcodec0_core" },
+};
+
+static struct tz_cp_config tz_cp_config_sm8250 = {
+	.cp_start = 0,
+	.cp_size = 0x25800000,
+	.cp_nonpixel_start = 0x01000000,
+	.cp_nonpixel_size = 0x24800000,
+};
+
+static const u32 sm8250_vdec_input_config_param_default[] = {
+	HFI_PROPERTY_CONFIG_VIDEOCORES_USAGE,
+	HFI_PROPERTY_PARAM_UNCOMPRESSED_FORMAT_SELECT,
+	HFI_PROPERTY_PARAM_UNCOMPRESSED_PLANE_ACTUAL_CONSTRAINTS_INFO,
+	HFI_PROPERTY_PARAM_BUFFER_COUNT_ACTUAL,
+	HFI_PROPERTY_PARAM_VDEC_MULTI_STREAM,
+	HFI_PROPERTY_PARAM_FRAME_SIZE,
+	HFI_PROPERTY_PARAM_BUFFER_SIZE_ACTUAL,
+	HFI_PROPERTY_PARAM_BUFFER_ALLOC_MODE,
+};
+
+static const u32 sm8250_dec_ip_int_buf_tbl[] = {
+	BUF_BIN,
+	BUF_SCRATCH_1,
+};
+
+static const u32 sm8250_dec_op_int_buf_tbl[] = {
+	BUF_DPB,
+};
+
+struct iris_platform_data sm8250_data = {
+	.get_instance = iris_hfi_gen1_get_instance,
+	.init_hfi_command_ops = &iris_hfi_gen1_command_ops_init,
+	.init_hfi_response_ops = iris_hfi_gen1_response_ops_init,
+	.vpu_ops = &iris_vpu2_ops,
+	.set_preset_registers = iris_set_sm8250_preset_registers,
+	.icc_tbl = sm8250_icc_table,
+	.icc_tbl_size = ARRAY_SIZE(sm8250_icc_table),
+	.clk_rst_tbl = sm8250_clk_reset_table,
+	.clk_rst_tbl_size = ARRAY_SIZE(sm8250_clk_reset_table),
+	.bw_tbl_dec = sm8250_bw_table_dec,
+	.bw_tbl_dec_size = ARRAY_SIZE(sm8250_bw_table_dec),
+	.pmdomain_tbl = sm8250_pmdomain_table,
+	.pmdomain_tbl_size = ARRAY_SIZE(sm8250_pmdomain_table),
+	.opp_pd_tbl = sm8250_opp_pd_table,
+	.opp_pd_tbl_size = ARRAY_SIZE(sm8250_opp_pd_table),
+	.clk_tbl = sm8250_clk_table,
+	.clk_tbl_size = ARRAY_SIZE(sm8250_clk_table),
+	.dma_mask = GENMASK(31, 29) - 1,
+	.fwname = "qcom/vpu-1.0/venus.mbn",
+	.pas_id = IRIS_PAS_ID,
+	.inst_caps = &platform_inst_cap_sm8250,
+	.inst_fw_caps = inst_fw_cap_sm8250,
+	.inst_fw_caps_size = ARRAY_SIZE(inst_fw_cap_sm8250),
+	.tz_cp_config_data = &tz_cp_config_sm8250,
+	.hw_response_timeout = HW_RESPONSE_TIMEOUT_VALUE,
+	.num_vpp_pipe = 4,
+	.max_session_count = 16,
+	.max_core_mbpf = (8192 * 4352) / 256,
+	.input_config_params_default =
+		sm8250_vdec_input_config_param_default,
+	.input_config_params_default_size =
+		ARRAY_SIZE(sm8250_vdec_input_config_param_default),
+
+	.dec_ip_int_buf_tbl = sm8250_dec_ip_int_buf_tbl,
+	.dec_ip_int_buf_tbl_size = ARRAY_SIZE(sm8250_dec_ip_int_buf_tbl),
+	.dec_op_int_buf_tbl = sm8250_dec_op_int_buf_tbl,
+	.dec_op_int_buf_tbl_size = ARRAY_SIZE(sm8250_dec_op_int_buf_tbl),
+};
diff --git a/drivers/media/platform/qcom/iris/iris_probe.c b/drivers/media/platform/qcom/iris/iris_probe.c
index 276461ade811..1b7aec817331 100644
--- a/drivers/media/platform/qcom/iris/iris_probe.c
+++ b/drivers/media/platform/qcom/iris/iris_probe.c
@@ -327,6 +327,10 @@ static const struct of_device_id iris_dt_match[] = {
 		.compatible = "qcom,sm8550-iris",
 		.data = &sm8550_data,
 	},
+	{
+		.compatible = "qcom,sm8250-venus",
+		.data = &sm8250_data,
+	},
 	{ },
 };
 MODULE_DEVICE_TABLE(of, iris_dt_match);
-- 
2.34.1