<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thank you for your feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 4 Oct 2022 at 17:40, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">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>
Thank you for the patch.<br>
<br>
On Mon, Oct 03, 2022 at 09:39:27AM +0100, Naushir Patuck via 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></blockquote><div><br></div><div>I'll fix this up in the next revision.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<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></blockquote><div><br></div><div>This is true - I am making the (perhaps incorrect) assumption that sensors that</div><div>do not dynamically control hblank will have the control value set to the minimum.</div><div>I made the change here so that the min/max LineDuration fields in IPACameraSensorInfo</div><div>matches with what min/max FrameLength conveys.</div><div><br></div><div>I can make one of two changes to fix this:</div><div><br></div><div>1) Revert the above code so that IPACameraSensorInfo continues to report</div><div>lineLength as before. I will then have to update our RPi mode setup so that</div><div>we essentially do the above in our own mode structure.</div><div><br></div><div>2) Keep the above change, and update rkisp1/ipu3/rpi IPAs to calculate line length</div><div>directly from the control value instead of from the fields in IPACameraSensorInfo.</div><div><br></div><div>For consistency, I would prefer (2), but happy to do either.</div><div><br></div><div>Regards,</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>