[PATCH xserver] edid: Prune duplicates after adding modes from DDC

[Emil Velikov]

On Wednesday, 22 February 2017, Michel Dänzer <michel at daenzer.net> wrote:

> From: Michel Dänzer <michel.daenzer at amd.com <javascript:;>>
>
> Multiple calls to xf86EdidMonitorSet (which can be triggered e.g. by
> running xrandr) would potentially keep adding the same modes, causing
> the Monitor->Modes list to keep growing larger and using up more memory.
>
> Fix this by calling xf86PruneDuplicateModes after adding the modes
> returned by xf86DDCGetModes. This removes any newly added modes which
> were already in the Monitor->Modes list before, but keeps new modes
> which weren't yet.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99521
> Signed-off-by: Michel Dänzer <michel.daenzer at amd.com <javascript:;>>
> ---
>
> Xiaogang / Jorge, does this fix the memory leak you're seeing?
>
>  hw/xfree86/modes/xf86EdidModes.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/hw/xfree86/modes/xf86EdidModes.c b/hw/xfree86/modes/
> xf86EdidModes.c
> index f0e1e974b..55833f886 100644
> --- a/hw/xfree86/modes/xf86EdidModes.c
> +++ b/hw/xfree86/modes/xf86EdidModes.c
> @@ -1198,21 +1198,19 @@ xf86EdidMonitorSet(int scrnIndex, MonPtr Monitor,
> xf86MonPtr DDC)
>          if (!Monitor->nHsync || !Monitor->nVrefresh)
>              DDCGuessRangesFromModes(scrnIndex, Monitor, Modes);
>
> -        /* look for last Mode */
> -        Mode = Modes;
> -
> -        while (Mode->next)
> -            Mode = Mode->next;
> -
>          /* add to MonPtr */
>          if (Monitor->Modes) {
>              Monitor->Last->next = Modes;
>              Modes->prev = Monitor->Last;
> -            Monitor->Last = Mode;
>          }
>          else {
>              Monitor->Modes = Modes;
> -            Monitor->Last = Mode;
>          }
> +
> +        xf86PruneDuplicateModes(Monitor->Modes);
> +

Wouldn't it be better to add the mode only if it doesn't exist, or it
doesn't really matter ?


> +        /* Update pointer to last mode */
> +        for (Mode = Monitor->Modes; Mode && Mode->next; Mode =
> Mode->next);
> +        Monitor->Last = Mode;

Perhaps I'm just tired but this looks like this will overwrite
Monitor->Last multiple times, right?

A simple while loop like the original code should do it.

-Emil
P.s. Sending from my phone - pardon the html formatting.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.x.org/archives/xorg-devel/attachments/20170223/2f517ebc/attachment.html>