[Mesa-dev] [PATCH] egl: android: add dma-buf fd support

[Emil Velikov]

On 20 April 2016 at 03:52, Rob Herring <robh at kernel.org> wrote:
> On Tue, Apr 19, 2016 at 8:03 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>  Hi Rob,
>>
>> Please bear in mind that there's a fair bit of comments, but before
>> all don't mix refactoring and new code. Please ?
>
> Okay.
>
Thanks.

>> On 15 April 2016 at 17:03, Rob Herring <robh at kernel.org> wrote:
>>> Add support for creating images from Android native buffers with dma-buf
>>> fd. As dma-buf support also requires DRI image loader extension, add
>>> that as well.
>>>
>>> This is based on several originally patches written by Varad Gautam.
>>> I've collapsed them down to one and done a bit of reformatting. dma-bufs
>>> and GEM handles are now both supported instead of being compile time
>>> selection.
>> How did you test this ? Afaict making this (at least in current shape)
>> isn't possible to be a runtime decision.
>
> For the GEM handle case, I just tried not to change things. I think
> the android-x86 folks have tested a similar version of this and they
> would use the GEM handle case.
>
Last time I've looked things did differ a fair bit wrt this patch. Not
a big deal though.


> What exactly determines libEGL and dri module are dmabuf capable? The
> image loader extension?
>
>From the loader side: presence of the image_loader extension +
detection of the module's image extension. And the opposite from the
module side - exposes image extension + uses the image_loader one.

>>
>>> +      EGLint attr_list[14];
>>> +      attr_list[0] = EGL_WIDTH;
>>> +      attr_list[1] = buf->width;
>>> +      attr_list[2] = EGL_HEIGHT;
>>> +      attr_list[3] = buf->height;
>>> +      attr_list[4] = EGL_LINUX_DRM_FOURCC_EXT;
>>> +      attr_list[5] = get_fourcc(get_format(buf->format));
>>> +      attr_list[6] = EGL_DMA_BUF_PLANE0_FD_EXT;
>>> +      attr_list[7] = fd;
>>> +      attr_list[8] = EGL_DMA_BUF_PLANE0_PITCH_EXT;
>>> +      attr_list[9] = buf->stride * get_format_bpp(buf->format);
>>> +      attr_list[10] = EGL_DMA_BUF_PLANE0_OFFSET_EXT;
>>> +      attr_list[11] = 0;
>>> +      attr_list[12] = EGL_NONE;
>>> +
>> Just throw these directly into a const attr_list[].
>
> How can it be const when most of the values are variables?
>
Something like below. It is a valid C, right ?

const EGLint attr_list[14] = {
   EGL_WIDTH, buf->width,
   EGL_HEIGHT, .....

};


>
> So dri2_loader and image loader are mutually exclusive?
>
Sadly there are there more than these two, but yes. If you have (start
using one) you cannot opt for the other at a later stage.


>> Looking at the other backends - wayland does a related thing:
>>  - Open a node
>>  - Am I render node ? No, add the dri2_loader extension to the list.
>
> Okay. Could I key off of that for dma-buf support as well?
>
Cannot really parse that. Please elaborate.

If you're asking "can I use the wayland approach/heuristics" - yes you
can. Search for drmGetNodeTypeFromFd.


> I'm not really a fan of adding yet another thing to Android device
> config that has to be set correctly or things fall over. There are
> enough already. Surely, the code can be smart enough to do the right
> thing based on which node or drv PRIME support.
>
Indeed it can if one uses the wayland approach. Namely: determine
"render/dmabuf or gem" based on the node type and populate libEGL
extension list appropriately.

>> Looking at this patch and Varad's work
>> there a hunk missing here [1]. Did you not come across the issue in
>> question ?
>
> I don't think it is a real issue. I traced the code for
> createNewDrawable and it only saves dri2_surf ptr, but never
> references the contents of it. And I didn't find any problems when
> reverting it and testing, so I dropped that patch.
>
Fair enough. Varad can you confirm if you've seen any actual uses for
[1] or was it based upon code observation ?

>>
>> Apologies that I'm the bearer of bad news and thank you for working on this.
>
> Apologies for pretending I know what I'm doing. :)
>
Not a problem. It's not like you've done any of it with ill intent.
Plus I'm happy to share as much knowledge I have in the area.

-Emil

[1] https://github.com/varadgautam/mesa/commit/96c533fd62db4bb21f0a778ad16848da2df24fd6