Re: [PATCH v4 3/3] tests/kms_async_flips: Create subtest for overlay planes

["André Almeida" <andrealmeid@igalia.com>]

Hi Melissa,

Thanks for the review!

Em 29/03/2025 16:25, Melissa Wen escreveu:
> On 03/27, André Almeida wrote:
>> amdgpu can perform async flips in overlay planes as well, so create a
>> test for that.
>>
>> Signed-off-by: André Almeida <andrealmeid@igalia.com>
>> ---
>>   tests/kms_async_flips.c | 56 ++++++++++++++++++++++++++++++++++++++---
>>   1 file changed, 52 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/kms_async_flips.c b/tests/kms_async_flips.c
>> index 5c7e1ae13..cc149dea2 100644
>> --- a/tests/kms_async_flips.c
>> +++ b/tests/kms_async_flips.c
>> @@ -85,6 +85,9 @@
>>    *
>>    * SUBTEST: async-flip-suspend-resume
>>    * Description: Verify the async flip functionality with suspend and resume cycle
>> + *
>> + * SUBTEST: overlay-atomic
>> + * Description: Verify overlay planes with async flips in atomic API
>>    */
>>   
>>   #define CURSOR_POS 128
>> @@ -105,12 +108,14 @@ typedef struct {
>>   	uint32_t crtc_id;
>>   	uint32_t refresh_rate;
>>   	struct igt_fb bufs[NUM_FBS];
>> +	struct igt_fb bufs_overlay[NUM_FBS];
>>   	igt_display_t display;
>>   	igt_output_t *output;
>>   	unsigned long flip_timestamp_us;
>>   	double flip_interval;
>>   	uint64_t modifier;
>>   	igt_plane_t *plane;
>> +	igt_plane_t *plane_overlay;
>>   	igt_pipe_crc_t *pipe_crc;
>>   	igt_crc_t ref_crc;
>>   	int flip_count;
>> @@ -122,6 +127,7 @@ typedef struct {
>>   	bool allow_fail;
>>   	struct buf_ops *bops;
>>   	bool atomic_path;
>> +	bool overlay_path;
>>   } data_t;
>>   
>>   static void flip_handler(int fd_, unsigned int sequence, unsigned int tv_sec,
>> @@ -213,6 +219,18 @@ static void require_atomic_async_cap(data_t *data)
>>   		      "Skipping, atomic flip async flip not supported");
>>   }
>>   
>> +static void require_overlay_flip_support(data_t *data)
>> +{
>> +	struct igt_fb *bufs = data->bufs_overlay;
>> +	igt_plane_t *plane = data->plane_overlay;
>> +	int flags = DRM_MODE_PAGE_FLIP_ASYNC | DRM_MODE_PAGE_FLIP_EVENT;
>> +
>> +	igt_plane_set_fb(plane, &bufs[0]);
> 
> Running `overlay-atomic` alone, the subtest passes, but running
> `kmas_async_flip` subtests in a row the `overlay-atomic` fails here in a
> kernel with overlay async support.
> 

The issue was happening because on test_init_fbs(), there's a `if 
(prev_output_id != output_id...)` that prevents fbs to be recreated if 
they are suitable for the next test. That was preventing the creation of 
the overlay fb in some cases, but I've fixed for the v5.

> I suspect something from previous execution needs a sync flip for
> setting modes before checking for overlay async support, or there is any
> other leak from the previous subtest.
> 
>> +
>> +	igt_require_f(!igt_display_try_commit_atomic(&data->display, flags, data),
>> +		      "Async flip for overlay planes not supported\n");
>> +}
>> +
>>   static void test_init(data_t *data)
>>   {
>>   	drmModeModeInfo *mode;
>> @@ -228,12 +246,15 @@ static void test_init(data_t *data)
>>   	igt_output_set_pipe(data->output, data->pipe);
>>   
>>   	data->plane = igt_output_get_plane_type(data->output, DRM_PLANE_TYPE_PRIMARY);
>> +	if (data->overlay_path)
>> +		data->plane_overlay = igt_output_get_plane_type(data->output, DRM_PLANE_TYPE_OVERLAY);
>>   }
>>   
>>   static void test_finish(data_t *data)
>>   {
>>   	data->alternate_sync_async = false;
>>   	data->atomic_path = false;
>> +	data->overlay_path = false;
>>   }
>>   
>>   static void test_init_fbs(data_t *data)
>> @@ -254,16 +275,27 @@ static void test_init_fbs(data_t *data)
>>   		prev_modifier = data->modifier;
>>   
>>   		if (data->bufs[0].fb_id) {
>> -			for (i = 0; i < NUM_FBS; i++)
>> +			for (i = 0; i < NUM_FBS; i++) {
>>   				igt_remove_fb(data->drm_fd, &data->bufs[i]);
>> +				if (data->overlay_path)
>> +					igt_remove_fb(data->drm_fd, &data->bufs_overlay[i]);
>> +			}
>>   		}
>>   
>> -		for (i = 0; i < NUM_FBS; i++)
>> +		for (i = 0; i < NUM_FBS; i++) {
>>   			make_fb(data, &data->bufs[i], width, height, i);
>> +			if (data->overlay_path)
>> +				make_fb(data, &data->bufs_overlay[i], width, height, i);
> These bufs_overlay fbs should be removed when the test finished,
> together with bufs fbs in the last igt_fixture of the test.

Done

>> +		}
>>   	}
>>   
>>   	igt_plane_set_fb(data->plane, &data->bufs[0]);
>>   	igt_plane_set_size(data->plane, width, height);
>> +
>> +	if (data->overlay_path) {
>> +		igt_plane_set_fb(data->plane_overlay, &data->bufs[0]);
> I didn't get why both primary and overlay planes are set to the same
> bufs[0] fb here, should it be data->bufs_overlay[0] ?

Ops, that was a typo!

>> +		igt_plane_set_size(data->plane_overlay, width, height);
>> +	}
>>   }
>>   
>>   static bool async_flip_needs_extra_frame(data_t *data)
>> @@ -291,12 +323,17 @@ static bool async_flip_needs_extra_frame(data_t *data)
>>   static int perform_flip(data_t *data, int frame, int flags)
>>   {
>>   	int ret;
>> +	igt_plane_t *plane;
>> +	struct igt_fb *bufs;
>> +
>> +	plane = data->overlay_path ? data->plane_overlay : data->plane;
>> +	bufs = data->overlay_path ? data->bufs_overlay : data->bufs;
>>   
>>   	if (!data->atomic_path) {
>>   		ret = drmModePageFlip(data->drm_fd, data->crtc_id,
>> -				      data->bufs[frame % NUM_FBS].fb_id, flags, data);
>> +				     bufs[frame % NUM_FBS].fb_id, flags, data);
>>   	} else {
>> -		igt_plane_set_fb(data->plane, &data->bufs[frame % NUM_FBS]);
>> +		igt_plane_set_fb(plane, &bufs[frame % NUM_FBS]);
>>   		ret = igt_display_try_commit_atomic(&data->display, flags, data);
>>   	}
>>   
>> @@ -312,6 +349,9 @@ static void test_async_flip(data_t *data)
>>   
>>   	igt_display_commit2(&data->display, data->display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
>>   
>> +	if (data->overlay_path)
> I wonder if the bufs_overlay fbs should be initialize and set here, after
> igt_display_commit2, instead of together with bufs fbs in the
> test_init_fbs()

test_init_fbs() is called once for all the tests in a subtest group, 
require_overlay_flip_support() is called every time a test run. I think 
it makes more sense to init those buffers just one time per run, like 
it's done with the other buffers. Let me know what do you think.

>> +		require_overlay_flip_support(data);
>> +