[Mesa-dev] [PATCH v1 0/7] Implement commont gralloc_handle_t in libdrm

Tue Feb 20 10:26:56 UTC 2018

On Tue, Feb 20, 2018 at 6:51 PM, Robert Foss <robert.foss at collabora.com> wrote:
> Hey Tomasz,
>
> On 02/20/2018 09:55 AM, Tomasz Figa wrote:
>>
>> Hi Rob,
>>
>> On Fri, Feb 16, 2018 at 11:48 PM, Tomasz Figa <tfiga at chromium.org> wrote:
>>>
>>> On Fri, Feb 16, 2018 at 11:33 PM, Robert Foss <robert.foss at collabora.com>
>>> wrote:
>>>>
>>>> Hey Tomasz,
>>>>
>>>>
>>>> On 02/16/2018 05:10 AM, Tomaszzz Figa wrote:
>>>>>
>>>>>
>>>>> On Fri, Feb 9, 2018 at 11:06 PM, Rob Herring <robh at kernel.org> wrote:
>>>>>>
>>>>>>
>>>>>> On Fri, Feb 9, 2018 at 3:58 AM, Tomasz Figa <tfiga at chromi
>>>>>>>>>
>>>>>>>>> On Fri, Feb 2, 2018 at 2:01 AM, Tomasz Figa <tfiga at chromium.org>
>>>>>>>>>
>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Hi Rob,
>>>>>>>>>>
>>>>>>>>>> On Tue, Jan 30, 2018 at 9:36 PM, Robert Foss
>>>>>>>>>> <robert.foss at collabora.com> wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>         uint32_t (*get_fd)(buffer_handle_t handle, uint32_t
>>>>>>>>>>>>>>>> plane);
>>>>>>>>>>>>>>>>         uint64_t (*get_modifier)(buffer_handle_t handle,
>>>>>>>>>>>>>>>> uint32_t
>>>>>>>>>>>>>>>> plane);
>>>>>>>>>>>>>>>>         uint32_t (*get_offsets)(buffer_handle_t handle,
>>>>>>>>>>>>>>>> uint32_t
>>>>>>>>>>>>>>>> plane);
>>>>>>>>>>>>>>>>         uint32_t (*get_stride)(buffer_handle_t handle,
>>>>>>>>>>>>>>>> uint32_t
>>>>>>>>>>>>>>>> plane);
>>>>>>>>>>>>>>>>         ...
>>>>>>>>>>>>>>>> } gralloc_funcs_t;
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> These ones? >
>>>>>>>>>>>> Yeah, if we could retrieve such function pointer struct using
>>>>>>>>>>>> perform
>>>>>>>>>>>> or any equivalent (like the implementation-specific methods in
>>>>>>>>>>>> gralloc1, but not sure if that's going to be used in practice
>>>>>>>>>>>> anywhere), it could work for us.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> So this is where you and Rob Herring lose me, I don't think I
>>>>>>>>>>> understand
>>>>>>>>>>> quite how the gralloc1 call would be used, and how it would tie
>>>>>>>>>>> into
>>>>>>>>>>> this
>>>>>>>>>>> handle struct. I think I could do with some guidance on this.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> This would be very similar to gralloc0 perform call. gralloc1
>>>>>>>>>> implementations need to provide getFunction() callback [1], which
>>>>>>>>>> returns a pointer to given function. The list of standard
>>>>>>>>>> functions
>>>>>>>>>> is
>>>>>>>>>> defined in the gralloc1.h header [2], but we could take some
>>>>>>>>>> random
>>>>>>>>>> big number and use it for our function that fills in provided
>>>>>>>>>> gralloc_funcs_t struct with necessary pointers.
>>>>>>>>>>
>>>>>>>>>> [1]
>>>>>>>>>>
>>>>>>>>>> https://android.googlesource.com/platform/hardware/libhardware/+/master/include/hardware/gralloc1.h#300
>>>>>>>>>> [2]
>>>>>>>>>>
>>>>>>>>>> https://android.googlesource.com/platform/hardware/libhardware/+/master/include/hardware/gralloc1.h#134
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This is a deadend because it won't work with a HIDL based
>>>>>>>>> implementation (aka gralloc 2.0). You can't set function pointers
>>>>>>>>> (or
>>>>>>>>> any pointers) because gralloc runs in a different process. Yes,
>>>>>>>>> currently gralloc is a pass-thru HAL, but AIUI that will go away.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Part of it. I can't see IMapper being implemented by a separate
>>>>>>>> process. You can't map a buffer into one process from another
>>>>>>>> process.
>>>>>>>>
>>>>>>>> But anyway, it's a good point, thanks, I almost forgot about its
>>>>>>>> existence. I'll do further investigation.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Okay, so IMapper indeed breaks the approach I suggested. I'm not sure
>>>>>>> at the moment what we could do about it. (The idea of a dynamic
>>>>>>> library of a pre-defined name, exporting functions we specify, might
>>>>>>> still work, though.)
>>>>>>>
>>>>>>> Note that the DRM_GRALLOC_GET_FD used currently by Mesa will also be
>>>>>>> impossible to implement with IAllocator/IMapper. (Although I still
>>>>>>> think Mesa and Gralloc are free to have separate logic for choosing
>>>>>>> the DRM device to use.)
>>>>>>
>>>>>>
>>>>>>
>>>>>> I think the need for GET_FD goes away when the render node is used. We
>>>>>> may still need the card node for s/w rendering (if I can ever get that
>>>>>> working) though. Of course, if we use the vgem approach like CrOS then
>>>>>> we wouldn't.
>>>>>
>>>>>
>>>>>
>>>>> Hmm, if so, then we probably wouldn't have any strict need for these
>>>>> function pointers anymore. We already have a makeshift format resolve
>>>>> in place and the only missing bits that we still need to patch up
>>>>> downstream are removing GET_FD, dropping drm_gralloc.h and adding a
>>>>> fallback to kms_swrast if hw driver loading fails.
>>>>
>>>>
>>>>
>>>> So this discussion is slightly unrelated, but it is where me looking at
>>>> this
>>>> started.
>>>>
>>>> So I've got a kms_swrast fallback series[1], that I've been wanting to
>>>> test
>>>> before trying to push upstream, but haven't been able to run it in the
>>>> Android environment and the arc++ + chromiumos has also been problematic
>>>> for
>>>> various reasons (which are being looked into).
>>>>
>>>> Tomasz: If you're interested in testing the series, it would be helpful.
>>>> Hopefully testing is everything that needed for upstreaming.
>>>>
>>>> [1] https://gitlab.collabora.com/robertfoss/mesa/commits/kms_swrast
>>>
>>>
>>
>> I checked the branch, but it isn't possible to test it directly with
>> Chrome OS, since we do not provide DRM_GRALLOC_GET_FD functionality
>> and adding support for probing devices changes the code introduced by
>> your patches significantly, which kind of defeats the purpose.
>>
>> I think it might make sense to actually remove the requirement for
>> DRM_GRALLOC_GET_FD from upstream first (as in my original attempt from
>> long time ago) and then add kms_swrast fallback on top of that.
>>
>
> I would like to look into this,

That would be much appreciated!

> is your previous DRM_GRALLOC_GET_FD removal
> attempt available somewhere?

Yep.

https://patchwork.freedesktop.org/patch/102547/

>
> What kind of snags did you end up hitting? Just upstream approval or
> technical ones?

Well, something in the middle. The probing of render nodes didn't seem
to be very well received. Having the device path read from a property
was suggested, but it wouldn't work for us, since we use the same
Android image on different boards, where the order of probing of DRM
drivers might be different (and so the paths). Reading DRM driver name
from a property could work, though...

Best regards,
Tomasz