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?
The code has explicit notes where map is leaked - see `git grep -10
"displaytarget_[un]*map" -- src/gallium/drivers/'
That said - no objections for changing the behaviour. Although lets:
a) have that as separate patch, and
b) drop the XXX/FIXME alongside it, and
c) optionally, fix the other sw winsys to do so
Thanks
Emil
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
