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

[Laurent Pinchart]

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.

> >>>>> 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