[Mesa-dev] [RFC 7/7] radeon: remove screen ref counting

[Christian König]

Am 21.06.2016 um 15:53 schrieb Rob Herring:
> On Mon, Jun 20, 2016 at 1:15 PM, Christian König
> <deathsimple at vodafone.de> wrote:
>> Am 20.06.2016 um 17:39 schrieb Rob Herring:
>>> On Mon, Jun 20, 2016 at 9:27 AM, Christian König
>>> <deathsimple at vodafone.de> wrote:
>>>> Am 20.06.2016 um 16:13 schrieb Rob Herring:
>>>>> On Mon, Jun 20, 2016 at 8:31 AM, Nicolai Hähnle <nhaehnle at gmail.com>
>>>>> wrote:
>>>>>> On 17.06.2016 21:05, Rob Herring wrote:
>>>>>>> On Fri, Jun 17, 2016 at 1:45 PM, Emil Velikov
>>>>>>> <emil.l.velikov at gmail.com>
>>>>>>> wrote:
>>>>>>>> On 17 June 2016 at 18:45, Rob Herring <robh at kernel.org> wrote:
>>>>>>>>> Now that the pipe-loader is reference counting the screen creation,
>>>>>>>>> it
>>>>>>>>> is unnecessary to do in it the winsys/driver.
>>>>>>>
>>>>>>> [...]
>>>>>>>
>>>>>>>>> -static unsigned hash_dev(void *key)
>>>>>>>>> -{
>>>>>>>>> -#if defined(PIPE_ARCH_X86_64)
>>>>>>>>> -   return pointer_to_intptr(key) ^ (pointer_to_intptr(key) >> 32);
>>>>>>>>> -#else
>>>>>>>>> -   return pointer_to_intptr(key);
>>>>>>>>> -#endif
>>>>>>>>> -}
>>>>>>>>> -
>>>>>>>> As you can see above the hashing algo is different for AMDGPU. Not
>>>>>>>> familiar with the story behind any of this, so hopefully the AMD folk
>>>>>>>> will give you some insights.
>>>>>>>
>>>>>>> They are also hashing the fd in libdrm amdgpu_device_initialize(), so
>>>>>>> I thought this was redundant (unless you have an old libdrm).
>>>>>>
>>>>>> Correct. The idea is that there may be non-Mesa users of libdrm_amdgpu
>>>>>> (in
>>>>>> particular, the amdgpu-pro stack).
>>>>>>
>>>>>> The hashing in libdrm is different as well, though, and apparently on
>>>>>> purpose, see commit c68d58aa14b7f of libdrm. This might mean the series
>>>>>> doesn't work as is. Christian or Marek might know more.
>>>>> Okay, so the screen needs to be re-used across card and render nodes.
>>>>> Seems like other drivers could need that behavior as well and I should
>>>>> use the libdrm version.
>>>>
>>>> Well not necessarily. Reusing the screen between cards and render nodes
>>>> only
>>>> works if the driver can deal with that.
>>>>
>>>> E.g. for amdgpu we ran into the situation that you can create the screen
>>>> initially only with a render node and then get a DRI2 authenticated fd
>>>> later
>>>> on.
>>>>
>>>> What we do in libdrm is to remember the DRI2 authenticated fd separately
>>>> to
>>>> make use of it for flink import/exports. I fear that your cleanup would
>>>> break that behavior.
>>> Okay, I'll leave the amdgpu hashing code in place. AFAICT, the
>>> pipe-loader hashing should not impact it. It will only mean that
>>> amdgpu_device_initialize is called once per node.
>>
>> And I fear that exactly this will break it.
>>
>> See to work correctly the libdrm implementation must be called with both
>> file-descriptors. When it then return the same device for both you should
>> use the same screen object in mesa. Without this the Rander node/DRI2 dance
>> we do in the winsys will break.
>>
>> The only solution I can see is to have device specific callbacks to do the
>> hashing for you.
> I'm planning on doing something like this where it is opt in for each
> driver. This should mean the dynamic symbols don't need to change
> either for GL-vdpau interop.

That should work, but at least for amdgpu that won't give us much.

The compared element isn't the fd, but rather the drm device structure 
and that needs to stay.

Maybe make the intptr_to_pointer call driver dependent?.

Christian.

>
> struct pipe_screen *
> pipe_screen_reference(int fd)
> {
>     struct pipe_screen *pscreen;
>
>     if (!fd_tab) {
>        fd_tab = util_hash_table_create(hash_fd, compare_fd);
>        return NULL;
>     }
>
>     pscreen = util_hash_table_get(fd_tab, intptr_to_pointer(fd));
>     if (pscreen)
>        pipe_reference(NULL, &pscreen->reference);
>     return pscreen;
> }
>
> void pipe_screen_reference_init(struct pipe_screen *pscreen, int fd)
> {
>        if (!pscreen)
>           return;
>        pipe_reference_init(&pscreen->reference, 1);
>        util_hash_table_set(fd_tab, intptr_to_pointer(fd), pscreen);
> }
>
> Then the winsys create screen func will look like this minimally:
>
> struct pipe_screen *
> vc4_drm_screen_create(int fd)
> {
>          struct pipe_screen *pscreen = pipe_screen_reference(fd);
>          if (pscreen)
>              return pscreen;
>
>          pscreen = vc4_screen_create(dup(fd));
>          pipe_screen_reference_init(pscreen, fd);
>          return pscreen;
> }
>
> Rob