<div dir="ltr"><div dir="ltr">Hi David,<div><br></div><div>Thank you for your feedback!</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 1 Nov 2022 at 11:50, David Plowman <<a href="mailto:david.plowman@raspberrypi.com">david.plowman@raspberrypi.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>
Thanks for the patch!<br>
<br>
On Fri, 14 Oct 2022 at 14:18, Naushir Patuck via libcamera-devel<br>
<<a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a>> wrote:<br>
><br>
> Add a configuration structure to store platform specific parameters used by<br>
> the pipeline handler. Currently, these only store Unicam buffer counts,<br>
> replacing the hardcoded static values in the source code.<br>
><br>
> In subsequent commits, more parameters will be added to the configuration<br>
> structure, and parameters will be read in through a config file.<br>
><br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
> .../pipeline/raspberrypi/raspberrypi.cpp | 43 ++++++++++++++++---<br>
> 1 file changed, 36 insertions(+), 7 deletions(-)<br>
><br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index d366a8bec007..7d1e454cddcd 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -294,6 +294,13 @@ public:<br>
> /* Have internal buffers been allocated? */<br>
> bool buffersAllocated_;<br>
><br>
> + struct Config {<br>
> + unsigned int minUnicamBuffers;<br>
> + unsigned int minTotalUnicamBuffers;<br>
<br>
I wonder slightly whether it's worth saying here what these numbers<br>
represent? Or maybe the comments in the code below are sufficient.<br></blockquote><div><br></div><div>Sure, I'll add comments to the struct - will probably copy out what is in the</div><div>json file!</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">
Either way:<br>
<br>
Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
<br>
Thanks!<br>
David<br>
<br>
> + };<br>
> +<br>
> + Config config_;<br>
> +<br>
> private:<br>
> void checkRequestCompleted();<br>
> void fillRequestMetadata(const ControlList &bufferControls,<br>
> @@ -343,6 +350,7 @@ private:<br>
> }<br>
><br>
> int registerCamera(MediaDevice *unicam, MediaDevice *isp, MediaEntity *sensorEntity);<br>
> + int configurePipelineHandler(RPiCameraData *data);<br>
> int queueAllBuffers(Camera *camera);<br>
> int prepareBuffers(Camera *camera);<br>
> void mapBuffers(Camera *camera, const RPi::BufferMap &buffers, unsigned int mask);<br>
> @@ -1368,6 +1376,12 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me<br>
> streams.insert(&data->isp_[Isp::Output0]);<br>
> streams.insert(&data->isp_[Isp::Output1]);<br>
><br>
> + int ret = configurePipelineHandler(data.get());<br>
> + if (ret) {<br>
> + LOG(RPI, Error) << "Unable to configure the pipeline handler!";<br>
> + return ret;<br>
> + }<br>
> +<br>
> /* Create and register the camera. */<br>
> const std::string &id = data->sensor_->id();<br>
> std::shared_ptr<Camera> camera =<br>
> @@ -1380,6 +1394,18 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me<br>
> return 0;<br>
> }<br>
><br>
> +int PipelineHandlerRPi::configurePipelineHandler(RPiCameraData *data)<br>
> +{<br>
> + RPiCameraData::Config &config = data->config_;<br>
> +<br>
> + config = {<br>
> + .minUnicamBuffers = 2,<br>
> + .minTotalUnicamBuffers = 4,<br>
> + };<br>
> +<br>
> + return 0;<br>
> +}<br>
> +<br>
> int PipelineHandlerRPi::queueAllBuffers(Camera *camera)<br>
> {<br>
> RPiCameraData *data = cameraData(camera);<br>
> @@ -1430,25 +1456,28 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)<br>
> for (auto const stream : data->streams_) {<br>
> unsigned int numBuffers;<br>
> /*<br>
> - * For Unicam, allocate a minimum of 4 buffers as we want<br>
> - * to avoid any frame drops.<br>
> + * For Unicam, allocate a minimum number of buffers for internal<br>
> + * use as we want to avoid any frame drops.<br>
> */<br>
> - constexpr unsigned int minBuffers = 4;<br>
> + const unsigned int minBuffers = data->config_.minTotalUnicamBuffers;<br>
> if (stream == &data->unicam_[Unicam::Image]) {<br>
> /*<br>
> * If an application has configured a RAW stream, allocate<br>
> * additional buffers to make up the minimum, but ensure<br>
> - * we have at least 2 sets of internal buffers to use to<br>
> - * minimise frame drops.<br>
> + * we have at least minUnicamBuffers sets of internal<br>
> + * buffers to use to minimise frame drops.<br>
> */<br>
> - numBuffers = std::max<int>(2, minBuffers - numRawBuffers);<br>
> + numBuffers = std::max<int>(data->config_.minUnicamBuffers,<br>
> + minBuffers - numRawBuffers);<br>
> } else if (stream == &data->isp_[Isp::Input]) {<br>
> /*<br>
> * ISP input buffers are imported from Unicam, so follow<br>
> * similar logic as above to count all the RAW buffers<br>
> * available.<br>
> */<br>
> - numBuffers = numRawBuffers + std::max<int>(2, minBuffers - numRawBuffers);<br>
> + numBuffers = numRawBuffers +<br>
> + std::max<int>(data->config_.minUnicamBuffers,<br>
> + minBuffers - numRawBuffers);<br>
><br>
> } else if (stream == &data->unicam_[Unicam::Embedded]) {<br>
> /*<br>
> --<br>
> 2.25.1<br>
><br>
</blockquote></div></div>