[libcamera-devel] [PATCH 01/30] libcamera: pipelines: Align bookkeeping in queueRequest()
Niklas Söderlund
niklas.soderlund at ragnatech.se
Tue Dec 10 20:31:24 CET 2019
Hi Laurent,
Thanks for your feedback.
On 2019-12-09 15:04:06 +0200, Laurent Pinchart wrote:
> Hi Niklas,
>
> Thank you for the patch.
>
> 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.
>
> >
> > 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
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list