Setup system in Vulkan to reuse VkDescriptorSet allocations.

Setup system in Vulkan to reuse VkDescriptorSet allocations.

This CL uses the new system for uniform buffers. In a follow up CL I will
add support for samplers.

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

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

[egdaniel, 2016-07-19 15:35:48]

Description was changed from

==========
Setup system in Vulkan to reuse VkDescriptorSet allocations.

This CL uses the new system for uniform buffers. In a follow up CL I will
add support for samplers.

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

to

==========
Setup system in Vulkan to reuse VkDescriptorSet allocations.

This CL uses the new system for uniform buffers. In a follow up CL I will
add support for samplers.

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

[egdaniel, 2016-07-20 16:55:39]

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

[jvanverth1, 2016-07-21 15:30:06]

https://codereview.chromium.org/2163673002/diff/60001/gyp/gpu.gypi
File gyp/gpu.gypi (right):

https://codereview.chromium.org/2163673002/diff/60001/gyp/gpu.gypi#newcode460
gyp/gpu.gypi:460: '<(skia_src_path)/gpu/vk/GrVkDescriptorSet.cpp',
I think this file is missing.

https://codereview.chromium.org/2163673002/diff/60001/src/gpu/vk/GrVkDescript...
File src/gpu/vk/GrVkDescriptorSet.h (right):

https://codereview.chromium.org/2163673002/diff/60001/src/gpu/vk/GrVkDescript...
src/gpu/vk/GrVkDescriptorSet.h:29: void dumpInfo() const override {
Indented too far.

https://codereview.chromium.org/2163673002/diff/60001/src/gpu/vk/GrVkDescript...
File src/gpu/vk/GrVkDescriptorSetManager.cpp (right):

https://codereview.chromium.org/2163673002/diff/60001/src/gpu/vk/GrVkDescript...
src/gpu/vk/GrVkDescriptorSetManager.cpp:22: const GrVkDescriptorSet*
GrVkDescriptorSetManager::refDescriptorSet(GrVkGpu* gpu,
It's not clear why this is called refDescriptorSet. Maybe getDescriptorSet?

https://codereview.chromium.org/2163673002/diff/60001/src/gpu/vk/GrVkDescript...
src/gpu/vk/GrVkDescriptorSetManager.cpp:28: fFreeSets.removeShuffle(count - 1);
I don't think it has to happen in this change, but we're using this pattern a
lot. I think SkTArray needs a pop_back() method.

https://codereview.chromium.org/2163673002/diff/60001/src/gpu/vk/GrVkDescript...
src/gpu/vk/GrVkDescriptorSetManager.cpp:41: descSet->ref();
I wonder if it makes sense to remove this, and the unref in
GrVkRecyclableResource::recycle.

https://codereview.chromium.org/2163673002/diff/60001/src/gpu/vk/GrVkDescript...
File src/gpu/vk/GrVkDescriptorSetManager.h (right):

https://codereview.chromium.org/2163673002/diff/60001/src/gpu/vk/GrVkDescript...
src/gpu/vk/GrVkDescriptorSetManager.h:37: const GrVkDescriptorSet*
refDescriptorSet(GrVkGpu* gpu, const Handle& handle);
why this?

https://codereview.chromium.org/2163673002/diff/60001/src/gpu/vk/GrVkDescript...
src/gpu/vk/GrVkDescriptorSetManager.h:81: private:
Indented one space too far?

https://codereview.chromium.org/2163673002/diff/60001/src/gpu/vk/GrVkDescript...
src/gpu/vk/GrVkDescriptorSetManager.h:93: // If the number of bindings in 0 we
assume this is for uniform buffers
sp: is 0

[egdaniel, 2016-07-21 17:27:11]

https://codereview.chromium.org/2163673002/diff/60001/gyp/gpu.gypi
File gyp/gpu.gypi (right):

https://codereview.chromium.org/2163673002/diff/60001/gyp/gpu.gypi#newcode460
gyp/gpu.gypi:460: '<(skia_src_path)/gpu/vk/GrVkDescriptorSet.cpp',
On 2016/07/21 15:30:06, jvanverth1 wrote:
> I think this file is missing.

Done.

https://codereview.chromium.org/2163673002/diff/60001/src/gpu/vk/GrVkDescript...
File src/gpu/vk/GrVkDescriptorSet.h (right):

https://codereview.chromium.org/2163673002/diff/60001/src/gpu/vk/GrVkDescript...
src/gpu/vk/GrVkDescriptorSet.h:29: void dumpInfo() const override {
On 2016/07/21 15:30:06, jvanverth1 wrote:
> Indented too far.

Done.

https://codereview.chromium.org/2163673002/diff/60001/src/gpu/vk/GrVkDescript...
File src/gpu/vk/GrVkDescriptorSetManager.cpp (right):

https://codereview.chromium.org/2163673002/diff/60001/src/gpu/vk/GrVkDescript...
src/gpu/vk/GrVkDescriptorSetManager.cpp:22: const GrVkDescriptorSet*
GrVkDescriptorSetManager::refDescriptorSet(GrVkGpu* gpu,
On 2016/07/21 15:30:06, jvanverth1 wrote:
> It's not clear why this is called refDescriptorSet. Maybe getDescriptorSet?

Ah when I originally wrote this I was storing all the allocations so I was
actually just doing a ref here. Never changed name when I updated.

https://codereview.chromium.org/2163673002/diff/60001/src/gpu/vk/GrVkDescript...
src/gpu/vk/GrVkDescriptorSetManager.cpp:28: fFreeSets.removeShuffle(count - 1);
On 2016/07/21 15:30:06, jvanverth1 wrote:
> I don't think it has to happen in this change, but we're using this pattern a
> lot. I think SkTArray needs a pop_back() method.

Yeah I think it does actaully have a pop_back() method, but the method does not
return the variable that it pops off. Also I think remove_shuffle is better than
pop_back since pop_back does a realloc check.

https://codereview.chromium.org/2163673002/diff/60001/src/gpu/vk/GrVkDescript...
src/gpu/vk/GrVkDescriptorSetManager.cpp:41: descSet->ref();
On 2016/07/21 15:30:06, jvanverth1 wrote:
> I wonder if it makes sense to remove this, and the unref in
> GrVkRecyclableResource::recycle.

Done.

https://codereview.chromium.org/2163673002/diff/60001/src/gpu/vk/GrVkDescript...
File src/gpu/vk/GrVkDescriptorSetManager.h (right):

https://codereview.chromium.org/2163673002/diff/60001/src/gpu/vk/GrVkDescript...
src/gpu/vk/GrVkDescriptorSetManager.h:37: const GrVkDescriptorSet*
refDescriptorSet(GrVkGpu* gpu, const Handle& handle);
On 2016/07/21 15:30:06, jvanverth1 wrote:
> why this?

changed to get.

https://codereview.chromium.org/2163673002/diff/60001/src/gpu/vk/GrVkDescript...
src/gpu/vk/GrVkDescriptorSetManager.h:81: private:
On 2016/07/21 15:30:06, jvanverth1 wrote:
> Indented one space too far?

Done.

https://codereview.chromium.org/2163673002/diff/60001/src/gpu/vk/GrVkDescript...
src/gpu/vk/GrVkDescriptorSetManager.h:93: // If the number of bindings in 0 we
assume this is for uniform buffers
On 2016/07/21 15:30:06, jvanverth1 wrote:
> sp: is 0

Done.

[jvanverth1, 2016-07-21 18:22:11]

lgtm

[egdaniel, 2016-07-21 18:23:55]

The CQ bit was checked by egdaniel@google.com

[commit-bot: I haz the power, 2016-07-21 18:24:06]

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-07-21 18:50:41]

Description was changed from

==========
Setup system in Vulkan to reuse VkDescriptorSet allocations.

This CL uses the new system for uniform buffers. In a follow up CL I will
add support for samplers.

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

to

==========
Setup system in Vulkan to reuse VkDescriptorSet allocations.

This CL uses the new system for uniform buffers. In a follow up CL I will
add support for samplers.

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

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

[commit-bot: I haz the power, 2016-07-21 18:50:42]

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