Refactor vulkan buffer mapping and unmapping

Refactor vulkan buffer mapping and unmapping

A lot of this is so we don't have duplicated code in both the map/unmap and
updateData functions of GrVkBuffer. Also there were slightly differences in
how we handled various things in the two cases that this now unifies.

Also I added a barrier after the vkUpdateBuffer call which I believe was
missing.

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

Committed: https://skia.googlesource.com/skia/+/927ac9c532084c816801434340383e4bd822c726

[egdaniel, 2016-09-16 19:40:11]

Description was changed from

==========
Refactor vulkan buffer mapping and unmapping

A lot of this is so we don't have duplicated code in both the map/unmap and
updateData functions of GrVkBuffer. Also there were slightly differences in
how we handled various things in the two cases that this now unifies.

Also I added a barrier after the vkUpdateBuffer call which I believe was
missing.

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

to

==========
Refactor vulkan buffer mapping and unmapping

A lot of this is so we don't have duplicated code in both the map/unmap and
updateData functions of GrVkBuffer. Also there were slightly differences in
how we handled various things in the two cases that this now unifies.

Also I added a barrier after the vkUpdateBuffer call which I believe was
missing.

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

[egdaniel, 2016-09-16 20:00:27]

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

[egdaniel, 2016-09-16 20:00:28]

https://codereview.chromium.org/2344323002/diff/40001/src/gpu/vk/GrVkBuffer.cpp
File src/gpu/vk/GrVkBuffer.cpp (right):

https://codereview.chromium.org/2344323002/diff/40001/src/gpu/vk/GrVkBuffer.c...
src/gpu/vk/GrVkBuffer.cpp:156: SkASSERT(fMapPtr);
we now never delete the resources for static buffers. Before that was true in
updateData but not map.

[egdaniel, 2016-09-16 20:43:12]

https://codereview.chromium.org/2344323002/diff/40001/src/gpu/vk/GrVkBuffer.cpp
File src/gpu/vk/GrVkBuffer.cpp (right):

https://codereview.chromium.org/2344323002/diff/40001/src/gpu/vk/GrVkBuffer.c...
src/gpu/vk/GrVkBuffer.cpp:163: delete [] (unsigned char*)fMapPtr;
We actually should not need to ever delete and realloc the cpu data since it is
copied to the cmdbuffer when updateBuffer is called.

[jvanverth1, 2016-09-19 14:44:11]

https://codereview.chromium.org/2344323002/diff/60001/src/gpu/vk/GrVkBuffer.cpp
File src/gpu/vk/GrVkBuffer.cpp (right):

https://codereview.chromium.org/2344323002/diff/60001/src/gpu/vk/GrVkBuffer.c...
src/gpu/vk/GrVkBuffer.cpp:157: this->addMemoryBarrier(gpu,
I believe we add barriers externally in some cases when we map/unmap, so you
might want to look for that and remove them.

https://codereview.chromium.org/2344323002/diff/60001/src/gpu/vk/GrVkBuffer.c...
src/gpu/vk/GrVkBuffer.cpp:216: this->innerMap(gpu, srcSizeInBytes,
createdNewBuffer);
I don't think that two new methods are needed, since vkMap and vkUnmap
essentially wrap them. You could just call vkMap here and vkUnmap below.

[egdaniel, 2016-09-19 15:17:24]

https://codereview.chromium.org/2344323002/diff/60001/src/gpu/vk/GrVkBuffer.cpp
File src/gpu/vk/GrVkBuffer.cpp (right):

https://codereview.chromium.org/2344323002/diff/60001/src/gpu/vk/GrVkBuffer.c...
src/gpu/vk/GrVkBuffer.cpp:216: this->innerMap(gpu, srcSizeInBytes,
createdNewBuffer);
On 2016/09/19 14:44:11, jvanverth1 wrote:
> I don't think that two new methods are needed, since vkMap and vkUnmap
> essentially wrap them. You could just call vkMap here and vkUnmap below.

The main difference here is the size parameter is different for vkMap/Unmap vs
the one called from updateData

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

https://codereview.chromium.org/2344323002/diff/80001/src/gpu/vk/GrVkGpu.cpp#...
src/gpu/vk/GrVkGpu.cpp:605: transferBuffer->addMemoryBarrier(this,
Removed this barrier, but I actually don't think this was ever needed since cpu
writes are sinked implicitly on commnad buffer submits to queue.

[jvanverth1, 2016-09-19 15:37:31]

https://codereview.chromium.org/2344323002/diff/60001/src/gpu/vk/GrVkBuffer.cpp
File src/gpu/vk/GrVkBuffer.cpp (right):

https://codereview.chromium.org/2344323002/diff/60001/src/gpu/vk/GrVkBuffer.c...
src/gpu/vk/GrVkBuffer.cpp:216: this->innerMap(gpu, srcSizeInBytes,
createdNewBuffer);
On 2016/09/19 15:17:24, egdaniel wrote:
> On 2016/09/19 14:44:11, jvanverth1 wrote:
> > I don't think that two new methods are needed, since vkMap and vkUnmap
> > essentially wrap them. You could just call vkMap here and vkUnmap below.
> 
> The main difference here is the size parameter is different for vkMap/Unmap vs
> the one called from updateData

Ah, I missed that. How about calling them internalMap/internalUnmap (fits naming
elsewhere in the code), and inlining them in the header for vkMap and vkUnmap?

[egdaniel, 2016-09-19 16:01:42]

https://codereview.chromium.org/2344323002/diff/60001/src/gpu/vk/GrVkBuffer.cpp
File src/gpu/vk/GrVkBuffer.cpp (right):

https://codereview.chromium.org/2344323002/diff/60001/src/gpu/vk/GrVkBuffer.c...
src/gpu/vk/GrVkBuffer.cpp:216: this->innerMap(gpu, srcSizeInBytes,
createdNewBuffer);
On 2016/09/19 15:37:31, jvanverth1 wrote:
> On 2016/09/19 15:17:24, egdaniel wrote:
> > On 2016/09/19 14:44:11, jvanverth1 wrote:
> > > I don't think that two new methods are needed, since vkMap and vkUnmap
> > > essentially wrap them. You could just call vkMap here and vkUnmap below.
> > 
> > The main difference here is the size parameter is different for vkMap/Unmap
vs
> > the one called from updateData
> 
> Ah, I missed that. How about calling them internalMap/internalUnmap (fits
naming
> elsewhere in the code), and inlining them in the header for vkMap and vkUnmap?

Done.

[jvanverth1, 2016-09-19 16:14:27]

lgtm

[egdaniel, 2016-09-19 16:16:24]

The CQ bit was checked by egdaniel@google.com

[commit-bot: I haz the power, 2016-09-19 16:16:33]

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-09-19 16:32:11]

Description was changed from

==========
Refactor vulkan buffer mapping and unmapping

A lot of this is so we don't have duplicated code in both the map/unmap and
updateData functions of GrVkBuffer. Also there were slightly differences in
how we handled various things in the two cases that this now unifies.

Also I added a barrier after the vkUpdateBuffer call which I believe was
missing.

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

to

==========
Refactor vulkan buffer mapping and unmapping

A lot of this is so we don't have duplicated code in both the map/unmap and
updateData functions of GrVkBuffer. Also there were slightly differences in
how we handled various things in the two cases that this now unifies.

Also I added a barrier after the vkUpdateBuffer call which I believe was
missing.

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

Committed:
https://skia.googlesource.com/skia/+/927ac9c532084c816801434340383e4bd822c726
==========

[commit-bot: I haz the power, 2016-09-19 16:32:12]

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/927ac9c532084c816801434340383e4bd822c726