| Message ID | 20210308162915.523021-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 4ce1a33e4b0f377e8b86cd80e965dc01a4907e17 |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. On Mon, Mar 08, 2021 at 04:29:15PM +0000, Kieran Bingham wrote: > The IPA should be stopped before the hardware devices to ensure that > all asynchronous actions have completed within the IPA before resources > are removed and released. This makes sense, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I however wonder if we should set a "stopping" flag before calling data->ipa_->stop(), to let the handlers for all the asynchronous actions invoked during stop() know that we're stopping. This would for instance allow us to avoid requeing buffers unnecessarily. Maybe it's not the best way to handle the issue, and in any case, if it's needed, it's a candidate for a patch on top. Does this solve the buffer queue after buffer free bug ? > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 2b4d31500533..498f85634a83 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -764,13 +764,13 @@ void PipelineHandlerIPU3::stop(Camera *camera) > IPU3CameraData *data = cameraData(camera); > int ret = 0; > > + data->ipa_->stop(); > + > ret |= data->imgu_->stop(); > ret |= data->cio2_.stop(); > if (ret) > LOG(IPU3, Warning) << "Failed to stop camera " << camera->id(); > > - data->ipa_->stop(); > - > freeBuffers(camera); > } >
Hi Kieran, On 2021-03-08 16:29:15 +0000, Kieran Bingham wrote: > The IPA should be stopped before the hardware devices to ensure that > all asynchronous actions have completed within the IPA before resources > are removed and released. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 2b4d31500533..498f85634a83 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -764,13 +764,13 @@ void PipelineHandlerIPU3::stop(Camera *camera) > IPU3CameraData *data = cameraData(camera); > int ret = 0; > > + data->ipa_->stop(); > + > ret |= data->imgu_->stop(); > ret |= data->cio2_.stop(); > if (ret) > LOG(IPU3, Warning) << "Failed to stop camera " << camera->id(); > > - data->ipa_->stop(); > - IIRC correctly this order was picked because before the cio2 is stopped buffers may still be dequeued and therefor handed to the IPA for processing. This was however done before the new IPC change and with a mock IPA. How does this work with the new IPC/IPA if the pipeline handler tries to post buffers to be processed before the CIO2 is stopped? > freeBuffers(camera); Should not all resources shared between pipeline and IPA be available until the buffers are freed here? > } > > -- > 2.25.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
On 08/03/2021 16:37, Niklas Söderlund wrote: > Hi Kieran, > > On 2021-03-08 16:29:15 +0000, Kieran Bingham wrote: >> The IPA should be stopped before the hardware devices to ensure that >> all asynchronous actions have completed within the IPA before resources >> are removed and released. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> src/libcamera/pipeline/ipu3/ipu3.cpp | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp >> index 2b4d31500533..498f85634a83 100644 >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp >> @@ -764,13 +764,13 @@ void PipelineHandlerIPU3::stop(Camera *camera) >> IPU3CameraData *data = cameraData(camera); >> int ret = 0; >> >> + data->ipa_->stop(); >> + >> ret |= data->imgu_->stop(); >> ret |= data->cio2_.stop(); >> if (ret) >> LOG(IPU3, Warning) << "Failed to stop camera " << camera->id(); >> >> - data->ipa_->stop(); >> - > > IIRC correctly this order was picked because before the cio2 is stopped > buffers may still be dequeued and therefor handed to the IPA for > processing. This was however done before the new IPC change and with a > mock IPA. How does this work with the new IPC/IPA if the pipeline > handler tries to post buffers to be processed before the CIO2 is > stopped? The IPA before stopped sends events that cause thigns to be queued to the v4l2 devices. This hits the fact that the v42l buffer cache is null because the buffers have been released. So we must stop the IPA before the devices.. But then we must also make sure the IPA is not processing frames and events from buffer completions after it has stopped.... > >> freeBuffers(camera); > > Should not all resources shared between pipeline and IPA be available > until the buffers are freed here? We're battling a use-after-free I think at the minute so indeed, this could be relevant. > >> } >> >> -- >> 2.25.1 >> >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel >
On 08/03/2021 16:34, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Mon, Mar 08, 2021 at 04:29:15PM +0000, Kieran Bingham wrote: >> The IPA should be stopped before the hardware devices to ensure that >> all asynchronous actions have completed within the IPA before resources >> are removed and released. > > This makes sense, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > I however wonder if we should set a "stopping" flag before calling > data->ipa_->stop(), to let the handlers for all the asynchronous actions > invoked during stop() know that we're stopping. This would for instance > allow us to avoid requeing buffers unnecessarily. Maybe it's not the > best way to handle the issue, and in any case, if it's needed, it's a > candidate for a patch on top. > > Does this solve the buffer queue after buffer free bug ? It solves the buffer queue after device stop bug - but indeed, pulls out (or leaves visible) a use-after-free on the FrameBufferAllocator with what seems like a unique pointer to a FrameBuffer being around after the Allocator releases all the FrameBuffers... https://paste.debian.net/1188407/ > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> src/libcamera/pipeline/ipu3/ipu3.cpp | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp >> index 2b4d31500533..498f85634a83 100644 >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp >> @@ -764,13 +764,13 @@ void PipelineHandlerIPU3::stop(Camera *camera) >> IPU3CameraData *data = cameraData(camera); >> int ret = 0; >> >> + data->ipa_->stop(); >> + >> ret |= data->imgu_->stop(); >> ret |= data->cio2_.stop(); >> if (ret) >> LOG(IPU3, Warning) << "Failed to stop camera " << camera->id(); >> >> - data->ipa_->stop(); >> - >> freeBuffers(camera); >> } >> >
On 08/03/2021 16:37, Niklas Söderlund wrote: > Hi Kieran, > > On 2021-03-08 16:29:15 +0000, Kieran Bingham wrote: >> The IPA should be stopped before the hardware devices to ensure that >> all asynchronous actions have completed within the IPA before resources >> are removed and released. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> src/libcamera/pipeline/ipu3/ipu3.cpp | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp >> index 2b4d31500533..498f85634a83 100644 >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp >> @@ -764,13 +764,13 @@ void PipelineHandlerIPU3::stop(Camera *camera) >> IPU3CameraData *data = cameraData(camera); >> int ret = 0; >> >> + data->ipa_->stop(); >> + >> ret |= data->imgu_->stop(); >> ret |= data->cio2_.stop(); >> if (ret) >> LOG(IPU3, Warning) << "Failed to stop camera " << camera->id(); >> >> - data->ipa_->stop(); >> - > > IIRC correctly this order was picked because before the cio2 is stopped > buffers may still be dequeued and therefor handed to the IPA for > processing. This was however done before the new IPC change and with a > mock IPA. How does this work with the new IPC/IPA if the pipeline > handler tries to post buffers to be processed before the CIO2 is > stopped? > > >> freeBuffers(camera); > > Should not all resources shared between pipeline and IPA be available > until the buffers are freed here? freeBuffers() releases: frameInfos->clear(); data->ipa_->unmapBuffers(ids); ipaBuffers_.clear(); data->imgu_->freeBuffers(); Surely we need to make sure both the devices and IPA have stopped before we free these resources? So I think freeBuffers should remain at the end... > >> } >> >> -- >> 2.25.1 >> >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel >
Hi Kieran, On Mon, Mar 08, 2021 at 04:40:56PM +0000, Kieran Bingham wrote: > On 08/03/2021 16:37, Niklas Söderlund wrote: > > On 2021-03-08 16:29:15 +0000, Kieran Bingham wrote: > >> The IPA should be stopped before the hardware devices to ensure that > >> all asynchronous actions have completed within the IPA before resources > >> are removed and released. > >> > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> --- > >> src/libcamera/pipeline/ipu3/ipu3.cpp | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > >> index 2b4d31500533..498f85634a83 100644 > >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > >> @@ -764,13 +764,13 @@ void PipelineHandlerIPU3::stop(Camera *camera) > >> IPU3CameraData *data = cameraData(camera); > >> int ret = 0; > >> > >> + data->ipa_->stop(); > >> + > >> ret |= data->imgu_->stop(); > >> ret |= data->cio2_.stop(); > >> if (ret) > >> LOG(IPU3, Warning) << "Failed to stop camera " << camera->id(); > >> > >> - data->ipa_->stop(); > >> - > > > > IIRC correctly this order was picked because before the cio2 is stopped > > buffers may still be dequeued and therefor handed to the IPA for > > processing. This was however done before the new IPC change and with a > > mock IPA. How does this work with the new IPC/IPA if the pipeline > > handler tries to post buffers to be processed before the CIO2 is > > stopped? > > The IPA before stopped sends events that cause thigns to be queued to > the v4l2 devices. > > This hits the fact that the v42l buffer cache is null because the > buffers have been released. > > So we must stop the IPA before the devices.. > > But then we must also make sure the IPA is not processing frames and > events from buffer completions after it has stopped.... I don't think this can happen, as V4L2 events are processed in the event loop, and we don't return to the event loop between data->ipa_->stop() and the imgu and cio2 stop calls. The IPA stop() call is different, as it calls IPCPipeUnixSocket::call(), which has a manual call to Thread::current()->eventDispatcher()->processEvents() to wait for the IPA response to the stop message. There's a race condition there, as other events could also be processed. We need to extend the EventDispatcher::processEvents() API to specify event types (or something similar). > >> freeBuffers(camera); > > > > Should not all resources shared between pipeline and IPA be available > > until the buffers are freed here? > > We're battling a use-after-free I think at the minute so indeed, this > could be relevant. > > >> } > >>
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 2b4d31500533..498f85634a83 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -764,13 +764,13 @@ void PipelineHandlerIPU3::stop(Camera *camera) IPU3CameraData *data = cameraData(camera); int ret = 0; + data->ipa_->stop(); + ret |= data->imgu_->stop(); ret |= data->cio2_.stop(); if (ret) LOG(IPU3, Warning) << "Failed to stop camera " << camera->id(); - data->ipa_->stop(); - freeBuffers(camera); }
The IPA should be stopped before the hardware devices to ensure that all asynchronous actions have completed within the IPA before resources are removed and released. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)