Message-ID: <20250724095607.GA20849@pendragon.ideasonboard.com>
Date: Thu, 24 Jul 2025 12:56:07 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Dave Stevenson <dave.stevenson@...pberrypi.com>
Cc: Fabian Pfitzner <f.pfitzner@...gutronix.de>,
	Jacopo Mondi <jacopo.mondi@...asonboard.com>,
	Rob Herring <robh@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
	devicetree@...r.kernel.org, Jacopo Mondi <jacopo@...ndi.org>,
	linux-kernel@...r.kernel.org,
	Sakari Ailus <sakari.ailus@...ux.intel.com>,
	entwicklung@...gutronix.de,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	linux-media@...r.kernel.org, hansg@...nel.org
Subject: Re: [PATCH 0/2] parse horizontal/vertical flip properties

On Wed, Jul 23, 2025 at 03:33:48PM +0100, Dave Stevenson wrote:
> On Wed, 23 Jul 2025 at 14:48, Fabian Pfitzner wrote:
> > On 7/23/25 15:00, Dave Stevenson wrote:
> > > On Wed, 23 Jul 2025 at 13:21, Jacopo Mondi wrote:
> > >> On Wed, Jul 23, 2025 at 12:09:58PM +0200, Fabian Pfitzner wrote:
> > >>> On 7/23/25 11:44, Jacopo Mondi wrote:
> > >>>> On Wed, Jul 23, 2025 at 11:29:27AM +0200, Fabian Pfitzner wrote:
> > >>>>> On 7/23/25 11:17, Jacopo Mondi wrote:
> > >>>>>> Hi Fabian
> > >>>>>>
> > >>>>>> On Wed, Jul 23, 2025 at 10:58:28AM +0200, Fabian Pfitzner wrote:
> > >>>>>>> There are cameras containing a mirror on their optical path e. g. when
> > >>>>>>> mounted upside down.
> > >>>>>>
> > >>>>>> How is this different from 'rotation = 180' ?
> > >>>>>
> > >>>>> If you simply want to flip the output (e. g. horizontally), you cannot do
> > >>>>> this with a rotation.
> > >>>>> The camera I'm referring to is not only upside down, but also flipped
> > >>>>> horizontally.
> > >>>>
> > >>>> 180 degress rotation = HFLIP + VFLIP
> > >>>
> > >>> I do not want to do both. Only one of them.
> > >>>
> > >>>> Yes, you can't express 'mirror' in DTS, because DTS are about the
> > >>>> physical mounting rotation of the camera. Sensor drivers shall not
> > >>>> apply any flip control automatically, it's userspace that by parsing
> > >>>> the rotation property through the associated v4l2 controls should decide
> > >>>> if it has to apply flips or not to correct the images.
> > >>>>
> > >>>> What is the use case you had in mind ? Tell the driver through a DTS
> > >>>> property it has to apply flips to auto-compensate ? Because I think we
> > >>>> shouldn't and if I'm not mistaken we also document it:
> > >>>> https://www.kernel.org/doc/html/latest/userspace-api/media/drivers/camera-sensor.html#rotation-orientation-and-flipping
> > >>>
> > >>> I have a camera that does a horizontal flip in its hardware, so the output
> > >>
> > >> Sorry, I don't want to be annoying, but what does it mean "does a
> > >> horizontal flip in the hardware" ?
> > >>
> > >> In my understanding either "in hardware" means you can't control it
> > >> from software (and so there's no point in telling drivers what to do)
> > >> or you can control it from software and it's a regular HFLIP.
> > >
> > > Can you say what this sensor/module is?
> >
> > ClairPixel 8320
> >
> > > To change flips due to physical sensor orientation is a very unusual
> > > one. That would imply some weird mechanics in the sensor to add the
> > > mirror and some form of orientation sensor being built in.
> >
> > Really? Imagine a door bell where an arbitrary camera is mounted such
> > that it faces upwards (e. g. due to space limitations).
> > Then you need a mirror in order to point into the "correct" direction.
> 
> That's not a function of the sensor then. I'd interpreted what you'd
> written as the sensor itself magically changed the readout order to
> add flips based on how it was mounted.
> 
> I'll agree with Jacopo that it is up to userspace to set the required
> flips based on information provided by the driver. Userspace could
> choose to flip the displayed image when rendering instead, which may
> be necessary if the sensor driver doesn't support flip controls.
> 
> Your second patch parses these new properties into struct
> v4l2_fwnode_device_properties, but then does nothing further with
> them. I would have expected similar handling to V4L2_CID_ORIENTATION
> and V4L2_CID_ROTATION in v4l2_ctrl_new_fwnode_properties to convert
> them into V4L2 controls. Trying to change the behaviour in the driver
> would again require changes for each and every sensor driver.
> It does run the risk of conflicting with rotation though, so needs
> some careful thought and specification with regard operation order
> (rot 90 + HFLIP != HFLIP + rot 90).

I agree. If the optical path contains a mirror, that can be indicated in
DT, and should be reported to userspace through a new (and carefully
documented) control. Drivers must not flip automatically, the same way
they must not automatically rotate by 180° when the sensor is mounted
upside down.

> > Fixing the driver for an arbitrary camera driver does not seem to be a
> > good solution.
> >
> > > The closest instance I can think of would be ov5647 where the sense of
> > > the H & V flip register bits are in opposition, but that doesn't
> > > change based on how the sensor is mounted.
> > > In that case the driver just needs to account for it when programming
> > > those registers [1]. And I now note that I haven't upstreamed the
> > > patch adding flip controls - another one for the to-do list. The
> > > hardcoded register set in the mainline driver sets HFLIP (0x3821 bit
> > > 2) but not VFLIP (0x3820 bit 2) [2].
> > >
> > >    Dave
> > >
> > > [1] https://github.com/raspberrypi/linux/commit/9e5d3fd3f47e91806a5c26f96732284f39098a58
> > > [2] https://elixir.bootlin.com/linux/v6.15.7/source/drivers/media/i2c/ov5647.c#L153
> > >
> > >>> is not what I want. My example above was misleading. The rotation fixes the
> > >>> "upside down" problem, but does not fix the flip.
> > >>>
> > >>> Doing that in userspace might be a solution, but in my opinion it is a bit
> > >>> ugly to write a script that always sets the flip property from userspace
> > >>> when the device was started.
> > >>> A much cleaner way would be to simply set this property in the device tree
> > >>> such that the driver can be initially configured with the proper values.
> > >> Sorry, don't agree here. What if a sensor is mounted 90/270 degrees
> > >> rotated (typical for mobile devices in example) ? You can't compensate
> > >> it completely with flips, would you 270+HFLIP=90 ? would you leave it
> > >> unmodified ? Userspace has to know and act accordingly, doing things
> > >> in driver (will all drivers behave the same ? Will some compensate or
> > >> other won't ?) is a recipe for more complex behaviours to handle.
> > >>
> > >>> PS: I have to send this email twice. The first one contained HTML parts that
> > >>> were rejected by some receivers...
> > >>>
> > >>>> TL;DR drivers shall not flip, userspace should. Mirroring is an effect
> > >>>> of drivers applying an HFLIP, because unless I'm missing something
> > >>>> obvious, 'mirror' is not a physical mounting configuration of the camera
> > >>>> sensor.
> > >>>>
> > >>>> FIY we're talking about something similar in libcamera
> > >>>> https://lists.libcamera.org/pipermail/libcamera-devel/2025-July/051533.html
> > >>>>
> > >>>>>>> Introduce two options to change the device's flip property via device tree.
> > >>>>>>>
> > >>>>>>> As there is already support for the panel-common driver [1], add it for cameras in the same way.
> > >>>>>>>
> > >>>>>>> [1] commit 3c0ecd83eee9 ("dt-bindings: display: panel: Move flip properties to panel-common")
> > >>>>>>>
> > >>>>>>> Signed-off-by: Fabian Pfitzner <f.pfitzner@...gutronix.de>
> > >>>>>>> ---
> > >>>>>>> Fabian Pfitzner (2):
> > >>>>>>>          media: dt-bindings: add flip properties
> > >>>>>>>          media: v4l: fwnode: parse horizontal/vertical flip properties
> > >>>>>>>
> > >>>>>>>     .../devicetree/bindings/media/video-interface-devices.yaml        | 8 ++++++++
> > >>>>>>>     drivers/media/v4l2-core/v4l2-fwnode.c                             | 3 +++
> > >>>>>>>     include/media/v4l2-fwnode.h                                       | 4 ++++
> > >>>>>>>     3 files changed, 15 insertions(+)
> > >>>>>>> ---
> > >>>>>>> base-commit: 6832a9317eee280117cd695fa885b2b7a7a38daf
> > >>>>>>> change-id: 20250718-fpf-media-dt-flip-7fcad30bcfb7

-- 
Regards,

Laurent Pinchart

Powered by blists - more mailing lists