Refactor creation of GrVkRenderPasses to make them move general

Refactor creation of GrVkRenderPasses to make them move general

This change is a refactorization to open up more flexable use for render passes
in the future. Once we start bundling draw calls into a single render pass we
will need to start using specific load and store ops instead of the default
currently of load and store everything.

We still will need to simply get a compatible render pass for creation of framebuffers
and pipeline objects. These render passes don't need to know load and store ops. Thus
in this change, I'm defaulting the "compatible" render pass to always be the one
which both loads and stores. All other load/store combinations will be created lazily
when requested for a specific draw (future change).

The CompatibleRPHandle is a way for us to avoid analysing the RenderTarget every time
we want to get a new GrVkRenderPass.

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

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

[egdaniel, 2016-05-16 15:18:53]

Description was changed from

==========
Refactor creation of GrVkRenderPasses to make them move general

This change is a refactorization to open up more flexable use for render passes
in the future. Once we start bundling draw calls into a single render pass we
will need to start using specific load and store ops instead of the default
currently of load and store everything.

We still will need to simply get a compatible render pass for creation of
framebuffers
and pipeline objects. These render passes don't need to know load and store ops.
Thus
in this change, I'm defaulting the "compatible" render pass to always be the one
which both loads and stores. All other load/store combinations will be created
lazily
when requested for a specific draw (future change).

The CompatibleRPHandle is a way for us to avoid analysing the RenderTarget every
time
we want to get a new GrVkRenderPass.

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

to

==========
Refactor creation of GrVkRenderPasses to make them move general

This change is a refactorization to open up more flexable use for render passes
in the future. Once we start bundling draw calls into a single render pass we
will need to start using specific load and store ops instead of the default
currently of load and store everything.

We still will need to simply get a compatible render pass for creation of
framebuffers
and pipeline objects. These render passes don't need to know load and store ops.
Thus
in this change, I'm defaulting the "compatible" render pass to always be the one
which both loads and stores. All other load/store combinations will be created
lazily
when requested for a specific draw (future change).

The CompatibleRPHandle is a way for us to avoid analysing the RenderTarget every
time
we want to get a new GrVkRenderPass.

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

[egdaniel, 2016-05-16 17:03:20]

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

[egdaniel, 2016-05-16 17:03:21]

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

[egdaniel, 2016-05-17 20:00:10]

ping

[jvanverth1, 2016-05-18 20:42:21]

LGTM + nits

https://codereview.chromium.org/1977403002/diff/10007/src/gpu/vk/GrVkRenderPa...
File src/gpu/vk/GrVkRenderPass.cpp (right):

https://codereview.chromium.org/1977403002/diff/10007/src/gpu/vk/GrVkRenderPa...
src/gpu/vk/GrVkRenderPass.cpp:24: if (VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL
== layout) {
I think a switch statement would be clearer here.

https://codereview.chromium.org/1977403002/diff/10007/src/gpu/vk/GrVkResource...
File src/gpu/vk/GrVkResourceProvider.h (right):

https://codereview.chromium.org/1977403002/diff/10007/src/gpu/vk/GrVkResource...
src/gpu/vk/GrVkResourceProvider.h:181: kDCStore_LoadStoreOp,
It would be clearer to write out DontCare or Discard. And maybe kLoadAndStore,
kDiscardAndStore, etc.

[egdaniel, 2016-06-02 17:26:25]

This has been updated from the original review to now use a dynamic array to
store render passes instead of a fixed size array with an enum. The reason being
I wasn't taking into account all the combinations of load/stores for every
attachment (previously the enum/array was just based off of color).

https://codereview.chromium.org/1977403002/diff/10007/src/gpu/vk/GrVkRenderPa...
File src/gpu/vk/GrVkRenderPass.cpp (right):

https://codereview.chromium.org/1977403002/diff/10007/src/gpu/vk/GrVkRenderPa...
src/gpu/vk/GrVkRenderPass.cpp:24: if (VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL
== layout) {
On 2016/05/18 20:42:21, jvanverth1 wrote:
> I think a switch statement would be clearer here.

Done.

https://codereview.chromium.org/1977403002/diff/10007/src/gpu/vk/GrVkResource...
File src/gpu/vk/GrVkResourceProvider.h (right):

https://codereview.chromium.org/1977403002/diff/10007/src/gpu/vk/GrVkResource...
src/gpu/vk/GrVkResourceProvider.h:181: kDCStore_LoadStoreOp,
On 2016/05/18 20:42:21, jvanverth1 wrote:
> It would be clearer to write out DontCare or Discard. And maybe kLoadAndStore,
> kDiscardAndStore, etc.

Removed enum.

[jvanverth1, 2016-06-02 20:09:35]

https://codereview.chromium.org/1977403002/diff/30001/src/gpu/vk/GrVkRenderPa...
File src/gpu/vk/GrVkRenderPass.cpp (right):

https://codereview.chromium.org/1977403002/diff/30001/src/gpu/vk/GrVkRenderPa...
src/gpu/vk/GrVkRenderPass.cpp:19: const AttachmentDesc& desc,
Nit: tabs

https://codereview.chromium.org/1977403002/diff/30001/src/gpu/vk/GrVkResource...
File src/gpu/vk/GrVkResourceProvider.cpp (right):

https://codereview.chromium.org/1977403002/diff/30001/src/gpu/vk/GrVkResource...
src/gpu/vk/GrVkResourceProvider.cpp:287: fRenderPasses[0]->initSimple(gpu,
target);
So at the moment each set only has one renderpass, correct? But eventually we'll
have other ones that differ only in their LoadStore params.

https://codereview.chromium.org/1977403002/diff/30001/src/gpu/vk/GrVkResource...
File src/gpu/vk/GrVkResourceProvider.h (right):

https://codereview.chromium.org/1977403002/diff/30001/src/gpu/vk/GrVkResource...
src/gpu/vk/GrVkResourceProvider.h:172: // The first GrVkRenderpass should always
exists since we create the basic load store
Nit: exist

[egdaniel, 2016-06-02 20:50:26]

https://codereview.chromium.org/1977403002/diff/30001/src/gpu/vk/GrVkRenderPa...
File src/gpu/vk/GrVkRenderPass.cpp (right):

https://codereview.chromium.org/1977403002/diff/30001/src/gpu/vk/GrVkRenderPa...
src/gpu/vk/GrVkRenderPass.cpp:19: const AttachmentDesc& desc,
On 2016/06/02 20:09:35, jvanverth1 wrote:
> Nit: tabs

Done.

https://codereview.chromium.org/1977403002/diff/30001/src/gpu/vk/GrVkResource...
File src/gpu/vk/GrVkResourceProvider.cpp (right):

https://codereview.chromium.org/1977403002/diff/30001/src/gpu/vk/GrVkResource...
src/gpu/vk/GrVkResourceProvider.cpp:287: fRenderPasses[0]->initSimple(gpu,
target);
On 2016/06/02 20:09:35, jvanverth1 wrote:
> So at the moment each set only has one renderpass, correct? But eventually
we'll
> have other ones that differ only in their LoadStore params.

Yes currently it will always be the load and store RP. The general flow of
things will be when starting a new GpuCommandBuffer in GrDrawTarget we send down
the RenderTarget and LoadStoreOps to the creation of the GrVkGpuCommandBuffer.
Here it will ask the ResourceProvider to find a RP which is compatible to the RT
and has the requested LoadStoreOps. This find function is implemented in the
follow on patch I've put up. But basically it will append a new RP to this array
if one with that LoadStoreOp hasn't been created yet.

https://codereview.chromium.org/1977403002/diff/30001/src/gpu/vk/GrVkResource...
File src/gpu/vk/GrVkResourceProvider.h (right):

https://codereview.chromium.org/1977403002/diff/30001/src/gpu/vk/GrVkResource...
src/gpu/vk/GrVkResourceProvider.h:172: // The first GrVkRenderpass should always
exists since we create the basic load store
On 2016/06/02 20:09:35, jvanverth1 wrote:
> Nit: exist

Done.

[jvanverth1, 2016-06-02 21:27:40]

lgtm

[egdaniel, 2016-06-07 15:29:31]

The CQ bit was checked by egdaniel@google.com

[commit-bot: I haz the power, 2016-06-07 15:29:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1977403002/50001

[commit-bot: I haz the power, 2016-06-07 15:43:32]

Description was changed from

==========
Refactor creation of GrVkRenderPasses to make them move general

This change is a refactorization to open up more flexable use for render passes
in the future. Once we start bundling draw calls into a single render pass we
will need to start using specific load and store ops instead of the default
currently of load and store everything.

We still will need to simply get a compatible render pass for creation of
framebuffers
and pipeline objects. These render passes don't need to know load and store ops.
Thus
in this change, I'm defaulting the "compatible" render pass to always be the one
which both loads and stores. All other load/store combinations will be created
lazily
when requested for a specific draw (future change).

The CompatibleRPHandle is a way for us to avoid analysing the RenderTarget every
time
we want to get a new GrVkRenderPass.

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

to

==========
Refactor creation of GrVkRenderPasses to make them move general

This change is a refactorization to open up more flexable use for render passes
in the future. Once we start bundling draw calls into a single render pass we
will need to start using specific load and store ops instead of the default
currently of load and store everything.

We still will need to simply get a compatible render pass for creation of
framebuffers
and pipeline objects. These render passes don't need to know load and store ops.
Thus
in this change, I'm defaulting the "compatible" render pass to always be the one
which both loads and stores. All other load/store combinations will be created
lazily
when requested for a specific draw (future change).

The CompatibleRPHandle is a way for us to avoid analysing the RenderTarget every
time
we want to get a new GrVkRenderPass.

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

[commit-bot: I haz the power, 2016-06-07 15:43:33]

Committed patchset #4 (id:50001) as
https://skia.googlesource.com/skia/+/d62e28b19a23b913c549b7891ecf79e779577181