<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thank you for your review feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 4 Feb 2021 at 20:07, 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 Fri, Jan 29, 2021 at 11:16:13AM +0000, Naushir Patuck wrote:<br>
> Sensor frame length is made up of active and inactive (blanking) lines.<br>
> The minimum and maximum frame length values may be used by pipeline<br>
> handlers to limit frame durations based on the sensor mode capabilities.<br>
> <br>
> Store the minimum and maximum allowable frame length values (in lines)<br>
> in the CameraSensorInfo structure. These values are computed in<br>
> CameraSensor::sensorInfo() by querying the sensor subdevice<br>
> V4L2_CID_VBLANK control limits. This in turn means that V4L2_CID_VBLANK<br>
> is now a mandatory subdevice control.<br>
> <br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> Reviewed-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>><br>
> ---<br>
> include/libcamera/internal/camera_sensor.h | 3 ++<br>
> src/libcamera/camera_sensor.cpp | 43 ++++++++++++++++++++--<br>
> test/ipa/ipa_wrappers_test.cpp | 2 +<br>
> 3 files changed, 44 insertions(+), 4 deletions(-)<br>
> <br>
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h<br>
> index fed36bf26e47..5d8c9b1a3121 100644<br>
> --- a/include/libcamera/internal/camera_sensor.h<br>
> +++ b/include/libcamera/internal/camera_sensor.h<br>
> @@ -33,6 +33,9 @@ struct CameraSensorInfo {<br>
> <br>
> uint64_t pixelRate;<br>
> uint32_t lineLength;<br>
> +<br>
> + uint32_t minFrameLength;<br>
> + uint32_t maxFrameLength;<br>
> };<br>
> <br>
> class CameraSensor : protected Loggable<br>
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp<br>
> index ab315bdc468c..f60d0cc9c6fa 100644<br>
> --- a/src/libcamera/camera_sensor.cpp<br>
> +++ b/src/libcamera/camera_sensor.cpp<br>
> @@ -113,6 +113,36 @@ LOG_DEFINE_CATEGORY(CameraSensor)<br>
> * The total line length in pixel clock periods, including blanking.<br>
> */<br>
> <br>
> +/**<br>
> + * \var CameraSensorInfo::minFrameLength<br>
> + * \brief The minimum allowable frame length in units of lines<br>
> + *<br>
> + * The sensor frame length comprises of active output lines and blanking lines<br>
<br>
s/comprises of/comprises/ ? If you can let me know which form is<br>
correct, I can fix this when applying.<br></blockquote><div><br></div><div>I would have said "comprises of" sounds more appropriate here. If it's ok with you, I'd leave as-is.</div><div><br></div><div>Regards,</div><div>Naush</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>
Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
<br>
> + * in a frame. The minimum frame length value dictates the minimum allowable<br>
> + * frame duration of the sensor mode.<br>
> + *<br>
> + * To obtain the minimum frame duration:<br>
> + *<br>
> + * \verbatim<br>
> + frameDuration(s) = minFrameLength(lines) * lineLength(pixels) / pixelRate(pixels per second)<br>
> + \endverbatim<br>
> + */<br>
> +<br>
> +/**<br>
> + * \var CameraSensorInfo::maxFrameLength<br>
> + * \brief The maximum allowable frame length in units of lines<br>
> + *<br>
> + * The sensor frame length comprises of active output lines and blanking lines<br>
> + * in a frame. The maximum frame length value dictates the maximum allowable<br>
> + * frame duration of the sensor mode.<br>
> + *<br>
> + * To obtain the maximum frame duration:<br>
> + *<br>
> + * \verbatim<br>
> + frameDuration(s) = maxFrameLength(lines) * lineLength(pixels) / pixelRate(pixels per second)<br>
> + \endverbatim<br>
> + */<br>
> +<br>
> /**<br>
> * \class CameraSensor<br>
> * \brief A camera sensor based on V4L2 subdevices<br>
> @@ -699,12 +729,13 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const<br>
> info->outputSize = format.size;<br>
> <br>
> /*<br>
> - * Retrieve the pixel rate and the line length through V4L2 controls.<br>
> - * Support for the V4L2_CID_PIXEL_RATE and V4L2_CID_HBLANK controls is<br>
> - * mandatory.<br>
> + * Retrieve the pixel rate, line length and minimum/maximum frame<br>
> + * duration through V4L2 controls. Support for the V4L2_CID_PIXEL_RATE,<br>
> + * V4L2_CID_HBLANK and V4L2_CID_VBLANK controls is mandatory.<br>
> */<br>
> ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE,<br>
> - V4L2_CID_HBLANK });<br>
> + V4L2_CID_HBLANK,<br>
> + V4L2_CID_VBLANK });<br>
> if (ctrls.empty()) {<br>
> LOG(CameraSensor, Error)<br>
> << "Failed to retrieve camera info controls";<br>
> @@ -715,6 +746,10 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const<br>
> info->lineLength = info->outputSize.width + hblank;<br>
> info->pixelRate = ctrls.get(V4L2_CID_PIXEL_RATE).get<int64_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>
> return 0;<br>
> }<br>
> <br>
> diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp<br>
> index 47533d105d03..eb6d783e8489 100644<br>
> --- a/test/ipa/ipa_wrappers_test.cpp<br>
> +++ b/test/ipa/ipa_wrappers_test.cpp<br>
> @@ -313,6 +313,8 @@ protected:<br>
> .outputSize = { 2560, 1940 },<br>
> .pixelRate = 96000000,<br>
> .lineLength = 2918,<br>
> + .minFrameLength = 1940,<br>
> + .maxFrameLength = 2880<br>
> };<br>
> std::map<unsigned int, IPAStream> config{<br>
> { 1, { V4L2_PIX_FMT_YUYV, { 1024, 768 } } },<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>