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

[Albert Freeman]

On 29 September 2015 at 15:01, Marek Olšák <maraeo at gmail.com> wrote:
> 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
What I am saying is, I know that now, I didn't then. :)