Move GPU fences into sk_gpu_test

Move GPU fences into sk_gpu_test

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2383383002

Committed: https://skia.googlesource.com/skia/+/421a3c1cc1b227084c7c84618d0b6a6804faabef

[csmartdalton, 2016-10-01 22:46:50]

Description was changed from

==========
Move GPU fences into sk_gpu_test

BUG=skia:
==========

to

==========
Move GPU fences into sk_gpu_test

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2383383002
==========

[csmartdalton, 2016-10-03 03:43:10]

csmartdalton@google.com changed reviewers:
+ bsalomon@google.com, egdaniel@google.com

[egdaniel, 2016-10-03 13:24:25]

So this is kind of outside the original purpose of this CL, but my high level
thought for this in terms of Vulkan (and I suppose GL) is that Jim basically
implemented an almost identical set of Fence functions on GrGpu recently
(https://codereview.chromium.org/2384463003/). I wonder if somehow the logic for
this two uses cases can be merged so we don't have duplicated code. Add Jim to
see if he has any ideas how best to merge these things.

Until we do have a good idea on how to merge, I've added a few comments to
cleanup the Vulkan code.

https://codereview.chromium.org/2383383002/diff/100001/tools/gpu/vk/VkTestCon...
File tools/gpu/vk/VkTestContext.cpp (right):

https://codereview.chromium.org/2383383002/diff/100001/tools/gpu/vk/VkTestCon...
tools/gpu/vk/VkTestContext.cpp:30: VkCommandPoolCreateInfo createInfo;
We don't need any of this command buffer stuff just add a fence

https://codereview.chromium.org/2383383002/diff/100001/tools/gpu/vk/VkTestCon...
tools/gpu/vk/VkTestContext.cpp:78: GR_VK_CALL_ERRCHECK(fVk, QueueSubmit(fQueue,
1, &submitInfo, fence));
since we don't need a command buffer, we can pass in nullptr for submitInfo here

https://codereview.chromium.org/2383383002/diff/100001/tools/gpu/vk/VkTestCon...
tools/gpu/vk/VkTestContext.cpp:87: return result != VK_TIMEOUT;
Should this assert be result == VK_SUCCESS, this would also catch errors besides
VK_TIMEOUT

[egdaniel, 2016-10-03 13:26:43]

egdaniel@google.com changed reviewers:
+ jvanverth@google.com

[egdaniel, 2016-10-03 13:26:44]

actually adding jim

[bsalomon, 2016-10-03 13:48:25]

On 2016/10/03 13:24:25, egdaniel wrote:
> So this is kind of outside the original purpose of this CL, but my high level
> thought for this in terms of Vulkan (and I suppose GL) is that Jim basically
> implemented an almost identical set of Fence functions on GrGpu recently
> (https://codereview.chromium.org/2384463003/). I wonder if somehow the logic
for
> this two uses cases can be merged so we don't have duplicated code. Add Jim to
> see if he has any ideas how best to merge these things.
> 

I actually think it would be good to have separate fence management for tool in
that it allows the tools to use fences without digging into the internals of
GrGpu and doesn't couple GrGpu's fence usage and interface with the benchmarking
tools.

> Until we do have a good idea on how to merge, I've added a few comments to
> cleanup the Vulkan code.
> 
>
https://codereview.chromium.org/2383383002/diff/100001/tools/gpu/vk/VkTestCon...
> File tools/gpu/vk/VkTestContext.cpp (right):
> 
>
https://codereview.chromium.org/2383383002/diff/100001/tools/gpu/vk/VkTestCon...
> tools/gpu/vk/VkTestContext.cpp:30: VkCommandPoolCreateInfo createInfo;
> We don't need any of this command buffer stuff just add a fence
> 
>
https://codereview.chromium.org/2383383002/diff/100001/tools/gpu/vk/VkTestCon...
> tools/gpu/vk/VkTestContext.cpp:78: GR_VK_CALL_ERRCHECK(fVk,
QueueSubmit(fQueue,
> 1, &submitInfo, fence));
> since we don't need a command buffer, we can pass in nullptr for submitInfo
here
> 
>
https://codereview.chromium.org/2383383002/diff/100001/tools/gpu/vk/VkTestCon...
> tools/gpu/vk/VkTestContext.cpp:87: return result != VK_TIMEOUT;
> Should this assert be result == VK_SUCCESS, this would also catch errors
besides
> VK_TIMEOUT

[csmartdalton, 2016-10-03 14:06:18]

On 2016/10/03 13:24:25, egdaniel wrote:
> So this is kind of outside the original purpose of this CL, but my high level
> thought for this in terms of Vulkan (and I suppose GL) is that Jim basically
> implemented an almost identical set of Fence functions on GrGpu recently
> (https://codereview.chromium.org/2384463003/). I wonder if somehow the logic
for
> this two uses cases can be merged so we don't have duplicated code. Add Jim to
> see if he has any ideas how best to merge these things.
> 
> Until we do have a good idea on how to merge, I've added a few comments to
> cleanup the Vulkan code.
> 
>
https://codereview.chromium.org/2383383002/diff/100001/tools/gpu/vk/VkTestCon...
> File tools/gpu/vk/VkTestContext.cpp (right):
> 
>
https://codereview.chromium.org/2383383002/diff/100001/tools/gpu/vk/VkTestCon...
> tools/gpu/vk/VkTestContext.cpp:30: VkCommandPoolCreateInfo createInfo;
> We don't need any of this command buffer stuff just add a fence
> 
>
https://codereview.chromium.org/2383383002/diff/100001/tools/gpu/vk/VkTestCon...
> tools/gpu/vk/VkTestContext.cpp:78: GR_VK_CALL_ERRCHECK(fVk,
QueueSubmit(fQueue,
> 1, &submitInfo, fence));
> since we don't need a command buffer, we can pass in nullptr for submitInfo
here
> 
>
https://codereview.chromium.org/2383383002/diff/100001/tools/gpu/vk/VkTestCon...
> tools/gpu/vk/VkTestContext.cpp:87: return result != VK_TIMEOUT;
> Should this assert be result == VK_SUCCESS, this would also catch errors
besides
> VK_TIMEOUT

Would you mind these changes being done in a separate CL? This CL only touches
this file because a few things were affected by renamings.

[egdaniel, 2016-10-03 14:12:10]

On 2016/10/03 14:06:18, csmartdalton wrote:
> On 2016/10/03 13:24:25, egdaniel wrote:
> > So this is kind of outside the original purpose of this CL, but my high
level
> > thought for this in terms of Vulkan (and I suppose GL) is that Jim basically
> > implemented an almost identical set of Fence functions on GrGpu recently
> > (https://codereview.chromium.org/2384463003/). I wonder if somehow the logic
> for
> > this two uses cases can be merged so we don't have duplicated code. Add Jim
to
> > see if he has any ideas how best to merge these things.
> > 
> > Until we do have a good idea on how to merge, I've added a few comments to
> > cleanup the Vulkan code.
> > 
> >
>
https://codereview.chromium.org/2383383002/diff/100001/tools/gpu/vk/VkTestCon...
> > File tools/gpu/vk/VkTestContext.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2383383002/diff/100001/tools/gpu/vk/VkTestCon...
> > tools/gpu/vk/VkTestContext.cpp:30: VkCommandPoolCreateInfo createInfo;
> > We don't need any of this command buffer stuff just add a fence
> > 
> >
>
https://codereview.chromium.org/2383383002/diff/100001/tools/gpu/vk/VkTestCon...
> > tools/gpu/vk/VkTestContext.cpp:78: GR_VK_CALL_ERRCHECK(fVk,
> QueueSubmit(fQueue,
> > 1, &submitInfo, fence));
> > since we don't need a command buffer, we can pass in nullptr for submitInfo
> here
> > 
> >
>
https://codereview.chromium.org/2383383002/diff/100001/tools/gpu/vk/VkTestCon...
> > tools/gpu/vk/VkTestContext.cpp:87: return result != VK_TIMEOUT;
> > Should this assert be result == VK_SUCCESS, this would also catch errors
> besides
> > VK_TIMEOUT
> 
> Would you mind these changes being done in a separate CL? This CL only touches
> this file because a few things were affected by renamings.

Yeah the Vulkan changes don't have to be in this CL. Just pointing out cleanups
that can happen in the code in general

[bsalomon, 2016-10-03 17:54:00]

https://codereview.chromium.org/2383383002/diff/100001/tools/gpu/gl/GLFenceSy...
File tools/gpu/gl/GLFenceSync.h (right):

https://codereview.chromium.org/2383383002/diff/100001/tools/gpu/gl/GLFenceSy...
tools/gpu/gl/GLFenceSync.h:18: class GLFenceSync : public FenceSync {
Can the existence of this class be an implementation detail of the GL context
code?

https://codereview.chromium.org/2383383002/diff/100001/tools/gpu/gl/egl/Creat...
File tools/gpu/gl/egl/CreatePlatformGLTestContext_egl.cpp (right):

https://codereview.chromium.org/2383383002/diff/100001/tools/gpu/gl/egl/Creat...
tools/gpu/gl/egl/CreatePlatformGLTestContext_egl.cpp:20: namespace sk_gpu_test {
Can't this still be in an anonymous namespace?

https://codereview.chromium.org/2383383002/diff/100001/tools/gpu/vk/VkTestCon...
File tools/gpu/vk/VkTestContext.cpp (right):

https://codereview.chromium.org/2383383002/diff/100001/tools/gpu/vk/VkTestCon...
tools/gpu/vk/VkTestContext.cpp:16: namespace sk_gpu_test {
Ditto

[csmartdalton, 2016-10-03 19:05:41]

https://codereview.chromium.org/2383383002/diff/100001/tools/gpu/gl/GLFenceSy...
File tools/gpu/gl/GLFenceSync.h (right):

https://codereview.chromium.org/2383383002/diff/100001/tools/gpu/gl/GLFenceSy...
tools/gpu/gl/GLFenceSync.h:18: class GLFenceSync : public FenceSync {
On 2016/10/03 17:54:00, bsalomon wrote:
> Can the existence of this class be an implementation detail of the GL context
> code?

Done.

https://codereview.chromium.org/2383383002/diff/100001/tools/gpu/gl/egl/Creat...
File tools/gpu/gl/egl/CreatePlatformGLTestContext_egl.cpp (right):

https://codereview.chromium.org/2383383002/diff/100001/tools/gpu/gl/egl/Creat...
tools/gpu/gl/egl/CreatePlatformGLTestContext_egl.cpp:20: namespace sk_gpu_test {
On 2016/10/03 17:54:00, bsalomon wrote:
> Can't this still be in an anonymous namespace?

Done.

https://codereview.chromium.org/2383383002/diff/100001/tools/gpu/vk/VkTestCon...
File tools/gpu/vk/VkTestContext.cpp (right):

https://codereview.chromium.org/2383383002/diff/100001/tools/gpu/vk/VkTestCon...
tools/gpu/vk/VkTestContext.cpp:16: namespace sk_gpu_test {
On 2016/10/03 17:54:00, bsalomon wrote:
> Ditto

Done.

[bsalomon, 2016-10-03 19:16:20]

https://codereview.chromium.org/2383383002/diff/120001/tools/gpu/gl/egl/Creat...
File tools/gpu/gl/egl/CreatePlatformGLTestContext_egl.cpp (right):

https://codereview.chromium.org/2383383002/diff/120001/tools/gpu/gl/egl/Creat...
tools/gpu/gl/egl/CreatePlatformGLTestContext_egl.cpp:22: namespace {
why nested? AFAIK we only ever use top level anonymous namespaces.

[csmartdalton, 2016-10-03 19:21:48]

On 2016/10/03 19:16:20, bsalomon wrote:
>
https://codereview.chromium.org/2383383002/diff/120001/tools/gpu/gl/egl/Creat...
> File tools/gpu/gl/egl/CreatePlatformGLTestContext_egl.cpp (right):
> 
>
https://codereview.chromium.org/2383383002/diff/120001/tools/gpu/gl/egl/Creat...
> tools/gpu/gl/egl/CreatePlatformGLTestContext_egl.cpp:22: namespace {
> why nested? AFAIK we only ever use top level anonymous namespaces.

It's nested because the interface this class implements now resides in the
sk_gpu_tools namespace.

On principle it feels right for the implementation to also reside in the same
namespace. And I also don't want to litter the code with 'sk_gpu_tools::' or
using declarations.

[bsalomon, 2016-10-03 19:28:59]

On 2016/10/03 19:21:48, csmartdalton wrote:
> On 2016/10/03 19:16:20, bsalomon wrote:
> >
>
https://codereview.chromium.org/2383383002/diff/120001/tools/gpu/gl/egl/Creat...
> > File tools/gpu/gl/egl/CreatePlatformGLTestContext_egl.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2383383002/diff/120001/tools/gpu/gl/egl/Creat...
> > tools/gpu/gl/egl/CreatePlatformGLTestContext_egl.cpp:22: namespace {
> > why nested? AFAIK we only ever use top level anonymous namespaces.
> 
> It's nested because the interface this class implements now resides in the
> sk_gpu_tools namespace.
> 
> On principle it feels right for the implementation to also reside in the same
> namespace. And I also don't want to litter the code with 'sk_gpu_tools::' or
> using declarations.

I tend to prefer that if the class is local to the cpp file that it is in a top
level anonymous namespace. I don't feel very strongly about this. If we're going
to adopt the standard that if a local class is implementing an interface that is
part of a namespace A then it goes in anonymous namespace inside A then I'd
rather convert the rest of the sk_gpu_test code to follow this standard rather
than just the classes modified in this change. IOW, I care more about
consistency than which style we choose.

[csmartdalton, 2016-10-03 22:13:36]

I think I'm at my quota for time spent worrying about things that don't matter.

Taking the path of least resistance.

https://codereview.chromium.org/2383383002/diff/190001/tools/gpu/gl/GLTestCon...
File tools/gpu/gl/GLTestContext.h (right):

https://codereview.chromium.org/2383383002/diff/190001/tools/gpu/gl/GLTestCon...
tools/gpu/gl/GLTestContext.h:67: const std::string& ext = "") const {
Should we still be using SkString in internal tools?

[bsalomon, 2016-10-04 13:40:45]

https://codereview.chromium.org/2383383002/diff/190001/tools/gpu/gl/GLTestCon...
File tools/gpu/gl/GLTestContext.h (right):

https://codereview.chromium.org/2383383002/diff/190001/tools/gpu/gl/GLTestCon...
tools/gpu/gl/GLTestContext.h:67: const std::string& ext = "") const {
On 2016/10/03 22:13:35, csmartdalton wrote:
> Should we still be using SkString in internal tools?

I think so. Not that SkString is necessarily any better, but consistency. Maybe
we should consider a widespread conversion to std::string.

[csmartdalton, 2016-10-04 15:51:00]

https://codereview.chromium.org/2383383002/diff/190001/tools/gpu/gl/GLTestCon...
File tools/gpu/gl/GLTestContext.h (right):

https://codereview.chromium.org/2383383002/diff/190001/tools/gpu/gl/GLTestCon...
tools/gpu/gl/GLTestContext.h:67: const std::string& ext = "") const {
On 2016/10/04 13:40:45, bsalomon wrote:
> On 2016/10/03 22:13:35, csmartdalton wrote:
> > Should we still be using SkString in internal tools?
> 
> I think so. Not that SkString is necessarily any better, but consistency.
Maybe
> we should consider a widespread conversion to std::string.

Done.

[bsalomon, 2016-10-04 16:00:26]

lgtm

https://codereview.chromium.org/2383383002/diff/230001/tools/gpu/vk/VkTestCon...
File tools/gpu/vk/VkTestContext.cpp (right):

https://codereview.chromium.org/2383383002/diff/230001/tools/gpu/vk/VkTestCon...
tools/gpu/vk/VkTestContext.cpp:144: }
keep comment?

[csmartdalton, 2016-10-04 16:09:19]

https://codereview.chromium.org/2383383002/diff/230001/tools/gpu/vk/VkTestCon...
File tools/gpu/vk/VkTestContext.cpp (right):

https://codereview.chromium.org/2383383002/diff/230001/tools/gpu/vk/VkTestCon...
tools/gpu/vk/VkTestContext.cpp:144: }
On 2016/10/04 16:00:25, bsalomon wrote:
> keep comment?

Done.

[csmartdalton, 2016-10-04 16:09:24]

The CQ bit was checked by csmartdalton@google.com

[csmartdalton, 2016-10-04 16:09:24]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/2383383002/#ps250001
(title: "Move GPU fences into sk_gpu_test")

[commit-bot: I haz the power, 2016-10-04 16:09:40]

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, 2016-10-04 16:11:24]

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

[commit-bot: I haz the power, 2016-10-04 16:11:25]

Try jobs failed on following builders:
  Build-Ubuntu-GCC-x86_64-Release-GN-Trybot on master.client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)

[csmartdalton, 2016-10-04 16:39:21]

The CQ bit was checked by csmartdalton@google.com

[csmartdalton, 2016-10-04 16:39:21]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/2383383002/#ps290001
(title: "dead code")

[commit-bot: I haz the power, 2016-10-04 16:39:26]

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, 2016-10-04 16:48:36]

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

[commit-bot: I haz the power, 2016-10-04 16:48:37]

Try jobs failed on following builders:
  Build-Win-MSVC-x86-Debug-Trybot on master.client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)

[csmartdalton, 2016-10-04 17:51:38]

The CQ bit was checked by csmartdalton@google.com

[csmartdalton, 2016-10-04 17:51:38]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/2383383002/#ps310001
(title: "fix for windowsx")

[commit-bot: I haz the power, 2016-10-04 17:51:47]

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, 2016-10-04 18:08:47]

Description was changed from

==========
Move GPU fences into sk_gpu_test

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2383383002
==========

to

==========
Move GPU fences into sk_gpu_test

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2383383002

Committed:
https://skia.googlesource.com/skia/+/421a3c1cc1b227084c7c84618d0b6a6804faabef
==========

[commit-bot: I haz the power, 2016-10-04 18:08:48]

Committed patchset #17 (id:310001) as
https://skia.googlesource.com/skia/+/421a3c1cc1b227084c7c84618d0b6a6804faabef

[djsollen, 2016-10-04 19:37:26]

A revert of this CL (patchset #17 id:310001) has been created in
https://codereview.chromium.org/2395573002/ by djsollen@google.com.

The reason for reverting is: This is breaking the android platform build with
the following errors...

external/skia/dm/../tools/gpu/vk/VkTestContext.cpp:80:16: error:
reinterpret_cast from 'VkFence' (aka 'unsigned long long') to
'sk_gpu_test::PlatformFence' (aka 'int') is not allowed
        return reinterpret_cast<sk_gpu_test::PlatformFence>(fence);
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
external/skia/dm/../tools/gpu/vk/VkTestContext.cpp:84:25: error:
reinterpret_cast from 'sk_gpu_test::PlatformFence' (aka 'int') to 'VkFence' (aka
'unsigned long long') is not allowed
        VkFence fence = reinterpret_cast<VkFence>(opaqueFence);
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
external/skia/dm/../tools/gpu/vk/VkTestContext.cpp:91:25: error:
reinterpret_cast from 'sk_gpu_test::PlatformFence' (aka 'int') to 'VkFence' (aka
'unsigned long long') is not allowed
        VkFence fence = reinterpret_cast<VkFence>(opaqueFence);
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
3 errors generated..