| Message ID | 20250526214224.13631-2-stefan.klug@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi 2025. 05. 26. 23:42 keltezéssel, Stefan Klug írta: > The waiting requests of one camera should not be able to influence > queuing to another camera. Therefore change the single waitingRequests_ > queue into one queue per camera. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > include/libcamera/internal/camera.h | 2 ++ > include/libcamera/internal/pipeline_handler.h | 3 -- > src/libcamera/pipeline_handler.cpp | 34 +++++++++++++------ > 3 files changed, 25 insertions(+), 14 deletions(-) > > diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h > index 18f5c32a18e4..8a2e9ed5894d 100644 > --- a/include/libcamera/internal/camera.h > +++ b/include/libcamera/internal/camera.h > @@ -10,6 +10,7 @@ > #include <atomic> > #include <list> > #include <memory> > +#include <queue> > #include <set> > #include <stdint.h> > #include <string> > @@ -36,6 +37,7 @@ public: > const PipelineHandler *pipe() const { return pipe_.get(); } > > std::list<Request *> queuedRequests_; > + std::queue<Request *> waitingRequests_; > ControlInfoMap controlInfo_; > ControlList properties_; > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > index 972a2fa65310..dedc29e815fb 100644 > --- a/include/libcamera/internal/pipeline_handler.h > +++ b/include/libcamera/internal/pipeline_handler.h > @@ -8,7 +8,6 @@ > #pragma once > > #include <memory> > -#include <queue> > #include <string> > #include <sys/types.h> > #include <vector> > @@ -94,8 +93,6 @@ private: > std::vector<std::shared_ptr<MediaDevice>> mediaDevices_; > std::vector<std::weak_ptr<Camera>> cameras_; > > - std::queue<Request *> waitingRequests_; > - > const char *name_; > unsigned int useCount_; > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index d84dff3c9f19..14d8602e67d8 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -363,15 +363,16 @@ void PipelineHandler::stop(Camera *camera) > /* Stop the pipeline handler and let the queued requests complete. */ > stopDevice(camera); > > + Camera::Private *data = camera->_d(); > + > /* Cancel and signal as complete all waiting requests. */ > - while (!waitingRequests_.empty()) { > - Request *request = waitingRequests_.front(); > - waitingRequests_.pop(); > + while (!data->waitingRequests_.empty()) { > + Request *request = data->waitingRequests_.front(); > + data->waitingRequests_.pop(); > cancelRequest(request); > } > > /* Make sure no requests are pending. */ > - Camera::Private *data = camera->_d(); > ASSERT(data->queuedRequests_.empty()); > > data->requestSequence_ = 0; > @@ -444,7 +445,9 @@ void PipelineHandler::queueRequest(Request *request) > { > LIBCAMERA_TRACEPOINT(request_queue, request); > > - waitingRequests_.push(request); > + Camera *camera = request->_d()->camera(); > + Camera::Private *data = camera->_d(); > + data->waitingRequests_.push(request); > > request->_d()->prepare(300ms); > } > @@ -480,13 +483,20 @@ void PipelineHandler::doQueueRequest(Request *request) > */ > void PipelineHandler::doQueueRequests() > { > - while (!waitingRequests_.empty()) { > - Request *request = waitingRequests_.front(); > - if (!request->_d()->prepared_) > - break; > + for (const std::weak_ptr<Camera> &ptr : cameras_) { > + std::shared_ptr<Camera> camera = ptr.lock(); > + if (!camera) > + continue; > > - doQueueRequest(request); > - waitingRequests_.pop(); > + Camera::Private *data = camera->_d(); > + while (!data->waitingRequests_.empty()) { As far as I understand `doQueueRequests()` runs when a request becomes "prepared". So I would expect the above inner loop to be sufficient. Is there any reason why the waiting request queues of all cameras are processed? > + Request *request = data->waitingRequests_.front(); > + if (!request->_d()->prepared_) > + break; > + > + doQueueRequest(request); > + data->waitingRequests_.pop(); > + } > } > } > > @@ -562,6 +572,8 @@ void PipelineHandler::completeRequest(Request *request) > data->queuedRequests_.pop_front(); > camera->requestComplete(req); > } > + > + doQueueRequests(); This only purpose of this is to handle early stopping due to `maxQueuedRequestsDevice()`, right? If so, then I think this should be in the next patch. Regards, Barnabás Pőcze > } > > /** > -- > 2.43.0 >
Hi Barnabás, On 5/27/25 09:27, Barnabás Pőcze wrote: > Hi > > 2025. 05. 26. 23:42 keltezéssel, Stefan Klug írta: >> The waiting requests of one camera should not be able to influence >> queuing to another camera. Therefore change the single waitingRequests_ >> queue into one queue per camera. >> >> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> >> --- >> include/libcamera/internal/camera.h | 2 ++ >> include/libcamera/internal/pipeline_handler.h | 3 -- >> src/libcamera/pipeline_handler.cpp | 34 +++++++++++++------ >> 3 files changed, 25 insertions(+), 14 deletions(-) >> >> diff --git a/include/libcamera/internal/camera.h >> b/include/libcamera/internal/camera.h >> index 18f5c32a18e4..8a2e9ed5894d 100644 >> --- a/include/libcamera/internal/camera.h >> +++ b/include/libcamera/internal/camera.h >> @@ -10,6 +10,7 @@ >> #include <atomic> >> #include <list> >> #include <memory> >> +#include <queue> >> #include <set> >> #include <stdint.h> >> #include <string> >> @@ -36,6 +37,7 @@ public: >> const PipelineHandler *pipe() const { return pipe_.get(); } >> >> std::list<Request *> queuedRequests_; >> + std::queue<Request *> waitingRequests_; >> ControlInfoMap controlInfo_; >> ControlList properties_; >> >> diff --git a/include/libcamera/internal/pipeline_handler.h >> b/include/libcamera/internal/pipeline_handler.h >> index 972a2fa65310..dedc29e815fb 100644 >> --- a/include/libcamera/internal/pipeline_handler.h >> +++ b/include/libcamera/internal/pipeline_handler.h >> @@ -8,7 +8,6 @@ >> #pragma once >> >> #include <memory> >> -#include <queue> >> #include <string> >> #include <sys/types.h> >> #include <vector> >> @@ -94,8 +93,6 @@ private: >> std::vector<std::shared_ptr<MediaDevice>> mediaDevices_; >> std::vector<std::weak_ptr<Camera>> cameras_; >> >> - std::queue<Request *> waitingRequests_; >> - >> const char *name_; >> unsigned int useCount_; >> >> diff --git a/src/libcamera/pipeline_handler.cpp >> b/src/libcamera/pipeline_handler.cpp >> index d84dff3c9f19..14d8602e67d8 100644 >> --- a/src/libcamera/pipeline_handler.cpp >> +++ b/src/libcamera/pipeline_handler.cpp >> @@ -363,15 +363,16 @@ void PipelineHandler::stop(Camera *camera) >> /* Stop the pipeline handler and let the queued requests >> complete. */ >> stopDevice(camera); >> >> + Camera::Private *data = camera->_d(); >> + >> /* Cancel and signal as complete all waiting requests. */ >> - while (!waitingRequests_.empty()) { >> - Request *request = waitingRequests_.front(); >> - waitingRequests_.pop(); >> + while (!data->waitingRequests_.empty()) { >> + Request *request = data->waitingRequests_.front(); >> + data->waitingRequests_.pop(); >> cancelRequest(request); >> } >> >> /* Make sure no requests are pending. */ >> - Camera::Private *data = camera->_d(); >> ASSERT(data->queuedRequests_.empty()); >> >> data->requestSequence_ = 0; >> @@ -444,7 +445,9 @@ void PipelineHandler::queueRequest(Request *request) >> { >> LIBCAMERA_TRACEPOINT(request_queue, request); >> >> - waitingRequests_.push(request); >> + Camera *camera = request->_d()->camera(); >> + Camera::Private *data = camera->_d(); >> + data->waitingRequests_.push(request); >> >> request->_d()->prepare(300ms); >> } >> @@ -480,13 +483,20 @@ void PipelineHandler::doQueueRequest(Request >> *request) >> */ >> void PipelineHandler::doQueueRequests() >> { >> - while (!waitingRequests_.empty()) { >> - Request *request = waitingRequests_.front(); >> - if (!request->_d()->prepared_) >> - break; >> + for (const std::weak_ptr<Camera> &ptr : cameras_) { >> + std::shared_ptr<Camera> camera = ptr.lock(); >> + if (!camera) >> + continue; >> >> - doQueueRequest(request); >> - waitingRequests_.pop(); >> + Camera::Private *data = camera->_d(); >> + while (!data->waitingRequests_.empty()) { > > As far as I understand `doQueueRequests()` runs when a request becomes > "prepared". > So I would expect the above inner loop to be sufficient. Is there any > reason why > the waiting request queues of all cameras are processed? This results from the fact that the patchset splits the waitingRequests_ from the PipelineHandler to the (multiple) individual Camera classes. Therefore an additional outer loop is necessary to iterate though all camera class instances holding potential waitingRequests_. I would propose to add a Camera shared pointer as an argument to the doQueueRequests function (and adjust the signal) to avoid looping over all cameras. > > >> + Request *request = data->waitingRequests_.front(); >> + if (!request->_d()->prepared_) >> + break; >> + >> + doQueueRequest(request); >> + data->waitingRequests_.pop(); >> + } >> } >> } >> >> @@ -562,6 +572,8 @@ void PipelineHandler::completeRequest(Request >> *request) >> data->queuedRequests_.pop_front(); >> camera->requestComplete(req); >> } >> + >> + doQueueRequests(); > > This only purpose of this is to handle early stopping due to > `maxQueuedRequestsDevice()`, right? > If so, then I think this should be in the next patch. yeah, exactly. I agree to move it to the next patch. Sincerely Sven > > > Regards, > Barnabás Pőcze > > >> } >> >> /** >> -- >> 2.43.0 >> > >
Quoting Stefan Klug (2025-05-26 22:42:15) > The waiting requests of one camera should not be able to influence > queuing to another camera. Therefore change the single waitingRequests_ > queue into one queue per camera. > Eeek. > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > include/libcamera/internal/camera.h | 2 ++ > include/libcamera/internal/pipeline_handler.h | 3 -- > src/libcamera/pipeline_handler.cpp | 34 +++++++++++++------ > 3 files changed, 25 insertions(+), 14 deletions(-) > > diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h > index 18f5c32a18e4..8a2e9ed5894d 100644 > --- a/include/libcamera/internal/camera.h > +++ b/include/libcamera/internal/camera.h > @@ -10,6 +10,7 @@ > #include <atomic> > #include <list> > #include <memory> > +#include <queue> > #include <set> > #include <stdint.h> > #include <string> > @@ -36,6 +37,7 @@ public: > const PipelineHandler *pipe() const { return pipe_.get(); } > > std::list<Request *> queuedRequests_; > + std::queue<Request *> waitingRequests_; > ControlInfoMap controlInfo_; > ControlList properties_; > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > index 972a2fa65310..dedc29e815fb 100644 > --- a/include/libcamera/internal/pipeline_handler.h > +++ b/include/libcamera/internal/pipeline_handler.h > @@ -8,7 +8,6 @@ > #pragma once > > #include <memory> > -#include <queue> > #include <string> > #include <sys/types.h> > #include <vector> > @@ -94,8 +93,6 @@ private: > std::vector<std::shared_ptr<MediaDevice>> mediaDevices_; > std::vector<std::weak_ptr<Camera>> cameras_; > > - std::queue<Request *> waitingRequests_; > - > const char *name_; > unsigned int useCount_; > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index d84dff3c9f19..14d8602e67d8 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -363,15 +363,16 @@ void PipelineHandler::stop(Camera *camera) > /* Stop the pipeline handler and let the queued requests complete. */ > stopDevice(camera); > > + Camera::Private *data = camera->_d(); > + > /* Cancel and signal as complete all waiting requests. */ > - while (!waitingRequests_.empty()) { Yikes, that was plain wrong wasn't it - it would cancel another cameras queued requests if two cameras were running in parallel from the same pipeline (which may not be common, but could be possible). So this definitely seems like a worth while change. Possibly even a fixes tag, but I won't dwell on that. > - Request *request = waitingRequests_.front(); > - waitingRequests_.pop(); > + while (!data->waitingRequests_.empty()) { > + Request *request = data->waitingRequests_.front(); > + data->waitingRequests_.pop(); > cancelRequest(request); > } > > /* Make sure no requests are pending. */ > - Camera::Private *data = camera->_d(); > ASSERT(data->queuedRequests_.empty()); There's so much operating on the Camera::Private *data here I wonder if there should be a Camera::Private::Stop() ... but I'm not sure that would be helpful - we probably want to keep the Camera::Private as a simple structure. > > data->requestSequence_ = 0; > @@ -444,7 +445,9 @@ void PipelineHandler::queueRequest(Request *request) > { > LIBCAMERA_TRACEPOINT(request_queue, request); > > - waitingRequests_.push(request); > + Camera *camera = request->_d()->camera(); > + Camera::Private *data = camera->_d(); > + data->waitingRequests_.push(request); > > request->_d()->prepare(300ms); > } > @@ -480,13 +483,20 @@ void PipelineHandler::doQueueRequest(Request *request) > */ > void PipelineHandler::doQueueRequests() > { > - while (!waitingRequests_.empty()) { > - Request *request = waitingRequests_.front(); > - if (!request->_d()->prepared_) > - break; > + for (const std::weak_ptr<Camera> &ptr : cameras_) { > + std::shared_ptr<Camera> camera = ptr.lock(); > + if (!camera) > + continue; > > - doQueueRequest(request); > - waitingRequests_.pop(); > + Camera::Private *data = camera->_d(); > + while (!data->waitingRequests_.empty()) { > + Request *request = data->waitingRequests_.front(); > + if (!request->_d()->prepared_) > + break; > + > + doQueueRequest(request); > + data->waitingRequests_.pop(); > + } > } > } > > @@ -562,6 +572,8 @@ void PipelineHandler::completeRequest(Request *request) > data->queuedRequests_.pop_front(); > camera->requestComplete(req); > } > + /* Allow any waiting requests to be queued to the pipeline. */ > + doQueueRequests(); I think I saw a later reply also suggest but perhaps: doQueueRequests(camera); could simplify - so that PipelineHandler::doQueueRequests() doesn't iterate over cameras which are not operating... At least being explicit would help make sure we don't have bugs that cameras only wake up and continue because of an event on a different camera ... ... unless that's desired behaviour? > } > > /** > -- > 2.43.0 >
diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h index 18f5c32a18e4..8a2e9ed5894d 100644 --- a/include/libcamera/internal/camera.h +++ b/include/libcamera/internal/camera.h @@ -10,6 +10,7 @@ #include <atomic> #include <list> #include <memory> +#include <queue> #include <set> #include <stdint.h> #include <string> @@ -36,6 +37,7 @@ public: const PipelineHandler *pipe() const { return pipe_.get(); } std::list<Request *> queuedRequests_; + std::queue<Request *> waitingRequests_; ControlInfoMap controlInfo_; ControlList properties_; diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h index 972a2fa65310..dedc29e815fb 100644 --- a/include/libcamera/internal/pipeline_handler.h +++ b/include/libcamera/internal/pipeline_handler.h @@ -8,7 +8,6 @@ #pragma once #include <memory> -#include <queue> #include <string> #include <sys/types.h> #include <vector> @@ -94,8 +93,6 @@ private: std::vector<std::shared_ptr<MediaDevice>> mediaDevices_; std::vector<std::weak_ptr<Camera>> cameras_; - std::queue<Request *> waitingRequests_; - const char *name_; unsigned int useCount_; diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index d84dff3c9f19..14d8602e67d8 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -363,15 +363,16 @@ void PipelineHandler::stop(Camera *camera) /* Stop the pipeline handler and let the queued requests complete. */ stopDevice(camera); + Camera::Private *data = camera->_d(); + /* Cancel and signal as complete all waiting requests. */ - while (!waitingRequests_.empty()) { - Request *request = waitingRequests_.front(); - waitingRequests_.pop(); + while (!data->waitingRequests_.empty()) { + Request *request = data->waitingRequests_.front(); + data->waitingRequests_.pop(); cancelRequest(request); } /* Make sure no requests are pending. */ - Camera::Private *data = camera->_d(); ASSERT(data->queuedRequests_.empty()); data->requestSequence_ = 0; @@ -444,7 +445,9 @@ void PipelineHandler::queueRequest(Request *request) { LIBCAMERA_TRACEPOINT(request_queue, request); - waitingRequests_.push(request); + Camera *camera = request->_d()->camera(); + Camera::Private *data = camera->_d(); + data->waitingRequests_.push(request); request->_d()->prepare(300ms); } @@ -480,13 +483,20 @@ void PipelineHandler::doQueueRequest(Request *request) */ void PipelineHandler::doQueueRequests() { - while (!waitingRequests_.empty()) { - Request *request = waitingRequests_.front(); - if (!request->_d()->prepared_) - break; + for (const std::weak_ptr<Camera> &ptr : cameras_) { + std::shared_ptr<Camera> camera = ptr.lock(); + if (!camera) + continue; - doQueueRequest(request); - waitingRequests_.pop(); + Camera::Private *data = camera->_d(); + while (!data->waitingRequests_.empty()) { + Request *request = data->waitingRequests_.front(); + if (!request->_d()->prepared_) + break; + + doQueueRequest(request); + data->waitingRequests_.pop(); + } } } @@ -562,6 +572,8 @@ void PipelineHandler::completeRequest(Request *request) data->queuedRequests_.pop_front(); camera->requestComplete(req); } + + doQueueRequests(); } /**
The waiting requests of one camera should not be able to influence queuing to another camera. Therefore change the single waitingRequests_ queue into one queue per camera. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- include/libcamera/internal/camera.h | 2 ++ include/libcamera/internal/pipeline_handler.h | 3 -- src/libcamera/pipeline_handler.cpp | 34 +++++++++++++------ 3 files changed, 25 insertions(+), 14 deletions(-)