| Message ID | 20210701192951.662584-1-nfraprado@collabora.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Nicolas, On Thu, Jul 01, 2021 at 04:29:51PM -0300, Nícolas F. R. A. Prado wrote: > Add an internal queue that stores requests until there are v4l2 buffer > slots available. This avoids the need to cancel requests when there is a > shortage of said buffers. > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > --- > > This is very similar to what 5a9d19210fad ("libcamera: pipeline: ipu3: Try > queuing pending requests if a buffer is available") and 89dae5844964 > ("libcamera: pipeline: ipu3: Store requests in the case a buffer shortage") did > for IPU3. > > I tested this with a new lc-compliance test from [1] which is a rebased version > of the series in [2]. I'll resend this series after the googletest refactor > series [3] is merged. But this patch for uvcvideo doesn't depend on that. > > [1] https://gitlab.collabora.com/nfraprado/libcamera/-/tree/lcc-lots-requests-v5 > [2] https://lists.libcamera.org/pipermail/libcamera-devel/2021-May/020150.html > [3] https://lists.libcamera.org/pipermail/libcamera-devel/2021-June/021891.html > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 64 +++++++++++++++++--- > 1 file changed, 54 insertions(+), 10 deletions(-) > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > index 0f634b8da609..a51131dce8e0 100644 > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > @@ -10,6 +10,7 @@ > #include <iomanip> > #include <math.h> > #include <memory> > +#include <queue> > #include <tuple> > > #include <libcamera/base/log.h> > @@ -45,6 +46,11 @@ public: > ControlInfoMap::Map *ctrls); > void bufferReady(FrameBuffer *buffer); > > + void queuePendingRequests(); > + void cancelPendingRequests(); Am I wrong or this is not called in this patch (or any other related patch I can find). Should you call it in stop() ? Thanks j > + > + std::queue<Request *> pendingRequests_; > + > std::unique_ptr<V4L2VideoDevice> video_; > Stream stream_; > }; > @@ -79,12 +85,13 @@ public: > > bool match(DeviceEnumerator *enumerator) override; > > + int processControls(UVCCameraData *data, Request *request); > + > private: > std::string generateId(const UVCCameraData *data); > > int processControl(ControlList *controls, unsigned int id, > const ControlValue &value); > - int processControls(UVCCameraData *data, Request *request); > > UVCCameraData *cameraData(const Camera *camera) > { > @@ -363,24 +370,59 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request) > return ret; > } > > +void UVCCameraData::cancelPendingRequests() > +{ > + while (!pendingRequests_.empty()) { > + Request *request = pendingRequests_.front(); > + FrameBuffer *buffer = request->findBuffer(&stream_); > + > + buffer->cancel(); > + pipe_->completeBuffer(request, buffer); > + pipe_->completeRequest(request); > + > + pendingRequests_.pop(); > + } > +} > + > +void UVCCameraData::queuePendingRequests() > +{ > + PipelineHandlerUVC *pipe = static_cast<PipelineHandlerUVC *>(pipe_); > + > + while (!pendingRequests_.empty()) { > + Request *request = pendingRequests_.front(); > + FrameBuffer *buffer = request->findBuffer(&stream_); > + > + int ret = pipe->processControls(this, request); > + if (ret < 0) { > + buffer->cancel(); > + pipe_->completeBuffer(request, buffer); > + pipe_->completeRequest(request); > + pendingRequests_.pop(); > + continue; > + } > + > + /* If we're missing v4l2 buffer slots, try again later */ > + ret = video_->queueBuffer(buffer); > + if (ret < 0) > + break; > + > + pendingRequests_.pop(); > + } > +} > + > int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request) > { > UVCCameraData *data = cameraData(camera); > - FrameBuffer *buffer = request->findBuffer(&data->stream_); > - if (!buffer) { > + > + if (!request->findBuffer(&data->stream_)) { > LOG(UVC, Error) > << "Attempt to queue request with invalid stream"; > > return -ENOENT; > } > > - int ret = processControls(data, request); > - if (ret < 0) > - return ret; > - > - ret = data->video_->queueBuffer(buffer); > - if (ret < 0) > - return ret; > + data->pendingRequests_.push(request); > + data->queuePendingRequests(); > > return 0; > } > @@ -668,6 +710,8 @@ void UVCCameraData::bufferReady(FrameBuffer *buffer) > > pipe_->completeBuffer(request, buffer); > pipe_->completeRequest(request); > + > + queuePendingRequests(); > } > > REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC) > -- > 2.32.0 >
Hi Jacopo, On Mon, Jul 05, 2021 at 10:09:28AM +0200, Jacopo Mondi wrote: > Hi Nicolas, > > On Thu, Jul 01, 2021 at 04:29:51PM -0300, Nícolas F. R. A. Prado wrote: > > Add an internal queue that stores requests until there are v4l2 buffer > > slots available. This avoids the need to cancel requests when there is a > > shortage of said buffers. > > > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > > --- > > > > This is very similar to what 5a9d19210fad ("libcamera: pipeline: ipu3: Try > > queuing pending requests if a buffer is available") and 89dae5844964 > > ("libcamera: pipeline: ipu3: Store requests in the case a buffer shortage") did > > for IPU3. > > > > I tested this with a new lc-compliance test from [1] which is a rebased version > > of the series in [2]. I'll resend this series after the googletest refactor > > series [3] is merged. But this patch for uvcvideo doesn't depend on that. > > > > [1] https://gitlab.collabora.com/nfraprado/libcamera/-/tree/lcc-lots-requests-v5 > > [2] https://lists.libcamera.org/pipermail/libcamera-devel/2021-May/020150.html > > [3] https://lists.libcamera.org/pipermail/libcamera-devel/2021-June/021891.html > > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 64 +++++++++++++++++--- > > 1 file changed, 54 insertions(+), 10 deletions(-) > > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > index 0f634b8da609..a51131dce8e0 100644 > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > @@ -10,6 +10,7 @@ > > #include <iomanip> > > #include <math.h> > > #include <memory> > > +#include <queue> > > #include <tuple> > > > > #include <libcamera/base/log.h> > > @@ -45,6 +46,11 @@ public: > > ControlInfoMap::Map *ctrls); > > void bufferReady(FrameBuffer *buffer); > > > > + void queuePendingRequests(); > > + void cancelPendingRequests(); > > Am I wrong or this is not called in this patch (or any other related > patch I can find). Should you call it in stop() ? Oops, indeed, totally forgot about that... I'll fix it for v2. Thanks, Nícolas > > Thanks > j > > > + > > + std::queue<Request *> pendingRequests_; > > + > > std::unique_ptr<V4L2VideoDevice> video_; > > Stream stream_; > > }; > > @@ -79,12 +85,13 @@ public: > > > > bool match(DeviceEnumerator *enumerator) override; > > > > + int processControls(UVCCameraData *data, Request *request); > > + > > private: > > std::string generateId(const UVCCameraData *data); > > > > int processControl(ControlList *controls, unsigned int id, > > const ControlValue &value); > > - int processControls(UVCCameraData *data, Request *request); > > > > UVCCameraData *cameraData(const Camera *camera) > > { > > @@ -363,24 +370,59 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request) > > return ret; > > } > > > > +void UVCCameraData::cancelPendingRequests() > > +{ > > + while (!pendingRequests_.empty()) { > > + Request *request = pendingRequests_.front(); > > + FrameBuffer *buffer = request->findBuffer(&stream_); > > + > > + buffer->cancel(); > > + pipe_->completeBuffer(request, buffer); > > + pipe_->completeRequest(request); > > + > > + pendingRequests_.pop(); > > + } > > +} > > + > > +void UVCCameraData::queuePendingRequests() > > +{ > > + PipelineHandlerUVC *pipe = static_cast<PipelineHandlerUVC *>(pipe_); > > + > > + while (!pendingRequests_.empty()) { > > + Request *request = pendingRequests_.front(); > > + FrameBuffer *buffer = request->findBuffer(&stream_); > > + > > + int ret = pipe->processControls(this, request); > > + if (ret < 0) { > > + buffer->cancel(); > > + pipe_->completeBuffer(request, buffer); > > + pipe_->completeRequest(request); > > + pendingRequests_.pop(); > > + continue; > > + } > > + > > + /* If we're missing v4l2 buffer slots, try again later */ > > + ret = video_->queueBuffer(buffer); > > + if (ret < 0) > > + break; > > + > > + pendingRequests_.pop(); > > + } > > +} > > + > > int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request) > > { > > UVCCameraData *data = cameraData(camera); > > - FrameBuffer *buffer = request->findBuffer(&data->stream_); > > - if (!buffer) { > > + > > + if (!request->findBuffer(&data->stream_)) { > > LOG(UVC, Error) > > << "Attempt to queue request with invalid stream"; > > > > return -ENOENT; > > } > > > > - int ret = processControls(data, request); > > - if (ret < 0) > > - return ret; > > - > > - ret = data->video_->queueBuffer(buffer); > > - if (ret < 0) > > - return ret; > > + data->pendingRequests_.push(request); > > + data->queuePendingRequests(); > > > > return 0; > > } > > @@ -668,6 +710,8 @@ void UVCCameraData::bufferReady(FrameBuffer *buffer) > > > > pipe_->completeBuffer(request, buffer); > > pipe_->completeRequest(request); > > + > > + queuePendingRequests(); > > } > > > > REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC) > > -- > > 2.32.0 > >
diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp index 0f634b8da609..a51131dce8e0 100644 --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp @@ -10,6 +10,7 @@ #include <iomanip> #include <math.h> #include <memory> +#include <queue> #include <tuple> #include <libcamera/base/log.h> @@ -45,6 +46,11 @@ public: ControlInfoMap::Map *ctrls); void bufferReady(FrameBuffer *buffer); + void queuePendingRequests(); + void cancelPendingRequests(); + + std::queue<Request *> pendingRequests_; + std::unique_ptr<V4L2VideoDevice> video_; Stream stream_; }; @@ -79,12 +85,13 @@ public: bool match(DeviceEnumerator *enumerator) override; + int processControls(UVCCameraData *data, Request *request); + private: std::string generateId(const UVCCameraData *data); int processControl(ControlList *controls, unsigned int id, const ControlValue &value); - int processControls(UVCCameraData *data, Request *request); UVCCameraData *cameraData(const Camera *camera) { @@ -363,24 +370,59 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request) return ret; } +void UVCCameraData::cancelPendingRequests() +{ + while (!pendingRequests_.empty()) { + Request *request = pendingRequests_.front(); + FrameBuffer *buffer = request->findBuffer(&stream_); + + buffer->cancel(); + pipe_->completeBuffer(request, buffer); + pipe_->completeRequest(request); + + pendingRequests_.pop(); + } +} + +void UVCCameraData::queuePendingRequests() +{ + PipelineHandlerUVC *pipe = static_cast<PipelineHandlerUVC *>(pipe_); + + while (!pendingRequests_.empty()) { + Request *request = pendingRequests_.front(); + FrameBuffer *buffer = request->findBuffer(&stream_); + + int ret = pipe->processControls(this, request); + if (ret < 0) { + buffer->cancel(); + pipe_->completeBuffer(request, buffer); + pipe_->completeRequest(request); + pendingRequests_.pop(); + continue; + } + + /* If we're missing v4l2 buffer slots, try again later */ + ret = video_->queueBuffer(buffer); + if (ret < 0) + break; + + pendingRequests_.pop(); + } +} + int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request) { UVCCameraData *data = cameraData(camera); - FrameBuffer *buffer = request->findBuffer(&data->stream_); - if (!buffer) { + + if (!request->findBuffer(&data->stream_)) { LOG(UVC, Error) << "Attempt to queue request with invalid stream"; return -ENOENT; } - int ret = processControls(data, request); - if (ret < 0) - return ret; - - ret = data->video_->queueBuffer(buffer); - if (ret < 0) - return ret; + data->pendingRequests_.push(request); + data->queuePendingRequests(); return 0; } @@ -668,6 +710,8 @@ void UVCCameraData::bufferReady(FrameBuffer *buffer) pipe_->completeBuffer(request, buffer); pipe_->completeRequest(request); + + queuePendingRequests(); } REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC)
Add an internal queue that stores requests until there are v4l2 buffer slots available. This avoids the need to cancel requests when there is a shortage of said buffers. Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> --- This is very similar to what 5a9d19210fad ("libcamera: pipeline: ipu3: Try queuing pending requests if a buffer is available") and 89dae5844964 ("libcamera: pipeline: ipu3: Store requests in the case a buffer shortage") did for IPU3. I tested this with a new lc-compliance test from [1] which is a rebased version of the series in [2]. I'll resend this series after the googletest refactor series [3] is merged. But this patch for uvcvideo doesn't depend on that. [1] https://gitlab.collabora.com/nfraprado/libcamera/-/tree/lcc-lots-requests-v5 [2] https://lists.libcamera.org/pipermail/libcamera-devel/2021-May/020150.html [3] https://lists.libcamera.org/pipermail/libcamera-devel/2021-June/021891.html src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 64 +++++++++++++++++--- 1 file changed, 54 insertions(+), 10 deletions(-)