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

[Luca Coelho]

On Fri, 2024-12-20 at 10:06 +0000, Reddy Guddati, Santhosh wrote:
> 
> 
> 
> 
> 
> 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

You patch will work, of course, but what I'm trying to say is that
you're sorting the list and then reverting the sort in case it's not
supported.  And the only reason why you're sorting it, is to find the
largest possible mode.  It's unnecessary to sort the list.  If you
don't sort, but just look for what you want in the existing list, you
don't need to "backup" and "restore" the list in case of failures.

--
Cheers,
Luca.