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

Submitted by Marek Olšák on Sept. 25, 2015, 11:49 p.m.

Details

Message ID 1443224986-29402-2-git-send-email-maraeo@gmail.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Marek Olšák Sept. 25, 2015, 11:49 p.m.
From: Marek Olšák <marek.olsak@amd.com>

The spec doesn't require it. This fixes a crash on Android.

Cc: 10.6 11.0 <mesa-stable@lists.freedesktop.org>
---
 src/egl/drivers/dri2/egl_dri2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
index 1740ee3..7d8977e 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -2430,7 +2430,7 @@  dri2_client_wait_sync(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSync *sync,
    /* the sync object should take a reference while waiting */
    dri2_egl_ref_sync(dri2_sync);
 
-   if (dri2_dpy->fence->client_wait_sync(dri2_ctx->dri_context,
+   if (dri2_dpy->fence->client_wait_sync(dri2_ctx ? dri2_ctx->dri_context : NULL,
                                          dri2_sync->fence, wait_flags,
                                          timeout))
       dri2_sync->base.SyncStatus = EGL_SIGNALED_KHR;

Comments

On 25 September 2015 at 23:49, Marek Olšák <maraeo@gmail.com> wrote:
> From: Marek Olšák <marek.olsak@amd.com>
>
> The spec doesn't require it. This fixes a crash on Android.
>
> Cc: 10.6 11.0 <mesa-stable@lists.freedesktop.org>
> ---
>  src/egl/drivers/dri2/egl_dri2.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
> index 1740ee3..7d8977e 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -2430,7 +2430,7 @@ dri2_client_wait_sync(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSync *sync,
>     /* the sync object should take a reference while waiting */
>     dri2_egl_ref_sync(dri2_sync);
>
> -   if (dri2_dpy->fence->client_wait_sync(dri2_ctx->dri_context,
> +   if (dri2_dpy->fence->client_wait_sync(dri2_ctx ? dri2_ctx->dri_context : NULL,
>                                           dri2_sync->fence, wait_flags,
>                                           timeout))
>        dri2_sync->base.SyncStatus = EGL_SIGNALED_KHR;
> --
> 2.1.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Reviewed-by: Albert Freeman <albertwdfreeman@gmail.com>
Can easily be reverted later if a better solution is found. :)
On 26 September 2015 at 00:49, Marek Olšák <maraeo@gmail.com> wrote:
> From: Marek Olšák <marek.olsak@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 :)
 - 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."

-Emil
On 28 September 2015 at 04:12, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> On 26 September 2015 at 00:49, Marek Olšák <maraeo@gmail.com> wrote:
>> From: Marek Olšák <marek.olsak@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?
>
> -Emil
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
On 28 September 2015 at 14:38, Albert Freeman <albertwdfreeman@gmail.com> wrote:
> On 28 September 2015 at 04:12, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>> On 26 September 2015 at 00:49, Marek Olšák <maraeo@gmail.com> wrote:
>>> From: Marek Olšák <marek.olsak@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?
>>
>> -Emil
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
On 28 September 2015 at 15:20, Albert Freeman <albertwdfreeman@gmail.com> wrote:
> On 28 September 2015 at 14:38, Albert Freeman <albertwdfreeman@gmail.com> wrote:
>> On 28 September 2015 at 04:12, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>> On 26 September 2015 at 00:49, Marek Olšák <maraeo@gmail.com> wrote:
>>>> From: Marek Olšák <marek.olsak@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".
Instead of
"the flush() must occur when eglClientWaitSync is called with
SYNC_FLUSH_COMMANDS_BIT"
I meant
"the flush() must occur at the moment 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?
>>>
>>> -Emil
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Mon, Sep 28, 2015 at 7:20 AM, Albert Freeman
<albertwdfreeman@gmail.com> wrote:
> On 28 September 2015 at 14:38, Albert Freeman <albertwdfreeman@gmail.com> wrote:
>> On 28 September 2015 at 04:12, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>> On 26 September 2015 at 00:49, Marek Olšák <maraeo@gmail.com> wrote:
>>>> From: Marek Olšák <marek.olsak@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
On 28 September 2015 at 08:10, Marek Olšák <maraeo@gmail.com> wrote:
> On Mon, Sep 28, 2015 at 7:20 AM, Albert Freeman
> <albertwdfreeman@gmail.com> wrote:
>> On 28 September 2015 at 14:38, Albert Freeman <albertwdfreeman@gmail.com> wrote:
>>> On 28 September 2015 at 04:12, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>>> On 26 September 2015 at 00:49, Marek Olšák <maraeo@gmail.com> wrote:
>>>>> From: Marek Olšák <marek.olsak@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.
On 29 September 2015 at 07:25, Albert Freeman <albertwdfreeman@gmail.com> wrote:
> On 28 September 2015 at 08:10, Marek Olšák <maraeo@gmail.com> wrote:
>> On Mon, Sep 28, 2015 at 7:20 AM, Albert Freeman
>> <albertwdfreeman@gmail.com> wrote:
>>> On 28 September 2015 at 14:38, Albert Freeman <albertwdfreeman@gmail.com> wrote:
>>>> On 28 September 2015 at 04:12, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>>>> On 26 September 2015 at 00:49, Marek Olšák <maraeo@gmail.com> wrote:
>>>>>> From: Marek Olšák <marek.olsak@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.
On 29 September 2015 at 07:28, Albert Freeman <albertwdfreeman@gmail.com> wrote:
> On 29 September 2015 at 07:25, Albert Freeman <albertwdfreeman@gmail.com> wrote:
>> On 28 September 2015 at 08:10, Marek Olšák <maraeo@gmail.com> wrote:
>>> On Mon, Sep 28, 2015 at 7:20 AM, Albert Freeman
>>> <albertwdfreeman@gmail.com> wrote:
>>>> On 28 September 2015 at 14:38, Albert Freeman <albertwdfreeman@gmail.com> wrote:
>>>>> On 28 September 2015 at 04:12, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>>>>> On 26 September 2015 at 00:49, Marek Olšák <maraeo@gmail.com> wrote:
>>>>>>> From: Marek Olšák <marek.olsak@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).
On Tue, Sep 29, 2015 at 4:54 PM, Albert Freeman
<albertwdfreeman@gmail.com> wrote:
> On 29 September 2015 at 07:28, Albert Freeman <albertwdfreeman@gmail.com> wrote:
>> On 29 September 2015 at 07:25, Albert Freeman <albertwdfreeman@gmail.com> wrote:
>>> On 28 September 2015 at 08:10, Marek Olšák <maraeo@gmail.com> wrote:
>>>> On Mon, Sep 28, 2015 at 7:20 AM, Albert Freeman
>>>> <albertwdfreeman@gmail.com> wrote:
>>>>> On 28 September 2015 at 14:38, Albert Freeman <albertwdfreeman@gmail.com> wrote:
>>>>>> On 28 September 2015 at 04:12, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>>>>>> On 26 September 2015 at 00:49, Marek Olšák <maraeo@gmail.com> wrote:
>>>>>>>> From: Marek Olšák <marek.olsak@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
On 29 September 2015 at 15:01, Marek Olšák <maraeo@gmail.com> wrote:
> On Tue, Sep 29, 2015 at 4:54 PM, Albert Freeman
> <albertwdfreeman@gmail.com> wrote:
>> On 29 September 2015 at 07:28, Albert Freeman <albertwdfreeman@gmail.com> wrote:
>>> On 29 September 2015 at 07:25, Albert Freeman <albertwdfreeman@gmail.com> wrote:
>>>> On 28 September 2015 at 08:10, Marek Olšák <maraeo@gmail.com> wrote:
>>>>> On Mon, Sep 28, 2015 at 7:20 AM, Albert Freeman
>>>>> <albertwdfreeman@gmail.com> wrote:
>>>>>> On 28 September 2015 at 14:38, Albert Freeman <albertwdfreeman@gmail.com> wrote:
>>>>>>> On 28 September 2015 at 04:12, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>>>>>>> On 26 September 2015 at 00:49, Marek Olšák <maraeo@gmail.com> wrote:
>>>>>>>>> From: Marek Olšák <marek.olsak@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. :)