[libcamera-devel] [PATCH v2 01/10] camera_sensor: Add minimum and maximum line length to IPACameraSensorInfo
Jacopo Mondi
jacopo at jmondi.org
Mon Oct 10 16:08:56 CEST 2022
Hi Dave
On Mon, Oct 10, 2022 at 02:44:41PM +0100, Dave Stevenson wrote:
> Hi Jacopo
>
> On Mon, 10 Oct 2022 at 12:10, Jacopo Mondi via libcamera-devel
> <libcamera-devel at lists.libcamera.org> wrote:
> >
> > Hi
> >
> > On Sat, Oct 08, 2022 at 07:12:55AM +0100, Naushir Patuck via libcamera-devel wrote:
> > > Hi Laurent,
> > >
> > >
> > > On Fri, 7 Oct 2022 at 23:28, Laurent Pinchart <
> > > laurent.pinchart at ideasonboard.com> wrote:
> > >
> > > > Hi Naush,
> > > >
> > > > Thank you for the patch.
> > > >
> > > > On Thu, Oct 06, 2022 at 02:17:35PM +0100, Naushir Patuck via
> > > > libcamera-devel wrote:
> > > > > Add fields for minimum and maximum line length (in units of pixels) to
> > > > the
> > > > > IPACameraSensorInfo structure. This replaces the existing lineLength
> > > > field.
> > > > >
> > > > > Update the ipu3, raspberrypi and rkisp1 IPAs to use
> > > > IPACameraSensorInfo::minLineLength
> > > > > instead of IPACameraSensorInfo::lineLength, as logically we will always
> > > > want to
> > > > > use the fastest sensor readout by default.
> > > > >
> > > > > Since the IPAs now use minLineLength for their calculations, set the
> > > > starting
> > > > > value of the V4L2_CID_HBLANK control to its minimum in
> > > > CameraSensor::init().
> > > > >
> > > > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > > > Tested-by: Dave Stevenson <dave.stevenson at raspberrypi.com>
> > > > > ---
> > > > > include/libcamera/ipa/core.mojom | 21 +++++++++++++++------
> > > > > src/ipa/ipu3/ipu3.cpp | 6 ++++--
> > > > > src/ipa/raspberrypi/raspberrypi.cpp | 2 +-
> > > > > src/ipa/rkisp1/rkisp1.cpp | 2 +-
> > > > > src/libcamera/camera_sensor.cpp | 15 +++++++++++++--
> > > > > 5 files changed, 34 insertions(+), 12 deletions(-)
> > > > >
> > > > > diff --git a/include/libcamera/ipa/core.mojom
> > > > b/include/libcamera/ipa/core.mojom
> > > > > index 74f3339e56f2..2bc3028c22d6 100644
> > > > > --- a/include/libcamera/ipa/core.mojom
> > > > > +++ b/include/libcamera/ipa/core.mojom
> > > > > @@ -172,10 +172,17 @@ module libcamera;
> > > > > */
> > > > >
> > > > > /**
> > > > > - * \var IPACameraSensorInfo::lineLength
> > > > > - * \brief Total line length in pixels
> > > > > + * \var IPACameraSensorInfo::minLineLength
> > > > > + * \brief The minimum line length in pixels
> > > > > *
> > > > > - * The total line length in pixel clock periods, including blanking.
> > > > > + * The minimum allowable line length in pixel clock periods, including
> > > > blanking.
> > > > > + */
> > > > > +
> > > > > +/**
> > > > > + * \var IPACameraSensorInfo::maxLineLength
> > > > > + * \brief The maximum line length in pixels
> > > > > + *
> > > > > + * The maximum allowable line length in pixel clock periods, including
> > > > blanking.
> > > > > */
> > > > >
> > > > > /**
> > > > > @@ -189,7 +196,7 @@ module libcamera;
> > > > > * To obtain the minimum frame duration:
> > > > > *
> > > > > * \verbatim
> > > > > - frameDuration(s) = minFrameLength(lines) * lineLength(pixels) /
> > > > pixelRate(pixels per second)
> > > > > + frameDuration(s) = minFrameLength(lines) * minLineLength(pixels)
> > > > / pixelRate(pixels per second)
> > > > > \endverbatim
> > > > > */
> > > > >
> > > > > @@ -204,7 +211,7 @@ module libcamera;
> > > > > * To obtain the maximum frame duration:
> > > > > *
> > > > > * \verbatim
> > > > > - frameDuration(s) = maxFrameLength(lines) * lineLength(pixels) /
> > > > pixelRate(pixels per second)
> > > > > + frameDuration(s) = maxFrameLength(lines) * maxLineLength(pixels)
> > > > / pixelRate(pixels per second)
> > > > > \endverbatim
> > > > > */
> > > > > struct IPACameraSensorInfo {
> > > > > @@ -217,7 +224,9 @@ struct IPACameraSensorInfo {
> > > > > Size outputSize;
> > > > >
> > > > > uint64 pixelRate;
> > > > > - uint32 lineLength;
> > > > > +
> > > > > + uint32 minLineLength;
> > > > > + uint32 maxLineLength;
> > > > >
> > > > > uint32 minFrameLength;
> > > > > uint32 maxFrameLength;
> > > > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > > > > index b93a09d40c39..7e26fc5639c2 100644
> > > > > --- a/src/ipa/ipu3/ipu3.cpp
> > > > > +++ b/src/ipa/ipu3/ipu3.cpp
> > > > > @@ -336,7 +336,8 @@ int IPAIPU3::init(const IPASettings &settings,
> > > > >
> > > > > /* Clean context */
> > > > > context_.configuration = {};
> > > > > - context_.configuration.sensor.lineDuration = sensorInfo.lineLength
> > > > * 1.0s / sensorInfo.pixelRate;
> > > > > + context_.configuration.sensor.lineDuration =
> > > > sensorInfo.minLineLength
> > > > > + * 1.0s /
> > > > sensorInfo.pixelRate;
> > > >
> > > > The * should be aligned below the =. Same below.
> > > >
> > > > >
> > > > > /* Load the tuning data file. */
> > > > > File file(settings.configurationFile);
> > > > > @@ -499,7 +500,8 @@ int IPAIPU3::configure(const IPAConfigInfo
> > > > &configInfo,
> > > > > context_.frameContexts.clear();
> > > > >
> > > > > /* Initialise the sensor configuration. */
> > > > > - context_.configuration.sensor.lineDuration =
> > > > sensorInfo_.lineLength * 1.0s / sensorInfo_.pixelRate;
> > > > > + context_.configuration.sensor.lineDuration =
> > > > sensorInfo_.minLineLength
> > > > > + * 1.0s /
> > > > sensorInfo_.pixelRate;
> > > > >
> > > > > /*
> > > > > * Compute the sensor V4L2 controls to be used by the algorithms
> > > > and
> > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> > > > b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > index 8d731435764e..358a119da222 100644
> > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > @@ -356,7 +356,7 @@ void IPARPi::setMode(const IPACameraSensorInfo
> > > > &sensorInfo)
> > > > > * Calculate the line length as the ratio between the line length
> > > > in
> > > > > * pixels and the pixel rate.
> > > > > */
> > > > > - mode_.lineLength = sensorInfo.lineLength * (1.0s /
> > > > sensorInfo.pixelRate);
> > > > > + mode_.lineLength = sensorInfo.minLineLength * (1.0s /
> > > > sensorInfo.pixelRate);
> > > > >
> > > > > /*
> > > > > * Set the frame length limits for the mode to ensure exposure and
> > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > > > index 32feb1682749..ddb22d98eb41 100644
> > > > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > > > @@ -246,7 +246,7 @@ int IPARkISP1::configure([[maybe_unused]] const
> > > > IPACameraSensorInfo &info,
> > > > > context_.configuration.hw.revision = hwRevision_;
> > > > >
> > > > > context_.configuration.sensor.size = info.outputSize;
> > > > > - context_.configuration.sensor.lineDuration = info.lineLength *
> > > > 1.0s / info.pixelRate;
> > > > > + context_.configuration.sensor.lineDuration = info.minLineLength *
> > > > 1.0s / info.pixelRate;
> > > > >
> > > > > /*
> > > > > * When the AGC computes the new exposure values for a frame, it
> > > > needs
> > > > > diff --git a/src/libcamera/camera_sensor.cpp
> > > > b/src/libcamera/camera_sensor.cpp
> > > > > index 911fd0beae4e..83f81d655395 100644
> > > > > --- a/src/libcamera/camera_sensor.cpp
> > > > > +++ b/src/libcamera/camera_sensor.cpp
> > > > > @@ -176,6 +176,15 @@ int CameraSensor::init()
> > > > > if (ret)
> > > > > return ret;
> > > > >
> > > > > + /* Set HBLANK to the minimum as a starting value. */
> > > >
> > > > I would capture the rationale here.
> > > >
> > > > /*
> > > > * Set HBLANK to the minimum to start with a well-defined line
> > > > length,
> > > > * allowing IPA modules that do not modify HBLANK to use the sensor
> > > > * minimum line length in their calculations.
> > > > */
> > > >
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > >
> > > > No need to resubmit if only those small changes are required.
> > > >
> > > > > + const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK);
> > > > > + ControlList ctrl(subdev_->controls());
> > > > > +
> > > > > + ctrl.set(V4L2_CID_HBLANK, hblank.min().get<int32_t>());
> > > > > + ret = subdev_->setControls(&ctrl);
> > > > > + if (ret)
> > > > > + return ret;
> > > >
> > >
> > > Actually, doing the setControls here unconditionally is probably incorrect.
> > >
> > > If the control is read-only, we will return an error. We probably want to
> > > do something like what I did for the IPA where we test for
> > > HBLANK::min() == HBLANK::max(), and if true, assume the control is
> > > read-only and don't do the setControl().
> > >
> > > Sorry I should have caught that, but my testing was done on sensors
> > > with adjustable HBLANK controls. I'll update the statement and post
> > > a revised patch first thing on Monday.
> > >
> >
> > I wonder if we shouldn't use (for hblank as well as per vblank) the
> > control's default value in the IPA calculations instead of assuming
> > (and here forcing) the minium value. Specifically, during the
> > IPA::configure() where we intialize values using the minium value
> > seems rather an arbitrary assumptions. After IPA::configure() the IPA
> > should be free to change H/VBlank as they like in the min/max ranges,
> > but for intiialization...
> >
> > Also most drivers (should?) update the blanking values to a sensible
> > defaul when a new mode is applied. Shouldn't we rely on their
> > selection ?
>
> Is it correct behaviour for a sensor driver to change a r/w HBLANK
> when changing mode? Changing limits for HBLANK is almost certainly the
> case, but AIUI generally controls shouldn't change values unless they
> are then invalid.
>
> I'll agree that not updating the value has annoyances when the client
> doesn't automatically compute and set HBLANK, but conversely updating
> it is annoying for cases where the app doesn't support it and you have
> deliberately set it to a long value (any mode change resets your
> chosen value, despite it being valid).
>
We've discussed the same in your review of my ar0521 changes...
I actually think it makes sense for a driver to reset blankings when a
new mode is applied to something sensible, possibily something that
maximizes the frame rate ?
You have a point, that users not aware of this change will suddenly
found the blankings to be modified, but at the same time when it comes
to user-facing features, no changing blankings when changing mode
could result in a relevant change in the produced FPS rate..
I do expect an "uninformed" userspace not to change blankings, stream
at a reasonable frame rate, change mode and maintain the same frame
rate.
"Informed" userspace should be capable of chaning blankings as they
like, so I don't expect this to impact it.
However, I understand this again is on the topic "userspace knows
everthing" vs "drivers do the right thing".
Anyway, I think we need to define a policy and align to that, whatever
it ends up being..
> libcamera always selecting the mode (even if the same) leading to a
> reset when I was trying to test configurable HBLANK was where I
> encountered this ambiguity.
>
I understand, but you're setting HBLANK outside of libcamera and
expect that not to be changed. It's annoying for debugging and
developing, I understand, but isn't it expected that the library takes
over and overrides pre-existing settings ?
> Dave
>
> > > Regards,
> > > Naush
> > >
> > >
> > >
> > > > > +
> > > > > return
> > > > applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff);
> > > > > }
> > > > >
> > > > > @@ -883,10 +892,12 @@ int CameraSensor::sensorInfo(IPACameraSensorInfo
> > > > *info) const
> > > > > return -EINVAL;
> > > > > }
> > > > >
> > > > > - int32_t hblank = ctrls.get(V4L2_CID_HBLANK).get<int32_t>();
> > > > > - info->lineLength = info->outputSize.width + hblank;
> > > > > info->pixelRate = ctrls.get(V4L2_CID_PIXEL_RATE).get<int64_t>();
> > > > >
> > > > > + const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK);
> > > > > + info->minLineLength = info->outputSize.width +
> > > > hblank.min().get<int32_t>();
> > > > > + info->maxLineLength = info->outputSize.width +
> > > > hblank.max().get<int32_t>();
> > > > > +
> > > > > const ControlInfo vblank = ctrls.infoMap()->at(V4L2_CID_VBLANK);
> > > > > info->minFrameLength = info->outputSize.height +
> > > > vblank.min().get<int32_t>();
> > > > > info->maxFrameLength = info->outputSize.height +
> > > > vblank.max().get<int32_t>();
> > > >
> > > > --
> > > > Regards,
> > > >
> > > > Laurent Pinchart
> > > >
More information about the libcamera-devel
mailing list