[libcamera-devel] [PATCH v1 1/9] camera_sensor: Add minimum and maximum line length to IPACameraSensorInfo
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Oct 5 14:39:28 CEST 2022
Hi Naush,
On Wed, Oct 05, 2022 at 01:01:41PM +0100, Naushir Patuck wrote:
> On Tue, 4 Oct 2022 at 17:40, Laurent Pinchart wrote:
> > On Mon, Oct 03, 2022 at 09:39:27AM +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.
> > >
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > ---
> > > include/libcamera/ipa/core.mojom | 21 +++++++++++++++------
> > > src/ipa/ipu3/ipu3.cpp | 2 +-
> > > src/ipa/raspberrypi/raspberrypi.cpp | 2 +-
> > > src/ipa/rkisp1/rkisp1.cpp | 2 +-
> > > src/libcamera/camera_sensor.cpp | 6 ++++--
> >
> > This causes a compilation failure in src/ipa/ipu3/ipu3.cpp due to a
> > change that has been merged recently. It's easy to fix with
> >
> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > index d66965e7d94e..c89f76c56ee3 100644
> > --- a/src/ipa/ipu3/ipu3.cpp
> > +++ b/src/ipa/ipu3/ipu3.cpp
> > @@ -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
> >
> > If that's the only change needed in this series, I'll fix it myself,
> > otherwise you can include this in v2.
>
> I'll fix this up in the next revision.
>
> > > 5 files changed, 22 insertions(+), 11 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 d1ea081d595d..da029571ba55 100644
> > > --- a/src/ipa/ipu3/ipu3.cpp
> > > +++ b/src/ipa/ipu3/ipu3.cpp
> > > @@ -341,7 +341,7 @@ 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;
> >
> > This is getting long, let's wrap it.
> >
> > context_.configuration.sensor.lineDuration = sensorInfo.minLineLength
> > * 1.0s / sensorInfo.pixelRate;
> >
> > (I can also apply this change myself if no v2 is otherwise needed.)
> >
> > >
> > > /* Load the tuning data file. */
> > > File file(settings.configurationFile);
> > > 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..c2c8d3d44e26 100644
> > > --- a/src/libcamera/camera_sensor.cpp
> > > +++ b/src/libcamera/camera_sensor.cpp
> > > @@ -883,10 +883,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>();
> >
> > Hmmm... If the current value of V4L2_CID_HBLANK is different than the
> > minimum, won't all the IPA modules compute an incorrect line length ?
> > I'm sure this is fixed for the Raspberry Pi IPA module in subsequent
> > patches, but the IPU3 and RkISP1 will likely be affected.
>
> This is true - I am making the (perhaps incorrect) assumption that sensors that
> do not dynamically control hblank will have the control value set to the minimum.
> I made the change here so that the min/max LineDuration fields in IPACameraSensorInfo
> matches with what min/max FrameLength conveys.
>
> I can make one of two changes to fix this:
>
> 1) Revert the above code so that IPACameraSensorInfo continues to report
> lineLength as before. I will then have to update our RPi mode setup so that
> we essentially do the above in our own mode structure.
>
> 2) Keep the above change, and update rkisp1/ipu3/rpi IPAs to calculate line length
> directly from the control value instead of from the fields in IPACameraSensorInfo.
>
> For consistency, I would prefer (2), but happy to do either.
3) Set V4L2_CID_HBLANK to the minimum when initializing the sensor.
Or could this introduce breakages ?
> > > +
> > > 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