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

[Reddy Guddati, Santhosh]

________________________________
From: Luca Coelho <luca at coelho.fi>
Sent: Friday, December 20, 2024 3:13 PM
To: Reddy Guddati, Santhosh <santhosh.reddy.guddati at intel.com>; igt-dev at lists.freedesktop.org <igt-dev at lists.freedesktop.org>
Cc: B, Jeevan <jeevan.b at intel.com>; Joshi, Kunal1 <kunal1.joshi at intel.com>; Murthy, Arun R <arun.r.murthy at intel.com>
Subject: Re: [PATCH i-g-t v1] lib/igt_kms: Backup connector modes before sorting

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?

The intention of the original change was to identify if the output supports joiner, as part of this we are checking this is_joiner_mode, While doing so identified the problem to be sorting all the modes on the connector inside `ultrajoiner_mode_found`, this change avoids sorting the passed connector modes.

Please let me know your thoughts or questions.

https://patchwork.freedesktop.org/patch/627125/?series=141614&rev=6

Thanks,
Santhosh

--
Cheers,
Luca.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/igt-dev/attachments/20241220/0ec5f980/attachment-0001.htm>