[libcamera-devel] [PATCH 1/3] apps: cam: kms_sink: Do not process requests after stop()

[Laurent Pinchart]

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 at 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_);
> >>   

-- 
Regards,

Laurent Pinchart