[libcamera-devel] [PATCH v4 2/7] libcamera: pipeline: raspberrypi: Set sensor orientation during validate

[Dave Stevenson]

On Fri, 28 Aug 2020 at 17:03, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Dave,
>
> On Fri, Aug 28, 2020 at 04:55:14PM +0100, Dave Stevenson wrote:
> > On Fri, 28 Aug 2020 at 16:36, Laurent Pinchart wrote:
> > > On Fri, Aug 28, 2020 at 04:26:05PM +0100, Kieran Bingham wrote:
> > >> On 28/08/2020 16:14, David Plowman wrote:
> > >>> On Fri, 28 Aug 2020 at 16:02, Kieran Bingham wrote:
> > >>>> On 28/08/2020 15:41, David Plowman wrote:
> > >>>>> We set the sensor orientation (h and v flips) during validate as this
> > >>>>> will in general affect the Bayer order output by the sensor. Doing it
> > >>>>> here means that the correct raw format gets advertised in any raw
> > >>>>> streams that the application requested.
> > >>>>
> > >>>> Eeep - I'm not sure if we could do this in validate().
> > >>>>
> > >>>> Validation should not actually make any change to the hardware, but it
> > >>>> should check that the configuration can be applied correctly, and make
> > >>>> any changes that would be necessary to support a correct (and 'valid')
> > >>>> configuration to be applied through the ->configure()
> > >>>
> > >>> Yes, that's an interesting one. The reason for doing it here is so
> > >>> that the Bayer format comes out correctly for any raw streams that
> > >>> were requested, and we're relying on the camera driver to give us the
> > >>> true Bayer order.
> > >>>
> > >>> Of course, the camera driver doesn't *have* to change the Bayer order
> > >>> when you transform it (it might do 1-pixel shits to maintain the
> > >>
> > >> haha, I hadn't noticed the typo until your follow up post ;-)
> > >>
> > >>> original Bayer order), so the puzzle then is...  how would you know?
> > >>>
> > >>> Another solution I toyed with - and indeed implemented first - was to
> > >>> do it in the configure() method, but then I had to dig around and find
> > >>> any raw stream configurations and update the pixelFormat post facto.
> > >>> This involves changing stream formats after validate(), which seemed
> > >>> bad to me too... but do we prefer it?
> > >>
> > >> Ayeee - no, we can't change a format after validate either.
> > >> Oh dear have we deadlocked...
> > >>
> > >> So - is the issue that the rotation/transform affects the pixel format?
> > >>
> > >> Are we intending to support those transforms on a per-frame basis? or
> > >> just a per-stream basis.
> > >>
> > >> As long as it's per-stream - Can we just store the state of the expected
> > >> rotation in the RPiCameraConfiguration, so that we can use it at
> > >> configure time appropriately? Or is it that we will need to determine
> > >> what adjustment will be made to the pixelformat based on the transform.
> > >
> > > The latter. We need to report to the user which raw format will be used
> > > based on the transformation.
> > >
> > >> And if that's the case, I assume it's the kernel driver which is going
> > >> to tell us what the new format will be. I fear we might have to
> > >> duplicate the determination of the format up here, and report the
> > >> adjustment.
> > >>
> > >> I guess what we're lacking is the ability to do a full atomic 'try' of
> > >> the state of the device at the driver level ...
> > >
> > > Correct, and I doubt V4L2 will ever give us that.
> > >
> > > I think we need to adjust the format in the pipeline handler, instead of
> > > querying the device. We however need to know if h/v flip will affect the
> > > bayer pattern, or if the sensor will compensate by shifting the crop
> > > rectangle.
> > >
> > > One option is to query the sensor driver at match time, to see if the
> > > media bus format changes when the h/v flip controls are modified. A bit
> > > of a hassle, but shouldn't be too complicated. Another option would be
> > > to ignore all this and consider that a proper sensor driver will never
> > > shift the crop rectangle behind the scene. We have the option to
> > > establish a set of rules for a sensor driver to be supported by
> > > libcamera.
> >
> > You have a very good indicator of that from the
> > V4L2_CTRL_FLAG_MODIFY_LAYOUT flag when querying controls. It would be
> > fair to deduce from that flag on flips that it is going to change the
> > bayer order. I've yet to see a Bayer sensor that offers transpose, but
> > there may be some out there.
>
> Good point, that's easier than changing the controls and checking the
> media bus format.
>
> > IMX327 and IMX290 are two that don't alter the Bayer order when using
> > the on-sensor flip controls. Reading the datasheet for IMX327 (page
> > 58), it explicitly states for the full frame mode the active lines are
> > "Line No during normal operation" 21 to 1100
> > "Line No during inverted operation" 1101 to 22
> > and
> > "Horizontal pixel output image normal operation" 13 to1932
> > "Horizontal pixel output image inverted operation" 1933 to 14.
> >
> > I'm also aware of one further sensor (can't say what) that I know for
> > definite doesn't change the Bayer order. Exactly how it achieves that
> > one can only assume is by shifting the readout by one pixel.
> >
> > Should the crop reported by G_SELECTION be shifted in response to the
> > flips then?
>
> I think it should, yes.

You make writing sensor drivers such fun! ;-)
At least with that one you don't have to fight the v4l2_ctrl framework.

> > >>>>> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > >>>>> ---
> > >>>>>  .../pipeline/raspberrypi/raspberrypi.cpp       | 18 +++++++++++-------
> > >>>>>  1 file changed, 11 insertions(+), 7 deletions(-)
> > >>>>>
> > >>>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > >>>>> index 42c9caa..7aace71 100644
> > >>>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > >>>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > >>>>> @@ -400,6 +400,17 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> > >>>>>       if (config_.empty())
> > >>>>>               return Invalid;
> > >>>>>
> > >>>>> +     /*
> > >>>>> +      * Configure the H/V flip controls based on the sensor rotation. We do
> > >>>>> +      * this here so that the sensor has the correct Bayer format that will
> > >>>>> +      * get advertised in the configuration of any raw streams.
> > >>>>> +      */
> > >>>>> +     ControlList ctrls(data_->unicam_[Unicam::Image].dev()->controls());
> > >>>>> +     int32_t rotation = data_->sensor_->properties().get(properties::Rotation);
> > >>>>> +     ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation));
> > >>>>> +     ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation));
> > >>>>> +     data_->unicam_[Unicam::Image].dev()->setControls(&ctrls);
> > >>>>> +
> > >>>>>       unsigned int rawCount = 0, outCount = 0, count = 0, maxIndex = 0;
> > >>>>>       std::pair<int, Size> outSize[2];
> > >>>>>       Size maxSize;
> > >>>>> @@ -1164,13 +1175,6 @@ int RPiCameraData::configureIPA()
> > >>>>>                                             { V4L2_CID_EXPOSURE, result.data[1] } });
> > >>>>>                       sensorMetadata_ = result.data[2];
> > >>>>>               }
> > >>>>> -
> > >>>>> -             /* Configure the H/V flip controls based on the sensor rotation. */
> > >>>>> -             ControlList ctrls(unicam_[Unicam::Image].dev()->controls());
> > >>>>> -             int32_t rotation = sensor_->properties().get(properties::Rotation);
> > >>>>> -             ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation));
> > >>>>> -             ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation));
> > >>>>> -             unicam_[Unicam::Image].dev()->setControls(&ctrls);
> > >>>>>       }
> > >>>>>
> > >>>>>       if (result.operation & RPI_IPA_CONFIG_SENSOR) {
> > >>>>>
>
> --
> Regards,
>
> Laurent Pinchart