[PATCH v7 1/4] lib/ioctl_wrappers: let the caller handle capability check result

[Kamil Konieczny]

Hi André,
On 2025-04-08 at 19:36:01 -0300, André Almeida wrote:
> Rework igt_has_drm_cap to just check if a DRM capability is supported
> and let the called decide what to do from this check. It prevents the
> test fails because of an assert done when it's called in
> igt_subtest_with_dynamics.
> 
> Co-Developed-by: Melissa Wen <mwen at igalia.com>
> Signed-off-by: Melissa Wen <mwen at igalia.com>
> Signed-off-by: André Almeida <andrealmeid at igalia.com>
> Reviewed-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> ---
>  lib/ioctl_wrappers.c    | 17 ++++++++++++-----
>  lib/ioctl_wrappers.h    |  2 +-
>  tests/kms_async_flips.c | 13 +++++++++----
>  3 files changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index 146973f0d..26748450e 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -1288,14 +1288,21 @@ int __kms_addfb(int fd, uint32_t handle,
>   * This helper verifies if the passed capability is
>   * supported by the kernel

Small nit: imho write here about asserting in case of bad file descriptor.

>   *
> - * Returns: Whether the capability is supported or not.
> + * Returns: negative value if error (bad fd, cap does not exist), 0 if cap is
> + * not supported, 1 if cap is supported.
>   */
> -bool igt_has_drm_cap(int fd, uint64_t capability)
> +int igt_has_drm_cap(int fd, uint64_t capability)
>  {
> -	struct drm_get_cap cap = { .capability = capability };
> +	uint64_t value = 0;
> +	int ret;
> +
> +	ret = drmGetCap(fd, capability, &value);
> +	if (ret) {
> +		igt_assert_neq(errno, EBADF);
> +		return -errno;
> +	}
>  
> -	igt_assert(drmIoctl(fd, DRM_IOCTL_GET_CAP, &cap) == 0);
> -	return cap.value;
> +	return value ? 1 : 0;
>  }
>  
>  /**
> diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
> index b7d7c2ad9..7cf05e626 100644
> --- a/lib/ioctl_wrappers.h
> +++ b/lib/ioctl_wrappers.h
> @@ -144,7 +144,7 @@ void prime_sync_end(int dma_buf_fd, bool write);
>  
>  bool igt_has_fb_modifiers(int fd);
>  void igt_require_fb_modifiers(int fd);
> -bool igt_has_drm_cap(int fd, uint64_t capability);
> +int igt_has_drm_cap(int fd, uint64_t capability);
>  bool igt_has_set_caching(uint32_t devid);
>  
>  /**
> diff --git a/tests/kms_async_flips.c b/tests/kms_async_flips.c
> index da426f753..63598bd95 100644
> --- a/tests/kms_async_flips.c
> +++ b/tests/kms_async_flips.c
> @@ -203,8 +203,10 @@ static void make_fb(data_t *data, struct igt_fb *fb,
>  
>  static void require_monotonic_timestamp(int fd)
>  {
> -	igt_require_f(igt_has_drm_cap(fd, DRM_CAP_TIMESTAMP_MONOTONIC),
> -		      "Monotonic timestamps not supported\n");
> +	int ret = igt_has_drm_cap(fd, DRM_CAP_TIMESTAMP_MONOTONIC);
> +
> +	igt_require_f(ret >= 0, "Monotonic timestamps cap doesn't exist in this kernel\n");
> +	igt_require_f(ret == 1, "Monotonic timestamps not supported\n");
>  }
>  
>  static void test_init(data_t *data)
> @@ -747,13 +749,16 @@ igt_main
>  	int i;
>  
>  	igt_fixture {
> +		int ret;
> +
>  		data.drm_fd = drm_open_driver_master(DRIVER_ANY);
>  		kmstest_set_vt_graphics_mode();
>  		igt_display_require(&data.display, data.drm_fd);
>  		igt_display_require_output(&data.display);
>  
> -		igt_require_f(igt_has_drm_cap(data.drm_fd, DRM_CAP_ASYNC_PAGE_FLIP),
> -			      "Async Flip is not supported\n");
> +		ret = igt_has_drm_cap(data.drm_fd, DRM_CAP_ASYNC_PAGE_FLIP);
> +		igt_require_f(ret >= 0, "Async flip cap doesn't exist in this kernel\n");
> +		igt_require_f(ret == 1, "Async Flip is not supported\n");

Be consistent here, either 'Async Flip' or 'Async flip' in both
igt_require. You could keep my r-b for these changes, looks fine.
Btw why not fuller name? 'Async Page Flip' looks better.

Also if you will go with 'Async Flip', consider also change in
above lines for 'Monotonic Timestamps'.

Regards,
Kamil

>  
>  		if (is_intel_device(data.drm_fd))
>  			data.bops = buf_ops_create(data.drm_fd);
> -- 
> 2.49.0
>