<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
<style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style>
</head>
<body dir="ltr">
<div class="elementToProof" style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
<br>
</div>
<div id="appendonsend"></div>
<div style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
<br>
</div>
<hr style="display: inline-block; width: 98%;">
<div id="divRplyFwdMsg" dir="ltr"><span style="font-family: Calibri, sans-serif; font-size: 11pt; color: rgb(0, 0, 0);"><b>From:</b> Luca Coelho <luca@coelho.fi><br>
<b>Sent:</b> Friday, December 20, 2024 3:13 PM<br>
<b>To:</b> Reddy Guddati, Santhosh <santhosh.reddy.guddati@intel.com>; igt-dev@lists.freedesktop.org <igt-dev@lists.freedesktop.org><br>
<b>Cc:</b> B, Jeevan <jeevan.b@intel.com>; Joshi, Kunal1 <kunal1.joshi@intel.com>; Murthy, Arun R <arun.r.murthy@intel.com><br>
<b>Subject:</b> Re: [PATCH i-g-t v1] lib/igt_kms: Backup connector modes before sorting</span>
<div> </div>
</div>
<div class="elementToProof" style="font-size: 11pt;">On Thu, 2024-12-19 at 21:24 +0530, Santhosh Reddy Guddati wrote:<br>
> Backup connector modes before sorting to ensure the original mode<br>
> is not changed if the joiner is not found.This will skip updating<br>
> connector->mode[0] if joiner is not available.<br>
><br>
> Closes: <a href="https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13287" id="OWAb8421631-352b-be0e-a191-416176034536" class="OWAAutoLink" data-auth="NotApplicable">
https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13287</a><br>
> Signed-off-by: Santhosh Reddy Guddati <santhosh.reddy.guddati@intel.com><br>
> ---<br>
> lib/igt_kms.c | 26 ++++++++++++++++++++++++--<br>
> 1 file changed, 24 insertions(+), 2 deletions(-)<br>
><br>
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c<br>
> index 3d061abc5..21a6a4639 100644<br>
> --- a/lib/igt_kms.c<br>
> +++ b/lib/igt_kms.c<br>
> @@ -6394,6 +6394,11 @@ bool bigjoiner_mode_found(int drm_fd, drmModeConnector *connector,<br>
> int max_dotclock, drmModeModeInfo *mode)<br>
> {<br>
> bool found = false;<br>
> + drmModeModeInfo *modes_backup;<br>
> +<br>
> + modes_backup = malloc(connector->count_modes * sizeof(drmModeModeInfo));<br>
> + igt_assert(modes_backup);<br>
> + memcpy(modes_backup, connector->modes, connector->count_modes * sizeof(drmModeModeInfo));<br>
<br>
It's usually considered safer to use the size of the instance and not<br>
of the structure itself (though in practice it doesn't make much<br>
difference): sizeof(*modes_backup).<br>
<br>
<br>
> igt_sort_connector_modes(connector, sort_drm_modes_by_res_dsc);<br>
> found = igt_bigjoiner_possible(drm_fd, &connector->modes[0], max_dotclock);<br>
> @@ -6401,8 +6406,14 @@ bool bigjoiner_mode_found(int drm_fd, drmModeConnector *connector,<br>
> igt_sort_connector_modes(connector, sort_drm_modes_by_clk_dsc);<br>
> found = igt_bigjoiner_possible(drm_fd, &connector->modes[0], max_dotclock);<br>
> }<br>
> - if (found)<br>
> + if (found) {<br>
> *mode = connector->modes[0];<br>
> + } else {<br>
> + memcpy(connector->modes, modes_backup,<br>
> + connector->count_modes * sizeof(drmModeModeInfo));<br>
> + }<br>
> +<br>
> + free(modes_backup);<br>
> return found;<br>
> }<br>
> <br>
> @@ -6438,6 +6449,11 @@ bool ultrajoiner_mode_found(int drm_fd, drmModeConnector *connector,<br>
> int max_dotclock, drmModeModeInfo *mode)<br>
> {<br>
> bool found = false;<br>
> + drmModeModeInfo *modes_backup;<br>
> +<br>
> + modes_backup = malloc(connector->count_modes * sizeof(drmModeModeInfo));<br>
> + igt_assert(modes_backup);<br>
> + memcpy(modes_backup, connector->modes, connector->count_modes * sizeof(drmModeModeInfo));<br>
> <br>
> igt_sort_connector_modes(connector, sort_drm_modes_by_res_dsc);<br>
> found = igt_ultrajoiner_possible(&connector->modes[0], max_dotclock);<br>
> @@ -6445,8 +6461,14 @@ bool ultrajoiner_mode_found(int drm_fd, drmModeConnector *connector,<br>
> igt_sort_connector_modes(connector, sort_drm_modes_by_clk_dsc);<br>
> found = igt_ultrajoiner_possible(&connector->modes[0], max_dotclock);<br>
> }<br>
> - if (found)<br>
> + if (found) {<br>
> *mode = connector->modes[0];<br>
> + } else {<br>
> + memcpy(connector->modes, modes_backup,<br>
> + connector->count_modes * sizeof(drmModeModeInfo));<br>
> + }<br>
> +<br>
> + free(modes_backup);<br>
> return found;<br>
> }<br>
> <br>
<br>
Generally this looks okay to fix the bug, but IMHO the actual problem<br>
is that we shouldn't really be sorting the modes in order to find them.<br>
Maybe it would be better to use an find_max_res_mode() or something<br>
similar instead of using the sort function to find the mode you want?</div>
<div class="elementToProof" style="font-size: 11pt;"><br>
</div>
<div class="elementToProof" style="font-size: 11pt;">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 `<span style="font-family: Aptos, Aptos_EmbeddedFont, Aptos_MSFontService, Calibri, Helvetica, sans-serif; color: rgb(0, 0, 0);">ultrajoiner_mode_found`</span>, this change avoids sorting the passed connector modes.</div>
<div class="elementToProof" style="font-size: 11pt;"><br>
Please let me know your thoughts or questions.<br>
<br>
</div>
<div class="elementToProof" style="font-size: 11pt;"><span style="color: rgb(0, 0, 0);"><a href="https://patchwork.freedesktop.org/patch/627125/?series=141614&rev=6" id="LPlnk822311">https://patchwork.freedesktop.org/patch/627125/?series=141614&rev=6</a><br>
</span><br>
</div>
<div class="elementToProof" style="font-size: 11pt; color: rgb(0, 0, 0);">Thanks,<br>
Santhosh</div>
<div class="elementToProof" style="font-size: 11pt;"><br>
--<br>
Cheers,<br>
Luca.</div>
</body>
</html>