[libcamera-devel] [PATCH v4 03/12] libcamera: camera: allocateBuffers: Pass the stream set
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Apr 15 13:48:40 CEST 2019
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 at 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 at 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 at 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
More information about the libcamera-devel
mailing list