Re: [PATCH RFC 1/2] media: i2c: ov8865: fix horizontal flip control polarity
Hans de Goede <[email protected]>
| Newsgroups | org.kernel.vger.linux-media,org.kernel.vger.linux-kernel |
|---|---|
| Message-ID | <[email protected]> |
Hi, On 29-Jul-26 00:30, Sakari Ailus wrote: > Hi Jacob, > > Thanks for the set. > > On Mon, Jul 20, 2026 at 04:28:40PM +0200, Jakob Berg Jespersen wrote: >> The ov8865'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 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+ rear 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 by a live 4-state flip/image matrix on the streaming sensor: >> >> hflip=0 vflip=0 -> 180 degree rotation (both flips) >> hflip=1 vflip=0 -> vertical flip only >> hflip=0 vflip=1 -> correct image >> hflip=1 vflip=1 -> horizontal flip only >> >> (vertical flip = reflection over a horizontal mirror line, as on water; >> horizontal flip = reflection over a vertical mirror line, as in a mirror) >> >> which is only consistent with an inverted HFLIP and a correct VFLIP. >> >> Invert the polarity so HFLIP=0 yields the unflipped image. > > Is this just about, looking at the patch, that the driver before this patch > effectively implements an inverted HFLIP control? I think the description > could be simplified. Ack and even more so for the second patch. The text about "like the ov8865" really does not belong in the commit message there. Regards, Hans > >> >> Signed-off-by: Jakob Berg Jespersen <[email protected]> >> --- >> drivers/media/i2c/ov8865.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c >> index c6d53c3d55ca..901ea7c16395 100644 >> --- a/drivers/media/i2c/ov8865.c >> +++ b/drivers/media/i2c/ov8865.c >> @@ -2204,8 +2204,14 @@ static int ov8865_flip_horz_configure(struct ov8865_sensor *sensor, bool enable) >> u8 bits = OV8865_FORMAT2_FLIP_HORZ_ISP_EN | >> OV8865_FORMAT2_FLIP_HORZ_SENSOR_EN; >> >> + /* >> + * 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+ rear >> + * camera by a live flip-control/image matrix test). >> + */ > > Just a comment saying this is inverted in the sensor considering HFLIP > should be enough here. > >> return ov8865_update_bits(sensor, OV8865_FORMAT2_REG, bits, >> - enable ? bits : 0); >> + enable ? 0 : bits); >> } >> >> /* Test Pattern */ >> >