Use transfer buffer for BatchAtlas texture copies

Use transfer buffer for BatchAtlas texture copies. 

Sets up use of transfer buffer (if available) to do one-copy transfers. 
Get transfer buffers working properly in GL. 
Implement GrVkGpu::onTransferPixels. 
Check caps to ensure we can create a transfer buffer. 

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

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

[jvanverth1, 2016-04-13 15:16:52]

Description was changed from

==========
First pass at getting transfer buffer working again

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

to

==========
First pass at getting transfer buffer working again

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

[jvanverth1, 2016-04-15 16:44:33]

Description was changed from

==========
First pass at getting transfer buffer working again

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

to

==========
Use transfer buffer for BatchAtlas texture copies

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

[jvanverth1, 2016-04-15 19:40:59]

Description was changed from

==========
Use transfer buffer for BatchAtlas texture copies

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

to

==========
Use transfer buffer for BatchAtlas texture copies

Sets up use of transfer buffer (if available) to do one-copy transfers
Get transfer buffers working properly in GL
Implement GrVkGpu::onTransferPixels
Check caps to ensure we can create a transfer buffer

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

[jvanverth1, 2016-04-15 19:40:59]

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

[jvanverth1, 2016-04-15 19:41:12]

Description was changed from

==========
Use transfer buffer for BatchAtlas texture copies

Sets up use of transfer buffer (if available) to do one-copy transfers
Get transfer buffers working properly in GL
Implement GrVkGpu::onTransferPixels
Check caps to ensure we can create a transfer buffer

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

to

==========
Use transfer buffer for BatchAtlas texture copies

Sets up use of transfer buffer (if available) to do one-copy transfers. 
Get transfer buffers working properly in GL. 
Implement GrVkGpu::onTransferPixels. 
Check caps to ensure we can create a transfer buffer. 

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

[jvanverth1, 2016-04-15 19:41:22]

Description was changed from

==========
Use transfer buffer for BatchAtlas texture copies

Sets up use of transfer buffer (if available) to do one-copy transfers. 
Get transfer buffers working properly in GL. 
Implement GrVkGpu::onTransferPixels. 
Check caps to ensure we can create a transfer buffer. 

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

to

==========
Use transfer buffer for BatchAtlas texture copies. 

Sets up use of transfer buffer (if available) to do one-copy transfers. 
Get transfer buffers working properly in GL. 
Implement GrVkGpu::onTransferPixels. 
Check caps to ensure we can create a transfer buffer. 

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

[jvanverth1, 2016-04-15 19:46:15]

This appears to work well even without a fence -- I've tested it on Mac, PC, and
Linux and there's no sign of sync issues when spilling the cache. It does
provide a tiny speedup of 1-2 ms on the things I've tried.

It will not be enabled in Chrome -- the Chromium extension doesn't seem to work
properly through the command buffer.

[egdaniel, 2016-04-18 16:46:35]

In general, I wonder if, at least in vulkan world, we can share a lot of code
between onTransferPixels and onReadPixels

https://codereview.chromium.org/1888473002/diff/80001/src/gpu/vk/GrVkGpu.cpp
File src/gpu/vk/GrVkGpu.cpp (right):

https://codereview.chromium.org/1888473002/diff/80001/src/gpu/vk/GrVkGpu.cpp#...
src/gpu/vk/GrVkGpu.cpp:285: if (!vkTex) {
why do you require that this is a texture for vulkan? seems like this could be
done on any generic VkImage. However you may need then check for multisampling
on render targets

https://codereview.chromium.org/1888473002/diff/80001/src/gpu/vk/GrVkGpu.cpp#...
src/gpu/vk/GrVkGpu.cpp:298: // TODO: Not clear how to handle y axis flip
well I guess there are two options here. First would be to create a second
buffer that we copy all the data row by row into but reversed. The second would
be to first do the copyBufferToImage into a temp surface with origin TopLeft,
and then do a blit to the dst surface to perform the flip

https://codereview.chromium.org/1888473002/diff/80001/src/gpu/vk/GrVkGpu.cpp#...
src/gpu/vk/GrVkGpu.cpp:323: VkOffset3D offset = {
why not just inline this like subresource or extent? In writePixels we pulled it
out cause we did some work on settings its values for flipping y.

https://codereview.chromium.org/1888473002/diff/80001/src/gpu/vk/GrVkGpu.cpp#...
src/gpu/vk/GrVkGpu.cpp:354: fCurrentCmdBuffer->copyBufferToImage(this,
if the surface is linearly tiled should we not just map and copy it directly?

[bsalomon, 2016-04-19 14:48:36]

lgtm modulo Greg's vk comments. Any win from this?

https://codereview.chromium.org/1888473002/diff/80001/src/gpu/GrBatchAtlas.cpp
File src/gpu/GrBatchAtlas.cpp (right):

https://codereview.chromium.org/1888473002/diff/80001/src/gpu/GrBatchAtlas.cp...
src/gpu/GrBatchAtlas.cpp:44: fTransferBuffer->unref();
Maybe use sk_sp to store fTransferBuffer? I guess you have to check whether it
exists here anyway, so not a big improvement.

https://codereview.chromium.org/1888473002/diff/80001/src/gpu/GrBatchAtlas.cp...
src/gpu/GrBatchAtlas.cpp:65: SkASSERT(fResourceProvider);
This seems a little overkill, but I don't really care.

[jvanverth1, 2016-04-19 17:14:27]

Addressing comments:

https://codereview.chromium.org/1888473002/diff/80001/src/gpu/GrBatchAtlas.cpp
File src/gpu/GrBatchAtlas.cpp (right):

https://codereview.chromium.org/1888473002/diff/80001/src/gpu/GrBatchAtlas.cp...
src/gpu/GrBatchAtlas.cpp:44: fTransferBuffer->unref();
On 2016/04/19 14:48:36, bsalomon wrote:
> Maybe use sk_sp to store fTransferBuffer? I guess you have to check whether it
> exists here anyway, so not a big improvement.

Yeah, I don't think it saves much.

https://codereview.chromium.org/1888473002/diff/80001/src/gpu/GrBatchAtlas.cp...
src/gpu/GrBatchAtlas.cpp:65: SkASSERT(fResourceProvider);
On 2016/04/19 14:48:36, bsalomon wrote:
> This seems a little overkill, but I don't really care.

Done.

https://codereview.chromium.org/1888473002/diff/80001/src/gpu/vk/GrVkGpu.cpp
File src/gpu/vk/GrVkGpu.cpp (right):

https://codereview.chromium.org/1888473002/diff/80001/src/gpu/vk/GrVkGpu.cpp#...
src/gpu/vk/GrVkGpu.cpp:285: if (!vkTex) {
On 2016/04/18 16:46:34, egdaniel wrote:
> why do you require that this is a texture for vulkan? seems like this could be
> done on any generic VkImage. However you may need then check for multisampling
> on render targets

The expected use case for this is to transfer CPU-rendered data to textures.
Adding support for RTs adds a lot of conditionals -- I'd rather not complicate
the code unless we're sure we'll use it.

Also, it means that transferPixels works on different kinds of data between
OpenGL and Vulkan, which I think would confuse things at a higher level.

https://codereview.chromium.org/1888473002/diff/80001/src/gpu/vk/GrVkGpu.cpp#...
src/gpu/vk/GrVkGpu.cpp:298: // TODO: Not clear how to handle y axis flip
On 2016/04/18 16:46:34, egdaniel wrote:
> well I guess there are two options here. First would be to create a second
> buffer that we copy all the data row by row into but reversed. The second
would
> be to first do the copyBufferToImage into a temp surface with origin TopLeft,
> and then do a blit to the dst surface to perform the flip

The temp image seems like the best bet -- I'm going to wait until I get the
image allocator working for that. And again, the textures we're using for
atlases and path uploads have top left origins.

https://codereview.chromium.org/1888473002/diff/80001/src/gpu/vk/GrVkGpu.cpp#...
src/gpu/vk/GrVkGpu.cpp:323: VkOffset3D offset = {
On 2016/04/18 16:46:34, egdaniel wrote:
> why not just inline this like subresource or extent? In writePixels we pulled
it
> out cause we did some work on settings its values for flipping y.

Done.

https://codereview.chromium.org/1888473002/diff/80001/src/gpu/vk/GrVkGpu.cpp#...
src/gpu/vk/GrVkGpu.cpp:354: fCurrentCmdBuffer->copyBufferToImage(this,
On 2016/04/18 16:46:34, egdaniel wrote:
> if the surface is linearly tiled should we not just map and copy it directly?

That would copy the data from the GPU to the CPU, and back to the GPU.

[egdaniel, 2016-04-19 17:22:18]

https://codereview.chromium.org/1888473002/diff/80001/src/gpu/vk/GrVkGpu.cpp
File src/gpu/vk/GrVkGpu.cpp (right):

https://codereview.chromium.org/1888473002/diff/80001/src/gpu/vk/GrVkGpu.cpp#...
src/gpu/vk/GrVkGpu.cpp:285: if (!vkTex) {
On 2016/04/19 17:14:27, jvanverth1 wrote:
> On 2016/04/18 16:46:34, egdaniel wrote:
> > why do you require that this is a texture for vulkan? seems like this could
be
> > done on any generic VkImage. However you may need then check for
multisampling
> > on render targets
> 
> The expected use case for this is to transfer CPU-rendered data to textures.
> Adding support for RTs adds a lot of conditionals -- I'd rather not complicate
> the code unless we're sure we'll use it.
> 
> Also, it means that transferPixels works on different kinds of data between
> OpenGL and Vulkan, which I think would confuse things at a higher level.

Well if we don't want to OpenGL and Vulkan be different, can we change the
function signature to take a GrTexture instead? If we leave it as GrSurface I
feel it should support RT if able.

https://codereview.chromium.org/1888473002/diff/80001/src/gpu/vk/GrVkGpu.cpp#...
src/gpu/vk/GrVkGpu.cpp:354: fCurrentCmdBuffer->copyBufferToImage(this,
On 2016/04/19 17:14:27, jvanverth1 wrote:
> On 2016/04/18 16:46:34, egdaniel wrote:
> > if the surface is linearly tiled should we not just map and copy it
directly?
> 
> That would copy the data from the GPU to the CPU, and back to the GPU.

unless it is cached :), but we can punt on this for now until we add some
tracking to know if the gpu memory is cached on cpu or not.

[jvanverth1, 2016-04-19 18:12:07]

On 2016/04/19 17:22:18, egdaniel wrote:
> https://codereview.chromium.org/1888473002/diff/80001/src/gpu/vk/GrVkGpu.cpp
> File src/gpu/vk/GrVkGpu.cpp (right):
> 
>
https://codereview.chromium.org/1888473002/diff/80001/src/gpu/vk/GrVkGpu.cpp#...
> src/gpu/vk/GrVkGpu.cpp:285: if (!vkTex) {
> On 2016/04/19 17:14:27, jvanverth1 wrote:
> > On 2016/04/18 16:46:34, egdaniel wrote:
> > > why do you require that this is a texture for vulkan? seems like this
could
> be
> > > done on any generic VkImage. However you may need then check for
> multisampling
> > > on render targets
> > 
> > The expected use case for this is to transfer CPU-rendered data to textures.
> > Adding support for RTs adds a lot of conditionals -- I'd rather not
complicate
> > the code unless we're sure we'll use it.
> > 
> > Also, it means that transferPixels works on different kinds of data between
> > OpenGL and Vulkan, which I think would confuse things at a higher level.
> 
> Well if we don't want to OpenGL and Vulkan be different, can we change the
> function signature to take a GrTexture instead? If we leave it as GrSurface I
> feel it should support RT if able.

That change propagates pretty far -- in particular, into
GrSurface::writePixels(). It doesn't seem appropriate for this patch.

>
https://codereview.chromium.org/1888473002/diff/80001/src/gpu/vk/GrVkGpu.cpp#...
> src/gpu/vk/GrVkGpu.cpp:354: fCurrentCmdBuffer->copyBufferToImage(this,
> On 2016/04/19 17:14:27, jvanverth1 wrote:
> > On 2016/04/18 16:46:34, egdaniel wrote:
> > > if the surface is linearly tiled should we not just map and copy it
> directly?
> > 
> > That would copy the data from the GPU to the CPU, and back to the GPU.
> 
> unless it is cached :), but we can punt on this for now until we add some
> tracking to know if the gpu memory is cached on cpu or not.

If the image is cached, then we don't need a transfer buffer.

[bsalomon, 2016-04-19 18:19:26]

https://codereview.chromium.org/1888473002/diff/80001/src/gpu/vk/GrVkGpu.cpp
File src/gpu/vk/GrVkGpu.cpp (right):

https://codereview.chromium.org/1888473002/diff/80001/src/gpu/vk/GrVkGpu.cpp#...
src/gpu/vk/GrVkGpu.cpp:285: if (!vkTex) {
On 2016/04/19 17:22:18, egdaniel wrote:
> On 2016/04/19 17:14:27, jvanverth1 wrote:
> > On 2016/04/18 16:46:34, egdaniel wrote:
> > > why do you require that this is a texture for vulkan? seems like this
could
> be
> > > done on any generic VkImage. However you may need then check for
> multisampling
> > > on render targets
> > 
> > The expected use case for this is to transfer CPU-rendered data to textures.
> > Adding support for RTs adds a lot of conditionals -- I'd rather not
complicate
> > the code unless we're sure we'll use it.
> > 
> > Also, it means that transferPixels works on different kinds of data between
> > OpenGL and Vulkan, which I think would confuse things at a higher level.
> 
> Well if we don't want to OpenGL and Vulkan be different, can we change the
> function signature to take a GrTexture instead? If we leave it as GrSurface I
> feel it should support RT if able.

If we are only planning to use it with textures and supporting RTs is
complicated on either GL or VK then it makes sense to me to make it take a
texture.

[jvanverth1, 2016-04-19 18:26:07]

On 2016/04/19 18:19:26, bsalomon wrote:
> https://codereview.chromium.org/1888473002/diff/80001/src/gpu/vk/GrVkGpu.cpp
> File src/gpu/vk/GrVkGpu.cpp (right):
> 
>
https://codereview.chromium.org/1888473002/diff/80001/src/gpu/vk/GrVkGpu.cpp#...
> src/gpu/vk/GrVkGpu.cpp:285: if (!vkTex) {
> On 2016/04/19 17:22:18, egdaniel wrote:
> > On 2016/04/19 17:14:27, jvanverth1 wrote:
> > > On 2016/04/18 16:46:34, egdaniel wrote:
> > > > why do you require that this is a texture for vulkan? seems like this
> could
> > be
> > > > done on any generic VkImage. However you may need then check for
> > multisampling
> > > > on render targets
> > > 
> > > The expected use case for this is to transfer CPU-rendered data to
textures.
> > > Adding support for RTs adds a lot of conditionals -- I'd rather not
> complicate
> > > the code unless we're sure we'll use it.
> > > 
> > > Also, it means that transferPixels works on different kinds of data
between
> > > OpenGL and Vulkan, which I think would confuse things at a higher level.
> > 
> > Well if we don't want to OpenGL and Vulkan be different, can we change the
> > function signature to take a GrTexture instead? If we leave it as GrSurface
I
> > feel it should support RT if able.
> 
> If we are only planning to use it with textures and supporting RTs is
> complicated on either GL or VK then it makes sense to me to make it take a
> texture.

I agree but my previous comment probably seems like a non-sequitur --
writePixels() also only works on textures, so we should change it there as well.

[bsalomon, 2016-04-19 18:30:25]

On 2016/04/19 18:26:07, jvanverth1 wrote:
> On 2016/04/19 18:19:26, bsalomon wrote:
> > https://codereview.chromium.org/1888473002/diff/80001/src/gpu/vk/GrVkGpu.cpp
> > File src/gpu/vk/GrVkGpu.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1888473002/diff/80001/src/gpu/vk/GrVkGpu.cpp#...
> > src/gpu/vk/GrVkGpu.cpp:285: if (!vkTex) {
> > On 2016/04/19 17:22:18, egdaniel wrote:
> > > On 2016/04/19 17:14:27, jvanverth1 wrote:
> > > > On 2016/04/18 16:46:34, egdaniel wrote:
> > > > > why do you require that this is a texture for vulkan? seems like this
> > could
> > > be
> > > > > done on any generic VkImage. However you may need then check for
> > > multisampling
> > > > > on render targets
> > > > 
> > > > The expected use case for this is to transfer CPU-rendered data to
> textures.
> > > > Adding support for RTs adds a lot of conditionals -- I'd rather not
> > complicate
> > > > the code unless we're sure we'll use it.
> > > > 
> > > > Also, it means that transferPixels works on different kinds of data
> between
> > > > OpenGL and Vulkan, which I think would confuse things at a higher level.
> > > 
> > > Well if we don't want to OpenGL and Vulkan be different, can we change the
> > > function signature to take a GrTexture instead? If we leave it as
GrSurface
> I
> > > feel it should support RT if able.
> > 
> > If we are only planning to use it with textures and supporting RTs is
> > complicated on either GL or VK then it makes sense to me to make it take a
> > texture.
> 
> I agree but my previous comment probably seems like a non-sequitur --
> writePixels() also only works on textures, so we should change it there as
well.

My hesitation there is that at the GrContext level we have a set of fallbacks
for dealing with the failure (using an intermediate surface). We could add
support for glDrawPixels on non-ES GL. Transfer is only used internally at the
GrGpu level and the callers don't have a fallback if it fails. I'm not opposed
to changing writePixels, but it is slightly different.

[jvanverth1, 2016-04-19 18:51:20]

On 2016/04/19 18:30:25, bsalomon wrote:
> On 2016/04/19 18:26:07, jvanverth1 wrote:
> > On 2016/04/19 18:19:26, bsalomon wrote:
> > >
https://codereview.chromium.org/1888473002/diff/80001/src/gpu/vk/GrVkGpu.cpp
> > > File src/gpu/vk/GrVkGpu.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1888473002/diff/80001/src/gpu/vk/GrVkGpu.cpp#...
> > > src/gpu/vk/GrVkGpu.cpp:285: if (!vkTex) {
> > > On 2016/04/19 17:22:18, egdaniel wrote:
> > > > On 2016/04/19 17:14:27, jvanverth1 wrote:
> > > > > On 2016/04/18 16:46:34, egdaniel wrote:
> > > > > > why do you require that this is a texture for vulkan? seems like
this
> > > could
> > > > be
> > > > > > done on any generic VkImage. However you may need then check for
> > > > multisampling
> > > > > > on render targets
> > > > > 
> > > > > The expected use case for this is to transfer CPU-rendered data to
> > textures.
> > > > > Adding support for RTs adds a lot of conditionals -- I'd rather not
> > > complicate
> > > > > the code unless we're sure we'll use it.
> > > > > 
> > > > > Also, it means that transferPixels works on different kinds of data
> > between
> > > > > OpenGL and Vulkan, which I think would confuse things at a higher
level.
> > > > 
> > > > Well if we don't want to OpenGL and Vulkan be different, can we change
the
> > > > function signature to take a GrTexture instead? If we leave it as
> GrSurface
> > I
> > > > feel it should support RT if able.
> > > 
> > > If we are only planning to use it with textures and supporting RTs is
> > > complicated on either GL or VK then it makes sense to me to make it take a
> > > texture.
> > 
> > I agree but my previous comment probably seems like a non-sequitur --
> > writePixels() also only works on textures, so we should change it there as
> well.
> 
> My hesitation there is that at the GrContext level we have a set of fallbacks
> for dealing with the failure (using an intermediate surface). We could add
> support for glDrawPixels on non-ES GL. Transfer is only used internally at the
> GrGpu level and the callers don't have a fallback if it fails. I'm not opposed
> to changing writePixels, but it is slightly different.

Done.

[egdaniel, 2016-04-19 18:54:36]

lgtm

[jvanverth1, 2016-04-20 12:43:38]

The CQ bit was checked by jvanverth@google.com

[jvanverth1, 2016-04-20 12:43:38]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com,
egdaniel@google.com
Link to the patchset: https://codereview.chromium.org/1888473002/#ps160001
(title: "Rebase to ToT")

[commit-bot: I haz the power, 2016-04-20 12:43:45]

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

[commit-bot: I haz the power, 2016-04-20 12:54:04]

Description was changed from

==========
Use transfer buffer for BatchAtlas texture copies. 

Sets up use of transfer buffer (if available) to do one-copy transfers. 
Get transfer buffers working properly in GL. 
Implement GrVkGpu::onTransferPixels. 
Check caps to ensure we can create a transfer buffer. 

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

to

==========
Use transfer buffer for BatchAtlas texture copies. 

Sets up use of transfer buffer (if available) to do one-copy transfers. 
Get transfer buffers working properly in GL. 
Implement GrVkGpu::onTransferPixels. 
Check caps to ensure we can create a transfer buffer. 

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

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

[commit-bot: I haz the power, 2016-04-20 12:54:05]

Committed patchset #9 (id:160001) as
https://skia.googlesource.com/skia/+/b0ec9836dbf7f2304a3a29289b818719ca0a39bd

[jvanverth1, 2016-04-20 17:33:14]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/1904723003/ by jvanverth@google.com.

The reason for reverting is: Lots of sync issues with paths and text..

[jvanverth1, 2016-04-22 16:54:08]

Description was changed from

==========
Use transfer buffer for BatchAtlas texture copies. 

Sets up use of transfer buffer (if available) to do one-copy transfers. 
Get transfer buffers working properly in GL. 
Implement GrVkGpu::onTransferPixels. 
Check caps to ensure we can create a transfer buffer. 

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

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

to

==========
Use transfer buffer for BatchAtlas texture copies. 

Sets up use of transfer buffer (if available) to do one-copy transfers. 
Get transfer buffers working properly in GL. 
Implement GrVkGpu::onTransferPixels. 
Check caps to ensure we can create a transfer buffer. 

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

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

[jvanverth1, 2016-07-12 17:54:02]

So I did a little more work on this, trying to add fence support in. I have a
first pass where I can create a fence for a transfer and associate it with a
GrBatchAtlas::Plot, but I have no idea where to wait on it. There doesn't seem
to be any way for a batch to trace back to its pre-uploads (the non-inline
ones). And in any case, the actual draw is handled by the VertexBatch
superclass. Any suggestions on how to hook this in?

As a side note, it's a shame we have to use a fence and stall on the CPU -- I
think what we'd ideally want is a barrier or semaphore on the GPU where the draw
command waits for the upload to finish.

[jvanverth1, 2016-07-12 18:11:37]

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

[commit-bot: I haz the power, 2016-07-12 18:11:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bsalomon, 2016-07-12 18:14:43]

On 2016/07/12 17:54:02, jvanverth1 wrote:
> So I did a little more work on this, trying to add fence support in. I have a
> first pass where I can create a fence for a transfer and associate it with a
> GrBatchAtlas::Plot, but I have no idea where to wait on it. There doesn't seem
> to be any way for a batch to trace back to its pre-uploads (the non-inline
> ones). And in any case, the actual draw is handled by the VertexBatch
> superclass. Any suggestions on how to hook this in?

Perhaps we need to hand something back to the batch when it adds an upload that
can then be passed to GrVertexBatch::Target::draw() that indicates that the draw
requires the upload to be complete.

Do we need to sync the other way? That is, make sure that the last draw that
reads the old contents has finished before the transfer begins?

> 
> As a side note, it's a shame we have to use a fence and stall on the CPU -- I
> think what we'd ideally want is a barrier or semaphore on the GPU where the
draw
> command waits for the upload to finish.

Yeah, that could be terrible since the GPU may be working on an early frame.
There is a glWaitSync (as opposed to ClientWaitSync).

[jvanverth1, 2016-07-12 18:17:47]

On 2016/07/12 18:14:43, bsalomon wrote:
> On 2016/07/12 17:54:02, jvanverth1 wrote:
> > So I did a little more work on this, trying to add fence support in. I have
a
> > first pass where I can create a fence for a transfer and associate it with a
> > GrBatchAtlas::Plot, but I have no idea where to wait on it. There doesn't
seem
> > to be any way for a batch to trace back to its pre-uploads (the non-inline
> > ones). And in any case, the actual draw is handled by the VertexBatch
> > superclass. Any suggestions on how to hook this in?
> 
> Perhaps we need to hand something back to the batch when it adds an upload
that
> can then be passed to GrVertexBatch::Target::draw() that indicates that the
draw
> requires the upload to be complete.
> 
> Do we need to sync the other way? That is, make sure that the last draw that
> reads the old contents has finished before the transfer begins?

We might need to, yes.

> > As a side note, it's a shame we have to use a fence and stall on the CPU --
I
> > think what we'd ideally want is a barrier or semaphore on the GPU where the
> draw
> > command waits for the upload to finish.
> 
> Yeah, that could be terrible since the GPU may be working on an early frame.
> There is a glWaitSync (as opposed to ClientWaitSync).

Ah, that makes a little more sense to me now. I think that might work. And on
Vulkan we have direct access to the GPU-only sync objects.

[commit-bot: I haz the power, 2016-07-12 18:34:35]

The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-12 18:34:36]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-Trybot on
master.client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...)