[Mesa-dev] [PATCH 4/4] gbm: Add map/unmap functions

[Emil Velikov]

On 27 April 2016 at 19:51, Eric Anholt <eric at anholt.net> wrote:
> Rob Herring <robh at kernel.org> writes:
>
>> On Mon, Apr 25, 2016 at 7:53 PM, Eric Anholt <eric at anholt.net> wrote:
>>> Rob Herring <robh at kernel.org> writes:
>>>
>>>> On Fri, Apr 22, 2016 at 9:08 PM, Rob Herring <robh at kernel.org> wrote:
>>>>> On Fri, Apr 22, 2016 at 6:32 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>>>>> Hi Rob,
>>>>>>
>>>>>> On 22 April 2016 at 16:50, Rob Herring <robh at kernel.org> wrote:
>>>>>>> This adds map and unmap functions to GBM utilizing the DRIimage extension
>>>>>>> mapImage/unmapImage functions or existing internal mapping for dumb
>>>>>>> buffers.
>>>>>> Ftr that this is quite sensitive and apart from the obvious breakage
>>>>>> (coming in a second) it will need some testing on a gnome-continuous
>>>>>> setup (iirc some used to hand out in #xorg-devel)
>>>>>>
>>>>>>> Unlike prior attempts, this version provides a region to map and
>>>>>>> usage flags for the mapping. The operation follows the same semantics as
>>>>>>> the gallium transfer_map() function.
>>>>>>>
>>>>>>> This was tested with GBM based gralloc on Android.
>>>>>>>
>>>>>>> This still creates a context, but I've moved it into gbm_create_device
>>>>>>> rather than in the map function. This should remove any need for reference
>>>>>>> counting and problems with memory leaks.
>>>>>>>
>>>>>>> Signed-off-by: Rob Herring <robh at kernel.org>
>>>>>
>>>>> [...]
>>>>>
>>>>
>>>>>>> @@ -1004,6 +1058,10 @@ dri_device_create(int fd)
>>>>>>>     if (ret)
>>>>>>>        goto err_dri;
>>>>>>>
>>>>>>> +   if (dri->image->base.version >= 12)
>>>>>>> +      dri->context = dri->dri2->createNewContext(dri->screen, NULL,
>>>>>>> +                                                 NULL, NULL);
>>>>>>> +
>>>>>> Have you measured how much this costs us (cpu time and/or memory) ?
>>>>>
>>>>> No, will do.
>>>>
>>>> On Android (x86_64 + virgl), it is 2ms and ~2MB (out of 20). A
>>>> standalone test with swrast is 4ms and ~4MB. I measured with
>>>> getrusage().
>>>
>>> Given that existing clients of GBM don't use this API, it's not OK to
>>> add this cost to all of them.
>>
>> Agreed.
>>
>>> If you need pthreads to protect the allocation check at map time,
>>
>> I do...
>>
>>> there's this bit of configure.ac from libdrm so that you don't need to
>>> force libgbm to pull in real pthreads and its overhead:
>>>
>>> PKG_CHECK_MODULES(PTHREADSTUBS, pthread-stubs)
>>> AC_SUBST(PTHREADSTUBS_CFLAGS)
>>> AC_SUBST(PTHREADSTUBS_LIBS)
>>
Side note: pthread-stubs expands to a single .pc file on GLIBC (Linux
in general ?) platforms. That's because libc.so already has these
stubs.

>> GBM requires DRI which already pulls in pthreads, so I don't think
>> this is needed. Or am I missing something?
>
> I'm fine with that, just noting that my libgbm currently doesn't link
> pthreads, and providing a solution if people didn't want to force
> linking pthreads for libgbm.so.
Not only yours - libgbm does not link against pthreads.so. Period.
The DRI modules on the other hand (amongst others) do.

If we're interested in making things mesa pthreads.so free - I've
prepped a bit of a summary on the topic.

https://lists.freedesktop.org/archives/mesa-dev/2016-April/114425.html

Feel free to chip in.

Regards,
Emil