| Message ID | 20191120015506.362440-9-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Delegated to: | Niklas Söderlund |
| Headers | show |
| Series |
|
| Related | show |
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@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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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); - 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
Expecting a pipeline handler implementation 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 the hardware. Signed-off-by: Niklas Söderlund <niklas.soderlund@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(-)