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.
Best regards,
Tomasz
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
