<div dir="ltr"><div dir="ltr">Hi Kieran,<div><br></div><div>Thank you for your feedback!</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 20 Jan 2023 at 10:51, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@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">Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:44)<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>
> Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> ---<br>
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 49 ++++++++++++++++---<br>
>  1 file changed, 42 insertions(+), 7 deletions(-)<br>
> <br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 8569df17976a..6bf9a669c679 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -199,6 +199,7 @@ public:<br>
>  <br>
>         int loadIPA(ipa::RPi::IPAInitResult *result);<br>
>         int configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result);<br>
> +       int configurePipeline();<br>
>  <br>
>         void enumerateVideoDevices(MediaLink *link);<br>
>  <br>
> @@ -295,6 +296,21 @@ public:<br>
>         /* Have internal buffers been allocated? */<br>
>         bool buffersAllocated_;<br>
>  <br>
> +       struct Config {<br>
> +               /*<br>
> +                * The minimum number of internal buffers to be allocated for<br>
> +                * the Unicam Image stream.<br>
> +                */<br>
> +               unsigned int minUnicamBuffers;<br>
> +               /*<br>
> +                * The minimum total (internal + external) buffer count used for<br>
> +                * the Unicam Image stream.<br>
<br>
Are there any limits that should be conveyed by this documentation?<br>
Like should minTotalUnicamBuffers always be recommended to at least 2?<br></blockquote><div><br></div><div>minTotalUnicamBuffers must be >= 1 and minTotalUnicamBuffers >= minUnicamBuffers<br></div><div><br></div><div>This is tested when we read the config file in RPiCameraData::configurePipeline(), but</div><div>perhaps a little comment in the config files might be needed?</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>
Can minTotalUnicamBuffers be less than minUnicamBuffers? If not - should<br>
this convey instead the quantity of buffers to allocate in addition to<br>
minUnicamBuffers instead?<br>
<br>
> +                */<br>
> +               unsigned int minTotalUnicamBuffers;<br>
> +       };<br>
> +<br>
> +       Config config_;<br>
> +<br>
>  private:<br>
>         void checkRequestCompleted();<br>
>         void fillRequestMetadata(const ControlList &bufferControls,<br>
> @@ -1378,6 +1394,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 = data->configurePipeline();<br>
<br>
Glancing above - would the number of supported streams ever be<br>
configurable, requiring this to load earlier ?<br></blockquote><div><br></div><div>No, I think that would be fixed at compile time.</div><div><br>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>
Of course such a change could load earlier as part of that so this is<br>
fine by me.<br>
<br>
Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
<br>
<br>
> +       if (ret) {<br>
> +               LOG(RPI, Error) << "Unable to load pipeline handler configuration";<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>
> @@ -1440,25 +1462,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 of internal buffers<br>
> +                        * 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>
> @@ -1631,6 +1656,16 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA<br>
>         return 0;<br>
>  }<br>
>  <br>
> +int RPiCameraData::configurePipeline()<br>
> +{<br>
> +       config_ = {<br>
> +               .minUnicamBuffers = 2,<br>
> +               .minTotalUnicamBuffers = 4,<br>
> +       };<br>
> +<br>
> +       return 0;<br>
> +}<br>
> +<br>
>  /*<br>
>   * enumerateVideoDevices() iterates over the Media Controller topology, starting<br>
>   * at the sensor and finishing at Unicam. For each sensor, RPiCameraData stores<br>
> -- <br>
> 2.25.1<br>
><br>
</blockquote></div></div>