[libcamera-devel] [PATCH v2 2/4] ipa: raspberrypi: Limit the calculated vblank based on the sensor mode
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jan 27 02:34:13 CET 2021
Hi Naush and Jacopo,
On Tue, Jan 26, 2021 at 05:20:45PM +0100, Jacopo Mondi wrote:
> On Tue, Jan 26, 2021 at 04:08:41PM +0000, Naushir Patuck wrote:
> > On Tue, 26 Jan 2021 at 14:39, Jacopo Mondi wrote:
> > > On Tue, Jan 26, 2021 at 02:26:59PM +0000, Naushir Patuck wrote:
> > > > On Tue, 26 Jan 2021 at 14:21, Jacopo Mondi wrote:
> > > > > On Tue, Jan 26, 2021 at 02:12:50PM +0000, Naushir Patuck wrote:
> > > > > > On Tue, 26 Jan 2021 at 13:44, Jacopo Mondi wrote:
> > > > >
> > > > > [snip]
> > > > >
> > > > > > > > > > + */
> > > > > > > > > > + const auto itVblank = sensorCtrls_.find(V4L2_CID_VBLANK);
> > > > > > > > > > + mode_.vblank_min = itVblank->second.min().get<int32_t>();
> > > > > > > > > > + mode_.vblank_max = itVblank->second.max().get<int32_t>();
> > > > > > > > >
> > > > > > > > > The trouble here is that sensorCtrls_ will not be updated after the
> > > > > > > > > sensor configuration has changed. It comes from the
> > > > > > > > > V4L2Device::controls() call in RPiCameraData::configureIPA(), and that
> > > > > > > > > function returns a cached copy.
> > > > > > > >
> > > > > > > > I don't exactly know all of this in very much detail, so apologies if the
> > > > > > > > following statements sound basic, or don't make much sense :-)
> > > > > > > >
> > > > > > > > In RPiCameraData::configureIPA(), would it be possible to "update" the
> > > > > > > > cached values of V4L2Device::controls_ by
> > > > > > > > calling V4L2Device::listControls(), or maybe a new method such
> > > > > > > > as V4L2Device::updateControls(). This only needs to be called once
> > > > > > > > in PipelineHandlerRPi::configure(), after the sensor mode has been set.
> > > > > > > > This way, the VBLANK control will have up-to-date numbers for the IPA to
> > > > > > > > use.
I believe that should be technically feasible. Whether it is desired is
a different question :-) More on that below.
> > > > > > > > Alternatively, CameraSensorInfo gets populated in
> > > > > > > > RPiCameraData::configureIPA() with a call
> > > > > > > > to CameraSensor::sensorInfo(). Looks like this directly queries the device
> > > > > > > > for controls via V4L2Device::getControls(). Perhaps we should re-visit
> > > > > > > > Jacopo's suggestion and add the min/max vblank values to the mode?
Regardless of how we retrieve the information (directly from the V4L2
device or from the CameraSensor class), the problem stays the same.
However, I believe CameraSensor is a better match, as it will help
abstracting V4L2 from pipeline handlers. We don't have to switch right
now, but I'm pretty sure we will down the line, so if it makes sense in
the grand scheme of things to already make the transition, I'd welcome
that.
> > > > > > > > > I really dislike the fact that V4L2 provides us with a control whose
> > > > > > > > > limits change when the analog crop rectangle height changes, while the
> > > > > > > > > sensors usually have a fixed vertical total size limit. Wouldn't it be
> > > > > > > > > better to base the code on the vertical total size limit in libcamera,
> > > > > > > > > instead of vertical blanking ?
> > > > > > > >
> > > > > > > > By vertical total size limit, I assume you mean what sensors generally call
> > > > > > > > vts or frame length? If so, then yes, it does make sense. In fact, you
> > > > > > > > can see from my patch, the first thing I do is convert from vblank to frame
> > > > > > > > length for all of my calculations.
Yes, that's what I meant, the total of active lines and blanking lines.
Blanking lines is a bit of a loosely defined concept here as it includes
the embedded data lines, optical black lines, ... It's really the frame
time expressed as a number of lines (which leads to interesting
questions if we consider sensors that would natively express the frame
time in a different way).
> > > > > > > Let me summarize your understandings:
> > > > > > > - You need the VBLANK limits to clamp frame durations in the sensor
> > > > > > > limits
> > > > > > > - VBLANK min and max are combined with the current mode height and
> > > > > > > line length to get the min and max frame sizes, from where the min
> > > > > > > and max frame duration limits are calculated
> > > > > > > - min frame size = (mode_.height + frameIntegrationDiff) * mode_.line_length
> > > > > > > - max frame size = (mode_.height + max vblank) * mode_.line_length
As we assume everywhere that frames are made of an integral number of
lines, I would express the minimum and maximum frame size as a number of
lines, not a number of pixels.
> > > > > > > From the CameraSensorInfo you already know the visibile height and the
> > > > > > > line_length. Wouldn't be enough to add to that structure
> > > > > > > - maxFrameHeight = (mode_.height + max vblank)
> > > > > > > - frameIntegrationDiff
> > > > > > > This is sensor property and will be made available in the
> > > > > > > sensor database. You can keep it in the IPA as long as no
> > > > > > > sensor database is available
> > > > > > >
> > > > > > > To sum it up: is it enough for you to pass to the IPA the maximum
> > > > > > > frame length (mode_.height + max vblank) in the CameraSensorClass ?
> > > > > >
> > > > > > This is *mostly* correct. max frame size as calculated above is fine.
> > > > > > However, I do also need min frame size. Your calculation above assumes
> > > > > > min_vblank = 0. This may not be the case always, so min frame size must be
> > > > > > computed and presented in CameraSensorInfo.
> > > > >
> > > > > Correct, I had assumed min_vblank = frameIntegrationDiff, which might
> > > > > not always be the case.
> > > > >
> > > > > Considering the requirement for 'updates' and the fact the
> > > > > CameraSensorInfo is guaranteed to be fresh at IPA::Configure() time,
> > > > > I would be happy to see minFrameSize (or Height) and maxFrameSize (or
> > > > > Height) added there an computed once for all IPAs in the CameraSensor
> > > > > class.
> > > >
> > > > Great, I can put together a change for this and submit for review as part
> > > > of this series. One question, what name would you and Laurent prefer for
> > > > this field - maxFrameSize or maxFrameLength (and equivalent for min)?
> > > > Generally, I think sensors prefer using frame length in their terminology,
> > > > but v4l2 probably prefers frame size. I am ok with either.
I'd use *Length or *Height here, as we usually use *Size for
two-dimensional sizes.
> > > This and also if we want to report only the frame heights limits
> > > and let the IPA compute the frame size, or report the actual frame
> > > size. It's a detail, I know, you can easily get from one to the other
> > > and viceversa..
> >
> > So just to confirm before I make a submission, is something like this what
> > you had in mind?
> >
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index 251691aedfa3..6037f5f72897 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > /**
> > * \class CameraSensor
> > * \brief A camera sensor based on V4L2 subdevices
> > @@ -698,11 +728,12 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const
> >
> > /*
> > * Retrieve the pixel rate and the line length through V4L2 controls.
> > - * Support for the V4L2_CID_PIXEL_RATE and V4L2_CID_HBLANK controls is
> > - * mandatory.
> > + * Support for the V4L2_CID_PIXEL_RATE, V4L2_CID_HBLANK and
> > + * V4L2_CID_VBLANK controls is mandatory.
> > */
> > ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE,
> > - V4L2_CID_HBLANK });
> > + V4L2_CID_HBLANK,
> > + V4L2_CID_VBLANK });
>
> Please be aware of
> https://patchwork.libcamera.org/patch/10937/
>
> You can rebase on that patch or do something similar and the first
> that lands it wins :)
>
> > if (ctrls.empty()) {
> > LOG(CameraSensor, Error)
> > << "Failed to retrieve camera info controls";
> > @@ -713,6 +744,10 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const
> > info->lineLength = info->outputSize.width + hblank;
> > info->pixelRate = ctrls.get(V4L2_CID_PIXEL_RATE).get<int64_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>();
> > +
>
> That's what I had in mind, yes!
>
> Works for me, let's see what Laurent thinks
This looks good to me.
> > return 0;
> > }
> >
> > Let me know if you think that is suitable or not.
> >
> > > > > > > > > We would still have the issue of
> > > > > > > > > retrieving this value, and I can see multiple options:
> > > > > > > > >
> > > > > > > > > - Compute it once at initialization from vblank limits + analog crop
> > > > > > > >
> > > > > > > > This is assuming that vblank + crop height is always going to be equal to
> > > > > > > > frame length? We need to think if that is strictly true for all sensors,
> > > > > > > > I'm not sure.
It depends on how we define vblank. Its current purpose in libcamera is
to control the frame rate, so from that point of view isn't it
effectively always the frame length minus the active height ?
> > > > > > > > > - Extend V4L2 with a new vertical total size control and use it (that
> > > > > > > > > will be more work as we need to synchronize with the kernel changes)
> > > > > > > >
> > > > > > > > I expect this would have the longest lead time :-)
Yes, I'm not looking forward to that :-S
> > > > > > > > > - Hardcode the value in a sensor database in libcamera, bypassing the
> > > > > > > > > kernel completly
> > > > > > > >
> > > > > > > > This is possible, but...
> > > > > > > >
> > > > > > > > > One question that I'm not sure I can answer by myself is whether we can
> > > > > > > > > rely on the vertical total size limits being constant.
> > > > > > > >
> > > > > > > > ... I don't think you can assume total vertical size (frame length) is
> > > > > > > > going to be the same for all modes a sensor advertises.
That's my worry as well. Do you have any example of a sensor that
wouldn't guarantee the maximum frame length being a constant ?
> > > > > > > > In this case, the
> > > > > > > > sensor database would have to know about every mode available in the kernel
> > > > > > > > driver. This is something we moved away from with the Raspberry Pi
> > > > > > > > CamHelper, so I doubt you want to re-introduce that link.
Fully agreed, I don't want that either. If we decide to solve this
through a camera sensor database, we would need to encode how the sensor
handles the frame length constraints, not their numerical values for
specific modes. For sensors that have a fixed maximum frame length it
would just be a number, for other sensors it may be a few numerical
constants for a fixed formula, or even sensor-specific code that would
take as input the sensor mode.
> > > > > > > > My feeling is that we could work something out by forcing a "refresh" of
> > > > > > > > the cached controls on every configure, and I think that should cover our
> > > > > > > > (or at least Raspberry Pi's in this particular instance) usage.
> > > > > > > >
> > > > > > > > Let me know your thoughts.
Let's start with a refresh, through the CameraSensor class, as discussed
above. This will move the problem away from pipeline handlers and IPA
modules, and will allow us to switch to a different method to retrieve
(or compute) the information later if desired.
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list