Re: [PATCH 1/3] media: i2c: ov5693: Program the mode registers on stream start

[Dan Scally]

Morning Marco

On 27/10/2026 20:29, Marco Ziliani via B4 Relay wrote:
> From: Marco Ziliani <rylos78@xxxxxxxxx>
>
> The mode registers (readout window, binning and subsampling) are only
> written from the runtime PM resume handler, and ov5693_set_fmt() merely
> stores the requested mode. If the sensor stays powered between two
> streams, the new mode never reaches the chip: the sensor keeps
> streaming the previous mode while the CSI-2 receiver expects the newly
> negotiated format.
>
> This happens whenever the format is changed while the sensor is still
> powered. On a Surface Pro 7+ (IPU6), with the sensor held powered
> between two streams, OUTPUT_SIZE_X read back over I2C showed the sensor
> stuck in the mode it was powered up in, and 0 of 16 streams that asked
> for a different mode produced a frame. With this change, 32 of 32
> streams completed and the register followed the requested mode. A
> Surface Pro 9 (IPU6) gave the same result: without the change
> OUTPUT_SIZE stayed at 2592x1944 in all 32 streams of 16 such switches,
> with it the register matched the requested size in all 32. On a
> Surface Pro 5 (IPU3), switching between 2592x1944 and a 1920x1080 crop
> the same way, OUTPUT_SIZE_X stayed at 2592 and none of the 16 streams
> in the new size produced a frame; with this change, 32 of 32 streams
> gave 10 of 10 frames at the requested size.
>
> Program the mode on every stream start, before the controls, so that
> the mode registers don't clobber them. This matches what commit
> 4e1446ac169a ("media: i2c: ov8865: Program the sensor on stream start")
> does for the ov8865.

This commit message is very long, most of this detail is not necessary:

"ov5693_mode_configure() is only called in the driver's PM .runtime_resume() hook. If the sensor 
remains powered between two streams with different modes, the settings for the second mode are never 
written to hardware.

Move the call to ov5693_mode_configure() to ov5693_s_stream() to ensure it's written every time."
> Fixes: 89aef879cb53 ("media: i2c: Add support for ov5693 sensor")
> Link: https://github.com/linux-surface/kernel/pull/181
> Signed-off-by: Marco Ziliani <rylos78@xxxxxxxxx>
> ---
>   drivers/media/i2c/ov5693.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
>
> diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
> index 56599f9ca..579aa55b7 100644
> --- a/drivers/media/i2c/ov5693.c
> +++ b/drivers/media/i2c/ov5693.c
> @@ -1000,6 +1000,19 @@ static int ov5693_s_stream(struct v4l2_subdev *sd, int enable)
>   			return ret;
>   
>   		mutex_lock(&ov5693->lock);
> +
> +		/*
> +		 * The mode registers are otherwise only written on runtime PM
> +		 * resume, so a format change on a sensor that stays powered
> +		 * would never reach the chip. Program the mode first, then the
> +		 * controls, so the mode registers don't clobber them.
> +		 */
> +		ret = ov5693_mode_configure(ov5693);
> +		if (ret) {
> +			mutex_unlock(&ov5693->lock);
> +			goto err_power_down;
> +		}

The same function call should come out of ov5693_sensor_init() then, or it'll be written twice on a 
stream on from an unpowered state.

Thanks
Dan
> +
>   		ret = __v4l2_ctrl_handler_setup(&ov5693->ctrls.handler);
>   		if (ret) {
>   			mutex_unlock(&ov5693->lock);