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

Wed Feb 21 04:19:54 UTC 2018

On Wed, Feb 21, 2018 at 4:03 AM, Rob Herring <robh at kernel.org> wrote:
> 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.

It is actually incorrect to have the same device FD used for different
screens, if PRIME is used, due to GEM namespace issues, e.g.
- screen 0: drmPrimeFdToHandle(buffer A) -> 1,
- screen 1: drmPrimeFdToHandle(buffer A) -> 1 (same handle in same
namespace, due to semantics of PRIME import and same device FD used),
- screen 0: GEM_CLOSE(handle = 1),
- screen 1 still thinks handle 1 is valid...

So this only works for control nodes using flink only. (Or if you
always use libdrm_* for BO management and your particular
implementation does its own GEM handle reference counting, but that's
not guaranted.)

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

It wouldn't and that's why I prefer probing available devices.

For Chrome OS, we don't include GPU bits in system image, but rather
have vendor images built separately for each board (although sharing
any possible contents across board family, chipset, GPU type and so
on), so we can have a custom .rc script (which sets a property) as
well. It would even be possible to have paths hardcoded, but that
would be prone to probe ordering issues which, with software fallback,
could be actually not obvious to notice, and so we'd rather not go
this route.

So I'd agree here that we should revisit probing.

Best regards,
Tomasz