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