[libcamera-devel] [PATCH v1 1/9] camera_sensor: Add minimum and maximum line length to IPACameraSensorInfo
Naushir Patuck
naush at raspberrypi.com
Wed Oct 5 15:16:04 CEST 2022
Hi Laurent,
On Wed, 5 Oct 2022 at 13:39, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> 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 ?
>
The only breakage I can think of is if a user was setting HBLANK outside of
libcamera (through v4l2-ctl) and expecting the IPA to just use that value
without
change. I don't know how common that is, particularly given how few sensor
drivers actually seem to have read/write HBLANK controls.
Naush
>
> > > > +
> > > > 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/20221005/99745834/attachment.htm>
More information about the libcamera-devel
mailing list