[libcamera-devel] [PATCH 01/30] libcamera: pipelines: Align bookkeeping in queueRequest()
Jacopo Mondi
jacopo at jmondi.org
Wed Nov 27 10:43:54 CET 2019
Hi Niklas,
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
> 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.
>
Just one minor nit..
> 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);
>
> 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
You are repeating "This method" and "The method" in two consecutive
lines.
I would just s/The method/It.
> + * 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);
>
> - return 0;
> + ret = queueRequestHardware(camera, request);
You could declare ret here.
With these two very minor issues addressed or not:
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
Thanks
j
> + 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
> --
> 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/20191127/f75088b5/attachment.sig>
More information about the libcamera-devel
mailing list