Create GrVkCopyPipeline class

Create GrVkCopyPipeline class

This creates a special subclass of GrVkPipeline that we can use for doing
copies on draws. Currently this is not hooked into the system.

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

Committed: https://skia.googlesource.com/skia/+/12c7563d7ec4eea4b55b1fb637e63ce3cf3ec384

[egdaniel, 2016-09-19 17:18:51]

Description was changed from

==========
Create GrVkCopyPipeline class

This creates a special subclass of GrVkPipeline that we can use for doing
copies on draws. Currently this is not hooked into the system.

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

to

==========
Create GrVkCopyPipeline class

This creates a special subclass of GrVkPipeline that we can use for doing
copies on draws. Currently this is not hooked into the system.

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

[egdaniel, 2016-09-19 17:29:33]

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

[egdaniel, 2016-09-19 17:29:33]

This is one of the pre-steps to landing copy as draws for vulkan

[jvanverth1, 2016-09-19 18:38:29]

https://codereview.chromium.org/2353563003/diff/20001/src/gpu/vk/GrVkCopyPipe...
File src/gpu/vk/GrVkCopyPipeline.cpp (right):

https://codereview.chromium.org/2353563003/diff/20001/src/gpu/vk/GrVkCopyPipe...
src/gpu/vk/GrVkCopyPipeline.cpp:185: return new GrVkCopyPipeline(vkPipeline,
&renderPass);
This seems a little dangerous. What happens if we create a renderPass locally on
the stack and pass it to Create()? Are these not intended to be long-lived?

[egdaniel, 2016-09-19 18:56:33]

https://codereview.chromium.org/2353563003/diff/20001/src/gpu/vk/GrVkCopyPipe...
File src/gpu/vk/GrVkCopyPipeline.cpp (right):

https://codereview.chromium.org/2353563003/diff/20001/src/gpu/vk/GrVkCopyPipe...
src/gpu/vk/GrVkCopyPipeline.cpp:185: return new GrVkCopyPipeline(vkPipeline,
&renderPass);
On 2016/09/19 18:38:29, jvanverth1 wrote:
> This seems a little dangerous. What happens if we create a renderPass locally
on
> the stack and pass it to Create()? Are these not intended to be long-lived?

So currently all render passes are created by a call to the
GrVkResourceProvider. As long as they are created in this fashion than they will
be long lived objects (or at least live through the life of the GrVkGpu). I
suppose someone could create one manually which will break this system (and many
other things). I could go into GrVkRenderPass and then do a bunch of friending
to hide the ctor to only those objects I want to see it.

Also in the follow up CL, only the resourceProvider actually ever creates the
GrVkCopyPipeline and it creates/sends in a compatible render pass to use here.

So the stored render pass is really only used to check for compatibility when
trying to reuse GrVkCopyPipelines, so another option would be to create a key
based off the render pass. This does have some overhead with it.

[jvanverth1, 2016-09-19 19:05:52]

lgtm

https://codereview.chromium.org/2353563003/diff/20001/src/gpu/vk/GrVkCopyPipe...
File src/gpu/vk/GrVkCopyPipeline.cpp (right):

https://codereview.chromium.org/2353563003/diff/20001/src/gpu/vk/GrVkCopyPipe...
src/gpu/vk/GrVkCopyPipeline.cpp:185: return new GrVkCopyPipeline(vkPipeline,
&renderPass);
On 2016/09/19 18:56:33, egdaniel wrote:
> On 2016/09/19 18:38:29, jvanverth1 wrote:
> > This seems a little dangerous. What happens if we create a renderPass
locally
> on
> > the stack and pass it to Create()? Are these not intended to be long-lived?
> 
> So currently all render passes are created by a call to the
> GrVkResourceProvider. As long as they are created in this fashion than they
will
> be long lived objects (or at least live through the life of the GrVkGpu). I
> suppose someone could create one manually which will break this system (and
many
> other things). I could go into GrVkRenderPass and then do a bunch of friending
> to hide the ctor to only those objects I want to see it.
> 
> Also in the follow up CL, only the resourceProvider actually ever creates the
> GrVkCopyPipeline and it creates/sends in a compatible render pass to use here.
> 
> So the stored render pass is really only used to check for compatibility when
> trying to reuse GrVkCopyPipelines, so another option would be to create a key
> based off the render pass. This does have some overhead with it.

That seems like overkill -- I forgot they were only created in the
ResourceProvider. Maybe just put a comment in the .h for Create() saying that
the renderpass is stored and we expect it's not local.

[egdaniel, 2016-09-19 20:10:29]

https://codereview.chromium.org/2353563003/diff/20001/src/gpu/vk/GrVkCopyPipe...
File src/gpu/vk/GrVkCopyPipeline.cpp (right):

https://codereview.chromium.org/2353563003/diff/20001/src/gpu/vk/GrVkCopyPipe...
src/gpu/vk/GrVkCopyPipeline.cpp:185: return new GrVkCopyPipeline(vkPipeline,
&renderPass);
On 2016/09/19 19:05:52, jvanverth1 wrote:
> On 2016/09/19 18:56:33, egdaniel wrote:
> > On 2016/09/19 18:38:29, jvanverth1 wrote:
> > > This seems a little dangerous. What happens if we create a renderPass
> locally
> > on
> > > the stack and pass it to Create()? Are these not intended to be
long-lived?
> > 
> > So currently all render passes are created by a call to the
> > GrVkResourceProvider. As long as they are created in this fashion than they
> will
> > be long lived objects (or at least live through the life of the GrVkGpu). I
> > suppose someone could create one manually which will break this system (and
> many
> > other things). I could go into GrVkRenderPass and then do a bunch of
friending
> > to hide the ctor to only those objects I want to see it.
> > 
> > Also in the follow up CL, only the resourceProvider actually ever creates
the
> > GrVkCopyPipeline and it creates/sends in a compatible render pass to use
here.
> > 
> > So the stored render pass is really only used to check for compatibility
when
> > trying to reuse GrVkCopyPipelines, so another option would be to create a
key
> > based off the render pass. This does have some overhead with it.
> 
> That seems like overkill -- I forgot they were only created in the
> ResourceProvider. Maybe just put a comment in the .h for Create() saying that
> the renderpass is stored and we expect it's not local.

Done.

[egdaniel, 2016-09-19 20:10:32]

The CQ bit was checked by egdaniel@google.com

[egdaniel, 2016-09-19 20:10:33]

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/2353563003/#ps40001
(title: "add comment about renderpass")

[commit-bot: I haz the power, 2016-09-19 20:10:42]

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 20:39:36]

Description was changed from

==========
Create GrVkCopyPipeline class

This creates a special subclass of GrVkPipeline that we can use for doing
copies on draws. Currently this is not hooked into the system.

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

to

==========
Create GrVkCopyPipeline class

This creates a special subclass of GrVkPipeline that we can use for doing
copies on draws. Currently this is not hooked into the system.

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

Committed:
https://skia.googlesource.com/skia/+/12c7563d7ec4eea4b55b1fb637e63ce3cf3ec384
==========

[commit-bot: I haz the power, 2016-09-19 20:39:36]

Committed patchset #3 (id:40001) as
https://skia.googlesource.com/skia/+/12c7563d7ec4eea4b55b1fb637e63ce3cf3ec384