[libcamera-devel] [RFC PATCH 2/2] libcamera: camera_sensor: Do not clear camera flips when listing formats
Jacopo Mondi
jacopo at jmondi.org
Thu Nov 3 18:29:28 CET 2022
Hi Dave
On Thu, Nov 03, 2022 at 04:26:07PM +0000, Dave Stevenson wrote:
> Hi Jacopo
>
> On Thu, 3 Nov 2022 at 15:26, Jacopo Mondi <jacopo at jmondi.org> wrote:
> >
> > Hi again
> >
> > On Thu, Nov 03, 2022 at 11:52:23AM +0000, Dave Stevenson via libcamera-devel wrote:
> > > On Thu, 3 Nov 2022 at 11:24, David Plowman
> > > <david.plowman at raspberrypi.com> wrote:
> > > >
> > > > Hi Dave
> > > >
> > > > On Thu, 3 Nov 2022 at 11:02, Dave Stevenson
> > > > <dave.stevenson at raspberrypi.com> wrote:
> > > > >
> > > > > Hi David
> > > > >
> > > > > On Thu, 3 Nov 2022 at 10:40, David Plowman via libcamera-devel
> > > > > <libcamera-devel at lists.libcamera.org> wrote:
> > > > > >
> > > > > > Previously the code used to clear the camnera's h and v flip bits when
> > > > > > enumerating the supported formats so as to obtain any Bayer formats in
> > > > > > the sensor's native (untransformed) orientation. However this fails
> > > > > > when the camera is already in use elsewhere.
> > > > > >
> > > > > > Instead, we query the current state of the flip bits and transform the
> > > > > > formats - which we obtain in their flipped orientation - back into
> > > > > > their native orientation to be stored.
> > > > >
> > > > > I don't believe that libcamera knows for definite whether the sensor
> > > > > changes the Bayer order or not. Several of the OnSemi sensors I have
> > > > > seen do not change, presumably as they shift their crop by 1 pixel.
> > > >
> > > > Yes, that's a good point. I've vaguely assumed that I can check the
> > > > control's V4L2_CTRL_FLAG_MODIFY_LAYOUT to discover whether this is the
> > > > case or not, but as we keep on discovering, there may be no guarantees
> > > > about this. Do we know what these particular sensors do here? The Pi
> > > > PH actually already assumes this flag "works", so the changes here
> > > > possibly don't make anything worse, though they do engrain the
> > > > assumption more deeply...
> > >
> > > Yes I believe V4L2_CTRL_FLAG_MODIFY_LAYOUT should do what you want.
> > >
> > > The OnSemi sensors I'm thinking of don't have drivers in mainline, so
> > > there's no driver to check currently.
> > >
> > > Looking at mainline, only imx219 and imx258 set the flag.
> > > - ov2680, imx208, imx319, imx355, mt9m001, ov08d10 all change the
> > > order but don't set V4L2_CTRL_FLAG_MODIFY_LAYOUT
> > > - hi847 and ov13b10 move the crop to maintain the Bayer order.
> > > - imx274 and ov5648 only ever report one order, but supports flips and
> > > has no apparent cropping shift.
> > > - I have missed it in my imx290 patches to add flips that I was about to send!
> > >
> > > So, as Jacopo summarised the situation on event handlers for controls,
> > > room for improvement!
> > >
> >
> > To add more pleasure, as discussed on a review comment on Dave's
> > series
> > https://patchwork.linuxtv.org/project/linux-media/patch/20221005152809.3785786-12-dave.stevenson@raspberrypi.com/
> > Some sensors assumes to be rotated and bury their rotation
> > settings in their register tables.
> >
> > In the example of the imx258 sensor Dave has mentioned:
> >
> > - The bayer order reported through the format is GRBG
> > https://github.com/torvalds/linux/blob/master/drivers/media/i2c/imx258.c#L822
> >
> > - The sensor driver applies mirroring by default at s_stream
> > time (without specifying the direction, vertical or horizontal)
> >
> > /* Set Orientation be 180 degree */
> > ret = imx258_write_reg(imx258, REG_MIRROR_FLIP_CONTROL,
> > IMX258_REG_VALUE_08BIT, REG_CONFIG_MIRROR_FLIP);
>
> #define REG_CONFIG_MIRROR_FLIP 0x03 [1]
> so this is doing BOTH H & V flips in one hit.
>
> Datasheet for imx258 at [2].
Your google-fu is certainly better than mine
>
> [1] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/imx258.c#L78
> [2] https://web.archive.org/web/20201027131326/www.hi.app/IMX258-datasheet.pdf
>
> > - I found an interesting comment on the patch series changelog:
> > https://www.spinics.net/lists/linux-media/msg133840.html
> > Output order of test pattern is always BGGR, so it needs a flip to
> > rotate bayer pattern to required one (GRBG)
>
> It's not uncommon that the test patterns are independent of flips as
> there is no image array to read out in the reverse direction.
> There are often 4 registers to configure the values to stick into each
> of the 4 colour channels, and those are simply inserted in order.
> Registers 0x0602-0x0609 on IMX258.
Thanks, indeed my assumption that the test pattern was reflecting the
native order was incorrect
>
> TBH I've never understood the love of implementing test patterns in
> drivers. If a driver is merged into mainline then there should be a
> fair confidence that it works, therefore why worry about test
> patterns? Isn't a captured image more valuable?
>
> > So it might seem legit to presume the native Bayer pattern on the
> > pixel array is BGGR, read from the right-bottom corner:
>
> I'm not sure if you're referencing imx258 specifically here, or
> talking generically.
> imx258 is a total mess, and I need to get around to upstreaming my
> patches at [3].
>
> The native Bayer order for imx258 is RGGB (see Fig 5-4 page 84, Fig
> 5-3 page 79, and several places in section 4). Surely the datasheet
> has to count as definitive.
>
Indeed
> As in patch [4], Y_ADD_STA register is set to 0, and Y_ADD_END to
> 3118, giving 3119 lines total for 3118 lines of readout. The hardcoded
> V flip on that starts us on the "wrong" line of the Bayer pattern for
> any sane person looking at it and trying to work out what the heck is
> going on - we're still getting RGGB. H flip that and the resulting
> Bayer order is GRBG. QED.
>
As usual, everything works by chance!
With a correct Y_ADD_END we would then get
RGGB -> vflip -> GBRG -> hflip -> BGGR
It mean that fixing the driver will imply changing the reported
bayer pattern I presume..
> [3] https://github.com/raspberrypi/linux/commits/rpi-5.15.y/drivers/media/i2c/imx258.c
> [4] https://github.com/raspberrypi/linux/commit/1942d0da85bac79f3d2a1c2d8e797e97bd16b618
Ah yes it does :)
>
> >
> > RG RG RG RG RG
> > GB GB GB GB GB
> > (1, n) RG RG RG RG RG (1,0)
> > GB GB GB GB GB
> > (0, n) <-- (0,0)
> >
> > To obtain the Bayer GRBG order advertised as output format I presume the
> > REG_CONFIG_MIRROR_FLIP vertically flips the image, moving the (0,0)
> > pixel coordinate, from where reading is started, on the top-right corner:
> >
> > (0, n) <-- (0,0)
> > RG RG RG RG RG
> > (1, n) GB GB GB GB GB (1, 0)
> > RG RG RG RG RG
> > GB GB GB GB GB
>
> Due to the error on Y_ADD_END, implementing a VFLIP gave no change in
> Bayer order! Work that one out in userspace (although it is a driver
> bug that causes it).
>
> > According to your below patch and Bayer::transform():
> > - BGGR becomes GRBG via a vertical flip, right ? So this should
> > confirm the above assumption about an unconditional vertical flip
> >
> > If the driver would register flip controls, it would register them as
> > VFLIP = 1, HFLIP = 0. Here below you would get GRBG, vertically
> > transform it to BGGR and store BGGR as "native" format, but now you
> > won't be able to apply it to the sensor, as it only supports GRBG.
> >
> > What drivers should do is to register somehow their -native- Bayer
> > pattern, use a generic BAYER format in set_fmt and let userspace deal
> > with rotations and Bayer pattern re-odering as userspace is
> > able to access the mounting rotation via the CID_CAMERA_ROTATION
> > control.
>
> Doing so goes against the V4L2 principle of the V4L2_PIX_FMT_* telling
> you exactly how to interpret the pixel data.
>
> I need to check the wording, but I thought CID_CAMERA_ROTATION told
> you the orientation of the sensor within the device such that you
> could stick it into EXIF or similar metadata. It doesn't tell you how
> the Bayer order would change.
>
Rotation != flipping, you're right I was mixing the two.
> > Unfortunately no driver afaict behaves this way. They might report via
> > v4l2-ctl if any flip is applied to them, as Dave does in the series,
> > but adjusting the Bayer pattern as you do below might lead to results
> > that won't apply on the sensor. Do you agree or did I get lost ?
>
> AIUI At the point of actually wanting to stream, flips will be
> applied, and the format reported by the sensor driver will then be
> used to configure the rest of the pipeline.
> I hope that userspace never thinks it knows better than the driver
> with regard to Bayer order - as above there are enough quirks that it
> will get it wrong.
>
just for sake of discussion:
libcamera would simply get from somewhere (driver or sensor properties
database) the native (or non-rotated) format, inspect the flips and
know what format report to applications. Driver will accept a wildcard
BAYER format. But yes, good luck configuring the pipeline if any
component along the way is sensible to the bayer ordering.
> > To be honest I would drop clearing the flips, and to
> > protect against concurrent usages, validate that the current flip values
> > match the default controls value. If the two do not match, and the
> > driver advertises V4L2_CTRL_FLAG_MODIFY_LAYOUT, it means the mbus code
> > we're seeing right now is the result of another user having flipped
> > the image. In this case you could flip the format like you're doing
> > below to obtain the "unflipped" Bayer pattern, as we know it will work
> > as it is the "defaul configuration" order.
>
> A colour equivalent to ov9282 is going to blow up your assumption on
> default value. The original driver didn't support flips, but H & V
> flipped the image (much like imx258), so to avoid regressions we have
> a default of being flipped.
Not sure I get why. The color equivalent will report the bayer order
of the H&V flipped Bayer format version. According to
your series, it will also report VFLIP = HFLIP = 1 by default and
read-only. As they're RO, no V4L2_CTRL_FLAG_MODIFY_LAYOUT flag is
registered, and no transform is applied. The sensor will only speak
the rot180 bayer format version.
The patch below transforms only if V4L2_CTRL_FLAG_MODIFY_LAYOUT, which
implies the driver can speak the non-modified format too, so I guess
it works under the assumptions that:
1) Drivers report all the flips they apply implicitly and not all of
them do
2) They correctly register V4L2_CTRL_FLAG_MODIFY_LAYOUT if flip
modifies the bayer pattern order and as you noticed not all of them
do so
I guess it will be hard to compliance-check drivers for that and this
requirements will have to be enforced during review, possibly by
inspecting the register tables making sure any flip bit is there
silently set.
> Same would have been true for imx258, except that there the original
> driver read the "rotation" fwnode parameter and ensured it was 180. I
> think that is now going to be incorrectly interpreted as the sensor
> has done the rot180, and it will tell userspace rot180 as well, so
> EXIF headers etc will say that it is stored inverted.
>
Yes, the ROTATION property should not be used to decide if the driver
should flip or not, but just by application to correctly visualize the
images..
> > Final question, and sorry for the long email, but what would it be the
> > real purpose of obtaining the native bayer pattern order in your use case. I
> > presume we had that attempt to get the native format because android
> > wants to have the native order reported. I think that CameraSensor
> > should instead try to use a bayer order that work for the drive in its
> > default configuration which, if the driver claims to be rotated by
> > default, won't match the actual native order.
> >
> > We have a camera properties database where such native information
> > could eventually be stored if it's of interest!
>
> Store it in two places - increase the chance of it being wrong! ;)
>
> I am starting to write a guide for things to check with sensor
> drivers, so hopefully future drivers will do what is defined as the
> correct thing. Perhaps we'll find the time to fix older drivers - I'm
> just doing a patch set for V4L2_CTRL_FLAG_MODIFY_LAYOUT.
>
Thanks, hopefully this will increases the sensor drivers consistency in
the kernel.
> Dave
>
> > Hope I didn't get lost!
> >
> > > Dave
> > >
> > > > Presumably you can rely on the formats being returned correctly after
> > > > setting the flip bits, though having to re-query the formats from the
> > > > device like that would be irritating too. And you'd never be able to
> > > > predict the format ahead of time, only after setting it. I don't know
> > > > if that's better or worse than saying "change the driver". Opinions
> > > > welcome on that one!
> > > >
> > > > To be honest I always have difficulty with the Bayer order being part
> > > > of the format. An application might legitimately want to have a say
> > > > about the bit depth or packing, but the Bayer order?? Being able to
> > > > request an XXXX ("anything goes") Bayer order, which gets updated once
> > > > the sensor is configured, would be much more application-friendly. But
> > > > another can of worms!!
> >
> > Agreed, userspace has all the information to adjust the ordering as it
> > likes..
> >
> > > >
> > > > David
> > > >
> > > > >
> > > > > The original comment mentioned that scenario:
> > > > > "This is harmless for sensors where the flips don't affect the Bayer order"
> > > > >
> > > > > Dave
> > > > >
> > > > > > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > > > > > ---
> > > > > > src/libcamera/camera_sensor.cpp | 51 +++++++++++++++++++++++++++------
> > > > > > 1 file changed, 43 insertions(+), 8 deletions(-)
> > > > > >
> > > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > > > > > index 572a313a..6670dfb9 100644
> > > > > > --- a/src/libcamera/camera_sensor.cpp
> > > > > > +++ b/src/libcamera/camera_sensor.cpp
> > > > > > @@ -19,6 +19,8 @@
> > > > > >
> > > > > > #include <libcamera/base/utils.h>
> > > > > >
> > > > > > +#include <libcamera/transform.h>
> > > > > > +
> > > > > > #include "libcamera/internal/bayer_format.h"
> > > > > > #include "libcamera/internal/camera_lens.h"
> > > > > > #include "libcamera/internal/camera_sensor_properties.h"
> > > > > > @@ -108,18 +110,51 @@ int CameraSensor::init()
> > > > > > return ret;
> > > > > >
> > > > > > /*
> > > > > > - * Clear any flips to be sure we get the "native" Bayer order. This is
> > > > > > - * harmless for sensors where the flips don't affect the Bayer order.
> > > > > > + * We want to get the native mbus codes for the sensor, without any flips.
> > > > > > + * We can't clear any flips here, so we have to read the current values
> > > > > > + * (if the flip controls exist), decide whether they actually modify any
> > > > > > + * output Bayer pattern, and finally undo their effect on the formats.
> > > > > > + *
> > > > > > + * First, check if the flip controls exist and if so read them.
> > > > > > */
> > > > > > ControlList ctrls(subdev_->controls());
> > > > > > - if (subdev_->controls().find(V4L2_CID_HFLIP) != subdev_->controls().end())
> > > > > > - ctrls.set(V4L2_CID_HFLIP, 0);
> > > > > > - if (subdev_->controls().find(V4L2_CID_VFLIP) != subdev_->controls().end())
> > > > > > - ctrls.set(V4L2_CID_VFLIP, 0);
> > > > > > - subdev_->setControls(&ctrls);
> > > > > > + std::vector<uint32_t> flipCtrlIds;
> > > > > > + bool hasHflip = subdev_->controls().find(V4L2_CID_HFLIP) != subdev_->controls().end();
> > > > > > + bool hasVflip = subdev_->controls().find(V4L2_CID_VFLIP) != subdev_->controls().end();
> > > > > > + if (hasHflip)
> > > > > > + flipCtrlIds.push_back(V4L2_CID_HFLIP);
> > > > > > + if (hasVflip)
> > > > > > + flipCtrlIds.push_back(V4L2_CID_VFLIP);
> > > > > > + ControlList flipCtrls = subdev_->getControls(flipCtrlIds);
> > > > > > +
> > > > > > + /* Now construct a transform that would undo any flips. */
> > > > > > + Transform transform = Transform::Identity;
> > > > > > + if (hasHflip && flipCtrls.get(V4L2_CID_HFLIP).get<int>()) {
> > > > > > + const struct v4l2_query_ext_ctrl *extCtrl = subdev_->controlInfo(V4L2_CID_HFLIP);
> > > > > > + if (extCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT)
> > > > > > + transform |= Transform::HFlip;
> > > > > > + }
> > > > > > + if (hasVflip && flipCtrls.get(V4L2_CID_VFLIP).get<int>()) {
> > > > > > + const struct v4l2_query_ext_ctrl *extCtrl = subdev_->controlInfo(V4L2_CID_VFLIP);
> > > > > > + if (extCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT)
> > > > > > + transform |= Transform::VFlip;
> > > > > > + }
> > > > > > +
> > > > > > + /* Finally get the formats, and apply the transform to the mbus codes. */
> > > > > > + auto formats = subdev_->formats(pad_);
> > > > > > + for (const auto &format : formats) {
> > > > > > + unsigned int mbusCode = format.first;
> > > > > > + BayerFormat bayerFormat = BayerFormat::fromMbusCode(mbusCode);
> > > > > > + bool valid = true;
> > > > > > +
> > > > > > + if (bayerFormat.isValid())
> > > > > > + mbusCode = bayerFormat.transform(transform).toMbusCode(valid);
> > > > > > +
> > > > > > + if (valid)
> > > > > > + formats_[mbusCode] = std::move(format.second);
> > > > > > + }
> > > > > >
> > > > > > /* Enumerate, sort and cache media bus codes and sizes. */
> > > > > > - formats_ = subdev_->formats(pad_);
> > > > > > if (formats_.empty()) {
> > > > > > LOG(CameraSensor, Error) << "No image format found";
> > > > > > return -EINVAL;
> > > > > > --
> > > > > > 2.30.2
> > > > > >
More information about the libcamera-devel
mailing list