[PATCH] media: ov5693: add OVTI5693 ACPI HID for IPU6 Surface devices

[PATCH] media: ov5693: add OVTI5693 ACPI HID for IPU6 Surface devices

[Fernando Rimoli]

The ov5693 driver only matches the "INT33BE" ACPI HID. Some Intel IPU6
Surface devices (e.g. Microsoft Surface Pro 9) enumerate the same OV5693
front camera with the ACPI HID "OVTI5693" instead. Without this HID the
i2c core never binds the driver, and the IPU bridge never builds the
firmware node graph for the sensor, so the front camera is unusable.

Add "OVTI5693" to the sensor driver's ACPI match table and to the IPU
bridge's supported-sensor list. The link frequency (419200000) matches
the existing INT33BE entry, as it is the same sensor. Devices that use
"INT33BE" (e.g. Surface Go 4) are unaffected.

Tested on Surface Pro 9 (IPU6): the sensor enumerates as OVTI5693:00
(ACPI path \_SB_.PC00.I2C3.CAMF) and binds with this change.

Link: https://github.com/linux-surface/linux-surface/pull/2171
Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.com>
---
Note: this HID addition makes the sensor bind and enumerate, but a fully
working front camera on IPU6 also requires programming MIPI_CTRL00
(0x4800 = 0x2d) before stream-on, otherwise the IPU6 CSI-2 receiver never
locks and capture times out. That register change is being handled
separately (see the Link: above). This patch is independent and useful on
its own for enumeration, so I'm sending it standalone; happy to reorder if
you'd prefer it after the register fix.

 drivers/media/i2c/ov5693.c           | 1 +
 drivers/media/pci/intel/ipu-bridge.c | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
index 4cc796bbe..02236f3db 100644
--- a/drivers/media/i2c/ov5693.c
+++ b/drivers/media/i2c/ov5693.c
@@ -1396,6 +1396,7 @@ static const struct dev_pm_ops ov5693_pm_ops = {
 
 static const struct acpi_device_id ov5693_acpi_match[] = {
 	{"INT33BE"},
+	{"OVTI5693"},
 	{},
 };
 MODULE_DEVICE_TABLE(acpi, ov5693_acpi_match);
diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index 88581a4c0..b70d61099 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -93,6 +93,8 @@ static const struct ipu_sensor_config ipu_supported_sensors[] = {
 	IPU_SENSOR_CONFIG("OVTI2680", 1, 331200000),
 	/* Omnivision OV5675 */
 	IPU_SENSOR_CONFIG("OVTI5675", 1, 450000000),
+	/* Omnivision OV5693 */
+	IPU_SENSOR_CONFIG("OVTI5693", 1, 419200000),
 	/* Omnivision OV8856 */
 	IPU_SENSOR_CONFIG("OVTI8856", 3, 180000000, 360000000, 720000000),
 	/* Sony IMX471 */
-- 
2.43.0

Re: [PATCH] media: ov5693: add OVTI5693 ACPI HID for IPU6 Surface devices

[Dan Scally]

Hi Fernando,thanks for the patch

On 08/07/2026 22:36, Fernando Rimoli wrote:
> The ov5693 driver only matches the "INT33BE" ACPI HID. Some Intel IPU6
> Surface devices (e.g. Microsoft Surface Pro 9) enumerate the same OV5693
> front camera with the ACPI HID "OVTI5693" instead. Without this HID the
> i2c core never binds the driver, and the IPU bridge never builds the
> firmware node graph for the sensor, so the front camera is unusable.
> 
> Add "OVTI5693" to the sensor driver's ACPI match table and to the IPU
> bridge's supported-sensor list. The link frequency (419200000) matches
> the existing INT33BE entry, as it is the same sensor. Devices that use
> "INT33BE" (e.g. Surface Go 4) are unaffected.
> 
> Tested on Surface Pro 9 (IPU6): the sensor enumerates as OVTI5693:00
> (ACPI path \_SB_.PC00.I2C3.CAMF) and binds with this change.
> 
> Link: https://github.com/linux-surface/linux-surface/pull/2171
> Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.com>
> ---

Looks good to me:

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

> Note: this HID addition makes the sensor bind and enumerate, but a fully
> working front camera on IPU6 also requires programming MIPI_CTRL00
> (0x4800 = 0x2d) before stream-on, otherwise the IPU6 CSI-2 receiver never
> locks and capture times out. That register change is being handled
> separately (see the Link: above). This patch is independent and useful on
> its own for enumeration, so I'm sending it standalone; happy to reorder if
> you'd prefer it after the register fix.

Standalone is fine :)


Thanks
Dan

> 
>   drivers/media/i2c/ov5693.c           | 1 +
>   drivers/media/pci/intel/ipu-bridge.c | 2 ++
>   2 files changed, 3 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
> index 4cc796bbe..02236f3db 100644
> --- a/drivers/media/i2c/ov5693.c
> +++ b/drivers/media/i2c/ov5693.c
> @@ -1396,6 +1396,7 @@ static const struct dev_pm_ops ov5693_pm_ops = {
>   
>   static const struct acpi_device_id ov5693_acpi_match[] = {
>   	{"INT33BE"},
> +	{"OVTI5693"},
>   	{},
>   };
>   MODULE_DEVICE_TABLE(acpi, ov5693_acpi_match);
> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> index 88581a4c0..b70d61099 100644
> --- a/drivers/media/pci/intel/ipu-bridge.c
> +++ b/drivers/media/pci/intel/ipu-bridge.c
> @@ -93,6 +93,8 @@ static const struct ipu_sensor_config ipu_supported_sensors[] = {
>   	IPU_SENSOR_CONFIG("OVTI2680", 1, 331200000),
>   	/* Omnivision OV5675 */
>   	IPU_SENSOR_CONFIG("OVTI5675", 1, 450000000),
> +	/* Omnivision OV5693 */
> +	IPU_SENSOR_CONFIG("OVTI5693", 1, 419200000),
>   	/* Omnivision OV8856 */
>   	IPU_SENSOR_CONFIG("OVTI8856", 3, 180000000, 360000000, 720000000),
>   	/* Sony IMX471 */

Re: [PATCH] media: ov5693: add OVTI5693 ACPI HID for IPU6 Surface devices

[Sakari Ailus]

Hi Fernando,

On Wed, Jul 08, 2026 at 11:36:33PM +0200, Fernando Rimoli wrote:
> The ov5693 driver only matches the "INT33BE" ACPI HID. Some Intel IPU6
> Surface devices (e.g. Microsoft Surface Pro 9) enumerate the same OV5693
> front camera with the ACPI HID "OVTI5693" instead. Without this HID the
> i2c core never binds the driver, and the IPU bridge never builds the
> firmware node graph for the sensor, so the front camera is unusable.

Could you split this into two, once for each?

> 
> Add "OVTI5693" to the sensor driver's ACPI match table and to the IPU
> bridge's supported-sensor list. The link frequency (419200000) matches
> the existing INT33BE entry, as it is the same sensor. Devices that use
> "INT33BE" (e.g. Surface Go 4) are unaffected.
> 
> Tested on Surface Pro 9 (IPU6): the sensor enumerates as OVTI5693:00
> (ACPI path \_SB_.PC00.I2C3.CAMF) and binds with this change.
> 
> Link: https://github.com/linux-surface/linux-surface/pull/2171
> Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.com>
> ---
> Note: this HID addition makes the sensor bind and enumerate, but a fully
> working front camera on IPU6 also requires programming MIPI_CTRL00
> (0x4800 = 0x2d) before stream-on, otherwise the IPU6 CSI-2 receiver never
> locks and capture times out. That register change is being handled
> separately (see the Link: above). This patch is independent and useful on
> its own for enumeration, so I'm sending it standalone; happy to reorder if
> you'd prefer it after the register fix.

Have you tested other values for 0x4800? Besides IPU3 (CIO2), the sensor is
used on a Rockchip board, so changing the settings needs to be done with
care.

If you haven't, I'd be interested to know how 0x04 works -- it should be a
safe change (LP00 -> LP11 in idle). If not, then testing other individual
bits would be useful as well; hopefully changing one bit makes a
difference.

-- 
Kind regards,

Sakari Ailus

[PATCH v2 0/3] media: Enable the OV5693 front camera on IPU6 Surface devices

[Fernando Rimoli]

Apologies for the slow turnaround on v2, I wanted to properly
characterise the MIPI_CTRL00 register before answering, and I was
coordinating with Arsalan Naeem, who originally found the register write
(linux-surface PR#2171); the third patch is co-developed with him.

This enables the OV5693 front camera on Intel IPU6 Surface devices such as
the Microsoft Surface Pro 9. Three independent pieces are needed on a stock
kernel:

  1/3  ov5693: add the "OVTI5693" ACPI HID so the i2c core binds the driver
       (these devices enumerate the sensor as OVTI5693, not INT33BE).
  2/3  ipu-bridge: add the same HID so the bridge builds the sensor's fwnode
       graph.
  3/3  ov5693: gate the MIPI clock lane, which the IPU6 CSI-2 receiver needs
       in order to lock (this is the register write from PR#2171).

On the MIPI_CTRL00 value, in reply to Sakari's question on v1:

I swept the register on a Surface Pro 9 (IPU6) with read-back. The power-on
default reads 0x00. Every value with bit 5 (clock-lane gate) set streams
(300/300 frames, steady ~28 fps); every value with bit 5 clear, including
0x00 and 0x04 (LP11-idle alone), fails with "stream stop time out" and no
frames. So bit 5 is the decisive bit for IPU6, and 0x04 alone is not enough.
Bit 5 is not an IPU6-specific hack: ov5640, ov5647 and ov5648 all gate the
clock lane on every platform. Patch 3 sets bit 5 plus bit 2 (LP11-idle,
i.e. 0x24, matching ov5640's value); I have only been able to test this on
IPU6, not on IPU3 (CIO2) or Rockchip, so I have kept it to the conventional
OV-family bits rather than the vendor's 0x2d (which also sets bit 3 and an
undocumented bit 0).

Patches 1-2 are independent of patch 3 and useful on their own for
enumeration; patch 1 carries Dan Scally's Reviewed-by from v1.

Changes since v1 [1]:
- Split the single v1 patch into the two HID patches (1/3, 2/3), as
  requested by Sakari.
- Added patch 3/3, the MIPI clock-lane gate, co-developed with Arsalan
  Naeem, with the characterisation above answering Sakari's v1 question.
- Carried Daniel Scally's Reviewed-by on patch 1/3.
- Used named bit macros instead of a bare register value.

[1] https://lore.kernel.org/linux-media/20260708213633.18392-1-fernandorimoli11@gmail.com/

Fernando Rimoli (3):
  media: i2c: ov5693: Add OVTI5693 ACPI HID
  media: ipu-bridge: Add OVTI5693 to the list of supported sensors
  media: i2c: ov5693: Gate the MIPI clock lane for IPU6

 drivers/media/i2c/ov5693.c           | 21 +++++++++++++++++++++
 drivers/media/pci/intel/ipu-bridge.c |  2 ++
 2 files changed, 23 insertions(+)

-- 
2.43.0

[PATCH v2 1/3] media: i2c: ov5693: Add OVTI5693 ACPI HID

[Fernando Rimoli]

The ov5693 driver only matches the "INT33BE" ACPI HID. Some Intel IPU6
Surface devices (e.g. Microsoft Surface Pro 9) enumerate the OV5693
front camera with the ACPI HID "OVTI5693" instead, so the i2c core never
binds the driver.

Add "OVTI5693" to the ACPI match table. Devices that use "INT33BE"
(e.g. Surface Go 4) are unaffected.

Tested on Surface Pro 9 (IPU6): the sensor enumerates as OVTI5693:00
(ACPI path \_SB_.PC00.I2C3.CAMF) and binds with this change.

Link: https://github.com/linux-surface/linux-surface/pull/2171
Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
---
 drivers/media/i2c/ov5693.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
index 4cc796bbe..02236f3db 100644
--- a/drivers/media/i2c/ov5693.c
+++ b/drivers/media/i2c/ov5693.c
@@ -1396,6 +1396,7 @@ static const struct dev_pm_ops ov5693_pm_ops = {
 
 static const struct acpi_device_id ov5693_acpi_match[] = {
 	{"INT33BE"},
+	{"OVTI5693"},
 	{},
 };
 MODULE_DEVICE_TABLE(acpi, ov5693_acpi_match);
-- 
2.43.0

[PATCH v2 2/3] media: ipu-bridge: Add OVTI5693 to the list of supported sensors

[Fernando Rimoli]

The IPU bridge builds the firmware node graph only for sensors listed in
ipu_supported_sensors[]. The OV5693 is currently listed only under its
legacy "INT33BE" HID, so on Intel IPU6 Surface devices that enumerate it
as "OVTI5693" (e.g. Microsoft Surface Pro 9) the bridge never wires up
the sensor and the front camera is unusable.

Add an "OVTI5693" entry. The link frequency (419200000) matches the
existing INT33BE entry, as it is the same sensor.

Tested on Surface Pro 9 (IPU6).

Link: https://github.com/linux-surface/linux-surface/pull/2171
Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.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 88581a4c0..b70d61099 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -93,6 +93,8 @@ static const struct ipu_sensor_config ipu_supported_sensors[] = {
 	IPU_SENSOR_CONFIG("OVTI2680", 1, 331200000),
 	/* Omnivision OV5675 */
 	IPU_SENSOR_CONFIG("OVTI5675", 1, 450000000),
+	/* Omnivision OV5693 */
+	IPU_SENSOR_CONFIG("OVTI5693", 1, 419200000),
 	/* Omnivision OV8856 */
 	IPU_SENSOR_CONFIG("OVTI8856", 3, 180000000, 360000000, 720000000),
 	/* Sony IMX471 */
-- 
2.43.0

[PATCH v2 3/3] media: i2c: ov5693: Gate the MIPI clock lane for IPU6

[Fernando Rimoli]

The ov5693 never programs MIPI_CTRL00 (0x4800), leaving it at its 0x00
power-on default (free-running MIPI clock). The IPU3 CSI-2 receiver
tolerates this, but the IPU6 receiver (e.g. on Microsoft Surface Pro 8/9
and Surface Go 4) fails to lock onto the link, so the sensor streams but
capture times out with "stream stop time out" and no frames arrive.

Gating the clock lane when idle (MIPI_CTRL00 bit 5) makes the IPU6
receiver lock. This was determined by sweeping the register on a Surface
Pro 9 (IPU6): every value with bit 5 set streams (300/300 frames, steady
28 fps), every value with bit 5 clear fails with the CSI-2 timeout;
register read-back confirmed the power-on default is 0x00. Bit 5 is the
conventional OmniVision clock-lane-gate setting (ov5640, ov5647 and
ov5648 all set it on every platform), so it is not an IPU6-specific hack.
Bit 2 (bus idle in LP11) is set as well, matching ov5640's value; this
was tested only on IPU6.

Link: https://github.com/linux-surface/linux-surface/pull/2171
Co-developed-by: Arsalan Naeem <naeemarsalan@gmail.com>
Signed-off-by: Arsalan Naeem <naeemarsalan@gmail.com>
Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.com>
---
 drivers/media/i2c/ov5693.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
index 02236f3db..d1b38348e 100644
--- a/drivers/media/i2c/ov5693.c
+++ b/drivers/media/i2c/ov5693.c
@@ -35,6 +35,12 @@
 #define OV5693_STOP_STREAMING			0x00
 #define OV5693_SW_RESET				0x01
 
+/* MIPI transmitter control */
+#define OV5693_MIPI_CTRL00_REG			CCI_REG8(0x4800)
+/* bit 5: gate the clock lane when idle; bit 2: keep the bus in LP11 when idle */
+#define OV5693_MIPI_CTRL00_CLOCK_LANE_GATE	BIT(5)
+#define OV5693_MIPI_CTRL00_BUS_IDLE		BIT(2)
+
 #define OV5693_REG_CHIP_ID			CCI_REG16(0x300a)
 /* Yes, this is right. The datasheet for the OV5693 gives its ID as 0x5690 */
 #define OV5693_CHIP_ID				0x5690
@@ -611,6 +617,20 @@ static int ov5693_enable_streaming(struct ov5693_device *ov5693, bool enable)
 {
 	int ret = 0;
 
+	/*
+	 * Gate the MIPI clock lane when idle. The power-on default of
+	 * MIPI_CTRL00 is 0x00 (free-running clock), which the IPU3 CSI-2
+	 * receiver tolerates but the IPU6 one does not: it fails to lock onto
+	 * the link and capture times out. Gating the clock lane (bit 5) is
+	 * required for IPU6 and is the conventional setting for OmniVision
+	 * sensors (cf. ov5640, ov5647, ov5648); also keep the bus in LP11 when
+	 * idle (bit 2). Harmless on IPU3.
+	 */
+	if (enable)
+		cci_write(ov5693->regmap, OV5693_MIPI_CTRL00_REG,
+			  OV5693_MIPI_CTRL00_CLOCK_LANE_GATE |
+			  OV5693_MIPI_CTRL00_BUS_IDLE, &ret);
+
 	cci_write(ov5693->regmap, OV5693_SW_STREAM_REG,
 		  enable ? OV5693_START_STREAMING : OV5693_STOP_STREAMING,
 		  &ret);
-- 
2.43.0

Re: [PATCH v2 3/3] media: i2c: ov5693: Gate the MIPI clock lane for IPU6

[Jakob Berg Jespersen]

On Fri, 17 Jul 2026, Fernando Rimoli wrote:
> The ov5693 never programs MIPI_CTRL00 (0x4800), leaving it at its 0x00
> power-on default (free-running MIPI clock). The IPU3 CSI-2 receiver
> tolerates this, but the IPU6 receiver [...] fails to lock onto the
> link, so the sensor streams but capture times out [...]

Confirmed on the Microsoft Surface Pro 7+ (Tiger Lake IPU6). 
Its front ov5693 enumerates as INT33BE, so just patch 3 seems relevant. 
With the unpatched driver, only 1/20 cold opens delivered frames, 
the rest timed out with zero frames. With this patch applied, 20/20 
streamed at ~28-29 fps. 

Tested-by: Jakob Berg Jespersen <dev@berg.pm>

(Surface Pro 7+, Fedora 44, kernel 7.0.12-200.fc44.x86_64; libcamera
cam tool.)

Re: [PATCH v2 3/3] media: i2c: ov5693: Gate the MIPI clock lane for IPU6

[Sakari Ailus]

Hi Fernando,

Thank you for the update.

On Fri, Jul 17, 2026 at 03:20:21PM +0200, Fernando Rimoli wrote:
> The ov5693 never programs MIPI_CTRL00 (0x4800), leaving it at its 0x00
> power-on default (free-running MIPI clock). The IPU3 CSI-2 receiver
> tolerates this, but the IPU6 receiver (e.g. on Microsoft Surface Pro 8/9
> and Surface Go 4) fails to lock onto the link, so the sensor streams but
> capture times out with "stream stop time out" and no frames arrive.
> 
> Gating the clock lane when idle (MIPI_CTRL00 bit 5) makes the IPU6
> receiver lock. This was determined by sweeping the register on a Surface
> Pro 9 (IPU6): every value with bit 5 set streams (300/300 frames, steady

Nice find!

> 28 fps), every value with bit 5 clear fails with the CSI-2 timeout;
> register read-back confirmed the power-on default is 0x00. Bit 5 is the
> conventional OmniVision clock-lane-gate setting (ov5640, ov5647 and
> ov5648 all set it on every platform), so it is not an IPU6-specific hack.
> Bit 2 (bus idle in LP11) is set as well, matching ov5640's value; this
> was tested only on IPU6.
> 
> Link: https://github.com/linux-surface/linux-surface/pull/2171
> Co-developed-by: Arsalan Naeem <naeemarsalan@gmail.com>
> Signed-off-by: Arsalan Naeem <naeemarsalan@gmail.com>
> Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.com>
> ---
>  drivers/media/i2c/ov5693.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
> index 02236f3db..d1b38348e 100644
> --- a/drivers/media/i2c/ov5693.c
> +++ b/drivers/media/i2c/ov5693.c
> @@ -35,6 +35,12 @@
>  #define OV5693_STOP_STREAMING			0x00
>  #define OV5693_SW_RESET				0x01
>  
> +/* MIPI transmitter control */
> +#define OV5693_MIPI_CTRL00_REG			CCI_REG8(0x4800)
> +/* bit 5: gate the clock lane when idle; bit 2: keep the bus in LP11 when idle */
> +#define OV5693_MIPI_CTRL00_CLOCK_LANE_GATE	BIT(5)
> +#define OV5693_MIPI_CTRL00_BUS_IDLE		BIT(2)
> +
>  #define OV5693_REG_CHIP_ID			CCI_REG16(0x300a)
>  /* Yes, this is right. The datasheet for the OV5693 gives its ID as 0x5690 */
>  #define OV5693_CHIP_ID				0x5690
> @@ -611,6 +617,20 @@ static int ov5693_enable_streaming(struct ov5693_device *ov5693, bool enable)
>  {
>  	int ret = 0;
>  
> +	/*
> +	 * Gate the MIPI clock lane when idle. The power-on default of
> +	 * MIPI_CTRL00 is 0x00 (free-running clock), which the IPU3 CSI-2
> +	 * receiver tolerates but the IPU6 one does not: it fails to lock onto
> +	 * the link and capture times out. Gating the clock lane (bit 5) is
> +	 * required for IPU6 and is the conventional setting for OmniVision
> +	 * sensors (cf. ov5640, ov5647, ov5648); also keep the bus in LP11 when
> +	 * idle (bit 2). Harmless on IPU3.
> +	 */
> +	if (enable)
> +		cci_write(ov5693->regmap, OV5693_MIPI_CTRL00_REG,
> +			  OV5693_MIPI_CTRL00_CLOCK_LANE_GATE |
> +			  OV5693_MIPI_CTRL00_BUS_IDLE, &ret);

This configuration should in fact be dependent on "clock-noncontinuous"
property; parsing the endpoint sets V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK
flag when the property is present.

Could you use that in v3?

The interesting question is then why are other sensors working with IPU6
just fine without this? In general, the non-contiguous clock mode is
something you enable explicitly and enabling it shouldn't make anything
work but possibly you could save a little bit of power.

It looks like drivers used with IPUs generally don't even support
non-continuous clock mode. Ov2740 (on Thinkpads with IPU6) has similar
configuration but there the same register is written 0, i.e. free running
clock lane). Different IPU6 variants do have different PHYs though, so this
could be the reason here.

The ipu-bridge needs to set the property, too. I think this needs to be
enabled only for the IPU6 variant in question (I'd just check the PCI ID;
those are found in include/media/ipu6-pci-table.h).

> +
>  	cci_write(ov5693->regmap, OV5693_SW_STREAM_REG,
>  		  enable ? OV5693_START_STREAMING : OV5693_STOP_STREAMING,
>  		  &ret);

-- 
Kind regards,

Sakari Ailus

[PATCH v3 0/4] media: Enable the OV5693 front camera on IPU6 Surface devices

[Fernando Rimoli]

This enables the OV5693 front camera on Intel IPU6 Surface devices such as
the Microsoft Surface Pro 9. Four pieces are needed on a stock kernel:

  1/4  ov5693: add the "OVTI5693" ACPI HID so the i2c core binds the driver
       (these devices enumerate the sensor as OVTI5693, not INT33BE).
  2/4  ipu-bridge: add the same HID so the bridge builds the sensor's fwnode
       graph.
  3/4  ov5693: gate the MIPI clock lane when the endpoint requests a
       non-continuous clock, which the IPU6 CSI-2 receiver needs to lock.
  4/4  ipu-bridge: request the non-continuous clock for the ov5693, but only
       on IPU6 (matched against ipu6_pci_tbl).

v3 reworks the clock-lane gate per Sakari's review of v2: instead of writing
MIPI_CTRL00 unconditionally, the sensor now gates the clock lane only when
the "clock-noncontinuous" endpoint property is present (the
V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK flag), the same way ov5647 does, and the
ipu-bridge supplies that property, gated on the IPU6 PCI IDs. The property is
set only for the ov5693 HIDs and only when the bridge sits on a PCI device
matching ipu6_pci_tbl, so IPU3 users of the same sensor and the non-PCI IVSC
bridge path are left untouched. I scoped it to all ipu6_pci_tbl IDs since the
gate is confirmed needed across two IPU6 generations (see below), happy to
narrow it to specific IDs if you would prefer.

On the register value: unlike ov5647, patch 3 sets bit 5 (clock-lane gate)
and bit 2 (LP11 idle) but not bit 4 (line sync). I re-swept the register at
runtime on my Surface Pro 9 (IPU6) to check bit 4 specifically: 0x24
(bit5+bit2) streams 300/300 frames at ~28 fps, but adding bit 4 (i.e.
ov5647's 0x34) collapses the stream to a couple of frames, and bit 4 alone
fails outright. A bit5-only value recovered to 300/300 in the same run, so
this is a value effect. This matches your point that the IPU6 PHYs differ
(ov2740 writes this register to 0 on Thinkpad IPU6), the ov5693 on IPU6
wants exactly bit5+bit2. Register read-back confirmed the power-on default
is 0x00.

Patches 1-2 are independent and useful on their own for enumeration; patch 1
carries Dan Scally's Reviewed-by from v1.

Changes since v2 [1]:
- Reworked patch 3: gate the MIPI clock lane on the "clock-noncontinuous"
  endpoint property (V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK) rather than writing
  MIPI_CTRL00 unconditionally, following ov5647. When the flag is absent the
  register is left at its reset default (IPU3 and others unaffected).
- New patch 4: ipu-bridge sets "clock-noncontinuous" for the ov5693, gated on
  the IPU6 PCI IDs (ipu6_pci_tbl), skipping non-PCI bridge instances.
- Characterised bit 4 (line sync) on IPU6 and documented why the value stays
  bit5+bit2, differing from ov5647.
- Patch 3 was reworked but is functionally identical on tested hardware (the
  same register value is written whenever the bridge sets the property), so I
  kept Jakob Berg Jespersen's Tested-by; Jakob, please shout if you would
  rather re-test v3.

The two IPU6 units the gate has been tested on: Surface Pro 9 (IPU6EP_ADLP,
0x465d) and, via Jakob, Surface Pro 7+ (IPU6, Tiger Lake, 0x9a19).

[1] https://lore.kernel.org/linux-media/20260717132021.18034-1-fernandorimoli11@gmail.com/

Fernando Rimoli (4):
  media: i2c: ov5693: Add OVTI5693 ACPI HID
  media: ipu-bridge: Add OVTI5693 to the list of supported sensors
  media: i2c: ov5693: Gate the MIPI clock lane for non-continuous clock
  media: ipu-bridge: Request non-continuous clock for ov5693 on IPU6

 drivers/media/i2c/ov5693.c           | 28 ++++++++++++++++++++++++
 drivers/media/pci/intel/ipu-bridge.c | 32 ++++++++++++++++++++++++++++
 include/media/ipu-bridge.h           |  2 +-
 3 files changed, 61 insertions(+), 1 deletion(-)

-- 
2.43.0

[PATCH v3 1/4] media: i2c: ov5693: Add OVTI5693 ACPI HID

[Fernando Rimoli]

The ov5693 driver only matches the "INT33BE" ACPI HID. Some Intel IPU6
Surface devices (e.g. Microsoft Surface Pro 9) enumerate the OV5693
front camera with the ACPI HID "OVTI5693" instead, so the i2c core never
binds the driver.

Add "OVTI5693" to the ACPI match table. Devices that use "INT33BE"
(e.g. Surface Go 4) are unaffected.

Tested on Surface Pro 9 (IPU6): the sensor enumerates as OVTI5693:00
(ACPI path \_SB_.PC00.I2C3.CAMF) and binds with this change.

Link: https://github.com/linux-surface/linux-surface/pull/2171
Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
---
 drivers/media/i2c/ov5693.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
index 4cc796bbe..02236f3db 100644
--- a/drivers/media/i2c/ov5693.c
+++ b/drivers/media/i2c/ov5693.c
@@ -1396,6 +1396,7 @@ static const struct dev_pm_ops ov5693_pm_ops = {
 
 static const struct acpi_device_id ov5693_acpi_match[] = {
 	{"INT33BE"},
+	{"OVTI5693"},
 	{},
 };
 MODULE_DEVICE_TABLE(acpi, ov5693_acpi_match);
-- 
2.43.0

[PATCH v3 2/4] media: ipu-bridge: Add OVTI5693 to the list of supported sensors

[Fernando Rimoli]

The IPU bridge builds the firmware node graph only for sensors listed in
ipu_supported_sensors[]. The OV5693 is currently listed only under its
legacy "INT33BE" HID, so on Intel IPU6 Surface devices that enumerate it
as "OVTI5693" (e.g. Microsoft Surface Pro 9) the bridge never wires up
the sensor and the front camera is unusable.

Add an "OVTI5693" entry. The link frequency (419200000) matches the
existing INT33BE entry, as it is the same sensor.

Tested on Surface Pro 9 (IPU6).

Link: https://github.com/linux-surface/linux-surface/pull/2171
Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.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 88581a4c0..b70d61099 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -93,6 +93,8 @@ static const struct ipu_sensor_config ipu_supported_sensors[] = {
 	IPU_SENSOR_CONFIG("OVTI2680", 1, 331200000),
 	/* Omnivision OV5675 */
 	IPU_SENSOR_CONFIG("OVTI5675", 1, 450000000),
+	/* Omnivision OV5693 */
+	IPU_SENSOR_CONFIG("OVTI5693", 1, 419200000),
 	/* Omnivision OV8856 */
 	IPU_SENSOR_CONFIG("OVTI8856", 3, 180000000, 360000000, 720000000),
 	/* Sony IMX471 */
-- 
2.43.0

Re: [PATCH v3 2/4] media: ipu-bridge: Add OVTI5693 to the list of supported sensors

[Dan Scally]

Hi Fernando - thanks for the set

On 20/07/2026 17:38, Fernando Rimoli wrote:
> The IPU bridge builds the firmware node graph only for sensors listed in
> ipu_supported_sensors[]. The OV5693 is currently listed only under its
> legacy "INT33BE" HID, so on Intel IPU6 Surface devices that enumerate it
> as "OVTI5693" (e.g. Microsoft Surface Pro 9) the bridge never wires up
> the sensor and the front camera is unusable.
> 
> Add an "OVTI5693" entry. The link frequency (419200000) matches the
> existing INT33BE entry, as it is the same sensor.
> 
> Tested on Surface Pro 9 (IPU6).
> 
> Link: https://github.com/linux-surface/linux-surface/pull/2171
> Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.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 88581a4c0..b70d61099 100644
> --- a/drivers/media/pci/intel/ipu-bridge.c
> +++ b/drivers/media/pci/intel/ipu-bridge.c
> @@ -93,6 +93,8 @@ static const struct ipu_sensor_config ipu_supported_sensors[] = {
>   	IPU_SENSOR_CONFIG("OVTI2680", 1, 331200000),
>   	/* Omnivision OV5675 */
>   	IPU_SENSOR_CONFIG("OVTI5675", 1, 450000000),
> +	/* Omnivision OV5693 */
> +	IPU_SENSOR_CONFIG("OVTI5693", 1, 419200000),
>   	/* Omnivision OV8856 */
>   	IPU_SENSOR_CONFIG("OVTI8856", 3, 180000000, 360000000, 720000000),
>   	/* Sony IMX471 */

[PATCH v3 3/4] media: i2c: ov5693: Gate the MIPI clock lane for non-continuous clock

[Fernando Rimoli]

The ov5693 never programs MIPI_CTRL00 (0x4800), leaving it at its 0x00
power-on default (free-running MIPI clock). The IPU3 CSI-2 receiver
tolerates this, but the IPU6 receiver (e.g. on Microsoft Surface Pro 8/9
and Surface Go 4) fails to lock onto the link, so the sensor streams but
capture times out with "stream stop time out" and no frames arrive.

Parse the "clock-noncontinuous" endpoint property (which sets the
V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK flag) and, when it is present, gate
the clock lane while idle (bit 5) and keep the bus in LP11 (bit 2) at
stream on, matching the approach ov5647 already uses for the same
register. When the flag is absent the register is left at its reset
default, so IPU3 and other users are unaffected.

Gating the clock lane was determined to be necessary and sufficient by
sweeping the register at runtime on a Surface Pro 9 (IPU6): every value
with bit 5 set streams (300/300 frames, steady 28 fps), every value with
bit 5 clear fails with the CSI-2 timeout; register read-back confirmed
the power-on default is 0x00. Bit 2 (bus idle in LP11) is set as well,
matching ov5640's value. Note that unlike ov5647 the line-sync bit
(bit 4) is deliberately not set: adding it collapses the IPU6 stream to
a couple of frames, so this value differs from ov5647's - the IPU6 D-PHY
behaves differently, consistent with these settings being PHY-specific.

The "clock-noncontinuous" property is supplied by the ipu-bridge for the
affected IPU6 variants in a subsequent patch.

Link: https://github.com/linux-surface/linux-surface/pull/2171
Co-developed-by: Arsalan Naeem <naeemarsalan@gmail.com>
Signed-off-by: Arsalan Naeem <naeemarsalan@gmail.com>
Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.com>
Tested-by: Jakob Berg Jespersen <dev@berg.pm>
---
 drivers/media/i2c/ov5693.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
index 02236f3db..5468e89d6 100644
--- a/drivers/media/i2c/ov5693.c
+++ b/drivers/media/i2c/ov5693.c
@@ -35,6 +35,14 @@
 #define OV5693_STOP_STREAMING			0x00
 #define OV5693_SW_RESET				0x01
 
+/* MIPI transmitter control */
+#define OV5693_MIPI_CTRL00_REG			CCI_REG8(0x4800)
+/* bit 5: gate the clock lane when idle; bit 2: keep the bus in LP11 when idle */
+#define OV5693_MIPI_CTRL00_CLOCK_LANE_GATE	BIT(5)
+#define OV5693_MIPI_CTRL00_BUS_IDLE		BIT(2)
+#define OV5693_MIPI_CTRL00_NCONT_CLOCK		(OV5693_MIPI_CTRL00_CLOCK_LANE_GATE | \
+						 OV5693_MIPI_CTRL00_BUS_IDLE)
+
 #define OV5693_REG_CHIP_ID			CCI_REG16(0x300a)
 /* Yes, this is right. The datasheet for the OV5693 gives its ID as 0x5690 */
 #define OV5693_CHIP_ID				0x5690
@@ -144,6 +152,9 @@ struct ov5693_device {
 	struct regulator_bulk_data supplies[OV5693_NUM_SUPPLIES];
 	struct clk *xvclk;
 
+	/* Gate the MIPI clock lane when idle (CSI-2 non-continuous clock) */
+	bool clock_ncont;
+
 	struct ov5693_mode {
 		struct v4l2_rect crop;
 		struct v4l2_mbus_framefmt format;
@@ -611,6 +622,19 @@ static int ov5693_enable_streaming(struct ov5693_device *ov5693, bool enable)
 {
 	int ret = 0;
 
+	/*
+	 * When the CSI-2 link is configured for a non-continuous clock, gate
+	 * the MIPI clock lane while idle (bit 5) and keep the bus in LP11
+	 * (bit 2). The power-on default of MIPI_CTRL00 is 0x00 (free-running
+	 * clock): the IPU3 CSI-2 receiver tolerates that, but the IPU6 one
+	 * fails to lock onto the link and capture times out. Only touch the
+	 * register when the endpoint requests a non-continuous clock, leaving
+	 * the reset default in place otherwise.
+	 */
+	if (enable && ov5693->clock_ncont)
+		cci_write(ov5693->regmap, OV5693_MIPI_CTRL00_REG,
+			  OV5693_MIPI_CTRL00_NCONT_CLOCK, &ret);
+
 	cci_write(ov5693->regmap, OV5693_SW_STREAM_REG,
 		  enable ? OV5693_START_STREAMING : OV5693_STOP_STREAMING,
 		  &ret);
@@ -1259,6 +1283,9 @@ static int ov5693_check_hwcfg(struct ov5693_device *ov5693)
 		goto out_free_bus_cfg;
 	}
 
+	ov5693->clock_ncont = bus_cfg.bus.mipi_csi2.flags &
+			      V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK;
+
 out_free_bus_cfg:
 	v4l2_fwnode_endpoint_free(&bus_cfg);
 
-- 
2.43.0

Re: [PATCH v3 3/4] media: i2c: ov5693: Gate the MIPI clock lane for non-continuous clock

[Dan Scally]

Hi Fernando, thanks for the patch

On 20/07/2026 17:38, Fernando Rimoli wrote:
> The ov5693 never programs MIPI_CTRL00 (0x4800), leaving it at its 0x00
> power-on default (free-running MIPI clock). The IPU3 CSI-2 receiver
> tolerates this, but the IPU6 receiver (e.g. on Microsoft Surface Pro 8/9
> and Surface Go 4) fails to lock onto the link, so the sensor streams but
> capture times out with "stream stop time out" and no frames arrive.
> 
> Parse the "clock-noncontinuous" endpoint property (which sets the
> V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK flag) and, when it is present, gate
> the clock lane while idle (bit 5) and keep the bus in LP11 (bit 2) at
> stream on, matching the approach ov5647 already uses for the same
> register. When the flag is absent the register is left at its reset
> default, so IPU3 and other users are unaffected.
> 
> Gating the clock lane was determined to be necessary and sufficient by
> sweeping the register at runtime on a Surface Pro 9 (IPU6): every value
> with bit 5 set streams (300/300 frames, steady 28 fps), every value with
> bit 5 clear fails with the CSI-2 timeout; register read-back confirmed
> the power-on default is 0x00. Bit 2 (bus idle in LP11) is set as well,
> matching ov5640's value. Note that unlike ov5647 the line-sync bit
> (bit 4) is deliberately not set: adding it collapses the IPU6 stream to
> a couple of frames, so this value differs from ov5647's - the IPU6 D-PHY
> behaves differently, consistent with these settings being PHY-specific.
> 
> The "clock-noncontinuous" property is supplied by the ipu-bridge for the
> affected IPU6 variants in a subsequent patch.
> 

This one looks good to me:

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

> Link: https://github.com/linux-surface/linux-surface/pull/2171
> Co-developed-by: Arsalan Naeem <naeemarsalan@gmail.com>
> Signed-off-by: Arsalan Naeem <naeemarsalan@gmail.com>
> Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.com>
> Tested-by: Jakob Berg Jespersen <dev@berg.pm>
> ---
>   drivers/media/i2c/ov5693.c | 27 +++++++++++++++++++++++++++
>   1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
> index 02236f3db..5468e89d6 100644
> --- a/drivers/media/i2c/ov5693.c
> +++ b/drivers/media/i2c/ov5693.c
> @@ -35,6 +35,14 @@
>   #define OV5693_STOP_STREAMING			0x00
>   #define OV5693_SW_RESET				0x01
>   
> +/* MIPI transmitter control */
> +#define OV5693_MIPI_CTRL00_REG			CCI_REG8(0x4800)
> +/* bit 5: gate the clock lane when idle; bit 2: keep the bus in LP11 when idle */
> +#define OV5693_MIPI_CTRL00_CLOCK_LANE_GATE	BIT(5)
> +#define OV5693_MIPI_CTRL00_BUS_IDLE		BIT(2)
> +#define OV5693_MIPI_CTRL00_NCONT_CLOCK		(OV5693_MIPI_CTRL00_CLOCK_LANE_GATE | \
> +						 OV5693_MIPI_CTRL00_BUS_IDLE)
> +
>   #define OV5693_REG_CHIP_ID			CCI_REG16(0x300a)
>   /* Yes, this is right. The datasheet for the OV5693 gives its ID as 0x5690 */
>   #define OV5693_CHIP_ID				0x5690
> @@ -144,6 +152,9 @@ struct ov5693_device {
>   	struct regulator_bulk_data supplies[OV5693_NUM_SUPPLIES];
>   	struct clk *xvclk;
>   
> +	/* Gate the MIPI clock lane when idle (CSI-2 non-continuous clock) */
> +	bool clock_ncont;
> +
>   	struct ov5693_mode {
>   		struct v4l2_rect crop;
>   		struct v4l2_mbus_framefmt format;
> @@ -611,6 +622,19 @@ static int ov5693_enable_streaming(struct ov5693_device *ov5693, bool enable)
>   {
>   	int ret = 0;
>   
> +	/*
> +	 * When the CSI-2 link is configured for a non-continuous clock, gate
> +	 * the MIPI clock lane while idle (bit 5) and keep the bus in LP11
> +	 * (bit 2). The power-on default of MIPI_CTRL00 is 0x00 (free-running
> +	 * clock): the IPU3 CSI-2 receiver tolerates that, but the IPU6 one
> +	 * fails to lock onto the link and capture times out. Only touch the
> +	 * register when the endpoint requests a non-continuous clock, leaving
> +	 * the reset default in place otherwise.
> +	 */
> +	if (enable && ov5693->clock_ncont)
> +		cci_write(ov5693->regmap, OV5693_MIPI_CTRL00_REG,
> +			  OV5693_MIPI_CTRL00_NCONT_CLOCK, &ret);
> +
>   	cci_write(ov5693->regmap, OV5693_SW_STREAM_REG,
>   		  enable ? OV5693_START_STREAMING : OV5693_STOP_STREAMING,
>   		  &ret);
> @@ -1259,6 +1283,9 @@ static int ov5693_check_hwcfg(struct ov5693_device *ov5693)
>   		goto out_free_bus_cfg;
>   	}
>   
> +	ov5693->clock_ncont = bus_cfg.bus.mipi_csi2.flags &
> +			      V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK;
> +
>   out_free_bus_cfg:
>   	v4l2_fwnode_endpoint_free(&bus_cfg);
>   

Re: [PATCH v3 3/4] media: i2c: ov5693: Gate the MIPI clock lane for non-continuous clock

[Sakari Ailus]

Hi Fernando,

On Mon, Jul 20, 2026 at 06:38:18PM +0200, Fernando Rimoli wrote:
> The ov5693 never programs MIPI_CTRL00 (0x4800), leaving it at its 0x00
> power-on default (free-running MIPI clock). The IPU3 CSI-2 receiver
> tolerates this, but the IPU6 receiver (e.g. on Microsoft Surface Pro 8/9
> and Surface Go 4) fails to lock onto the link, so the sensor streams but
> capture times out with "stream stop time out" and no frames arrive.
> 
> Parse the "clock-noncontinuous" endpoint property (which sets the
> V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK flag) and, when it is present, gate
> the clock lane while idle (bit 5) and keep the bus in LP11 (bit 2) at
> stream on, matching the approach ov5647 already uses for the same
> register. When the flag is absent the register is left at its reset
> default, so IPU3 and other users are unaffected.
> 
> Gating the clock lane was determined to be necessary and sufficient by
> sweeping the register at runtime on a Surface Pro 9 (IPU6): every value
> with bit 5 set streams (300/300 frames, steady 28 fps), every value with
> bit 5 clear fails with the CSI-2 timeout; register read-back confirmed
> the power-on default is 0x00. Bit 2 (bus idle in LP11) is set as well,
> matching ov5640's value. Note that unlike ov5647 the line-sync bit
> (bit 4) is deliberately not set: adding it collapses the IPU6 stream to
> a couple of frames, so this value differs from ov5647's - the IPU6 D-PHY
> behaves differently, consistent with these settings being PHY-specific.

This paragraph fits better to the cover page than to a commit message.

> 
> The "clock-noncontinuous" property is supplied by the ipu-bridge for the
> affected IPU6 variants in a subsequent patch.
> 
> Link: https://github.com/linux-surface/linux-surface/pull/2171
> Co-developed-by: Arsalan Naeem <naeemarsalan@gmail.com>
> Signed-off-by: Arsalan Naeem <naeemarsalan@gmail.com>
> Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.com>
> Tested-by: Jakob Berg Jespersen <dev@berg.pm>
> ---
>  drivers/media/i2c/ov5693.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
> index 02236f3db..5468e89d6 100644
> --- a/drivers/media/i2c/ov5693.c
> +++ b/drivers/media/i2c/ov5693.c
> @@ -35,6 +35,14 @@
>  #define OV5693_STOP_STREAMING			0x00
>  #define OV5693_SW_RESET				0x01
>  
> +/* MIPI transmitter control */
> +#define OV5693_MIPI_CTRL00_REG			CCI_REG8(0x4800)
> +/* bit 5: gate the clock lane when idle; bit 2: keep the bus in LP11 when idle */
> +#define OV5693_MIPI_CTRL00_CLOCK_LANE_GATE	BIT(5)
> +#define OV5693_MIPI_CTRL00_BUS_IDLE		BIT(2)

How about calling this OV5693_MIPI_CTRL00_LP11?

Didn't IPU6 work with this sensor without setting the 2nd bit?

> +#define OV5693_MIPI_CTRL00_NCONT_CLOCK		(OV5693_MIPI_CTRL00_CLOCK_LANE_GATE | \
> +						 OV5693_MIPI_CTRL00_BUS_IDLE)
> +
>  #define OV5693_REG_CHIP_ID			CCI_REG16(0x300a)
>  /* Yes, this is right. The datasheet for the OV5693 gives its ID as 0x5690 */
>  #define OV5693_CHIP_ID				0x5690
> @@ -144,6 +152,9 @@ struct ov5693_device {
>  	struct regulator_bulk_data supplies[OV5693_NUM_SUPPLIES];
>  	struct clk *xvclk;
>  
> +	/* Gate the MIPI clock lane when idle (CSI-2 non-continuous clock) */
> +	bool clock_ncont;
> +
>  	struct ov5693_mode {
>  		struct v4l2_rect crop;
>  		struct v4l2_mbus_framefmt format;
> @@ -611,6 +622,19 @@ static int ov5693_enable_streaming(struct ov5693_device *ov5693, bool enable)
>  {
>  	int ret = 0;
>  
> +	/*
> +	 * When the CSI-2 link is configured for a non-continuous clock, gate
> +	 * the MIPI clock lane while idle (bit 5) and keep the bus in LP11
> +	 * (bit 2). The power-on default of MIPI_CTRL00 is 0x00 (free-running
> +	 * clock): the IPU3 CSI-2 receiver tolerates that, but the IPU6 one
> +	 * fails to lock onto the link and capture times out. Only touch the
> +	 * register when the endpoint requests a non-continuous clock, leaving
> +	 * the reset default in place otherwise.
> +	 */
> +	if (enable && ov5693->clock_ncont)
> +		cci_write(ov5693->regmap, OV5693_MIPI_CTRL00_REG,
> +			  OV5693_MIPI_CTRL00_NCONT_CLOCK, &ret);
> +
>  	cci_write(ov5693->regmap, OV5693_SW_STREAM_REG,
>  		  enable ? OV5693_START_STREAMING : OV5693_STOP_STREAMING,
>  		  &ret);
> @@ -1259,6 +1283,9 @@ static int ov5693_check_hwcfg(struct ov5693_device *ov5693)
>  		goto out_free_bus_cfg;
>  	}
>  
> +	ov5693->clock_ncont = bus_cfg.bus.mipi_csi2.flags &
> +			      V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK;
> +
>  out_free_bus_cfg:
>  	v4l2_fwnode_endpoint_free(&bus_cfg);
>  

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v3 3/4] media: i2c: ov5693: Gate the MIPI clock lane for non-continuous clock

[Fernando Rimoli]

Hi Sakari,

Thanks for the review, and sorry for the slow response.

> > Gating the clock lane was determined to be necessary and sufficient by
> > sweeping the register at runtime on a Surface Pro 9 (IPU6): every value
[...]
> This paragraph fits better to the cover page than to a commit message.

Moved to the cover letter in v4, and expanded there since it now also has
to cover the bit 2 question below.

> How about calling this OV5693_MIPI_CTRL00_LP11?
>
> Didn't IPU6 work with this sensor without setting the 2nd bit?

It did, so I have dropped bit 2 entirely in v4 rather than renaming it.
The macro is gone and only the clock-lane gate is written. The remaining
macro keeps the name ov5647 uses for the same bit.

To answer it properly, from the runtime sweep on my Surface Pro 9 (IPU6),
3 trials of 30 frames plus a 300-frame stability run per value:

  0x20  bit5                    300/300 frames, steady 28.6 fps
  0x24  bit5+bit2               300/300 frames, steady 28.6 fps
  0x00  power-on default        0 frames, "stream stop time out"
  0x04  bit2 alone              0 frames, "stream stop time out"

0x20 and 0x24 are indistinguishable, and bit 2 on its own does nothing
for the link, so bit 5 is both necessary and sufficient here. I had set
bit 2 only because it is part of ov5640's canonical value for this
register, not because anything on IPU6 needed it. That is a bad reason
to write a bit, so it is gone. v4 writes bit 5 only.

While I was in there, the same sweep also covers why the value differs
from ov5647's despite the mechanism being copied from it: bit 4 (line
sync) breaks the link on this receiver rather than being merely
unnecessary.

  0x10  bit4 alone              0 frames, "stream stop time out"
  0x30  bit5+bit4               2 frames, stream collapses
  0x34  bit5+bit4+bit2          2 frames, stream collapses

A bit5-only value recovered to 300/300 later in the same run, after the
bit 4 failures, so these are genuine value effects and not a link that
had got itself wedged.

Since v3 a linux-surface user has reproduced the value question
independently on a Surface Pro 8, a different IPU6 generation (0x9a19,
Tiger Lake) with the INT33BE HID rather than OVTI5693, reading every
value back after writing [1]:

  0x2d  vendor value (control)  30 frames, 28.65 fps
  0x24  bit5+bit2               30 frames, 28.65 fps
  0x20  bit5                    30 frames, 28.65 fps
  0x0d  0x2d with bit 5 clear   0 frames
  0x08  bit3 alone              0 frames
  0x04  bit2 alone              0 frames
  0x01  bit0 alone              0 frames

That 0x0d row is the test I had not run: the vendor value with only bit 5
removed does not stream, so bit 5 is necessary and not just sufficient.
That set has two limits. The register was written over I2C into a stalled
capture rather than by running the patch, and the write lands after
stream on rather than before. So it confirms the value on a second device
and IPU generation, but says nothing about the plumbing. Both sweeps are
in the cover letter.

One further change in v4 that you have not seen: the bit is now set with
cci_update_bits() rather than cci_write(). The ov5693 does not otherwise
program MIPI_CTRL00, and the driver also serves IPU3/CIO2 and Rockchip,
so modifying the single bit leaves whatever the platform left in the
register intact. On the devices in question it reads 0x00 beforehand, so
the two are equivalent in practice and this is just the smaller claim.
It was suggested on the linux-surface thread [1] and seemed right.

One consequence: Dan reviewed and Jakob tested v3's 0x24, and since v4
changes both the value and the form of the write, I dropped both tags
rather than carry them across a behaviour change. Both are asked in the
cover letter to re-confirm.

[1] https://github.com/linux-surface/linux-surface/pull/2171#issuecomment-5298352360

Thanks,
Fernando

[PATCH v3 4/4] media: ipu-bridge: Request non-continuous clock for ov5693 on IPU6

[Fernando Rimoli]

The ov5693 (INT33BE / OVTI5693) needs its MIPI clock lane gated while
idle for the IPU6 CSI-2 receiver to lock onto the link; otherwise the
sensor streams but capture times out and no frames arrive. The sensor
driver gates the clock lane when the endpoint requests a non-continuous
clock, so the bridge has to supply that property.

The same sensor works with a free-running clock on the IPU3, and other
sensors are unaffected, so add the "clock-noncontinuous" endpoint
property only for the ov5693 and only when the bridge sits on an IPU6
(matched against ipu6_pci_tbl). Non-PCI bridge instances (e.g. the IVSC
mei_csi path) are skipped.

Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.com>
---
 drivers/media/pci/intel/ipu-bridge.c | 30 ++++++++++++++++++++++++++++
 include/media/ipu-bridge.h           |  2 +-
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index b70d61099..98773a99a 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -8,6 +8,7 @@
 #include <linux/dmi.h>
 #include <linux/i2c.h>
 #include <linux/mei_cl_bus.h>
+#include <linux/pci.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/property.h>
@@ -15,6 +16,7 @@
 #include <linux/workqueue.h>
 
 #include <media/ipu-bridge.h>
+#include <media/ipu6-pci-table.h>
 #include <media/v4l2-fwnode.h>
 
 #define ADEV_DEV(adev) ACPI_PTR(&((adev)->dev))
@@ -383,6 +385,25 @@ int ipu_bridge_parse_ssdb(struct acpi_device *adev, struct ipu_sensor *sensor)
 }
 EXPORT_SYMBOL_NS_GPL(ipu_bridge_parse_ssdb, "INTEL_IPU_BRIDGE");
 
+/*
+ * The ov5693 (INT33BE / OVTI5693, on Microsoft Surface devices) only locks
+ * onto the IPU6 CSI-2 receiver when its clock lane is gated while idle, which
+ * the sensor driver does when the endpoint requests a non-continuous clock.
+ * The same sensor works with a free-running clock on the IPU3, so only request
+ * the non-continuous clock on the IPU6 variants, keyed off the bridge's PCI ID.
+ */
+static bool ipu_bridge_needs_ncont_clock(struct ipu_bridge *bridge,
+					 const struct ipu_sensor_config *cfg)
+{
+	if (strcmp(cfg->hid, "INT33BE") && strcmp(cfg->hid, "OVTI5693"))
+		return false;
+
+	if (!dev_is_pci(bridge->dev))
+		return false;
+
+	return !!pci_match_id(ipu6_pci_tbl, to_pci_dev(bridge->dev));
+}
+
 static void ipu_bridge_create_fwnode_properties(
 	struct ipu_sensor *sensor,
 	struct ipu_bridge *bridge,
@@ -463,6 +484,15 @@ static void ipu_bridge_create_fwnode_properties(
 			cfg->link_freqs,
 			cfg->nr_link_freqs);
 
+	/*
+	 * This must stay after the link-frequencies above: the property array
+	 * is NULL-terminated, so the entry only takes effect when a preceding
+	 * slot is populated. All IPU6 ov5693 configs define a link frequency.
+	 */
+	if (ipu_bridge_needs_ncont_clock(bridge, cfg))
+		sensor->ep_properties[4] =
+			PROPERTY_ENTRY_BOOL("clock-noncontinuous");
+
 	sensor->ipu_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(
 					sensor->prop_names.data_lanes,
 					bridge->data_lanes, sensor->lanes);
diff --git a/include/media/ipu-bridge.h b/include/media/ipu-bridge.h
index 16fac7654..48eb1e270 100644
--- a/include/media/ipu-bridge.h
+++ b/include/media/ipu-bridge.h
@@ -141,7 +141,7 @@ struct ipu_sensor {
 	const char *vcm_type;
 
 	struct ipu_property_names prop_names;
-	struct property_entry ep_properties[5];
+	struct property_entry ep_properties[6];
 	struct property_entry dev_properties[5];
 	struct property_entry ipu_properties[3];
 	struct property_entry ivsc_properties[1];
-- 
2.43.0

Re: [PATCH v3 4/4] media: ipu-bridge: Request non-continuous clock for ov5693 on IPU6

[Dan Scally]

Hi Fernando

On 20/07/2026 17:38, Fernando Rimoli wrote:
> The ov5693 (INT33BE / OVTI5693) needs its MIPI clock lane gated while
> idle for the IPU6 CSI-2 receiver to lock onto the link; otherwise the
> sensor streams but capture times out and no frames arrive. The sensor
> driver gates the clock lane when the endpoint requests a non-continuous
> clock, so the bridge has to supply that property.
> 
> The same sensor works with a free-running clock on the IPU3, and other
> sensors are unaffected, so add the "clock-noncontinuous" endpoint
> property only for the ov5693 and only when the bridge sits on an IPU6
> (matched against ipu6_pci_tbl). Non-PCI bridge instances (e.g. the IVSC
> mei_csi path) are skipped.

Hm, I think this is functionally fine, but matching on PCI ID and sensor does seem a bit quirky...do 
you know if the IPU3 case is fine with the clock-noncontinuous flag too? If not I can test it tomorrow.

Thanks
Dan


> 
> Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.com>
> ---
>   drivers/media/pci/intel/ipu-bridge.c | 30 ++++++++++++++++++++++++++++
>   include/media/ipu-bridge.h           |  2 +-
>   2 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> index b70d61099..98773a99a 100644
> --- a/drivers/media/pci/intel/ipu-bridge.c
> +++ b/drivers/media/pci/intel/ipu-bridge.c
> @@ -8,6 +8,7 @@
>   #include <linux/dmi.h>
>   #include <linux/i2c.h>
>   #include <linux/mei_cl_bus.h>
> +#include <linux/pci.h>
>   #include <linux/platform_device.h>
>   #include <linux/pm_runtime.h>
>   #include <linux/property.h>
> @@ -15,6 +16,7 @@
>   #include <linux/workqueue.h>
>   
>   #include <media/ipu-bridge.h>
> +#include <media/ipu6-pci-table.h>
>   #include <media/v4l2-fwnode.h>
>   
>   #define ADEV_DEV(adev) ACPI_PTR(&((adev)->dev))
> @@ -383,6 +385,25 @@ int ipu_bridge_parse_ssdb(struct acpi_device *adev, struct ipu_sensor *sensor)
>   }
>   EXPORT_SYMBOL_NS_GPL(ipu_bridge_parse_ssdb, "INTEL_IPU_BRIDGE");
>   
> +/*
> + * The ov5693 (INT33BE / OVTI5693, on Microsoft Surface devices) only locks
> + * onto the IPU6 CSI-2 receiver when its clock lane is gated while idle, which
> + * the sensor driver does when the endpoint requests a non-continuous clock.
> + * The same sensor works with a free-running clock on the IPU3, so only request
> + * the non-continuous clock on the IPU6 variants, keyed off the bridge's PCI ID.
> + */
> +static bool ipu_bridge_needs_ncont_clock(struct ipu_bridge *bridge,
> +					 const struct ipu_sensor_config *cfg)
> +{
> +	if (strcmp(cfg->hid, "INT33BE") && strcmp(cfg->hid, "OVTI5693"))
> +		return false;
> +
> +	if (!dev_is_pci(bridge->dev))
> +		return false;
> +
> +	return !!pci_match_id(ipu6_pci_tbl, to_pci_dev(bridge->dev));
> +}
> +
>   static void ipu_bridge_create_fwnode_properties(
>   	struct ipu_sensor *sensor,
>   	struct ipu_bridge *bridge,
> @@ -463,6 +484,15 @@ static void ipu_bridge_create_fwnode_properties(
>   			cfg->link_freqs,
>   			cfg->nr_link_freqs);
>   
> +	/*
> +	 * This must stay after the link-frequencies above: the property array
> +	 * is NULL-terminated, so the entry only takes effect when a preceding
> +	 * slot is populated. All IPU6 ov5693 configs define a link frequency.
> +	 */
> +	if (ipu_bridge_needs_ncont_clock(bridge, cfg))
> +		sensor->ep_properties[4] =
> +			PROPERTY_ENTRY_BOOL("clock-noncontinuous");
> +
>   	sensor->ipu_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(
>   					sensor->prop_names.data_lanes,
>   					bridge->data_lanes, sensor->lanes);
> diff --git a/include/media/ipu-bridge.h b/include/media/ipu-bridge.h
> index 16fac7654..48eb1e270 100644
> --- a/include/media/ipu-bridge.h
> +++ b/include/media/ipu-bridge.h
> @@ -141,7 +141,7 @@ struct ipu_sensor {
>   	const char *vcm_type;
>   
>   	struct ipu_property_names prop_names;
> -	struct property_entry ep_properties[5];
> +	struct property_entry ep_properties[6];
>   	struct property_entry dev_properties[5];
>   	struct property_entry ipu_properties[3];
>   	struct property_entry ivsc_properties[1];

Re: [PATCH v3 4/4] media: ipu-bridge: Request non-continuous clock for ov5693 on IPU6

[Fernando Rimoli]

Hi Dan,

Thanks for the reviews on 1-3.

You're right that keying on both the PCI ID and the sensor is a bit awkward. My
reasoning for scoping it that tightly was caution rather than a known IPU3
failure: I only have IPU6 hardware (Surface Pro 9), so I couldn't confirm that
gating the ov5693's clock lane is safe on the IPU3 CSI-2 receiver, and I didn't
want to risk regressing the existing cio2 + ov5693 users (the INT33BE Surface
Pro/Book devices) that work today with the free-running default.

For what it's worth, from the receiver side IPU3 looks agnostic to the flag:
ipu3-cio2 only consumes bus.mipi_csi2.num_data_lanes from the parsed endpoint
and programs its D-PHY Rx timing (clk_termen/clk_settle) the same way regardless
of V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK, it never looks at that flag. So the open
question is purely sensor-side: whether the ov5693 idling its clock lane in LP11
(bit 5) upsets the cio2 D-PHY's lock. I can't answer that without IPU3 hardware.

If your test tomorrow shows cio2 + ov5693 still streams fine with
clock-noncontinuous set, I'm happy to drop the ipu6_pci_tbl check entirely and
just request the property for the ov5693 unconditionally in v4 which removes
the PCI quirk and is much cleaner. (The sensor-driver side already no-ops when
the flag is absent, so nothing else needs to change.)

If it turns out IPU3 doesn't like it, then the PCI gate is doing real work and
I'd keep it, but I can add a comment making that rationale explicit.

Either way I'll respin once we know. Thanks a lot for offering to test on IPU3,
that's the one platform I can't cover.

Thanks,
Fernando

Re: [PATCH v3 4/4] media: ipu-bridge: Request non-continuous clock for ov5693 on IPU6

[Sakari Ailus]

Hi Fernando,

On Tue, Jul 21, 2026 at 01:50:17AM +0200, Fernando Rimoli wrote:
> Hi Dan,
> 
> Thanks for the reviews on 1-3.
> 
> You're right that keying on both the PCI ID and the sensor is a bit awkward. My
> reasoning for scoping it that tightly was caution rather than a known IPU3
> failure: I only have IPU6 hardware (Surface Pro 9), so I couldn't confirm that
> gating the ov5693's clock lane is safe on the IPU3 CSI-2 receiver, and I didn't
> want to risk regressing the existing cio2 + ov5693 users (the INT33BE Surface
> Pro/Book devices) that work today with the free-running default.

Please limit the line length to around 75.

> 
> For what it's worth, from the receiver side IPU3 looks agnostic to the flag:
> ipu3-cio2 only consumes bus.mipi_csi2.num_data_lanes from the parsed endpoint
> and programs its D-PHY Rx timing (clk_termen/clk_settle) the same way regardless
> of V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK, it never looks at that flag. So the open
> question is purely sensor-side: whether the ov5693 idling its clock lane in LP11
> (bit 5) upsets the cio2 D-PHY's lock. I can't answer that without IPU3 hardware.

The support for non-contiguous clock isn't mandatory on either side
(whereas free-running clock is and should always "just work") so as a whole
this is weird. But as we know the sensor works with IPU6 with non-continous
clock, that's what I guess we'll just have to do then.

> 
> If your test tomorrow shows cio2 + ov5693 still streams fine with
> clock-noncontinuous set, I'm happy to drop the ipu6_pci_tbl check entirely and
> just request the property for the ov5693 unconditionally in v4 which removes
> the PCI quirk and is much cleaner. (The sensor-driver side already no-ops when
> the flag is absent, so nothing else needs to change.)
> 
> If it turns out IPU3 doesn't like it, then the PCI gate is doing real work and
> I'd keep it, but I can add a comment making that rationale explicit.
> 
> Either way I'll respin once we know. Thanks a lot for offering to test on IPU3,
> that's the one platform I can't cover.

How about adding PCI IDs (for matching the particualr IPU) and flags to
struct ipu_sensor_config? I have a feeling we'll need this elsewhere, too.

Then e.g.

#define IPU_SENSOR_CONFIG_MATCH_FL(_HID, _ID, _FLAGS, _NR, ...)	\
	(const struct ipu_sensor_config) {			\
		.hid = _HID,					\
		.pci_id = _ID,					\
		.flags = IPU_BR_FL_##_FLAGS,			\
		.nr_link_freqs = _NR,				\
		.link_freqs = { __VA_ARGS__ }			\
	}

#define IPU_SENSOR_CONFIG(_HID, _ID, ...) \
	IPU_SENSOR_CONFIG_MATCH_FL(_HID, 0, 0, _NR, ...)

Where _ID is the IPU PCI product ID and flags is e.g.

#define IPU_BR_FL_CSI2_CLK_NONCONTINUOUS		BIT(0)

You could also switch to dynamically assigning the property index so
there's no need to rely on a particular device having a list of link
frequencies. See NEXT_PROPERTY() macro in drivers/acpi/mipi-disco-img.c .
That should go to a separate patch, like adding the above mechanism.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v3 4/4] media: ipu-bridge: Request non-continuous clock for ov5693 on IPU6

[Fernando Rimoli]

Hi Sakari,

Thanks. Noted on the line length, I will keep prose to 75.

> The support for non-contiguous clock isn't mandatory on either side
> (whereas free-running clock is and should always "just work") so as a
> whole this is weird. But as we know the sensor works with IPU6 with
> non-continous clock, that's what I guess we'll just have to do then.

Agreed that it is odd. The sensor side is not optional in practice here:
with the free-running default the IPU6 receiver never locks and capture
times out, so on these devices the "should just work" case is the one
that does not. I have no visibility into why the receiver behaves that
way, only that gating the clock lane is what makes it lock.

> How about adding PCI IDs (for matching the particualr IPU) and flags to
> struct ipu_sensor_config? I have a feeling we'll need this elsewhere,
> too.

Done in v4, and it is a much better shape than what I had. The quirk
helper is gone; struct ipu_sensor_config gains pci_id and flags, and the
ov5693 becomes table entries rather than code.

Two small deviations from your sketch, in case they were deliberate and I
have missed the point:

- IPU_SENSOR_CONFIG's expansion referenced _NR without it being in the
  parameter list, and forwarded "..." rather than __VA_ARGS__, so I wrote
  it as:

      #define IPU_SENSOR_CONFIG(_HID, _NR, ...)                       \
              IPU_SENSOR_CONFIG_MATCH_FL(_HID, 0, NONE, _NR, __VA_ARGS__)

- .flags = IPU_BR_FL_##_FLAGS does not paste to anything for a plain 0, so
  there is an IPU_BR_FL_NONE for the generic case. That keeps every
  existing table entry unchanged, which seemed worth having.

The one thing I would like your opinion on is a semantic that comes with
putting a PCI ID in the table. A sensor with both a specific and a generic
entry for the same HID matches twice on the specific IPU, and
ipu_bridge_connect_sensor() would then enumerate the same ACPI device
twice and consume two of the four IPU ports. So in v4 the more specific
entry wins and the generic one is skipped. It is implemented as a filter
in ipu_bridge_connect_sensors() rather than by requiring the table to be
ordered, so it does not depend on entry order. If you would rather have
this expressed differently, say so and I will rework it. An explicit
"generic" marker, or resolving it at match time, would both work.

ipu_bridge_ivsc_is_ready() also walks the table, but it runs before the
bridge exists and is an idempotent readiness check, so duplicate HIDs are
harmless there and I left it alone.

On scope: v4 sets the flag for IPU6 (0x9a19, Tiger Lake) and IPU6EP_ADLP
(0x465d), for both HIDs. To be exact about what hardware stands behind
each: OVTI5693 on ADL-P is this series running on my Surface Pro 9;
INT33BE on Tiger Lake is the register value, confirmed on a Surface Pro 8
and a Pro 7+ by linux-surface users. Jakob's Tested-by from v2 covered
the Pro 7+ but at the old value, which is why I dropped it. v3 matched
all of ipu6_pci_tbl, but now that the IDs are spelled out per entry I
would rather list only what is confirmed on hardware and add the rest as
reports arrive, Surface Go 4 (ADL-N) being the likely next one. Happy
to broaden it if you would prefer the whole family up front.

> You could also switch to dynamically assigning the property index so
> there's no need to rely on a particular device having a list of link
> frequencies. See NEXT_PROPERTY() macro in drivers/acpi/mipi-disco-img.c
> . That should go to a separate patch, like adding the above mechanism.

Also done, as patch 4/6, before the mechanism. It is a no-op refactor:
the endpoint property slots are named in an enum and assigned through a
bounds-checked running index, so the array is sized by the enum and a
conditional property no longer has to sit at a fixed slot.

This was a latent bug and not just untidiness. Because the property array
is NULL-terminated, v3's approach would have silently dropped the
property for any sensor with nr_link_freqs == 0 (INTC10C5 is the one
in-tree example): the empty link-frequencies slot terminated the array
before anything after it. With the running index that cannot happen.

v4 is 6 patches:

  1-2  HID enumeration, unchanged (both have Dan's Reviewed-by)
  3    ov5693 clock-lane gate, now bit 5 only, set with cci_update_bits()
  4    ipu-bridge: dynamic endpoint property indices
  5    ipu-bridge: per-IPU config matching + flags
  6    ipu-bridge: use them for the ov5693 on IPU6

Each patch builds without warnings on its own, at W=1 as well, on top of
v7.3-rc1.

Thanks,
Fernando

Re: [PATCH v3 4/4] media: ipu-bridge: Request non-continuous clock for ov5693 on IPU6

[Fernando Rimoli]

Hi Dan,

Thanks for the reviews on v3, and sorry for the slow follow-up.

> Hm, I think this is functionally fine, but matching on PCI ID and sensor
> does seem a bit quirky...do you know if the IPU3 case is fine with the
> clock-noncontinuous flag too? If not I can test it tomorrow.

You and Sakari landed on the same objection, and v4 drops that helper
entirely. Instead struct ipu_sensor_config gains an optional IPU PCI
product ID and a flags field (Sakari's suggestion), so the ov5693 on IPU6
becomes table entries rather than a special case in code:

    IPU_SENSOR_CONFIG("INT33BE", 1, 419200000),
    IPU_SENSOR_CONFIG_MATCH_FL("INT33BE", PCI_DEVICE_ID_INTEL_IPU6,
                               CSI2_CLK_NONCONTINUOUS, 1, 419200000),
    IPU_SENSOR_CONFIG_MATCH_FL("INT33BE", PCI_DEVICE_ID_INTEL_IPU6EP_ADLP,
                               CSI2_CLK_NONCONTINUOUS, 1, 419200000),

That also answers your IPU3 question without needing the test: IPU3 has
no matching entry, so it keeps using the generic one and never sees the
flag, by construction rather than by a PCI check. So please don't spend
hardware time on it on my account. If you are curious anyway I would
still be interested in the result, since knowing IPU3 tolerates the flag
would let a later patch collapse those three entries back into one, but
it is not blocking anything now.

Since a PCI ID in the table is new, one semantic came with it that I would
value your view on as the bridge's author: a sensor with both a specific
and a generic entry for the same HID matches twice on the specific IPU, and
ipu_bridge_connect_sensor() would then enumerate the same ACPI device twice
and consume two of the four IPU ports. v4 therefore skips the generic entry
when a specific one matches, as an order-independent filter in
ipu_bridge_connect_sensors() rather than a rule about table ordering.

Two other things you should know about v4:

- Patch 3 now writes only bit 5 (clock-lane gate), not bit 5 + bit 2.
  Sakari asked whether IPU6 needed bit 2; it does not, and my sweep data
  agreed, so it is gone. It is also now set with cci_update_bits() rather
  than a full-register cci_write(): the ov5693 does not otherwise program
  MIPI_CTRL00, and since this driver serves IPU3/CIO2 and Rockchip too,
  touching the one bit leaves anything the platform left there intact.
  Because that changes behaviour I dropped your Reviewed-by from that
  patch rather than carry it. Happy to add it back if you are still
  content with the narrower write.

  Since v3 a linux-surface user also reproduced the value question
  independently on a Surface Pro 8 (Tiger Lake IPU6, INT33BE rather than
  OVTI5693) and got the one result I was missing: the vendor value with
  only bit 5 cleared does not stream, so bit 5 is necessary and not just
  sufficient. Details and the caveats in the cover letter.

- Your Reviewed-by on v3 2/4 is carried forward unchanged onto v4 2/6, as
  that patch is untouched. Thank you for it.

There is also a new no-op refactor as patch 4/6 (endpoint property indices
assigned dynamically, per NEXT_PROPERTY() in mipi-disco-img.c). It turned
out to fix a latent issue in my v3: because the property array is
NULL-terminated, a conditional property at a fixed index would have been
silently dropped for any sensor with nr_link_freqs == 0, INTC10C5 being
the in-tree example.

Thanks,
Fernando

[PATCH v4 0/6] media: Enable the OV5693 front camera on IPU6 Surface devices

[Fernando Rimoli]

This enables the OV5693 front camera on Intel IPU6 Surface devices such
as the Microsoft Surface Pro 9. Six patches on a stock kernel:

  1/6  ov5693: add the "OVTI5693" ACPI HID so the i2c core binds the
       driver (these devices enumerate the sensor as OVTI5693, not
       INT33BE).
  2/6  ipu-bridge: add the same HID so the bridge builds the sensor's
       fwnode graph.
  3/6  ov5693: gate the MIPI clock lane when the endpoint requests a
       non-continuous clock, which the IPU6 CSI-2 receiver needs to
       lock.
  4/6  ipu-bridge: assign endpoint property indices dynamically.
  5/6  ipu-bridge: add per-IPU sensor config matching and config flags.
  6/6  ipu-bridge: use those to request the non-continuous clock for
       the ov5693 on the IPU6 variants that need it.

v4 restructures the bridge side per Sakari's review of v3. Instead of
the ad-hoc "is this the ov5693 on an IPU6" helper v3 had, struct
ipu_sensor_config gains an optional IPU PCI product ID and a flags
field, so a sensor that needs different treatment on different IPUs is
described by the table rather than by code. Patch 5 adds that mechanism
and patch 6 is its first user. Patch 4 is the prerequisite Sakari asked
for: endpoint property indices are now assigned through a running index
in the style of NEXT_PROPERTY() in mipi-disco-img.c, so a conditional
property no longer depends on the sensor having a link frequency.

Patch 5 introduces one semantic I would like checked: where both a
PCI-specific and a generic entry exist for the same HID, the specific one
wins and the generic one is skipped, so the sensor is connected once
rather than twice. Doing it the other way round would consume two of the
four IPU ports for one sensor.

On the register value: patch 3 now sets only bit 5 (clock-lane gate).
Sakari asked whether IPU6 worked without bit 2. It does, so v4 drops it.
For the record, sweeping MIPI_CTRL00 at runtime on a Surface Pro 9
(IPU6EP_ADLP, sensor as OVTI5693), 3 trials of 30 frames plus a
300-frame stability run per value:

  0x20  bit5                    300/300 frames, steady 28.6 fps
  0x24  bit5+bit2               300/300 frames, steady 28.6 fps
  0x2d  vendor value            300/300 frames, steady 28.6 fps
  0x00  power-on default        0 frames, "stream stop time out"
  0x04  bit2 alone              0 frames, "stream stop time out"
  0x10  bit4 alone              0 frames, "stream stop time out"
  0x30  bit5+bit4               2 frames, stream collapses
  0x34  bit5+bit4+bit2          2 frames, stream collapses

So bit 5 is necessary and sufficient, bit 2 makes no difference, and bit 4
actively breaks the link. That last one is why this differs from ov5647's
value for the same register even though the mechanism is copied from it. A
bit5-only value recovered to 300/300 in the same run after the bit 4
failures, so those are genuine value effects and not a wedged link.
Register read-back confirmed the power-on default is 0x00.

This was reproduced independently on a Surface Pro 8 (IPU6 0x9a19,
sensor as INT33BE) by a linux-surface user [3], 30 frames per value,
every value read back after writing:

  0x2d  vendor value (control)  30 frames, 28.65 fps
  0x24  bit5+bit2               30 frames, 28.65 fps
  0x20  bit5                    30 frames, 28.65 fps
  0x0d  0x2d with bit 5 clear   0 frames
  0x08  bit3 alone              0 frames
  0x04  bit2 alone              0 frames
  0x01  bit0 alone              0 frames
  0x00  power-on default        0 frames

The 0x0d row is the one I could not produce myself: everything in the
vendor value except bit 5 fails to stream, so bit 5 is necessary and not
just sufficient. That second set has two limits. It was produced by poking
the register over I2C into a stalled capture rather than by running this
patch, and the write lands after stream on rather than before. So it
confirms the value on a second device and IPU generation, but says nothing
about the plumbing. The patch itself is tested on the Pro 9.

Patch 3 sets the bit with cci_update_bits() rather than writing the whole
register. The ov5693 does not otherwise program MIPI_CTRL00, and this
driver also serves IPU3/CIO2 and Rockchip platforms, so modifying one bit
leaves anything the platform put there intact. On the devices here the
register reads 0x00 before the write, so in practice the two forms are
equivalent. I went with the narrower one because it claims less.

The write is issued at each stream on rather than once per power-on.
That placement seems to matter in practice: in the same thread [4],
setups that write MIPI_CTRL00 once from userspace lose the binned
1296x972 mode, which reprograms the sensor, while setups that write it
from the driver at every stream on keep it.

Patch 6 sets the flag for two IPU6 product IDs, the ones the gate has
been exercised on: IPU6 (Tiger Lake, 0x9a19) and IPU6EP_ADLP (Alder
Lake-P, 0x465d), for both HIDs, four entries in total. The combinations
covered by hardware are OVTI5693 on ADL-P (this series, Surface Pro 9)
and INT33BE on Tiger Lake (the register value, Surface Pro 8 [3] and
Pro 7+). v3 matched all of ipu6_pci_tbl; with the IDs now spelled out
in the table I would rather list only what is confirmed and add the
others as reports come in. Surface Go 4 (ADL-N, 0x462e) is the likely
next one.

Patches 1-2 are independent and useful on their own for enumeration,
and both carry Dan Scally's Reviewed-by (patch 1 from v1, patch 2 from
v3).

Dan, Jakob: patch 3 changed since you reviewed and tested it. 0x24
became 0x20 and the write became a read-modify-write, so I dropped Dan's
Reviewed-by and Jakob's Tested-by rather than carry them across a
behaviour change. Happy to add both back if you are still content with
the narrower write.

Dan, on your v3 4/4 question about matching on PCI ID and sensor being
quirky: agreed, and patches 5-6 replace that with per-IPU entries in
the sensor config table instead. That also settles the IPU3 question you
offered to test, since IPU3 keeps matching the generic entry and never
sees the flag. No need spend hardware time on it on my account.

Two checkpatch complaints in this series are deliberate, in case they
show up in a report: patch 4 trips "CHECK: Macro argument reuse
'index'", which upstream's NEXT_PROPERTY() trips identically and is
inherent to the pattern; and patch 5 trips "ERROR: Macros with complex
values should be enclosed in parentheses", which the existing
ipu-bridge.h macros already trip five times over. Parenthesising a
compound literal would break the designated initialisers.

Changes since v3 [1]:
- Patch 3: write only bit 5, dropping bit 2 (not needed on IPU6);
  OV5693_MIPI_CTRL00_BUS_IDLE is gone rather than renamed to _LP11.
  Set the bit with cci_update_bits() instead of writing the whole
  register. Moved the register-sweep evidence from the commit message
  to this cover letter, and added an independent sweep on a second
  device and IPU generation. Dropped Dan's Reviewed-by and Jakob's
  Tested-by (the patch changed behaviour since both were given).
- New patch 4: assign endpoint property indices dynamically.
- New patch 5: PCI ID + flags in struct ipu_sensor_config, with
  IPU_SENSOR_CONFIG_MATCH_FL() and most-specific-entry-wins matching.
- Patch 6 rewritten on top of patch 5: the HID-strcmp plus
  pci_match_id(ipu6_pci_tbl) helper is gone, replaced by table
  entries. Scoped to the two tested IPU6 product IDs instead of all of
  ipu6_pci_tbl.
- Rebased onto v7.3-rc1.

Changes since v2 [2]:
- Reworked patch 3: gate the MIPI clock lane on the
  "clock-noncontinuous" endpoint property
  (V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK) rather than writing MIPI_CTRL00
  unconditionally, following ov5647. When the flag is absent the
  register is left alone (IPU3 and others unaffected).
- Added the ipu-bridge side that supplies the property.

Tested on a Surface Pro 9 (IPU6EP_ADLP): with the series applied the
front camera streams 300/300 frames at 28.6 fps. That testing predates
the switch to cci_update_bits(). The register reads 0x00 before the write
on this hardware, so both forms resolve to the same value and I have not
re-measured it. Each patch builds without warnings on its own, at W=1 as
well, so the series is bisectable.

[1] https://lore.kernel.org/linux-media/20260720163819.104130-1-fernandorimoli11@gmail.com/
[2] https://lore.kernel.org/linux-media/20260717132021.18034-1-fernandorimoli11@gmail.com/
[3] https://github.com/linux-surface/linux-surface/pull/2171#issuecomment-5298352360
[4] https://github.com/linux-surface/linux-surface/pull/2171#issuecomment-5372616619

Fernando Rimoli (6):
  media: i2c: ov5693: Add OVTI5693 ACPI HID
  media: ipu-bridge: Add OVTI5693 to the list of supported sensors
  media: i2c: ov5693: Gate the MIPI clock lane for non-continuous clock
  media: ipu-bridge: Assign endpoint property indices dynamically
  media: ipu-bridge: Match sensor configs per IPU and add config flags
  media: ipu-bridge: Request non-continuous clock for ov5693 on IPU6

 drivers/media/i2c/ov5693.c           | 25 ++++++++++
 drivers/media/pci/intel/ipu-bridge.c | 73 +++++++++++++++++++++++-----
 include/media/ipu-bridge.h           | 51 ++++++++++++++++---
 3 files changed, 130 insertions(+), 19 deletions(-)


base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
-- 
2.43.0

[PATCH v4 1/6] media: i2c: ov5693: Add OVTI5693 ACPI HID

[Fernando Rimoli]

The ov5693 driver only matches the "INT33BE" ACPI HID. Some Intel IPU6
Surface devices (e.g. Microsoft Surface Pro 9) enumerate the OV5693
front camera with the ACPI HID "OVTI5693" instead, so the i2c core never
binds the driver.

Add "OVTI5693" to the ACPI match table. Devices that use "INT33BE"
(e.g. Surface Go 4) are unaffected.

Tested on Surface Pro 9 (IPU6): the sensor enumerates as OVTI5693:00
(ACPI path \_SB_.PC00.I2C3.CAMF) and binds with this change.

Link: https://github.com/linux-surface/linux-surface/pull/2171
Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
---
 drivers/media/i2c/ov5693.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
index 4cc796bbe..02236f3db 100644
--- a/drivers/media/i2c/ov5693.c
+++ b/drivers/media/i2c/ov5693.c
@@ -1396,6 +1396,7 @@ static const struct dev_pm_ops ov5693_pm_ops = {
 
 static const struct acpi_device_id ov5693_acpi_match[] = {
 	{"INT33BE"},
+	{"OVTI5693"},
 	{},
 };
 MODULE_DEVICE_TABLE(acpi, ov5693_acpi_match);
-- 
2.43.0

[PATCH v4 2/6] media: ipu-bridge: Add OVTI5693 to the list of supported sensors

[Fernando Rimoli]

The IPU bridge builds the firmware node graph only for sensors listed in
ipu_supported_sensors[]. The OV5693 is currently listed only under its
legacy "INT33BE" HID, so on Intel IPU6 Surface devices that enumerate it
as "OVTI5693" (e.g. Microsoft Surface Pro 9) the bridge never wires up
the sensor and the front camera is unusable.

Add an "OVTI5693" entry. The link frequency (419200000) matches the
existing INT33BE entry, as it is the same sensor.

Tested on Surface Pro 9 (IPU6).

Link: https://github.com/linux-surface/linux-surface/pull/2171
Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.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 1bb3a3e98..131c70844 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -95,6 +95,8 @@ static const struct ipu_sensor_config ipu_supported_sensors[] = {
 	IPU_SENSOR_CONFIG("OVTI2680", 1, 331200000),
 	/* Omnivision OV5675 */
 	IPU_SENSOR_CONFIG("OVTI5675", 1, 450000000),
+	/* Omnivision OV5693 */
+	IPU_SENSOR_CONFIG("OVTI5693", 1, 419200000),
 	/* Omnivision OV8856 */
 	IPU_SENSOR_CONFIG("OVTI8856", 3, 180000000, 360000000, 720000000),
 	/* Sony IMX471 */
-- 
2.43.0

[PATCH v4 3/6] media: i2c: ov5693: Gate the MIPI clock lane for non-continuous clock

[Fernando Rimoli]

The ov5693 never programs MIPI_CTRL00 (0x4800), leaving it at its 0x00
power-on default, which lets the MIPI clock run freely. The IPU3 CSI-2
receiver tolerates this, but the IPU6 receiver (e.g. on Microsoft
Surface Pro 8/9 and Surface Go 4) fails to lock onto the link, so the
sensor streams but capture times out with "stream stop time out" and no
frames arrive.

Parse the "clock-noncontinuous" endpoint property (which sets the
V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK flag) and, when it is present, gate
the clock lane while idle at stream on. Bit 5 of MIPI_CTRL00 has the
same meaning here as in ov5647, which sets it for the same purpose.

Unlike ov5647, which owns the register across its own stream on and off,
this is a read-modify-write of the single gate bit: the ov5693 otherwise
never touches MIPI_CTRL00, so preserving the rest of it keeps every
platform that does not ask for a non-continuous clock bit-for-bit as it
was. No counterpart is needed at stream off, as the link is down by then
and the register returns to its default when the sensor is powered off.

The "clock-noncontinuous" property is supplied by the ipu-bridge for the
affected IPU6 variants in a subsequent patch.

Link: https://github.com/linux-surface/linux-surface/pull/2171
Co-developed-by: Arsalan Naeem <naeemarsalan@gmail.com>
Signed-off-by: Arsalan Naeem <naeemarsalan@gmail.com>
Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.com>
---
 drivers/media/i2c/ov5693.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
index 02236f3db..cedc6ea03 100644
--- a/drivers/media/i2c/ov5693.c
+++ b/drivers/media/i2c/ov5693.c
@@ -35,6 +35,11 @@
 #define OV5693_STOP_STREAMING			0x00
 #define OV5693_SW_RESET				0x01
 
+/* MIPI transmitter control */
+#define OV5693_MIPI_CTRL00_REG			CCI_REG8(0x4800)
+/* Gate the clock lane when there is no packet to transmit */
+#define OV5693_MIPI_CTRL00_CLOCK_LANE_GATE	BIT(5)
+
 #define OV5693_REG_CHIP_ID			CCI_REG16(0x300a)
 /* Yes, this is right. The datasheet for the OV5693 gives its ID as 0x5690 */
 #define OV5693_CHIP_ID				0x5690
@@ -144,6 +149,9 @@ struct ov5693_device {
 	struct regulator_bulk_data supplies[OV5693_NUM_SUPPLIES];
 	struct clk *xvclk;
 
+	/* Gate the MIPI clock lane when idle (CSI-2 non-continuous clock) */
+	bool clock_ncont;
+
 	struct ov5693_mode {
 		struct v4l2_rect crop;
 		struct v4l2_mbus_framefmt format;
@@ -611,6 +619,19 @@ static int ov5693_enable_streaming(struct ov5693_device *ov5693, bool enable)
 {
 	int ret = 0;
 
+	/*
+	 * Gate the MIPI clock lane while idle if the CSI-2 link is configured
+	 * for a non-continuous clock. Only that bit is touched, and only in
+	 * that case, so the register keeps whatever the platform left in it
+	 * and the clock stays free-running as before everywhere else. It
+	 * needs no counterpart at stream off: the link is down by then, and
+	 * the register returns to its default when the sensor is powered off.
+	 */
+	if (enable && ov5693->clock_ncont)
+		cci_update_bits(ov5693->regmap, OV5693_MIPI_CTRL00_REG,
+				OV5693_MIPI_CTRL00_CLOCK_LANE_GATE,
+				OV5693_MIPI_CTRL00_CLOCK_LANE_GATE, &ret);
+
 	cci_write(ov5693->regmap, OV5693_SW_STREAM_REG,
 		  enable ? OV5693_START_STREAMING : OV5693_STOP_STREAMING,
 		  &ret);
@@ -1259,6 +1280,9 @@ static int ov5693_check_hwcfg(struct ov5693_device *ov5693)
 		goto out_free_bus_cfg;
 	}
 
+	ov5693->clock_ncont = bus_cfg.bus.mipi_csi2.flags &
+			      V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK;
+
 out_free_bus_cfg:
 	v4l2_fwnode_endpoint_free(&bus_cfg);
 
-- 
2.43.0

Re: [PATCH v4 3/6] media: i2c: ov5693: Gate the MIPI clock lane for non-continuous clock

[Jakob Berg Jespersen]

Hi, just did a new test, see below


On Monday, August 31st, 2026 at 20:19, Fernando Rimoli <fernandorimoli11@gmail.com> wrote:
...
> Parse the "clock-noncontinuous" endpoint property (which sets the
> V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK flag) and, when it is present, gate
> the clock lane while idle at stream on. 
...

Tested-by: Jakob Berg Jespersen <dev@berg.pm> # Surface Pro 7+, IPU6 Tiger Lake

Retested on the Surface Pro 7+ now that both the value and the mechanism
have changed. This covers patches 3 to 6, so it exercises the bridge
matching on Tiger Lake rather than only the register value. Patches 1 and
2 are not exercised here: this machine enumerates the sensor as INT33BE.

Both kernels are v7.3-rc1, your base-commit, differing only by this
series. Front camera, 20 cold opens each, libcamera 0.7.1, full
resolution (sensor 2592x1944 BGGR-10):

  v7.3-rc1 unpatched       3/20 runs delivered frames; the other 17 ended
                           in "stream stop time out" with zero frames
  v7.3-rc1 + this series   20/20 runs at 28.64-28.65 fps, and one run of
                           600 frames with no drops

Reading MIPI_CTRL00 back over I2C during a live capture, on both kernels:

  unpatched    0x4800 = 0x00
  patched      0x4800 = 0x20

with 0x300a/0x300b reading 0x56/0x90 as a control that the bus and address
are right. So the bridge did supply clock-noncontinuous, the driver did act
on it, and the read-modify-write set bit 5 and nothing else.

Two things that may be worth having:

The unpatched register really is 0x00 on this machine, so your argument
that the whole-register write and the read-modify-write resolve to the
same value holds on Tiger Lake as well as on the Pro 9.

Zann580 asked on GitHub, when posting the 0x20 row, that the minimal 
value be confirmed from a built module before the patch was narrowed 
on the strength of a userspace poke. I think this does that.

(Surface Pro 7+, Fedora 44; kernel 7.3.0-rc1 versus 7.3.0-rc1 plus this
series; Tiger Lake IPU6 8086:9a19; ov5693 as INT33BE; libcamera 0.7.1.)

Regards,
Jakob

Re: [PATCH v4 3/6] media: i2c: ov5693: Gate the MIPI clock lane for non-continuous clock

[Fernando Rimoli]

Hi Jakob,

Thank you, and thanks for re-testing rather than letting the old tag stand.

I will carry the tag on patches 3 to 6 as you scoped it, and not on 1 and
2, since your machine enumerates INT33BE and neither of those is exercised
there.

The register read-back is the most valuable part. Nobody
had shown the mechanism working end to end before: 0x4800 reading 0x00
unpatched and 0x20 patched, with the chip ID as a bus control, demonstrates
that the bridge really did supply clock-noncontinuous, that the driver
acted on it, and that the read-modify-write set bit 5 and disturbed nothing
else.

Your unpatched result also corrects something I should fix. You saw 3 of 20
runs deliver frames without the series; on my Pro 9 the unpatched case is a
hard zero every time. So the failure is intermittent on Tiger Lake rather
than absolute, and patch 3's commit message currently reads as though it
always fails.

That is too strong given your data. In v5 I will say the receiver usually
fails to lock, note that it is intermittent on some units, and cite your 3
of 20 alongside my 0 of N.

Two smaller things from your mail worth recording. Your read of 0x00 on the
unpatched Tiger Lake machine confirms on a second IPU generation that the
whole-register write and the read-modify-write resolve to the same value,
so that argument in the cover letter is no longer only about my Pro 9. And
you have answered the caveat Zann580 attached to the 0x20 row on GitHub,
which was that the minimal value should be confirmed from a built module
rather than from a userspace poke before the patch was narrowed on its
strength. It now has been.

Thanks again,
Fernando

Re: [PATCH v4 3/6] media: i2c: ov5693: Gate the MIPI clock lane for non-continuous clock

[Fernando Rimoli]

Hi Jakob,

One follow-up question, because two later results bear on your unpatched
numbers and they do not agree with each other.

A Surface Pro 8 user ran the negative control, removing only the flagged
INT33BE entry for Tiger Lake. Without it his front camera streams on the
first capture after boot, with real image data, and every later capture in
that boot hangs at zero bytes. With the entry, every capture works.

A Surface Pro 7+ user then ran the same control on a machine that also
enumerates INT33BE on Tiger Lake, so the same configuration as yours, and
got the opposite: his first capture of that boot already returned zero
frames. He checked the journal to confirm nothing had opened the sensor
earlier in the boot.

So "works once per boot" describes one of those machines and not the other.
The Pro 8 user now suspects his own result is an artefact of how his machine
is set up: he streams an IR camera continuously for face unlock, so a
neighbouring CSI-2 port on the same ISYS was active during his first
capture. He is retesting with that stopped.

Your 3 of 20 is the measurement that would help separate these. Do you
remember whether your harness rebooted or power-cycled the sensor between
runs, or were all 20 opens inside a single boot?

Either answer is useful and neither needs a retest on your part. If all 20
were in one boot, then the machines genuinely differ and I will say so
rather than fold them into one story. If some followed a reboot or a power
cycle, that supports reading it as once per sensor power cycle, which I
would then describe as holding on some machines rather than as the general
behaviour.

I am only trying to describe the unpatched behaviour accurately, since the
current commit message says no frames arrive at all and all three results
contradict that in different ways.

Thanks,
Fernando

Re: [PATCH v4 3/6] media: i2c: ov5693: Gate the MIPI clock lane for non-continuous clock

[Jakob Berg Jespersen]

Hi Fernando,

For the front camera, ov5693: all 20 were inside a single boot, uptime 6
minutes at the start and 19 at the end. No reboot and no deliberate
power cycle anywhere in the run.

The part that bears on your question is which runs worked. It was runs
1, 7 and 14, not the first few. So on this machine it is not "works once
per boot" - runs 7 and 14 came well into the same boot, after several
failures each. I would expect a rerun to give a somewhat different
count.

Each run was an independent open and close of the camera rather than one
long capture, and the sensor runtime-suspends in between: on the patched
arm an i2c read a few seconds after a capture had finished returned
Remote I/O error, because the sensor had already powered down. So if the
behavior were once per sensor power cycle, I would have expected close
to 20 of 20 rather than 3.

For whatever it adds, I measured 1 of 20 on this machine in July, on
7.0.12. And last year, before I started developing on this, I made a few
comments on the randomness of the (unpatched) front camera on Fedora 42
and 43:
https://github.com/linux-surface/linux-surface/discussions/1354#discussioncomment-12863982

On the confound you mention: during my 20 runs the IR camera was not
merely idle, it was absent. ov7251 failed to probe with -121 on that
kernel, and libcamera enumerated two cameras, front and back only. So
nothing was streaming on a neighbouring port while I measured. For what
it adds, on the Pro 7+ the front camera sits on csi2-4 and the IR camera
on csi2-5, and those two share a D-PHY - so the Pro 8 user's suspicion
looks mechanically plausible on hardware laid out this way, though I
cannot say his mapping is the same.

Two details that may help you separate the machines. The ov5693 sensor
is genuinely up during the failures - the privacy LED lights - so it is
not a rail or a probe problem. And the successful runs log 10 to 11 csi2
errors while the failing runs log none at all, so the failure looks like
the receiver never locking rather than the sensor never starting.

Regards,
Jakob

Re: [PATCH v4 3/6] media: i2c: ov5693: Gate the MIPI clock lane for non-continuous clock

[Fil Dunsky]

Tested-by: Fil Dunsky <filipp.dunsky@gmail.com>

Scope: patches 3-6. Patch 1 was already in my tree and this machine is
INT33BE, so patches 1 and 2 are not functionally exercised here.

Hardware: Surface Pro 8, IPU6 Tiger Lake (8086:9a19), OV5693 front
sensor at INT33BE:00, OV13858 rear, VD55G0 IR. Kernel 7.2.2 plus the
linux-surface patch set, not the v7.3-rc1 base the series declares;
patch 6 needed that tree's duplicate OVTI5693 entry dropped before it
would apply.

With the series applied, streaming from the ISYS capture node:

   60 frames, SBGGR10 2592x1944, 604661760 bytes, 28.64 fps
   MIPI_CTRL00 (0x4800) read back over i2c while streaming: 0x20

0x20 is the bit-5-only value patch 3 writes, so the clock-noncontinuous
property did reach the sensor driver: the path from the table entry in
patch 4 through to the register is exercised, not merely "the camera
works".

I also ran the negative control, with PCI_DEVICE_ID_INTEL_IPU6 dropped
from the INT33BE entries and nothing else changed. How it fails is
worth recording, because the obvious test misses it:

   - the first capture after boot succeeds, 60 frames at 28.64 fps, with
     0x4800 reading 0x00;
   - every subsequent capture in that boot returns zero bytes and times
     out, with nothing in dmesg;
   - writing 0x20 to 0x4800 over i2c into a stalled stream starts frames
     immediately, reproduced on three separate streams, while clearing
     the bit again mid-stream does not stop them.

With bit 5 set, the same script captures three times in a row without
trouble; I measured that with our downstream driver, which writes 0x2d
unconditionally. So the entry is needed at stream start, and a single
capture after a reboot is not enough to tell whether it is present.

The free first capture appears to be particular to this machine: two
other testers of this series, on a Surface Pro 7+ and a Pro 9, get zero
bytes on the first attempt as well. I have not been able to explain the
difference. It only affects how the entry should be verified, not
whether it is needed.

Patch 5's precedence rule is exercised here as well: INT33BE appears
twice in the table, the generic entry and the Tiger Lake one, and the
bridge connects the sensor once - "Connected 3 cameras", no double
connect. The two sensors that take no flags, OV13858 and the VD55G0 IR
camera, are unaffected; the IR camera still does face authentication.

The teardown "stream stop time out" appears identically with and
without the series, so it is not introduced by it.

One note for out-of-tree builders: patch 4 grows struct ipu_sensor,
which moves the CRC of ipu_bridge_init() and ipu_bridge_parse_ssdb(),
so with CONFIG_MODVERSIONS ipu-bridge and intel-ipu6 have to be built
together. intel-ipu6-isys imports only ipu_bridge_instantiate_vcm,
whose CRC does not move; an unrebuilt intel-ipu6-isys loaded fine
against the new pair on 7.2.2.

Re: [PATCH v4 3/6] media: i2c: ov5693: Gate the MIPI clock lane for non-continuous clock

[Sakari Ailus]

Hi Fernando,

On Mon, Aug 31, 2026 at 08:18:55PM +0200, Fernando Rimoli wrote:
> The ov5693 never programs MIPI_CTRL00 (0x4800), leaving it at its 0x00
> power-on default, which lets the MIPI clock run freely. The IPU3 CSI-2
> receiver tolerates this, but the IPU6 receiver (e.g. on Microsoft
> Surface Pro 8/9 and Surface Go 4) fails to lock onto the link, so the
> sensor streams but capture times out with "stream stop time out" and no
> frames arrive.
> 
> Parse the "clock-noncontinuous" endpoint property (which sets the
> V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK flag) and, when it is present, gate
> the clock lane while idle at stream on. Bit 5 of MIPI_CTRL00 has the

I'd clip what's after the period above.

> same meaning here as in ov5647, which sets it for the same purpose.
> 
> Unlike ov5647, which owns the register across its own stream on and off,
> this is a read-modify-write of the single gate bit: the ov5693 otherwise
> never touches MIPI_CTRL00, so preserving the rest of it keeps every
> platform that does not ask for a non-continuous clock bit-for-bit as it
> was. No counterpart is needed at stream off, as the link is down by then
> and the register returns to its default when the sensor is powered off.
> 
> The "clock-noncontinuous" property is supplied by the ipu-bridge for the
> affected IPU6 variants in a subsequent patch.
> 
> Link: https://github.com/linux-surface/linux-surface/pull/2171
> Co-developed-by: Arsalan Naeem <naeemarsalan@gmail.com>
> Signed-off-by: Arsalan Naeem <naeemarsalan@gmail.com>
> Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.com>
> ---
>  drivers/media/i2c/ov5693.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
> index 02236f3db..cedc6ea03 100644
> --- a/drivers/media/i2c/ov5693.c
> +++ b/drivers/media/i2c/ov5693.c
> @@ -35,6 +35,11 @@
>  #define OV5693_STOP_STREAMING			0x00
>  #define OV5693_SW_RESET				0x01
>  
> +/* MIPI transmitter control */
> +#define OV5693_MIPI_CTRL00_REG			CCI_REG8(0x4800)
> +/* Gate the clock lane when there is no packet to transmit */
> +#define OV5693_MIPI_CTRL00_CLOCK_LANE_GATE	BIT(5)
> +
>  #define OV5693_REG_CHIP_ID			CCI_REG16(0x300a)
>  /* Yes, this is right. The datasheet for the OV5693 gives its ID as 0x5690 */
>  #define OV5693_CHIP_ID				0x5690
> @@ -144,6 +149,9 @@ struct ov5693_device {
>  	struct regulator_bulk_data supplies[OV5693_NUM_SUPPLIES];
>  	struct clk *xvclk;
>  
> +	/* Gate the MIPI clock lane when idle (CSI-2 non-continuous clock) */
> +	bool clock_ncont;
> +
>  	struct ov5693_mode {
>  		struct v4l2_rect crop;
>  		struct v4l2_mbus_framefmt format;
> @@ -611,6 +619,19 @@ static int ov5693_enable_streaming(struct ov5693_device *ov5693, bool enable)
>  {
>  	int ret = 0;
>  
> +	/*
> +	 * Gate the MIPI clock lane while idle if the CSI-2 link is configured
> +	 * for a non-continuous clock. Only that bit is touched, and only in
> +	 * that case, so the register keeps whatever the platform left in it
> +	 * and the clock stays free-running as before everywhere else. It
> +	 * needs no counterpart at stream off: the link is down by then, and
> +	 * the register returns to its default when the sensor is powered off.
> +	 */
> +	if (enable && ov5693->clock_ncont)
> +		cci_update_bits(ov5693->regmap, OV5693_MIPI_CTRL00_REG,
> +				OV5693_MIPI_CTRL00_CLOCK_LANE_GATE,
> +				OV5693_MIPI_CTRL00_CLOCK_LANE_GATE, &ret);
> +
>  	cci_write(ov5693->regmap, OV5693_SW_STREAM_REG,
>  		  enable ? OV5693_START_STREAMING : OV5693_STOP_STREAMING,
>  		  &ret);
> @@ -1259,6 +1280,9 @@ static int ov5693_check_hwcfg(struct ov5693_device *ov5693)
>  		goto out_free_bus_cfg;
>  	}
>  
> +	ov5693->clock_ncont = bus_cfg.bus.mipi_csi2.flags &
> +			      V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK;

Could you also make the change to the DT bindings, adding "clock-noncontinuous:
true" there?

> +
>  out_free_bus_cfg:
>  	v4l2_fwnode_endpoint_free(&bus_cfg);
>  

-- 
Regards,

Sakari Ailus

[PATCH v4 4/6] media: ipu-bridge: Assign endpoint property indices dynamically

[Fernando Rimoli]

The endpoint property array is populated with hardcoded indices, so a
property that is only set conditionally has to be placed at a fixed slot.
As the array is NULL-terminated, such a property is silently dropped when
an earlier optional slot is left empty: "link-frequencies" is skipped for
configs with nr_link_freqs == 0, which would truncate the array before
anything following it.

Name the endpoint property slots in an enum, size the array accordingly
and assign the indices through a bounds-checked running index, as done
for the MIPI DisCo for Imaging properties in mipi-disco-img.c. No
functional change intended: the same properties are set in the same
order.

Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.com>
---
 drivers/media/pci/intel/ipu-bridge.c | 27 ++++++++++++++-------------
 include/media/ipu-bridge.h           | 19 ++++++++++++++++++-
 2 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index 131c70844..cd3c36d44 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -449,6 +449,7 @@ static void ipu_bridge_create_fwnode_properties(
 	struct ipu_bridge *bridge,
 	const struct ipu_sensor_config *cfg)
 {
+	unsigned int i = IPU_SENSOR_EP_BUS_TYPE;
 	struct ipu_property_names *names = &sensor->prop_names;
 	struct software_node *nodes = sensor->swnodes;
 
@@ -508,21 +509,21 @@ static void ipu_bridge_create_fwnode_properties(
 			PROPERTY_ENTRY_REF_ARRAY("lens-focus", sensor->vcm_ref);
 	}
 
-	sensor->ep_properties[0] = PROPERTY_ENTRY_U32(
-					sensor->prop_names.bus_type,
-					V4L2_FWNODE_BUS_TYPE_CSI2_DPHY);
-	sensor->ep_properties[1] = PROPERTY_ENTRY_U32_ARRAY_LEN(
-					sensor->prop_names.data_lanes,
-					bridge->data_lanes, sensor->lanes);
-	sensor->ep_properties[2] = PROPERTY_ENTRY_REF_ARRAY(
-					sensor->prop_names.remote_endpoint,
-					sensor->local_ref);
+	sensor->ep_properties[IPU_NEXT_EP_PROPERTY(i, NUM_OF)] =
+		PROPERTY_ENTRY_U32(names->bus_type,
+				   V4L2_FWNODE_BUS_TYPE_CSI2_DPHY);
+	sensor->ep_properties[IPU_NEXT_EP_PROPERTY(i, NUM_OF)] =
+		PROPERTY_ENTRY_U32_ARRAY_LEN(names->data_lanes,
+					     bridge->data_lanes, sensor->lanes);
+	sensor->ep_properties[IPU_NEXT_EP_PROPERTY(i, NUM_OF)] =
+		PROPERTY_ENTRY_REF_ARRAY(names->remote_endpoint,
+					 sensor->local_ref);
 
 	if (cfg->nr_link_freqs > 0)
-		sensor->ep_properties[3] = PROPERTY_ENTRY_U64_ARRAY_LEN(
-			sensor->prop_names.link_frequencies,
-			cfg->link_freqs,
-			cfg->nr_link_freqs);
+		sensor->ep_properties[IPU_NEXT_EP_PROPERTY(i, NUM_OF)] =
+			PROPERTY_ENTRY_U64_ARRAY_LEN(names->link_frequencies,
+						     cfg->link_freqs,
+						     cfg->nr_link_freqs);
 
 	sensor->ipu_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(
 					sensor->prop_names.data_lanes,
diff --git a/include/media/ipu-bridge.h b/include/media/ipu-bridge.h
index 16fac7654..61e10cef1 100644
--- a/include/media/ipu-bridge.h
+++ b/include/media/ipu-bridge.h
@@ -64,6 +64,23 @@ enum ipu_sensor_swnodes {
 	SWNODE_COUNT
 };
 
+enum ipu_sensor_ep_props {
+	IPU_SENSOR_EP_BUS_TYPE,
+	IPU_SENSOR_EP_DATA_LANES,
+	IPU_SENSOR_EP_REMOTE_EP,
+	IPU_SENSOR_EP_LINK_FREQUENCIES,
+	IPU_SENSOR_EP_NUM_OF,
+	IPU_SENSOR_EP_NUM_ENTRIES
+};
+
+/*
+ * Get the index of the next endpoint property in the property array, with a
+ * given maximum value.
+ */
+#define IPU_NEXT_EP_PROPERTY(index, max)		\
+	(WARN_ON((index) > IPU_SENSOR_EP_##max) ?	\
+	 IPU_SENSOR_EP_##max : (index)++)
+
 /* Data representation as it is in ACPI SSDB buffer */
 struct ipu_sensor_ssdb {
 	u8 version;
@@ -141,7 +158,7 @@ struct ipu_sensor {
 	const char *vcm_type;
 
 	struct ipu_property_names prop_names;
-	struct property_entry ep_properties[5];
+	struct property_entry ep_properties[IPU_SENSOR_EP_NUM_ENTRIES];
 	struct property_entry dev_properties[5];
 	struct property_entry ipu_properties[3];
 	struct property_entry ivsc_properties[1];
-- 
2.43.0

Re: [PATCH v4 4/6] media: ipu-bridge: Assign endpoint property indices dynamically

[Sakari Ailus]

On Mon, Aug 31, 2026 at 08:18:56PM +0200, Fernando Rimoli wrote:
> The endpoint property array is populated with hardcoded indices, so a
> property that is only set conditionally has to be placed at a fixed slot.
> As the array is NULL-terminated, such a property is silently dropped when
> an earlier optional slot is left empty: "link-frequencies" is skipped for
> configs with nr_link_freqs == 0, which would truncate the array before
> anything following it.
> 
> Name the endpoint property slots in an enum, size the array accordingly
> and assign the indices through a bounds-checked running index, as done
> for the MIPI DisCo for Imaging properties in mipi-disco-img.c. No
> functional change intended: the same properties are set in the same
> order.

There's a lot of unneeded information here. For simple patches like this
there's no need to go deep in details that can be easily seen from the
code. How about:

Index the ep_properties array dynamically instead of plain numerical values
as is done in mipi-disco-img.c.

> 
> Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.com>
> ---
>  drivers/media/pci/intel/ipu-bridge.c | 27 ++++++++++++++-------------
>  include/media/ipu-bridge.h           | 19 ++++++++++++++++++-
>  2 files changed, 32 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> index 131c70844..cd3c36d44 100644
> --- a/drivers/media/pci/intel/ipu-bridge.c
> +++ b/drivers/media/pci/intel/ipu-bridge.c
> @@ -449,6 +449,7 @@ static void ipu_bridge_create_fwnode_properties(
>  	struct ipu_bridge *bridge,
>  	const struct ipu_sensor_config *cfg)
>  {
> +	unsigned int i = IPU_SENSOR_EP_BUS_TYPE;

You can just initialise this to 0.

>  	struct ipu_property_names *names = &sensor->prop_names;
>  	struct software_node *nodes = sensor->swnodes;
>  
> @@ -508,21 +509,21 @@ static void ipu_bridge_create_fwnode_properties(
>  			PROPERTY_ENTRY_REF_ARRAY("lens-focus", sensor->vcm_ref);
>  	}
>  
> -	sensor->ep_properties[0] = PROPERTY_ENTRY_U32(
> -					sensor->prop_names.bus_type,
> -					V4L2_FWNODE_BUS_TYPE_CSI2_DPHY);
> -	sensor->ep_properties[1] = PROPERTY_ENTRY_U32_ARRAY_LEN(
> -					sensor->prop_names.data_lanes,
> -					bridge->data_lanes, sensor->lanes);
> -	sensor->ep_properties[2] = PROPERTY_ENTRY_REF_ARRAY(
> -					sensor->prop_names.remote_endpoint,
> -					sensor->local_ref);
> +	sensor->ep_properties[IPU_NEXT_EP_PROPERTY(i, NUM_OF)] =

You should use the maximum value of the property index here. Same below.
Right now it's fairly simple, but in more complicated cases it helps
keeping track of the properties.

> +		PROPERTY_ENTRY_U32(names->bus_type,
> +				   V4L2_FWNODE_BUS_TYPE_CSI2_DPHY);
> +	sensor->ep_properties[IPU_NEXT_EP_PROPERTY(i, NUM_OF)] =
> +		PROPERTY_ENTRY_U32_ARRAY_LEN(names->data_lanes,
> +					     bridge->data_lanes, sensor->lanes);
> +	sensor->ep_properties[IPU_NEXT_EP_PROPERTY(i, NUM_OF)] =
> +		PROPERTY_ENTRY_REF_ARRAY(names->remote_endpoint,
> +					 sensor->local_ref);
>  
>  	if (cfg->nr_link_freqs > 0)
> -		sensor->ep_properties[3] = PROPERTY_ENTRY_U64_ARRAY_LEN(
> -			sensor->prop_names.link_frequencies,
> -			cfg->link_freqs,
> -			cfg->nr_link_freqs);
> +		sensor->ep_properties[IPU_NEXT_EP_PROPERTY(i, NUM_OF)] =
> +			PROPERTY_ENTRY_U64_ARRAY_LEN(names->link_frequencies,
> +						     cfg->link_freqs,
> +						     cfg->nr_link_freqs);
>  
>  	sensor->ipu_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(
>  					sensor->prop_names.data_lanes,
> diff --git a/include/media/ipu-bridge.h b/include/media/ipu-bridge.h
> index 16fac7654..61e10cef1 100644
> --- a/include/media/ipu-bridge.h
> +++ b/include/media/ipu-bridge.h
> @@ -64,6 +64,23 @@ enum ipu_sensor_swnodes {
>  	SWNODE_COUNT
>  };
>  
> +enum ipu_sensor_ep_props {
> +	IPU_SENSOR_EP_BUS_TYPE,
> +	IPU_SENSOR_EP_DATA_LANES,
> +	IPU_SENSOR_EP_REMOTE_EP,
> +	IPU_SENSOR_EP_LINK_FREQUENCIES,
> +	IPU_SENSOR_EP_NUM_OF,
> +	IPU_SENSOR_EP_NUM_ENTRIES
> +};
> +
> +/*
> + * Get the index of the next endpoint property in the property array, with a
> + * given maximum value.
> + */
> +#define IPU_NEXT_EP_PROPERTY(index, max)		\

The macro isn't limited to endpoint properties, how about calling it
e.g. IPU_BRIDGE_NEXT_PROPERTY?

> +	(WARN_ON((index) > IPU_SENSOR_EP_##max) ?	\

How about:

s/SENSOR_EP/BRIDGE/

> +	 IPU_SENSOR_EP_##max : (index)++)
> +
>  /* Data representation as it is in ACPI SSDB buffer */
>  struct ipu_sensor_ssdb {
>  	u8 version;
> @@ -141,7 +158,7 @@ struct ipu_sensor {
>  	const char *vcm_type;
>  
>  	struct ipu_property_names prop_names;
> -	struct property_entry ep_properties[5];
> +	struct property_entry ep_properties[IPU_SENSOR_EP_NUM_ENTRIES];
>  	struct property_entry dev_properties[5];
>  	struct property_entry ipu_properties[3];
>  	struct property_entry ivsc_properties[1];

-- 
Regards,

Sakari Ailus

[PATCH v4 5/6] media: ipu-bridge: Match sensor configs per IPU and add config flags

[Fernando Rimoli]

Some sensors need different treatment depending on which IPU they are
connected to, so the sensor's ACPI HID alone is not always enough to
describe what the bridge has to set up.

Add an optional IPU PCI product ID and a set of flags to struct
ipu_sensor_config, along with an IPU_SENSOR_CONFIG_MATCH_FL() macro to
define such an entry. A config naming a PCI product ID only applies to
that IPU and takes precedence over a generic config for the same sensor,
so that a sensor covered by both is connected once, through the more
specific entry. Existing entries are unchanged and keep matching any IPU.

No flags are defined yet and no entry uses the new macro, so there is no
functional change.

Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.com>
---
 drivers/media/pci/intel/ipu-bridge.c | 31 ++++++++++++++++++++++++++++
 include/media/ipu-bridge.h           | 29 +++++++++++++++++++++-----
 2 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index cd3c36d44..38ad3e54e 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -8,6 +8,7 @@
 #include <linux/dmi.h>
 #include <linux/i2c.h>
 #include <linux/mei_cl_bus.h>
+#include <linux/pci.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/property.h>
@@ -853,6 +854,32 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
 	return ret;
 }
 
+/*
+ * Whether a sensor config applies to the IPU the bridge sits on. A config
+ * naming a PCI product ID only applies to that IPU, and takes precedence over
+ * a generic config for the same sensor, which is skipped so that the sensor is
+ * not connected twice.
+ */
+static bool ipu_bridge_config_matches(const struct ipu_sensor_config *cfg,
+				      struct ipu_bridge *bridge)
+{
+	unsigned int i;
+
+	if (cfg->pci_id)
+		return cfg->pci_id == bridge->pci_id;
+
+	for (i = 0; i < ARRAY_SIZE(ipu_supported_sensors); i++) {
+		const struct ipu_sensor_config *sp =
+			&ipu_supported_sensors[i];
+
+		if (sp->pci_id && sp->pci_id == bridge->pci_id &&
+		    !strcmp(sp->hid, cfg->hid))
+			return false;
+	}
+
+	return true;
+}
+
 static int ipu_bridge_connect_sensors(struct ipu_bridge *bridge)
 {
 	unsigned int i;
@@ -862,6 +889,9 @@ static int ipu_bridge_connect_sensors(struct ipu_bridge *bridge)
 		const struct ipu_sensor_config *cfg =
 			&ipu_supported_sensors[i];
 
+		if (!ipu_bridge_config_matches(cfg, bridge))
+			continue;
+
 		ret = ipu_bridge_connect_sensor(cfg, bridge);
 		if (ret)
 			goto err_unregister_sensors;
@@ -948,6 +978,7 @@ int ipu_bridge_init(struct device *dev,
 		sizeof(bridge->ipu_node_name));
 	bridge->ipu_hid_node.name = bridge->ipu_node_name;
 	bridge->dev = dev;
+	bridge->pci_id = dev_is_pci(dev) ? to_pci_dev(dev)->device : 0;
 	bridge->parse_sensor_fwnode = parse_sensor_fwnode;
 
 	ret = software_node_register(&bridge->ipu_hid_node);
diff --git a/include/media/ipu-bridge.h b/include/media/ipu-bridge.h
index 61e10cef1..d12e51336 100644
--- a/include/media/ipu-bridge.h
+++ b/include/media/ipu-bridge.h
@@ -17,13 +17,27 @@
 #define IPU_SENSOR_ROTATION_NORMAL		0
 #define IPU_SENSOR_ROTATION_INVERTED		1
 
-#define IPU_SENSOR_CONFIG(_HID, _NR, ...)	\
-	(const struct ipu_sensor_config) {	\
-		.hid = _HID,			\
-		.nr_link_freqs = _NR,		\
-		.link_freqs = { __VA_ARGS__ }	\
+/* Flags for struct ipu_sensor_config */
+#define IPU_BR_FL_NONE				0
+
+/*
+ * Sensor config specific to a single IPU, identified by its PCI product ID,
+ * with flags describing what the sensor needs on that IPU. Where both a
+ * specific and a generic (IPU_SENSOR_CONFIG) entry exist for the same HID,
+ * the specific one takes precedence.
+ */
+#define IPU_SENSOR_CONFIG_MATCH_FL(_HID, _ID, _FLAGS, _NR, ...)	\
+	(const struct ipu_sensor_config) {			\
+		.hid = _HID,					\
+		.pci_id = _ID,					\
+		.flags = IPU_BR_FL_##_FLAGS,			\
+		.nr_link_freqs = _NR,				\
+		.link_freqs = { __VA_ARGS__ }			\
 	}
 
+#define IPU_SENSOR_CONFIG(_HID, _NR, ...)			\
+	IPU_SENSOR_CONFIG_MATCH_FL(_HID, 0, NONE, _NR, __VA_ARGS__)
+
 #define NODE_SENSOR(_HID, _PROPS)		\
 	(const struct software_node) {		\
 		.name = _HID,			\
@@ -132,6 +146,9 @@ struct ipu_node_names {
 
 struct ipu_sensor_config {
 	const char *hid;
+	/* IPU PCI product ID this config is specific to, 0 for any */
+	const u16 pci_id;
+	const u32 flags;
 	const u8 nr_link_freqs;
 	const u64 link_freqs[MAX_NUM_LINK_FREQS];
 };
@@ -177,6 +194,8 @@ typedef int (*ipu_parse_sensor_fwnode_t)(struct acpi_device *adev,
 
 struct ipu_bridge {
 	struct device *dev;
+	/* PCI product ID of the IPU, 0 if it is not a PCI device */
+	u16 pci_id;
 	ipu_parse_sensor_fwnode_t parse_sensor_fwnode;
 	char ipu_node_name[ACPI_ID_LEN];
 	struct software_node ipu_hid_node;
-- 
2.43.0

Re: [PATCH v4 5/6] media: ipu-bridge: Match sensor configs per IPU and add config flags

[Fernando Rimoli]

Sakari, Dan,

The semantic I asked you both to check in this patch, where a PCI-specific
entry wins and the generic entry for the same HID is skipped, now has
hardware confirmation rather than only my reasoning, so you may not need to
spend thought on it.

A linux-surface user tested the series on a Surface Pro 8 (Tiger Lake IPU6,
0x9a19, ov5693 as INT33BE) on a machine that has INT33BE in the table
twice, the generic entry plus the flagged one this patch adds, alongside
two other sensors on other ports. The bridge connects each sensor exactly
once:

  intel-ipu6 0000:00:05.0: Found supported sensor INT33BE:00
  intel-ipu6 0000:00:05.0: Found supported sensor OVTID858:00
  intel-ipu6 0000:00:05.0: Found supported sensor SMO55F0:00
  intel-ipu6 0000:00:05.0: Connected 3 cameras

Three sensors, three connections, no double-connect and no port consumed
twice, which is the failure I was trying to avoid. The other two sensors
take no flags and came up normally afterwards, including an IR sensor that
still streams and still serves face authentication on that machine, so the
filter does not perturb entries it should not touch.

He also read MIPI_CTRL00 back over I2C during a live capture and got 0x20,
which means the flagged INT33BE plus PCI_DEVICE_ID_INTEL_IPU6 entry is what
matched and the property reached the sensor endpoint. So the path from the
table through to the register is exercised, not only the end result. That
is a second Tiger Lake confirmation independent of Jakob's on a Surface Pro
7+ elsewhere in this thread.

Two caveats on how much this carries. His tree is 7.2.2 with the
linux-surface patches rather than the v7.3-rc1 base this series declares,
and that tree carries its own duplicate OVTI5693 entry which he had to drop
for patch 6 to apply, since patch 2 adds it properly. Jakob's test was on
the declared base. Both are good, but they are not the same tree so I just
wanted to have that on record.

He intends to send his own Tested-by, and has offered to run the negative
control, the same build with the PCI_DEVICE_ID_INTEL_IPU6 entry removed,
expecting no frames. I have asked him to do it. That would show the table
entry is what makes this work rather than something incidental, which is
the one thing none of the positive results establish. I will report the
outcome either way.

Thanks,
Fernando

Re: [PATCH v4 5/6] media: ipu-bridge: Match sensor configs per IPU and add config flags

[Sakari Ailus]

Hi Fernando,

On Mon, Aug 31, 2026 at 08:18:57PM +0200, Fernando Rimoli wrote:
> Some sensors need different treatment depending on which IPU they are
> connected to, so the sensor's ACPI HID alone is not always enough to
> describe what the bridge has to set up.
> 
> Add an optional IPU PCI product ID and a set of flags to struct
> ipu_sensor_config, along with an IPU_SENSOR_CONFIG_MATCH_FL() macro to
> define such an entry. A config naming a PCI product ID only applies to
> that IPU and takes precedence over a generic config for the same sensor,
> so that a sensor covered by both is connected once, through the more
> specific entry. Existing entries are unchanged and keep matching any IPU.
> 
> No flags are defined yet and no entry uses the new macro, so there is no
> functional change.

There's quite a bit of irrelevant information here.

> 
> Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.com>
> ---
>  drivers/media/pci/intel/ipu-bridge.c | 31 ++++++++++++++++++++++++++++
>  include/media/ipu-bridge.h           | 29 +++++++++++++++++++++-----
>  2 files changed, 55 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> index cd3c36d44..38ad3e54e 100644
> --- a/drivers/media/pci/intel/ipu-bridge.c
> +++ b/drivers/media/pci/intel/ipu-bridge.c
> @@ -8,6 +8,7 @@
>  #include <linux/dmi.h>
>  #include <linux/i2c.h>
>  #include <linux/mei_cl_bus.h>
> +#include <linux/pci.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/property.h>
> @@ -853,6 +854,32 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
>  	return ret;
>  }
>  
> +/*
> + * Whether a sensor config applies to the IPU the bridge sits on. A config
> + * naming a PCI product ID only applies to that IPU, and takes precedence over
> + * a generic config for the same sensor, which is skipped so that the sensor is
> + * not connected twice.
> + */
> +static bool ipu_bridge_config_matches(const struct ipu_sensor_config *cfg,
> +				      struct ipu_bridge *bridge)
> +{
> +	unsigned int i;
> +
> +	if (cfg->pci_id)
> +		return cfg->pci_id == bridge->pci_id;
> +
> +	for (i = 0; i < ARRAY_SIZE(ipu_supported_sensors); i++) {

Is there really a need to go through the entire array for each entry? Can't
you simply arrange the entries with a pci_id before the generic one?

> +		const struct ipu_sensor_config *sp =
> +			&ipu_supported_sensors[i];
> +
> +		if (sp->pci_id && sp->pci_id == bridge->pci_id &&
> +		    !strcmp(sp->hid, cfg->hid))
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
>  static int ipu_bridge_connect_sensors(struct ipu_bridge *bridge)
>  {
>  	unsigned int i;
> @@ -862,6 +889,9 @@ static int ipu_bridge_connect_sensors(struct ipu_bridge *bridge)
>  		const struct ipu_sensor_config *cfg =
>  			&ipu_supported_sensors[i];
>  
> +		if (!ipu_bridge_config_matches(cfg, bridge))
> +			continue;
> +
>  		ret = ipu_bridge_connect_sensor(cfg, bridge);
>  		if (ret)
>  			goto err_unregister_sensors;
> @@ -948,6 +978,7 @@ int ipu_bridge_init(struct device *dev,
>  		sizeof(bridge->ipu_node_name));
>  	bridge->ipu_hid_node.name = bridge->ipu_node_name;
>  	bridge->dev = dev;
> +	bridge->pci_id = dev_is_pci(dev) ? to_pci_dev(dev)->device : 0;
>  	bridge->parse_sensor_fwnode = parse_sensor_fwnode;
>  
>  	ret = software_node_register(&bridge->ipu_hid_node);
> diff --git a/include/media/ipu-bridge.h b/include/media/ipu-bridge.h
> index 61e10cef1..d12e51336 100644
> --- a/include/media/ipu-bridge.h
> +++ b/include/media/ipu-bridge.h
> @@ -17,13 +17,27 @@
>  #define IPU_SENSOR_ROTATION_NORMAL		0
>  #define IPU_SENSOR_ROTATION_INVERTED		1
>  
> -#define IPU_SENSOR_CONFIG(_HID, _NR, ...)	\
> -	(const struct ipu_sensor_config) {	\
> -		.hid = _HID,			\
> -		.nr_link_freqs = _NR,		\
> -		.link_freqs = { __VA_ARGS__ }	\
> +/* Flags for struct ipu_sensor_config */
> +#define IPU_BR_FL_NONE				0
> +
> +/*
> + * Sensor config specific to a single IPU, identified by its PCI product ID,
> + * with flags describing what the sensor needs on that IPU. Where both a
> + * specific and a generic (IPU_SENSOR_CONFIG) entry exist for the same HID,
> + * the specific one takes precedence.
> + */
> +#define IPU_SENSOR_CONFIG_MATCH_FL(_HID, _ID, _FLAGS, _NR, ...)	\
> +	(const struct ipu_sensor_config) {			\
> +		.hid = _HID,					\
> +		.pci_id = _ID,					\
> +		.flags = IPU_BR_FL_##_FLAGS,			\

Please don't assume a flag; setting multiple flags also doesn't work this
way.

> +		.nr_link_freqs = _NR,				\
> +		.link_freqs = { __VA_ARGS__ }			\
>  	}
>  
> +#define IPU_SENSOR_CONFIG(_HID, _NR, ...)			\
> +	IPU_SENSOR_CONFIG_MATCH_FL(_HID, 0, NONE, _NR, __VA_ARGS__)
> +
>  #define NODE_SENSOR(_HID, _PROPS)		\
>  	(const struct software_node) {		\
>  		.name = _HID,			\
> @@ -132,6 +146,9 @@ struct ipu_node_names {
>  
>  struct ipu_sensor_config {
>  	const char *hid;
> +	/* IPU PCI product ID this config is specific to, 0 for any */
> +	const u16 pci_id;

In later patches we already get two extra entries per sensor that only
differ on pci_id. How about making this a pointer to an array? Zero
termination should be fine here.

> +	const u32 flags;
>  	const u8 nr_link_freqs;
>  	const u64 link_freqs[MAX_NUM_LINK_FREQS];
>  };
> @@ -177,6 +194,8 @@ typedef int (*ipu_parse_sensor_fwnode_t)(struct acpi_device *adev,
>  
>  struct ipu_bridge {
>  	struct device *dev;
> +	/* PCI product ID of the IPU, 0 if it is not a PCI device */

All IPUs are PCI devices. ipu_bridge_init() should fail if a device isn't.
I think I might just omit the check.

> +	u16 pci_id;
>  	ipu_parse_sensor_fwnode_t parse_sensor_fwnode;
>  	char ipu_node_name[ACPI_ID_LEN];
>  	struct software_node ipu_hid_node;

-- 
Regards,

Sakari Ailus

[PATCH v4 6/6] media: ipu-bridge: Request non-continuous clock for ov5693 on IPU6

[Fernando Rimoli]

The ov5693 (INT33BE / OVTI5693) needs its MIPI clock lane gated while
idle for the IPU6 CSI-2 receiver to lock onto the link; otherwise the
sensor streams but capture times out and no frames arrive. The sensor
driver gates the clock lane when the endpoint requests a non-continuous
clock, so the bridge has to supply that property.

The same sensor works with a free-running clock on the IPU3, so add
IPU_BR_FL_CSI2_CLK_NONCONTINUOUS and set it only for the ov5693 on the
IPU6 variants where the gate is known to be needed: IPU6 (Tiger Lake,
tested on a Surface Pro 7+) and IPU6EP_ADLP (Alder Lake-P, tested on a
Surface Pro 9). Other IPU6 product IDs are left out until someone can
confirm them on hardware; the generic entries continue to apply there
and on the IPU3.

Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.com>
---
 drivers/media/pci/intel/ipu-bridge.c | 13 +++++++++++++
 include/media/ipu-bridge.h           |  3 +++
 2 files changed, 16 insertions(+)

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index 38ad3e54e..7498531f2 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -16,6 +16,7 @@
 #include <linux/workqueue.h>
 
 #include <media/ipu-bridge.h>
+#include <media/ipu6-pci-table.h>
 #include <media/v4l2-fwnode.h>
 
 #define ADEV_DEV(adev) ACPI_PTR(&((adev)->dev))
@@ -62,6 +63,10 @@ static const struct ipu_sensor_config ipu_supported_sensors[] = {
 	IPU_SENSOR_CONFIG("INT0310", 1, 55692000),
 	/* Omnivision OV5693 */
 	IPU_SENSOR_CONFIG("INT33BE", 1, 419200000),
+	IPU_SENSOR_CONFIG_MATCH_FL("INT33BE", PCI_DEVICE_ID_INTEL_IPU6,
+				   CSI2_CLK_NONCONTINUOUS, 1, 419200000),
+	IPU_SENSOR_CONFIG_MATCH_FL("INT33BE", PCI_DEVICE_ID_INTEL_IPU6EP_ADLP,
+				   CSI2_CLK_NONCONTINUOUS, 1, 419200000),
 	/* Onsemi MT9M114 */
 	IPU_SENSOR_CONFIG("INT33F0", 1, 384000000),
 	/* Omnivision OV2740 */
@@ -98,6 +103,10 @@ static const struct ipu_sensor_config ipu_supported_sensors[] = {
 	IPU_SENSOR_CONFIG("OVTI5675", 1, 450000000),
 	/* Omnivision OV5693 */
 	IPU_SENSOR_CONFIG("OVTI5693", 1, 419200000),
+	IPU_SENSOR_CONFIG_MATCH_FL("OVTI5693", PCI_DEVICE_ID_INTEL_IPU6,
+				   CSI2_CLK_NONCONTINUOUS, 1, 419200000),
+	IPU_SENSOR_CONFIG_MATCH_FL("OVTI5693", PCI_DEVICE_ID_INTEL_IPU6EP_ADLP,
+				   CSI2_CLK_NONCONTINUOUS, 1, 419200000),
 	/* Omnivision OV8856 */
 	IPU_SENSOR_CONFIG("OVTI8856", 3, 180000000, 360000000, 720000000),
 	/* Sony IMX471 */
@@ -526,6 +535,10 @@ static void ipu_bridge_create_fwnode_properties(
 						     cfg->link_freqs,
 						     cfg->nr_link_freqs);
 
+	if (cfg->flags & IPU_BR_FL_CSI2_CLK_NONCONTINUOUS)
+		sensor->ep_properties[IPU_NEXT_EP_PROPERTY(i, NUM_OF)] =
+			PROPERTY_ENTRY_BOOL("clock-noncontinuous");
+
 	sensor->ipu_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(
 					sensor->prop_names.data_lanes,
 					bridge->data_lanes, sensor->lanes);
diff --git a/include/media/ipu-bridge.h b/include/media/ipu-bridge.h
index d12e51336..a5f8c5d4b 100644
--- a/include/media/ipu-bridge.h
+++ b/include/media/ipu-bridge.h
@@ -19,6 +19,8 @@
 
 /* Flags for struct ipu_sensor_config */
 #define IPU_BR_FL_NONE				0
+/* The sensor's CSI-2 transmitter needs a non-continuous clock */
+#define IPU_BR_FL_CSI2_CLK_NONCONTINUOUS	BIT(0)
 
 /*
  * Sensor config specific to a single IPU, identified by its PCI product ID,
@@ -83,6 +85,7 @@ enum ipu_sensor_ep_props {
 	IPU_SENSOR_EP_DATA_LANES,
 	IPU_SENSOR_EP_REMOTE_EP,
 	IPU_SENSOR_EP_LINK_FREQUENCIES,
+	IPU_SENSOR_EP_CLOCK_NONCONTINUOUS,
 	IPU_SENSOR_EP_NUM_OF,
 	IPU_SENSOR_EP_NUM_ENTRIES
 };
-- 
2.43.0

Re: [PATCH v4 6/6] media: ipu-bridge: Request non-continuous clock for ov5693 on IPU6

[Kengo Oki]

Hi Fernando,

I completed the Surface Go 4 A/B test for the proposed Alder Lake-N entry.

Test system:

* Microsoft Surface Go 4
* IPU6: 8086:462e (Alder Lake-N)
* Front sensor: OV5693
* Sensor enumeration: INT33BE:00
* Kernel: Ubuntu 7.0.0-30-generic

I tested the v4 semantics as a minimal backport to Ubuntu 7.0.0-30.30. Patch 2 needed a small semantic backport because Ubuntu's sensor table differs from the upstream base.

Without the ADL-N INT33BE match, after a cold boot the first deliberate front-camera capture produced:

```
requested_frames=300
frame_count=0
exit_code=124
```

with:

```
intel_ipu6_isys.isys intel_ipu6.isys.40: stream stop time out
intel_ipu6_isys.isys intel_ipu6.isys.40: stream close time out
```

I then added only:

```
IPU_SENSOR_CONFIG_MATCH_FL("INT33BE",
                           PCI_DEVICE_ID_INTEL_IPU6EP_ADLN,
                           CSI2_CLK_NONCONTINUOUS, 1, 419200000),
```

After a full power-off and cold boot, with no earlier IPU6 stream activity in that boot, the same 300-frame test produced:

```
requested_frames=300
frame_count=300
exit_code=0
last sequence=000299
~28.64 fps
```

There were no relevant stream/CSI timeout or error messages during the successful run.

So on this Surface Go 4 the A/B result is:

```
v4 without ADL-N match: 0/300, timeout
v4 with ADL-N match:    300/300, success
```

This unit enumerates the OV5693 as INT33BE:00, not OVTI5693.

Full test/build evidence:
https://github.com/Fugu0141/Surface-Go4-IPU6-camera-linux/blob/main/tests/2026-09-01-ov5693-v4-adln/RESULT.md

For v5 with the ADL-N entry added:

Tested-by: Kengo Oki <dev.kengo.fugu0141@gmail.com> # Surface Go 4, IPU6 Alder Lake-N 8086:462e

[PATCH v5 0/7] media: Enable the OV5693 front camera on IPU6 Surface devices

[Fernando Rimoli]

The ov5693 front camera does not stream on IPU6 Surface devices. The sensor needs
its MIPI clock lane gated while idle, which needs a driver change and a way for
the ipu-bridge to ask for it per IPU.

v4 is at [1]. This addresses Sakari's review of patches 3, 4 and 5, adds the DT
bindings property he asked for, and adds an Alder Lake-N entry now that a Surface
Go 4 has been measured.

Changes since v4:

- New patch 3 adds "clock-noncontinuous" to the ov5693 DT bindings. The series is
  now seven patches; v4's patches 3 to 6 are 4 to 7 here, and the Tested-by tags
  are scoped to those four.
- Commit messages cut throughout, per your comments on v4. Patch 5 now carries
  your wording and nothing else. Patch 4 lost the ov5647 comparison and the
  per-machine failure detail, which is below instead. Patches 6 and 7 trimmed.
- Patch 5: the running index starts at 0, each call site passes its own property's
  maximum index rather than a shared one, and the macro is IPU_BRIDGE_NEXT_PROPERTY
  with the enum renamed to match, following mipi-disco-img.c more closely.
- Patch 6: struct ipu_sensor_config now carries a zero-terminated list of IPU PCI
  product IDs rather than a single ID, so one entry covers several IPUs. The flags
  macro argument is a plain value, so flags can be combined. The scan of the whole
  table is gone: entries for one HID are adjacent with the IPU-specific ones first,
  and the generic entry is skipped once a specific one has matched.
- Patch 7: the four entries collapse to one shared ID list, with Alder Lake-N added.

On the enum naming in patch 5: "s/SENSOR_EP/BRIDGE/" taken literally gives
IPU_BRIDGE_BUS_TYPE, but since the macro is not endpoint-specific and may later
serve dev_properties, I kept the group marker as IPU_BRIDGE_EP_*, which is also how
mipi-disco-img.c separates its EP and DEV entries. Say the word if you meant the
shorter form.

Unpatched behaviour, which patch 4 now only summarises:

Four people measured it on five machines and it is not uniform. It is absolute on
three (a Pro 9, a Pro 7+ and a Go 4). On another Pro 7+ it is intermittent, 3 of 20
opens delivering frames inside a single boot, with the successes at opens 1, 7 and
14 and the sensor runtime-suspending in between. On a Pro 8 the first capture after
a cold boot succeeds and every later one in that boot hangs. No mechanism explains
the spread: it is not once per boot, given where that machine's successes fell, and
it is not Tiger Lake versus Alder Lake, since two Tiger Lake machines fall on
opposite sides. The sensor is up during the failures on every machine checked: the
privacy LED lights, the failing runs log no CSI-2 errors where the working ones log
ten or more, and setting bit 5 over i2c on an already stalled stream starts delivery
immediately.

**If you verify this, capture twice.** On one of the five machines a single capture
immediately after a cold boot succeeds without the flagged entry, so the obvious
check gives a false negative. Both testers who hit this asked for it to be written
down.

Two other things worth knowing:

- The teardown "stream stop time out" appears identically with and without this
  series, so it is pre-existing and not introduced here.
- checkpatch flags "Macros with complex values should be enclosed in parentheses" on
  patch 6. The pristine header already trips it five times and the patched header
  still trips it exactly five times, because IPU_SENSOR_CONFIG becomes a wrapper as
  IPU_SENSOR_CONFIG_MATCH_FL takes over the compound literal. No net new instance.

Testing coverage, all scoped to patches 4 to 7. Patches 1 and 2 are not exercised by
the testers, whose machines enumerate INT33BE; the Pro 9 is the OVTI5693 machine.

  Tiger Lake   0x9a19  Pro 7+ (Jakob), Pro 8 (Fil Dunsky), Pro 7+ (Lucas Lis)
  Alder Lake-P 0x465d  Pro 9 (mine)
  Alder Lake-N 0x462e  Go 4 (Kengo Oki), 0/300 without the entry, 300/300 with

Kengo Oki's Alder Lake-N result is what justifies the third ID in patch 7, and his
tag is given for the version that carries it.

The reworked bridge in patches 5 to 7 was also run on the Pro 9 with all three of
its sensors: three cameras connected once each with no double connect, link
frequencies unchanged, and the front camera streaming at 28.63 fps, which is only
possible if the flag reaches the sensor driver through the new per-IPU match.

The Pro 9 measurement of 300 consecutive opens predates the switch from a full write
to cci_update_bits(), but the register reads 0x00 first there, so it resolves to the
same write.

[1] https://lore.kernel.org/linux-media/20260831181858.325109-1-fernandorimoli11@gmail.com/

Fernando Rimoli (7):
  media: i2c: ov5693: Add OVTI5693 ACPI HID
  media: ipu-bridge: Add OVTI5693 to the list of supported sensors
  dt-bindings: media: ov5693: Add clock-noncontinuous
  media: i2c: ov5693: Gate the MIPI clock lane for non-continuous clock
  media: ipu-bridge: Assign endpoint property indices dynamically
  media: ipu-bridge: Match sensor configs per IPU and add config flags
  media: ipu-bridge: Request non-continuous clock for ov5693 on IPU6

 .../bindings/media/i2c/ovti,ov5693.yaml       |  2 +
 drivers/media/i2c/ov5693.c                    | 25 ++++++
 drivers/media/pci/intel/ipu-bridge.c          | 83 ++++++++++++++++---
 include/media/ipu-bridge.h                    | 50 +++++++++--
 4 files changed, 141 insertions(+), 19 deletions(-)


base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
-- 
2.43.0

[PATCH v5 1/7] media: i2c: ov5693: Add OVTI5693 ACPI HID

[Fernando Rimoli]

The ov5693 driver only matches the "INT33BE" ACPI HID. Some Intel IPU6
Surface devices (e.g. Microsoft Surface Pro 9) enumerate the OV5693
front camera with the ACPI HID "OVTI5693" instead, so the i2c core never
binds the driver.

Add "OVTI5693" to the ACPI match table. Devices that use "INT33BE"
(e.g. Surface Go 4) are unaffected.

Tested on Surface Pro 9 (IPU6): the sensor enumerates as OVTI5693:00
(ACPI path \_SB_.PC00.I2C3.CAMF) and binds with this change.

Link: https://github.com/linux-surface/linux-surface/pull/2171
Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
---
 drivers/media/i2c/ov5693.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
index 4cc796bbe..02236f3db 100644
--- a/drivers/media/i2c/ov5693.c
+++ b/drivers/media/i2c/ov5693.c
@@ -1396,6 +1396,7 @@ static const struct dev_pm_ops ov5693_pm_ops = {
 
 static const struct acpi_device_id ov5693_acpi_match[] = {
 	{"INT33BE"},
+	{"OVTI5693"},
 	{},
 };
 MODULE_DEVICE_TABLE(acpi, ov5693_acpi_match);
-- 
2.43.0

[PATCH v5 2/7] media: ipu-bridge: Add OVTI5693 to the list of supported sensors

[Fernando Rimoli]

The IPU bridge builds the firmware node graph only for sensors listed in
ipu_supported_sensors[]. The OV5693 is currently listed only under its
legacy "INT33BE" HID, so on Intel IPU6 Surface devices that enumerate it
as "OVTI5693" (e.g. Microsoft Surface Pro 9) the bridge never wires up
the sensor and the front camera is unusable.

Add an "OVTI5693" entry. The link frequency (419200000) matches the
existing INT33BE entry, as it is the same sensor.

Tested on Surface Pro 9 (IPU6).

Link: https://github.com/linux-surface/linux-surface/pull/2171
Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.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 1bb3a3e98..131c70844 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -95,6 +95,8 @@ static const struct ipu_sensor_config ipu_supported_sensors[] = {
 	IPU_SENSOR_CONFIG("OVTI2680", 1, 331200000),
 	/* Omnivision OV5675 */
 	IPU_SENSOR_CONFIG("OVTI5675", 1, 450000000),
+	/* Omnivision OV5693 */
+	IPU_SENSOR_CONFIG("OVTI5693", 1, 419200000),
 	/* Omnivision OV8856 */
 	IPU_SENSOR_CONFIG("OVTI8856", 3, 180000000, 360000000, 720000000),
 	/* Sony IMX471 */
-- 
2.43.0

[PATCH v5 3/7] dt-bindings: media: ov5693: Add clock-noncontinuous

[Fernando Rimoli]

The sensor can gate its MIPI clock lane while idle, which the CSI-2
receivers on some platforms require. Allow the endpoint property that
asks for it, as ov5647 already does.

Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.com>
---
 Documentation/devicetree/bindings/media/i2c/ovti,ov5693.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5693.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5693.yaml
index 3368b3bd8..0d8ca0a51 100644
--- a/Documentation/devicetree/bindings/media/i2c/ovti,ov5693.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5693.yaml
@@ -82,6 +82,8 @@ properties:
         unevaluatedProperties: false
 
         properties:
+          clock-noncontinuous: true
+
           link-frequencies: true
 
           data-lanes:
-- 
2.43.0

Re: [PATCH v5 3/7] dt-bindings: media: ov5693: Add clock-noncontinuous

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 75 bytes --]

Acked-by: Conor Dooley <conor.dooley@microchip.com>
pw-bot: not-applicable

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v5 4/7] media: i2c: ov5693: Gate the MIPI clock lane for non-continuous clock

[Fernando Rimoli]

The ov5693 never programs MIPI_CTRL00 (0x4800), leaving it at its 0x00
power-on default, which lets the MIPI clock run freely. The IPU3 CSI-2
receiver tolerates this, but the IPU6 receiver (e.g. on Microsoft
Surface Pro 7+, Pro 8, Pro 9 and Surface Go 4) fails to lock onto the
link, so the sensor streams but capture times out with "stream stop
time out". On most affected machines no frames arrive at all; on some
the failure is intermittent.

Gate the clock lane while idle at stream on when the endpoint requests a
non-continuous clock.

Only the gate bit is touched, so platforms that do not request it are
unaffected. No counterpart is needed at stream off, as the link is down
by then and the register returns to its default when the sensor is
powered off.

The property is supplied by the ipu-bridge in a subsequent patch.

Link: https://github.com/linux-surface/linux-surface/pull/2171
Co-developed-by: Arsalan Naeem <naeemarsalan@gmail.com>
Signed-off-by: Arsalan Naeem <naeemarsalan@gmail.com>
Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.com>
Tested-by: Jakob Berg Jespersen <dev@berg.pm> # Surface Pro 7+, IPU6 Tiger Lake
Tested-by: Fil Dunsky <filipp.dunsky@gmail.com> # Surface Pro 8, IPU6 Tiger Lake (8086:9a19)
Tested-by: Lucas Lis <lucaseze.lis@gmail.com> # Surface Pro 7+, IPU6 Tiger Lake (0x9a19)
Tested-by: Kengo Oki <dev.kengo.fugu0141@gmail.com> # Surface Go 4, IPU6 Alder Lake-N 8086:462e
---
 drivers/media/i2c/ov5693.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
index 02236f3db..cedc6ea03 100644
--- a/drivers/media/i2c/ov5693.c
+++ b/drivers/media/i2c/ov5693.c
@@ -35,6 +35,11 @@
 #define OV5693_STOP_STREAMING			0x00
 #define OV5693_SW_RESET				0x01
 
+/* MIPI transmitter control */
+#define OV5693_MIPI_CTRL00_REG			CCI_REG8(0x4800)
+/* Gate the clock lane when there is no packet to transmit */
+#define OV5693_MIPI_CTRL00_CLOCK_LANE_GATE	BIT(5)
+
 #define OV5693_REG_CHIP_ID			CCI_REG16(0x300a)
 /* Yes, this is right. The datasheet for the OV5693 gives its ID as 0x5690 */
 #define OV5693_CHIP_ID				0x5690
@@ -144,6 +149,9 @@ struct ov5693_device {
 	struct regulator_bulk_data supplies[OV5693_NUM_SUPPLIES];
 	struct clk *xvclk;
 
+	/* Gate the MIPI clock lane when idle (CSI-2 non-continuous clock) */
+	bool clock_ncont;
+
 	struct ov5693_mode {
 		struct v4l2_rect crop;
 		struct v4l2_mbus_framefmt format;
@@ -611,6 +619,19 @@ static int ov5693_enable_streaming(struct ov5693_device *ov5693, bool enable)
 {
 	int ret = 0;
 
+	/*
+	 * Gate the MIPI clock lane while idle if the CSI-2 link is configured
+	 * for a non-continuous clock. Only that bit is touched, and only in
+	 * that case, so the register keeps whatever the platform left in it
+	 * and the clock stays free-running as before everywhere else. It
+	 * needs no counterpart at stream off: the link is down by then, and
+	 * the register returns to its default when the sensor is powered off.
+	 */
+	if (enable && ov5693->clock_ncont)
+		cci_update_bits(ov5693->regmap, OV5693_MIPI_CTRL00_REG,
+				OV5693_MIPI_CTRL00_CLOCK_LANE_GATE,
+				OV5693_MIPI_CTRL00_CLOCK_LANE_GATE, &ret);
+
 	cci_write(ov5693->regmap, OV5693_SW_STREAM_REG,
 		  enable ? OV5693_START_STREAMING : OV5693_STOP_STREAMING,
 		  &ret);
@@ -1259,6 +1280,9 @@ static int ov5693_check_hwcfg(struct ov5693_device *ov5693)
 		goto out_free_bus_cfg;
 	}
 
+	ov5693->clock_ncont = bus_cfg.bus.mipi_csi2.flags &
+			      V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK;
+
 out_free_bus_cfg:
 	v4l2_fwnode_endpoint_free(&bus_cfg);
 
-- 
2.43.0

Re: [PATCH v5 4/7] media: i2c: ov5693: Gate the MIPI clock lane for non-continuous clock

[Fil Dunsky]

Tested-by: Fil Dunsky <filipp.dunsky@gmail.com> # Surface Pro 8, IPU6 
Tiger Lake (8086:9a19)

Scope: patches 4-7, as in v4. Patches 1 and 2 are already in my tree and
this machine enumerates INT33BE, so neither is exercised here; patch 3 is
not either, there is no DT on this platform.

Retested rather than carried over, because patches 5 and 6 are not the
code I tagged in v4. Same machine and kernel as before: 7.2.2 plus the
linux-surface patch set. Patches 3 to 6 applied to it unchanged; patch 7
needed two hunks by hand, both because that tree's sensor table differs
from yours — it has no HIMX1092 entry, which the first hunk uses as
context, and it carries its own out-of-order OVTI5693 entry. I put the
flagged entry immediately before the generic one, as the new adjacency
rule requires.

Capture from the ISYS node, three times in a row with the sensor
runtime-suspending in between:

   60 frames, SBGGR10 2592x1944, 604661760 bytes, 28.63-28.65 fps  (x3)

   MIPI_CTRL00 (0x4800) read over i2c while streaming: 0x20
   MIPI_CTRL00 with no stream: i2c times out, the sensor is powered off

0x20 is bit 5 alone, so the property still reaches the sensor driver
through the reworked per-IPU match, and the whole path is exercised
rather than "the camera works". Three captures because of the warning in
your cover letter: on this machine the first one after a cold boot
succeeds even without the entry.

Patch 6's adjacency rule behaves as described. INT33BE appears twice in
this tree, the Tiger Lake entry and the generic one, and the sensor is
connected once: "Connected 3 cameras" and exactly one ov5693 subdev in
the media graph. I also ran it with the pristine upstream table, where
this machine has no third sensor: "Connected 2 cameras", one ov5693, and
the same three captures.

Patch 5 is exercised by the sensor probing at all. ov5693 refuses an
endpoint with no link frequencies, and then refuses one that does not
offer 419.2 MHz, so a probe that succeeds means the running index put
link-frequencies and clock-noncontinuous in the right slots and did not
displace anything. The other case, where nr_link_freqs is 0 and
clock-noncontinuous packs down into the freed slot, is not exercised
here: both flagged entries carry one frequency.

The other two sensors on this machine are unaffected: OV13858 rear
captures normally, and the VD55G0 IR camera still does face
authentication.

The "stream stop time out" and "stream close time out" at teardown appear
with and without the series, as you say.

Built clean, no new warnings.

One practical note for anyone testing this by swapping modules rather
than booting a whole kernel. With CONFIG_MODVERSIONS the series moves
symbol CRCs, because struct ipu_sensor changes size:

   ipu_bridge_parse_ssdb  0x8730390b -> 0x222edd7f
   ipu_bridge_init        0xbb0996a9 -> 0x573da924

So intel-ipu6 and intel-ipu6-isys have to come from the same build as
ipu-bridge. I compared Module.symvers before swapping and rebuilt all
three together, so I did not hit it, but installing ipu-bridge alone
would have left the other two refusing to load.

On 9/2/26 17:23, Fernando Rimoli wrote:
> The ov5693 never programs MIPI_CTRL00 (0x4800), leaving it at its 0x00
> power-on default, which lets the MIPI clock run freely. The IPU3 CSI-2
> receiver tolerates this, but the IPU6 receiver (e.g. on Microsoft
> Surface Pro 7+, Pro 8, Pro 9 and Surface Go 4) fails to lock onto the
> link, so the sensor streams but capture times out with "stream stop
> time out". On most affected machines no frames arrive at all; on some
> the failure is intermittent.
> 
> Gate the clock lane while idle at stream on when the endpoint requests a
> non-continuous clock.
> 
> Only the gate bit is touched, so platforms that do not request it are
> unaffected. No counterpart is needed at stream off, as the link is down
> by then and the register returns to its default when the sensor is
> powered off.
> 
> The property is supplied by the ipu-bridge in a subsequent patch.
> 
> Link: https://github.com/linux-surface/linux-surface/pull/2171
> Co-developed-by: Arsalan Naeem <naeemarsalan@gmail.com>
> Signed-off-by: Arsalan Naeem <naeemarsalan@gmail.com>
> Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.com>
> Tested-by: Jakob Berg Jespersen <dev@berg.pm> # Surface Pro 7+, IPU6 Tiger Lake
> Tested-by: Fil Dunsky <filipp.dunsky@gmail.com> # Surface Pro 8, IPU6 Tiger Lake (8086:9a19)
> Tested-by: Lucas Lis <lucaseze.lis@gmail.com> # Surface Pro 7+, IPU6 Tiger Lake (0x9a19)
> Tested-by: Kengo Oki <dev.kengo.fugu0141@gmail.com> # Surface Go 4, IPU6 Alder Lake-N 8086:462e
> ---
>   drivers/media/i2c/ov5693.c | 24 ++++++++++++++++++++++++
>   1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
> index 02236f3db..cedc6ea03 100644
> --- a/drivers/media/i2c/ov5693.c
> +++ b/drivers/media/i2c/ov5693.c
> @@ -35,6 +35,11 @@
>   #define OV5693_STOP_STREAMING			0x00
>   #define OV5693_SW_RESET				0x01
>   
> +/* MIPI transmitter control */
> +#define OV5693_MIPI_CTRL00_REG			CCI_REG8(0x4800)
> +/* Gate the clock lane when there is no packet to transmit */
> +#define OV5693_MIPI_CTRL00_CLOCK_LANE_GATE	BIT(5)
> +
>   #define OV5693_REG_CHIP_ID			CCI_REG16(0x300a)
>   /* Yes, this is right. The datasheet for the OV5693 gives its ID as 0x5690 */
>   #define OV5693_CHIP_ID				0x5690
> @@ -144,6 +149,9 @@ struct ov5693_device {
>   	struct regulator_bulk_data supplies[OV5693_NUM_SUPPLIES];
>   	struct clk *xvclk;
>   
> +	/* Gate the MIPI clock lane when idle (CSI-2 non-continuous clock) */
> +	bool clock_ncont;
> +
>   	struct ov5693_mode {
>   		struct v4l2_rect crop;
>   		struct v4l2_mbus_framefmt format;
> @@ -611,6 +619,19 @@ static int ov5693_enable_streaming(struct ov5693_device *ov5693, bool enable)
>   {
>   	int ret = 0;
>   
> +	/*
> +	 * Gate the MIPI clock lane while idle if the CSI-2 link is configured
> +	 * for a non-continuous clock. Only that bit is touched, and only in
> +	 * that case, so the register keeps whatever the platform left in it
> +	 * and the clock stays free-running as before everywhere else. It
> +	 * needs no counterpart at stream off: the link is down by then, and
> +	 * the register returns to its default when the sensor is powered off.
> +	 */
> +	if (enable && ov5693->clock_ncont)
> +		cci_update_bits(ov5693->regmap, OV5693_MIPI_CTRL00_REG,
> +				OV5693_MIPI_CTRL00_CLOCK_LANE_GATE,
> +				OV5693_MIPI_CTRL00_CLOCK_LANE_GATE, &ret);
> +
>   	cci_write(ov5693->regmap, OV5693_SW_STREAM_REG,
>   		  enable ? OV5693_START_STREAMING : OV5693_STOP_STREAMING,
>   		  &ret);
> @@ -1259,6 +1280,9 @@ static int ov5693_check_hwcfg(struct ov5693_device *ov5693)
>   		goto out_free_bus_cfg;
>   	}
>   
> +	ov5693->clock_ncont = bus_cfg.bus.mipi_csi2.flags &
> +			      V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK;
> +
>   out_free_bus_cfg:
>   	v4l2_fwnode_endpoint_free(&bus_cfg);
>   

[PATCH v5 5/7] media: ipu-bridge: Assign endpoint property indices dynamically

[Fernando Rimoli]

Index the ep_properties array dynamically instead of plain numerical
values as is done in mipi-disco-img.c.

Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.com>
Tested-by: Jakob Berg Jespersen <dev@berg.pm> # Surface Pro 7+, IPU6 Tiger Lake
Tested-by: Fil Dunsky <filipp.dunsky@gmail.com> # Surface Pro 8, IPU6 Tiger Lake (8086:9a19)
Tested-by: Lucas Lis <lucaseze.lis@gmail.com> # Surface Pro 7+, IPU6 Tiger Lake (0x9a19)
Tested-by: Kengo Oki <dev.kengo.fugu0141@gmail.com> # Surface Go 4, IPU6 Alder Lake-N 8086:462e
---
 drivers/media/pci/intel/ipu-bridge.c | 27 ++++++++++++++-------------
 include/media/ipu-bridge.h           | 19 ++++++++++++++++++-
 2 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index 131c70844..eb7d1611b 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -451,6 +451,7 @@ static void ipu_bridge_create_fwnode_properties(
 {
 	struct ipu_property_names *names = &sensor->prop_names;
 	struct software_node *nodes = sensor->swnodes;
+	unsigned int i = 0;
 
 	sensor->prop_names = prop_names;
 
@@ -508,21 +509,21 @@ static void ipu_bridge_create_fwnode_properties(
 			PROPERTY_ENTRY_REF_ARRAY("lens-focus", sensor->vcm_ref);
 	}
 
-	sensor->ep_properties[0] = PROPERTY_ENTRY_U32(
-					sensor->prop_names.bus_type,
-					V4L2_FWNODE_BUS_TYPE_CSI2_DPHY);
-	sensor->ep_properties[1] = PROPERTY_ENTRY_U32_ARRAY_LEN(
-					sensor->prop_names.data_lanes,
-					bridge->data_lanes, sensor->lanes);
-	sensor->ep_properties[2] = PROPERTY_ENTRY_REF_ARRAY(
-					sensor->prop_names.remote_endpoint,
-					sensor->local_ref);
+	sensor->ep_properties[IPU_BRIDGE_NEXT_PROPERTY(i, EP_BUS_TYPE)] =
+		PROPERTY_ENTRY_U32(names->bus_type,
+				   V4L2_FWNODE_BUS_TYPE_CSI2_DPHY);
+	sensor->ep_properties[IPU_BRIDGE_NEXT_PROPERTY(i, EP_DATA_LANES)] =
+		PROPERTY_ENTRY_U32_ARRAY_LEN(names->data_lanes,
+					     bridge->data_lanes, sensor->lanes);
+	sensor->ep_properties[IPU_BRIDGE_NEXT_PROPERTY(i, EP_REMOTE_EP)] =
+		PROPERTY_ENTRY_REF_ARRAY(names->remote_endpoint,
+					 sensor->local_ref);
 
 	if (cfg->nr_link_freqs > 0)
-		sensor->ep_properties[3] = PROPERTY_ENTRY_U64_ARRAY_LEN(
-			sensor->prop_names.link_frequencies,
-			cfg->link_freqs,
-			cfg->nr_link_freqs);
+		sensor->ep_properties[IPU_BRIDGE_NEXT_PROPERTY(i, EP_LINK_FREQUENCIES)] =
+			PROPERTY_ENTRY_U64_ARRAY_LEN(names->link_frequencies,
+						     cfg->link_freqs,
+						     cfg->nr_link_freqs);
 
 	sensor->ipu_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(
 					sensor->prop_names.data_lanes,
diff --git a/include/media/ipu-bridge.h b/include/media/ipu-bridge.h
index 16fac7654..06701d151 100644
--- a/include/media/ipu-bridge.h
+++ b/include/media/ipu-bridge.h
@@ -64,6 +64,23 @@ enum ipu_sensor_swnodes {
 	SWNODE_COUNT
 };
 
+enum ipu_bridge_ep_props {
+	IPU_BRIDGE_EP_BUS_TYPE,
+	IPU_BRIDGE_EP_DATA_LANES,
+	IPU_BRIDGE_EP_REMOTE_EP,
+	IPU_BRIDGE_EP_LINK_FREQUENCIES,
+	IPU_BRIDGE_EP_NUM_OF,
+	IPU_BRIDGE_EP_NUM_ENTRIES
+};
+
+/*
+ * Get the index of the next property in a property array, with a given maximum
+ * value.
+ */
+#define IPU_BRIDGE_NEXT_PROPERTY(index, max)		\
+	(WARN_ON((index) > IPU_BRIDGE_##max) ?		\
+	 IPU_BRIDGE_##max : (index)++)
+
 /* Data representation as it is in ACPI SSDB buffer */
 struct ipu_sensor_ssdb {
 	u8 version;
@@ -141,7 +158,7 @@ struct ipu_sensor {
 	const char *vcm_type;
 
 	struct ipu_property_names prop_names;
-	struct property_entry ep_properties[5];
+	struct property_entry ep_properties[IPU_BRIDGE_EP_NUM_ENTRIES];
 	struct property_entry dev_properties[5];
 	struct property_entry ipu_properties[3];
 	struct property_entry ivsc_properties[1];
-- 
2.43.0

[PATCH v5 6/7] media: ipu-bridge: Match sensor configs per IPU and add config flags

[Fernando Rimoli]

Some sensors need different treatment depending on which IPU they are
connected to, so the ACPI HID alone is not enough.

Match on an optional list of IPU PCI product IDs. Entries for one HID
must be adjacent, IPU-specific ones first, so the generic entry is
skipped once a specific one has matched.

Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.com>
Tested-by: Jakob Berg Jespersen <dev@berg.pm> # Surface Pro 7+, IPU6 Tiger Lake
Tested-by: Fil Dunsky <filipp.dunsky@gmail.com> # Surface Pro 8, IPU6 Tiger Lake (8086:9a19)
Tested-by: Lucas Lis <lucaseze.lis@gmail.com> # Surface Pro 7+, IPU6 Tiger Lake (0x9a19)
Tested-by: Kengo Oki <dev.kengo.fugu0141@gmail.com> # Surface Go 4, IPU6 Alder Lake-N 8086:462e
---
 drivers/media/pci/intel/ipu-bridge.c | 35 ++++++++++++++++++++++++++++
 include/media/ipu-bridge.h           | 26 +++++++++++++++++----
 2 files changed, 56 insertions(+), 5 deletions(-)

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index eb7d1611b..5efdcb9c8 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -8,6 +8,7 @@
 #include <linux/dmi.h>
 #include <linux/i2c.h>
 #include <linux/mei_cl_bus.h>
+#include <linux/pci.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/property.h>
@@ -853,8 +854,28 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
 	return ret;
 }
 
+/*
+ * Whether a sensor config applies to the IPU this bridge sits on. A config
+ * listing PCI product IDs only applies to those IPUs.
+ */
+static bool ipu_bridge_config_matches(const struct ipu_sensor_config *cfg,
+				      struct ipu_bridge *bridge)
+{
+	const u16 *id;
+
+	if (!cfg->pci_ids)
+		return true;
+
+	for (id = cfg->pci_ids; *id; id++)
+		if (*id == bridge->pci_id)
+			return true;
+
+	return false;
+}
+
 static int ipu_bridge_connect_sensors(struct ipu_bridge *bridge)
 {
+	const char *done_hid = NULL;
 	unsigned int i;
 	int ret;
 
@@ -862,9 +883,22 @@ static int ipu_bridge_connect_sensors(struct ipu_bridge *bridge)
 		const struct ipu_sensor_config *cfg =
 			&ipu_supported_sensors[i];
 
+		/*
+		 * Entries for one HID are adjacent, IPU-specific ones first,
+		 * so the generic entry is skipped once a specific one has
+		 * matched and the sensor is not connected twice.
+		 */
+		if (done_hid && !strcmp(cfg->hid, done_hid))
+			continue;
+
+		if (!ipu_bridge_config_matches(cfg, bridge))
+			continue;
+
 		ret = ipu_bridge_connect_sensor(cfg, bridge);
 		if (ret)
 			goto err_unregister_sensors;
+
+		done_hid = cfg->hid;
 	}
 
 	return 0;
@@ -948,6 +982,7 @@ int ipu_bridge_init(struct device *dev,
 		sizeof(bridge->ipu_node_name));
 	bridge->ipu_hid_node.name = bridge->ipu_node_name;
 	bridge->dev = dev;
+	bridge->pci_id = dev_is_pci(dev) ? to_pci_dev(dev)->device : 0;
 	bridge->parse_sensor_fwnode = parse_sensor_fwnode;
 
 	ret = software_node_register(&bridge->ipu_hid_node);
diff --git a/include/media/ipu-bridge.h b/include/media/ipu-bridge.h
index 06701d151..aca76ff71 100644
--- a/include/media/ipu-bridge.h
+++ b/include/media/ipu-bridge.h
@@ -17,13 +17,24 @@
 #define IPU_SENSOR_ROTATION_NORMAL		0
 #define IPU_SENSOR_ROTATION_INVERTED		1
 
-#define IPU_SENSOR_CONFIG(_HID, _NR, ...)	\
-	(const struct ipu_sensor_config) {	\
-		.hid = _HID,			\
-		.nr_link_freqs = _NR,		\
-		.link_freqs = { __VA_ARGS__ }	\
+/*
+ * Sensor config specific to one or more IPUs, identified by their PCI product
+ * IDs, with flags describing what the sensor needs there. Entries for one HID
+ * must be adjacent in ipu_supported_sensors[], with the IPU-specific ones
+ * before the generic one.
+ */
+#define IPU_SENSOR_CONFIG_MATCH_FL(_HID, _IDS, _FLAGS, _NR, ...)	\
+	(const struct ipu_sensor_config) {				\
+		.hid = _HID,						\
+		.pci_ids = _IDS,					\
+		.flags = _FLAGS,					\
+		.nr_link_freqs = _NR,					\
+		.link_freqs = { __VA_ARGS__ }				\
 	}
 
+#define IPU_SENSOR_CONFIG(_HID, _NR, ...)				\
+	IPU_SENSOR_CONFIG_MATCH_FL(_HID, NULL, 0, _NR, __VA_ARGS__)
+
 #define NODE_SENSOR(_HID, _PROPS)		\
 	(const struct software_node) {		\
 		.name = _HID,			\
@@ -132,6 +143,9 @@ struct ipu_node_names {
 
 struct ipu_sensor_config {
 	const char *hid;
+	/* Zero-terminated list of IPU PCI product IDs, NULL for any IPU */
+	const u16 *pci_ids;
+	const u32 flags;
 	const u8 nr_link_freqs;
 	const u64 link_freqs[MAX_NUM_LINK_FREQS];
 };
@@ -177,6 +191,8 @@ typedef int (*ipu_parse_sensor_fwnode_t)(struct acpi_device *adev,
 
 struct ipu_bridge {
 	struct device *dev;
+	/* PCI product ID of the IPU, 0 if it is not a PCI device */
+	u16 pci_id;
 	ipu_parse_sensor_fwnode_t parse_sensor_fwnode;
 	char ipu_node_name[ACPI_ID_LEN];
 	struct software_node ipu_hid_node;
-- 
2.43.0

[PATCH v5 7/7] media: ipu-bridge: Request non-continuous clock for ov5693 on IPU6

[Fernando Rimoli]

The ov5693 (INT33BE / OVTI5693) needs its MIPI clock lane gated while
idle for the IPU6 CSI-2 receiver to lock onto the link, which the sensor
driver does when the endpoint requests a non-continuous clock.

The same sensor works with a free-running clock on the IPU3, so request
it only for the IPU6 variants where it has been confirmed on hardware:
Tiger Lake, Alder Lake-P and Alder Lake-N.

Signed-off-by: Fernando Rimoli <fernandorimoli11@gmail.com>
Tested-by: Jakob Berg Jespersen <dev@berg.pm> # Surface Pro 7+, IPU6 Tiger Lake
Tested-by: Fil Dunsky <filipp.dunsky@gmail.com> # Surface Pro 8, IPU6 Tiger Lake (8086:9a19)
Tested-by: Lucas Lis <lucaseze.lis@gmail.com> # Surface Pro 7+, IPU6 Tiger Lake (0x9a19)
Tested-by: Kengo Oki <dev.kengo.fugu0141@gmail.com> # Surface Go 4, IPU6 Alder Lake-N 8086:462e
---
 drivers/media/pci/intel/ipu-bridge.c | 19 +++++++++++++++++++
 include/media/ipu-bridge.h           |  5 +++++
 2 files changed, 24 insertions(+)

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index 5efdcb9c8..12f1cccd8 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -16,6 +16,7 @@
 #include <linux/workqueue.h>
 
 #include <media/ipu-bridge.h>
+#include <media/ipu6-pci-table.h>
 #include <media/v4l2-fwnode.h>
 
 #define ADEV_DEV(adev) ACPI_PTR(&((adev)->dev))
@@ -49,6 +50,14 @@
  *
  * Please keep the list sorted by ACPI HID.
  */
+/* IPU6 variants whose CSI-2 receiver needs the ov5693 clock lane gated */
+static const u16 ipu6_ov5693_ncont_clk[] = {
+	PCI_DEVICE_ID_INTEL_IPU6,		/* Tiger Lake */
+	PCI_DEVICE_ID_INTEL_IPU6EP_ADLP,	/* Alder Lake-P */
+	PCI_DEVICE_ID_INTEL_IPU6EP_ADLN,	/* Alder Lake-N */
+	0
+};
+
 static const struct ipu_sensor_config ipu_supported_sensors[] = {
 	/* Himax HM1092 */
 	IPU_SENSOR_CONFIG("HIMX1092", 2, 180000000, 180480000),
@@ -61,6 +70,9 @@ static const struct ipu_sensor_config ipu_supported_sensors[] = {
 	/* GalaxyCore GC0310 */
 	IPU_SENSOR_CONFIG("INT0310", 1, 55692000),
 	/* Omnivision OV5693 */
+	IPU_SENSOR_CONFIG_MATCH_FL("INT33BE", ipu6_ov5693_ncont_clk,
+				   IPU_BR_FL_CSI2_CLK_NONCONTINUOUS,
+				   1, 419200000),
 	IPU_SENSOR_CONFIG("INT33BE", 1, 419200000),
 	/* Onsemi MT9M114 */
 	IPU_SENSOR_CONFIG("INT33F0", 1, 384000000),
@@ -97,6 +109,9 @@ static const struct ipu_sensor_config ipu_supported_sensors[] = {
 	/* Omnivision OV5675 */
 	IPU_SENSOR_CONFIG("OVTI5675", 1, 450000000),
 	/* Omnivision OV5693 */
+	IPU_SENSOR_CONFIG_MATCH_FL("OVTI5693", ipu6_ov5693_ncont_clk,
+				   IPU_BR_FL_CSI2_CLK_NONCONTINUOUS,
+				   1, 419200000),
 	IPU_SENSOR_CONFIG("OVTI5693", 1, 419200000),
 	/* Omnivision OV8856 */
 	IPU_SENSOR_CONFIG("OVTI8856", 3, 180000000, 360000000, 720000000),
@@ -526,6 +541,10 @@ static void ipu_bridge_create_fwnode_properties(
 						     cfg->link_freqs,
 						     cfg->nr_link_freqs);
 
+	if (cfg->flags & IPU_BR_FL_CSI2_CLK_NONCONTINUOUS)
+		sensor->ep_properties[IPU_BRIDGE_NEXT_PROPERTY(i, EP_CLOCK_NONCONTINUOUS)] =
+			PROPERTY_ENTRY_BOOL("clock-noncontinuous");
+
 	sensor->ipu_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(
 					sensor->prop_names.data_lanes,
 					bridge->data_lanes, sensor->lanes);
diff --git a/include/media/ipu-bridge.h b/include/media/ipu-bridge.h
index aca76ff71..eec8c4688 100644
--- a/include/media/ipu-bridge.h
+++ b/include/media/ipu-bridge.h
@@ -17,6 +17,10 @@
 #define IPU_SENSOR_ROTATION_NORMAL		0
 #define IPU_SENSOR_ROTATION_INVERTED		1
 
+/* Flags for struct ipu_sensor_config */
+/* The sensor's CSI-2 transmitter needs a non-continuous clock */
+#define IPU_BR_FL_CSI2_CLK_NONCONTINUOUS	BIT(0)
+
 /*
  * Sensor config specific to one or more IPUs, identified by their PCI product
  * IDs, with flags describing what the sensor needs there. Entries for one HID
@@ -80,6 +84,7 @@ enum ipu_bridge_ep_props {
 	IPU_BRIDGE_EP_DATA_LANES,
 	IPU_BRIDGE_EP_REMOTE_EP,
 	IPU_BRIDGE_EP_LINK_FREQUENCIES,
+	IPU_BRIDGE_EP_CLOCK_NONCONTINUOUS,
 	IPU_BRIDGE_EP_NUM_OF,
 	IPU_BRIDGE_EP_NUM_ENTRIES
 };
-- 
2.43.0

Re: [PATCH v4 3/6] media: i2c: ov5693: Gate the MIPI clock lane for non-continuous clock

[Lucas Lis]

Tested-by: Lucas Lis <lucaseze.lis@gmail.com> # Surface Pro 7+, IPU6
Tiger Lake (0x9a19)

Tested the v4 series on a Microsoft Surface Pro 7+: Intel Tiger Lake IPU6
(8086:9a19), front sensor enumerating as INT33BE, Fedora 44 running the
linux-surface kernel 6.19.8-3.surface.fc43.

Full build log, negative control and disassembly evidence also posted as
luqqas96 on GitHub:
https://github.com/linux-surface/linux-surface/pull/2171#issuecomment-5495563179

Base: vanilla 6.19.8 with the ov5693.c and ipu-bridge.c hunks of the
linux-surface 0013-cameras.patch applied, then this series on top. Patches 1
and 2 were already present in that tree, so they are not exercised here;
patch 6's OVTI5693 hunk needed manual application because that tree carries a
duplicate OVTI5693 entry, as reported by Fil Dunsky. Only the INT33BE path is
exercised on this machine.

With the series applied:
200/200 frames, 2592x1944, 28.64 fps
MIPI_CTRL00 (0x4800), read over i2c while streaming: 0x20

Negative control, same build with only this line removed:
- IPU_SENSOR_CONFIG_MATCH_FL("INT33BE", PCI_DEVICE_ID_INTEL_IPU6,
- CSI2_CLK_NONCONTINUOUS, 1, 419200000),

0/200 frames
MIPI_CTRL00 (0x4800), read over i2c: 0x00
intel_ipu6_isys: stream stop time out / stream close time out

Verified at the binary level that the sensor config array went from 31 to 30
entries, so exactly one entry was removed.

One difference from Fil Dunsky's negative control worth recording: he saw the
first capture after boot succeed with the entry removed, and only later
captures hang. On this machine the first capture after boot already returned
zero frames. I checked the journal for that boot: there was no earlier stream
attempt and no teardown before my test, so it really was the first stream of
that boot. I capture through libcamera rather than from the raw ISYS node, and
a desktop session had enumerated the sensor at login; either could explain the
difference, and I have not isolated which. It does not change the conclusion,
and for an end user on a normal desktop it means there is no working first
stream at all without the entry.

On the out-of-tree rebuild set: rebuilding ipu-bridge alone was sufficient
here, with an unmodified in-tree intel-ipu6 and intel-ipu6-isys. The
linux-surface Fedora kernel sets "# CONFIG_MODVERSIONS is not set", so there
is no symbol CRC check to fail. Of the five files including media/ipu-bridge.h,
only ipu-bridge.c dereferences members of struct ipu_sensor; ipu6.c only calls
ipu_bridge_init() and passes a function pointer. The "rebuild the pair"
guidance therefore applies to CONFIG_MODVERSIONS=y kernels.

Caveats on the frame counts: libcamera on this machine is not stock. It is
0.7.1 rebuilt locally with a patch that filters out the sensor's binned modes,
held back with versionlock. Capturing from the raw ISYS node as a cross-check
is not possible here: it returns csi2-4 Transfer FIFO overflow even with the
camera healthy and libcamera pulling 28.64 fps at the same time. The register
reads above do not go through libcamera; they are direct i2c reads taken while
the sensor is streaming.

This machine also carries two downstream workarounds that write
MIPI_CTRL00 = 0x2d, a polling systemd service and a locally patched ov5693.ko.
Both were stopped and removed for every measurement above, since 0x2d has bit 5
set and would have masked the result.