| Message ID | 20250623163414.3936727-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
On Mon, Jun 23, 2025 at 05:34:14PM +0100, Kieran Bingham wrote: > When stopping a camera, we may now find that there are requests > queued to the camera but not yet available in the pipeline handler. > > These are "waitingReqeusts" which are not yet queued to the device. > > While stopping a camera, we ask the pipeline to stop with stopDevice() > and then cancel any waiting requests after. > > This however can lead to a race where having stopped the camera and > completed the requests, the pipeline may try to further consume requests > from the waiting lists. > > When we call PipelineHandler::stop() we know that we can not process any > of the requests which are sat in the waiting queue, therefore cancel > those first before asking the pipeline handler to complete or cancel and > requests that it has ownership of. Doesn't the lead to requests completing out of order ? We should fix races, but ensuring strict completion order is important. > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > > Hi Stefan, > > Testing your series I discovered this shutdown race. > > Without this I get: > > > > [1:48:10.770431125] [2675] FATAL default 6490.777390] audit: type=1701 audit(1741278393.952:115): auid=4294967295 uid=0 gid=0 ses=4294967295 pid=2674 comm="cam" exe="/usr/bin/cam" sig=6 res=1 > ;34mrkisp1_ipa_proxy.cpp:467 assertion "state_ == ProxyRunning" failed in queueRequestThread() > Backtrace: > /lib/libcamera.so.0.5(_ZN9libcamera3ipa6rkisp114IPAProxyRkISP118queueRequestThreadEjRKNS_11ControlListE+0x94) [0xffffb32d[ 6490.810893] audit: type=1334 audit(1741278393.984:116): prog-id=51 op=LOAD > 7f8c] > /lib/libcamera.so.0.5(_ZN9[ 6490.819683] audit: type=1334 audit(1741278393.996:117): prog-id=52 op=LOAD > libcamera3ipa6rkisp114IPAProxyRkI[ 6490.820111] audit: type=1334 audit(1741278393.996:118): prog-id=53 op=LOAD > SP112queueRequestEjRKNS_11ControlListE+0x28) [0xffffb32d8048] > /lib/libcamera.so.0.5(_ZN9libcamera21PipelineHandlerRkISP118queueRequestDeviceEPNS_6CameraEPNS_7RequestE+0x50) [0xffffb3360b14] > > > This patch ensures that requests which are in the waitingRequests queue > are cancelled first to ensure they don't get processed by the pipeline > after it has completed it's stop(). > > > src/libcamera/pipeline_handler.cpp | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index 0389f34486ab..9cd3504dc9db 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -360,9 +360,6 @@ void PipelineHandler::unlockMediaDevices() > */ > 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. */ > @@ -372,8 +369,12 @@ void PipelineHandler::stop(Camera *camera) > cancelRequest(request); > } > > + /* Stop the pipeline handler and let the queued requests complete. */ > + stopDevice(camera); > + > /* Make sure no requests are pending. */ > ASSERT(data->queuedRequests_.empty()); > + ASSERT(data->waitingRequests_.empty()); > > data->requestSequence_ = 0; > }
diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index 0389f34486ab..9cd3504dc9db 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -360,9 +360,6 @@ void PipelineHandler::unlockMediaDevices() */ 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. */ @@ -372,8 +369,12 @@ void PipelineHandler::stop(Camera *camera) cancelRequest(request); } + /* Stop the pipeline handler and let the queued requests complete. */ + stopDevice(camera); + /* Make sure no requests are pending. */ ASSERT(data->queuedRequests_.empty()); + ASSERT(data->waitingRequests_.empty()); data->requestSequence_ = 0; }
When stopping a camera, we may now find that there are requests queued to the camera but not yet available in the pipeline handler. These are "waitingReqeusts" which are not yet queued to the device. While stopping a camera, we ask the pipeline to stop with stopDevice() and then cancel any waiting requests after. This however can lead to a race where having stopped the camera and completed the requests, the pipeline may try to further consume requests from the waiting lists. When we call PipelineHandler::stop() we know that we can not process any of the requests which are sat in the waiting queue, therefore cancel those first before asking the pipeline handler to complete or cancel and requests that it has ownership of. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- Hi Stefan, Testing your series I discovered this shutdown race. Without this I get: [1:48:10.770431125] [2675] FATAL default 6490.777390] audit: type=1701 audit(1741278393.952:115): auid=4294967295 uid=0 gid=0 ses=4294967295 pid=2674 comm="cam" exe="/usr/bin/cam" sig=6 res=1 ;34mrkisp1_ipa_proxy.cpp:467 assertion "state_ == ProxyRunning" failed in queueRequestThread() Backtrace: /lib/libcamera.so.0.5(_ZN9libcamera3ipa6rkisp114IPAProxyRkISP118queueRequestThreadEjRKNS_11ControlListE+0x94) [0xffffb32d[ 6490.810893] audit: type=1334 audit(1741278393.984:116): prog-id=51 op=LOAD 7f8c] /lib/libcamera.so.0.5(_ZN9[ 6490.819683] audit: type=1334 audit(1741278393.996:117): prog-id=52 op=LOAD libcamera3ipa6rkisp114IPAProxyRkI[ 6490.820111] audit: type=1334 audit(1741278393.996:118): prog-id=53 op=LOAD SP112queueRequestEjRKNS_11ControlListE+0x28) [0xffffb32d8048] /lib/libcamera.so.0.5(_ZN9libcamera21PipelineHandlerRkISP118queueRequestDeviceEPNS_6CameraEPNS_7RequestE+0x50) [0xffffb3360b14] This patch ensures that requests which are in the waitingRequests queue are cancelled first to ensure they don't get processed by the pipeline after it has completed it's stop(). src/libcamera/pipeline_handler.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)