ozone: Wait on EGLFence before committing buffers. Avoid using GL.

ozone: Wait on EGLFence before committing buffers. Avoid using GL.

GbmSurfaceless::SwapBuffersAsync used to always call glFlush, to call
glFinish for universal link display device, and to insert an EGL fence
and wait on it when "EGL_ARM_implicit_external_sync" or
"EGL_EXT_image_flush_external" was available.

There is no guarantee that the current GL context when
GbmSurfaceless::SwapBuffersAsync is called is the context that was
used to draw to the buffers we are about to commit.
Additionally a glFlush or glFinish call does not guarantee content
is written and flushed to buffers after those calls return.

This caused some flickering on Mimo (udl device) since we'd submit
for scanout buffers while GL was still drawing to them.

This CL removes all the GL calls in GbmSurfaceless::SwapBuffersAsync,
that might have happened on the wrong context. It changes the code so
that we always insert and wait on an EGL fence, even on devices where
we don't have implicit sync or flush external, making SwapBuffersAsync
behavior more consistent across platforms.

BUG=692508
Review-Url: https://codereview.chromium.org/2858693002
Cr-Commit-Position: refs/heads/master@{#468785}
Committed: https://chromium.googlesource.com/chromium/src/+/00ef929f434505f4d09a130e0bc1d4a4938d490f

[Daniele Castagna, 2017-05-02 19:01:06]

The CQ bit was checked by dcastagna@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 19:02:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Daniele Castagna, 2017-05-02 19:03:05]

Description was changed from

==========
ozone: Wait on EGLFence on swapbuffers. Avoid using GL.

GbmSurfaceless::SwapBuffersAsync used to always call glFlush, to call
glFinish for universal link display device, and to insert an egl fence
and wait for it when "EGL_ARM_implicit_external_sync" or
"EGL_EXT_image_flush_external" was available.

There is no guarantee that the current GL context when
GbmSurfaceless::SwapBuffersAsync is called is the context that was
used to draw to the buffers we are about to submit. Any glFlush or
glFinish call does not guarantee content was written to the buffers
after they return.

This caused some flickering on Mimo (udl device) since we'd submit
and scanout the buffer while GL was drawing to it.

This CL removes all the GL Calls in GbmSurfaceless::SwapBuffersAsync
and makes it so we always insert an EGL fence and wait for it before
committing the buffers, making the SwapBuffers behavior more consistent
across platforms.

BUG=692508
==========

to

==========
ozone: Wait on EGLFence before committing buffers. Avoid using GL.

GbmSurfaceless::SwapBuffersAsync used to always call glFlush, to call
glFinish for universal link display device, and to insert an EGL fence
and wait on it when "EGL_ARM_implicit_external_sync" or
"EGL_EXT_image_flush_external" was available.

There is no guarantee that the current GL context when
GbmSurfaceless::SwapBuffersAsync is called is the context that was
used to draw to the buffers we are about to submit. Any glFlush or
glFinish call does not guarantee content is written and flushed to
the buffers after those calls return.

This caused some flickering on Mimo (udl device) since we'd submit
and scanout buffers while GL was still drawing to it.

This CL removes all the GL calls in GbmSurfaceless::SwapBuffersAsync
and makes it so we always insert an EGL fence and wait for it before
committing the buffers, making the SwapBuffersAsync behavior more
consistent across platforms.

BUG=692508
==========

[Daniele Castagna, 2017-05-02 19:03:57]

dcastagna@chromium.org changed reviewers:
+ dnicoara@chromium.org, marcheu@chromium.org, reveman@chromium.org

[commit-bot: I haz the power, 2017-05-02 19:07:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-02 19:07:37]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Daniele Castagna, 2017-05-02 19:25:22]

The CQ bit was checked by dcastagna@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 19:25:57]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[reveman, 2017-05-02 19:31:51]

Non-owner lgtm. Aligns well with our plans to remove implicit sync in favor of
flush external and explicit sync fences.

Maybe make it clear in the description that this is now creating a fence on
devices where we don't have implicit sync or flush external instead of just
calling glFlush on a possibly incorrect context.

[dnicoara, 2017-05-02 19:39:11]

lgtm

[commit-bot: I haz the power, 2017-05-02 19:53:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-02 19:54:00]

Dry run: This issue passed the CQ dry run.

[marcheu, 2017-05-02 21:10:14]

https://codereview.chromium.org/2858693002/diff/20001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/gbm_surfaceless.cc (left):

https://codereview.chromium.org/2858693002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_surfaceless.cc:41:
HasEGLExtension("EGL_EXT_image_flush_external")),
hmm, note that we need this to happen somehow... But right now it is also on the
wrong context which I guess has no effect. Should the eglimageflush be moved
outside of ozone?

[Daniele Castagna, 2017-05-02 21:23:31]

https://codereview.chromium.org/2858693002/diff/20001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/gbm_surfaceless.cc (left):

https://codereview.chromium.org/2858693002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_surfaceless.cc:41:
HasEGLExtension("EGL_EXT_image_flush_external")),
On 2017/05/02 at 21:10:14, marcheu wrote:
> hmm, note that we need this to happen somehow... But right now it is also on
the wrong context which I guess has no effect. Should the eglimageflush be moved
outside of ozone?

This happens per image. Look at GLImageNativePixmap::Flush, it gets called from
unsubmitted_frames_.back()->Flush(); in GbmSurfaceless::SwapBuffersAsync just
before we insert and wait for the fence.
The boolean here was just to make sure we'd insert the fence if
eglImageFlushExternalEXT was used. Now we always insert the fence so we don't
need the check at this level.

eglImageFlushExternalEXT shouldn't use the GL context, so it should work.

[marcheu, 2017-05-02 21:29:12]

On 2017/05/02 21:23:31, Daniele Castagna wrote:
>
https://codereview.chromium.org/2858693002/diff/20001/ui/ozone/platform/drm/g...
> File ui/ozone/platform/drm/gpu/gbm_surfaceless.cc (left):
> 
>
https://codereview.chromium.org/2858693002/diff/20001/ui/ozone/platform/drm/g...
> ui/ozone/platform/drm/gpu/gbm_surfaceless.cc:41:
> HasEGLExtension("EGL_EXT_image_flush_external")),
> On 2017/05/02 at 21:10:14, marcheu wrote:
> > hmm, note that we need this to happen somehow... But right now it is also on
> the wrong context which I guess has no effect. Should the eglimageflush be
moved
> outside of ozone?
> 
> This happens per image. Look at GLImageNativePixmap::Flush, it gets called
from
> unsubmitted_frames_.back()->Flush(); in GbmSurfaceless::SwapBuffersAsync just
> before we insert and wait for the fence.
> The boolean here was just to make sure we'd insert the fence if
> eglImageFlushExternalEXT was used. Now we always insert the fence so we don't
> need the check at this level.
> 
> eglImageFlushExternalEXT shouldn't use the GL context, so it should work.

We discussed the source of my confusion offline, lgtm

[Daniele Castagna, 2017-05-02 21:49:11]

Description was changed from

==========
ozone: Wait on EGLFence before committing buffers. Avoid using GL.

GbmSurfaceless::SwapBuffersAsync used to always call glFlush, to call
glFinish for universal link display device, and to insert an EGL fence
and wait on it when "EGL_ARM_implicit_external_sync" or
"EGL_EXT_image_flush_external" was available.

There is no guarantee that the current GL context when
GbmSurfaceless::SwapBuffersAsync is called is the context that was
used to draw to the buffers we are about to submit. Any glFlush or
glFinish call does not guarantee content is written and flushed to
the buffers after those calls return.

This caused some flickering on Mimo (udl device) since we'd submit
and scanout buffers while GL was still drawing to it.

This CL removes all the GL calls in GbmSurfaceless::SwapBuffersAsync
and makes it so we always insert an EGL fence and wait for it before
committing the buffers, making the SwapBuffersAsync behavior more
consistent across platforms.

BUG=692508
==========

to

==========
ozone: Wait on EGLFence before committing buffers. Avoid using GL.

GbmSurfaceless::SwapBuffersAsync used to always call glFlush, to call
glFinish for universal link display device, and to insert an EGL fence
and wait on it when "EGL_ARM_implicit_external_sync" or
"EGL_EXT_image_flush_external" was available.

There is no guarantee that the current GL context when
GbmSurfaceless::SwapBuffersAsync is called is the context that was
used to draw to the buffers we are about to commit.
Additionally a glFlush or glFinish call does not guarantee content
is written and flushed to buffers after those calls return.

This caused some flickering on Mimo (udl device) since we'd submit
for scanout buffers while GL was still drawing to them.

This CL removes all the GL calls in GbmSurfaceless::SwapBuffersAsync,
that might have happened on the wrong context. It changes the code so
that we always insert and wait on an EGL fence, even on devices where
we don't have implicit sync or flush external, making SwapBuffersAsync
behavior more consistent across platforms.

BUG=692508
==========

[Daniele Castagna, 2017-05-02 21:49:26]

On 2017/05/02 at 19:31:51, reveman wrote:
> Non-owner lgtm. Aligns well with our plans to remove implicit sync in favor of
flush external and explicit sync fences.
> 
> Maybe make it clear in the description that this is now creating a fence on
devices where we don't have implicit sync or flush external instead of just
calling glFlush on a possibly incorrect context.

Done.

[Daniele Castagna, 2017-05-02 21:49:30]

The CQ bit was checked by dcastagna@chromium.org

[commit-bot: I haz the power, 2017-05-02 21:50:07]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-02 21:56:26]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493761770247440,
"parent_rev": "6d2b8a0a27a195902caaac61c11f26b10e02b5a3", "commit_rev":
"00ef929f434505f4d09a130e0bc1d4a4938d490f"}

[commit-bot: I haz the power, 2017-05-02 21:56:39]

Description was changed from

==========
ozone: Wait on EGLFence before committing buffers. Avoid using GL.

GbmSurfaceless::SwapBuffersAsync used to always call glFlush, to call
glFinish for universal link display device, and to insert an EGL fence
and wait on it when "EGL_ARM_implicit_external_sync" or
"EGL_EXT_image_flush_external" was available.

There is no guarantee that the current GL context when
GbmSurfaceless::SwapBuffersAsync is called is the context that was
used to draw to the buffers we are about to commit.
Additionally a glFlush or glFinish call does not guarantee content
is written and flushed to buffers after those calls return.

This caused some flickering on Mimo (udl device) since we'd submit
for scanout buffers while GL was still drawing to them.

This CL removes all the GL calls in GbmSurfaceless::SwapBuffersAsync,
that might have happened on the wrong context. It changes the code so
that we always insert and wait on an EGL fence, even on devices where
we don't have implicit sync or flush external, making SwapBuffersAsync
behavior more consistent across platforms.

BUG=692508
==========

to

==========
ozone: Wait on EGLFence before committing buffers. Avoid using GL.

GbmSurfaceless::SwapBuffersAsync used to always call glFlush, to call
glFinish for universal link display device, and to insert an EGL fence
and wait on it when "EGL_ARM_implicit_external_sync" or
"EGL_EXT_image_flush_external" was available.

There is no guarantee that the current GL context when
GbmSurfaceless::SwapBuffersAsync is called is the context that was
used to draw to the buffers we are about to commit.
Additionally a glFlush or glFinish call does not guarantee content
is written and flushed to buffers after those calls return.

This caused some flickering on Mimo (udl device) since we'd submit
for scanout buffers while GL was still drawing to them.

This CL removes all the GL calls in GbmSurfaceless::SwapBuffersAsync,
that might have happened on the wrong context. It changes the code so
that we always insert and wait on an EGL fence, even on devices where
we don't have implicit sync or flush external, making SwapBuffersAsync
behavior more consistent across platforms.

BUG=692508

Review-Url: https://codereview.chromium.org/2858693002
Cr-Commit-Position: refs/heads/master@{#468785}
Committed:
https://chromium.googlesource.com/chromium/src/+/00ef929f434505f4d09a130e0bc1...
==========

[commit-bot: I haz the power, 2017-05-02 21:56:41]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/00ef929f434505f4d09a130e0bc1...

[kcwu, 2017-05-06 09:02:47]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/2866903002/ by kcwu@chromium.org.

The reason for reverting is: This makes video_VideoDecodeAccelerator failed on
all ChromeOS boards.

BUG=719145.

[James Cook, 2017-05-10 00:58:05]

On 2017/05/06 09:02:47, kcwu wrote:
> A revert of this CL (patchset #2 id:20001) has been created in
> https://codereview.chromium.org/2866903002/ by mailto:kcwu@chromium.org.
> 
> The reason for reverting is: This makes video_VideoDecodeAccelerator failed on
> all ChromeOS boards.
> 
> BUG=719145.

This is also causing crbug.com/719983 Black screen in VM, Context lost because
SwapBuffers failed

Note that the revert above did not go through.

[xiangze.zhang, 2017-06-01 02:09:57]

On 2017/05/10 00:58:05, James Cook wrote:
> On 2017/05/06 09:02:47, kcwu wrote:
> > A revert of this CL (patchset #2 id:20001) has been created in
> > https://codereview.chromium.org/2866903002/ by mailto:kcwu@chromium.org.
> > 
> > The reason for reverting is: This makes video_VideoDecodeAccelerator failed
on
> > all ChromeOS boards.
> > 
> > BUG=719145.
> 
> This is also causing crbug.com/719983 Black screen in VM, Context lost because
> SwapBuffers failed
> 
> Note that the revert above did not go through.

Is this patch going to be reverted or stay?
We found it is also causing performance regression of a WebGL workload
(http://webglsamples.org/aquarium/aquarium.html) on Intel Chromebooks.

[reveman, 2017-06-01 14:16:17]

On 2017/06/01 at 02:09:57, xiangze.zhang wrote:
> On 2017/05/10 00:58:05, James Cook wrote:
> > On 2017/05/06 09:02:47, kcwu wrote:
> > > A revert of this CL (patchset #2 id:20001) has been created in
> > > https://codereview.chromium.org/2866903002/ by mailto:kcwu@chromium.org.
> > > 
> > > The reason for reverting is: This makes video_VideoDecodeAccelerator
failed on
> > > all ChromeOS boards.
> > > 
> > > BUG=719145.
> > 
> > This is also causing crbug.com/719983 Black screen in VM, Context lost
because
> > SwapBuffers failed
> > 
> > Note that the revert above did not go through.
> 
> Is this patch going to be reverted or stay?
> We found it is also causing performance regression of a WebGL workload
(http://webglsamples.org/aquarium/aquarium.html) on Intel Chromebooks.

I think this should stay as we need it for correctness. Any performance
regression from it should recover when we switch to explicit sync. How
significant is the performance regression?

[Daniele Castagna, 2017-06-03 21:02:28]

On 2017/06/01 at 14:16:17, reveman wrote:
> On 2017/06/01 at 02:09:57, xiangze.zhang wrote:
> > On 2017/05/10 00:58:05, James Cook wrote:
> > > On 2017/05/06 09:02:47, kcwu wrote:
> > > > A revert of this CL (patchset #2 id:20001) has been created in
> > > > https://codereview.chromium.org/2866903002/ by mailto:kcwu@chromium.org.
> > > > 
> > > > The reason for reverting is: This makes video_VideoDecodeAccelerator
failed on
> > > > all ChromeOS boards.
> > > > 
> > > > BUG=719145.
> > > 
> > > This is also causing crbug.com/719983 Black screen in VM, Context lost
because
> > > SwapBuffers failed
> > > 
> > > Note that the revert above did not go through.
> > 
> > Is this patch going to be reverted or stay?
> > We found it is also causing performance regression of a WebGL workload
(http://webglsamples.org/aquarium/aquarium.html) on Intel Chromebooks.
> 
> I think this should stay as we need it for correctness. Any performance
regression from it should recover when we switch to explicit sync. How
significant is the performance regression?

I think Xiangze is referring to what we're tracking in crbug.com/721463
Xiangze, I couldn't reproduce it on a local Chromebook, can you add more
information to crbug.com/721463 how you reproduced it and which Chromebook you
used?