| Message ID | 20190409192548.20325-4-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your patch. On 2019-04-09 21:25:39 +0200, Jacopo Mondi wrote: > Pipeline handlers might need to perform allocation of internal buffers, > setup operations, or simple sanity check before going into the > per-stream buffer allocation. > > As of now, PipelineHandler::allocateBuffers() is called once for each > active stream, leaving no space for stream-independent configuration. > > Change this by providing to the pipeline handlers the full set of active > streams, and ask them to loop over them to perform per-streams > allocations. > > While at it, propagate the freeBuffer() error code to the applications > in case of failure. I think you should do this in a separate patch... yea I know you heard it before. I'm going to keep pestering you with comments about this as reviewing patches larger then ~40 lines which do more then one thing is a huge pain for me ;-) > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> I think you can put this patch at the beginning of the series or potentially merge/post it as a separate patch as it solves a design issue not strictly related to the IPU3 code. I would drop 'allocateBuffers:' from the path subject as it's not our convention to annotate using function names, how about libcamera: camera: Pass all active streams in allocateBuffers() With the subject fixed and with or without breaking the patch in two, Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/camera.cpp | 17 +++++++++-------- > src/libcamera/include/pipeline_handler.h | 6 ++++-- > src/libcamera/pipeline/ipu3/ipu3.cpp | 13 +++++++++---- > src/libcamera/pipeline/uvcvideo.cpp | 13 +++++++++---- > src/libcamera/pipeline/vimc.cpp | 13 +++++++++---- > src/libcamera/pipeline_handler.cpp | 11 ++++++----- > 6 files changed, 46 insertions(+), 27 deletions(-) > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index bdf14b31d8ee..cb392ca3b7e7 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -647,13 +647,11 @@ int Camera::allocateBuffers() > return -EINVAL; > } > > - for (Stream *stream : activeStreams_) { > - int ret = pipe_->allocateBuffers(this, stream); > - if (ret) { > - LOG(Camera, Error) << "Failed to allocate buffers"; > - freeBuffers(); > - return ret; > - } > + int ret = pipe_->allocateBuffers(this, activeStreams_); > + if (ret) { > + LOG(Camera, Error) << "Failed to allocate buffers"; > + freeBuffers(); > + return ret; > } > > state_ = CameraPrepared; > @@ -683,9 +681,12 @@ int Camera::freeBuffers() > * by the V4L2 device that has allocated them. > */ > stream->bufferPool().destroyBuffers(); > - pipe_->freeBuffers(this, stream); > } > > + int ret = pipe_->freeBuffers(this, activeStreams_); > + if (ret) > + return ret; > + > state_ = CameraConfigured; > > return 0; > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h > index b6cbd3bae51b..253820e8eaf8 100644 > --- a/src/libcamera/include/pipeline_handler.h > +++ b/src/libcamera/include/pipeline_handler.h > @@ -57,8 +57,10 @@ public: > streamConfiguration(Camera *camera, const std::vector<StreamUsage> &usages) = 0; > virtual int configureStreams(Camera *camera, const CameraConfiguration &config) = 0; > > - virtual int allocateBuffers(Camera *camera, Stream *stream) = 0; > - virtual int freeBuffers(Camera *camera, Stream *stream) = 0; > + virtual int allocateBuffers(Camera *camera, > + const std::set<Stream *> &streams) = 0; > + virtual int freeBuffers(Camera *camera, > + const std::set<Stream *> &streams) = 0; > > virtual int start(Camera *camera) = 0; > virtual void stop(Camera *camera); > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 712e57c5a459..527213a8970a 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -159,8 +159,10 @@ public: > int configureStreams(Camera *camera, > const CameraConfiguration &config) override; > > - int allocateBuffers(Camera *camera, Stream *stream) override; > - int freeBuffers(Camera *camera, Stream *stream) override; > + int allocateBuffers(Camera *camera, > + const std::set<Stream *> &streams) override; > + int freeBuffers(Camera *camera, > + const std::set<Stream *> &streams) override; > > int start(Camera *camera) override; > void stop(Camera *camera) override; > @@ -378,9 +380,11 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera, > return 0; > } > > -int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream) > +int PipelineHandlerIPU3::allocateBuffers(Camera *camera, > + const std::set<Stream *> &streams) > { > IPU3CameraData *data = cameraData(camera); > + Stream *stream = *streams.begin(); > CIO2Device *cio2 = &data->cio2_; > ImgUDevice *imgu = data->imgu_; > int ret; > @@ -419,7 +423,8 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream) > return 0; > } > > -int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream) > +int PipelineHandlerIPU3::freeBuffers(Camera *camera, > + const std::set<Stream *> &streams) > { > IPU3CameraData *data = cameraData(camera); > > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp > index cd472cfadd86..b8f634d88b46 100644 > --- a/src/libcamera/pipeline/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo.cpp > @@ -32,8 +32,10 @@ public: > int configureStreams(Camera *camera, > const CameraConfiguration &config) override; > > - int allocateBuffers(Camera *camera, Stream *stream) override; > - int freeBuffers(Camera *camera, Stream *stream) override; > + int allocateBuffers(Camera *camera, > + const std::set<Stream *> &streams) override; > + int freeBuffers(Camera *camera, > + const std::set<Stream *> &streams) override; > > int start(Camera *camera) override; > void stop(Camera *camera) override; > @@ -127,9 +129,11 @@ int PipelineHandlerUVC::configureStreams(Camera *camera, > return 0; > } > > -int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream) > +int PipelineHandlerUVC::allocateBuffers(Camera *camera, > + const std::set<Stream *> &streams) > { > UVCCameraData *data = cameraData(camera); > + Stream *stream = *streams.begin(); > const StreamConfiguration &cfg = stream->configuration(); > > LOG(UVC, Debug) << "Requesting " << cfg.bufferCount << " buffers"; > @@ -137,7 +141,8 @@ int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream) > return data->video_->exportBuffers(&stream->bufferPool()); > } > > -int PipelineHandlerUVC::freeBuffers(Camera *camera, Stream *stream) > +int PipelineHandlerUVC::freeBuffers(Camera *camera, > + const std::set<Stream *> &streams) > { > UVCCameraData *data = cameraData(camera); > return data->video_->releaseBuffers(); > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp > index c8bbe2a19847..22449e47bc2d 100644 > --- a/src/libcamera/pipeline/vimc.cpp > +++ b/src/libcamera/pipeline/vimc.cpp > @@ -32,8 +32,10 @@ public: > int configureStreams(Camera *camera, > const CameraConfiguration &config) override; > > - int allocateBuffers(Camera *camera, Stream *stream) override; > - int freeBuffers(Camera *camera, Stream *stream) override; > + int allocateBuffers(Camera *camera, > + const std::set<Stream *> &streams) override; > + int freeBuffers(Camera *camera, > + const std::set<Stream *> &streams) override; > > int start(Camera *camera) override; > void stop(Camera *camera) override; > @@ -127,9 +129,11 @@ int PipelineHandlerVimc::configureStreams(Camera *camera, > return 0; > } > > -int PipelineHandlerVimc::allocateBuffers(Camera *camera, Stream *stream) > +int PipelineHandlerVimc::allocateBuffers(Camera *camera, > + const std::set<Stream *> &streams) > { > VimcCameraData *data = cameraData(camera); > + Stream *stream = *streams.begin(); > const StreamConfiguration &cfg = stream->configuration(); > > LOG(VIMC, Debug) << "Requesting " << cfg.bufferCount << " buffers"; > @@ -137,7 +141,8 @@ int PipelineHandlerVimc::allocateBuffers(Camera *camera, Stream *stream) > return data->video_->exportBuffers(&stream->bufferPool()); > } > > -int PipelineHandlerVimc::freeBuffers(Camera *camera, Stream *stream) > +int PipelineHandlerVimc::freeBuffers(Camera *camera, > + const std::set<Stream *> &streams) > { > VimcCameraData *data = cameraData(camera); > return data->video_->releaseBuffers(); > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index 43550c0e0210..911d08448e69 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -193,10 +193,11 @@ PipelineHandler::~PipelineHandler() > * \fn PipelineHandler::allocateBuffers() > * \brief Allocate buffers for a stream > * \param[in] camera The camera the \a stream belongs to > - * \param[in] stream The stream to allocate buffers for > + * \param[in] streams The set of streams to allocate buffers for > * > - * This method allocates buffers internally in the pipeline handler and > - * associates them with the stream's buffer pool. > + * This method allocates buffers internally in the pipeline handler for each > + * stream in the \a streams buffer set, and associates them with the stream's > + * buffer pool. > * > * The intended caller of this method is the Camera class. > * > @@ -207,9 +208,9 @@ PipelineHandler::~PipelineHandler() > * \fn PipelineHandler::freeBuffers() > * \brief Free all buffers associated with a stream > * \param[in] camera The camera the \a stream belongs to > - * \param[in] stream The stream to free buffers from > + * \param[in] streams The set of streams to free buffers from > * > - * After a capture session has been stopped all buffers associated with the > + * After a capture session has been stopped all buffers associated with each > * stream shall be freed. > * > * The intended caller of this method is the Camera class. > -- > 2.21.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hello, On Sun, Apr 14, 2019 at 09:53:58PM +0200, Niklas Söderlund wrote: > On 2019-04-09 21:25:39 +0200, Jacopo Mondi wrote: > > Pipeline handlers might need to perform allocation of internal buffers, > > setup operations, or simple sanity check before going into the > > per-stream buffer allocation. > > > > As of now, PipelineHandler::allocateBuffers() is called once for each > > active stream, leaving no space for stream-independent configuration. > > > > Change this by providing to the pipeline handlers the full set of active > > streams, and ask them to loop over them to perform per-streams > > allocations. > > > > While at it, propagate the freeBuffer() error code to the applications > > in case of failure. > > I think you should do this in a separate patch... yea I know you heard > it before. I'm going to keep pestering you with comments about this as > reviewing patches larger then ~40 lines which do more then one thing is > a huge pain for me ;-) I'm fine with a single patch or two separate patches. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > I think you can put this patch at the beginning of the series or > potentially merge/post it as a separate patch as it solves a design > issue not strictly related to the IPU3 code. > > I would drop 'allocateBuffers:' from the path subject as it's not our > convention to annotate using function names, how about > > libcamera: camera: Pass all active streams in allocateBuffers() I agree with all this. > With the subject fixed and with or without breaking the patch in two, > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > --- > > src/libcamera/camera.cpp | 17 +++++++++-------- > > src/libcamera/include/pipeline_handler.h | 6 ++++-- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 13 +++++++++---- > > src/libcamera/pipeline/uvcvideo.cpp | 13 +++++++++---- > > src/libcamera/pipeline/vimc.cpp | 13 +++++++++---- > > src/libcamera/pipeline_handler.cpp | 11 ++++++----- > > 6 files changed, 46 insertions(+), 27 deletions(-) > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > index bdf14b31d8ee..cb392ca3b7e7 100644 > > --- a/src/libcamera/camera.cpp > > +++ b/src/libcamera/camera.cpp > > @@ -647,13 +647,11 @@ int Camera::allocateBuffers() > > return -EINVAL; > > } > > > > - for (Stream *stream : activeStreams_) { > > - int ret = pipe_->allocateBuffers(this, stream); > > - if (ret) { > > - LOG(Camera, Error) << "Failed to allocate buffers"; > > - freeBuffers(); > > - return ret; > > - } > > + int ret = pipe_->allocateBuffers(this, activeStreams_); > > + if (ret) { > > + LOG(Camera, Error) << "Failed to allocate buffers"; > > + freeBuffers(); This is under discussion so I won't repeat my comment here :-) > > + return ret; > > } > > > > state_ = CameraPrepared; > > @@ -683,9 +681,12 @@ int Camera::freeBuffers() > > * by the V4L2 device that has allocated them. > > */ > > stream->bufferPool().destroyBuffers(); > > - pipe_->freeBuffers(this, stream); > > } > > > > + int ret = pipe_->freeBuffers(this, activeStreams_); > > + if (ret) > > + return ret; > > + > > state_ = CameraConfigured; I wonder if we should set state_ to CameraConfigured even when pipe_->freeBuffers() fails. Otherwise we won't be able to release the camera, and we'll get more errors when closing the application (such as stopping the camera manager with a camera still in use). > > > > return 0; > > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h > > index b6cbd3bae51b..253820e8eaf8 100644 > > --- a/src/libcamera/include/pipeline_handler.h > > +++ b/src/libcamera/include/pipeline_handler.h > > @@ -57,8 +57,10 @@ public: > > streamConfiguration(Camera *camera, const std::vector<StreamUsage> &usages) = 0; > > virtual int configureStreams(Camera *camera, const CameraConfiguration &config) = 0; > > > > - virtual int allocateBuffers(Camera *camera, Stream *stream) = 0; > > - virtual int freeBuffers(Camera *camera, Stream *stream) = 0; > > + virtual int allocateBuffers(Camera *camera, > > + const std::set<Stream *> &streams) = 0; > > + virtual int freeBuffers(Camera *camera, > > + const std::set<Stream *> &streams) = 0; > > > > virtual int start(Camera *camera) = 0; > > virtual void stop(Camera *camera); > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 712e57c5a459..527213a8970a 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -159,8 +159,10 @@ public: > > int configureStreams(Camera *camera, > > const CameraConfiguration &config) override; > > > > - int allocateBuffers(Camera *camera, Stream *stream) override; > > - int freeBuffers(Camera *camera, Stream *stream) override; > > + int allocateBuffers(Camera *camera, > > + const std::set<Stream *> &streams) override; > > + int freeBuffers(Camera *camera, > > + const std::set<Stream *> &streams) override; > > > > int start(Camera *camera) override; > > void stop(Camera *camera) override; > > @@ -378,9 +380,11 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera, > > return 0; > > } > > > > -int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream) > > +int PipelineHandlerIPU3::allocateBuffers(Camera *camera, > > + const std::set<Stream *> &streams) > > { > > IPU3CameraData *data = cameraData(camera); > > + Stream *stream = *streams.begin(); > > CIO2Device *cio2 = &data->cio2_; > > ImgUDevice *imgu = data->imgu_; > > int ret; > > @@ -419,7 +423,8 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream) > > return 0; > > } > > > > -int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream) > > +int PipelineHandlerIPU3::freeBuffers(Camera *camera, > > + const std::set<Stream *> &streams) > > { > > IPU3CameraData *data = cameraData(camera); > > > > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp > > index cd472cfadd86..b8f634d88b46 100644 > > --- a/src/libcamera/pipeline/uvcvideo.cpp > > +++ b/src/libcamera/pipeline/uvcvideo.cpp > > @@ -32,8 +32,10 @@ public: > > int configureStreams(Camera *camera, > > const CameraConfiguration &config) override; > > > > - int allocateBuffers(Camera *camera, Stream *stream) override; > > - int freeBuffers(Camera *camera, Stream *stream) override; > > + int allocateBuffers(Camera *camera, > > + const std::set<Stream *> &streams) override; > > + int freeBuffers(Camera *camera, > > + const std::set<Stream *> &streams) override; > > > > int start(Camera *camera) override; > > void stop(Camera *camera) override; > > @@ -127,9 +129,11 @@ int PipelineHandlerUVC::configureStreams(Camera *camera, > > return 0; > > } > > > > -int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream) > > +int PipelineHandlerUVC::allocateBuffers(Camera *camera, > > + const std::set<Stream *> &streams) > > { > > UVCCameraData *data = cameraData(camera); > > + Stream *stream = *streams.begin(); > > const StreamConfiguration &cfg = stream->configuration(); > > > > LOG(UVC, Debug) << "Requesting " << cfg.bufferCount << " buffers"; > > @@ -137,7 +141,8 @@ int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream) > > return data->video_->exportBuffers(&stream->bufferPool()); > > } > > > > -int PipelineHandlerUVC::freeBuffers(Camera *camera, Stream *stream) > > +int PipelineHandlerUVC::freeBuffers(Camera *camera, > > + const std::set<Stream *> &streams) > > { > > UVCCameraData *data = cameraData(camera); > > return data->video_->releaseBuffers(); > > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp > > index c8bbe2a19847..22449e47bc2d 100644 > > --- a/src/libcamera/pipeline/vimc.cpp > > +++ b/src/libcamera/pipeline/vimc.cpp > > @@ -32,8 +32,10 @@ public: > > int configureStreams(Camera *camera, > > const CameraConfiguration &config) override; > > > > - int allocateBuffers(Camera *camera, Stream *stream) override; > > - int freeBuffers(Camera *camera, Stream *stream) override; > > + int allocateBuffers(Camera *camera, > > + const std::set<Stream *> &streams) override; > > + int freeBuffers(Camera *camera, > > + const std::set<Stream *> &streams) override; > > > > int start(Camera *camera) override; > > void stop(Camera *camera) override; > > @@ -127,9 +129,11 @@ int PipelineHandlerVimc::configureStreams(Camera *camera, > > return 0; > > } > > > > -int PipelineHandlerVimc::allocateBuffers(Camera *camera, Stream *stream) > > +int PipelineHandlerVimc::allocateBuffers(Camera *camera, > > + const std::set<Stream *> &streams) > > { > > VimcCameraData *data = cameraData(camera); > > + Stream *stream = *streams.begin(); > > const StreamConfiguration &cfg = stream->configuration(); > > > > LOG(VIMC, Debug) << "Requesting " << cfg.bufferCount << " buffers"; > > @@ -137,7 +141,8 @@ int PipelineHandlerVimc::allocateBuffers(Camera *camera, Stream *stream) > > return data->video_->exportBuffers(&stream->bufferPool()); > > } > > > > -int PipelineHandlerVimc::freeBuffers(Camera *camera, Stream *stream) > > +int PipelineHandlerVimc::freeBuffers(Camera *camera, > > + const std::set<Stream *> &streams) > > { > > VimcCameraData *data = cameraData(camera); > > return data->video_->releaseBuffers(); > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > index 43550c0e0210..911d08448e69 100644 > > --- a/src/libcamera/pipeline_handler.cpp > > +++ b/src/libcamera/pipeline_handler.cpp > > @@ -193,10 +193,11 @@ PipelineHandler::~PipelineHandler() > > * \fn PipelineHandler::allocateBuffers() > > * \brief Allocate buffers for a stream > > * \param[in] camera The camera the \a stream belongs to > > - * \param[in] stream The stream to allocate buffers for > > + * \param[in] streams The set of streams to allocate buffers for > > * > > - * This method allocates buffers internally in the pipeline handler and > > - * associates them with the stream's buffer pool. > > + * This method allocates buffers internally in the pipeline handler for each > > + * stream in the \a streams buffer set, and associates them with the stream's > > + * buffer pool. > > * > > * The intended caller of this method is the Camera class. > > * > > @@ -207,9 +208,9 @@ PipelineHandler::~PipelineHandler() > > * \fn PipelineHandler::freeBuffers() > > * \brief Free all buffers associated with a stream > > * \param[in] camera The camera the \a stream belongs to > > - * \param[in] stream The stream to free buffers from > > + * \param[in] streams The set of streams to free buffers from s/from/for/ With the small comments addressed, and the freeBuffers() issue solved, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > * > > - * After a capture session has been stopped all buffers associated with the > > + * After a capture session has been stopped all buffers associated with each > > * stream shall be freed. > > * > > * The intended caller of this method is the Camera class.
Hi Laurent, On Mon, Apr 15, 2019 at 02:48:40PM +0300, Laurent Pinchart wrote: > Hello, > > On Sun, Apr 14, 2019 at 09:53:58PM +0200, Niklas Söderlund wrote: > > On 2019-04-09 21:25:39 +0200, Jacopo Mondi wrote: > > > Pipeline handlers might need to perform allocation of internal buffers, > > > setup operations, or simple sanity check before going into the > > > per-stream buffer allocation. > > > > > > As of now, PipelineHandler::allocateBuffers() is called once for each > > > active stream, leaving no space for stream-independent configuration. > > > > > > Change this by providing to the pipeline handlers the full set of active > > > streams, and ask them to loop over them to perform per-streams > > > allocations. > > > > > > While at it, propagate the freeBuffer() error code to the applications > > > in case of failure. > > > > I think you should do this in a separate patch... yea I know you heard > > it before. I'm going to keep pestering you with comments about this as > > reviewing patches larger then ~40 lines which do more then one thing is > > a huge pain for me ;-) > > I'm fine with a single patch or two separate patches. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > I think you can put this patch at the beginning of the series or > > potentially merge/post it as a separate patch as it solves a design > > issue not strictly related to the IPU3 code. > > > > I would drop 'allocateBuffers:' from the path subject as it's not our > > convention to annotate using function names, how about > > > > libcamera: camera: Pass all active streams in allocateBuffers() > > I agree with all this. > > > With the subject fixed and with or without breaking the patch in two, > > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > > --- > > > src/libcamera/camera.cpp | 17 +++++++++-------- > > > src/libcamera/include/pipeline_handler.h | 6 ++++-- > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 13 +++++++++---- > > > src/libcamera/pipeline/uvcvideo.cpp | 13 +++++++++---- > > > src/libcamera/pipeline/vimc.cpp | 13 +++++++++---- > > > src/libcamera/pipeline_handler.cpp | 11 ++++++----- > > > 6 files changed, 46 insertions(+), 27 deletions(-) > > > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > > index bdf14b31d8ee..cb392ca3b7e7 100644 > > > --- a/src/libcamera/camera.cpp > > > +++ b/src/libcamera/camera.cpp > > > @@ -647,13 +647,11 @@ int Camera::allocateBuffers() > > > return -EINVAL; > > > } > > > > > > - for (Stream *stream : activeStreams_) { > > > - int ret = pipe_->allocateBuffers(this, stream); > > > - if (ret) { > > > - LOG(Camera, Error) << "Failed to allocate buffers"; > > > - freeBuffers(); > > > - return ret; > > > - } > > > + int ret = pipe_->allocateBuffers(this, activeStreams_); > > > + if (ret) { > > > + LOG(Camera, Error) << "Failed to allocate buffers"; > > > + freeBuffers(); > > This is under discussion so I won't repeat my comment here :-) > > > > + return ret; > > > } > > > > > > state_ = CameraPrepared; > > > @@ -683,9 +681,12 @@ int Camera::freeBuffers() > > > * by the V4L2 device that has allocated them. > > > */ > > > stream->bufferPool().destroyBuffers(); > > > - pipe_->freeBuffers(this, stream); > > > } > > > > > > + int ret = pipe_->freeBuffers(this, activeStreams_); > > > + if (ret) > > > + return ret; > > > + > > > state_ = CameraConfigured; > > I wonder if we should set state_ to CameraConfigured even when > pipe_->freeBuffers() fails. Otherwise we won't be able to release the > camera, and we'll get more errors when closing the application (such as > stopping the camera manager with a camera still in use). > Good point... I should only propagate the error up but not return eralier then. Thanks. > > > > > > return 0; > > > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h > > > index b6cbd3bae51b..253820e8eaf8 100644 > > > --- a/src/libcamera/include/pipeline_handler.h > > > +++ b/src/libcamera/include/pipeline_handler.h > > > @@ -57,8 +57,10 @@ public: > > > streamConfiguration(Camera *camera, const std::vector<StreamUsage> &usages) = 0; > > > virtual int configureStreams(Camera *camera, const CameraConfiguration &config) = 0; > > > > > > - virtual int allocateBuffers(Camera *camera, Stream *stream) = 0; > > > - virtual int freeBuffers(Camera *camera, Stream *stream) = 0; > > > + virtual int allocateBuffers(Camera *camera, > > > + const std::set<Stream *> &streams) = 0; > > > + virtual int freeBuffers(Camera *camera, > > > + const std::set<Stream *> &streams) = 0; > > > > > > virtual int start(Camera *camera) = 0; > > > virtual void stop(Camera *camera); > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > index 712e57c5a459..527213a8970a 100644 > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > @@ -159,8 +159,10 @@ public: > > > int configureStreams(Camera *camera, > > > const CameraConfiguration &config) override; > > > > > > - int allocateBuffers(Camera *camera, Stream *stream) override; > > > - int freeBuffers(Camera *camera, Stream *stream) override; > > > + int allocateBuffers(Camera *camera, > > > + const std::set<Stream *> &streams) override; > > > + int freeBuffers(Camera *camera, > > > + const std::set<Stream *> &streams) override; > > > > > > int start(Camera *camera) override; > > > void stop(Camera *camera) override; > > > @@ -378,9 +380,11 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera, > > > return 0; > > > } > > > > > > -int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream) > > > +int PipelineHandlerIPU3::allocateBuffers(Camera *camera, > > > + const std::set<Stream *> &streams) > > > { > > > IPU3CameraData *data = cameraData(camera); > > > + Stream *stream = *streams.begin(); > > > CIO2Device *cio2 = &data->cio2_; > > > ImgUDevice *imgu = data->imgu_; > > > int ret; > > > @@ -419,7 +423,8 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream) > > > return 0; > > > } > > > > > > -int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream) > > > +int PipelineHandlerIPU3::freeBuffers(Camera *camera, > > > + const std::set<Stream *> &streams) > > > { > > > IPU3CameraData *data = cameraData(camera); > > > > > > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp > > > index cd472cfadd86..b8f634d88b46 100644 > > > --- a/src/libcamera/pipeline/uvcvideo.cpp > > > +++ b/src/libcamera/pipeline/uvcvideo.cpp > > > @@ -32,8 +32,10 @@ public: > > > int configureStreams(Camera *camera, > > > const CameraConfiguration &config) override; > > > > > > - int allocateBuffers(Camera *camera, Stream *stream) override; > > > - int freeBuffers(Camera *camera, Stream *stream) override; > > > + int allocateBuffers(Camera *camera, > > > + const std::set<Stream *> &streams) override; > > > + int freeBuffers(Camera *camera, > > > + const std::set<Stream *> &streams) override; > > > > > > int start(Camera *camera) override; > > > void stop(Camera *camera) override; > > > @@ -127,9 +129,11 @@ int PipelineHandlerUVC::configureStreams(Camera *camera, > > > return 0; > > > } > > > > > > -int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream) > > > +int PipelineHandlerUVC::allocateBuffers(Camera *camera, > > > + const std::set<Stream *> &streams) > > > { > > > UVCCameraData *data = cameraData(camera); > > > + Stream *stream = *streams.begin(); > > > const StreamConfiguration &cfg = stream->configuration(); > > > > > > LOG(UVC, Debug) << "Requesting " << cfg.bufferCount << " buffers"; > > > @@ -137,7 +141,8 @@ int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream) > > > return data->video_->exportBuffers(&stream->bufferPool()); > > > } > > > > > > -int PipelineHandlerUVC::freeBuffers(Camera *camera, Stream *stream) > > > +int PipelineHandlerUVC::freeBuffers(Camera *camera, > > > + const std::set<Stream *> &streams) > > > { > > > UVCCameraData *data = cameraData(camera); > > > return data->video_->releaseBuffers(); > > > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp > > > index c8bbe2a19847..22449e47bc2d 100644 > > > --- a/src/libcamera/pipeline/vimc.cpp > > > +++ b/src/libcamera/pipeline/vimc.cpp > > > @@ -32,8 +32,10 @@ public: > > > int configureStreams(Camera *camera, > > > const CameraConfiguration &config) override; > > > > > > - int allocateBuffers(Camera *camera, Stream *stream) override; > > > - int freeBuffers(Camera *camera, Stream *stream) override; > > > + int allocateBuffers(Camera *camera, > > > + const std::set<Stream *> &streams) override; > > > + int freeBuffers(Camera *camera, > > > + const std::set<Stream *> &streams) override; > > > > > > int start(Camera *camera) override; > > > void stop(Camera *camera) override; > > > @@ -127,9 +129,11 @@ int PipelineHandlerVimc::configureStreams(Camera *camera, > > > return 0; > > > } > > > > > > -int PipelineHandlerVimc::allocateBuffers(Camera *camera, Stream *stream) > > > +int PipelineHandlerVimc::allocateBuffers(Camera *camera, > > > + const std::set<Stream *> &streams) > > > { > > > VimcCameraData *data = cameraData(camera); > > > + Stream *stream = *streams.begin(); > > > const StreamConfiguration &cfg = stream->configuration(); > > > > > > LOG(VIMC, Debug) << "Requesting " << cfg.bufferCount << " buffers"; > > > @@ -137,7 +141,8 @@ int PipelineHandlerVimc::allocateBuffers(Camera *camera, Stream *stream) > > > return data->video_->exportBuffers(&stream->bufferPool()); > > > } > > > > > > -int PipelineHandlerVimc::freeBuffers(Camera *camera, Stream *stream) > > > +int PipelineHandlerVimc::freeBuffers(Camera *camera, > > > + const std::set<Stream *> &streams) > > > { > > > VimcCameraData *data = cameraData(camera); > > > return data->video_->releaseBuffers(); > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > > index 43550c0e0210..911d08448e69 100644 > > > --- a/src/libcamera/pipeline_handler.cpp > > > +++ b/src/libcamera/pipeline_handler.cpp > > > @@ -193,10 +193,11 @@ PipelineHandler::~PipelineHandler() > > > * \fn PipelineHandler::allocateBuffers() > > > * \brief Allocate buffers for a stream > > > * \param[in] camera The camera the \a stream belongs to > > > - * \param[in] stream The stream to allocate buffers for > > > + * \param[in] streams The set of streams to allocate buffers for > > > * > > > - * This method allocates buffers internally in the pipeline handler and > > > - * associates them with the stream's buffer pool. > > > + * This method allocates buffers internally in the pipeline handler for each > > > + * stream in the \a streams buffer set, and associates them with the stream's > > > + * buffer pool. > > > * > > > * The intended caller of this method is the Camera class. > > > * > > > @@ -207,9 +208,9 @@ PipelineHandler::~PipelineHandler() > > > * \fn PipelineHandler::freeBuffers() > > > * \brief Free all buffers associated with a stream > > > * \param[in] camera The camera the \a stream belongs to > > > - * \param[in] stream The stream to free buffers from > > > + * \param[in] streams The set of streams to free buffers from > > s/from/for/ > > With the small comments addressed, and the freeBuffers() issue solved, > I will send separate patches to: 1) propagate the freeBuffers() error up 2) remove freeBuffers() call in allocateBuffers() error path 3) pass the active stream vector to allocate/free Thanks j > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > * > > > - * After a capture session has been stopped all buffers associated with the > > > + * After a capture session has been stopped all buffers associated with each > > > * stream shall be freed. > > > * > > > * The intended caller of this method is the Camera class. > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index bdf14b31d8ee..cb392ca3b7e7 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -647,13 +647,11 @@ int Camera::allocateBuffers() return -EINVAL; } - for (Stream *stream : activeStreams_) { - int ret = pipe_->allocateBuffers(this, stream); - if (ret) { - LOG(Camera, Error) << "Failed to allocate buffers"; - freeBuffers(); - return ret; - } + int ret = pipe_->allocateBuffers(this, activeStreams_); + if (ret) { + LOG(Camera, Error) << "Failed to allocate buffers"; + freeBuffers(); + return ret; } state_ = CameraPrepared; @@ -683,9 +681,12 @@ int Camera::freeBuffers() * by the V4L2 device that has allocated them. */ stream->bufferPool().destroyBuffers(); - pipe_->freeBuffers(this, stream); } + int ret = pipe_->freeBuffers(this, activeStreams_); + if (ret) + return ret; + state_ = CameraConfigured; return 0; diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h index b6cbd3bae51b..253820e8eaf8 100644 --- a/src/libcamera/include/pipeline_handler.h +++ b/src/libcamera/include/pipeline_handler.h @@ -57,8 +57,10 @@ public: streamConfiguration(Camera *camera, const std::vector<StreamUsage> &usages) = 0; virtual int configureStreams(Camera *camera, const CameraConfiguration &config) = 0; - virtual int allocateBuffers(Camera *camera, Stream *stream) = 0; - virtual int freeBuffers(Camera *camera, Stream *stream) = 0; + virtual int allocateBuffers(Camera *camera, + const std::set<Stream *> &streams) = 0; + virtual int freeBuffers(Camera *camera, + const std::set<Stream *> &streams) = 0; virtual int start(Camera *camera) = 0; virtual void stop(Camera *camera); diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 712e57c5a459..527213a8970a 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -159,8 +159,10 @@ public: int configureStreams(Camera *camera, const CameraConfiguration &config) override; - int allocateBuffers(Camera *camera, Stream *stream) override; - int freeBuffers(Camera *camera, Stream *stream) override; + int allocateBuffers(Camera *camera, + const std::set<Stream *> &streams) override; + int freeBuffers(Camera *camera, + const std::set<Stream *> &streams) override; int start(Camera *camera) override; void stop(Camera *camera) override; @@ -378,9 +380,11 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera, return 0; } -int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream) +int PipelineHandlerIPU3::allocateBuffers(Camera *camera, + const std::set<Stream *> &streams) { IPU3CameraData *data = cameraData(camera); + Stream *stream = *streams.begin(); CIO2Device *cio2 = &data->cio2_; ImgUDevice *imgu = data->imgu_; int ret; @@ -419,7 +423,8 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream) return 0; } -int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream) +int PipelineHandlerIPU3::freeBuffers(Camera *camera, + const std::set<Stream *> &streams) { IPU3CameraData *data = cameraData(camera); diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp index cd472cfadd86..b8f634d88b46 100644 --- a/src/libcamera/pipeline/uvcvideo.cpp +++ b/src/libcamera/pipeline/uvcvideo.cpp @@ -32,8 +32,10 @@ public: int configureStreams(Camera *camera, const CameraConfiguration &config) override; - int allocateBuffers(Camera *camera, Stream *stream) override; - int freeBuffers(Camera *camera, Stream *stream) override; + int allocateBuffers(Camera *camera, + const std::set<Stream *> &streams) override; + int freeBuffers(Camera *camera, + const std::set<Stream *> &streams) override; int start(Camera *camera) override; void stop(Camera *camera) override; @@ -127,9 +129,11 @@ int PipelineHandlerUVC::configureStreams(Camera *camera, return 0; } -int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream) +int PipelineHandlerUVC::allocateBuffers(Camera *camera, + const std::set<Stream *> &streams) { UVCCameraData *data = cameraData(camera); + Stream *stream = *streams.begin(); const StreamConfiguration &cfg = stream->configuration(); LOG(UVC, Debug) << "Requesting " << cfg.bufferCount << " buffers"; @@ -137,7 +141,8 @@ int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream) return data->video_->exportBuffers(&stream->bufferPool()); } -int PipelineHandlerUVC::freeBuffers(Camera *camera, Stream *stream) +int PipelineHandlerUVC::freeBuffers(Camera *camera, + const std::set<Stream *> &streams) { UVCCameraData *data = cameraData(camera); return data->video_->releaseBuffers(); diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp index c8bbe2a19847..22449e47bc2d 100644 --- a/src/libcamera/pipeline/vimc.cpp +++ b/src/libcamera/pipeline/vimc.cpp @@ -32,8 +32,10 @@ public: int configureStreams(Camera *camera, const CameraConfiguration &config) override; - int allocateBuffers(Camera *camera, Stream *stream) override; - int freeBuffers(Camera *camera, Stream *stream) override; + int allocateBuffers(Camera *camera, + const std::set<Stream *> &streams) override; + int freeBuffers(Camera *camera, + const std::set<Stream *> &streams) override; int start(Camera *camera) override; void stop(Camera *camera) override; @@ -127,9 +129,11 @@ int PipelineHandlerVimc::configureStreams(Camera *camera, return 0; } -int PipelineHandlerVimc::allocateBuffers(Camera *camera, Stream *stream) +int PipelineHandlerVimc::allocateBuffers(Camera *camera, + const std::set<Stream *> &streams) { VimcCameraData *data = cameraData(camera); + Stream *stream = *streams.begin(); const StreamConfiguration &cfg = stream->configuration(); LOG(VIMC, Debug) << "Requesting " << cfg.bufferCount << " buffers"; @@ -137,7 +141,8 @@ int PipelineHandlerVimc::allocateBuffers(Camera *camera, Stream *stream) return data->video_->exportBuffers(&stream->bufferPool()); } -int PipelineHandlerVimc::freeBuffers(Camera *camera, Stream *stream) +int PipelineHandlerVimc::freeBuffers(Camera *camera, + const std::set<Stream *> &streams) { VimcCameraData *data = cameraData(camera); return data->video_->releaseBuffers(); diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index 43550c0e0210..911d08448e69 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -193,10 +193,11 @@ PipelineHandler::~PipelineHandler() * \fn PipelineHandler::allocateBuffers() * \brief Allocate buffers for a stream * \param[in] camera The camera the \a stream belongs to - * \param[in] stream The stream to allocate buffers for + * \param[in] streams The set of streams to allocate buffers for * - * This method allocates buffers internally in the pipeline handler and - * associates them with the stream's buffer pool. + * This method allocates buffers internally in the pipeline handler for each + * stream in the \a streams buffer set, and associates them with the stream's + * buffer pool. * * The intended caller of this method is the Camera class. * @@ -207,9 +208,9 @@ PipelineHandler::~PipelineHandler() * \fn PipelineHandler::freeBuffers() * \brief Free all buffers associated with a stream * \param[in] camera The camera the \a stream belongs to - * \param[in] stream The stream to free buffers from + * \param[in] streams The set of streams to free buffers from * - * After a capture session has been stopped all buffers associated with the + * After a capture session has been stopped all buffers associated with each * stream shall be freed. * * The intended caller of this method is the Camera class.
Pipeline handlers might need to perform allocation of internal buffers, setup operations, or simple sanity check before going into the per-stream buffer allocation. As of now, PipelineHandler::allocateBuffers() is called once for each active stream, leaving no space for stream-independent configuration. Change this by providing to the pipeline handlers the full set of active streams, and ask them to loop over them to perform per-streams allocations. While at it, propagate the freeBuffer() error code to the applications in case of failure. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/camera.cpp | 17 +++++++++-------- src/libcamera/include/pipeline_handler.h | 6 ++++-- src/libcamera/pipeline/ipu3/ipu3.cpp | 13 +++++++++---- src/libcamera/pipeline/uvcvideo.cpp | 13 +++++++++---- src/libcamera/pipeline/vimc.cpp | 13 +++++++++---- src/libcamera/pipeline_handler.cpp | 11 ++++++----- 6 files changed, 46 insertions(+), 27 deletions(-)