On Tue, Mar 20, 2018 at 10:44 PM, Emil Velikov <emil.l.veli...@gmail.com> wrote:
> On 20 March 2018 at 04:40, Tomasz Figa <tf...@chromium.org> wrote:
>> On Tue, Mar 20, 2018 at 2:55 AM, Emil Velikov <emil.l.veli...@gmail.com>
>> wrote:
>>> Hi Lepton,
>>>
>>> On 19 March 2018 at 17:33, Lepton Wu <lep...@chromium.org> wrote:
>>>> If user calls map twice for kms_sw_displaytarget, the first mapped
>>>> buffer could get leaked. Instead of calling mmap every time, just
>>>> reuse previous mapping. Since user could map same displaytarget with
>>>> different flags, we have to keep two different pointers, one for rw
>>>> mapping and one for ro mapping. Also introduce reference count for
>>>> mapped buffer so we can unmap them at right time.
>>>>
>>>> Reviewed-by: Emil Velikov <emil.veli...@collabora.com>
>>>> Reviewed-by: Tomasz Figa <tf...@chromium.org>
>>>> Signed-off-by: Lepton Wu <lep...@chromium.org>
>>>
>>> Nit: normally it's a good idea to have brief revision log when sending
>>> new version:
>>> v2:
>>> - split from larger patch (Emil)
>>> v3:
>>> - remove munmap w/a from dt_destory(Emil)
>>> ...
>>>
>>>> @@ -170,6 +172,14 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
>>>> if (kms_sw_dt->ref_count > 0)
>>>> return;
>>>>
>>>> + if (kms_sw_dt->map_count > 0) {
>>>> + DEBUG_PRINT("KMS-DEBUG: fix leaked map buffer %u\n",
>>>> kms_sw_dt->handle);
>>>> + munmap(kms_sw_dt->mapped, kms_sw_dt->size);
>>>> + kms_sw_dt->mapped = NULL;
>>>> + munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
>>>> + kms_sw_dt->ro_mapped = NULL;
>>>> + }
>>>> +
>>> I could swear this workaround was missing in earlier revisions. I
>>> don't see anything in Tomasz' reply that suggesting we should bring it
>>> back?
>>> AFAICT the added refcounting makes no difference - the driver isn't
>>> cleaning up after itself.
>>>
>>> Am I missing something?
>>
>> I think this is actually consistent with what other winsys
>> implementations do. They free the map (or shadow malloc/shm buffer) in
>> _destroy() callback, so we should probably do the same.
>>
> Looking at the SW winsys - none of them seem to unmap at destroy time.
> Perhaps you meant that the HW ones do?
dri:
https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/winsys/sw/dri/dri_sw_winsys.c#n128
gdi:
https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/winsys/sw/gdi/gdi_sw_winsys.c#n116
hgl:
https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/winsys/sw/hgl/hgl_sw_winsys.c#n152
xlib:
https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/winsys/sw/xlib/xlib_sw_winsys.c#n260
https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/winsys/sw/xlib/xlib_sw_winsys.c#n271
The don't do real mapping - they all work on locally allocated memory.
However, after destroy, no resources are leaked and the pointers
returned from _map() are not valid anymore.
On the other hand, wrapper only releases a reference to pipe_resource,
so the related transfer remains valid:
https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/winsys/sw/wrapper/wrapper_sw_winsys.c#n266
Best regards,
Tomasz
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
