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
