[Mesa-dev] [PATCH 2/2] egl/dri2: don't require a context for ClientWaitSync

[Marek Olšák]

On Tue, Sep 29, 2015 at 4:54 PM, Albert Freeman
<albertwdfreeman at gmail.com> wrote:
> On 29 September 2015 at 07:28, Albert Freeman <albertwdfreeman at gmail.com> wrote:
>> On 29 September 2015 at 07:25, Albert Freeman <albertwdfreeman at gmail.com> wrote:
>>> On 28 September 2015 at 08:10, Marek Olšák <maraeo at gmail.com> wrote:
>>>> On Mon, Sep 28, 2015 at 7:20 AM, Albert Freeman
>>>> <albertwdfreeman at gmail.com> wrote:
>>>>> On 28 September 2015 at 14:38, Albert Freeman <albertwdfreeman at gmail.com> wrote:
>>>>>> On 28 September 2015 at 04:12, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>>>>>> On 26 September 2015 at 00:49, Marek Olšák <maraeo at gmail.com> wrote:
>>>>>>>> From: Marek Olšák <marek.olsak at amd.com>
>>>>>>>>
>>>>>>>> The spec doesn't require it. This fixes a crash on Android.
>>>>>>>>
>>>>>>> Nicely spotted Marek.
>>>>>>>
>>>>>>> A couple of related (not supposed to be part of this patch) notes:
>>>>>>>  - This will just move the crash (from libEGL to i965_dri.so) for i965
>>>>>>> hardware :)
>>>>>> I rediscovered this before I read the email related to this patch
>>>>>> (where Marek mentions it): "Re: [Mesa-dev] Pending issues of
>>>>>> lollipop-x86".
>>>>>> Who (if anyone) should we CC this to?
>>>>>>>  - We really shouldn't be setting the flags if ctx is NULL, as per the spec.
>>>>>>>
>>>>>>>     "If no context is
>>>>>>>     current for the bound API, the EGL_SYNC_FLUSH_COMMANDS_BIT_KHR bit
>>>>>>>     is ignored."
>>>>>> Maybe handle this in the driver (probably state_tracker in the case of
>>>>>> gallium) level. Both here and drivers?
>>>>> OpenGL ClientWaitSync is very clear about SYNC_FLUSH_COMMANDS_BIT only
>>>>> flushing the GL context when the fence sync is created (it doesn’t
>>>>> seem to require it at the moment wait sync is called (probably to
>>>>> leave room for drivers to optimise)). EGL is a lot less clear, it
>>>>> could be interpreted as behaving just like GL, but to someone writing
>>>>> an EGL program it could be easily interpreted as "the flush() must
>>>>> occur when eglClientWaitSync is called with SYNC_FLUSH_COMMANDS_BIT".
>>>>>
>>>>> The current behaviour of gallium (not sure about i965) is to always
>>>>> flush() when creating the sync object (hence always behave as
>>>>> SYNC_FLUSH_COMMANDS_BIT is set according to GL). However
>>>>> implementations are allowed to behave as if flush was not required (in
>>>>> GL and I am assuming EGL). So technically the current behaviour of the
>>>>> gallium dri state tracker could reduce performance (since flushing
>>>>> tends to do that). It however doesn’t actually violate the EGL spec.
>>>>> Perhaps some heuristics on when to flush would be a good solution?
>>>>
>>>> Flushing is currently required for creating fences. This is a gallium
>>>> design decision as well as radeon/amdgpu kernel driver decision. It's
>>>> simply not possible to put a fence anywhere in the GPU command buffer.
>>>> We can only submit a command buffer to the kernel and then get a fence
>>>> tracking execution of the whole command buffer.
>>>>
>>>> Marek
>>> Thanks for the clarification.
>> Also when I said "Can easily be reverted later if a better solution is
>> found. :)". It seemed to me that this was an issue better resolved in
>> the code outside of mesa that had the issue.
> So basically:
> Sorry if I upset anyone. The only thing that has changed is that I
> approve of the patch a lot more than I originally did (since my
> misunderstanding was cleared up). "The spec doesn't require it." in
> the original patch implies an issue in the application and not the
> driver. This lead me to believe that for some reason Marek had decided
> that an applications incorrect behavior should be fixed in the driver,
> hence my reply (I thought Marek was thinking approximately the same
> thing). I understood what the patch did and its implications within
> mesa, just not what it fixed (now I understand both).

No, this is not an application bug. It's really driver bug that was
incorrectly evaluated as an application bug.

Marek