Re: [PATCH i-g-t] tests/intel/kms_pwrite_crc: Create basic subtest

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

Hi Kamil,

On 4/20/2026 8:37 PM, Kamil Konieczny wrote:
> Create a basic subtests which allows to properly cleanup after
> any failure or a skip. Also while at this, move an igt header
> to proper place.
>
> Cc: Karthik B S <karthik.b.s@intel.com>
> Signed-off-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> ---
>   tests/intel/kms_pwrite_crc.c | 39 +++++++++++++++++++++++-------------
>   1 file changed, 25 insertions(+), 14 deletions(-)
>
> diff --git a/tests/intel/kms_pwrite_crc.c b/tests/intel/kms_pwrite_crc.c
> index bd536007c..92f5292dd 100644
> --- a/tests/intel/kms_pwrite_crc.c
> +++ b/tests/intel/kms_pwrite_crc.c
> @@ -30,15 +30,16 @@
>    * Mega feature: General Display Features
>    */
>   
> -#include "igt.h"
>   #include <errno.h>
>   #include <limits.h>
>   #include <stdbool.h>
>   #include <stdio.h>
>   #include <string.h>
>   
> +#include "igt.h"
> +
>   /**
> - * SUBTEST:
> + * SUBTEST: basic
>    * Description: Use the display CRC support to validate pwrite to an already
>    *              uncached future scanout buffer.
>    */
> @@ -192,22 +193,32 @@ static void run_test(data_t *data)
>   	igt_skip("no valid crtc/connector combinations found\n");
>   }
>   
> -static data_t data;
> +static data_t data = { };
>   
> -int igt_simple_main()
> +int igt_main()
>   {
> -	data.drm_fd = drm_open_driver_master(DRIVER_INTEL);
> -	kmstest_set_vt_graphics_mode();
> +	igt_fixture() {
> +		data.drm_fd = -1;

This is redundant with the next line?

> +		data.drm_fd = drm_open_driver_master(DRIVER_INTEL);
> +		kmstest_set_vt_graphics_mode();
>   
> -	igt_display_require(&data.display, data.drm_fd);
> -	igt_display_require_output(&data.display);
> -	igt_require_pipe_crc(data.drm_fd);
> +		igt_display_require(&data.display, data.drm_fd);
> +		igt_display_require_output(&data.display);
> +		igt_require_pipe_crc(data.drm_fd);
>   
> -	data.devid = intel_get_drm_devid(data.drm_fd);
> -	data.pipe_crc = NULL;
> +		data.devid = intel_get_drm_devid(data.drm_fd);
> +		data.pipe_crc = NULL;
> +	}
>   
> -	run_test(&data);
> +	igt_describe("Use the display CRC support to validate pwrite "
> +		     "to an already uncached future scanout buffer.");
> +	igt_subtest("basic")
> +		run_test(&data);
>   
> -	igt_display_fini(&data.display);
> -	drm_close_driver(data.drm_fd);
> +	igt_fixture() {
> +		if (data.drm_fd != -1) {

Similar redundancy here? If drm_fd = -1, the test will be skipped in the 
first fixture itself and this check is not needed?

With these removed the patch LGTM,

Reviewed-by: Karthik B S <karthik.b.s@intel.com>

> +			igt_display_fini(&data.display);
> +			drm_close_driver(data.drm_fd);
> +		}
> +	}
>   }