[PATCH] pipeline: rkisp1: clear pending requests when stopping camera device

[Kieran Bingham]

Quoting Umang Jain (2024-10-25 10:46:42)
> Hello Javier
> 
> On 25/10/24 2:08 pm, Javier Carrasco wrote:
> > If the camera device is stopped, there may still be requests within
> > 'data->queuedRequests_' that are mistakenly never popped. If this is the
> > case, libcamera terminates with the assertion
> > 'ASSERT(data->queuedRequests_.empty())'.
> >
> > Walk over the pending queued requests, destroy the info frames and
> > complete the requests to ensure that no requests are left behind.
> >
> > Co-developed-by: Lukas Riezler <lukas.riezler at wolfvision.net>
> > Signed-off-by: Lukas Riezler <lukas.riezler at wolfvision.net>
> > Signed-off-by: Javier Carrasco <javier.carrasco at wolfvision.net>
> >
> > ---
> > This is my first patch for libcamera, apologies for anything I might
> > have overlooked from the "Submitting Patches" section in the docs.
> >
> > There is already a patch to fix a similar issue for swISP[1], which
> > adresses a bug tracked on Bugzilla[2]. The latter contains some mentions
> > to the possibility of having the same issue for rkisp1, and that is what
> > we are observing on our RK3568-based system. I did not add a 'Bug:' tag
> > as [2] addresses a different pipeline, but the problem seems to be the
> > same.
> >
> > The developers involved in this fix are by no means libcamera experts,
> > and we have tried to follow a similar approach as in [1] by walking over
> > pending requests and completing them before stopDevice() is completed.
> > The proposed patch fixes the issue, but we are open to any suggestions
> > and improvements.
> >
> > Link: https://patchwork.libcamera.org/patch/21537/
> > <https://patchwork.libcamera.org/patch/21537/> [1]
> > Link: https://bugs.libcamera.org/show_bug.cgi?id=234
> > <https://bugs.libcamera.org/show_bug.cgi?id=234> [2]
> > ---
> >    src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 +++++++
> >    1 file changed, 7 insertions(+)
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index c02c7cf3..efa7dbd5 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -997,6 +997,13 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera)
> >                                    << "Failed to stop parameters for " <<
> > camera->id();
> >            }
> >
> > +       while (!data->queuedRequests_.empty()) {
> > +               Request *req = data->queuedRequests_.front();
> > +               RkISP1FrameInfo *info = data->frameInfo_.find(req);
> > +               data->frameInfo_.destroy(info->frame);
> > +               completeRequest(req);

This should 'cancel' the request - not complete it I think.

> > +       }
> > +
> 
> We should never do this - if we need to this, there is clearly a bug 
> somewhere. stop() makes sure all requests are completed (even with 
> status = cancelled)
> 
> Refer to PipelineHandler::stop() documentatio
> 
> >            ASSERT(data->queuedRequests_.empty());
> 
> What I think is, this is a redundant assert and should be dropped from 
> RkISP1PipelineHandler::stopDevice()

Well - it wasn't entirely redundant... Especially if it's caught this
issue earlier than at the end of PipelineHandler::stop()..

> 
> We are already in stopDevice() which is called by 
> PipelineHandler::stop() - which already has this ASSERT
> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline_handler.cpp#n377
> 
> So, we should the drop the ASSERT from rkisp1 pipeline handler and let 
> the base pipeline handler class handle the ASSERT (which it already does).

I think after the while (!data->queuedRequests_.empty()) we can more
safely believe that ASSERT(data->queuedRequests_.empty()); is indeed
redundant as it's just been emptied.

But it's important that each pipeline handler ensures all queued
requests are returned back to applciation ownership before stop
completes.

> 
> It's redundant and called in wrong order currently. That should solve 
> the issue!
> 
> >            data->frameInfo_.clear();

Clearing the data->frameInfo_ before the assert isn't enough to be sure
that all requests are cancelled.

It's up to the pipeline handler to make sure any references it takes
when a request  comes in (such as associating with the FrameInfo) are
released - so I do think there could be some merit in this patch ...

--
Kieran


> >
> > --
> > 2.43.0
> >
> > Javier Carrasco
> > R&D Engineer
> > Mail: javier.carrasco at wolfvision.net <mailto:%7BE-mail%7D>
> >
> > WolfVision GmbH
> > Oberes Ried 14 | 6833 Klaus | Österreich
> > T: +43 5523 52250 <tel:+43552352250>
> >
> > Webpage: www.wolfvision.com <https://www.wolfvision.com/> |
> > www.wolfvision.com/green <https://www.wolfvision.com/green>
> > Firmenbuch / Commercial Register: FN283521v Feldkirch/Austria
> >
> > RegisterNow-Signatur_ISE25_v1.jpg
> > <https://marketing.wolfvision.com/l/908602/2024-10-24/by7n7><https://info.wolfvision.com/ise/?utm_source=email&utm_medium=referral&utm_campaign=ise2024&utm_content=signature>
> > See our new solutions LIVE in Barcelona at ISE booth 3N500
> > Use our discount code QK7Z38BS to secure your free ticket here
> > <https://marketing.wolfvision.com/l/908602/2024-10-24/by7n7>
> >
>