[PATCH 0/2] IPU bridge improvements for IPU6

[PATCH 0/2] IPU bridge improvements for IPU6

[Sakari Ailus]

Hi folks,

Here are two simple patches, one to add support for ov01a10 in Dell XPS
9315 and improve reliability of driver probing on systems with IVSC.

Sakari Ailus (2):
  media: ipu-bridge: Add ov01a10 in Dell XPS 9315
  media: ipu-bridge: Poll for IVSC CSI device

 drivers/media/pci/intel/ipu-bridge.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

-- 
2.39.2

[PATCH 1/2] media: ipu-bridge: Add ov01a10 in Dell XPS 9315

[Sakari Ailus]

Add Omnivision ov01a10 sensor used in Dell XPS 9315, and use the driver
default frequency.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/pci/intel/ipu-bridge.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index f980e3125a7b..b2cf80d62ba2 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -60,6 +60,8 @@ static const struct ipu_sensor_config ipu_supported_sensors[] = {
 	IPU_SENSOR_CONFIG("OVTIDB10", 1, 560000000),
 	/* GalaxyCore GC0310 */
 	IPU_SENSOR_CONFIG("INT0310", 0),
+	/* Omnivision ov01a10 */
+	IPU_SENSOR_CONFIG("OVTI01A0", 1, 400000000),
 };
 
 static const struct ipu_property_names prop_names = {
-- 
2.39.2

Re: [PATCH 1/2] media: ipu-bridge: Add ov01a10 in Dell XPS 9315

[Dan Scally]

Hi Sakari

On 12/02/2024 09:19, Sakari Ailus wrote:
> Add Omnivision ov01a10 sensor used in Dell XPS 9315, and use the driver
> default frequency.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>


Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

> ---
>   drivers/media/pci/intel/ipu-bridge.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> index f980e3125a7b..b2cf80d62ba2 100644
> --- a/drivers/media/pci/intel/ipu-bridge.c
> +++ b/drivers/media/pci/intel/ipu-bridge.c
> @@ -60,6 +60,8 @@ static const struct ipu_sensor_config ipu_supported_sensors[] = {
>   	IPU_SENSOR_CONFIG("OVTIDB10", 1, 560000000),
>   	/* GalaxyCore GC0310 */
>   	IPU_SENSOR_CONFIG("INT0310", 0),
> +	/* Omnivision ov01a10 */
> +	IPU_SENSOR_CONFIG("OVTI01A0", 1, 400000000),
>   };
>   
>   static const struct ipu_property_names prop_names = {

[PATCH 2/2] media: ipu-bridge: Poll for IVSC CSI device

[Sakari Ailus]

The MEI CSI device is created as MEI boots up. This often takes place
after the IPU6 driver probes, in which case the IPU6 driver returned
-EPROBE_DEFER. The MEI CSI driver also returns -EPROBE_DEFER if the
firmware nodes created by the IPU bridge (via IPU6 driver) aren't in
place.

If no other drivers are being probed after this point, neither IPU6 nor
MEI CSI drivers will be re-probed. Address this (hopefully temporarily) by
polling MEI CSI device in the IPU bridge initialisation.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/pci/intel/ipu-bridge.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index b2cf80d62ba2..45c39bd93d74 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -4,6 +4,7 @@
 #include <linux/acpi.h>
 #include <linux/device.h>
 #include <linux/i2c.h>
+#include <linux/iopoll.h>
 #include <linux/mei_cl_bus.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
@@ -138,17 +139,21 @@ static struct device *ipu_bridge_get_ivsc_csi_dev(struct acpi_device *adev)
 	/* IVSC device on platform bus */
 	dev = bus_find_device(&platform_bus_type, NULL, adev,
 			      ipu_bridge_match_ivsc_dev);
-	if (dev) {
-		snprintf(name, sizeof(name), "%s-%pUl", dev_name(dev), &uuid);
+	if (!dev)
+		return NULL;
 
-		csi_dev = device_find_child_by_name(dev, name);
+	snprintf(name, sizeof(name), "%s-%pUl", dev_name(dev), &uuid);
 
-		put_device(dev);
+	/*
+	 * FIXME: instantiate MEI CSI software nodes outside the IPU bridge (or
+	 * call IPU bridge from MEI CSI). Wait up to 60 seconds here.
+	 */
+	read_poll_timeout(device_find_child_by_name, csi_dev, csi_dev,
+			  20000, 60000000, false, dev, name);
 
-		return csi_dev;
-	}
+	put_device(dev);
 
-	return NULL;
+	return csi_dev;
 }
 
 static int ipu_bridge_check_ivsc_dev(struct ipu_sensor *sensor,
-- 
2.39.2

Re: [PATCH 2/2] media: ipu-bridge: Poll for IVSC CSI device

[Hans de Goede]

Hi Sakari,

On 2/12/24 10:19, Sakari Ailus wrote:
> The MEI CSI device is created as MEI boots up. This often takes place
> after the IPU6 driver probes, in which case the IPU6 driver returned
> -EPROBE_DEFER. The MEI CSI driver also returns -EPROBE_DEFER if the
> firmware nodes created by the IPU bridge (via IPU6 driver) aren't in
> place.
> 
> If no other drivers are being probed after this point, neither IPU6 nor
> MEI CSI drivers will be re-probed. Address this (hopefully temporarily) by
> polling MEI CSI device in the IPU bridge initialisation.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/pci/intel/ipu-bridge.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> index b2cf80d62ba2..45c39bd93d74 100644
> --- a/drivers/media/pci/intel/ipu-bridge.c
> +++ b/drivers/media/pci/intel/ipu-bridge.c
> @@ -4,6 +4,7 @@
>  #include <linux/acpi.h>
>  #include <linux/device.h>
>  #include <linux/i2c.h>
> +#include <linux/iopoll.h>
>  #include <linux/mei_cl_bus.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
> @@ -138,17 +139,21 @@ static struct device *ipu_bridge_get_ivsc_csi_dev(struct acpi_device *adev)
>  	/* IVSC device on platform bus */
>  	dev = bus_find_device(&platform_bus_type, NULL, adev,
>  			      ipu_bridge_match_ivsc_dev);
> -	if (dev) {
> -		snprintf(name, sizeof(name), "%s-%pUl", dev_name(dev), &uuid);
> +	if (!dev)
> +		return NULL;
>  
> -		csi_dev = device_find_child_by_name(dev, name);
> +	snprintf(name, sizeof(name), "%s-%pUl", dev_name(dev), &uuid);
>  
> -		put_device(dev);
> +	/*
> +	 * FIXME: instantiate MEI CSI software nodes outside the IPU bridge (or
> +	 * call IPU bridge from MEI CSI). Wait up to 60 seconds here.
> +	 */
> +	read_poll_timeout(device_find_child_by_name, csi_dev, csi_dev,
> +			  20000, 60000000, false, dev, name);
>  
> -		return csi_dev;
> -	}
> +	put_device(dev);
>  
> -	return NULL;
> +	return csi_dev;
>  }
>  
>  static int ipu_bridge_check_ivsc_dev(struct ipu_sensor *sensor,


Hmm, ok so the issue here is that the MEI CSI device's creation does
not trigger a running of the work to re-probe deferred devices
because the probe() for the MEI CSI device's driver does not succeed.

And at this point nothing else is getting probed, so the IPU6
driver's probe() will never get called again, causing things
to not work.

So 3 options:

1. Instantiate MEI CSI software nodes outside the IPU bridge
this seems undesirable since the nodes point to each other
so doing this will be tricky I think.

2. Call ipu_bridge_init from the MEI CSI driver, this seems
like the best option to me. The MEI CSI driver can lookup
the PCI device for the bridge and call ipu_bridge_init()
on it.

ipu_bridge_init() is intended to get called only once
even if the driver is unbound + rebound. So this should
be ok, but we would need to:

a) Add a staitc mutex in ipu_bridge.c which is locked for
the duration of ipu_bridge_init()

b) Move the checks which ensure that ipu_bridge_init()
gets called only once to inside ipu_bridge_init(), this
part may be tricky when also taking chromebooks which don't
need ipu_bridge_init() at all into account. Although that
would only be a problem if those chromebooks also have
an ivsc chip.

3. In the -EPROBE_DEFER path of the MEI CSI driver force
deferred probing to run, this would require:

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 85152537dbf1..3836ac02332d 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -194,6 +194,7 @@ void driver_deferred_probe_trigger(void)
 	 */
 	queue_work(system_unbound_wq, &deferred_probe_work);
 }
+EXPORT_SYMBOL_GPL(driver_deferred_probe_trigger);
 
 /**
  * device_block_probing() - Block/defer device's probes

And a matching .h change and then we can just call
driver_deferred_probe_trigger() in the -EPROBE_DEFER
path of the MEI CSI driver.

This option would be my preferred solution.

I think we can sell exporting this to Greg KH,
otherwise we can also call:

platform_create_bundle() to create + successfully
probe() a fake platform device and then immediately
afterwards unregister the plat-dev + plat-drv again.

But it would be much better if we can just call
driver_deferred_probe_trigger().

Note if you decide to add:

EXPORT_SYMBOL_GPL(driver_deferred_probe_trigger);

it is important that the commit message properly
explains the problem (with a step by step what
happens in time which leads to the case where
no deferred-probes are running while one should
run for our purpose).

I would be happy to review the commit message before
you submit such a patch to Greg.

Regards,

Hans