Add DescriptorPool and set manager to GrVkProgram

Add DescriptorPool and set manager to GrVkProgram

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

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

[egdaniel, 2016-03-04 14:33:28]

Description was changed from

==========
Add DescriptorPool and set manager to GrVkProgram

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

to

==========
Add DescriptorPool and set manager to GrVkProgram

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

[egdaniel, 2016-03-04 14:58:23]

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

[egdaniel, 2016-03-04 14:58:23]

So this change does not try to reuse descriptor pools and/or descriptor sets
even if they could be. That will come in a future change.

[egdaniel, 2016-03-07 18:10:22]

ping

[egdaniel, 2016-03-07 18:10:22]

ping

[jvanverth1, 2016-03-07 19:04:11]

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

https://codereview.chromium.org/1765923002/diff/40001/src/gpu/vk/GrVkProgram....
src/gpu/vk/GrVkProgram.cpp:59: fStartDS =
SkTMin(GrVkUniformHandler::kSamplerDescSet,
GrVkUniformHandler::kSamplerDescSet);
I'm not sure what this is trying to do.

https://codereview.chromium.org/1765923002/diff/40001/src/gpu/vk/GrVkProgram....
src/gpu/vk/GrVkProgram.cpp:349: commandBuffer->bindDescriptorSets(gpu, this,
fPipelineLayout, fStartDS, fDSCount,
So is the goal here to bind either 1 or 2 descriptor sets, depending whether we
need a sampler?

https://codereview.chromium.org/1765923002/diff/40001/src/gpu/vk/GrVkProgram....
src/gpu/vk/GrVkProgram.cpp:389: }
This seems unlikely, but if fPool is non-null and fDescCount is 0, then you'll
have a dangling pointer. Maybe assert that fDescCount > 0 instead? (the class
doesn't seem to make sense otherwise)

https://codereview.chromium.org/1765923002/diff/40001/src/gpu/vk/GrVkProgram.h
File src/gpu/vk/GrVkProgram.h (right):

https://codereview.chromium.org/1765923002/diff/40001/src/gpu/vk/GrVkProgram....
src/gpu/vk/GrVkProgram.h:78: if (fDescCount * fMaxDescriptorSets > 1 << 8) {
Use a constant rather than 1 << 8? And why 256?

[egdaniel, 2016-03-07 20:02:25]

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

https://codereview.chromium.org/1765923002/diff/40001/src/gpu/vk/GrVkProgram....
src/gpu/vk/GrVkProgram.cpp:59: fStartDS =
SkTMin(GrVkUniformHandler::kSamplerDescSet,
GrVkUniformHandler::kSamplerDescSet);
On 2016/03/07 19:04:11, jvanverth1 wrote:
> I'm not sure what this is trying to do.

We are always using a VkPipelineLayout that has a slot for two descriptor sets.
However, if we don't have any samplers I do not actually allocate a descriptor
set for the samplers since it is an error to make a descriptor pool of size 0.
Thus when it comes to bind the descriptor sets to the command buffer, I need to
tell the driver which descriptor sets in the VkPipelineLayout I'm binding (and
can't bind a null descriptor set to the sampler spot). The SkTMin stuff is there
so we aren't relying on one of the samplers to be in the 0 or 1 position.

https://codereview.chromium.org/1765923002/diff/40001/src/gpu/vk/GrVkProgram....
src/gpu/vk/GrVkProgram.cpp:349: commandBuffer->bindDescriptorSets(gpu, this,
fPipelineLayout, fStartDS, fDSCount,
On 2016/03/07 19:04:11, jvanverth1 wrote:
> So is the goal here to bind either 1 or 2 descriptor sets, depending whether
we
> need a sampler?

yes see above comment

https://codereview.chromium.org/1765923002/diff/40001/src/gpu/vk/GrVkProgram....
src/gpu/vk/GrVkProgram.cpp:389: }
On 2016/03/07 19:04:11, jvanverth1 wrote:
> This seems unlikely, but if fPool is non-null and fDescCount is 0, then you'll
> have a dangling pointer. Maybe assert that fDescCount > 0 instead? (the class
> doesn't seem to make sense otherwise)

Technically we would have never made an fPool to begin we have an fDescCount of
0, so don't think there is any worry of a hanging pointer. To assert that
fDescCount is not 0 we would have to change how we create the
DescriptorPoolManager for samplers so that we don't create one if fDescCount is
0. But then I feel like I would need to move a bunch of these if checks into the
using class to know if the sampler DescriptorPoolManager is valid or not.

https://codereview.chromium.org/1765923002/diff/40001/src/gpu/vk/GrVkProgram.h
File src/gpu/vk/GrVkProgram.h (right):

https://codereview.chromium.org/1765923002/diff/40001/src/gpu/vk/GrVkProgram....
src/gpu/vk/GrVkProgram.h:78: if (fDescCount * fMaxDescriptorSets > 1 << 8) {
On 2016/03/07 19:04:11, jvanverth1 wrote:
> Use a constant rather than 1 << 8? And why 256?

The descriptorpool type count struct I made previously (with array of the
descriptor types), we decided to make the counts be uint8_t to reduce the size.
Thus I need to make sure we never try to make more than 256.

I will add a constant though.

[jvanverth1, 2016-03-07 20:38:39]

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

https://codereview.chromium.org/1765923002/diff/40001/src/gpu/vk/GrVkProgram....
src/gpu/vk/GrVkProgram.cpp:59: fStartDS =
SkTMin(GrVkUniformHandler::kSamplerDescSet,
GrVkUniformHandler::kSamplerDescSet);
On 2016/03/07 20:02:25, egdaniel wrote:
> On 2016/03/07 19:04:11, jvanverth1 wrote:
> > I'm not sure what this is trying to do.
> 
> We are always using a VkPipelineLayout that has a slot for two descriptor
sets.
> However, if we don't have any samplers I do not actually allocate a descriptor
> set for the samplers since it is an error to make a descriptor pool of size 0.
> Thus when it comes to bind the descriptor sets to the command buffer, I need
to
> tell the driver which descriptor sets in the VkPipelineLayout I'm binding (and
> can't bind a null descriptor set to the sampler spot). The SkTMin stuff is
there
> so we aren't relying on one of the samplers to be in the 0 or 1 position.

I think maybe you want SkTMin(fStartDS, GrVkUniformHandler::kSamplerDescSet);

https://codereview.chromium.org/1765923002/diff/40001/src/gpu/vk/GrVkProgram....
src/gpu/vk/GrVkProgram.cpp:389: }
On 2016/03/07 20:02:25, egdaniel wrote:
> On 2016/03/07 19:04:11, jvanverth1 wrote:
> > This seems unlikely, but if fPool is non-null and fDescCount is 0, then
you'll
> > have a dangling pointer. Maybe assert that fDescCount > 0 instead? (the
class
> > doesn't seem to make sense otherwise)
> 
> Technically we would have never made an fPool to begin we have an fDescCount
of
> 0, so don't think there is any worry of a hanging pointer. To assert that
> fDescCount is not 0 we would have to change how we create the
> DescriptorPoolManager for samplers so that we don't create one if fDescCount
is
> 0. But then I feel like I would need to move a bunch of these if checks into
the
> using class to know if the sampler DescriptorPoolManager is valid or not.

Ah, I see. We could have one where fDescCount == 0 in that case. I guess this is
okay, though it still makes me uneasy.

[egdaniel, 2016-03-07 21:30:15]

more changes to come based on what we discussed

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

https://codereview.chromium.org/1765923002/diff/40001/src/gpu/vk/GrVkProgram....
src/gpu/vk/GrVkProgram.cpp:389: }
On 2016/03/07 20:38:38, jvanverth1 wrote:
> On 2016/03/07 20:02:25, egdaniel wrote:
> > On 2016/03/07 19:04:11, jvanverth1 wrote:
> > > This seems unlikely, but if fPool is non-null and fDescCount is 0, then
> you'll
> > > have a dangling pointer. Maybe assert that fDescCount > 0 instead? (the
> class
> > > doesn't seem to make sense otherwise)
> > 
> > Technically we would have never made an fPool to begin we have an fDescCount
> of
> > 0, so don't think there is any worry of a hanging pointer. To assert that
> > fDescCount is not 0 we would have to change how we create the
> > DescriptorPoolManager for samplers so that we don't create one if fDescCount
> is
> > 0. But then I feel like I would need to move a bunch of these if checks into
> the
> > using class to know if the sampler DescriptorPoolManager is valid or not.
> 
> Ah, I see. We could have one where fDescCount == 0 in that case. I guess this
is
> okay, though it still makes me uneasy.

What if at the end of the function we have SkASSERT(fPool || !fTypeCount)

[jvanverth1, 2016-03-08 13:36:03]

On 2016/03/07 21:30:15, egdaniel wrote:
> more changes to come based on what we discussed
> 
>
https://codereview.chromium.org/1765923002/diff/40001/src/gpu/vk/GrVkProgram.cpp
> File src/gpu/vk/GrVkProgram.cpp (right):
> 
>
https://codereview.chromium.org/1765923002/diff/40001/src/gpu/vk/GrVkProgram....
> src/gpu/vk/GrVkProgram.cpp:389: }
> On 2016/03/07 20:38:38, jvanverth1 wrote:
> > On 2016/03/07 20:02:25, egdaniel wrote:
> > > On 2016/03/07 19:04:11, jvanverth1 wrote:
> > > > This seems unlikely, but if fPool is non-null and fDescCount is 0, then
> > you'll
> > > > have a dangling pointer. Maybe assert that fDescCount > 0 instead? (the
> > class
> > > > doesn't seem to make sense otherwise)
> > > 
> > > Technically we would have never made an fPool to begin we have an
fDescCount
> > of
> > > 0, so don't think there is any worry of a hanging pointer. To assert that
> > > fDescCount is not 0 we would have to change how we create the
> > > DescriptorPoolManager for samplers so that we don't create one if
fDescCount
> > is
> > > 0. But then I feel like I would need to move a bunch of these if checks
into
> > the
> > > using class to know if the sampler DescriptorPoolManager is valid or not.
> > 
> > Ah, I see. We could have one where fDescCount == 0 in that case. I guess
this
> is
> > okay, though it still makes me uneasy.
> 
> What if at the end of the function we have SkASSERT(fPool || !fTypeCount)

That sounds good.

[egdaniel, 2016-03-08 15:59:55]

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

https://codereview.chromium.org/1765923002/diff/40001/src/gpu/vk/GrVkProgram....
src/gpu/vk/GrVkProgram.cpp:59: fStartDS =
SkTMin(GrVkUniformHandler::kSamplerDescSet,
GrVkUniformHandler::kSamplerDescSet);
On 2016/03/07 20:38:38, jvanverth1 wrote:
> On 2016/03/07 20:02:25, egdaniel wrote:
> > On 2016/03/07 19:04:11, jvanverth1 wrote:
> > > I'm not sure what this is trying to do.
> > 
> > We are always using a VkPipelineLayout that has a slot for two descriptor
> sets.
> > However, if we don't have any samplers I do not actually allocate a
descriptor
> > set for the samplers since it is an error to make a descriptor pool of size
0.
> > Thus when it comes to bind the descriptor sets to the command buffer, I need
> to
> > tell the driver which descriptor sets in the VkPipelineLayout I'm binding
(and
> > can't bind a null descriptor set to the sampler spot). The SkTMin stuff is
> there
> > so we aren't relying on one of the samplers to be in the 0 or 1 position.
> 
> I think maybe you want SkTMin(fStartDS, GrVkUniformHandler::kSamplerDescSet);

Done.

https://codereview.chromium.org/1765923002/diff/40001/src/gpu/vk/GrVkProgram....
src/gpu/vk/GrVkProgram.cpp:389: }
On 2016/03/07 21:30:15, egdaniel wrote:
> On 2016/03/07 20:38:38, jvanverth1 wrote:
> > On 2016/03/07 20:02:25, egdaniel wrote:
> > > On 2016/03/07 19:04:11, jvanverth1 wrote:
> > > > This seems unlikely, but if fPool is non-null and fDescCount is 0, then
> > you'll
> > > > have a dangling pointer. Maybe assert that fDescCount > 0 instead? (the
> > class
> > > > doesn't seem to make sense otherwise)
> > > 
> > > Technically we would have never made an fPool to begin we have an
fDescCount
> > of
> > > 0, so don't think there is any worry of a hanging pointer. To assert that
> > > fDescCount is not 0 we would have to change how we create the
> > > DescriptorPoolManager for samplers so that we don't create one if
fDescCount
> > is
> > > 0. But then I feel like I would need to move a bunch of these if checks
into
> > the
> > > using class to know if the sampler DescriptorPoolManager is valid or not.
> > 
> > Ah, I see. We could have one where fDescCount == 0 in that case. I guess
this
> is
> > okay, though it still makes me uneasy.
> 
> What if at the end of the function we have SkASSERT(fPool || !fTypeCount)

Done.

[egdaniel, 2016-03-08 20:11:43]

additional fix

[jvanverth1, 2016-03-11 14:24:22]

lgtm

https://codereview.chromium.org/1765923002/diff/100001/src/gpu/vk/GrVkProgram...
File src/gpu/vk/GrVkProgram.cpp (right):

https://codereview.chromium.org/1765923002/diff/100001/src/gpu/vk/GrVkProgram...
src/gpu/vk/GrVkProgram.cpp:54: // Currently we are always binding a desriptor
set for uniform buffers.
sp: descriptor

[egdaniel, 2016-03-18 19:55:10]

The CQ bit was checked by egdaniel@google.com

[egdaniel, 2016-03-18 19:55:11]

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/1765923002/#ps140001
(title: "rebase")

[commit-bot: I haz the power, 2016-03-18 19:55:21]

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

[egdaniel, 2016-03-18 19:55:24]

The CQ bit was unchecked by egdaniel@google.com

[egdaniel, 2016-03-18 20:03:18]

The CQ bit was checked by egdaniel@google.com

[commit-bot: I haz the power, 2016-03-18 20:03:29]

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

[commit-bot: I haz the power, 2016-03-18 20:18:26]

Description was changed from

==========
Add DescriptorPool and set manager to GrVkProgram

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

to

==========
Add DescriptorPool and set manager to GrVkProgram

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/+/c2dc1b29ba82e352f174a7b84519e9a5af55347b
==========

[commit-bot: I haz the power, 2016-03-18 20:18:27]

Committed patchset #8 (id:140001) as
https://skia.googlesource.com/skia/+/c2dc1b29ba82e352f174a7b84519e9a5af55347b