On 20 March 2018 at 14:24, Tomasz Figa <tf...@chromium.org> wrote: > 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. > As mentioned before - zero objections against changing that, but keep it separate patch. Pretty please?
-Emil _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev