| Message ID | 20190403150735.27580-8-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your work. On 2019-04-03 17:07:34 +0200, Jacopo Mondi wrote: > Add a method to CameraData base class to retrieve a pointer to the > Request that contains a given buffer. Intended users are buffer > completion slots that needs to associate a Request to a just completed > Buffer. > > In preparation to support multiple requests from different streams, > update all the pipeline handler implementations to use this method > instead of using the last queued request. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > include/libcamera/request.h | 2 ++ > src/libcamera/include/pipeline_handler.h | 3 +++ > src/libcamera/pipeline/ipu3/ipu3.cpp | 3 ++- > src/libcamera/pipeline/uvcvideo.cpp | 3 ++- > src/libcamera/pipeline/vimc.cpp | 3 ++- > src/libcamera/pipeline_handler.cpp | 29 ++++++++++++++++++++++++ > src/libcamera/request.cpp | 7 ++++++ > 7 files changed, 47 insertions(+), 3 deletions(-) > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > index 5ac4d20d1d9f..8f5892fd3111 100644 > --- a/include/libcamera/request.h > +++ b/include/libcamera/request.h > @@ -38,6 +38,8 @@ public: > > const std::list<Stream *> streams() const; > > + const std::unordered_set<Buffer *> &pending() const { return pending_; } > + > Status status() const { return status_; } > > private: > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h > index 920b57609470..6cdadcbdc3ea 100644 > --- a/src/libcamera/include/pipeline_handler.h > +++ b/src/libcamera/include/pipeline_handler.h > @@ -39,6 +39,9 @@ public: > PipelineHandler *pipe_; > std::list<Request *> queuedRequests_; > > +protected: > + Request *requestFromBuffer(Buffer *buffer); > + > private: > CameraData(const CameraData &) = delete; > CameraData &operator=(const CameraData &) = delete; > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 8c67cf985d1e..17e3e8677e28 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -801,7 +801,8 @@ void PipelineHandlerIPU3::IPU3CameraData::imguInputBufferReady(Buffer *buffer) > */ > void PipelineHandlerIPU3::IPU3CameraData::imguOutputBufferReady(Buffer *buffer) > { > - Request *request = queuedRequests_.front(); > + Request *request = requestFromBuffer(buffer); > + ASSERT(request); > > pipe_->completeBuffer(camera_, request, buffer); > pipe_->completeRequest(camera_, request); > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp > index 128f0c49dba3..d571b8b4ea83 100644 > --- a/src/libcamera/pipeline/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo.cpp > @@ -226,7 +226,8 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator) > > void PipelineHandlerUVC::UVCCameraData::bufferReady(Buffer *buffer) > { > - Request *request = queuedRequests_.front(); > + Request *request = requestFromBuffer(buffer); This make me feel uneasy :-( We talked in the past about how to handle request completion. The idea from the beginning was conceived with the reequest API in mind, which IIRC guarantees that requests are completed in the same order they are queued. While V4L2 have no such guarantees for for buffers. This was one of the issues I hit with my single stream old UVC camera where the first buffer queue would contains errors and the uvcvideo driver would not return it to user-space and move on to the second buffer queued and libcamera fail as buffers are dequeued out of order (from it's point of view). I understand what you try to do in this patch and I agree it might be needed. But I fear we first need to discuss how we should handle V4L2 buffers being dequeued out of order. As this change would hide this behavior while it's intent is indeed something different. > + ASSERT(request); > > pipe_->completeBuffer(camera_, request, buffer); > pipe_->completeRequest(camera_, request); > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp > index 6735940799d8..e83416effad8 100644 > --- a/src/libcamera/pipeline/vimc.cpp > +++ b/src/libcamera/pipeline/vimc.cpp > @@ -223,7 +223,8 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator) > > void PipelineHandlerVimc::VimcCameraData::bufferReady(Buffer *buffer) > { > - Request *request = queuedRequests_.front(); > + Request *request = requestFromBuffer(buffer); > + ASSERT(request); > > pipe_->completeBuffer(camera_, request, buffer); > pipe_->completeRequest(camera_, request); > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index 9a8a4fde57e6..830ff354ed3e 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -86,6 +86,35 @@ LOG_DEFINE_CATEGORY(Pipeline) > * PipelineHandler::completeRequest() > */ > > +/** > + * \brief Retrieve the pending request that contains \a buffer > + * \param[in] buffer The buffer contained in the returned request > + * > + * Return the request that contains \a buffer, or nullptr if no such request > + * is found. The intended callers of this method are buffer completion slots > + * implemented in CameraData sub-classes which needs to associated a request > + * to the just completed buffer. It is up to the caller of this function to > + * deal with the case the buffer does not belong to any previously queued > + * request or the request has already completed, possibly because of a > + * duplicated buffer completion notification. This is generally considered > + * a fatal error, and callers are expected to assert the validity of the > + * returned request. > + * > + * \return A pointer to the pending Request that contains the Buffer \a buffer, > + * or nullptr if no such request is found > + */ > +Request *CameraData::requestFromBuffer(Buffer *buffer) > +{ > + for (Request *req : queuedRequests_) { > + for (Buffer *b : req->pending()) { > + if (b == buffer) > + return req; > + } > + } > + > + return nullptr; > +} > + > /** > * \class PipelineHandler > * \brief Create and manage cameras based on a set of media devices > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > index 3a7841fb2bb3..c555752b2c0b 100644 > --- a/src/libcamera/request.cpp > +++ b/src/libcamera/request.cpp > @@ -108,6 +108,13 @@ const std::list<Stream *> Request::streams() const > return streams; > } > > +/** > + * \fn Request::pending() > + * \brief Retrieve the list of not-yet-completed buffers > + * > + * \return The set of pending buffers > + */ > + > /** > * \fn Request::status() > * \brief Retrieve the request completion status > -- > 2.21.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi again, On 2019-04-03 17:07:34 +0200, Jacopo Mondi wrote: > Add a method to CameraData base class to retrieve a pointer to the > Request that contains a given buffer. Intended users are buffer > completion slots that needs to associate a Request to a just completed > Buffer. > > In preparation to support multiple requests from different streams, > update all the pipeline handler implementations to use this method > instead of using the last queued request. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > include/libcamera/request.h | 2 ++ > src/libcamera/include/pipeline_handler.h | 3 +++ > src/libcamera/pipeline/ipu3/ipu3.cpp | 3 ++- > src/libcamera/pipeline/uvcvideo.cpp | 3 ++- > src/libcamera/pipeline/vimc.cpp | 3 ++- > src/libcamera/pipeline_handler.cpp | 29 ++++++++++++++++++++++++ > src/libcamera/request.cpp | 7 ++++++ > 7 files changed, 47 insertions(+), 3 deletions(-) > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > index 5ac4d20d1d9f..8f5892fd3111 100644 > --- a/include/libcamera/request.h > +++ b/include/libcamera/request.h > @@ -38,6 +38,8 @@ public: > > const std::list<Stream *> streams() const; > > + const std::unordered_set<Buffer *> &pending() const { return pending_; } > + You could break adding of this function out to a separate patch or merge it with the one adding streams() earlier in the series. > Status status() const { return status_; } > > private: > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h > index 920b57609470..6cdadcbdc3ea 100644 > --- a/src/libcamera/include/pipeline_handler.h > +++ b/src/libcamera/include/pipeline_handler.h > @@ -39,6 +39,9 @@ public: > PipelineHandler *pipe_; > std::list<Request *> queuedRequests_; > > +protected: > + Request *requestFromBuffer(Buffer *buffer); > + > private: > CameraData(const CameraData &) = delete; > CameraData &operator=(const CameraData &) = delete; > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 8c67cf985d1e..17e3e8677e28 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -801,7 +801,8 @@ void PipelineHandlerIPU3::IPU3CameraData::imguInputBufferReady(Buffer *buffer) > */ > void PipelineHandlerIPU3::IPU3CameraData::imguOutputBufferReady(Buffer *buffer) > { > - Request *request = queuedRequests_.front(); > + Request *request = requestFromBuffer(buffer); > + ASSERT(request); > > pipe_->completeBuffer(camera_, request, buffer); > pipe_->completeRequest(camera_, request); > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp > index 128f0c49dba3..d571b8b4ea83 100644 > --- a/src/libcamera/pipeline/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo.cpp > @@ -226,7 +226,8 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator) > > void PipelineHandlerUVC::UVCCameraData::bufferReady(Buffer *buffer) > { > - Request *request = queuedRequests_.front(); > + Request *request = requestFromBuffer(buffer); > + ASSERT(request); > > pipe_->completeBuffer(camera_, request, buffer); > pipe_->completeRequest(camera_, request); > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp > index 6735940799d8..e83416effad8 100644 > --- a/src/libcamera/pipeline/vimc.cpp > +++ b/src/libcamera/pipeline/vimc.cpp > @@ -223,7 +223,8 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator) > > void PipelineHandlerVimc::VimcCameraData::bufferReady(Buffer *buffer) > { > - Request *request = queuedRequests_.front(); > + Request *request = requestFromBuffer(buffer); > + ASSERT(request); > > pipe_->completeBuffer(camera_, request, buffer); > pipe_->completeRequest(camera_, request); > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index 9a8a4fde57e6..830ff354ed3e 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -86,6 +86,35 @@ LOG_DEFINE_CATEGORY(Pipeline) > * PipelineHandler::completeRequest() > */ > > +/** > + * \brief Retrieve the pending request that contains \a buffer > + * \param[in] buffer The buffer contained in the returned request > + * > + * Return the request that contains \a buffer, or nullptr if no such request > + * is found. The intended callers of this method are buffer completion slots > + * implemented in CameraData sub-classes which needs to associated a request > + * to the just completed buffer. It is up to the caller of this function to > + * deal with the case the buffer does not belong to any previously queued > + * request or the request has already completed, possibly because of a > + * duplicated buffer completion notification. This is generally considered > + * a fatal error, and callers are expected to assert the validity of the > + * returned request. > + * > + * \return A pointer to the pending Request that contains the Buffer \a buffer, > + * or nullptr if no such request is found > + */ > +Request *CameraData::requestFromBuffer(Buffer *buffer) > +{ > + for (Request *req : queuedRequests_) { > + for (Buffer *b : req->pending()) { > + if (b == buffer) > + return req; > + } > + } > + > + return nullptr; > +} > + > /** > * \class PipelineHandler > * \brief Create and manage cameras based on a set of media devices > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > index 3a7841fb2bb3..c555752b2c0b 100644 > --- a/src/libcamera/request.cpp > +++ b/src/libcamera/request.cpp > @@ -108,6 +108,13 @@ const std::list<Stream *> Request::streams() const > return streams; > } > > +/** > + * \fn Request::pending() > + * \brief Retrieve the list of not-yet-completed buffers > + * > + * \return The set of pending buffers > + */ > + > /** > * \fn Request::status() > * \brief Retrieve the request completion status > -- > 2.21.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hello Jacopo, Thank you for the patch. On Fri, Apr 05, 2019 at 01:48:35PM +0200, Niklas Söderlund wrote: > On 2019-04-03 17:07:34 +0200, Jacopo Mondi wrote: > > Add a method to CameraData base class to retrieve a pointer to the > > Request that contains a given buffer. Intended users are buffer > > completion slots that needs to associate a Request to a just completed > > Buffer. > > > > In preparation to support multiple requests from different streams, > > update all the pipeline handler implementations to use this method > > instead of using the last queued request. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > include/libcamera/request.h | 2 ++ > > src/libcamera/include/pipeline_handler.h | 3 +++ > > src/libcamera/pipeline/ipu3/ipu3.cpp | 3 ++- > > src/libcamera/pipeline/uvcvideo.cpp | 3 ++- > > src/libcamera/pipeline/vimc.cpp | 3 ++- > > src/libcamera/pipeline_handler.cpp | 29 ++++++++++++++++++++++++ > > src/libcamera/request.cpp | 7 ++++++ > > 7 files changed, 47 insertions(+), 3 deletions(-) > > > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > > index 5ac4d20d1d9f..8f5892fd3111 100644 > > --- a/include/libcamera/request.h > > +++ b/include/libcamera/request.h > > @@ -38,6 +38,8 @@ public: > > > > const std::list<Stream *> streams() const; > > > > + const std::unordered_set<Buffer *> &pending() const { return pending_; } > > + > > Status status() const { return status_; } > > > > private: > > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h > > index 920b57609470..6cdadcbdc3ea 100644 > > --- a/src/libcamera/include/pipeline_handler.h > > +++ b/src/libcamera/include/pipeline_handler.h > > @@ -39,6 +39,9 @@ public: > > PipelineHandler *pipe_; > > std::list<Request *> queuedRequests_; > > > > +protected: > > + Request *requestFromBuffer(Buffer *buffer); > > + > > private: > > CameraData(const CameraData &) = delete; > > CameraData &operator=(const CameraData &) = delete; > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 8c67cf985d1e..17e3e8677e28 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -801,7 +801,8 @@ void PipelineHandlerIPU3::IPU3CameraData::imguInputBufferReady(Buffer *buffer) > > */ > > void PipelineHandlerIPU3::IPU3CameraData::imguOutputBufferReady(Buffer *buffer) > > { > > - Request *request = queuedRequests_.front(); > > + Request *request = requestFromBuffer(buffer); > > + ASSERT(request); > > > > pipe_->completeBuffer(camera_, request, buffer); > > pipe_->completeRequest(camera_, request); > > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp > > index 128f0c49dba3..d571b8b4ea83 100644 > > --- a/src/libcamera/pipeline/uvcvideo.cpp > > +++ b/src/libcamera/pipeline/uvcvideo.cpp > > @@ -226,7 +226,8 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator) > > > > void PipelineHandlerUVC::UVCCameraData::bufferReady(Buffer *buffer) > > { > > - Request *request = queuedRequests_.front(); > > + Request *request = requestFromBuffer(buffer); > > This make me feel uneasy :-( > > We talked in the past about how to handle request completion. The idea > from the beginning was conceived with the reequest API in mind, which > IIRC guarantees that requests are completed in the same order they are > queued. While V4L2 have no such guarantees for for buffers. This was one > of the issues I hit with my single stream old UVC camera where the first > buffer queue would contains errors and the uvcvideo driver would not > return it to user-space and move on to the second buffer queued and > libcamera fail as buffers are dequeued out of order (from it's point of > view). > > I understand what you try to do in this patch and I agree it might be > needed. But I fear we first need to discuss how we should handle V4L2 > buffers being dequeued out of order. As this change would hide this > behavior while it's intent is indeed something different. I share Niklas' concern here. Buffers may complete out of order at the V4L2 level, but we must ensure that requests complete in order. Niklas, have you thought about how you would like to solve this ? Thinking out loud, I'm thinking about the following. - When a buffer is dequeued, find the corresponding request. - If the buffer doesn't belong to any request this is an error, but ASSERT is a bit too harsh, we should notify the application instead to allow a clean shutdown. - Otherwise, mark the buffer as complete in the request (remove it from the pending list). - Then, in the pipeline handler, if no more buffer is pending, and if all other data that need to be stored in the request are available, mark the request as complete. We don't have any additional data beyond buffers yet, but the point here is that this decision should be under control of the pipeline handler to make this possible. - If the request just marked as complete is not the first in the queue, leave and there. - Otherwise, notify the application of request completion for all complete requests starting at the beginning of the queue. > > + ASSERT(request); > > > > pipe_->completeBuffer(camera_, request, buffer); > > pipe_->completeRequest(camera_, request); > > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp > > index 6735940799d8..e83416effad8 100644 > > --- a/src/libcamera/pipeline/vimc.cpp > > +++ b/src/libcamera/pipeline/vimc.cpp > > @@ -223,7 +223,8 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator) > > > > void PipelineHandlerVimc::VimcCameraData::bufferReady(Buffer *buffer) > > { > > - Request *request = queuedRequests_.front(); > > + Request *request = requestFromBuffer(buffer); > > + ASSERT(request); > > > > pipe_->completeBuffer(camera_, request, buffer); > > pipe_->completeRequest(camera_, request); > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > index 9a8a4fde57e6..830ff354ed3e 100644 > > --- a/src/libcamera/pipeline_handler.cpp > > +++ b/src/libcamera/pipeline_handler.cpp > > @@ -86,6 +86,35 @@ LOG_DEFINE_CATEGORY(Pipeline) > > * PipelineHandler::completeRequest() > > */ > > > > +/** > > + * \brief Retrieve the pending request that contains \a buffer > > + * \param[in] buffer The buffer contained in the returned request > > + * > > + * Return the request that contains \a buffer, or nullptr if no such request > > + * is found. The intended callers of this method are buffer completion slots > > + * implemented in CameraData sub-classes which needs to associated a request > > + * to the just completed buffer. It is up to the caller of this function to > > + * deal with the case the buffer does not belong to any previously queued > > + * request or the request has already completed, possibly because of a > > + * duplicated buffer completion notification. This is generally considered > > + * a fatal error, and callers are expected to assert the validity of the > > + * returned request. > > + * > > + * \return A pointer to the pending Request that contains the Buffer \a buffer, > > + * or nullptr if no such request is found > > + */ > > +Request *CameraData::requestFromBuffer(Buffer *buffer) > > +{ > > + for (Request *req : queuedRequests_) { > > + for (Buffer *b : req->pending()) { > > + if (b == buffer) > > + return req; > > + } > > + } >From an implementation point of view we may want to add a Request pointer to the Buffer class to make this more efficient. > > + > > + return nullptr; > > +} > > + > > /** > > * \class PipelineHandler > > * \brief Create and manage cameras based on a set of media devices > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > > index 3a7841fb2bb3..c555752b2c0b 100644 > > --- a/src/libcamera/request.cpp > > +++ b/src/libcamera/request.cpp > > @@ -108,6 +108,13 @@ const std::list<Stream *> Request::streams() const > > return streams; > > } > > > > +/** > > + * \fn Request::pending() > > + * \brief Retrieve the list of not-yet-completed buffers > > + * > > + * \return The set of pending buffers > > + */ > > + > > /** > > * \fn Request::status() > > * \brief Retrieve the request completion status
Hi Laurent, Niklas, On Fri, Apr 05, 2019 at 06:45:16PM +0300, Laurent Pinchart wrote: > Hello Jacopo, > > Thank you for the patch. > > On Fri, Apr 05, 2019 at 01:48:35PM +0200, Niklas Söderlund wrote: > > On 2019-04-03 17:07:34 +0200, Jacopo Mondi wrote: > > > Add a method to CameraData base class to retrieve a pointer to the > > > Request that contains a given buffer. Intended users are buffer > > > completion slots that needs to associate a Request to a just completed > > > Buffer. > > > > > > In preparation to support multiple requests from different streams, > > > update all the pipeline handler implementations to use this method > > > instead of using the last queued request. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > include/libcamera/request.h | 2 ++ > > > src/libcamera/include/pipeline_handler.h | 3 +++ > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 3 ++- > > > src/libcamera/pipeline/uvcvideo.cpp | 3 ++- > > > src/libcamera/pipeline/vimc.cpp | 3 ++- > > > src/libcamera/pipeline_handler.cpp | 29 ++++++++++++++++++++++++ > > > src/libcamera/request.cpp | 7 ++++++ > > > 7 files changed, 47 insertions(+), 3 deletions(-) > > > > > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > > > index 5ac4d20d1d9f..8f5892fd3111 100644 > > > --- a/include/libcamera/request.h > > > +++ b/include/libcamera/request.h > > > @@ -38,6 +38,8 @@ public: > > > > > > const std::list<Stream *> streams() const; > > > > > > + const std::unordered_set<Buffer *> &pending() const { return pending_; } > > > + > > > Status status() const { return status_; } > > > > > > private: > > > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h > > > index 920b57609470..6cdadcbdc3ea 100644 > > > --- a/src/libcamera/include/pipeline_handler.h > > > +++ b/src/libcamera/include/pipeline_handler.h > > > @@ -39,6 +39,9 @@ public: > > > PipelineHandler *pipe_; > > > std::list<Request *> queuedRequests_; > > > > > > +protected: > > > + Request *requestFromBuffer(Buffer *buffer); > > > + > > > private: > > > CameraData(const CameraData &) = delete; > > > CameraData &operator=(const CameraData &) = delete; > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > index 8c67cf985d1e..17e3e8677e28 100644 > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > @@ -801,7 +801,8 @@ void PipelineHandlerIPU3::IPU3CameraData::imguInputBufferReady(Buffer *buffer) > > > */ > > > void PipelineHandlerIPU3::IPU3CameraData::imguOutputBufferReady(Buffer *buffer) > > > { > > > - Request *request = queuedRequests_.front(); > > > + Request *request = requestFromBuffer(buffer); > > > + ASSERT(request); > > > > > > pipe_->completeBuffer(camera_, request, buffer); > > > pipe_->completeRequest(camera_, request); > > > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp > > > index 128f0c49dba3..d571b8b4ea83 100644 > > > --- a/src/libcamera/pipeline/uvcvideo.cpp > > > +++ b/src/libcamera/pipeline/uvcvideo.cpp > > > @@ -226,7 +226,8 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator) > > > > > > void PipelineHandlerUVC::UVCCameraData::bufferReady(Buffer *buffer) > > > { > > > - Request *request = queuedRequests_.front(); > > > + Request *request = requestFromBuffer(buffer); > > > > This make me feel uneasy :-( > > > > We talked in the past about how to handle request completion. The idea > > from the beginning was conceived with the reequest API in mind, which > > IIRC guarantees that requests are completed in the same order they are > > queued. While V4L2 have no such guarantees for for buffers. This was one > > of the issues I hit with my single stream old UVC camera where the first > > buffer queue would contains errors and the uvcvideo driver would not > > return it to user-space and move on to the second buffer queued and > > libcamera fail as buffers are dequeued out of order (from it's point of > > view). > > > > I understand what you try to do in this patch and I agree it might be > > needed. But I fear we first need to discuss how we should handle V4L2 > > buffers being dequeued out of order. As this change would hide this > > behavior while it's intent is indeed something different. > > I share Niklas' concern here. Buffers may complete out of order at the > V4L2 level, but we must ensure that requests complete in order. Niklas, > have you thought about how you would like to solve this ? > > Thinking out loud, I'm thinking about the following. > > - When a buffer is dequeued, find the corresponding request. > - If the buffer doesn't belong to any request this is an error, but > ASSERT is a bit too harsh, we should notify the application instead to > allow a clean shutdown. > - Otherwise, mark the buffer as complete in the request (remove it from > the pending list). > - Then, in the pipeline handler, if no more buffer is pending, and if > all other data that need to be stored in the request are available, > mark the request as complete. We don't have any additional data beyond > buffers yet, but the point here is that this decision should be under > control of the pipeline handler to make this possible. > - If the request just marked as complete is not the first in the queue, > leave and there. > - Otherwise, notify the application of request completion for all > complete requests starting at the beginning of the queue. > I wonder how much of this could be moved to the PipelineHandler base class and make it transparent for pipeline handler implementations. > > > + ASSERT(request); > > > > > > pipe_->completeBuffer(camera_, request, buffer); > > > pipe_->completeRequest(camera_, request); > > > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp > > > index 6735940799d8..e83416effad8 100644 > > > --- a/src/libcamera/pipeline/vimc.cpp > > > +++ b/src/libcamera/pipeline/vimc.cpp > > > @@ -223,7 +223,8 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator) > > > > > > void PipelineHandlerVimc::VimcCameraData::bufferReady(Buffer *buffer) > > > { > > > - Request *request = queuedRequests_.front(); > > > + Request *request = requestFromBuffer(buffer); > > > + ASSERT(request); > > > > > > pipe_->completeBuffer(camera_, request, buffer); > > > pipe_->completeRequest(camera_, request); > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > > index 9a8a4fde57e6..830ff354ed3e 100644 > > > --- a/src/libcamera/pipeline_handler.cpp > > > +++ b/src/libcamera/pipeline_handler.cpp > > > @@ -86,6 +86,35 @@ LOG_DEFINE_CATEGORY(Pipeline) > > > * PipelineHandler::completeRequest() > > > */ > > > > > > +/** > > > + * \brief Retrieve the pending request that contains \a buffer > > > + * \param[in] buffer The buffer contained in the returned request > > > + * > > > + * Return the request that contains \a buffer, or nullptr if no such request > > > + * is found. The intended callers of this method are buffer completion slots > > > + * implemented in CameraData sub-classes which needs to associated a request > > > + * to the just completed buffer. It is up to the caller of this function to > > > + * deal with the case the buffer does not belong to any previously queued > > > + * request or the request has already completed, possibly because of a > > > + * duplicated buffer completion notification. This is generally considered > > > + * a fatal error, and callers are expected to assert the validity of the > > > + * returned request. > > > + * > > > + * \return A pointer to the pending Request that contains the Buffer \a buffer, > > > + * or nullptr if no such request is found > > > + */ > > > +Request *CameraData::requestFromBuffer(Buffer *buffer) > > > +{ > > > + for (Request *req : queuedRequests_) { > > > + for (Buffer *b : req->pending()) { > > > + if (b == buffer) > > > + return req; > > > + } > > > + } > > From an implementation point of view we may want to add a Request > pointer to the Buffer class to make this more efficient. > Indeed, as Niklas pointed out, there are enough loops on the Stream and Buffers containers around. Thanks j > > > + > > > + return nullptr; > > > +} > > > + > > > /** > > > * \class PipelineHandler > > > * \brief Create and manage cameras based on a set of media devices > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > > > index 3a7841fb2bb3..c555752b2c0b 100644 > > > --- a/src/libcamera/request.cpp > > > +++ b/src/libcamera/request.cpp > > > @@ -108,6 +108,13 @@ const std::list<Stream *> Request::streams() const > > > return streams; > > > } > > > > > > +/** > > > + * \fn Request::pending() > > > + * \brief Retrieve the list of not-yet-completed buffers > > > + * > > > + * \return The set of pending buffers > > > + */ > > > + > > > /** > > > * \fn Request::status() > > > * \brief Retrieve the request completion status > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Mon, Apr 08, 2019 at 10:05:58AM +0200, Jacopo Mondi wrote: > On Fri, Apr 05, 2019 at 06:45:16PM +0300, Laurent Pinchart wrote: > > On Fri, Apr 05, 2019 at 01:48:35PM +0200, Niklas Söderlund wrote: > >> On 2019-04-03 17:07:34 +0200, Jacopo Mondi wrote: > >>> Add a method to CameraData base class to retrieve a pointer to the > >>> Request that contains a given buffer. Intended users are buffer > >>> completion slots that needs to associate a Request to a just completed > >>> Buffer. > >>> > >>> In preparation to support multiple requests from different streams, > >>> update all the pipeline handler implementations to use this method > >>> instead of using the last queued request. > >>> > >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >>> --- > >>> include/libcamera/request.h | 2 ++ > >>> src/libcamera/include/pipeline_handler.h | 3 +++ > >>> src/libcamera/pipeline/ipu3/ipu3.cpp | 3 ++- > >>> src/libcamera/pipeline/uvcvideo.cpp | 3 ++- > >>> src/libcamera/pipeline/vimc.cpp | 3 ++- > >>> src/libcamera/pipeline_handler.cpp | 29 ++++++++++++++++++++++++ > >>> src/libcamera/request.cpp | 7 ++++++ > >>> 7 files changed, 47 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h > >>> index 5ac4d20d1d9f..8f5892fd3111 100644 > >>> --- a/include/libcamera/request.h > >>> +++ b/include/libcamera/request.h > >>> @@ -38,6 +38,8 @@ public: > >>> > >>> const std::list<Stream *> streams() const; > >>> > >>> + const std::unordered_set<Buffer *> &pending() const { return pending_; } > >>> + > >>> Status status() const { return status_; } > >>> > >>> private: > >>> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h > >>> index 920b57609470..6cdadcbdc3ea 100644 > >>> --- a/src/libcamera/include/pipeline_handler.h > >>> +++ b/src/libcamera/include/pipeline_handler.h > >>> @@ -39,6 +39,9 @@ public: > >>> PipelineHandler *pipe_; > >>> std::list<Request *> queuedRequests_; > >>> > >>> +protected: > >>> + Request *requestFromBuffer(Buffer *buffer); > >>> + > >>> private: > >>> CameraData(const CameraData &) = delete; > >>> CameraData &operator=(const CameraData &) = delete; > >>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > >>> index 8c67cf985d1e..17e3e8677e28 100644 > >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > >>> @@ -801,7 +801,8 @@ void PipelineHandlerIPU3::IPU3CameraData::imguInputBufferReady(Buffer *buffer) > >>> */ > >>> void PipelineHandlerIPU3::IPU3CameraData::imguOutputBufferReady(Buffer *buffer) > >>> { > >>> - Request *request = queuedRequests_.front(); > >>> + Request *request = requestFromBuffer(buffer); > >>> + ASSERT(request); > >>> > >>> pipe_->completeBuffer(camera_, request, buffer); > >>> pipe_->completeRequest(camera_, request); > >>> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp > >>> index 128f0c49dba3..d571b8b4ea83 100644 > >>> --- a/src/libcamera/pipeline/uvcvideo.cpp > >>> +++ b/src/libcamera/pipeline/uvcvideo.cpp > >>> @@ -226,7 +226,8 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator) > >>> > >>> void PipelineHandlerUVC::UVCCameraData::bufferReady(Buffer *buffer) > >>> { > >>> - Request *request = queuedRequests_.front(); > >>> + Request *request = requestFromBuffer(buffer); > >> > >> This make me feel uneasy :-( > >> > >> We talked in the past about how to handle request completion. The idea > >> from the beginning was conceived with the reequest API in mind, which > >> IIRC guarantees that requests are completed in the same order they are > >> queued. While V4L2 have no such guarantees for for buffers. This was one > >> of the issues I hit with my single stream old UVC camera where the first > >> buffer queue would contains errors and the uvcvideo driver would not > >> return it to user-space and move on to the second buffer queued and > >> libcamera fail as buffers are dequeued out of order (from it's point of > >> view). > >> > >> I understand what you try to do in this patch and I agree it might be > >> needed. But I fear we first need to discuss how we should handle V4L2 > >> buffers being dequeued out of order. As this change would hide this > >> behavior while it's intent is indeed something different. > > > > I share Niklas' concern here. Buffers may complete out of order at the > > V4L2 level, but we must ensure that requests complete in order. Niklas, > > have you thought about how you would like to solve this ? > > > > Thinking out loud, I'm thinking about the following. > > > > - When a buffer is dequeued, find the corresponding request. > > - If the buffer doesn't belong to any request this is an error, but > > ASSERT is a bit too harsh, we should notify the application instead to > > allow a clean shutdown. > > - Otherwise, mark the buffer as complete in the request (remove it from > > the pending list). > > - Then, in the pipeline handler, if no more buffer is pending, and if > > all other data that need to be stored in the request are available, > > mark the request as complete. We don't have any additional data beyond > > buffers yet, but the point here is that this decision should be under > > control of the pipeline handler to make this possible. > > - If the request just marked as complete is not the first in the queue, > > leave and there. > > - Otherwise, notify the application of request completion for all > > complete requests starting at the beginning of the queue. > > I wonder how much of this could be moved to the PipelineHandler base class > and make it transparent for pipeline handler implementations. Probably quite a bit, but I think we should implement it in the IPU3 pipeline handler first, and then refactor the code when a second pipeline handler will need this (of course if you already have a clear view of how it will look like, you can also have a go :-)). > >>> + ASSERT(request); > >>> > >>> pipe_->completeBuffer(camera_, request, buffer); > >>> pipe_->completeRequest(camera_, request); > >>> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp > >>> index 6735940799d8..e83416effad8 100644 > >>> --- a/src/libcamera/pipeline/vimc.cpp > >>> +++ b/src/libcamera/pipeline/vimc.cpp > >>> @@ -223,7 +223,8 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator) > >>> > >>> void PipelineHandlerVimc::VimcCameraData::bufferReady(Buffer *buffer) > >>> { > >>> - Request *request = queuedRequests_.front(); > >>> + Request *request = requestFromBuffer(buffer); > >>> + ASSERT(request); > >>> > >>> pipe_->completeBuffer(camera_, request, buffer); > >>> pipe_->completeRequest(camera_, request); > >>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > >>> index 9a8a4fde57e6..830ff354ed3e 100644 > >>> --- a/src/libcamera/pipeline_handler.cpp > >>> +++ b/src/libcamera/pipeline_handler.cpp > >>> @@ -86,6 +86,35 @@ LOG_DEFINE_CATEGORY(Pipeline) > >>> * PipelineHandler::completeRequest() > >>> */ > >>> > >>> +/** > >>> + * \brief Retrieve the pending request that contains \a buffer > >>> + * \param[in] buffer The buffer contained in the returned request > >>> + * > >>> + * Return the request that contains \a buffer, or nullptr if no such request > >>> + * is found. The intended callers of this method are buffer completion slots > >>> + * implemented in CameraData sub-classes which needs to associated a request > >>> + * to the just completed buffer. It is up to the caller of this function to > >>> + * deal with the case the buffer does not belong to any previously queued > >>> + * request or the request has already completed, possibly because of a > >>> + * duplicated buffer completion notification. This is generally considered > >>> + * a fatal error, and callers are expected to assert the validity of the > >>> + * returned request. > >>> + * > >>> + * \return A pointer to the pending Request that contains the Buffer \a buffer, > >>> + * or nullptr if no such request is found > >>> + */ > >>> +Request *CameraData::requestFromBuffer(Buffer *buffer) > >>> +{ > >>> + for (Request *req : queuedRequests_) { > >>> + for (Buffer *b : req->pending()) { > >>> + if (b == buffer) > >>> + return req; > >>> + } > >>> + } > > > > From an implementation point of view we may want to add a Request > > pointer to the Buffer class to make this more efficient. > > Indeed, as Niklas pointed out, there are enough loops on the Stream > and Buffers containers around. > > >>> + > >>> + return nullptr; > >>> +} > >>> + > >>> /** > >>> * \class PipelineHandler > >>> * \brief Create and manage cameras based on a set of media devices > >>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > >>> index 3a7841fb2bb3..c555752b2c0b 100644 > >>> --- a/src/libcamera/request.cpp > >>> +++ b/src/libcamera/request.cpp > >>> @@ -108,6 +108,13 @@ const std::list<Stream *> Request::streams() const > >>> return streams; > >>> } > >>> > >>> +/** > >>> + * \fn Request::pending() > >>> + * \brief Retrieve the list of not-yet-completed buffers > >>> + * > >>> + * \return The set of pending buffers > >>> + */ > >>> + > >>> /** > >>> * \fn Request::status() > >>> * \brief Retrieve the request completion status
diff --git a/include/libcamera/request.h b/include/libcamera/request.h index 5ac4d20d1d9f..8f5892fd3111 100644 --- a/include/libcamera/request.h +++ b/include/libcamera/request.h @@ -38,6 +38,8 @@ public: const std::list<Stream *> streams() const; + const std::unordered_set<Buffer *> &pending() const { return pending_; } + Status status() const { return status_; } private: diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h index 920b57609470..6cdadcbdc3ea 100644 --- a/src/libcamera/include/pipeline_handler.h +++ b/src/libcamera/include/pipeline_handler.h @@ -39,6 +39,9 @@ public: PipelineHandler *pipe_; std::list<Request *> queuedRequests_; +protected: + Request *requestFromBuffer(Buffer *buffer); + private: CameraData(const CameraData &) = delete; CameraData &operator=(const CameraData &) = delete; diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 8c67cf985d1e..17e3e8677e28 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -801,7 +801,8 @@ void PipelineHandlerIPU3::IPU3CameraData::imguInputBufferReady(Buffer *buffer) */ void PipelineHandlerIPU3::IPU3CameraData::imguOutputBufferReady(Buffer *buffer) { - Request *request = queuedRequests_.front(); + Request *request = requestFromBuffer(buffer); + ASSERT(request); pipe_->completeBuffer(camera_, request, buffer); pipe_->completeRequest(camera_, request); diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp index 128f0c49dba3..d571b8b4ea83 100644 --- a/src/libcamera/pipeline/uvcvideo.cpp +++ b/src/libcamera/pipeline/uvcvideo.cpp @@ -226,7 +226,8 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator) void PipelineHandlerUVC::UVCCameraData::bufferReady(Buffer *buffer) { - Request *request = queuedRequests_.front(); + Request *request = requestFromBuffer(buffer); + ASSERT(request); pipe_->completeBuffer(camera_, request, buffer); pipe_->completeRequest(camera_, request); diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp index 6735940799d8..e83416effad8 100644 --- a/src/libcamera/pipeline/vimc.cpp +++ b/src/libcamera/pipeline/vimc.cpp @@ -223,7 +223,8 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator) void PipelineHandlerVimc::VimcCameraData::bufferReady(Buffer *buffer) { - Request *request = queuedRequests_.front(); + Request *request = requestFromBuffer(buffer); + ASSERT(request); pipe_->completeBuffer(camera_, request, buffer); pipe_->completeRequest(camera_, request); diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index 9a8a4fde57e6..830ff354ed3e 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -86,6 +86,35 @@ LOG_DEFINE_CATEGORY(Pipeline) * PipelineHandler::completeRequest() */ +/** + * \brief Retrieve the pending request that contains \a buffer + * \param[in] buffer The buffer contained in the returned request + * + * Return the request that contains \a buffer, or nullptr if no such request + * is found. The intended callers of this method are buffer completion slots + * implemented in CameraData sub-classes which needs to associated a request + * to the just completed buffer. It is up to the caller of this function to + * deal with the case the buffer does not belong to any previously queued + * request or the request has already completed, possibly because of a + * duplicated buffer completion notification. This is generally considered + * a fatal error, and callers are expected to assert the validity of the + * returned request. + * + * \return A pointer to the pending Request that contains the Buffer \a buffer, + * or nullptr if no such request is found + */ +Request *CameraData::requestFromBuffer(Buffer *buffer) +{ + for (Request *req : queuedRequests_) { + for (Buffer *b : req->pending()) { + if (b == buffer) + return req; + } + } + + return nullptr; +} + /** * \class PipelineHandler * \brief Create and manage cameras based on a set of media devices diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp index 3a7841fb2bb3..c555752b2c0b 100644 --- a/src/libcamera/request.cpp +++ b/src/libcamera/request.cpp @@ -108,6 +108,13 @@ const std::list<Stream *> Request::streams() const return streams; } +/** + * \fn Request::pending() + * \brief Retrieve the list of not-yet-completed buffers + * + * \return The set of pending buffers + */ + /** * \fn Request::status() * \brief Retrieve the request completion status
Add a method to CameraData base class to retrieve a pointer to the Request that contains a given buffer. Intended users are buffer completion slots that needs to associate a Request to a just completed Buffer. In preparation to support multiple requests from different streams, update all the pipeline handler implementations to use this method instead of using the last queued request. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- include/libcamera/request.h | 2 ++ src/libcamera/include/pipeline_handler.h | 3 +++ src/libcamera/pipeline/ipu3/ipu3.cpp | 3 ++- src/libcamera/pipeline/uvcvideo.cpp | 3 ++- src/libcamera/pipeline/vimc.cpp | 3 ++- src/libcamera/pipeline_handler.cpp | 29 ++++++++++++++++++++++++ src/libcamera/request.cpp | 7 ++++++ 7 files changed, 47 insertions(+), 3 deletions(-)