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

[Chen, Xiaogang]

> -----Original Message-----
> From: Michel Dänzer [mailto:michel at daenzer.net]
> Sent: Monday, March 20, 2017 9:52 PM
> To: Chen, Xiaogang <Xiaogang.Chen at amd.com>
> Cc: xorg-devel at lists.x.org; jorge_monteagudo at hotmail.com
> Subject: Re: [PATCH xserver] edid: Prune duplicates after adding modes from
> DDC
> 
> On 20/03/17 02:32 PM, Chen, Xiaogang wrote:
> >> From: Michel Dänzer [mailto:michel at daenzer.net] On 23/02/17 06:46 AM,
> >> Chen, Xiaogang wrote:
> >>>> From: Michel Dänzer [mailto:michel at daenzer.net]
> >>>>
> >>>> 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>
> >>>> ---
> >>>>
> >>>> Xiaogang / Jorge, does this fix the memory leak you're seeing?
> >>>>
> >>> [XCHEN] Hi Michel: I tested your patch with same script I used
> >>> before (while true; do xrandr --verbose; done), the memory leak is
> >>> still there.
> >>
> >> I stepped through the execution of the xf86PruneDuplicateModes call
> >> added by my patch in gdb and verified that it works as intended,
> >> eliminating the duplicate modes.
> >>
> >> I suspect you're being mislead my the memleax tool: It reports any
> >> memory which is allocated and not freed within a certain period of
> >> time (10 seconds by default). However, that's expected to happen
> >> here, not necessarily a sign of a leak, since we're keeping the list
> >> of modes in
> >> Monitor->Modes. (xf86PruneDuplicateModes may delete the old modes
> >> Monitor->from
> >> the previous xf86DDCGetModes call, not the new ones allocated in the
> >> current
> >> call)
> > [XCHEN] I have same understanding on memleax tool: it reports memory
> > allocated and not freed in a certain time. It just gives clues where
> > the memory leaks may happen.
> > We claim there is memory leak by using top or htop cmd to see Xorg
> > virtual memory space size or system memory consumption. They keep
> > increasing with or without your patch.
> 
> I confirmed that my patch fixes the leak (technically not a leak but unbounded
> growth in memory allocation) in xf86EdidMonitorSet.
> 
[XCHEN] Do you mean your patch prevents unbounded memory growth or xf86PruneDuplicateModes does delete duplicated modes?
The original issue is unbounded memory growth if running xrandr in loop. If I delete the modes allocated at xf86EdidMonitorSet(as my patch did) I sought Xorg VM and system memory is stable by using top/htop when run xrandr in loop. I think it means these modes cause memory leak(these modes are never deallocated) since after delete these modes I did not see unbound memory growth any more by running xrandr in loop for long time.

Did you test your patch with running xrandr in loop and not see unbounded memory usage growth?

> You may be running into other similar issues. You can try isolating those e.g.
> with the valgrind vgdb technique I described.
> 
> (Note that in theory it's even possible for the virtual memory usage to keep
> growing without any leaks or similar issues, due to heap fragmentation. But at
> this point it seems more likely that there are other similar issues in the xserver
> code)
> 
> 
> --
> Earthling Michel Dänzer               |               http://www.amd.com
> Libre software enthusiast             |             Mesa and X developer