Jmondi/rkisp1 ipu3 ipa/kbingham rfc/v2 by jmondi · Pull Request #2 · jmondi/libcamera

jmondi · 2022-08-04T15:15:39Z

No description provided.

Add error flags to the Request to indicate non-fatal errors. Applications should check the error() state of the Request to determine if any fault occured while processing the request. Initially, a single error flag ControlError is added to allow a pipeline handler to report a failure to set controls on a hardware device while processing the request. ControlErrors occur when a Request was asked to set a control and the pipeline handler attempted to do so, but it was rejected by the kernel. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Address review comments from Umang and - Use Request::ErrorId and Request::Errors - Reword documentation of enum members as suggested - Rework documentation of Request::errors() - Update documentation of Request::status() to remove the term 'errors' from there Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Report an error when failing to process controls, but still allow the request to process and complete where possible. The Request ControlError flag is raised on the request. Bug: https://bugs.libcamera.org/show_bug.cgi?id=135 Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Provide a new flag for the Request error state to indicate that a per-frame control error has occurred. One or more controls set for this request could not be guaranteed on this frame. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Provide a direct FCQueue abstraction to facilitate creating a queue of Frame Context structures. The IPAFrameContext is moved back to the ipu3/ipa_context.h and a queue added to the IPU3, but the Algorithms are not yet moved from the single FrameContext exposed by the IPA context to use the correct one from the queue until the Algorithm interfaces are fully updated. Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Doing the same at IPA::start() could conflict with queueRequest().

Apply review comments on documentation and FCQueue implementation.

Harden the error condition in the case the desired frame context has been forever lost as too many requests has been queued by the pipeline handler. As this is a programming error, it is fair to ASSERT() on it. While at it expand the comment on the opposite case, where a request underrun makes the queue overflow and record that we might possibily want to notify that to applications. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Move the existing frame context structure to the IPAActiveState. This structure should store the most up to date results for the IPA which may be shared to other algorithms that operate on the data. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Provide a common IPAFrameContext as a base for IPA modules to inherit from. This will allow having a common set of parameters for every FrameContext. We still have to use a templated instance of the FCQueue though to make sure we allocate the correct types. Both the RKISP1 and the IPU3 create IPA specific FrameContext structures which inherit from the IPAFrameContext. While adjusting interface to Algorithm::process() the FrameContext is converted from a pointer to a reference. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Establish a queue of FrameContexts using the new FCQueue and use it to supply the FrameContext to the algorithms. The algorithms on the RKISP1 do not use this yet themselves, but are able to do so after the introduction of this patch. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Pass the current frame number, and the current FrameContext for calls to prepare. Until there is a Frame Context Queue in the RKISP1 the FrameContext will be a nullptr, and must not be dereferenced until the queue is implemented. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> - Obtain and use the RKISP1FrameContext Queue entry for IPARkISP1::fillParamsBuffer Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Pass the frame number of the current frame being processed. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> - Use RKISP1FrameContext from the queue during IPARkISP1::processStatsBuffer Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Pass the current FrameContext for calls to queueRequest(). Now that both the IPU3 and RkISP1 IPA modules comply with the same interface, propagate the queueRequest() call to algorithms in the IPU3 IPA module. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

kbingham · 2022-08-04T16:00:11Z

 		 * Do not re-initialise if a get() call has already fetched this
 		 * frame context to preseve the error flags already raised.
+		 *
+		 * \todo If the the sequence number of the context to initialise


Doesn't it simply mean that there was an underrun and an algorithm tried to obtain a FrameContext that wasn't yet queued?

almost: in an underrurn, where algorithms call "get(x)" before "initialise(x)" has been called we would have that "frame == slot.frame". If instead "frame < slot.frame" it means that the queue has been overwritten (ie get() has been called more than 16 times after the last queueRequest()) and the context we wanted is forever lost. Does this deserve an error ?

Ah right - of course- I see now.
I think it might help to be clearer if we break the conditions apart yes.

so during intiialise we have:

if (frame > frameContext.frame) { // All is as expected. Initialising a new frame context. } else if (frame == frameContext.frame) { // AHA! An algorithm called 'get' before we called initiailse here... } else if (frame < frameContext.frame) { // Complete FCQ Overflow. This initialise call for queueRequest is now so far behind that it's // going to store data in an FC for a *future* frame, (incorrectly). // That future frame we already know is in the PFCError state - (because we're only just queuing the request now) // We still need to process the controls in the request, but now - we know that even before we catch // up - this will already get overwritten again. // By definition - there can't be any frame that will complete to access // the metadata for frame, so that doesn't matter so much - but we must still make sure // that we return (as correct as possible) the metadata for the ('future') frame. // There's definitely scope for things to be wrong here in the metadata being returned. // So I'd probably raise a MetadataError flag on this FCQ entry! // Even if we don't use that flag on the Request, it means we can prevent setting incorrect // metadata (leave it blank) in the Request. }

almost: in an underrurn, where algorithms have "get(x)" before "initialise(x)" has been called we would have that "frame == slot.frame". If instead "frame < slot.frame" it means that we have queue has been overwritten (ie get() has been called more than 16 times after the last queueRequest()) and the context we wanted is forever lost. Does this deserve an error ?

I think so. We atleast need to be warned about all underruns and overruns happening on the queue itself.

Observing in detail, the commit message is correct on the lines that PH can overrun the queue and it is better to ASSERT such situation. On the other than, the way get() is currently implemented, algorithms can overrun the queue to a scale that there is a "induced" underrun. Such underrun is not from the application side but just for the fact that algorithms kept calling get() with such a look-ahead (for e.g. as you mentionde 16 times) and a legit queueRequest() will now look like a 'under-run'.

Which brings to my point, that we might need to think about algorithms not to mess up the queue as well, similar to the over-queuing checks in pipeline-handler.

if an algorithm has called a look ahead for a frame which is 16 frames ahead of the most recent request number - then the request is late.
Algorithms can not peek ahead greater than the pipeline depth - and they should never peek ahead greater than the maximum sensor latency.

That means the only reason an algorithm peeks an entry in the queue is because there is a hardware event generating that - and that means the request are underrunning. - So it's not possible for a 'legitimate' request to 'appear' to be underrunning. It either is an underrun - or it is on time.

Ah right - of course- I see now. I think it might help to be clearer if we break the conditions apart yes.

so during intiialise we have:

if (frame > frameContext.frame) { // All is as expected. Initialising a new frame context. } else if (frame == frameContext.frame) { // AHA! An algorithm called 'get' before we called initiailse here... } else if (frame < frameContext.frame) { // Complete FCQ Overflow. This initialise call for queueRequest is now so far behind that it's // going to store data in an FC for a *future* frame, (incorrectly). // That future frame we already know is in the PFCError state - (because we're only just queuing the request now) // We still need to process the controls in the request, but now - we know that even before we catch // up - this will already get overwritten again. // By definition - there can't be any frame that will complete to access // the metadata for frame, so that doesn't matter so much - but we must still make sure // that we return (as correct as possible) the metadata for the ('future') frame. // There's definitely scope for things to be wrong here in the metadata being returned. // So I'd probably raise a MetadataError flag on this FCQ entry! // Even if we don't use that flag on the Request, it means we can prevent setting incorrect // metadata (leave it blank) in the Request. }

I have not broken this part out in v2 as it seems we are not sure yet what to do here. We can resume the discussion on the patch on the list maybe ?

kbingham · 2022-08-04T16:02:43Z

 int IPARkISP1::start()
 {
+	/* Clean the IPA context before starting the streaming session. */
+	context_ = {};


I worry about this, as requests could be queued before start is called I think...

I thought I moved it to stop() indeed :(

ah indeed, has been reintroduced in 97d07ed
I'll remove it

Edit: That's not true, it's a bad fixup and I'll change it

I worry about this, as requests could be queued before start is called I think...

hmm so this a FCQueue concern right? does context_ = {} clears out FCQueue too? I am guessing it doesn't.

We can choose an alternative by context_ = {} in configure() and context_ = {} in stop(). I think that will avoid having context_ = {} in start()

@uajain don't worry, that has been reintroduced by mistake.

The intended implementation looks like:

void IPARkISP1::stop() { /* Clean the IPA context at the end of the streaming session. */ context_.frameContexts.clear(); context_ = {}; }

Do you think we need to cleanup in "configure()" ?
The only case for that is a user that calls Camera::configure(), then queue requests, then calls configure() again instead of starting a streaming session.

It can be dropped from Camera::configure()

sorry, I keep getting confused... to queueRequest a camera should be started,

int Camera::queueRequest(Request *request) { int ret = d->isAccessAllowed(Private::CameraRunning); if (ret < 0) return ret; }

there's no such a use case as the one I described above, hence the cleanup can be done in stop() and that's enough

Aha - ok - so requests can not be prequeued before calling start()! So much for filling the request queue sufficiently for PFC at startup? (but that's separate from here.

kbingham · 2022-08-04T16:04:24Z

 	};

-	enum ErrorFlag {
+	enum ErrorId {


I would still call this ErrorFlag
Each one is a flag that can be set.

makes indeed sense, I'll change it back

Rahi374 and others added 15 commits August 4, 2022 16:06

fixup: Address Umang's review comments

4d3364d

fixup: Clean context at IPA::stop()

e9e354a

Doing the same at IPA::start() could conflict with queueRequest().

fixup: Apply minor review comments

db69b09

Apply review comments on documentation and FCQueue implementation.

kbingham reviewed Aug 4, 2022

View reviewed changes

Comment thread include/libcamera/request.h

Conversation

jmondi commented Aug 4, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmondi Aug 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmondi Aug 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmondi Aug 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jmondi Aug 5, 2022 •

edited

Loading

jmondi Aug 5, 2022 •

edited

Loading

jmondi Aug 5, 2022 •

edited

Loading