Re: [PATCH RFC 2/2] media: i2c: ov5693: fix horizontal flip polarity and Bayer phase

Hi,

On 20-Jul-26 16:28, Jakob Berg Jespersen wrote:
> Like the ov8865 (see previous patch), the ov5693's native readout is
> horizontally mirrored and the FORMAT2 FLIP_HORZ bits (reg 0x3821)
> un-mirror it: with the bits cleared the image is mirrored, with them set
> it is not. The vendor init table sets the bits by default (0x3821 =
> 0x1e). The driver maps V4L2_CID_HFLIP=1 to setting the bits, so
> requesting a flip produces an unflipped image and vice versa.
> 
> This is user-visible on the Surface Pro 7+ front camera (mounted with a
> 180 degree rotation, SSDB degree=180): libcamera requests HFLIP=1+VFLIP=1
> to undo the mount rotation and gets a horizontally flipped image.
> 
> Verified the same way as the ov8865, with a live 4-state flip/image
> matrix on the streaming sensor giving the same result: an inverted HFLIP
> with a correct VFLIP.
> 
> Invert the polarity so HFLIP=0 yields the unflipped image (bits set) and
> HFLIP=1 the flipped one (bits cleared).
> 
> Clearing the bits also shifts the Bayer sampling by one column: the
> fixed SBGGR10 mbus code the driver reports is only correct for the
> bits-set orientation, so the HFLIP=1 state has the wrong colors.
> Compensate by shifting the ISP output window (reg 0x3810) one column in
> that state only: OFFSET_START_X = 1 when HFLIP=1, 0 otherwise. There is
> a spare column to shift into because the crop end is programmed
> inclusively (crop end = crop start + width). Validated on the SP7+:
> correct geometry and colors in both flip states.
> 
> Signed-off-by: Jakob Berg Jespersen <[email protected]>
> ---
>  drivers/media/i2c/ov5693.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/ov5693.c b/drivers/media/i2c/ov5693.c
> index 4cc796bbee92..95dc6af45125 100644
> --- a/drivers/media/i2c/ov5693.c
> +++ b/drivers/media/i2c/ov5693.c
> @@ -375,8 +375,28 @@ static int ov5693_flip_horz_configure(struct ov5693_device *ov5693,
>  		  OV5693_FORMAT2_FLIP_HORZ_SENSOR_EN;
>  	int ret;
>  
> +	/*
> +	 * The sensor's native readout is horizontally mirrored; the
> +	 * FLIP_HORZ bits un-mirror it. Map the control so that HFLIP=0
> +	 * yields an unmirrored image (verified on Surface Pro 7+ front
> +	 * camera by a live flip-control/image matrix test, matching the
> +	 * same behaviour found on the ov8865).
> +	 */
>  	ret = cci_update_bits(ov5693->regmap, OV5693_FORMAT2_REG, bits,
> -			      enable ? bits : 0, NULL);
> +			      enable ? 0 : bits, NULL);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Clearing the FLIP_HORZ bits shifts the Bayer sampling by one
> +	 * column relative to the orientation the init table (0x3821 = 0x1e)
> +	 * and the reported SBGGR10 mbus code assume, which corrupts colors.
> +	 * Offset the ISP output window by one column in that state to keep
> +	 * the BGGR phase; the crop window always has one spare column
> +	 * (crop end = start + width, inclusive).
> +	 */
> +	ret = cci_write(ov5693->regmap, OV5693_OFFSET_START_X_REG,
> +			enable ? 1 : 0, NULL);

Both the commit message and the comments here could be a bit less
verbose.

Otherwise this looks good to me:

Reviewed-by: Hans de Goede <[email protected]>

Regards,

Hans