<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 7 Oct 2022 at 23:28, 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 Thu, Oct 06, 2022 at 02:17:35PM +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>
> Since the IPAs now use minLineLength for their calculations, set the starting<br>
> value of the V4L2_CID_HBLANK control to its minimum in CameraSensor::init().<br>
> <br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> Tested-by: Dave Stevenson <<a href="mailto:dave.stevenson@raspberrypi.com" target="_blank">dave.stevenson@raspberrypi.com</a>><br>
> ---<br>
> include/libcamera/ipa/core.mojom | 21 +++++++++++++++------<br>
> src/ipa/ipu3/ipu3.cpp | 6 ++++--<br>
> src/ipa/raspberrypi/raspberrypi.cpp | 2 +-<br>
> src/ipa/rkisp1/rkisp1.cpp | 2 +-<br>
> src/libcamera/camera_sensor.cpp | 15 +++++++++++++--<br>
> 5 files changed, 34 insertions(+), 12 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 b93a09d40c39..7e26fc5639c2 100644<br>
> --- a/src/ipa/ipu3/ipu3.cpp<br>
> +++ b/src/ipa/ipu3/ipu3.cpp<br>
> @@ -336,7 +336,8 @@ 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<br>
> + * 1.0s / sensorInfo.pixelRate;<br>
<br>
The * should be aligned below the =. Same below.<br>
<br>
> <br>
> /* Load the tuning data file. */<br>
> File file(settings.configurationFile);<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>
> 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..83f81d655395 100644<br>
> --- a/src/libcamera/camera_sensor.cpp<br>
> +++ b/src/libcamera/camera_sensor.cpp<br>
> @@ -176,6 +176,15 @@ int CameraSensor::init()<br>
> if (ret)<br>
> return ret;<br>
> <br>
> + /* Set HBLANK to the minimum as a starting value. */<br>
<br>
I would capture the rationale here.<br>
<br>
/*<br>
* Set HBLANK to the minimum to start with a well-defined line length,<br>
* allowing IPA modules that do not modify HBLANK to use the sensor<br>
* minimum line length in their calculations.<br>
*/<br>
<br>
Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
<br>
No need to resubmit if only those small changes are required.<br>
<br>
> + const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK);<br>
> + ControlList ctrl(subdev_->controls());<br>
> +<br>
> + ctrl.set(V4L2_CID_HBLANK, hblank.min().get<int32_t>());<br>
> + ret = subdev_->setControls(&ctrl);<br>
> + if (ret)<br>
> + return ret;<br></blockquote><div><br></div><div>Actually, doing the setControls here unconditionally is probably incorrect.</div><div><br></div><div>If the control is read-only, we will return an error. We probably want to</div><div>do something like what I did for the IPA where we test for</div><div>HBLANK::min() == HBLANK::max(), and if true, assume the control is</div><div>read-only and don't do the setControl().</div><div><br></div><div>Sorry I should have caught that, but my testing was done on sensors</div><div>with adjustable HBLANK controls. I'll update the statement and post</div><div>a revised patch first thing on Monday.</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></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>
> return applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff);<br>
> }<br>
> <br>
> @@ -883,10 +892,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>
> 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>