<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thanks for the feedback!</div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 2 Dec 2022 at 11:42, 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 Tue, Nov 29, 2022 at 01:45:26PM +0000, Naushir Patuck via libcamera-devel wrote:<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>
One question here, does the configuration apply to the pipeline, or to<br>
the camera ? If multiple sensors are connected to the same Unicam<br>
instance (through an external multiplexers), will all these cameras<br>
always have the same configuration, or are there use cases for<br>
per-camera configuration ? Looking at the series, the configuration is<br>
stored per-camera, but is the same for all cameras handled by the<br>
pipeline handler.<br></blockquote><div><br></div><div>The intention is to allow us to run alternate configurations per-camera. </div><div>With this series, raspberrypi/default.json is loaded, unless the env variable</div><div>LIBCAMERA_RPI_CONFIG_FILE is set for the process.  Admittedly this<br></div><div>is not the neatest way of doing things.  We did alway talk about a mechanism</div><div>for the application to set the IPA tuning file via some API.  Whatever that API</div><div>may be, I expect the pipeline handler config to follow a similar mechanism.</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>
The answer to that question matters for the implementation in the<br>
Raspberry Pi pipeline handler, but also in the common helpers in case<br>
other platforms would need per-camera configurations.<br></blockquote><div><br></div><div>Without any sort of override mechanism, this is indeed a problem.</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>
> 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      | 51 ++++++++++++++++---<br>
>  1 file changed, 44 insertions(+), 7 deletions(-)<br>
> <br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 0e0b71945640..4486d31ea78d 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -294,6 +294,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 steram.<br>
<br>
s/steram/stream/<br>
<br>
> +              */<br>
> +             unsigned int minTotalUnicamBuffers;<br>
> +     };<br>
> +<br>
> +     Config config_;<br>
> +<br>
>  private:<br>
>       void checkRequestCompleted();<br>
>       void fillRequestMetadata(const ControlList &bufferControls,<br>
> @@ -346,6 +361,7 @@ private:<br>
>       }<br>
>  <br>
>       int registerCamera(MediaDevice *unicam, MediaDevice *isp, MediaEntity *sensorEntity);<br>
> +     int configurePipelineHandler(RPiCameraData *data);<br>
<br>
Wouldn't this be better placed in the RPiCameraData class ? I would also<br>
name the function loadPipelineHandlerConfiguration() (or similar) to<br>
make its purpose clearer.<br></blockquote><div><br></div><div>Ack</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>
I wonder if we shouldn't instead talk about "pipeline configuration"<br>
instead of "pipeline handler configuration", especially if the<br>
configuration is specific to a camera. What does everybody think ?<br></blockquote><div><br></div><div>Yes, I agree with that.  Pipeline configuration does sound more appropriate.</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></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>
>       int queueAllBuffers(Camera *camera);<br>
>       int prepareBuffers(Camera *camera);<br>
>       void mapBuffers(Camera *camera, const RPi::BufferMap &buffers, unsigned int mask);<br>
> @@ -1377,6 +1393,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>
<br>
"Unable to load pipeline handler configuration"<br>
<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>
> @@ -1389,6 +1411,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>
> @@ -1439,25 +1473,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>
<br>
I'd drop 'sets' here as it's not two buffer sets but two buffers.<br>
<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>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>