| Message ID | 20220307124633.115452-5-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush Thanks for this patch! On Mon, 7 Mar 2022 at 12:46, Naushir Patuck via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote: > > Add a flag to indicate a call to PipelineHandlerRPi::configure() has taken > place. This flag signals that buffer allocations need to be done on the next > call to PipelineHandlerRPi::start(). > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 21 ++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 8bb9fc429912..b17ffa235ac7 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -185,7 +185,7 @@ public: > RPiCameraData(PipelineHandler *pipe) > : Camera::Private(pipe), state_(State::Stopped), > supportsFlips_(false), flipsAlterBayerOrder_(false), > - dropFrameCount_(0), ispOutputCount_(0) > + dropFrameCount_(0), reconfigured_(true), ispOutputCount_(0) > { > } > > @@ -284,6 +284,9 @@ public: > */ > std::optional<int32_t> notifyGainsUnity_; > > + /* Has this camera been reconfigured? */ > + bool reconfigured_; > + > private: > void checkRequestCompleted(); > void fillRequestMetadata(const ControlList &bufferControls, > @@ -961,6 +964,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > << " on pad " << sinkPad->index(); > } > > + data->reconfigured_ = true; > return ret; > } > > @@ -981,12 +985,15 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls) > RPiCameraData *data = cameraData(camera); > int ret; > > - /* Allocate buffers for internal pipeline usage. */ > - ret = prepareBuffers(camera); > - if (ret) { > - LOG(RPI, Error) << "Failed to allocate buffers"; > - stop(camera); > - return ret; > + if (data->reconfigured_) { > + /* Allocate buffers for internal pipeline usage. */ > + ret = prepareBuffers(camera); > + if (ret) { > + LOG(RPI, Error) << "Failed to allocate buffers"; > + stop(camera); > + return ret; > + } > + data->reconfigured_ = false; > } > > /* Check if a ScalerCrop control was specified. */ > -- > 2.25.1 > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Tested-by: David Plowman <david.plowman@raspberrypi.com> Thanks! David
Hi Naush, Thank you for the patch. On Mon, Mar 07, 2022 at 12:46:31PM +0000, Naushir Patuck via libcamera-devel wrote: > Add a flag to indicate a call to PipelineHandlerRPi::configure() has taken > place. This flag signals that buffer allocations need to be done on the next > call to PipelineHandlerRPi::start(). > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 21 ++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 8bb9fc429912..b17ffa235ac7 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -185,7 +185,7 @@ public: > RPiCameraData(PipelineHandler *pipe) > : Camera::Private(pipe), state_(State::Stopped), > supportsFlips_(false), flipsAlterBayerOrder_(false), > - dropFrameCount_(0), ispOutputCount_(0) > + dropFrameCount_(0), reconfigured_(true), ispOutputCount_(0) Given that you can't start a camera without configuring it first, this could be initialized to false. > { > } > > @@ -284,6 +284,9 @@ public: > */ > std::optional<int32_t> notifyGainsUnity_; > > + /* Has this camera been reconfigured? */ > + bool reconfigured_; > + > private: > void checkRequestCompleted(); > void fillRequestMetadata(const ControlList &bufferControls, > @@ -961,6 +964,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > << " on pad " << sinkPad->index(); > } > > + data->reconfigured_ = true; > return ret; > } > > @@ -981,12 +985,15 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls) > RPiCameraData *data = cameraData(camera); > int ret; > > - /* Allocate buffers for internal pipeline usage. */ > - ret = prepareBuffers(camera); > - if (ret) { > - LOG(RPI, Error) << "Failed to allocate buffers"; > - stop(camera); > - return ret; > + if (data->reconfigured_) { > + /* Allocate buffers for internal pipeline usage. */ > + ret = prepareBuffers(camera); > + if (ret) { > + LOG(RPI, Error) << "Failed to allocate buffers"; > + stop(camera); > + return ret; > + } > + data->reconfigured_ = false; > } > > /* Check if a ScalerCrop control was specified. */
Hi Laurent, Thank you for your feedback! On Sun, 13 Mar 2022 at 16:04, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > Thank you for the patch. > > On Mon, Mar 07, 2022 at 12:46:31PM +0000, Naushir Patuck via > libcamera-devel wrote: > > Add a flag to indicate a call to PipelineHandlerRPi::configure() has > taken > > place. This flag signals that buffer allocations need to be done on the > next > > call to PipelineHandlerRPi::start(). > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > .../pipeline/raspberrypi/raspberrypi.cpp | 21 ++++++++++++------- > > 1 file changed, 14 insertions(+), 7 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 8bb9fc429912..b17ffa235ac7 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -185,7 +185,7 @@ public: > > RPiCameraData(PipelineHandler *pipe) > > : Camera::Private(pipe), state_(State::Stopped), > > supportsFlips_(false), flipsAlterBayerOrder_(false), > > - dropFrameCount_(0), ispOutputCount_(0) > > + dropFrameCount_(0), reconfigured_(true), > ispOutputCount_(0) > > Given that you can't start a camera without configuring it first, this > could be initialized to false. > I think the value of the flag here is correct, but perhaps my naming of the variable needs to change. This flags tells the pipeline handler that we need to allocate buffers before starting. So perhaps a rename to something like allocateBuffers_ might be more appropriate. I'll change it in the next version. Regards, Naush > > > { > > } > > > > @@ -284,6 +284,9 @@ public: > > */ > > std::optional<int32_t> notifyGainsUnity_; > > > > + /* Has this camera been reconfigured? */ > > + bool reconfigured_; > > + > > private: > > void checkRequestCompleted(); > > void fillRequestMetadata(const ControlList &bufferControls, > > @@ -961,6 +964,7 @@ int PipelineHandlerRPi::configure(Camera *camera, > CameraConfiguration *config) > > << " on pad " << sinkPad->index(); > > } > > > > + data->reconfigured_ = true; > > return ret; > > } > > > > @@ -981,12 +985,15 @@ int PipelineHandlerRPi::start(Camera *camera, > const ControlList *controls) > > RPiCameraData *data = cameraData(camera); > > int ret; > > > > - /* Allocate buffers for internal pipeline usage. */ > > - ret = prepareBuffers(camera); > > - if (ret) { > > - LOG(RPI, Error) << "Failed to allocate buffers"; > > - stop(camera); > > - return ret; > > + if (data->reconfigured_) { > > + /* Allocate buffers for internal pipeline usage. */ > > + ret = prepareBuffers(camera); > > + if (ret) { > > + LOG(RPI, Error) << "Failed to allocate buffers"; > > + stop(camera); > > + return ret; > > + } > > + data->reconfigured_ = false; > > } > > > > /* Check if a ScalerCrop control was specified. */ > > -- > Regards, > > Laurent Pinchart >
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 8bb9fc429912..b17ffa235ac7 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -185,7 +185,7 @@ public: RPiCameraData(PipelineHandler *pipe) : Camera::Private(pipe), state_(State::Stopped), supportsFlips_(false), flipsAlterBayerOrder_(false), - dropFrameCount_(0), ispOutputCount_(0) + dropFrameCount_(0), reconfigured_(true), ispOutputCount_(0) { } @@ -284,6 +284,9 @@ public: */ std::optional<int32_t> notifyGainsUnity_; + /* Has this camera been reconfigured? */ + bool reconfigured_; + private: void checkRequestCompleted(); void fillRequestMetadata(const ControlList &bufferControls, @@ -961,6 +964,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) << " on pad " << sinkPad->index(); } + data->reconfigured_ = true; return ret; } @@ -981,12 +985,15 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls) RPiCameraData *data = cameraData(camera); int ret; - /* Allocate buffers for internal pipeline usage. */ - ret = prepareBuffers(camera); - if (ret) { - LOG(RPI, Error) << "Failed to allocate buffers"; - stop(camera); - return ret; + if (data->reconfigured_) { + /* Allocate buffers for internal pipeline usage. */ + ret = prepareBuffers(camera); + if (ret) { + LOG(RPI, Error) << "Failed to allocate buffers"; + stop(camera); + return ret; + } + data->reconfigured_ = false; } /* Check if a ScalerCrop control was specified. */
Add a flag to indicate a call to PipelineHandlerRPi::configure() has taken place. This flag signals that buffer allocations need to be done on the next call to PipelineHandlerRPi::start(). Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- .../pipeline/raspberrypi/raspberrypi.cpp | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)