<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 5 Oct 2022 at 13:39, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush,<br>
<br>
On Wed, Oct 05, 2022 at 01:01:41PM +0100, Naushir Patuck wrote:<br>
> On Tue, 4 Oct 2022 at 17:40, Laurent Pinchart wrote:<br>
> > On Mon, Oct 03, 2022 at 09:39:27AM +0100, Naushir Patuck via<br>
> > libcamera-devel wrote:<br>
> > > Add fields for minimum and maximum line length (in units of pixels) to the<br>
> > > IPACameraSensorInfo structure. This replaces the existing lineLength field.<br>
> > ><br>
> > > Update the ipu3, raspberrypi and rkisp1 IPAs to use IPACameraSensorInfo::minLineLength<br>
> > > instead of IPACameraSensorInfo::lineLength, as logically we will always want to<br>
> > > use the fastest sensor readout by default.<br>
> > ><br>
> > > Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> > > ---<br>
> > > include/libcamera/ipa/core.mojom | 21 +++++++++++++++------<br>
> > > src/ipa/ipu3/ipu3.cpp | 2 +-<br>
> > > src/ipa/raspberrypi/raspberrypi.cpp | 2 +-<br>
> > > src/ipa/rkisp1/rkisp1.cpp | 2 +-<br>
> > > src/libcamera/camera_sensor.cpp | 6 ++++--<br>
> ><br>
> > This causes a compilation failure in src/ipa/ipu3/ipu3.cpp due to a<br>
> > change that has been merged recently. It's easy to fix with<br>
> ><br>
> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp<br>
> > index d66965e7d94e..c89f76c56ee3 100644<br>
> > --- a/src/ipa/ipu3/ipu3.cpp<br>
> > +++ b/src/ipa/ipu3/ipu3.cpp<br>
> > @@ -499,7 +500,8 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,<br>
> > context_.frameContexts.clear();<br>
> ><br>
> > /* Initialise the sensor configuration. */<br>
> > - context_.configuration.sensor.lineDuration = sensorInfo_.lineLength * 1.0s / sensorInfo_.pixelRate;<br>
> > + context_.configuration.sensor.lineDuration = sensorInfo_.minLineLength<br>
> > + * 1.0s / sensorInfo_.pixelRate;<br>
> ><br>
> > /*<br>
> > * Compute the sensor V4L2 controls to be used by the algorithms and<br>
> ><br>
> > If that's the only change needed in this series, I'll fix it myself,<br>
> > otherwise you can include this in v2.<br>
> <br>
> I'll fix this up in the next revision.<br>
> <br>
> > > 5 files changed, 22 insertions(+), 11 deletions(-)<br>
> > ><br>
> > > diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom<br>
> > > index 74f3339e56f2..2bc3028c22d6 100644<br>
> > > --- a/include/libcamera/ipa/core.mojom<br>
> > > +++ b/include/libcamera/ipa/core.mojom<br>
> > > @@ -172,10 +172,17 @@ module libcamera;<br>
> > > */<br>
> > ><br>
> > > /**<br>
> > > - * \var IPACameraSensorInfo::lineLength<br>
> > > - * \brief Total line length in pixels<br>
> > > + * \var IPACameraSensorInfo::minLineLength<br>
> > > + * \brief The minimum line length in pixels<br>
> > > *<br>
> > > - * The total line length in pixel clock periods, including blanking.<br>
> > > + * The minimum allowable line length in pixel clock periods, including blanking.<br>
> > > + */<br>
> > > +<br>
> > > +/**<br>
> > > + * \var IPACameraSensorInfo::maxLineLength<br>
> > > + * \brief The maximum line length in pixels<br>
> > > + *<br>
> > > + * The maximum allowable line length in pixel clock periods, including blanking.<br>
> > > */<br>
> > ><br>
> > > /**<br>
> > > @@ -189,7 +196,7 @@ module libcamera;<br>
> > > * To obtain the minimum frame duration:<br>
> > > *<br>
> > > * \verbatim<br>
> > > - frameDuration(s) = minFrameLength(lines) * lineLength(pixels) / pixelRate(pixels per second)<br>
> > > + frameDuration(s) = minFrameLength(lines) * minLineLength(pixels) / pixelRate(pixels per second)<br>
> > > \endverbatim<br>
> > > */<br>
> > ><br>
> > > @@ -204,7 +211,7 @@ module libcamera;<br>
> > > * To obtain the maximum frame duration:<br>
> > > *<br>
> > > * \verbatim<br>
> > > - frameDuration(s) = maxFrameLength(lines) * lineLength(pixels) / pixelRate(pixels per second)<br>
> > > + frameDuration(s) = maxFrameLength(lines) * maxLineLength(pixels) / pixelRate(pixels per second)<br>
> > > \endverbatim<br>
> > > */<br>
> > > struct IPACameraSensorInfo {<br>
> > > @@ -217,7 +224,9 @@ struct IPACameraSensorInfo {<br>
> > > Size outputSize;<br>
> > ><br>
> > > uint64 pixelRate;<br>
> > > - uint32 lineLength;<br>
> > > +<br>
> > > + uint32 minLineLength;<br>
> > > + uint32 maxLineLength;<br>
> > ><br>
> > > uint32 minFrameLength;<br>
> > > uint32 maxFrameLength;<br>
> > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp<br>
> > > index d1ea081d595d..da029571ba55 100644<br>
> > > --- a/src/ipa/ipu3/ipu3.cpp<br>
> > > +++ b/src/ipa/ipu3/ipu3.cpp<br>
> > > @@ -341,7 +341,7 @@ int IPAIPU3::init(const IPASettings &settings,<br>
> > ><br>
> > > /* Clean context */<br>
> > > context_.configuration = {};<br>
> > > - context_.configuration.sensor.lineDuration = sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate;<br>
> > > + context_.configuration.sensor.lineDuration = sensorInfo.minLineLength * 1.0s / sensorInfo.pixelRate;<br>
> ><br>
> > This is getting long, let's wrap it.<br>
> ><br>
> > context_.configuration.sensor.lineDuration = sensorInfo.minLineLength<br>
> > * 1.0s / sensorInfo.pixelRate;<br>
> ><br>
> > (I can also apply this change myself if no v2 is otherwise needed.)<br>
> ><br>
> > ><br>
> > > /* Load the tuning data file. */<br>
> > > File file(settings.configurationFile);<br>
> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> > > index 8d731435764e..358a119da222 100644<br>
> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> > > @@ -356,7 +356,7 @@ void IPARPi::setMode(const IPACameraSensorInfo &sensorInfo)<br>
> > > * Calculate the line length as the ratio between the line length in<br>
> > > * pixels and the pixel rate.<br>
> > > */<br>
> > > - mode_.lineLength = sensorInfo.lineLength * (1.0s / sensorInfo.pixelRate);<br>
> > > + mode_.lineLength = sensorInfo.minLineLength * (1.0s / sensorInfo.pixelRate);<br>
> > ><br>
> > > /*<br>
> > > * Set the frame length limits for the mode to ensure exposure and<br>
> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp<br>
> > > index 32feb1682749..ddb22d98eb41 100644<br>
> > > --- a/src/ipa/rkisp1/rkisp1.cpp<br>
> > > +++ b/src/ipa/rkisp1/rkisp1.cpp<br>
> > > @@ -246,7 +246,7 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,<br>
> > > context_.configuration.hw.revision = hwRevision_;<br>
> > ><br>
> > > context_.configuration.sensor.size = info.outputSize;<br>
> > > - context_.configuration.sensor.lineDuration = info.lineLength * 1.0s / info.pixelRate;<br>
> > > + context_.configuration.sensor.lineDuration = info.minLineLength * 1.0s / info.pixelRate;<br>
> > ><br>
> > > /*<br>
> > > * When the AGC computes the new exposure values for a frame, it needs<br>
> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp<br>
> > > index 911fd0beae4e..c2c8d3d44e26 100644<br>
> > > --- a/src/libcamera/camera_sensor.cpp<br>
> > > +++ b/src/libcamera/camera_sensor.cpp<br>
> > > @@ -883,10 +883,12 @@ int CameraSensor::sensorInfo(IPACameraSensorInfo *info) const<br>
> > > return -EINVAL;<br>
> > > }<br>
> > ><br>
> > > - int32_t hblank = ctrls.get(V4L2_CID_HBLANK).get<int32_t>();<br>
> > > - info->lineLength = info->outputSize.width + hblank;<br>
> > > info->pixelRate = ctrls.get(V4L2_CID_PIXEL_RATE).get<int64_t>();<br>
> > ><br>
> > > + const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK);<br>
> > > + info->minLineLength = info->outputSize.width + hblank.min().get<int32_t>();<br>
> > > + info->maxLineLength = info->outputSize.width + hblank.max().get<int32_t>();<br>
> ><br>
> > Hmmm... If the current value of V4L2_CID_HBLANK is different than the<br>
> > minimum, won't all the IPA modules compute an incorrect line length ?<br>
> > I'm sure this is fixed for the Raspberry Pi IPA module in subsequent<br>
> > patches, but the IPU3 and RkISP1 will likely be affected.<br>
> <br>
> This is true - I am making the (perhaps incorrect) assumption that sensors that<br>
> do not dynamically control hblank will have the control value set to the minimum.<br>
> I made the change here so that the min/max LineDuration fields in IPACameraSensorInfo<br>
> matches with what min/max FrameLength conveys.<br>
> <br>
> I can make one of two changes to fix this:<br>
> <br>
> 1) Revert the above code so that IPACameraSensorInfo continues to report<br>
> lineLength as before. I will then have to update our RPi mode setup so that<br>
> we essentially do the above in our own mode structure.<br>
> <br>
> 2) Keep the above change, and update rkisp1/ipu3/rpi IPAs to calculate line length<br>
> directly from the control value instead of from the fields in IPACameraSensorInfo.<br>
> <br>
> For consistency, I would prefer (2), but happy to do either.<br>
<br>
3) Set V4L2_CID_HBLANK to the minimum when initializing the sensor.<br>
<br>
Or could this introduce breakages ?<br></blockquote><div><br></div><div>The only breakage I can think of is if a user was setting HBLANK outside of</div><div>libcamera (through v4l2-ctl) and expecting the IPA to just use that value without</div><div>change. I don't know how common that is, particularly given how few sensor</div><div>drivers actually seem to have read/write HBLANK controls.</div><div><br></div><div>Naush</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> > > +<br>
> > > const ControlInfo vblank = ctrls.infoMap()->at(V4L2_CID_VBLANK);<br>
> > > info->minFrameLength = info->outputSize.height + vblank.min().get<int32_t>();<br>
> > > info->maxFrameLength = info->outputSize.height + vblank.max().get<int32_t>();<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>