[libcamera-devel] [PATCH 01/30] libcamera: pipelines: Align bookkeeping in queueRequest()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Dec 10 20:41:22 CET 2019
Hi Niklas,
On Tue, Dec 10, 2019 at 08:31:24PM +0100, Niklas Söderlund wrote:
> On 2019-12-09 15:04:06 +0200, Laurent Pinchart wrote:
> > On Wed, Nov 27, 2019 at 12:35:51AM +0100, Niklas Söderlund wrote:
> > > Expecting pipeline handler implementations of queueRequest() to call
> > > the base class queueRequest() at the correct point have lead to
> >
> > s/lead/led/
> >
> > > different behaviors between the pipelines.
> > >
> > > Fix this by splitting queueRequest() into a base class implementation
> > > which handles the bookkeeping and a new queueRequestHardware() that is
> > > to be implemented by pipeline handlers and only deals with committing the
> > > request to hardware.
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > ---
> > > src/libcamera/include/pipeline_handler.h | 4 ++-
> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 ++--
> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 ++---
> > > src/libcamera/pipeline/uvcvideo.cpp | 6 ++--
> > > src/libcamera/pipeline/vimc.cpp | 6 ++--
> > > src/libcamera/pipeline_handler.cpp | 35 +++++++++++++++++-------
> > > 6 files changed, 37 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> > > index a02e6e77dc9ce0e7..6ce8ea1c8d31b870 100644
> > > --- a/src/libcamera/include/pipeline_handler.h
> > > +++ b/src/libcamera/include/pipeline_handler.h
> > > @@ -77,7 +77,7 @@ public:
> > > virtual int start(Camera *camera) = 0;
> > > virtual void stop(Camera *camera) = 0;
> > >
> > > - virtual int queueRequest(Camera *camera, Request *request);
> > > + int queueRequest(Camera *camera, Request *request);
> >
> > I would make this method final.
>
> To mark this method as final I need to first make it virtual.
>
> virtual int queueRequest(Camera *camera, Request *request) final;
>
> Looks a but suspicious to me and we have a lot of other methods which
> should not be overloaded by pipeline implementations. I have keep this
> as is for now. I'm holding of adding your tag tho until you had a chance
> to provide your feedback to this.
My bad, you're right.
> > >
> > > bool completeBuffer(Camera *camera, Request *request, Buffer *buffer);
> > > void completeRequest(Camera *camera, Request *request);
> > > @@ -89,6 +89,8 @@ protected:
> > > std::unique_ptr<CameraData> data);
> > > void hotplugMediaDevice(MediaDevice *media);
> > >
> > > + virtual int queueRequestHardware(Camera *camera, Request *request) = 0;
> > > +
> > > CameraData *cameraData(const Camera *camera);
> > >
> > > CameraManager *manager_;
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index 1c5fccf694289ae9..ad223d9101bdc6ed 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -220,7 +220,7 @@ public:
> > > int start(Camera *camera) override;
> > > void stop(Camera *camera) override;
> > >
> > > - int queueRequest(Camera *camera, Request *request) override;
> > > + int queueRequestHardware(Camera *camera, Request *request) override;
> > >
> > > bool match(DeviceEnumerator *enumerator) override;
> > >
> > > @@ -756,7 +756,7 @@ void PipelineHandlerIPU3::stop(Camera *camera)
> > > data->rawBuffers_.clear();
> > > }
> > >
> > > -int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)
> > > +int PipelineHandlerIPU3::queueRequestHardware(Camera *camera, Request *request)
> > > {
> > > int error = 0;
> > >
> > > @@ -769,8 +769,6 @@ int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)
> > > error = ret;
> > > }
> > >
> > > - PipelineHandler::queueRequest(camera, request);
> > > -
> > > return error;
> > > }
> > >
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index 4a583a7a1d7ef1fd..f75b25c6787e40df 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -184,7 +184,7 @@ public:
> > > int start(Camera *camera) override;
> > > void stop(Camera *camera) override;
> > >
> > > - int queueRequest(Camera *camera, Request *request) override;
> > > + int queueRequestHardware(Camera *camera, Request *request) override;
> > >
> > > bool match(DeviceEnumerator *enumerator) override;
> > >
> > > @@ -810,13 +810,12 @@ void PipelineHandlerRkISP1::stop(Camera *camera)
> > > activeCamera_ = nullptr;
> > > }
> > >
> > > -int PipelineHandlerRkISP1::queueRequest(Camera *camera, Request *request)
> > > +int PipelineHandlerRkISP1::queueRequestHardware(Camera *camera,
> > > + Request *request)
> > > {
> > > RkISP1CameraData *data = cameraData(camera);
> > > Stream *stream = &data->stream_;
> > >
> > > - PipelineHandler::queueRequest(camera, request);
> > > -
> > > RkISP1FrameInfo *info = data->frameInfo_.create(data->frame_, request,
> > > stream);
> > > if (!info)
> > > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> > > index 522229241ffb9e8a..3a76653ff6dc2b5e 100644
> > > --- a/src/libcamera/pipeline/uvcvideo.cpp
> > > +++ b/src/libcamera/pipeline/uvcvideo.cpp
> > > @@ -72,7 +72,7 @@ public:
> > > int start(Camera *camera) override;
> > > void stop(Camera *camera) override;
> > >
> > > - int queueRequest(Camera *camera, Request *request) override;
> > > + int queueRequestHardware(Camera *camera, Request *request) override;
> > >
> > > bool match(DeviceEnumerator *enumerator) override;
> > >
> > > @@ -262,7 +262,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
> > > return ret;
> > > }
> > >
> > > -int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)
> > > +int PipelineHandlerUVC::queueRequestHardware(Camera *camera, Request *request)
> > > {
> > > UVCCameraData *data = cameraData(camera);
> > > Buffer *buffer = request->findBuffer(&data->stream_);
> > > @@ -281,8 +281,6 @@ int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)
> > > if (ret < 0)
> > > return ret;
> > >
> > > - PipelineHandler::queueRequest(camera, request);
> > > -
> > > return 0;
> > > }
> > >
> > > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > > index 06664fed42e796e4..f5550a1723668106 100644
> > > --- a/src/libcamera/pipeline/vimc.cpp
> > > +++ b/src/libcamera/pipeline/vimc.cpp
> > > @@ -90,7 +90,7 @@ public:
> > > int start(Camera *camera) override;
> > > void stop(Camera *camera) override;
> > >
> > > - int queueRequest(Camera *camera, Request *request) override;
> > > + int queueRequestHardware(Camera *camera, Request *request) override;
> > >
> > > bool match(DeviceEnumerator *enumerator) override;
> > >
> > > @@ -323,7 +323,7 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)
> > > return ret;
> > > }
> > >
> > > -int PipelineHandlerVimc::queueRequest(Camera *camera, Request *request)
> > > +int PipelineHandlerVimc::queueRequestHardware(Camera *camera, Request *request)
> > > {
> > > VimcCameraData *data = cameraData(camera);
> > > Buffer *buffer = request->findBuffer(&data->stream_);
> > > @@ -342,8 +342,6 @@ int PipelineHandlerVimc::queueRequest(Camera *camera, Request *request)
> > > if (ret < 0)
> > > return ret;
> > >
> > > - PipelineHandler::queueRequest(camera, request);
> > > -
> > > return 0;
> > > }
> > >
> > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > > index 4349ca8957e403fe..c9e348b98da7b736 100644
> > > --- a/src/libcamera/pipeline_handler.cpp
> > > +++ b/src/libcamera/pipeline_handler.cpp
> > > @@ -345,27 +345,42 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera)
> > > * \param[in] request The request to queue
> > > *
> > > * This method queues a capture request to the pipeline handler for processing.
> > > - * The request contains a set of buffers associated with streams and a set of
> > > - * parameters. The pipeline handler shall program the device to ensure that the
> > > - * parameters will be applied to the frames captured in the buffers provided in
> > > - * the request.
> > > - *
> > > - * Pipeline handlers shall override this method. The base implementation in the
> > > - * PipelineHandler class keeps track of queued requests in order to ensure
> > > - * completion of all requests when the pipeline handler is stopped with stop().
> > > - * Requests completion shall be signalled by the pipeline handler using the
> > > + * The method keeps track of queued requests in order to ensure completion of
> > > + * all requests when the pipeline handler is stopped with stop(). Requests
> > > + * completion shall be signalled by the pipeline handler using the
> > > * completeRequest() method.
> >
> > I would mention queueRequestHardware() here:
> >
> > * This method queues a capture request to the pipeline handler for processing.
> > * The request is first added to the internal list of queued requests, and
> > * then passed to the pipeline handler with a call to queueRequestHardware().
> > *
> > * Keeping track of queued requests ensures automatic completion of all requests
> > * when the pipeline handler is stopped with stop(). Request completion shall be
> > * signalled by the pipeline handler using the completeRequest() method.
> >
> > It took me a while to write this paragraph, which is I think a sign that
> > the API isn't very clean, in particular that the queueRequest() and
> > queueRequestHardware() names are confusing (the latter especially
> > because it may queue the request to the pipeline handler first, not
> > immediately to the hardware). I think I would prefer
> > queueRequestDevice() to queueRequestHardware() (with s/hardware/device/
> > where applicable), but even that isn't perfect. I don't have better
> > names to propose at this moment, so
>
> Tanks! I went with queueRequestDevice() as I too like it more.
>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > > *
> > > * \return 0 on success or a negative error code otherwise
> > > */
> > > int PipelineHandler::queueRequest(Camera *camera, Request *request)
> > > {
> > > + int ret;
> > > +
> > > CameraData *data = cameraData(camera);
> > > data->queuedRequests_.push_back(request);
> > >
> > > - return 0;
> > > + ret = queueRequestHardware(camera, request);
> > > + if (ret)
> > > + data->queuedRequests_.remove(request);
> > > +
> > > + return ret;
> > > }
> > >
> > > +/**
> > > + * \fn PipelineHandler::queueRequestHardware()
> > > + * \brief Queue a request to the hardware
> > > + * \param[in] camera The camera to queue the request to
> > > + * \param[in] request The request to queue
> > > + *
> > > + * This method queues a capture request to the hardware for processing. The
> > > + * request contains a set of buffers associated with streams and a set of
> > > + * parameters. The pipeline handler shall program the device to ensure that the
> > > + * parameters will be applied to the frames captured in the buffers provided in
> > > + * the request.
> > > + *
> > > + * \return 0 on success or a negative error code otherwise
> > > + */
> > > +
> > > /**
> > > * \brief Complete a buffer for a request
> > > * \param[in] camera The camera the request belongs to
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list