| Message ID | 20210208094416.2124569-1-niklas.soderlund@ragnatech.se |
|---|---|
| State | Rejected |
| Delegated to: | Niklas Söderlund |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, On Mon, Feb 08, 2021 at 10:44:16AM +0100, Niklas Söderlund wrote: > Running cam with the --capture=N option only queues N Requests to the > camera. If N is less then the number of requests needed by the camera to > operate cam may deadlock waiting for the camera to complete requests > while the camera waits for the application to give it more buffers. > > Fix this by adding a check in cam to no attempt a capture session if to > few requests are requested. > > Reported-by: Sebastian Fricke <sebastian.fricke@posteo.net> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/cam/capture.cpp | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp > index 7b55fc6770225e2a..18aa97c7cdb55dcc 100644 > --- a/src/cam/capture.cpp > +++ b/src/cam/capture.cpp > @@ -90,6 +90,13 @@ int Capture::capture(FrameBufferAllocator *allocator) > nbuffers = std::min(nbuffers, allocated); > } > > + if (captureLimit_ && captureLimit_ < nbuffers) { > + std::cerr << "Camera requiers at least " << nbuffers s/requiers/requires > + << " reqests to function, " << captureLimit_ s/reqests/requests > + << " requested" << std::endl; I would make it shorter << "Camera requires " << nbuffers << " requests, " << "but only " << captureLimit_ << " have been provided"; Which is not that shorter :/ Up to you Spelling fixes apart Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > + return -EINVAL; > + } > + > /* > * TODO: make cam tool smarter to support still capture by for > * example pushing a button. For now run all streams all the time. > -- > 2.30.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, Also s/to/too/ in $SUBJECT On 08/02/2021 10:14, Jacopo Mondi wrote: > Hi Niklas, > > On Mon, Feb 08, 2021 at 10:44:16AM +0100, Niklas Söderlund wrote: >> Running cam with the --capture=N option only queues N Requests to the >> camera. If N is less then the number of requests needed by the camera to >> operate cam may deadlock waiting for the camera to complete requests >> while the camera waits for the application to give it more buffers. >> >> Fix this by adding a check in cam to no attempt a capture session if to >> few requests are requested. and here s/to/too/ >> >> Reported-by: Sebastian Fricke <sebastian.fricke@posteo.net> >> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> >> --- >> src/cam/capture.cpp | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp >> index 7b55fc6770225e2a..18aa97c7cdb55dcc 100644 >> --- a/src/cam/capture.cpp >> +++ b/src/cam/capture.cpp >> @@ -90,6 +90,13 @@ int Capture::capture(FrameBufferAllocator *allocator) >> nbuffers = std::min(nbuffers, allocated); >> } >> >> + if (captureLimit_ && captureLimit_ < nbuffers) { >> + std::cerr << "Camera requiers at least " << nbuffers > > s/requiers/requires > >> + << " reqests to function, " << captureLimit_ > > s/reqests/requests > >> + << " requested" << std::endl; > > I would make it shorter > << "Camera requires " << nbuffers << " requests, " > << "but only " << captureLimit_ << " have been provided"; > > Which is not that shorter :/ > > Up to you > > Spelling fixes apart > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Should we increase the capture lmit to the minimum? Or do you want to make sure cam fails if too few are requested? Either way, Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Thanks > j > >> + return -EINVAL; >> + } >> + >> /* >> * TODO: make cam tool smarter to support still capture by for >> * example pushing a button. For now run all streams all the time. >> -- >> 2.30.0 >> >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
Hi Kieran and Jacopo, Thanks for your feedback. On 2021-02-08 10:20:07 +0000, Kieran Bingham wrote: > Hi Niklas, > > Also s/to/too/ in $SUBJECT > > On 08/02/2021 10:14, Jacopo Mondi wrote: > > Hi Niklas, > > > > On Mon, Feb 08, 2021 at 10:44:16AM +0100, Niklas Söderlund wrote: > >> Running cam with the --capture=N option only queues N Requests to the > >> camera. If N is less then the number of requests needed by the camera to > >> operate cam may deadlock waiting for the camera to complete requests > >> while the camera waits for the application to give it more buffers. > >> > >> Fix this by adding a check in cam to no attempt a capture session if to > >> few requests are requested. > > and here s/to/too/ > > >> > >> Reported-by: Sebastian Fricke <sebastian.fricke@posteo.net> > >> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > >> --- > >> src/cam/capture.cpp | 7 +++++++ > >> 1 file changed, 7 insertions(+) > >> > >> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp > >> index 7b55fc6770225e2a..18aa97c7cdb55dcc 100644 > >> --- a/src/cam/capture.cpp > >> +++ b/src/cam/capture.cpp > >> @@ -90,6 +90,13 @@ int Capture::capture(FrameBufferAllocator *allocator) > >> nbuffers = std::min(nbuffers, allocated); > >> } > >> > >> + if (captureLimit_ && captureLimit_ < nbuffers) { > >> + std::cerr << "Camera requiers at least " << nbuffers > > > > s/requiers/requires > > > >> + << " reqests to function, " << captureLimit_ > > > > s/reqests/requests > > > >> + << " requested" << std::endl; > > > > I would make it shorter > > << "Camera requires " << nbuffers << " requests, " > > << "but only " << captureLimit_ << " have been provided"; > > > > Which is not that shorter :/ > > > > Up to you > > > > Spelling fixes apart > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > Should we increase the capture lmit to the minimum? Or do you want to > make sure cam fails if too few are requested? I would prefers to fail as one could do $ cam --camera foo --capture=2 --file=frame-#.bin And expect 2 files to be written to disk. If we auto increment it to the minimum 2 or more files may be written to disk which might not be what the user wants. > > Either way, > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > Thanks > > j > > > >> + return -EINVAL; > >> + } > >> + > >> /* > >> * TODO: make cam tool smarter to support still capture by for > >> * example pushing a button. For now run all streams all the time. > >> -- > >> 2.30.0 > >> > >> _______________________________________________ > >> libcamera-devel mailing list > >> libcamera-devel@lists.libcamera.org > >> https://lists.libcamera.org/listinfo/libcamera-devel > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > Regards > -- > Kieran
Hi Niklas, Thank you for the patch. On Mon, Feb 08, 2021 at 10:44:16AM +0100, Niklas Söderlund wrote: > Running cam with the --capture=N option only queues N Requests to the > camera. If N is less then the number of requests needed by the camera to > operate cam may deadlock waiting for the camera to complete requests > while the camera waits for the application to give it more buffers. > > Fix this by adding a check in cam to no attempt a capture session if to > few requests are requested. > > Reported-by: Sebastian Fricke <sebastian.fricke@posteo.net> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/cam/capture.cpp | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp > index 7b55fc6770225e2a..18aa97c7cdb55dcc 100644 > --- a/src/cam/capture.cpp > +++ b/src/cam/capture.cpp > @@ -90,6 +90,13 @@ int Capture::capture(FrameBufferAllocator *allocator) > nbuffers = std::min(nbuffers, allocated); > } > > + if (captureLimit_ && captureLimit_ < nbuffers) { > + std::cerr << "Camera requiers at least " << nbuffers > + << " reqests to function, " << captureLimit_ > + << " requested" << std::endl; > + return -EINVAL; > + } I'm afraid this isn't right. The pipeline handler may not require nbuffers to be queued to operate properly, and this also won't handle the end of stream case where some drivers will require enqueing more buffers to push the last buffers out. We need a more complex solution for this, involving an explicit queue depth. > + > /* > * TODO: make cam tool smarter to support still capture by for > * example pushing a button. For now run all streams all the time.
Hi Laurent, Thanks for your feedback. On 2021-02-08 13:36:39 +0200, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Mon, Feb 08, 2021 at 10:44:16AM +0100, Niklas Söderlund wrote: > > Running cam with the --capture=N option only queues N Requests to the > > camera. If N is less then the number of requests needed by the camera to > > operate cam may deadlock waiting for the camera to complete requests > > while the camera waits for the application to give it more buffers. > > > > Fix this by adding a check in cam to no attempt a capture session if to > > few requests are requested. > > > > Reported-by: Sebastian Fricke <sebastian.fricke@posteo.net> > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > src/cam/capture.cpp | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp > > index 7b55fc6770225e2a..18aa97c7cdb55dcc 100644 > > --- a/src/cam/capture.cpp > > +++ b/src/cam/capture.cpp > > @@ -90,6 +90,13 @@ int Capture::capture(FrameBufferAllocator *allocator) > > nbuffers = std::min(nbuffers, allocated); > > } > > > > + if (captureLimit_ && captureLimit_ < nbuffers) { > > + std::cerr << "Camera requiers at least " << nbuffers > > + << " reqests to function, " << captureLimit_ > > + << " requested" << std::endl; > > + return -EINVAL; > > + } > > I'm afraid this isn't right. The pipeline handler may not require > nbuffers to be queued to operate properly, and this also won't handle > the end of stream case where some drivers will require enqueing more > buffers to push the last buffers out. First off, I agree we should move towards not requiring more then 1 Request queued for a camera to operate popery. But that is the design we have today and why we added nbuffers. I think it make sens to express this in cam until we rework our pipelines to not need nbuffers and instead add scratch buffers or something else. Further more I don't think we should handle scenarios where we need to queue more buffers at stream stop. Is this not something v4l2-compliance checks for? And if we have a pipeline where this does not work we should fix it in the kernel. But for start conditions there is code in vb2 that deals with N buffers needs to be queued for capture to start so we need to handle that somehow in libcamera. Right now this is pushed to applications and until we rework this I think cam should act on the information it have instead of dead locking. > > We need a more complex solution for this, involving an explicit queue > depth. Is this not more needed for operational guarantees of 3A algorithms unless we are going to have a larger memory footprint by using internal buffers to keep the algorithms running? > > > + > > /* > > * TODO: make cam tool smarter to support still capture by for > > * example pushing a button. For now run all streams all the time. > > -- > Regards, > > Laurent Pinchart
diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp index 7b55fc6770225e2a..18aa97c7cdb55dcc 100644 --- a/src/cam/capture.cpp +++ b/src/cam/capture.cpp @@ -90,6 +90,13 @@ int Capture::capture(FrameBufferAllocator *allocator) nbuffers = std::min(nbuffers, allocated); } + if (captureLimit_ && captureLimit_ < nbuffers) { + std::cerr << "Camera requiers at least " << nbuffers + << " reqests to function, " << captureLimit_ + << " requested" << std::endl; + return -EINVAL; + } + /* * TODO: make cam tool smarter to support still capture by for * example pushing a button. For now run all streams all the time.
Running cam with the --capture=N option only queues N Requests to the camera. If N is less then the number of requests needed by the camera to operate cam may deadlock waiting for the camera to complete requests while the camera waits for the application to give it more buffers. Fix this by adding a check in cam to no attempt a capture session if to few requests are requested. Reported-by: Sebastian Fricke <sebastian.fricke@posteo.net> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/cam/capture.cpp | 7 +++++++ 1 file changed, 7 insertions(+)