Message-ID: <a89dbe5d-a30b-455d-adaf-31eadf2b3751@pengutronix.de>
Date: Wed, 23 Jul 2025 15:48:11 +0200
From: Fabian Pfitzner <f.pfitzner@...gutronix.de>
To: Dave Stevenson <dave.stevenson@...pberrypi.com>,
 Jacopo Mondi <jacopo.mondi@...asonboard.com>
Cc: 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 7/23/25 15:00, Dave Stevenson wrote:
> Hi Jacopo and Fabian
>
> On Wed, 23 Jul 2025 at 13:21, Jacopo Mondi
> <jacopo.mondi@...asonboard.com> wrote:
>> Hi Fabian
>>
>> 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. 
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
>>>>>>>
>>>>>>> Best regards,
>>>>>>> --
>>>>>>> Fabian Pfitzner <f.pfitzner@...gutronix.de>
>>>>>>>
>>>>> --
>>>>> Pengutronix e.K.                           | Fabian Pfitzner             |
>>>>> Steuerwalder Str. 21                       | https://www.pengutronix.de/ |
>>>>> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
>>>>> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-9    |
>>>>>
>>> --
>>> Pengutronix e.K.                           | Fabian Pfitzner             |
>>> Steuerwalder Str. 21                       | https://www.pengutronix.de/ |
>>> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
>>> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-9    |
>>>
>
-- 
Pengutronix e.K.                           | Fabian Pfitzner             |
Steuerwalder Str. 21                       | https://www.pengutronix.de/ |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-9    |

Powered by blists - more mailing lists