[igt-dev] [i-g-t, v4 2/8] tests/amdgpu/amd_freesync_video_mode: Fix memory leak and corruption

Alex Hung alex.hung at amd.com
Mon Jul 10 17:16:11 UTC 2023


Reviewed-by: Alex Hung <alex.hung at amd.com>

On 2023-07-10 01:57, Tom Chung wrote:
> [Why]
> There are some memory leak and corruption during the test.
> 
> [How]
> 1. Memory allocated for cicle_sprite.data is too small for drawing the circle.
>     Adjust the circle radius from 50 to 48 to fit the cicle_sprite.data size.
> 2. Pointer preferred_mode does not point to a correct location.
>     Use the data->hdisplay and data->vdisplay instead of it.
> 3. Fix some code for boundary issues.
> 4. Add some error check for potential memory allocate failed.
> 5. Free some allocated memory and resources before exit test.
> 6. Use memcpy() to copy the data->modes data.
> 
> Signed-off-by: Tom Chung <chiahsuan.chung at amd.com>
> ---
>   tests/amdgpu/amd_freesync_video_mode.c | 60 +++++++++++++++++---------
>   1 file changed, 39 insertions(+), 21 deletions(-)
> 
> diff --git a/tests/amdgpu/amd_freesync_video_mode.c b/tests/amdgpu/amd_freesync_video_mode.c
> index 805988c19..be1bf4944 100644
> --- a/tests/amdgpu/amd_freesync_video_mode.c
> +++ b/tests/amdgpu/amd_freesync_video_mode.c
> @@ -326,8 +326,8 @@ static void sprite_anim_init(void)
>   	sprite_init(&cicle_sprite, 100, 100);
>   
>   	sprite_draw_rect(&cicle_sprite, 0, 0, 100, 100, MK_COLOR(128, 128, 128));
> -	/* draw filled circle with center (50, 50), radius 50. */
> -	sprite_draw_circle(&cicle_sprite, 50, 50, 50, MK_COLOR(0, 0, 255));
> +	/* draw filled circle with center (50, 50), radius 48. */
> +	sprite_draw_circle(&cicle_sprite, 50, 50, 48, MK_COLOR(0, 0, 255));
>   }
>   
>   static void sprite_anim(data_t *data, uint32_t *addr)
> @@ -414,7 +414,7 @@ static drmModeModeInfo *select_mode(
>   				break;
>   			}
>   		}
> -		if (i == data->count_modes)
> +		if (i >= data->count_modes)
>   			mode = NULL;
>   		break;
>   
> @@ -426,7 +426,7 @@ static drmModeModeInfo *select_mode(
>   				break;
>   			}
>   		}
> -		if (i == data->count_modes)
> +		if (i >= data->count_modes)
>   			mode = NULL;
>   		break;
>   
> @@ -506,7 +506,7 @@ static uint64_t nsec_per_frame(uint64_t refresh)
>   static range_t
>   get_vrr_range(data_t *data, igt_output_t *output)
>   {
> -	char buf[256];
> +	char buf[256] = {0};
>   	char *start_loc;
>   	int fd, res;
>   	range_t range;
> @@ -557,19 +557,25 @@ static void prepare_test(
>   
>   	/* Prepare resources */
>   	if (!data->fb_initialized) {
> -		igt_create_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
> -				DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR, &data->fbs[0]);
> +		int fb_id;
> +
> +		fb_id = igt_create_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
> +			DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR, &data->fbs[0]);
> +		igt_assert(fb_id);
> +
> +		fb_id = igt_create_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
> +			DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR, &data->fbs[1]);
> +		igt_assert(fb_id);
>   
> -		igt_create_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
> -				DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR, &data->fbs[1]);
>   		data->fb_mem[0] = igt_fb_map_buffer(data->drm_fd, &data->fbs[0]);
>   		data->fb_mem[1] = igt_fb_map_buffer(data->drm_fd, &data->fbs[1]);
> +
> +		fbmem_draw_smpte_pattern(data->fb_mem[0], data->hdisplay, data->vdisplay);
> +		fbmem_draw_smpte_pattern(data->fb_mem[1], data->hdisplay, data->vdisplay);
> +
>   		data->fb_initialized = true;
>   	}
>   
> -	fbmem_draw_smpte_pattern(data->fb_mem[0], data->hdisplay, data->vdisplay);
> -	fbmem_draw_smpte_pattern(data->fb_mem[1], data->hdisplay, data->vdisplay);
> -
>   	/* Take care of any required modesetting before the test begins. */
>   	data->primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
>   	igt_plane_set_fb(data->primary, &data->fbs[0]);
> @@ -677,21 +683,21 @@ flip_and_measure(
>   static void init_data(data_t *data, igt_output_t *output)
>   {
>   	int i;
> -	uint32_t pm_hdisplay, pm_vdisplay, max_clk = 0;
> -	drmModeModeInfo *preferred_mode;
> +	uint32_t max_clk = 0;
>   	drmModeConnector *connector;
>   
>   	connector = data->connector = output->config.connector;
>   	data->count_modes = connector->count_modes;
>   	data->modes = (drmModeModeInfo *)malloc(sizeof(drmModeModeInfo) * data->count_modes);
> +	igt_assert(data->modes);
> +	memcpy(data->modes, connector->modes, sizeof(drmModeModeInfo) * data->count_modes);
>   
> -	for (i = 0; i < data->count_modes; i++) {
> -		data->modes[i] = connector->modes[i];
>   #ifdef FSV_DEBUG
> +	for (i = 0; i < data->count_modes; i++) {
>   		igt_info("mode %d:", i);
>   		kmstest_dump_mode(&data->modes[i]);
> -#endif
>   	}
> +#endif
>   
>   	/* searching the preferred mode */
>   	for (i = 0; i < connector->count_modes; i++) {
> @@ -701,8 +707,6 @@ static void init_data(data_t *data, igt_output_t *output)
>   			data->preferred_mode_index = i;
>   			data->hdisplay = mode->hdisplay;
>   			data->vdisplay = mode->vdisplay;
> -			pm_hdisplay = preferred_mode->hdisplay;
> -			pm_vdisplay = preferred_mode->vdisplay;
>   			break;
>   		}
>   	}
> @@ -711,7 +715,7 @@ static void init_data(data_t *data, igt_output_t *output)
>   	for (i = 0; i < connector->count_modes; i++) {
>   		drmModeModeInfo *mode = &connector->modes[i];
>   
> -		if (mode->hdisplay == pm_hdisplay && mode->vdisplay == pm_vdisplay) {
> +		if (mode->hdisplay == data->hdisplay && mode->vdisplay == data->vdisplay) {
>   			if (mode->clock > max_clk) {
>   				max_clk = mode->clock;
>   				data->base_mode_index = i;
> @@ -742,11 +746,22 @@ static void finish_test(data_t *data, enum pipe pipe, igt_output_t *output)
>   	igt_fb_unmap_buffer(&data->fbs[0], data->fb_mem[0]);
>   	igt_remove_fb(data->drm_fd, &data->fbs[1]);
>   	igt_remove_fb(data->drm_fd, &data->fbs[0]);
> +
> +	data->fb_initialized = false;
> +	if (data->modes) {
> +		free(data->modes);
> +		data->modes = NULL;
> +	}
> +	if (cicle_sprite.data) {
> +		free(cicle_sprite.data);
> +		memset(&cicle_sprite, 0, sizeof(cicle_sprite));
> +	}
>   }
>   
>   static void
>   mode_transition(data_t *data, enum pipe pipe, igt_output_t *output, uint32_t scene)
>   {
> +	int ret;
>   	uint32_t result;
>   	uint64_t interval;
>   	drmModeModeInfo *mode_start = NULL, *mode_playback = NULL, mode_custom;
> @@ -770,7 +785,8 @@ mode_transition(data_t *data, enum pipe pipe, igt_output_t *output, uint32_t sce
>   		break;
>   	case SCENE_BASE_MODE_TO_CUSTUM_MODE:
>   		mode_start = select_mode(data, FSV_BASE_MODE, 0);
> -		prepare_custom_mode(data, &mode_custom, 72);
> +		ret = prepare_custom_mode(data, &mode_custom, 72);
> +		igt_assert_eq(ret, 0);
>   		mode_playback = &mode_custom;
>   		break;
>   	case SCENE_NON_FSV_MODE_TO_NON_FSV_MODE:
> @@ -862,5 +878,7 @@ igt_main {
>   	igt_fixture {
>   		igt_display_fini(&data.display);
>   		drm_close_driver(data.drm_fd);
> +		if (data.modes)
> +			free(data.modes);
>   	}
>   }


More information about the igt-dev mailing list