[Mesa-dev] [PATCH v4 1/2] gallium/winsys/kms: Fix possible leak in map/unmap.

[Lepton Wu]

I am fine to add ref count for map pointer but then the code looks a
little complex:
We already have ref count for display target, and it seems most other
drivers don't have a
ref count for map pointer. (I checked dri_sw_displaytarget_map /
gdi_sw_displaytarget_map/hgl_winsys_displaytarget_map
/xlib_displaytarget_map)

If you really want a reference count for map pointer, I will add one.
But I am just feeling that introduce  some unnecessary complex.
Just want to get confirmation before I begin to write code.

Thanks!

On Tue, Mar 13, 2018 at 8:41 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> On 13 March 2018 at 11:46, Tomasz Figa <tfiga at chromium.org> wrote:
>> On Thu, Mar 8, 2018 at 7:39 AM, Lepton Wu <lepton at 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.
>>>
>>> Change-Id: I65308f0ff2640bd57b2577c6a3469540c9722859
>>> Signed-off-by: Lepton Wu <lepton at chromium.org>
>>> ---
>>>  .../winsys/sw/kms-dri/kms_dri_sw_winsys.c     | 21 ++++++++++++-------
>>>  1 file changed, 14 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
>>> index 22e1c936ac5..7fc40488c2e 100644
>>> --- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
>>> +++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
>>> @@ -70,6 +70,7 @@ struct kms_sw_displaytarget
>>>
>>>     uint32_t handle;
>>>     void *mapped;
>>> +   void *ro_mapped;
>>>
>>>     int ref_count;
>>>     struct list_head link;
>>> @@ -198,16 +199,19 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
>>>        return NULL;
>>>
>>>     prot = (flags == PIPE_TRANSFER_READ) ? PROT_READ : (PROT_READ | PROT_WRITE);
>>> -   kms_sw_dt->mapped = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
>>> -                            kms_sw->fd, map_req.offset);
>>> -
>>> -   if (kms_sw_dt->mapped == MAP_FAILED)
>>> -      return NULL;
>>> +   void **ptr = (flags == PIPE_TRANSFER_READ) ? &kms_sw_dt->ro_mapped : &kms_sw_dt->mapped;
>>> +   if (!*ptr) {
>>> +      void *tmp = mmap(0, kms_sw_dt->size, prot, MAP_SHARED,
>>> +                       kms_sw->fd, map_req.offset);
>>> +      if (tmp == MAP_FAILED)
>>> +         return NULL;
>>> +      *ptr = tmp;
>>> +   }
>>>
>>>     DEBUG_PRINT("KMS-DEBUG: mapped buffer %u (size %u) at %p\n",
>>> -         kms_sw_dt->handle, kms_sw_dt->size, kms_sw_dt->mapped);
>>> +         kms_sw_dt->handle, kms_sw_dt->size, *ptr);
>>>
>>> -   return kms_sw_dt->mapped;
>>> +   return *ptr;
>>>  }
>>>
>>>  static struct kms_sw_displaytarget *
>>> @@ -278,9 +282,12 @@ kms_sw_displaytarget_unmap(struct sw_winsys *ws,
>>>     struct kms_sw_displaytarget *kms_sw_dt = kms_sw_displaytarget(dt);
>>>
>>>     DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->mapped);
>>> +   DEBUG_PRINT("KMS-DEBUG: unmapped buffer %u (was %p)\n", kms_sw_dt->handle, kms_sw_dt->ro_mapped);
>>>
>>>     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;
>>>  }
>>
>> If user calls map twice, wouldn't they also call unmap twice?
>> Moreover, wouldn't the pointer be expected to be still valid between
>> first and second unmap?
>>
>> The answer obviously depends on how the API is designed, but i feels
>> really weird being asymmetrical like that. Typically the mapping would
>> be refcounted and maps would have to be balanced with unmaps to free
>> the mapping.
>>
> Valid points.
>
> If you guys prefer we could land 2/2 (multiplane support), since it
> has no dependency of the mapping work.
> And polish out ro/rw mappings (even the leaks) at later stage, as time permits?
>
> -Emil