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

[Robert Foss]

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, is your previous DRM_GRALLOC_GET_FD removal 
attempt available somewhere?

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


Rob.