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

[Lepton Wu]

I misunderstanding your point, now I got it and will do more test to
see if removing this hunk will cause some issue
in some cases of multi plane code path. Thanks.

On Wed, Mar 7, 2018 at 10:14 AM, Lepton Wu <lepton at chromium.org> wrote:
> Actually the reason why I need this CL:
>
> In multi plane patch I'd like to only mmap once for different planes
> of same buffer. So actually I need some way
> to reuse same mmap for different planes. Then it's natural to have
> this CL.  The fix to leak is a side effect of this CL.
> dt_unmap still works with this CL,  if user call dt_map for single
> plane buffer multiple times,  they will get same pointer, and if they
> call dt_unmap
> ,  with this CL, it still get unmapped.
>
> On Wed, Mar 7, 2018 at 7:14 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> On 6 March 2018 at 22:43, 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     | 26 ++++++++++++++-----
>>>  1 file changed, 19 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..30343222470 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;
>>> @@ -170,6 +171,11 @@ kms_sw_displaytarget_destroy(struct sw_winsys *ws,
>>>     if (kms_sw_dt->ref_count > 0)
>>>        return;
>>>
>>> +   if (kms_sw_dt->ro_mapped)
>>> +      munmap(kms_sw_dt->ro_mapped, kms_sw_dt->size);
>>> +   if (kms_sw_dt->mapped)
>>> +      munmap(kms_sw_dt->mapped, kms_sw_dt->size);
>>> +
>> I'm not 100% sure about this. There's a reason why dt_unmap exists.
>>
>> Skimming through the existing code [1] - there's a handful of cases
>> that indicate the leaks you're hitting.
>> I'd look into addressing those properly because as-is this looks alike
>> a duck-taping the problem.
>>
>> With the hunk gone the patch is
>> Reviewed-by: Emil Velikov <emil.velikov at collabora.com>
>>
>> -Emil
>>
>> [1] $ git grep -20 -w displaytarget_[a-z]*map