Implement vulkan fence syncs for nanobench

Implement vulkan fence syncs for nanobench

GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1974913003

Committed: https://skia.googlesource.com/skia/+/edea94c356dddd339d776e1f0d906f44427222b9

[bsalomon, 2016-05-12 20:34:08]

Description was changed from

==========
Start of adding vulkan fence sync to nanobench
==========

to

==========
Start of adding vulkan fence sync to nanobench
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[bsalomon, 2016-05-13 15:25:42]

Description was changed from

==========
Start of adding vulkan fence sync to nanobench
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Implement vulkan fence syncs for nanobench

GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[bsalomon, 2016-05-13 15:25:42]

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

[bsalomon, 2016-05-13 20:29:22]

Forgot to send this out this morning

[jvanverth1, 2016-05-13 20:49:25]

lgtm

The only weird situation I was trying to think of was if we tried to delete the
VkDevice in GrVkGpu before deleting the VkCommandPool and VkCommandBuffer here,
but I couldn't come with a case where that would happen.

[bsalomon, 2016-05-13 20:57:37]

On 2016/05/13 20:49:25, jvanverth1 wrote:
> lgtm
> 
> The only weird situation I was trying to think of was if we tried to delete
the
> VkDevice in GrVkGpu before deleting the VkCommandPool and VkCommandBuffer
here,
> but I couldn't come with a case where that would happen.

GrVkBackendContext is ref counted and VkTestContext is an owner of it (and also
an owner of the VkFenceSync which owns the pool/cmd buffer). So I think it is
ok.

[bsalomon, 2016-05-13 20:57:42]

The CQ bit was checked by bsalomon@google.com

[commit-bot: I haz the power, 2016-05-13 20:57:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1974913003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1974913003/60001

[commit-bot: I haz the power, 2016-05-13 21:01:16]

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

[commit-bot: I haz the power, 2016-05-13 21:01:18]

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

[egdaniel, 2016-05-16 17:19:42]

https://codereview.chromium.org/1974913003/diff/60001/tools/gpu/vk/VkTestCont...
File tools/gpu/vk/VkTestContext.cpp (right):

https://codereview.chromium.org/1974913003/diff/60001/tools/gpu/vk/VkTestCont...
tools/gpu/vk/VkTestContext.cpp:11: #include <vulkan/vulkan.h>
would need to include third_party in gyp includes?

https://codereview.chromium.org/1974913003/diff/60001/tools/gpu/vk/VkTestCont...
tools/gpu/vk/VkTestContext.cpp:55: // If the above assertion is true then the
command buffer should not be in flight.
For the above assertion and comment to be true it requires that we called
deleteFence only after first calling waitFence on it. Is this always true?
Should deleteFence have some debug assert that the fence has been signaled?

https://codereview.chromium.org/1974913003/diff/60001/tools/gpu/vk/VkTestCont...
tools/gpu/vk/VkTestContext.cpp:79: return
reinterpret_cast<SkPlatformGpuFence>(fence);
nit: remove tab

https://codereview.chromium.org/1974913003/diff/60001/tools/gpu/vk/VkTestCont...
tools/gpu/vk/VkTestContext.cpp:127: INHERITED::teardown(); fVk.reset(nullptr);
put these on two lines?

[bsalomon, 2016-05-16 20:37:08]

https://codereview.chromium.org/1974913003/diff/60001/tools/gpu/vk/VkTestCont...
File tools/gpu/vk/VkTestContext.cpp (right):

https://codereview.chromium.org/1974913003/diff/60001/tools/gpu/vk/VkTestCont...
tools/gpu/vk/VkTestContext.cpp:11: #include <vulkan/vulkan.h>
On 2016/05/16 17:19:41, egdaniel wrote:
> would need to include third_party in gyp includes?

I'm not sure I understand this comment. I moved all but the first include inside
the below #ifdef. Does that address your concern?

https://codereview.chromium.org/1974913003/diff/60001/tools/gpu/vk/VkTestCont...
tools/gpu/vk/VkTestContext.cpp:55: // If the above assertion is true then the
command buffer should not be in flight.
On 2016/05/16 17:19:41, egdaniel wrote:
> For the above assertion and comment to be true it requires that we called
> deleteFence only after first calling waitFence on it. Is this always true?
> Should deleteFence have some debug assert that the fence has been signaled?

Right, the assertion only checks that deleteFence was called.

I'll follow this up with a CL that combines waitFence and deleteFence into a
single call.

https://codereview.chromium.org/1974913003/diff/60001/tools/gpu/vk/VkTestCont...
tools/gpu/vk/VkTestContext.cpp:79: return
reinterpret_cast<SkPlatformGpuFence>(fence);
On 2016/05/16 17:19:41, egdaniel wrote:
> nit: remove tab

Done.

https://codereview.chromium.org/1974913003/diff/60001/tools/gpu/vk/VkTestCont...
tools/gpu/vk/VkTestContext.cpp:127: INHERITED::teardown(); fVk.reset(nullptr);
On 2016/05/16 17:19:41, egdaniel wrote:
> put these on two lines?

Done.

[bsalomon, 2016-05-16 20:37:17]

The CQ bit was checked by bsalomon@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-05-16 20:37:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1974913003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1974913003/80001

[egdaniel, 2016-05-16 20:45:41]

https://codereview.chromium.org/1974913003/diff/60001/tools/gpu/vk/VkTestCont...
File tools/gpu/vk/VkTestContext.cpp (right):

https://codereview.chromium.org/1974913003/diff/60001/tools/gpu/vk/VkTestCont...
tools/gpu/vk/VkTestContext.cpp:11: #include <vulkan/vulkan.h>
On 2016/05/16 20:37:08, bsalomon wrote:
> On 2016/05/16 17:19:41, egdaniel wrote:
> > would need to include third_party in gyp includes?
> 
> I'm not sure I understand this comment. I moved all but the first include
inside
> the below #ifdef. Does that address your concern?

Oh it was just asking if vulkan/vulkan.h would be accessible from the tools
directory. The #ifdef should work fine to block it from non vulkan builds trying
to include it.

https://codereview.chromium.org/1974913003/diff/60001/tools/gpu/vk/VkTestCont...
tools/gpu/vk/VkTestContext.cpp:55: // If the above assertion is true then the
command buffer should not be in flight.
On 2016/05/16 20:37:08, bsalomon wrote:
> On 2016/05/16 17:19:41, egdaniel wrote:
> > For the above assertion and comment to be true it requires that we called
> > deleteFence only after first calling waitFence on it. Is this always true?
> > Should deleteFence have some debug assert that the fence has been signaled?
> 
> Right, the assertion only checks that deleteFence was called.
> 
> I'll follow this up with a CL that combines waitFence and deleteFence into a
> single call.

Sounds good, any way we can require that both have been called works for me.

[commit-bot: I haz the power, 2016-05-16 20:56:09]

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

[commit-bot: I haz the power, 2016-05-16 20:56:10]

Dry run: This issue passed the CQ dry run.

[bsalomon, 2016-05-16 21:09:01]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2016-05-16 21:09:01]

The patchset sent to the CQ was uploaded after l-g-t-m from jvanverth@google.com
Link to the patchset: https://codereview.chromium.org/1974913003/#ps80001
(title: "-Address comments and fix compilation of non-vulkan build")

[commit-bot: I haz the power, 2016-05-16 21:09:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1974913003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1974913003/80001

[commit-bot: I haz the power, 2016-05-16 21:09:58]

Description was changed from

==========
Implement vulkan fence syncs for nanobench

GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Implement vulkan fence syncs for nanobench

GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/edea94c356dddd339d776e1f0d906f44427222b9
==========

[commit-bot: I haz the power, 2016-05-16 21:09:59]

Committed patchset #5 (id:80001) as
https://skia.googlesource.com/skia/+/edea94c356dddd339d776e1f0d906f44427222b9