Re: [igt-dev] [PATCH 1/2] tests/i915/kms_draw_crc: Convert tests to dynamic

["Modem, Bhanuprakash" <bhanuprakash.modem@intel.com>]

On Wed-03-08-2022 09:50 am, Nidhi Gupta wrote:
> Convert the existing subtests to dynamic subtests.
> 
> v2: check all per-requisites before starting dynamic
>      tests, since to avoid skip in igt_dynamic().
>      (Bhanu)
> 
> Signed-off-by: Nidhi Gupta <nidhi1.gupta@intel.com>
> ---
>   tests/i915/kms_draw_crc.c | 54 ++++++++++++++++++++++++---------------
>   1 file changed, 33 insertions(+), 21 deletions(-)
> 
> diff --git a/tests/i915/kms_draw_crc.c b/tests/i915/kms_draw_crc.c
> index 33fefed4..edab2f3b 100644
> --- a/tests/i915/kms_draw_crc.c
> +++ b/tests/i915/kms_draw_crc.c
> @@ -177,15 +177,6 @@ static void draw_method_subtest(enum igt_draw_method method,
>   {
>   	igt_crc_t crc;
>   
> -	igt_skip_on(modifier == I915_FORMAT_MOD_4_TILED &&
> -		    !HAS_4TILE(intel_get_drm_devid(drm_fd)));
> -
> -	igt_skip_on(method == IGT_DRAW_MMAP_WC && !gem_mmap__has_wc(drm_fd));
> -	igt_skip_on(method == IGT_DRAW_MMAP_GTT &&
> -		    !gem_has_mappable_ggtt(drm_fd));
> -
> -	igt_require(format_is_supported(formats[format_index], modifier));
> -
>   	/* Use IGT_DRAW_MMAP_GTT/WC on an untiled buffer as the parameter for
>   	 * comparison. Cache the value so we don't recompute it for every single
>   	 * subtest. */
> @@ -331,22 +322,43 @@ igt_main
>   {
>   	enum igt_draw_method method;
>   	int format_idx, modifier_idx;
> +	uint64_t modifier;
>   
>   	igt_fixture
>   		setup_environment();
>   
> -	for (format_idx = 0; format_idx < ARRAY_SIZE(formats); format_idx++) {
> -	for (method = 0; method < IGT_DRAW_METHOD_COUNT; method++) {
> -	for (modifier_idx = 0; modifier_idx < ARRAY_SIZE(modifiers); modifier_idx++) {
> -		igt_describe("This subtest verfies igt_draw library works "
> -			     "with different modifiers, DRM_FORMATS, DRAW_METHODS.");
> -		igt_subtest_f("draw-method-%s-%s-%s",
> -			      format_str(format_idx),
> -			      igt_draw_get_method_name(method),
> -			      modifier_str(modifier_idx))
> -			draw_method_subtest(method, format_idx,
> -					    modifiers[modifier_idx]);
> -	} } }
> +	igt_describe("This subtest verfies igt_draw library works "
> +		     "with different modifiers, DRM_FORMATS, DRAW_METHODS.");
> +	igt_subtest_with_dynamic("draw-method") {
> +		for (format_idx = 0; format_idx < ARRAY_SIZE(formats); format_idx++) {
> +			for (method = 0; method < IGT_DRAW_METHOD_COUNT; method++) {
> +				for (modifier_idx = 0; modifier_idx < ARRAY_SIZE(modifiers); modifier_idx++) {
> +					modifier = modifiers[modifier_idx];
> +
> +					if (modifier == I915_FORMAT_MOD_4_TILED &&
> +					    !HAS_4TILE(intel_get_drm_devid(drm_fd)))
> +						continue;
> +
> +					if (method == IGT_DRAW_MMAP_WC && !gem_mmap__has_wc(drm_fd))
> +						continue;
> +
> +					if (method == IGT_DRAW_MMAP_GTT &&
> +					    !gem_has_mappable_ggtt(drm_fd))
> +						continue;
> +
> +					if (!(format_is_supported(formats[format_idx], modifier)))
---------------------------------------------^
NIT: Please drop unnecessary braces.

> +						continue;
> +
> +					igt_dynamic_f("draw-method-%s-%s-%s",
-------------------------------------------------------^
Please drop "draw-method" from dynamic subtest name.

> +						      format_str(format_idx),
> +						      igt_draw_get_method_name(method),
> +						      modifier_str(modifier_idx))
> +						draw_method_subtest(method, format_idx,
> +								    modifiers[modifier_idx]);--------------------------------------------------------------------^
We can use "modifier" instead of "modifiers[modifier_idx]".

By addressing above comments, this patch is
Reviewed-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com>

- Bhanu

> +				}
> +			}
> +		}
> +	}
>   
>   	igt_describe("This subtest verifies CRC after filling fb with x-tiling "
>   		     "or none.");