[Mesa-dev] [PATCH 3/3] targets/va: export radeon winsys_create functions

[Emil Velikov]

On 24 March 2017 at 14:42, Marek Olšák <maraeo at gmail.com> wrote:
> On Fri, Mar 24, 2017 at 12:24 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> On 24 March 2017 at 11:02, Nicolai Hähnle <nhaehnle at gmail.com> wrote:
>>> On 24.03.2017 01:00, Marek Olšák wrote:
>>>>
>>>> From: Marek Olšák <marek.olsak at amd.com>
>>>>
>>>> This should fix this radeonsi error:
>>>>   "mesa: for the -simplifycfg-sink-common option: may only occur zero or
>>>> one
>>>>    times!"
>> Can we have some commit message. Feel free to reuse the following:
>>
>> Earlier commit added a LLVM 4.0 workaround by passing
>> -simplifycfg-sink-common=false to LLVM.
>> When using multiple drivers, for example GL/dri and VAAPI, we may end
>> up with the option being parsed multiple times.
>> Hence we'll see errors like
>>
>>   "mesa: for the -simplifycfg-sink-common option: may only occur zero or one
>>    times!"
>>
>> Workaround this by exporting the driver entry point. This will lead to
>> the function being called once.
>>
>> Fixes: 7751ed39e40 ("radeonsi: disable sinking common instructions
>> down to the end block")
>>
>>>> ---
>>>>  src/gallium/targets/va/va.sym | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/src/gallium/targets/va/va.sym b/src/gallium/targets/va/va.sym
>>>> index c925b2e..b19bc36 100644
>>>> --- a/src/gallium/targets/va/va.sym
>>>> +++ b/src/gallium/targets/va/va.sym
>>>> @@ -1,6 +1,8 @@
>>>>  {
>>>>         global:
>>>>                 __vaDriverInit_*_*;
>>>> +               radeon_drm_winsys_create;
>>>> +               amdgpu_winsys_create;
>>
>> Please add a reference to the si_shader_tgsi_setup.c and vice-versa.
>> Otherwise we will end up removing one but not the other.
>
> It's actually not a workaround. It's a fix of va.sym that happens to
> fix that bug and it would be the correct thing to do even if there was
> no issue. Thus, I think a reference to si_shader_tgsi_setup is
> unnecessary. I'm not fixing the bug here. I'm just enforcing one
> screen+winsys per device per process like we already do between GL and
> VDPAU.
>
One of us is getting confused by how it works, hence the confusion how
it should be called.

On DRI2 we need to share the same $driver_winsys_create as need the
same GEM handles.
For DRI3 we don't need either of these, due to the handy code written
by Christian.

Since we have a GL extension for GL VDPAU interop, the DRI and VDPAU
drivers must expose the entry point, for as long as DRI2 is supported.
Other APIs, such as VAAPI, implicitly fall-back to the DRI3 codepath.

Or if we ignore everything I said, for a minute:

Does the workflow work correctly, before the si_shader_tgsi_setup
patch, even without this fix ?
I believe the answer is, yes it does. So even if workaround is the
wrong word, the two are interconnected and should be cross-referenced.
Pretty please ?

>>
>>>
>>>
>>> Oof, that's ugly, but at least it's only a change to va and the chance of
>>> collision is low. Have you verified that it fixes the bug?
>
> I've not verified that, no. If it doesn't fix the bug, we still need
> this patch because of aforementioned reasons, and then we have to fix
> the bug.
>
>>
>> Same question - does it work without the -Wl,--dynamic-list?
>
> What is that?
>
This is part of how everything works together.

The modules must expose the winsys symbol, and the very same must be
in the dynamic list.
The latter ensures that the first module which provide
$driver_winsys_create which will be used the any other modules,
instead of their own copy.

Thanks
Emil