[libcamera-devel] [PATCH v2 01/10] camera_sensor: Add minimum and maximum line length to IPACameraSensorInfo
Naushir Patuck
naush at raspberrypi.com
Sat Oct 8 08:12:55 CEST 2022
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.
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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20221008/0466a33f/attachment.htm>
More information about the libcamera-devel
mailing list