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

[Umang Jain]

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