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

Tue Feb 20 19:03:32 UTC 2018

On Tue, Feb 20, 2018 at 4:26 AM, Tomasz Figa <tfiga at chromium.org> wrote:
> 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.

Perhaps worth revisiting. Given we've failed to progress at all since
then may change opinions some. We already have to handle multiple
opens share the same pipe_screen, so I don't think reusing the fd buys
us anything.

Maybe we're close to the point of removing the flink name support too.
The android-x86 folks have been working to get dma-bufs working.
Chih-Wei, any comments on this?

> 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...

How would that work if you support different GPUs in one image?

Rob