[libcamera-devel] [PATCH v2 01/10] camera_sensor: Add minimum and maximum line length to IPACameraSensorInfo
Dave Stevenson
dave.stevenson at raspberrypi.com
Mon Oct 10 15:44:41 CEST 2022
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).
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.
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