[libcamera-devel] [PATCH v2 08/10] libcamera: pipeline: Rework PipelineHandler::queueRequest()
Jacopo Mondi
jacopo at jmondi.org
Thu Nov 21 13:40:36 CET 2019
Hi Niklas,
On Wed, Nov 20, 2019 at 02:55:04AM +0100, Niklas Söderlund wrote:
> Expecting a pipeline handler implementation of queueRequest() to call
> the base class queueRequest() at the correct point have lead to
s/have/has
> 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 the 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 241af58beec0eb13..ad7d08e681d0f28e 100644
> --- a/src/libcamera/include/pipeline_handler.h
> +++ b/src/libcamera/include/pipeline_handler.h
> @@ -78,7 +78,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);
>
> bool completeBuffer(Camera *camera, Request *request, Buffer *buffer);
> void completeRequest(Camera *camera, Request *request);
> @@ -90,6 +90,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 b21cf92435e7bbc9..f5d19dc047aa5a31 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 45448d6f8c05ddf5..76c2d377fb160ce4 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 e6ab6a0858247549..13f5a3aca697f566 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.
> *
> * \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);
Before this patch, the Request was added to the queuedRequests_ vector
-after- the pipeline handler specific implementation was called.
Here you're inverting the behaviour, maybe it's intended.
Otherwise couldn't you simply call queueRequestHardware() and append
the Request -after- it successfully returns, to maintain the original
behaviour ?
>
> - 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
request or Request ?
> + *
> + * This method queues a capture request to the hardware for processing. The
we generally use 'operation' not 'method'
Although, in all this class 'method' is used
confused...
> + * request contains a set of buffers associated with streams and a set of
Here I would use 'Request' and 'Stream instances', although I just
noticed this comment was already in this form before this patch.
Thanks
j
> + * 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
> --
> 2.24.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20191121/16c9b88f/attachment.sig>
More information about the libcamera-devel
mailing list