| Message ID | 20230423203931.108022-2-umang.jain@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On 4/24/23 6:35 AM, Laurent Pinchart wrote: > Hi Umang, > > Thank you for the patch. > > On Mon, Apr 24, 2023 at 02:09:29AM +0530, Umang Jain via libcamera-devel wrote: >> KMSSink might process completed page flip requests from DRM >> after stop() has been called. This is not right hence connect the >> Device::requestCompleted signal on start() and disconnect it on stop(). > I wonder if this doesn't hide another issue. The stop() function queues > a synchronous request to disable the display pipeline. This should flush > all the other in-flight requests. If another request completes after > that, it may indicate that we queue it after calling stop(), which I'd > rather fix than hiding the problem. Ok - partially agreeing with you here. I agree the application should not queue requests if the sink has stopped. But if they do (erratic behaviour by the app), KMSSink should return false in processRequest() instead of actually processing it. It can be implemented with state (isStopped_) boolean check or similar. > >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> src/apps/cam/kms_sink.cpp | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/src/apps/cam/kms_sink.cpp b/src/apps/cam/kms_sink.cpp >> index 353209cd..a508977d 100644 >> --- a/src/apps/cam/kms_sink.cpp >> +++ b/src/apps/cam/kms_sink.cpp >> @@ -63,7 +63,6 @@ KMSSink::KMSSink(const std::string &connectorName) >> return; >> } >> >> - dev_.requestComplete.connect(this, &KMSSink::requestComplete); >> } >> >> void KMSSink::mapBuffer(libcamera::FrameBuffer *buffer) >> @@ -328,11 +327,15 @@ int KMSSink::start() >> return ret; >> } >> >> + dev_.requestComplete.connect(this, &KMSSink::requestComplete); >> + >> return 0; >> } >> >> int KMSSink::stop() >> { >> + dev_.requestComplete.disconnect(); >> + >> /* Display pipeline. */ >> DRM::AtomicRequest request(&dev_); >>
Hi Umang, On Mon, Apr 24, 2023 at 01:16:41PM +0530, Umang Jain wrote: > On 4/24/23 6:35 AM, Laurent Pinchart wrote: > > On Mon, Apr 24, 2023 at 02:09:29AM +0530, Umang Jain via libcamera-devel wrote: > >> KMSSink might process completed page flip requests from DRM > >> after stop() has been called. This is not right hence connect the > >> Device::requestCompleted signal on start() and disconnect it on stop(). > > > > I wonder if this doesn't hide another issue. The stop() function queues > > a synchronous request to disable the display pipeline. This should flush > > all the other in-flight requests. If another request completes after > > that, it may indicate that we queue it after calling stop(), which I'd > > rather fix than hiding the problem. > > Ok - partially agreeing with you here. > > I agree the application should not queue requests if the sink has > stopped. But if they do (erratic behaviour by the app), KMSSink should > return false in processRequest() instead of actually processing it. It > can be implemented with state (isStopped_) boolean check or similar. I'm fine with catching the issue in KMSSink as well as fixing the caller. > >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > >> --- > >> src/apps/cam/kms_sink.cpp | 5 ++++- > >> 1 file changed, 4 insertions(+), 1 deletion(-) > >> > >> diff --git a/src/apps/cam/kms_sink.cpp b/src/apps/cam/kms_sink.cpp > >> index 353209cd..a508977d 100644 > >> --- a/src/apps/cam/kms_sink.cpp > >> +++ b/src/apps/cam/kms_sink.cpp > >> @@ -63,7 +63,6 @@ KMSSink::KMSSink(const std::string &connectorName) > >> return; > >> } > >> > >> - dev_.requestComplete.connect(this, &KMSSink::requestComplete); > >> } > >> > >> void KMSSink::mapBuffer(libcamera::FrameBuffer *buffer) > >> @@ -328,11 +327,15 @@ int KMSSink::start() > >> return ret; > >> } > >> > >> + dev_.requestComplete.connect(this, &KMSSink::requestComplete); > >> + > >> return 0; > >> } > >> > >> int KMSSink::stop() > >> { > >> + dev_.requestComplete.disconnect(); > >> + > >> /* Display pipeline. */ > >> DRM::AtomicRequest request(&dev_); > >>
diff --git a/src/apps/cam/kms_sink.cpp b/src/apps/cam/kms_sink.cpp index 353209cd..a508977d 100644 --- a/src/apps/cam/kms_sink.cpp +++ b/src/apps/cam/kms_sink.cpp @@ -63,7 +63,6 @@ KMSSink::KMSSink(const std::string &connectorName) return; } - dev_.requestComplete.connect(this, &KMSSink::requestComplete); } void KMSSink::mapBuffer(libcamera::FrameBuffer *buffer) @@ -328,11 +327,15 @@ int KMSSink::start() return ret; } + dev_.requestComplete.connect(this, &KMSSink::requestComplete); + return 0; } int KMSSink::stop() { + dev_.requestComplete.disconnect(); + /* Display pipeline. */ DRM::AtomicRequest request(&dev_);
KMSSink might process completed page flip requests from DRM after stop() has been called. This is not right hence connect the Device::requestCompleted signal on start() and disconnect it on stop(). Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- src/apps/cam/kms_sink.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)