| Message ID | 20230118085953.7027-4-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:44) > Add a configuration structure to store platform specific parameters used by > the pipeline handler. Currently, these only store Unicam buffer counts, > replacing the hardcoded static values in the source code. > > In subsequent commits, more parameters will be added to the configuration > structure, and parameters will be read in through a config file. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 49 ++++++++++++++++--- > 1 file changed, 42 insertions(+), 7 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 8569df17976a..6bf9a669c679 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -199,6 +199,7 @@ public: > > int loadIPA(ipa::RPi::IPAInitResult *result); > int configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result); > + int configurePipeline(); > > void enumerateVideoDevices(MediaLink *link); > > @@ -295,6 +296,21 @@ public: > /* Have internal buffers been allocated? */ > bool buffersAllocated_; > > + struct Config { > + /* > + * The minimum number of internal buffers to be allocated for > + * the Unicam Image stream. > + */ > + unsigned int minUnicamBuffers; > + /* > + * The minimum total (internal + external) buffer count used for > + * the Unicam Image stream. Are there any limits that should be conveyed by this documentation? Like should minTotalUnicamBuffers always be recommended to at least 2? Can minTotalUnicamBuffers be less than minUnicamBuffers? If not - should this convey instead the quantity of buffers to allocate in addition to minUnicamBuffers instead? > + */ > + unsigned int minTotalUnicamBuffers; > + }; > + > + Config config_; > + > private: > void checkRequestCompleted(); > void fillRequestMetadata(const ControlList &bufferControls, > @@ -1378,6 +1394,12 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me > streams.insert(&data->isp_[Isp::Output0]); > streams.insert(&data->isp_[Isp::Output1]); > > + int ret = data->configurePipeline(); Glancing above - would the number of supported streams ever be configurable, requiring this to load earlier ? Of course such a change could load earlier as part of that so this is fine by me. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + if (ret) { > + LOG(RPI, Error) << "Unable to load pipeline handler configuration"; > + return ret; > + } > + > /* Create and register the camera. */ > const std::string &id = data->sensor_->id(); > std::shared_ptr<Camera> camera = > @@ -1440,25 +1462,28 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera) > for (auto const stream : data->streams_) { > unsigned int numBuffers; > /* > - * For Unicam, allocate a minimum of 4 buffers as we want > - * to avoid any frame drops. > + * For Unicam, allocate a minimum number of buffers for internal > + * use as we want to avoid any frame drops. > */ > - constexpr unsigned int minBuffers = 4; > + const unsigned int minBuffers = data->config_.minTotalUnicamBuffers; > if (stream == &data->unicam_[Unicam::Image]) { > /* > * If an application has configured a RAW stream, allocate > * additional buffers to make up the minimum, but ensure > - * we have at least 2 sets of internal buffers to use to > - * minimise frame drops. > + * we have at least minUnicamBuffers of internal buffers > + * to use to minimise frame drops. > */ > - numBuffers = std::max<int>(2, minBuffers - numRawBuffers); > + numBuffers = std::max<int>(data->config_.minUnicamBuffers, > + minBuffers - numRawBuffers); > } else if (stream == &data->isp_[Isp::Input]) { > /* > * ISP input buffers are imported from Unicam, so follow > * similar logic as above to count all the RAW buffers > * available. > */ > - numBuffers = numRawBuffers + std::max<int>(2, minBuffers - numRawBuffers); > + numBuffers = numRawBuffers + > + std::max<int>(data->config_.minUnicamBuffers, > + minBuffers - numRawBuffers); > > } else if (stream == &data->unicam_[Unicam::Embedded]) { > /* > @@ -1631,6 +1656,16 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA > return 0; > } > > +int RPiCameraData::configurePipeline() > +{ > + config_ = { > + .minUnicamBuffers = 2, > + .minTotalUnicamBuffers = 4, > + }; > + > + return 0; > +} > + > /* > * enumerateVideoDevices() iterates over the Media Controller topology, starting > * at the sensor and finishing at Unicam. For each sensor, RPiCameraData stores > -- > 2.25.1 >
Hi Kieran, Thank you for your feedback! On Fri, 20 Jan 2023 at 10:51, Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:44) > > Add a configuration structure to store platform specific parameters used > by > > the pipeline handler. Currently, these only store Unicam buffer counts, > > replacing the hardcoded static values in the source code. > > > > In subsequent commits, more parameters will be added to the configuration > > structure, and parameters will be read in through a config file. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > .../pipeline/raspberrypi/raspberrypi.cpp | 49 ++++++++++++++++--- > > 1 file changed, 42 insertions(+), 7 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 8569df17976a..6bf9a669c679 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -199,6 +199,7 @@ public: > > > > int loadIPA(ipa::RPi::IPAInitResult *result); > > int configureIPA(const CameraConfiguration *config, > ipa::RPi::IPAConfigResult *result); > > + int configurePipeline(); > > > > void enumerateVideoDevices(MediaLink *link); > > > > @@ -295,6 +296,21 @@ public: > > /* Have internal buffers been allocated? */ > > bool buffersAllocated_; > > > > + struct Config { > > + /* > > + * The minimum number of internal buffers to be > allocated for > > + * the Unicam Image stream. > > + */ > > + unsigned int minUnicamBuffers; > > + /* > > + * The minimum total (internal + external) buffer count > used for > > + * the Unicam Image stream. > > Are there any limits that should be conveyed by this documentation? > Like should minTotalUnicamBuffers always be recommended to at least 2? > minTotalUnicamBuffers must be >= 1 and minTotalUnicamBuffers >= minUnicamBuffers This is tested when we read the config file in RPiCameraData::configurePipeline(), but perhaps a little comment in the config files might be needed? > > Can minTotalUnicamBuffers be less than minUnicamBuffers? If not - should > this convey instead the quantity of buffers to allocate in addition to > minUnicamBuffers instead? > > > + */ > > + unsigned int minTotalUnicamBuffers; > > + }; > > + > > + Config config_; > > + > > private: > > void checkRequestCompleted(); > > void fillRequestMetadata(const ControlList &bufferControls, > > @@ -1378,6 +1394,12 @@ int > PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me > > streams.insert(&data->isp_[Isp::Output0]); > > streams.insert(&data->isp_[Isp::Output1]); > > > > + int ret = data->configurePipeline(); > > Glancing above - would the number of supported streams ever be > configurable, requiring this to load earlier ? > No, I think that would be fixed at compile time. Regards, Naush > > Of course such a change could load earlier as part of that so this is > fine by me. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > + if (ret) { > > + LOG(RPI, Error) << "Unable to load pipeline handler > configuration"; > > + return ret; > > + } > > + > > /* Create and register the camera. */ > > const std::string &id = data->sensor_->id(); > > std::shared_ptr<Camera> camera = > > @@ -1440,25 +1462,28 @@ int PipelineHandlerRPi::prepareBuffers(Camera > *camera) > > for (auto const stream : data->streams_) { > > unsigned int numBuffers; > > /* > > - * For Unicam, allocate a minimum of 4 buffers as we want > > - * to avoid any frame drops. > > + * For Unicam, allocate a minimum number of buffers for > internal > > + * use as we want to avoid any frame drops. > > */ > > - constexpr unsigned int minBuffers = 4; > > + const unsigned int minBuffers = > data->config_.minTotalUnicamBuffers; > > if (stream == &data->unicam_[Unicam::Image]) { > > /* > > * If an application has configured a RAW > stream, allocate > > * additional buffers to make up the minimum, > but ensure > > - * we have at least 2 sets of internal buffers > to use to > > - * minimise frame drops. > > + * we have at least minUnicamBuffers of internal > buffers > > + * to use to minimise frame drops. > > */ > > - numBuffers = std::max<int>(2, minBuffers - > numRawBuffers); > > + numBuffers = > std::max<int>(data->config_.minUnicamBuffers, > > + minBuffers - > numRawBuffers); > > } else if (stream == &data->isp_[Isp::Input]) { > > /* > > * ISP input buffers are imported from Unicam, > so follow > > * similar logic as above to count all the RAW > buffers > > * available. > > */ > > - numBuffers = numRawBuffers + std::max<int>(2, > minBuffers - numRawBuffers); > > + numBuffers = numRawBuffers + > > + > std::max<int>(data->config_.minUnicamBuffers, > > + minBuffers - > numRawBuffers); > > > > } else if (stream == &data->unicam_[Unicam::Embedded]) { > > /* > > @@ -1631,6 +1656,16 @@ int RPiCameraData::configureIPA(const > CameraConfiguration *config, ipa::RPi::IPA > > return 0; > > } > > > > +int RPiCameraData::configurePipeline() > > +{ > > + config_ = { > > + .minUnicamBuffers = 2, > > + .minTotalUnicamBuffers = 4, > > + }; > > + > > + return 0; > > +} > > + > > /* > > * enumerateVideoDevices() iterates over the Media Controller topology, > starting > > * at the sensor and finishing at Unicam. For each sensor, > RPiCameraData stores > > -- > > 2.25.1 > > >
Quoting Naushir Patuck (2023-01-20 11:37:13) > Hi Kieran, > > Thank you for your feedback! > > On Fri, 20 Jan 2023 at 10:51, Kieran Bingham < > kieran.bingham@ideasonboard.com> wrote: > > > Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:44) > > > Add a configuration structure to store platform specific parameters used > > by > > > the pipeline handler. Currently, these only store Unicam buffer counts, > > > replacing the hardcoded static values in the source code. > > > > > > In subsequent commits, more parameters will be added to the configuration > > > structure, and parameters will be read in through a config file. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > .../pipeline/raspberrypi/raspberrypi.cpp | 49 ++++++++++++++++--- > > > 1 file changed, 42 insertions(+), 7 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > index 8569df17976a..6bf9a669c679 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -199,6 +199,7 @@ public: > > > > > > int loadIPA(ipa::RPi::IPAInitResult *result); > > > int configureIPA(const CameraConfiguration *config, > > ipa::RPi::IPAConfigResult *result); > > > + int configurePipeline(); > > > > > > void enumerateVideoDevices(MediaLink *link); > > > > > > @@ -295,6 +296,21 @@ public: > > > /* Have internal buffers been allocated? */ > > > bool buffersAllocated_; > > > > > > + struct Config { > > > + /* > > > + * The minimum number of internal buffers to be > > allocated for > > > + * the Unicam Image stream. > > > + */ > > > + unsigned int minUnicamBuffers; > > > + /* > > > + * The minimum total (internal + external) buffer count > > used for > > > + * the Unicam Image stream. > > > > Are there any limits that should be conveyed by this documentation? > > Like should minTotalUnicamBuffers always be recommended to at least 2? > > > > minTotalUnicamBuffers must be >= 1 and minTotalUnicamBuffers >= > minUnicamBuffers > > This is tested when we read the config file > in RPiCameraData::configurePipeline(), but > perhaps a little comment in the config files might be needed? It needs to be whereever the option is documented such that the user (or system integrator?) will read that documentation. At the moment it seems we only have that in the config files, but we can't generate documentation from the config files... (or can we?) Essentially - I belive this information needs to find its way into: https://libcamera.org/api-html/ I plan to split this into 'public' api documentation for libcamera, and internal documentation. But configuration files are essentially a public API. > > Can minTotalUnicamBuffers be less than minUnicamBuffers? If not - should > > this convey instead the quantity of buffers to allocate in addition to > > minUnicamBuffers instead? > > > > > + */ > > > + unsigned int minTotalUnicamBuffers; > > > + }; > > > + > > > + Config config_; > > > + > > > private: > > > void checkRequestCompleted(); > > > void fillRequestMetadata(const ControlList &bufferControls, > > > @@ -1378,6 +1394,12 @@ int > > PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me > > > streams.insert(&data->isp_[Isp::Output0]); > > > streams.insert(&data->isp_[Isp::Output1]); > > > > > > + int ret = data->configurePipeline(); > > > > Glancing above - would the number of supported streams ever be > > configurable, requiring this to load earlier ? > > > > No, I think that would be fixed at compile time. Ok - that's fine. > > Regards, > Naush > > > > > > Of course such a change could load earlier as part of that so this is > > fine by me. > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > + if (ret) { > > > + LOG(RPI, Error) << "Unable to load pipeline handler > > configuration"; > > > + return ret; > > > + } > > > + > > > /* Create and register the camera. */ > > > const std::string &id = data->sensor_->id(); > > > std::shared_ptr<Camera> camera = > > > @@ -1440,25 +1462,28 @@ int PipelineHandlerRPi::prepareBuffers(Camera > > *camera) > > > for (auto const stream : data->streams_) { > > > unsigned int numBuffers; > > > /* > > > - * For Unicam, allocate a minimum of 4 buffers as we want > > > - * to avoid any frame drops. > > > + * For Unicam, allocate a minimum number of buffers for > > internal > > > + * use as we want to avoid any frame drops. > > > */ > > > - constexpr unsigned int minBuffers = 4; > > > + const unsigned int minBuffers = > > data->config_.minTotalUnicamBuffers; > > > if (stream == &data->unicam_[Unicam::Image]) { > > > /* > > > * If an application has configured a RAW > > stream, allocate > > > * additional buffers to make up the minimum, > > but ensure > > > - * we have at least 2 sets of internal buffers > > to use to > > > - * minimise frame drops. > > > + * we have at least minUnicamBuffers of internal > > buffers > > > + * to use to minimise frame drops. > > > */ > > > - numBuffers = std::max<int>(2, minBuffers - > > numRawBuffers); > > > + numBuffers = > > std::max<int>(data->config_.minUnicamBuffers, > > > + minBuffers - > > numRawBuffers); > > > } else if (stream == &data->isp_[Isp::Input]) { > > > /* > > > * ISP input buffers are imported from Unicam, > > so follow > > > * similar logic as above to count all the RAW > > buffers > > > * available. > > > */ > > > - numBuffers = numRawBuffers + std::max<int>(2, > > minBuffers - numRawBuffers); > > > + numBuffers = numRawBuffers + > > > + > > std::max<int>(data->config_.minUnicamBuffers, > > > + minBuffers - > > numRawBuffers); > > > > > > } else if (stream == &data->unicam_[Unicam::Embedded]) { > > > /* > > > @@ -1631,6 +1656,16 @@ int RPiCameraData::configureIPA(const > > CameraConfiguration *config, ipa::RPi::IPA > > > return 0; > > > } > > > > > > +int RPiCameraData::configurePipeline() > > > +{ > > > + config_ = { > > > + .minUnicamBuffers = 2, > > > + .minTotalUnicamBuffers = 4, > > > + }; > > > + > > > + return 0; > > > +} > > > + > > > /* > > > * enumerateVideoDevices() iterates over the Media Controller topology, > > starting > > > * at the sensor and finishing at Unicam. For each sensor, > > RPiCameraData stores > > > -- > > > 2.25.1 > > > > >
On Fri, Jan 20, 2023 at 02:07:53PM +0000, Kieran Bingham via libcamera-devel wrote: > Quoting Naushir Patuck (2023-01-20 11:37:13) > > On Fri, 20 Jan 2023 at 10:51, Kieran Bingham wrote: > > > Quoting Naushir Patuck via libcamera-devel (2023-01-18 08:59:44) > > > > Add a configuration structure to store platform specific parameters used > > > by > > > > the pipeline handler. Currently, these only store Unicam buffer counts, > > > > replacing the hardcoded static values in the source code. > > > > > > > > In subsequent commits, more parameters will be added to the configuration > > > > structure, and parameters will be read in through a config file. > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > > --- > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 49 ++++++++++++++++--- > > > > 1 file changed, 42 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > index 8569df17976a..6bf9a669c679 100644 > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > @@ -199,6 +199,7 @@ public: > > > > > > > > int loadIPA(ipa::RPi::IPAInitResult *result); > > > > int configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result); > > > > + int configurePipeline(); > > > > > > > > void enumerateVideoDevices(MediaLink *link); > > > > > > > > @@ -295,6 +296,21 @@ public: > > > > /* Have internal buffers been allocated? */ > > > > bool buffersAllocated_; > > > > > > > > + struct Config { > > > > + /* > > > > + * The minimum number of internal buffers to be allocated for > > > > + * the Unicam Image stream. > > > > + */ > > > > + unsigned int minUnicamBuffers; > > > > + /* > > > > + * The minimum total (internal + external) buffer count used for > > > > + * the Unicam Image stream. > > > > > > Are there any limits that should be conveyed by this documentation? > > > Like should minTotalUnicamBuffers always be recommended to at least 2? > > > > > > > minTotalUnicamBuffers must be >= 1 and minTotalUnicamBuffers >= > > minUnicamBuffers > > > > This is tested when we read the config file > > in RPiCameraData::configurePipeline(), but > > perhaps a little comment in the config files might be needed? > > It needs to be whereever the option is documented such that the user > (or system integrator?) will read that documentation. At the moment it > seems we only have that in the config files, but we can't generate > documentation from the config files... (or can we?) > > Essentially - I belive this information needs to find its way into: > https://libcamera.org/api-html/ > > I plan to split this into 'public' api documentation for libcamera, and > internal documentation. > > But configuration files are essentially a public API. I'm not sure about that, I think it will end up in a different documentation. End-users are not expected to read the API reference documentation, and neither are system integrator. What is quite sure is that it should be documented somewhere, and having the information in the comments above is a good idea. We can later move it somewhere else when the time will be right. > > > Can minTotalUnicamBuffers be less than minUnicamBuffers? If not - should > > > this convey instead the quantity of buffers to allocate in addition to > > > minUnicamBuffers instead? > > > > > > > + */ > > > > + unsigned int minTotalUnicamBuffers; > > > > + }; > > > > + > > > > + Config config_; > > > > + > > > > private: > > > > void checkRequestCompleted(); > > > > void fillRequestMetadata(const ControlList &bufferControls, > > > > @@ -1378,6 +1394,12 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me > > > > streams.insert(&data->isp_[Isp::Output0]); > > > > streams.insert(&data->isp_[Isp::Output1]); > > > > > > > > + int ret = data->configurePipeline(); > > > > > > Glancing above - would the number of supported streams ever be > > > configurable, requiring this to load earlier ? > > > > > > > No, I think that would be fixed at compile time. > > Ok - that's fine. > > > > Of course such a change could load earlier as part of that so this is > > > fine by me. > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > + if (ret) { > > > > + LOG(RPI, Error) << "Unable to load pipeline handler configuration"; > > > > + return ret; > > > > + } > > > > + > > > > /* Create and register the camera. */ > > > > const std::string &id = data->sensor_->id(); > > > > std::shared_ptr<Camera> camera = > > > > @@ -1440,25 +1462,28 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera) > > > > for (auto const stream : data->streams_) { > > > > unsigned int numBuffers; > > > > /* > > > > - * For Unicam, allocate a minimum of 4 buffers as we want > > > > - * to avoid any frame drops. > > > > + * For Unicam, allocate a minimum number of buffers for internal > > > > + * use as we want to avoid any frame drops. > > > > */ > > > > - constexpr unsigned int minBuffers = 4; > > > > + const unsigned int minBuffers = data->config_.minTotalUnicamBuffers; > > > > if (stream == &data->unicam_[Unicam::Image]) { > > > > /* > > > > * If an application has configured a RAW stream, allocate > > > > * additional buffers to make up the minimum, but ensure > > > > - * we have at least 2 sets of internal buffers to use to > > > > - * minimise frame drops. > > > > + * we have at least minUnicamBuffers of internal buffers > > > > + * to use to minimise frame drops. > > > > */ > > > > - numBuffers = std::max<int>(2, minBuffers - numRawBuffers); > > > > + numBuffers = std::max<int>(data->config_.minUnicamBuffers, > > > > + minBuffers - numRawBuffers); > > > > } else if (stream == &data->isp_[Isp::Input]) { > > > > /* > > > > * ISP input buffers are imported from Unicam, so follow > > > > * similar logic as above to count all the RAW buffers > > > > * available. > > > > */ > > > > - numBuffers = numRawBuffers + std::max<int>(2, minBuffers - numRawBuffers); > > > > + numBuffers = numRawBuffers + > > > > + std::max<int>(data->config_.minUnicamBuffers, > > > > + minBuffers - numRawBuffers); > > > > > > > > } else if (stream == &data->unicam_[Unicam::Embedded]) { > > > > /* > > > > @@ -1631,6 +1656,16 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA > > > > return 0; > > > > } > > > > > > > > +int RPiCameraData::configurePipeline() > > > > +{ > > > > + config_ = { > > > > + .minUnicamBuffers = 2, > > > > + .minTotalUnicamBuffers = 4, > > > > + }; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > /* > > > > * enumerateVideoDevices() iterates over the Media Controller topology, starting > > > > * at the sensor and finishing at Unicam. For each sensor, RPiCameraData stores
Hi Naush, Thank you for the patch. On Wed, Jan 18, 2023 at 08:59:44AM +0000, Naushir Patuck via libcamera-devel wrote: > Add a configuration structure to store platform specific parameters used by > the pipeline handler. Currently, these only store Unicam buffer counts, > replacing the hardcoded static values in the source code. > > In subsequent commits, more parameters will be added to the configuration > structure, and parameters will be read in through a config file. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 49 ++++++++++++++++--- > 1 file changed, 42 insertions(+), 7 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 8569df17976a..6bf9a669c679 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -199,6 +199,7 @@ public: > > int loadIPA(ipa::RPi::IPAInitResult *result); > int configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result); > + int configurePipeline(); I think loadPipelineConfiguration() would be more explicit. Would it be too long though ? > > void enumerateVideoDevices(MediaLink *link); > > @@ -295,6 +296,21 @@ public: > /* Have internal buffers been allocated? */ > bool buffersAllocated_; > > + struct Config { > + /* > + * The minimum number of internal buffers to be allocated for > + * the Unicam Image stream. > + */ > + unsigned int minUnicamBuffers; > + /* > + * The minimum total (internal + external) buffer count used for > + * the Unicam Image stream. > + */ > + unsigned int minTotalUnicamBuffers; > + }; > + > + Config config_; > + > private: > void checkRequestCompleted(); > void fillRequestMetadata(const ControlList &bufferControls, > @@ -1378,6 +1394,12 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me > streams.insert(&data->isp_[Isp::Output0]); > streams.insert(&data->isp_[Isp::Output1]); > > + int ret = data->configurePipeline(); > + if (ret) { > + LOG(RPI, Error) << "Unable to load pipeline handler configuration"; And to match the function name, s/handler // This patch otherwise looks good to me. > + return ret; > + } > + > /* Create and register the camera. */ > const std::string &id = data->sensor_->id(); > std::shared_ptr<Camera> camera = > @@ -1440,25 +1462,28 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera) > for (auto const stream : data->streams_) { > unsigned int numBuffers; > /* > - * For Unicam, allocate a minimum of 4 buffers as we want > - * to avoid any frame drops. > + * For Unicam, allocate a minimum number of buffers for internal > + * use as we want to avoid any frame drops. > */ > - constexpr unsigned int minBuffers = 4; > + const unsigned int minBuffers = data->config_.minTotalUnicamBuffers; > if (stream == &data->unicam_[Unicam::Image]) { > /* > * If an application has configured a RAW stream, allocate > * additional buffers to make up the minimum, but ensure > - * we have at least 2 sets of internal buffers to use to > - * minimise frame drops. > + * we have at least minUnicamBuffers of internal buffers > + * to use to minimise frame drops. > */ > - numBuffers = std::max<int>(2, minBuffers - numRawBuffers); > + numBuffers = std::max<int>(data->config_.minUnicamBuffers, > + minBuffers - numRawBuffers); > } else if (stream == &data->isp_[Isp::Input]) { > /* > * ISP input buffers are imported from Unicam, so follow > * similar logic as above to count all the RAW buffers > * available. > */ > - numBuffers = numRawBuffers + std::max<int>(2, minBuffers - numRawBuffers); > + numBuffers = numRawBuffers + > + std::max<int>(data->config_.minUnicamBuffers, > + minBuffers - numRawBuffers); > > } else if (stream == &data->unicam_[Unicam::Embedded]) { > /* > @@ -1631,6 +1656,16 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA > return 0; > } > > +int RPiCameraData::configurePipeline() > +{ > + config_ = { > + .minUnicamBuffers = 2, > + .minTotalUnicamBuffers = 4, > + }; > + > + return 0; > +} > + > /* > * enumerateVideoDevices() iterates over the Media Controller topology, starting > * at the sensor and finishing at Unicam. For each sensor, RPiCameraData stores
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 8569df17976a..6bf9a669c679 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -199,6 +199,7 @@ public: int loadIPA(ipa::RPi::IPAInitResult *result); int configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result); + int configurePipeline(); void enumerateVideoDevices(MediaLink *link); @@ -295,6 +296,21 @@ public: /* Have internal buffers been allocated? */ bool buffersAllocated_; + struct Config { + /* + * The minimum number of internal buffers to be allocated for + * the Unicam Image stream. + */ + unsigned int minUnicamBuffers; + /* + * The minimum total (internal + external) buffer count used for + * the Unicam Image stream. + */ + unsigned int minTotalUnicamBuffers; + }; + + Config config_; + private: void checkRequestCompleted(); void fillRequestMetadata(const ControlList &bufferControls, @@ -1378,6 +1394,12 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me streams.insert(&data->isp_[Isp::Output0]); streams.insert(&data->isp_[Isp::Output1]); + int ret = data->configurePipeline(); + if (ret) { + LOG(RPI, Error) << "Unable to load pipeline handler configuration"; + return ret; + } + /* Create and register the camera. */ const std::string &id = data->sensor_->id(); std::shared_ptr<Camera> camera = @@ -1440,25 +1462,28 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera) for (auto const stream : data->streams_) { unsigned int numBuffers; /* - * For Unicam, allocate a minimum of 4 buffers as we want - * to avoid any frame drops. + * For Unicam, allocate a minimum number of buffers for internal + * use as we want to avoid any frame drops. */ - constexpr unsigned int minBuffers = 4; + const unsigned int minBuffers = data->config_.minTotalUnicamBuffers; if (stream == &data->unicam_[Unicam::Image]) { /* * If an application has configured a RAW stream, allocate * additional buffers to make up the minimum, but ensure - * we have at least 2 sets of internal buffers to use to - * minimise frame drops. + * we have at least minUnicamBuffers of internal buffers + * to use to minimise frame drops. */ - numBuffers = std::max<int>(2, minBuffers - numRawBuffers); + numBuffers = std::max<int>(data->config_.minUnicamBuffers, + minBuffers - numRawBuffers); } else if (stream == &data->isp_[Isp::Input]) { /* * ISP input buffers are imported from Unicam, so follow * similar logic as above to count all the RAW buffers * available. */ - numBuffers = numRawBuffers + std::max<int>(2, minBuffers - numRawBuffers); + numBuffers = numRawBuffers + + std::max<int>(data->config_.minUnicamBuffers, + minBuffers - numRawBuffers); } else if (stream == &data->unicam_[Unicam::Embedded]) { /* @@ -1631,6 +1656,16 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPA return 0; } +int RPiCameraData::configurePipeline() +{ + config_ = { + .minUnicamBuffers = 2, + .minTotalUnicamBuffers = 4, + }; + + return 0; +} + /* * enumerateVideoDevices() iterates over the Media Controller topology, starting * at the sensor and finishing at Unicam. For each sensor, RPiCameraData stores