[libcamera-devel,2/2] test: fence: Signal fence once
diff mbox series

Message ID 20211213145143.218734-3-jacopo@jmondi.org
State Accepted
Commit 5c4f9e3ae3f06d97453e18230dc55caacb0ea6fe
Headers show
Series
  • test: fence: Fixes for the fence test
Related show

Commit Message

Jacopo Mondi Dec. 13, 2021, 2:51 p.m. UTC
The unit test associates a fence with a framebuffer and makes sure
the fence is correctly signalled.

Once a fence is correctly signalled, it is released by the core, and the
underlying file descriptor closed.

The unit test however tries to write on the fence file descriptor every
time the designated Request is queued, and error which is now visible
as the return value of the fence signalling write() is checked.

Fix that by associating a fence with a framebuffer only once.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 test/fence.cpp | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Laurent Pinchart Dec. 13, 2021, 3:24 p.m. UTC | #1
Hi Jacopo,

Thank you for the patch.

On Mon, Dec 13, 2021 at 03:51:43PM +0100, Jacopo Mondi wrote:
> The unit test associates a fence with a framebuffer and makes sure
> the fence is correctly signalled.
> 
> Once a fence is correctly signalled, it is released by the core, and the
> underlying file descriptor closed.
> 
> The unit test however tries to write on the fence file descriptor every
> time the designated Request is queued, and error which is now visible

s/and/an/

> as the return value of the fence signalling write() is checked.
> 
> Fix that by associating a fence with a framebuffer only once.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  test/fence.cpp | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/test/fence.cpp b/test/fence.cpp
> index d2865398784e..524db2a10757 100644
> --- a/test/fence.cpp
> +++ b/test/fence.cpp
> @@ -56,6 +56,7 @@ private:
>  	Stream *stream_;
>  
>  	bool expectedCompletionResult_ = true;
> +	bool setFence_ = true;
>  
>  	unsigned int completedRequest_;
>  
> @@ -193,7 +194,7 @@ void FenceTest::requestRequeue(Request *request)
>  
>  	request->reuse();
>  
> -	if (cookie == signalledRequestId_) {
> +	if (cookie == signalledRequestId_ && setFence_) {
>  		/*
>  		 * The second time this request is queued add a fence to it.
>  		 *
> @@ -257,6 +258,7 @@ void FenceTest::signalFence()
>  	if (ret != sizeof(value))
>  		cerr << "Failed to signal fence" << endl;
>  
> +	setFence_ = false;
>  	dispatcher_->processEvents();
>  }
>  
> @@ -316,7 +318,7 @@ int FenceTest::run()
>  	Timer timer;
>  	timer.start(1000);
>  	while (timer.isRunning() && expectedCompletionResult_) {
> -		if (completedRequest_ == signalledRequestId_)
> +		if (completedRequest_ == signalledRequestId_ && setFence_)

A bit of a hack, but I suppose it works. The test is hard to read,
there's room for improvement (and possibly a race condition or two), but
that's not urgent.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  			/*
>  			 * signalledRequestId_ has just completed and it has
>  			 * been re-queued with a fence. Start the timer to
Umang Jain Dec. 13, 2021, 3:42 p.m. UTC | #2
Hi Jacopo,

On 12/13/21 8:54 PM, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, Dec 13, 2021 at 03:51:43PM +0100, Jacopo Mondi wrote:
>> The unit test associates a fence with a framebuffer and makes sure
>> the fence is correctly signalled.
>>
>> Once a fence is correctly signalled, it is released by the core, and the
>> underlying file descriptor closed.
>>
>> The unit test however tries to write on the fence file descriptor every
>> time the designated Request is queued, and error which is now visible
> s/and/an/
>
>> as the return value of the fence signalling write() is checked.
>>
>> Fix that by associating a fence with a framebuffer only once.
>>
>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>> ---
>>   test/fence.cpp | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/test/fence.cpp b/test/fence.cpp
>> index d2865398784e..524db2a10757 100644
>> --- a/test/fence.cpp
>> +++ b/test/fence.cpp
>> @@ -56,6 +56,7 @@ private:
>>   	Stream *stream_;
>>   
>>   	bool expectedCompletionResult_ = true;
>> +	bool setFence_ = true;
>>   
>>   	unsigned int completedRequest_;
>>   
>> @@ -193,7 +194,7 @@ void FenceTest::requestRequeue(Request *request)
>>   
>>   	request->reuse();
>>   
>> -	if (cookie == signalledRequestId_) {
>> +	if (cookie == signalledRequestId_ && setFence_) {
>>   		/*
>>   		 * The second time this request is queued add a fence to it.
>>   		 *
>> @@ -257,6 +258,7 @@ void FenceTest::signalFence()
>>   	if (ret != sizeof(value))
>>   		cerr << "Failed to signal fence" << endl;
>>   
>> +	setFence_ = false;
>>   	dispatcher_->processEvents();
>>   }
>>   
>> @@ -316,7 +318,7 @@ int FenceTest::run()
>>   	Timer timer;
>>   	timer.start(1000);
>>   	while (timer.isRunning() && expectedCompletionResult_) {
>> -		if (completedRequest_ == signalledRequestId_)
>> +		if (completedRequest_ == signalledRequestId_ && setFence_)
> A bit of a hack, but I suppose it works. The test is hard to read,
> there's room for improvement (and possibly a race condition or two), but
> that's not urgent.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>


Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

>
>>   			/*
>>   			 * signalledRequestId_ has just completed and it has
>>   			 * been re-queued with a fence. Start the timer to

Patch
diff mbox series

diff --git a/test/fence.cpp b/test/fence.cpp
index d2865398784e..524db2a10757 100644
--- a/test/fence.cpp
+++ b/test/fence.cpp
@@ -56,6 +56,7 @@  private:
 	Stream *stream_;
 
 	bool expectedCompletionResult_ = true;
+	bool setFence_ = true;
 
 	unsigned int completedRequest_;
 
@@ -193,7 +194,7 @@  void FenceTest::requestRequeue(Request *request)
 
 	request->reuse();
 
-	if (cookie == signalledRequestId_) {
+	if (cookie == signalledRequestId_ && setFence_) {
 		/*
 		 * The second time this request is queued add a fence to it.
 		 *
@@ -257,6 +258,7 @@  void FenceTest::signalFence()
 	if (ret != sizeof(value))
 		cerr << "Failed to signal fence" << endl;
 
+	setFence_ = false;
 	dispatcher_->processEvents();
 }
 
@@ -316,7 +318,7 @@  int FenceTest::run()
 	Timer timer;
 	timer.start(1000);
 	while (timer.isRunning() && expectedCompletionResult_) {
-		if (completedRequest_ == signalledRequestId_)
+		if (completedRequest_ == signalledRequestId_ && setFence_)
 			/*
 			 * signalledRequestId_ has just completed and it has
 			 * been re-queued with a fence. Start the timer to