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

[Emil Velikov]

On 24 March 2017 at 15:26, Marek Olšák <maraeo at gmail.com> wrote:
> On Fri, Mar 24, 2017 at 4:09 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> 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 ?
>
> Yes, I understand your point of view. BTW, the message is only an LLVM
> warning. AFAIK, everything works both with and without the fix.
>
Your earlier message said "error", so I get confused there.

If everything works without the patch then it sounds like a
workaround. But let's ignore the name and call it "X".

> My question is whether we should establish this one-screen-per-process
> policy for all libs. It's not only VAAPI that's affected. Any lib that
> contains gallium but doesn't export the symbols is affected.
>
> These files export the symbols (including this patch):
> ./src/gallium/targets/vdpau/vdpau.sym
> ./src/gallium/targets/dri/dri.sym
> ./src/gallium/targets/dri-vdpau.dyn
These are _absolutely_ necessary for GL VDPAU to work on DRI2.

> ./src/gallium/targets/va/va.sym
>
> These files should export the symbols too, because the APIs are
> commonly used alongside other Mesa libs:
> ./src/gallium/targets/omx/omx.sym
> ./src/gallium/targets/opencl/opencl.sym
>
Above you want to swap opencl.sym with pipe.sym. The former is merely
st/clover + gallium/aux while the latter has the driver specifics.

If you want to update those, please do. All I'm kindly asking for is a
3-4 word comment in either place.

Thanks
Emil