| Message ID | 20220603132259.188845-4-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, On Fri, Jun 03, 2022 at 03:22:58PM +0200, Umang Jain via libcamera-devel wrote: > The IPAIPU3 processes each IPAFrameContext present in the FCQueue > ring buffer. If the application queues the requests at a pace where > IPAFrameContext(s) can get potentially over-written without getting > processed by the IPA, it would lead to un-desirable effects. > > Hence, inspect the FCQueue if it is full and reject any further > queuing of requests. This requires changes to the IPAIPU3 mojom > interface. The IPAIPU3::queueRequest() cannot be [async] anymore > since we need to return a success/failure value. > > The rejection by the IPA doesn't mean it cannot be queued > later. The request is still preserved in > IPU3CameraData::pendingRequests_. It shall be queued sequentially > on subsequent calls to IPU3CameraData::queuePendingRequests(). > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > I am not super-happy with changing the interface and dropping [async] > but this is what I've for now. Would appreciate any other ideas... I feel like this change handles the issue at the very bottom of the call stack, once a request has been queued from the ph to the IPA. I wonder if we shouldn't try to tackle the problem from the top instead, which means at the time a request is queued to the camera by the application. The lenght of the FCQueue represents the maximum number of requests in-flight at a time, iow the number of requests queued to the camera and not yet completed. Would it make sense to make this a global camera property and have the base pipeline handler class defer queueing requests when too many are in-flight ? Thanks j > --- > include/libcamera/ipa/ipu3.mojom | 2 +- > src/ipa/ipu3/ipu3.cpp | 11 +++++++++-- > src/libcamera/pipeline/ipu3/ipu3.cpp | 10 +++++++++- > 3 files changed, 19 insertions(+), 4 deletions(-) > > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom > index d1b1c6b8..3faffd9c 100644 > --- a/include/libcamera/ipa/ipu3.mojom > +++ b/include/libcamera/ipa/ipu3.mojom > @@ -30,7 +30,7 @@ interface IPAIPU3Interface { > mapBuffers(array<libcamera.IPABuffer> buffers); > unmapBuffers(array<uint32> ids); > > - [async] queueRequest(uint32 frame, libcamera.ControlList controls); > + queueRequest(uint32 frame, libcamera.ControlList controls) => (int32 ret); > [async] fillParamsBuffer(uint32 frame, uint32 bufferId); > [async] processStatsBuffer(uint32 frame, int64 frameTimestamp, > uint32 bufferId, libcamera.ControlList sensorControls); > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > index 1d6ee515..63aa9b56 100644 > --- a/src/ipa/ipu3/ipu3.cpp > +++ b/src/ipa/ipu3/ipu3.cpp > @@ -145,7 +145,7 @@ public: > void mapBuffers(const std::vector<IPABuffer> &buffers) override; > void unmapBuffers(const std::vector<unsigned int> &ids) override; > > - void queueRequest(const uint32_t frame, const ControlList &controls) override; > + int queueRequest(const uint32_t frame, const ControlList &controls) override; > void fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) override; > void processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp, > const uint32_t bufferId, > @@ -616,11 +616,18 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, > * > * Parse the request to handle any IPA-managed controls that were set from the > * application such as manual sensor settings. > + * > + * \return 0 if successful, -EPERM otherwise. > */ > -void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls) > +int IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls) > { > + if (context_.frameContexts.isFull()) > + return -EPERM; > + > /* \todo Start processing for 'frame' based on 'controls'. */ > *context_.frameContexts.get(frame) = { frame, controls }; > + > + return 0; > } > > /** > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index fd989e61..d69e28d4 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -874,7 +874,15 @@ void IPU3CameraData::queuePendingRequests() > > info->rawBuffer = rawBuffer; > > - ipa_->queueRequest(info->id, request->controls()); > + /* > + * Queueing request to the IPA can fail in cases like over-flowing > + * its queue. Hence, keep the request preserved in pendingRequests_ > + * if a failure occurs. > + */ > + if (ipa_->queueRequest(info->id, request->controls()) != 0) { > + frameInfos_.remove(info); > + break; > + } > > pendingRequests_.pop(); > processingRequests_.push(request); > -- > 2.31.1 >
Hi Jacopo, On 6/10/22 09:49, Jacopo Mondi wrote: > Hi Umang, > > On Fri, Jun 03, 2022 at 03:22:58PM +0200, Umang Jain via libcamera-devel wrote: >> The IPAIPU3 processes each IPAFrameContext present in the FCQueue >> ring buffer. If the application queues the requests at a pace where >> IPAFrameContext(s) can get potentially over-written without getting >> processed by the IPA, it would lead to un-desirable effects. >> >> Hence, inspect the FCQueue if it is full and reject any further >> queuing of requests. This requires changes to the IPAIPU3 mojom >> interface. The IPAIPU3::queueRequest() cannot be [async] anymore >> since we need to return a success/failure value. >> >> The rejection by the IPA doesn't mean it cannot be queued >> later. The request is still preserved in >> IPU3CameraData::pendingRequests_. It shall be queued sequentially >> on subsequent calls to IPU3CameraData::queuePendingRequests(). >> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> I am not super-happy with changing the interface and dropping [async] >> but this is what I've for now. Would appreciate any other ideas... > I feel like this change handles the issue at the very bottom of the > call stack, once a request has been queued from the ph to the IPA. > > I wonder if we shouldn't try to tackle the problem from the top > instead, which means at the time a request is queued to the camera by > the application. > > The lenght of the FCQueue represents the maximum number of requests > in-flight at a time, iow the number of requests queued to the camera > and not yet completed. > > Would it make sense to make this a global camera property and have the > base pipeline handler class defer queueing requests when too many are > in-flight ? In the last call, I brought up this issue. However, it was geared towards a solution to avoid dropping [async] from the mojom interface. The design I have in mind and shared in the call as well is basically that, the PH should be responsible to queue up requests to the IPA - in a way that it doesn't do over-queuing. The way it should be done is - communicating the size of the FCQueue to the PH during init/configure phase (or make a global in mojom interface where it's shared to both parts - not sure yet, I haven't decided). The PH will queue requests to the IPA, increment a counter. We already have a callback from IPA to PH when a frame processing completes - so decrement the counter. The max the counter can reach is FCQueue.size() - beyond that it should just stop queuing and let the request sit in PipelineHandler::pendingRequests_ queue. The queuePendingRequests() is a recurring call on every request queue by the application - so as long as the counter < FCQueue.size() - it will keep queuing and stop when it's > FCQueue.size(). As frames get completed, counter will decrement and queuing shall start again. > Thanks > j > >> --- >> include/libcamera/ipa/ipu3.mojom | 2 +- >> src/ipa/ipu3/ipu3.cpp | 11 +++++++++-- >> src/libcamera/pipeline/ipu3/ipu3.cpp | 10 +++++++++- >> 3 files changed, 19 insertions(+), 4 deletions(-) >> >> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom >> index d1b1c6b8..3faffd9c 100644 >> --- a/include/libcamera/ipa/ipu3.mojom >> +++ b/include/libcamera/ipa/ipu3.mojom >> @@ -30,7 +30,7 @@ interface IPAIPU3Interface { >> mapBuffers(array<libcamera.IPABuffer> buffers); >> unmapBuffers(array<uint32> ids); >> >> - [async] queueRequest(uint32 frame, libcamera.ControlList controls); >> + queueRequest(uint32 frame, libcamera.ControlList controls) => (int32 ret); >> [async] fillParamsBuffer(uint32 frame, uint32 bufferId); >> [async] processStatsBuffer(uint32 frame, int64 frameTimestamp, >> uint32 bufferId, libcamera.ControlList sensorControls); >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp >> index 1d6ee515..63aa9b56 100644 >> --- a/src/ipa/ipu3/ipu3.cpp >> +++ b/src/ipa/ipu3/ipu3.cpp >> @@ -145,7 +145,7 @@ public: >> void mapBuffers(const std::vector<IPABuffer> &buffers) override; >> void unmapBuffers(const std::vector<unsigned int> &ids) override; >> >> - void queueRequest(const uint32_t frame, const ControlList &controls) override; >> + int queueRequest(const uint32_t frame, const ControlList &controls) override; >> void fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) override; >> void processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp, >> const uint32_t bufferId, >> @@ -616,11 +616,18 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, >> * >> * Parse the request to handle any IPA-managed controls that were set from the >> * application such as manual sensor settings. >> + * >> + * \return 0 if successful, -EPERM otherwise. >> */ >> -void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls) >> +int IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls) >> { >> + if (context_.frameContexts.isFull()) >> + return -EPERM; >> + >> /* \todo Start processing for 'frame' based on 'controls'. */ >> *context_.frameContexts.get(frame) = { frame, controls }; >> + >> + return 0; >> } >> >> /** >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp >> index fd989e61..d69e28d4 100644 >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp >> @@ -874,7 +874,15 @@ void IPU3CameraData::queuePendingRequests() >> >> info->rawBuffer = rawBuffer; >> >> - ipa_->queueRequest(info->id, request->controls()); >> + /* >> + * Queueing request to the IPA can fail in cases like over-flowing >> + * its queue. Hence, keep the request preserved in pendingRequests_ >> + * if a failure occurs. >> + */ >> + if (ipa_->queueRequest(info->id, request->controls()) != 0) { >> + frameInfos_.remove(info); >> + break; >> + } >> >> pendingRequests_.pop(); >> processingRequests_.push(request); >> -- >> 2.31.1 >>
Hi Umang, On Fri, Jun 10, 2022 at 10:32:03AM +0200, Umang Jain wrote: > Hi Jacopo, > > On 6/10/22 09:49, Jacopo Mondi wrote: > > Hi Umang, > > > > On Fri, Jun 03, 2022 at 03:22:58PM +0200, Umang Jain via libcamera-devel wrote: > > > The IPAIPU3 processes each IPAFrameContext present in the FCQueue > > > ring buffer. If the application queues the requests at a pace where > > > IPAFrameContext(s) can get potentially over-written without getting > > > processed by the IPA, it would lead to un-desirable effects. > > > > > > Hence, inspect the FCQueue if it is full and reject any further > > > queuing of requests. This requires changes to the IPAIPU3 mojom > > > interface. The IPAIPU3::queueRequest() cannot be [async] anymore > > > since we need to return a success/failure value. > > > > > > The rejection by the IPA doesn't mean it cannot be queued > > > later. The request is still preserved in > > > IPU3CameraData::pendingRequests_. It shall be queued sequentially > > > on subsequent calls to IPU3CameraData::queuePendingRequests(). > > > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > > --- > > > I am not super-happy with changing the interface and dropping [async] > > > but this is what I've for now. Would appreciate any other ideas... > > I feel like this change handles the issue at the very bottom of the > > call stack, once a request has been queued from the ph to the IPA. > > > > I wonder if we shouldn't try to tackle the problem from the top > > instead, which means at the time a request is queued to the camera by > > the application. > > > > The lenght of the FCQueue represents the maximum number of requests > > in-flight at a time, iow the number of requests queued to the camera > > and not yet completed. > > > > Would it make sense to make this a global camera property and have the > > base pipeline handler class defer queueing requests when too many are > > in-flight ? > > > In the last call, I brought up this issue. However, it was geared towards a > solution to avoid dropping [async] from the mojom interface. I was clearly distracted, sorry > > The design I have in mind and shared in the call as well is basically that, > the PH should be responsible to queue up requests to the IPA - in a way that > it doesn't do over-queuing. > > The way it should be done is - communicating the size of the FCQueue to the > PH during init/configure phase (or make a global in mojom interface where > it's shared to both parts - not sure yet, I haven't decided). The PH will > queue requests to the IPA, increment a counter. We already have a callback > from IPA to PH when a frame processing completes - so decrement the counter. > The max the counter can reach is FCQueue.size() - beyond that it should just > stop queuing and let the request sit in PipelineHandler::pendingRequests_ > queue. The queuePendingRequests() is a recurring call on every request queue > by the application - so as long as the counter < FCQueue.size() - it will > keep queuing and stop when it's > FCQueue.size(). As frames get completed, > counter will decrement and queuing shall start again. I agree on the principle, but I wonder if this couldn't be done at the base PipelineHandler class level, where we have a waitingRequests_ queue already. Right now it only serves to handle waiting on fences, but it could be probably be instrumented to also take a max number of requests in flight counter. If the length of FCQ can be made a property of the camera system (not sure if a proper application visibile libcamera::property or else) and the parameter can be shared with the PH base class (it could even be a base class member each derived PH class have to override as an opt-in mechanism) the PipelineHandler::doQueueRequests() function can be made aware of this. We have PipelineHandler::completeRequest() as well, which is the request completion point where the counter could be decreased and waiting requests queued again. > > > Thanks > > j > > > > > --- > > > include/libcamera/ipa/ipu3.mojom | 2 +- > > > src/ipa/ipu3/ipu3.cpp | 11 +++++++++-- > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 10 +++++++++- > > > 3 files changed, 19 insertions(+), 4 deletions(-) > > > > > > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom > > > index d1b1c6b8..3faffd9c 100644 > > > --- a/include/libcamera/ipa/ipu3.mojom > > > +++ b/include/libcamera/ipa/ipu3.mojom > > > @@ -30,7 +30,7 @@ interface IPAIPU3Interface { > > > mapBuffers(array<libcamera.IPABuffer> buffers); > > > unmapBuffers(array<uint32> ids); > > > > > > - [async] queueRequest(uint32 frame, libcamera.ControlList controls); > > > + queueRequest(uint32 frame, libcamera.ControlList controls) => (int32 ret); > > > [async] fillParamsBuffer(uint32 frame, uint32 bufferId); > > > [async] processStatsBuffer(uint32 frame, int64 frameTimestamp, > > > uint32 bufferId, libcamera.ControlList sensorControls); > > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > > > index 1d6ee515..63aa9b56 100644 > > > --- a/src/ipa/ipu3/ipu3.cpp > > > +++ b/src/ipa/ipu3/ipu3.cpp > > > @@ -145,7 +145,7 @@ public: > > > void mapBuffers(const std::vector<IPABuffer> &buffers) override; > > > void unmapBuffers(const std::vector<unsigned int> &ids) override; > > > > > > - void queueRequest(const uint32_t frame, const ControlList &controls) override; > > > + int queueRequest(const uint32_t frame, const ControlList &controls) override; > > > void fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) override; > > > void processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp, > > > const uint32_t bufferId, > > > @@ -616,11 +616,18 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, > > > * > > > * Parse the request to handle any IPA-managed controls that were set from the > > > * application such as manual sensor settings. > > > + * > > > + * \return 0 if successful, -EPERM otherwise. > > > */ > > > -void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls) > > > +int IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls) > > > { > > > + if (context_.frameContexts.isFull()) > > > + return -EPERM; > > > + > > > /* \todo Start processing for 'frame' based on 'controls'. */ > > > *context_.frameContexts.get(frame) = { frame, controls }; > > > + > > > + return 0; > > > } > > > > > > /** > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > index fd989e61..d69e28d4 100644 > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > @@ -874,7 +874,15 @@ void IPU3CameraData::queuePendingRequests() > > > > > > info->rawBuffer = rawBuffer; > > > > > > - ipa_->queueRequest(info->id, request->controls()); > > > + /* > > > + * Queueing request to the IPA can fail in cases like over-flowing > > > + * its queue. Hence, keep the request preserved in pendingRequests_ > > > + * if a failure occurs. > > > + */ > > > + if (ipa_->queueRequest(info->id, request->controls()) != 0) { > > > + frameInfos_.remove(info); > > > + break; > > > + } > > > > > > pendingRequests_.pop(); > > > processingRequests_.push(request); > > > -- > > > 2.31.1 > > >
Hi Jacopo, On 6/10/22 10:52, Jacopo Mondi wrote: > Hi Umang, > > On Fri, Jun 10, 2022 at 10:32:03AM +0200, Umang Jain wrote: >> Hi Jacopo, >> >> On 6/10/22 09:49, Jacopo Mondi wrote: >>> Hi Umang, >>> >>> On Fri, Jun 03, 2022 at 03:22:58PM +0200, Umang Jain via libcamera-devel wrote: >>>> The IPAIPU3 processes each IPAFrameContext present in the FCQueue >>>> ring buffer. If the application queues the requests at a pace where >>>> IPAFrameContext(s) can get potentially over-written without getting >>>> processed by the IPA, it would lead to un-desirable effects. >>>> >>>> Hence, inspect the FCQueue if it is full and reject any further >>>> queuing of requests. This requires changes to the IPAIPU3 mojom >>>> interface. The IPAIPU3::queueRequest() cannot be [async] anymore >>>> since we need to return a success/failure value. >>>> >>>> The rejection by the IPA doesn't mean it cannot be queued >>>> later. The request is still preserved in >>>> IPU3CameraData::pendingRequests_. It shall be queued sequentially >>>> on subsequent calls to IPU3CameraData::queuePendingRequests(). >>>> >>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >>>> --- >>>> I am not super-happy with changing the interface and dropping [async] >>>> but this is what I've for now. Would appreciate any other ideas... >>> I feel like this change handles the issue at the very bottom of the >>> call stack, once a request has been queued from the ph to the IPA. >>> >>> I wonder if we shouldn't try to tackle the problem from the top >>> instead, which means at the time a request is queued to the camera by >>> the application. >>> >>> The lenght of the FCQueue represents the maximum number of requests >>> in-flight at a time, iow the number of requests queued to the camera >>> and not yet completed. >>> >>> Would it make sense to make this a global camera property and have the >>> base pipeline handler class defer queueing requests when too many are >>> in-flight ? >> >> In the last call, I brought up this issue. However, it was geared towards a >> solution to avoid dropping [async] from the mojom interface. > I was clearly distracted, sorry > >> The design I have in mind and shared in the call as well is basically that, >> the PH should be responsible to queue up requests to the IPA - in a way that >> it doesn't do over-queuing. >> >> The way it should be done is - communicating the size of the FCQueue to the >> PH during init/configure phase (or make a global in mojom interface where >> it's shared to both parts - not sure yet, I haven't decided). The PH will >> queue requests to the IPA, increment a counter. We already have a callback >> from IPA to PH when a frame processing completes - so decrement the counter. >> The max the counter can reach is FCQueue.size() - beyond that it should just >> stop queuing and let the request sit in PipelineHandler::pendingRequests_ >> queue. The queuePendingRequests() is a recurring call on every request queue >> by the application - so as long as the counter < FCQueue.size() - it will >> keep queuing and stop when it's > FCQueue.size(). As frames get completed, >> counter will decrement and queuing shall start again. > I agree on the principle, but I wonder if this couldn't be done at the > base PipelineHandler class level, where we have a waitingRequests_ > queue already. Right now it only serves to handle waiting on fences, > but it could be probably be instrumented to also take a max number of > requests in flight counter. > > If the length of FCQ can be made a property of the camera system (not > sure if a proper application visibile libcamera::property or else) and > the parameter can be shared with the PH base class (it could even be a > base class member each derived PH class have to override as an > opt-in mechanism) the PipelineHandler::doQueueRequests() function > can be made aware of this. We have PipelineHandler::completeRequest() > as well, which is the request completion point where the counter could > be decreased and waiting requests queued again. 2 Points: - I haven't had a look at the PH base for this yet. But reading from your explanation (thanks for the details), it seems it should be quite do-able. So yes, we have two "similar" options now - Next points is basically its PH "base". So "generic" stuff - which might apply to other PH and IPAs as well. I don't want to go into doing it "generically" yet but touching the PH base for FCQueue (which is still IPU3 sepcific) seems a violation of the layers to me. So I am a bit resistant going in that direction (for now). One option could be - to implement the solution PipelinehandlerIPU3 class for now and include a \todo which basically summarises your solution in the last reply. When we start moving these parts from the PH / IPA to more "generic" location(s)- we can address that \todo as well. This way we don't have things spread out between specific and generic, I think. The principle in both is similar basically. What do you think ? > >>> Thanks >>> j >>> >>>> --- >>>> include/libcamera/ipa/ipu3.mojom | 2 +- >>>> src/ipa/ipu3/ipu3.cpp | 11 +++++++++-- >>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 10 +++++++++- >>>> 3 files changed, 19 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom >>>> index d1b1c6b8..3faffd9c 100644 >>>> --- a/include/libcamera/ipa/ipu3.mojom >>>> +++ b/include/libcamera/ipa/ipu3.mojom >>>> @@ -30,7 +30,7 @@ interface IPAIPU3Interface { >>>> mapBuffers(array<libcamera.IPABuffer> buffers); >>>> unmapBuffers(array<uint32> ids); >>>> >>>> - [async] queueRequest(uint32 frame, libcamera.ControlList controls); >>>> + queueRequest(uint32 frame, libcamera.ControlList controls) => (int32 ret); >>>> [async] fillParamsBuffer(uint32 frame, uint32 bufferId); >>>> [async] processStatsBuffer(uint32 frame, int64 frameTimestamp, >>>> uint32 bufferId, libcamera.ControlList sensorControls); >>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp >>>> index 1d6ee515..63aa9b56 100644 >>>> --- a/src/ipa/ipu3/ipu3.cpp >>>> +++ b/src/ipa/ipu3/ipu3.cpp >>>> @@ -145,7 +145,7 @@ public: >>>> void mapBuffers(const std::vector<IPABuffer> &buffers) override; >>>> void unmapBuffers(const std::vector<unsigned int> &ids) override; >>>> >>>> - void queueRequest(const uint32_t frame, const ControlList &controls) override; >>>> + int queueRequest(const uint32_t frame, const ControlList &controls) override; >>>> void fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) override; >>>> void processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp, >>>> const uint32_t bufferId, >>>> @@ -616,11 +616,18 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, >>>> * >>>> * Parse the request to handle any IPA-managed controls that were set from the >>>> * application such as manual sensor settings. >>>> + * >>>> + * \return 0 if successful, -EPERM otherwise. >>>> */ >>>> -void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls) >>>> +int IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls) >>>> { >>>> + if (context_.frameContexts.isFull()) >>>> + return -EPERM; >>>> + >>>> /* \todo Start processing for 'frame' based on 'controls'. */ >>>> *context_.frameContexts.get(frame) = { frame, controls }; >>>> + >>>> + return 0; >>>> } >>>> >>>> /** >>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp >>>> index fd989e61..d69e28d4 100644 >>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp >>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp >>>> @@ -874,7 +874,15 @@ void IPU3CameraData::queuePendingRequests() >>>> >>>> info->rawBuffer = rawBuffer; >>>> >>>> - ipa_->queueRequest(info->id, request->controls()); >>>> + /* >>>> + * Queueing request to the IPA can fail in cases like over-flowing >>>> + * its queue. Hence, keep the request preserved in pendingRequests_ >>>> + * if a failure occurs. >>>> + */ >>>> + if (ipa_->queueRequest(info->id, request->controls()) != 0) { >>>> + frameInfos_.remove(info); >>>> + break; >>>> + } >>>> >>>> pendingRequests_.pop(); >>>> processingRequests_.push(request); >>>> -- >>>> 2.31.1 >>>>
Hi Umang, On Fri, Jun 10, 2022 at 11:03:35AM +0200, Umang Jain wrote: > Hi Jacopo, > > On 6/10/22 10:52, Jacopo Mondi wrote: > > Hi Umang, > > > > On Fri, Jun 10, 2022 at 10:32:03AM +0200, Umang Jain wrote: > > > Hi Jacopo, > > > > > > On 6/10/22 09:49, Jacopo Mondi wrote: > > > > Hi Umang, > > > > > > > > On Fri, Jun 03, 2022 at 03:22:58PM +0200, Umang Jain via libcamera-devel wrote: > > > > > The IPAIPU3 processes each IPAFrameContext present in the FCQueue > > > > > ring buffer. If the application queues the requests at a pace where > > > > > IPAFrameContext(s) can get potentially over-written without getting > > > > > processed by the IPA, it would lead to un-desirable effects. > > > > > > > > > > Hence, inspect the FCQueue if it is full and reject any further > > > > > queuing of requests. This requires changes to the IPAIPU3 mojom > > > > > interface. The IPAIPU3::queueRequest() cannot be [async] anymore > > > > > since we need to return a success/failure value. > > > > > > > > > > The rejection by the IPA doesn't mean it cannot be queued > > > > > later. The request is still preserved in > > > > > IPU3CameraData::pendingRequests_. It shall be queued sequentially > > > > > on subsequent calls to IPU3CameraData::queuePendingRequests(). > > > > > > > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > > > > --- > > > > > I am not super-happy with changing the interface and dropping [async] > > > > > but this is what I've for now. Would appreciate any other ideas... > > > > I feel like this change handles the issue at the very bottom of the > > > > call stack, once a request has been queued from the ph to the IPA. > > > > > > > > I wonder if we shouldn't try to tackle the problem from the top > > > > instead, which means at the time a request is queued to the camera by > > > > the application. > > > > > > > > The lenght of the FCQueue represents the maximum number of requests > > > > in-flight at a time, iow the number of requests queued to the camera > > > > and not yet completed. > > > > > > > > Would it make sense to make this a global camera property and have the > > > > base pipeline handler class defer queueing requests when too many are > > > > in-flight ? > > > > > > In the last call, I brought up this issue. However, it was geared towards a > > > solution to avoid dropping [async] from the mojom interface. > > I was clearly distracted, sorry > > > > > The design I have in mind and shared in the call as well is basically that, > > > the PH should be responsible to queue up requests to the IPA - in a way that > > > it doesn't do over-queuing. > > > > > > The way it should be done is - communicating the size of the FCQueue to the > > > PH during init/configure phase (or make a global in mojom interface where > > > it's shared to both parts - not sure yet, I haven't decided). The PH will > > > queue requests to the IPA, increment a counter. We already have a callback > > > from IPA to PH when a frame processing completes - so decrement the counter. > > > The max the counter can reach is FCQueue.size() - beyond that it should just > > > stop queuing and let the request sit in PipelineHandler::pendingRequests_ > > > queue. The queuePendingRequests() is a recurring call on every request queue > > > by the application - so as long as the counter < FCQueue.size() - it will > > > keep queuing and stop when it's > FCQueue.size(). As frames get completed, > > > counter will decrement and queuing shall start again. > > I agree on the principle, but I wonder if this couldn't be done at the > > base PipelineHandler class level, where we have a waitingRequests_ > > queue already. Right now it only serves to handle waiting on fences, > > but it could be probably be instrumented to also take a max number of > > requests in flight counter. > > > > If the length of FCQ can be made a property of the camera system (not > > sure if a proper application visibile libcamera::property or else) and > > the parameter can be shared with the PH base class (it could even be a > > base class member each derived PH class have to override as an > > opt-in mechanism) the PipelineHandler::doQueueRequests() function > > can be made aware of this. We have PipelineHandler::completeRequest() > > as well, which is the request completion point where the counter could > > be decreased and waiting requests queued again. > > > 2 Points: > > - I haven't had a look at the PH base for this yet. But reading from your > explanation (thanks for the details), it seems it should be quite do-able. > So yes, we have two "similar" options now > > - Next points is basically its PH "base". So "generic" stuff - which might > apply to other PH and IPAs as well. I don't want to go into doing it > "generically" yet but touching the PH base for FCQueue (which is still IPU3 > sepcific) seems a violation of the layers to me. So I am a bit resistant > going in that direction (for now). I do expect all pipeline handlers would potentially be interested in exposing a max number of in-flight requests and to have a mechanism that allows them automatically be guaranteed not to receive more, even more if they don't have to do anything to have that realized :) For IPU3 this is currently the size of the FCQ too, for other platforms might be different and could simply be set to the number of buffers requested on the video device. > > One option could be - to implement the solution PipelinehandlerIPU3 class > for now and include a \todo which basically summarises your solution in the > last reply. When we start moving these parts from the PH / IPA to more > "generic" location(s)- we can address that \todo as well. This way we don't > have things spread out between specific and generic, I think. The principle > in both is similar basically. What do you think ? > Doing the work in the IPU3 ph to then have to re-implement it in the base class seems like a work duplication to me, if the change can be self-contained and implemented in the base class already with the same effort. The mechanism could be made opt-in, conditional to the presence of a camera property or on the value of a base class member field which derived ph will override. I would be happy to help if you want this as not part of this series. > > > > > > Thanks > > > > j > > > > > > > > > --- > > > > > include/libcamera/ipa/ipu3.mojom | 2 +- > > > > > src/ipa/ipu3/ipu3.cpp | 11 +++++++++-- > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 10 +++++++++- > > > > > 3 files changed, 19 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom > > > > > index d1b1c6b8..3faffd9c 100644 > > > > > --- a/include/libcamera/ipa/ipu3.mojom > > > > > +++ b/include/libcamera/ipa/ipu3.mojom > > > > > @@ -30,7 +30,7 @@ interface IPAIPU3Interface { > > > > > mapBuffers(array<libcamera.IPABuffer> buffers); > > > > > unmapBuffers(array<uint32> ids); > > > > > > > > > > - [async] queueRequest(uint32 frame, libcamera.ControlList controls); > > > > > + queueRequest(uint32 frame, libcamera.ControlList controls) => (int32 ret); > > > > > [async] fillParamsBuffer(uint32 frame, uint32 bufferId); > > > > > [async] processStatsBuffer(uint32 frame, int64 frameTimestamp, > > > > > uint32 bufferId, libcamera.ControlList sensorControls); > > > > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > > > > > index 1d6ee515..63aa9b56 100644 > > > > > --- a/src/ipa/ipu3/ipu3.cpp > > > > > +++ b/src/ipa/ipu3/ipu3.cpp > > > > > @@ -145,7 +145,7 @@ public: > > > > > void mapBuffers(const std::vector<IPABuffer> &buffers) override; > > > > > void unmapBuffers(const std::vector<unsigned int> &ids) override; > > > > > > > > > > - void queueRequest(const uint32_t frame, const ControlList &controls) override; > > > > > + int queueRequest(const uint32_t frame, const ControlList &controls) override; > > > > > void fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) override; > > > > > void processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp, > > > > > const uint32_t bufferId, > > > > > @@ -616,11 +616,18 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, > > > > > * > > > > > * Parse the request to handle any IPA-managed controls that were set from the > > > > > * application such as manual sensor settings. > > > > > + * > > > > > + * \return 0 if successful, -EPERM otherwise. > > > > > */ > > > > > -void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls) > > > > > +int IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls) > > > > > { > > > > > + if (context_.frameContexts.isFull()) > > > > > + return -EPERM; > > > > > + > > > > > /* \todo Start processing for 'frame' based on 'controls'. */ > > > > > *context_.frameContexts.get(frame) = { frame, controls }; > > > > > + > > > > > + return 0; > > > > > } > > > > > > > > > > /** > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > > > index fd989e61..d69e28d4 100644 > > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > > > @@ -874,7 +874,15 @@ void IPU3CameraData::queuePendingRequests() > > > > > > > > > > info->rawBuffer = rawBuffer; > > > > > > > > > > - ipa_->queueRequest(info->id, request->controls()); > > > > > + /* > > > > > + * Queueing request to the IPA can fail in cases like over-flowing > > > > > + * its queue. Hence, keep the request preserved in pendingRequests_ > > > > > + * if a failure occurs. > > > > > + */ > > > > > + if (ipa_->queueRequest(info->id, request->controls()) != 0) { > > > > > + frameInfos_.remove(info); > > > > > + break; > > > > > + } > > > > > > > > > > pendingRequests_.pop(); > > > > > processingRequests_.push(request); > > > > > -- > > > > > 2.31.1 > > > > >
Quoting Jacopo Mondi via libcamera-devel (2022-06-10 10:26:48) > Hi Umang, > > On Fri, Jun 10, 2022 at 11:03:35AM +0200, Umang Jain wrote: > > Hi Jacopo, > > > > On 6/10/22 10:52, Jacopo Mondi wrote: > > > Hi Umang, > > > > > > On Fri, Jun 10, 2022 at 10:32:03AM +0200, Umang Jain wrote: > > > > Hi Jacopo, > > > > > > > > On 6/10/22 09:49, Jacopo Mondi wrote: > > > > > Hi Umang, > > > > > > > > > > On Fri, Jun 03, 2022 at 03:22:58PM +0200, Umang Jain via libcamera-devel wrote: > > > > > > The IPAIPU3 processes each IPAFrameContext present in the FCQueue > > > > > > ring buffer. If the application queues the requests at a pace where > > > > > > IPAFrameContext(s) can get potentially over-written without getting > > > > > > processed by the IPA, it would lead to un-desirable effects. > > > > > > > > > > > > Hence, inspect the FCQueue if it is full and reject any further > > > > > > queuing of requests. This requires changes to the IPAIPU3 mojom > > > > > > interface. The IPAIPU3::queueRequest() cannot be [async] anymore > > > > > > since we need to return a success/failure value. > > > > > > > > > > > > The rejection by the IPA doesn't mean it cannot be queued > > > > > > later. The request is still preserved in > > > > > > IPU3CameraData::pendingRequests_. It shall be queued sequentially > > > > > > on subsequent calls to IPU3CameraData::queuePendingRequests(). > > > > > > > > > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > > > > > --- > > > > > > I am not super-happy with changing the interface and dropping [async] > > > > > > but this is what I've for now. Would appreciate any other ideas... > > > > > I feel like this change handles the issue at the very bottom of the > > > > > call stack, once a request has been queued from the ph to the IPA. > > > > > > > > > > I wonder if we shouldn't try to tackle the problem from the top > > > > > instead, which means at the time a request is queued to the camera by > > > > > the application. > > > > > > > > > > The lenght of the FCQueue represents the maximum number of requests > > > > > in-flight at a time, iow the number of requests queued to the camera > > > > > and not yet completed. > > > > > > > > > > Would it make sense to make this a global camera property and have the > > > > > base pipeline handler class defer queueing requests when too many are > > > > > in-flight ? > > > > > > > > In the last call, I brought up this issue. However, it was geared towards a > > > > solution to avoid dropping [async] from the mojom interface. > > > I was clearly distracted, sorry > > > > > > > The design I have in mind and shared in the call as well is basically that, > > > > the PH should be responsible to queue up requests to the IPA - in a way that > > > > it doesn't do over-queuing. > > > > > > > > The way it should be done is - communicating the size of the FCQueue to the > > > > PH during init/configure phase (or make a global in mojom interface where > > > > it's shared to both parts - not sure yet, I haven't decided). The PH will > > > > queue requests to the IPA, increment a counter. We already have a callback > > > > from IPA to PH when a frame processing completes - so decrement the counter. > > > > The max the counter can reach is FCQueue.size() - beyond that it should just > > > > stop queuing and let the request sit in PipelineHandler::pendingRequests_ > > > > queue. The queuePendingRequests() is a recurring call on every request queue > > > > by the application - so as long as the counter < FCQueue.size() - it will > > > > keep queuing and stop when it's > FCQueue.size(). As frames get completed, > > > > counter will decrement and queuing shall start again. > > > I agree on the principle, but I wonder if this couldn't be done at the > > > base PipelineHandler class level, where we have a waitingRequests_ > > > queue already. Right now it only serves to handle waiting on fences, > > > but it could be probably be instrumented to also take a max number of > > > requests in flight counter. > > > > > > If the length of FCQ can be made a property of the camera system (not > > > sure if a proper application visibile libcamera::property or else) and > > > the parameter can be shared with the PH base class (it could even be a > > > base class member each derived PH class have to override as an > > > opt-in mechanism) the PipelineHandler::doQueueRequests() function > > > can be made aware of this. We have PipelineHandler::completeRequest() > > > as well, which is the request completion point where the counter could > > > be decreased and waiting requests queued again. > > > > > > 2 Points: > > > > - I haven't had a look at the PH base for this yet. But reading from your > > explanation (thanks for the details), it seems it should be quite do-able. > > So yes, we have two "similar" options now > > > > - Next points is basically its PH "base". So "generic" stuff - which might > > apply to other PH and IPAs as well. I don't want to go into doing it > > "generically" yet but touching the PH base for FCQueue (which is still IPU3 > > sepcific) seems a violation of the layers to me. So I am a bit resistant > > going in that direction (for now). > > I do expect all pipeline handlers would potentially be interested in > exposing a max number of in-flight requests and to have a mechanism > that allows them automatically be guaranteed not to receive more, even > more if they don't have to do anything to have that realized :) > > For IPU3 this is currently the size of the FCQ too, for other > platforms might be different and could simply be set to the number of > buffers requested on the video device. > > > > > One option could be - to implement the solution PipelinehandlerIPU3 class > > for now and include a \todo which basically summarises your solution in the > > last reply. When we start moving these parts from the PH / IPA to more > > "generic" location(s)- we can address that \todo as well. This way we don't > > have things spread out between specific and generic, I think. The principle > > in both is similar basically. What do you think ? > > > > Doing the work in the IPU3 ph to then have to re-implement it in the > base class seems like a work duplication to me, if the change can be > self-contained and implemented in the base class already with the same > effort. > > The mechanism could be made opt-in, conditional to the presence of a > camera property or on the value of a base class member field which > derived ph will override. > > I would be happy to help if you want this as not part of this series. Preventing overflowing the IPA using the existing queue at the PH sounds quite optimal to me too. Let me know if I can help there in anyway too - or if we need to add it to a discussion point. If we 'know' that we'll prevent overflowing the FCQ, we don't have to handle it in this IPU3 series. I wonder if there are underflow issues still though... -- Kieran > > > > > > > > > Thanks > > > > > j > > > > > > > > > > > --- > > > > > > include/libcamera/ipa/ipu3.mojom | 2 +- > > > > > > src/ipa/ipu3/ipu3.cpp | 11 +++++++++-- > > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 10 +++++++++- > > > > > > 3 files changed, 19 insertions(+), 4 deletions(-) > > > > > > > > > > > > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom > > > > > > index d1b1c6b8..3faffd9c 100644 > > > > > > --- a/include/libcamera/ipa/ipu3.mojom > > > > > > +++ b/include/libcamera/ipa/ipu3.mojom > > > > > > @@ -30,7 +30,7 @@ interface IPAIPU3Interface { > > > > > > mapBuffers(array<libcamera.IPABuffer> buffers); > > > > > > unmapBuffers(array<uint32> ids); > > > > > > > > > > > > - [async] queueRequest(uint32 frame, libcamera.ControlList controls); > > > > > > + queueRequest(uint32 frame, libcamera.ControlList controls) => (int32 ret); > > > > > > [async] fillParamsBuffer(uint32 frame, uint32 bufferId); > > > > > > [async] processStatsBuffer(uint32 frame, int64 frameTimestamp, > > > > > > uint32 bufferId, libcamera.ControlList sensorControls); > > > > > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > > > > > > index 1d6ee515..63aa9b56 100644 > > > > > > --- a/src/ipa/ipu3/ipu3.cpp > > > > > > +++ b/src/ipa/ipu3/ipu3.cpp > > > > > > @@ -145,7 +145,7 @@ public: > > > > > > void mapBuffers(const std::vector<IPABuffer> &buffers) override; > > > > > > void unmapBuffers(const std::vector<unsigned int> &ids) override; > > > > > > > > > > > > - void queueRequest(const uint32_t frame, const ControlList &controls) override; > > > > > > + int queueRequest(const uint32_t frame, const ControlList &controls) override; > > > > > > void fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) override; > > > > > > void processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp, > > > > > > const uint32_t bufferId, > > > > > > @@ -616,11 +616,18 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, > > > > > > * > > > > > > * Parse the request to handle any IPA-managed controls that were set from the > > > > > > * application such as manual sensor settings. > > > > > > + * > > > > > > + * \return 0 if successful, -EPERM otherwise. > > > > > > */ > > > > > > -void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls) > > > > > > +int IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls) > > > > > > { > > > > > > + if (context_.frameContexts.isFull()) > > > > > > + return -EPERM; > > > > > > + > > > > > > /* \todo Start processing for 'frame' based on 'controls'. */ > > > > > > *context_.frameContexts.get(frame) = { frame, controls }; > > > > > > + > > > > > > + return 0; > > > > > > } > > > > > > > > > > > > /** > > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > > > > index fd989e61..d69e28d4 100644 > > > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > > > > @@ -874,7 +874,15 @@ void IPU3CameraData::queuePendingRequests() > > > > > > > > > > > > info->rawBuffer = rawBuffer; > > > > > > > > > > > > - ipa_->queueRequest(info->id, request->controls()); > > > > > > + /* > > > > > > + * Queueing request to the IPA can fail in cases like over-flowing > > > > > > + * its queue. Hence, keep the request preserved in pendingRequests_ > > > > > > + * if a failure occurs. > > > > > > + */ > > > > > > + if (ipa_->queueRequest(info->id, request->controls()) != 0) { > > > > > > + frameInfos_.remove(info); > > > > > > + break; > > > > > > + } > > > > > > > > > > > > pendingRequests_.pop(); > > > > > > processingRequests_.push(request); > > > > > > -- > > > > > > 2.31.1 > > > > > >
diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom index d1b1c6b8..3faffd9c 100644 --- a/include/libcamera/ipa/ipu3.mojom +++ b/include/libcamera/ipa/ipu3.mojom @@ -30,7 +30,7 @@ interface IPAIPU3Interface { mapBuffers(array<libcamera.IPABuffer> buffers); unmapBuffers(array<uint32> ids); - [async] queueRequest(uint32 frame, libcamera.ControlList controls); + queueRequest(uint32 frame, libcamera.ControlList controls) => (int32 ret); [async] fillParamsBuffer(uint32 frame, uint32 bufferId); [async] processStatsBuffer(uint32 frame, int64 frameTimestamp, uint32 bufferId, libcamera.ControlList sensorControls); diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp index 1d6ee515..63aa9b56 100644 --- a/src/ipa/ipu3/ipu3.cpp +++ b/src/ipa/ipu3/ipu3.cpp @@ -145,7 +145,7 @@ public: void mapBuffers(const std::vector<IPABuffer> &buffers) override; void unmapBuffers(const std::vector<unsigned int> &ids) override; - void queueRequest(const uint32_t frame, const ControlList &controls) override; + int queueRequest(const uint32_t frame, const ControlList &controls) override; void fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) override; void processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp, const uint32_t bufferId, @@ -616,11 +616,18 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, * * Parse the request to handle any IPA-managed controls that were set from the * application such as manual sensor settings. + * + * \return 0 if successful, -EPERM otherwise. */ -void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls) +int IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls) { + if (context_.frameContexts.isFull()) + return -EPERM; + /* \todo Start processing for 'frame' based on 'controls'. */ *context_.frameContexts.get(frame) = { frame, controls }; + + return 0; } /** diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index fd989e61..d69e28d4 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -874,7 +874,15 @@ void IPU3CameraData::queuePendingRequests() info->rawBuffer = rawBuffer; - ipa_->queueRequest(info->id, request->controls()); + /* + * Queueing request to the IPA can fail in cases like over-flowing + * its queue. Hence, keep the request preserved in pendingRequests_ + * if a failure occurs. + */ + if (ipa_->queueRequest(info->id, request->controls()) != 0) { + frameInfos_.remove(info); + break; + } pendingRequests_.pop(); processingRequests_.push(request);
The IPAIPU3 processes each IPAFrameContext present in the FCQueue ring buffer. If the application queues the requests at a pace where IPAFrameContext(s) can get potentially over-written without getting processed by the IPA, it would lead to un-desirable effects. Hence, inspect the FCQueue if it is full and reject any further queuing of requests. This requires changes to the IPAIPU3 mojom interface. The IPAIPU3::queueRequest() cannot be [async] anymore since we need to return a success/failure value. The rejection by the IPA doesn't mean it cannot be queued later. The request is still preserved in IPU3CameraData::pendingRequests_. It shall be queued sequentially on subsequent calls to IPU3CameraData::queuePendingRequests(). Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- I am not super-happy with changing the interface and dropping [async] but this is what I've for now. Would appreciate any other ideas... --- include/libcamera/ipa/ipu3.mojom | 2 +- src/ipa/ipu3/ipu3.cpp | 11 +++++++++-- src/libcamera/pipeline/ipu3/ipu3.cpp | 10 +++++++++- 3 files changed, 19 insertions(+), 4 deletions(-)