[PATCH i-g-t v1] lib/igt_kms: Backup connector modes before sorting

[Luca Coelho]

On Thu, 2024-12-19 at 21:24 +0530, Santhosh Reddy Guddati wrote:
> Backup connector modes before sorting to ensure the original mode
> is not changed if the joiner is not found.This will skip updating
> connector->mode[0] if joiner is not available.
> 
> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13287
> Signed-off-by: Santhosh Reddy Guddati <santhosh.reddy.guddati at intel.com>
> ---
>  lib/igt_kms.c | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 3d061abc5..21a6a4639 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -6394,6 +6394,11 @@ bool bigjoiner_mode_found(int drm_fd, drmModeConnector *connector,
>  			  int max_dotclock, drmModeModeInfo *mode)
>  {
>  	bool found = false;
> +	drmModeModeInfo *modes_backup;
> +
> +	modes_backup = malloc(connector->count_modes * sizeof(drmModeModeInfo));
> +	igt_assert(modes_backup);
> +	memcpy(modes_backup, connector->modes, connector->count_modes * sizeof(drmModeModeInfo));

It's usually considered safer to use the size of the instance and not
of the structure itself (though in practice it doesn't make much
difference): sizeof(*modes_backup).


>  	igt_sort_connector_modes(connector, sort_drm_modes_by_res_dsc);
>  	found = igt_bigjoiner_possible(drm_fd, &connector->modes[0], max_dotclock);
> @@ -6401,8 +6406,14 @@ bool bigjoiner_mode_found(int drm_fd, drmModeConnector *connector,
>  		igt_sort_connector_modes(connector, sort_drm_modes_by_clk_dsc);
>  		found = igt_bigjoiner_possible(drm_fd, &connector->modes[0], max_dotclock);
>  	}
> -	if (found)
> +	if (found) {
>  		*mode = connector->modes[0];
> +	} else {
> +		memcpy(connector->modes, modes_backup,
> +		       connector->count_modes * sizeof(drmModeModeInfo));
> +	}
> +
> +	free(modes_backup);
>  	return found;
>  }
>  
> @@ -6438,6 +6449,11 @@ bool ultrajoiner_mode_found(int drm_fd, drmModeConnector *connector,
>  			  int max_dotclock, drmModeModeInfo *mode)
>  {
>  	bool found = false;
> +	drmModeModeInfo *modes_backup;
> +
> +	modes_backup = malloc(connector->count_modes * sizeof(drmModeModeInfo));
> +	igt_assert(modes_backup);
> +	memcpy(modes_backup, connector->modes, connector->count_modes * sizeof(drmModeModeInfo));
>  
>  	igt_sort_connector_modes(connector, sort_drm_modes_by_res_dsc);
>  	found = igt_ultrajoiner_possible(&connector->modes[0], max_dotclock);
> @@ -6445,8 +6461,14 @@ bool ultrajoiner_mode_found(int drm_fd, drmModeConnector *connector,
>  		igt_sort_connector_modes(connector, sort_drm_modes_by_clk_dsc);
>  		found = igt_ultrajoiner_possible(&connector->modes[0], max_dotclock);
>  	}
> -	if (found)
> +	if (found) {
>  		*mode = connector->modes[0];
> +	} else {
> +		memcpy(connector->modes, modes_backup,
> +		       connector->count_modes * sizeof(drmModeModeInfo));
> +	}
> +
> +	free(modes_backup);
>  	return found;
>  }
>  

Generally this looks okay to fix the bug, but IMHO the actual problem
is that we shouldn't really be sorting the modes in order to find them.
Maybe it would be better to use an find_max_res_mode() or something
similar instead of using the sort function to find the mode you want?

--
Cheers,
Luca.