Re: [PATCH i-g-t 1/3] tests/kms_hdr: Move framebuffer into data_t for centralized cleanup

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

Hi Pranay,

On 4/29/2026 3:09 PM, Pranay Samala wrote:
> Move igt_fb_t afb from local variable in test functions into the
> shared data_t struct. Add igt_remove_fb() to test_fini() so the
> framebuffer is always freed during cleanup, even when a test exits
> early.

This commit message can be improved. Currently the two sentences seem 
disjoint, may be ",and add igt_remove_fb()"?

With this updated, patch LGTM.

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

>
> Signed-off-by: Pranay Samala <pranay.samala@intel.com>
> ---
>   tests/kms_hdr.c | 30 +++++++++++++-----------------
>   1 file changed, 13 insertions(+), 17 deletions(-)
>
> diff --git a/tests/kms_hdr.c b/tests/kms_hdr.c
> index b215b0e6c..c4680298a 100644
> --- a/tests/kms_hdr.c
> +++ b/tests/kms_hdr.c
> @@ -123,12 +123,14 @@ typedef struct data {
>   	int fd;
>   	int w;
>   	int h;
> +	igt_fb_t afb;
>   } data_t;
>   
>   /* Common test cleanup. */
>   static void test_fini(data_t *data)
>   {
>   	igt_pipe_crc_free(data->pipe_crc);
> +	igt_remove_fb(data->fd, &data->afb);
>   	igt_display_reset(&data->display);
>   }
>   
> @@ -244,20 +246,19 @@ static void test_bpc_switch_on_output(data_t *data, igt_crtc_t *crtc,
>   {
>   	igt_display_t *display = &data->display;
>   	igt_crc_t ref_crc, new_crc;
> -	igt_fb_t afb;
>   	int afb_id, ret;
>   
>   	/* 10-bit formats are slow, so limit the size. */
>   	afb_id = igt_create_fb(data->fd, 512, 512,
> -			       format, DRM_FORMAT_MOD_LINEAR, &afb);
> +			       format, DRM_FORMAT_MOD_LINEAR, &data->afb);
>   	igt_assert(afb_id);
>   
> -	draw_hdr_pattern(&afb);
> +	draw_hdr_pattern(&data->afb);
>   
>   	/* Plane may be required to fit fullscreen. Check it here and allow
>   	 * smaller plane size in following tests.
>   	 */
> -	igt_plane_set_fb(data->primary, &afb);
> +	igt_plane_set_fb(data->primary, &data->afb);
>   	if (igt_crtc_num_scalers(crtc) >= 1)
>   		igt_plane_set_size(data->primary, data->w, data->h);
>   	else
> @@ -265,8 +266,8 @@ static void test_bpc_switch_on_output(data_t *data, igt_crtc_t *crtc,
>   
>   	ret = igt_display_try_commit_atomic(display, DRM_MODE_ATOMIC_TEST_ONLY, NULL);
>   	if (!ret) {
> -		data->w = afb.width;
> -		data->h = afb.height;
> +		data->w = data->afb.width;
> +		data->h = data->afb.height;
>   	}
>   
>   	/* Start in 8bpc. */
> @@ -305,7 +306,6 @@ static void test_bpc_switch_on_output(data_t *data, igt_crtc_t *crtc,
>   	igt_assert_crc_equal(&ref_crc, &new_crc);
>   
>   	test_fini(data);
> -	igt_remove_fb(data->fd, &afb);
>   }
>   
>   /* Returns true if an output supports max bpc property. */
> @@ -490,20 +490,19 @@ static void test_static_toggle(data_t *data, igt_crtc_t *crtc,
>   	igt_display_t *display = &data->display;
>   	struct hdr_output_metadata hdr;
>   	igt_crc_t ref_crc, new_crc;
> -	igt_fb_t afb;
>   	int afb_id;
>   
>   	/* 10-bit formats are slow, so limit the size. */
>   	afb_id = igt_create_fb(data->fd, 512, 512,
> -			       format, DRM_FORMAT_MOD_LINEAR, &afb);
> +			       format, DRM_FORMAT_MOD_LINEAR, &data->afb);
>   	igt_assert(afb_id);
>   
> -	draw_hdr_pattern(&afb);
> +	draw_hdr_pattern(&data->afb);
>   
>   	fill_hdr_output_metadata_st2084(&hdr);
>   
>   	/* Start with no metadata. */
> -	igt_plane_set_fb(data->primary, &afb);
> +	igt_plane_set_fb(data->primary, &data->afb);
>   	igt_plane_set_size(data->primary, data->w, data->h);
>   	set_hdr_output_metadata(data, NULL);
>   	igt_output_set_prop_value(data->output, IGT_CONNECTOR_MAX_BPC, 8);
> @@ -566,7 +565,6 @@ static void test_static_toggle(data_t *data, igt_crtc_t *crtc,
>   
>   cleanup:
>   	test_fini(data);
> -	igt_remove_fb(data->fd, &afb);
>   }
>   
>   /* Fills some test values for HDR metadata targeting SDR. */
> @@ -605,19 +603,18 @@ static void test_static_swap(data_t *data, igt_crtc_t *crtc,
>   {
>   	igt_display_t *display = &data->display;
>   	igt_crc_t ref_crc, new_crc;
> -	igt_fb_t afb;
>   	int afb_id;
>   	struct hdr_output_metadata hdr;
>   
>   	/* 10-bit formats are slow, so limit the size. */
>   	afb_id = igt_create_fb(data->fd, 512, 512,
> -			       format, DRM_FORMAT_MOD_LINEAR, &afb);
> +			       format, DRM_FORMAT_MOD_LINEAR, &data->afb);
>   	igt_assert(afb_id);
>   
> -	draw_hdr_pattern(&afb);
> +	draw_hdr_pattern(&data->afb);
>   
>   	/* Start in SDR. */
> -	igt_plane_set_fb(data->primary, &afb);
> +	igt_plane_set_fb(data->primary, &data->afb);
>   	igt_plane_set_size(data->primary, data->w, data->h);
>   	igt_output_set_prop_value(data->output, IGT_CONNECTOR_MAX_BPC, 8);
>   
> @@ -690,7 +687,6 @@ static void test_static_swap(data_t *data, igt_crtc_t *crtc,
>   	}
>   
>   	test_fini(data);
> -	igt_remove_fb(data->fd, &afb);
>   }
>   
>   static void test_invalid_metadata_sizes(data_t *data, igt_output_t *output)