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

[Albert Freeman]

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).