Hi Lepton,
Please avoid top-posting in mailing lists in the future. It's against
the netiquette.
On Wed, Mar 14, 2018 at 10:20 AM, Lepton Wu <lep...@chromium.org> wrote:
[Message moved to bottom]
> On Tue, Mar 13, 2018 at 8:41 AM, Emil Velikov <emil.l.veli...@gmail.com>
> wrote:
>> On 13 March 2018 at 11:46, Tomasz Figa <tf...@chromium.org> wrote:
>>> On Thu, Mar 8, 2018 at 7:39 AM, 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.
>>>>
>>>> Change-Id: I65308f0ff2640bd57b2577c6a3469540c9722859
>>>> Signed-off-by: Lepton Wu <lep...@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
>
> 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.
Looking at users, I found at least one that it's quite realistic to
have two separate maps created and expected to work, because of
semantics of pipe->transfer_map() API:
https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/drivers/softpipe/sp_texture.c#n355
You can see that every time it creates a new, independent
pipe_transfer object, which should be expected to be valid until
pipe->transfer_unmap() on this particular object is not called.
Also, looking at the implementations you mentioned:
1) dri_sw
The real buffer is not being mapped. There is a staging buffer
allocated with malloc() at display target creation time and data are
read-back to it in dri_sw_displaytarget_map() and written-back to the
real buffer in dri_sw_displaytarget_unmap(). The dri_sw_dt->mapped is
not needed for anything.
2) gdi_sw, hgl_winsys
The buffer is allocated by malloc_aligned() and so map() simply
returns that pointer and unmap() is a no-op.
3) xlib
The buffer is either an shmem or malloc buffer and the behavior is
essentially the same as 2). The xlib_dt->mapped variable is not needed
for anything.
So, if we are mapping the real buffer, we need to reference count the mapping.
Best regards,
Tomasz
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
