Re: [PATCH i-g-t 2/3] tests/kms_hdr: Move test_fini() for proper cleanup

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

Hi Pranay,

On 4/29/2026 3:09 PM, Pranay Samala wrote:
> Move test_fini() out of individual test functions to right
> after the igt_dynamic_f() block.
>
> Previously, if an igt_require_f() or igt_assert() inside
> the dynamic subtest caused a skip or failure, test_fini()
> would never execute since the longjmp exits the dynamic block.
> By placing test_fini() after igt_dynamic_f(), cleanup
> (pipe_crc free, fb removal, display reset) is guaranteed to
> run regardless of how the dynamic subtest exits.
>
> Signed-off-by: Pranay Samala <pranay.samala@intel.com>
> ---
>   tests/kms_hdr.c | 11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/tests/kms_hdr.c b/tests/kms_hdr.c
> index c4680298a..e880b280e 100644
> --- a/tests/kms_hdr.c
> +++ b/tests/kms_hdr.c
> @@ -304,8 +304,6 @@ static void test_bpc_switch_on_output(data_t *data, igt_crtc_t *crtc,
>   
>   	/* CRC capture is clamped to 8bpc, so capture should match. */
>   	igt_assert_crc_equal(&ref_crc, &new_crc);
> -
> -	test_fini(data);
>   }
>   
>   /* Returns true if an output supports max bpc property. */
> @@ -367,6 +365,8 @@ static void test_bpc_switch(data_t *data, uint32_t flags)
>   					      test_bpc_switch_on_output(data,
>   								       crtc,
>   								       output, hdr_test_formats[i], flags);
> +
> +				test_fini(data);
>   			}
>   
>   			/* One pipe is enough */
> @@ -564,7 +564,7 @@ static void test_static_toggle(data_t *data, igt_crtc_t *crtc,
>   	}
>   
>   cleanup:
> -	test_fini(data);
> +	return;

The 'cleanup' label is not required anymore. Just return from where 
cleanup is being called.

With this fixed, patch LGTM.

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

>   }
>   
>   /* Fills some test values for HDR metadata targeting SDR. */
> @@ -686,7 +686,6 @@ static void test_static_swap(data_t *data, igt_crtc_t *crtc,
>   		igt_assert(igt_is_force_dsc_disabled(data->fd, output->name));
>   	}
>   
> -	test_fini(data);
>   }
>   
>   static void test_invalid_metadata_sizes(data_t *data, igt_output_t *output)
> @@ -700,8 +699,6 @@ static void test_invalid_metadata_sizes(data_t *data, igt_output_t *output)
>   	igt_assert_eq(set_invalid_hdr_output_metadata(data, &hdr, metadata_size + 1), -EINVAL);
>   	igt_assert_eq(set_invalid_hdr_output_metadata(data, &hdr, metadata_size - 1), -EINVAL);
>   	igt_assert_eq(set_invalid_hdr_output_metadata(data, &hdr, metadata_size * 2), -EINVAL);
> -
> -	test_fini(data);
>   }
>   
>   /* Returns true if an output supports HDR metadata property. */
> @@ -815,6 +812,8 @@ static void test_hdr(data_t *data, uint32_t flags)
>   					if (flags & TEST_INVALID_METADATA_SIZES)
>   						test_invalid_metadata_sizes(data, output);
>   				}
> +
> +				test_fini(data);
>   			}
>   
>   			/* One pipe is enough */