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

[Michel Dänzer]

On 21/03/17 01:09 PM, Chen, Xiaogang wrote:
>> From: Michel Dänzer [mailto:michel at daenzer.net]
>> 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?

Yes. As I described in detail, running xrandr does result in additional
allocated memory from xf86EdidMonitorSet without my patch, but none with
my patch. It's the same whether xrandr is run once or hundreds of times
or indeed any number of times.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer