Re: [PATCH i-g-t] tests/kms_async_flip: skip subtest if invalid driver-specific condition

[Karthik B S <karthik.b.s@intel.com>]

Hi Melissa,

Although I agree that async flips can be rejected for multiple 
driver-specific reasons, my concern here is that there could potentially 
be a genuine failure(as EINVAL is a very generic failure check) as well 
which might get caught in this category and be treated as expected skip, 
whereas it ideally was something which should've been allowed by the driver.

IMHO, if we need to have this, we need to have some more checks together 
with EINVAL (though I'm not aware of what this could be currently) to 
treat it as a valid skip.

Regards,
Karthik.B.S
On 4/16/2025 1:17 AM, Melissa Wen wrote:
> Async page flips can fail for any driver-specific reasons, i.e. when the
> driver can't commit to async page flip request for hw limitations, the
> commit is rejected with -EINVAL since there is one or more conditions
> that make the request invalid. These limitations varies between vendors
> and hw versions. Skip a subtest if any invalid driver-specific condition
> happens. This behavior is in line with userspace expectations and fits
> better the plurality of drivers in the DRM subsystem.
>
> Signed-off-by: Melissa Wen <mwen@igalia.com>
>
> ---
>
> Hi,
>
> Recent changes to async page flip conditions and limitations in the DRM
> subsystem shed light on unexpected restrictions of async page flip
> tests. The current approach doesn't make the test generic enough to
> correctly deal with the diversity of reasons for a async page flip being
> rejected by drivers of any HW vendor.
>
> As an example, AMD display driver can't perform async flip when the
> memory type changes [1][2][3]. After further investigation performed by
> my colleague Thadeu Cascardo, he elucidated that this situation can
> happen whenever there is not enough space for FBs in VRAM, so they are
> allocated in GTT. Moreover, the space available in VRAM varies between
> hw families and other system needs. Therefore, the driver rejects the
> async flip due to memory type change, instead of silently downgrade the
> page flip from async to sync - which seems correct.
>
> The userspace expects the rejection (-EINVAL), but not the downgrade.
> So, I understand the test_async_flip should just validate if async page
> flips aren't downgrade in acceptable conditions and skip for any invalid
> driver-specific condition.
>
> [1] https://lore.kernel.org/amd-gfx/20230621202459.979661-2-andrealmeid@igalia.com/
> [2] https://lore.kernel.org/amd-gfx/20230804182054.142988-1-hamza.mahfooz@amd.com/
> [3] https://lore.kernel.org/amd-gfx/20250107152855.2953302-17-chiahsuan.chung@amd.com/
>
> Let me know your thoughts!
>
> Melissa
>
> ---
>   tests/kms_async_flips.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/tests/kms_async_flips.c b/tests/kms_async_flips.c
> index 031417ed8..0de299c2b 100644
> --- a/tests/kms_async_flips.c
> +++ b/tests/kms_async_flips.c
> @@ -397,8 +397,10 @@ static void test_async_flip(data_t *data)
>   
>   		ret = perform_flip(data, frame, flags);
>   
> -		if (frame == 1 && data->allow_fail)
> -			igt_skip_on(ret == -EINVAL);
> +		if (data->allow_fail)
> +			igt_skip_on_f(ret == -EINVAL,
> +				      "Skipping, async flip not supported at frame %d " \
> +				      "due to invalid driver-specific conditions.\n", frame);
>   		else
>   			igt_assert_eq(ret, 0);
>   
> @@ -768,7 +770,7 @@ static void run_test(data_t *data, void (*test)(data_t *))
>   			continue;
>   
>   		test_init(data);
> -		data->allow_fail = false;
> +		data->allow_fail = true;
>   		data->modifier = default_modifier(data);
>   		igt_dynamic_f("pipe-%s-%s", kmstest_pipe_name(data->pipe), data->output->name) {
>   			/*