Start using GrGpuCommandBuffer in GrDrawTarget.

Start using GrGpuCommandBuffer in GrDrawTarget.

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

Committed: https://skia.googlesource.com/skia/+/9cb6340a62a5d748e4189d50e51fa527c8c80c03

[egdaniel, 2016-06-16 20:13:35]

https://codereview.chromium.org/2078483002/diff/1/tests/VkClearTests.cpp
File tests/VkClearTests.cpp (right):

https://codereview.chromium.org/2078483002/diff/1/tests/VkClearTests.cpp#newc...
tests/VkClearTests.cpp:51: #if 0
currently these test don't work since doing clears on VK isn't implemented with
new command buffer system. Even when we add the command buffer support, sub
clears will probably be the only one directly implemented and testable.

https://codereview.chromium.org/2078483002/diff/1/tests/VkUploadPixelsTests.cpp
File tests/VkUploadPixelsTests.cpp (left):

https://codereview.chromium.org/2078483002/diff/1/tests/VkUploadPixelsTests.c...
tests/VkUploadPixelsTests.cpp:55: gpu->discard(NULL);
don't even know why this was here originally since vk discard did nothing

[egdaniel, 2016-06-16 20:23:55]

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

[jvanverth1, 2016-06-17 15:25:16]

Lots o' comments

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (right):

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/GrDrawTarget.cp...
src/gpu/GrDrawTarget.cpp:219: if (fBatches[i]->renderTarget() != currentRT /* ||
commandBuffer*/) {
Remove /* || commandBuffer */?

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/GrGpuCommandBuf...
File src/gpu/GrGpuCommandBuffer.cpp (right):

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/GrGpuCommandBuf...
src/gpu/GrGpuCommandBuffer.cpp:15: this->gpu()->handleDirtyContext();
It'd be less verbose to just say fGpu->

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/GrGpuCommandBuf...
File src/gpu/GrGpuCommandBuffer.h (right):

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/GrGpuCommandBuf...
src/gpu/GrGpuCommandBuffer.h:55: this->onClear(renderTarget, rect, color);
No asserts from GrGpu?

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/GrGpuCommandBuf...
src/gpu/GrGpuCommandBuffer.h:59: this->onClearStencilClip(renderTarget, rect,
insideClip);
No assert?

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/batches/GrCopyS...
File src/gpu/batches/GrCopySurfaceBatch.h (right):

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/batches/GrCopyS...
src/gpu/batches/GrCopySurfaceBatch.h:39: GrRenderTarget* renderTarget() const
override { return nullptr; }
It's not clear to me why this is okay.

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/batches/GrCopyS...
src/gpu/batches/GrCopySurfaceBatch.h:70: // currently we are not sending copies
through the GrGpuCommandBuffer
Does this mean that copies will be performed immediately on Vulkan? Is that
safe?

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/batches/GrStenc...
File src/gpu/batches/GrStencilPathBatch.h (right):

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/batches/GrStenc...
src/gpu/batches/GrStencilPathBatch.h:71:
state->gpu()->pathRendering()->stencilPath(args, fPath.get());
What happens with pathRendering? Does it get inlined with the GpuCommandBuffer
somehow?

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/vk/GrVkGpuComma...
File src/gpu/vk/GrVkGpuCommandBuffer.cpp (right):

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/vk/GrVkGpuComma...
src/gpu/vk/GrVkGpuCommandBuffer.cpp:63: , fRenderTarget(target) {
Why a non-const pointer rather than a const reference?

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/vk/GrVkGpuComma...
src/gpu/vk/GrVkGpuCommandBuffer.cpp:185: // We do this since some clears
currently can come in the must happen outside a render pass
Nit: that must happen

Also: seems like this might be a temporary perf hit, as a clear on a tiled
renderer is practically free.

[egdaniel, 2016-06-17 15:57:35]

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/GrGpuCommandBuf...
File src/gpu/GrGpuCommandBuffer.cpp (right):

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/GrGpuCommandBuf...
src/gpu/GrGpuCommandBuffer.cpp:15: this->gpu()->handleDirtyContext();
On 2016/06/17 15:25:15, jvanverth1 wrote:
> It'd be less verbose to just say fGpu->

The base class doesn't have a fGpu. I could pass the gpu up from the sub class,
but then when I want to call a backend specific function in the subclasses I
need to cast the gpu to the backend version. Alternatively I could store
pointers the gpu object both here and the subclass.

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/GrGpuCommandBuf...
File src/gpu/GrGpuCommandBuffer.h (right):

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/GrGpuCommandBuf...
src/gpu/GrGpuCommandBuffer.h:55: this->onClear(renderTarget, rect, color);
On 2016/06/17 15:25:15, jvanverth1 wrote:
> No asserts from GrGpu?

What assert are you referring to here? GrGpu is not involved in clear calls
anymore. GrGLGpu happens to have a clear command but that is only called from
GrGLGpuCommandBuffer.

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/batches/GrCopyS...
File src/gpu/batches/GrCopySurfaceBatch.h (right):

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/batches/GrCopyS...
src/gpu/batches/GrCopySurfaceBatch.h:39: GrRenderTarget* renderTarget() const
override { return nullptr; }
On 2016/06/17 15:25:16, jvanverth1 wrote:
> It's not clear to me why this is okay.

Before my change, this call was only called by drawBatches for drawing the
bounding rectangle around a batch. So changing this has no side effect (outside
of not drawing bounds around a copy which we can live with). I then use this
function as a trigger of whether to use a command buffer or not in the
individual batch draw calls.

This really needs Robs MDB changes to clean it up nicely since he has special
handling of copies which I believe will not be part of the normal drawTargets.

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/batches/GrCopyS...
src/gpu/batches/GrCopySurfaceBatch.h:70: // currently we are not sending copies
through the GrGpuCommandBuffer
On 2016/06/17 15:25:16, jvanverth1 wrote:
> Does this mean that copies will be performed immediately on Vulkan? Is that
> safe?

Yes it is safe. Since the renderTarget above returns nullptr, it will trigger us
to end the current command buffer we are writing to and submit it. Thus this
gets inlined in the correct place.

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/batches/GrStenc...
File src/gpu/batches/GrStencilPathBatch.h (right):

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/batches/GrStenc...
src/gpu/batches/GrStencilPathBatch.h:71:
state->gpu()->pathRendering()->stencilPath(args, fPath.get());
On 2016/06/17 15:25:16, jvanverth1 wrote:
> What happens with pathRendering? Does it get inlined with the GpuCommandBuffer
> somehow?

Well in Vulkan we never get Path Batches. In GL the command buffers just
immediately forward the calls to the gpu anyways. So I figured this was easier
than having command buffer need to be able to return a PathRendering instance to
send this calls to.

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/vk/GrVkGpuComma...
File src/gpu/vk/GrVkGpuCommandBuffer.cpp (right):

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/vk/GrVkGpuComma...
src/gpu/vk/GrVkGpuCommandBuffer.cpp:63: , fRenderTarget(target) {
On 2016/06/17 15:25:16, jvanverth1 wrote:
> Why a non-const pointer rather than a const reference?

setImageLayout call on the RT is non const

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/vk/GrVkGpuComma...
src/gpu/vk/GrVkGpuCommandBuffer.cpp:185: // We do this since some clears
currently can come in the must happen outside a render pass
On 2016/06/17 15:25:16, jvanverth1 wrote:
> Nit: that must happen
> 
> Also: seems like this might be a temporary perf hit, as a clear on a tiled
> renderer is practically free.

Yes I believe this is a perf hit (though it hopefully will at least be grouped
up in a render pass with other draws to the same RT). Once we get clears set up
on the load/stores I expect to see a perf gain.

[jvanverth1, 2016-06-17 16:49:13]

Looks fine to me, but unless you're really in a hurry I'd feel better if
bsalomon@ looked at it as well.

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/GrGpuCommandBuf...
File src/gpu/GrGpuCommandBuffer.cpp (right):

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/GrGpuCommandBuf...
src/gpu/GrGpuCommandBuffer.cpp:15: this->gpu()->handleDirtyContext();
On 2016/06/17 15:57:35, egdaniel wrote:
> On 2016/06/17 15:25:15, jvanverth1 wrote:
> > It'd be less verbose to just say fGpu->
> 
> The base class doesn't have a fGpu. I could pass the gpu up from the sub
class,
> but then when I want to call a backend specific function in the subclasses I
> need to cast the gpu to the backend version. Alternatively I could store
> pointers the gpu object both here and the subclass.

It's fine the way it is.

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/GrGpuCommandBuf...
File src/gpu/GrGpuCommandBuffer.h (right):

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/GrGpuCommandBuf...
src/gpu/GrGpuCommandBuffer.h:55: this->onClear(renderTarget, rect, color);
On 2016/06/17 15:57:35, egdaniel wrote:
> On 2016/06/17 15:25:15, jvanverth1 wrote:
> > No asserts from GrGpu?
> 
> What assert are you referring to here? GrGpu is not involved in clear calls
> anymore. GrGLGpu happens to have a clear command but that is only called from
> GrGLGpuCommandBuffer.

In the old GrGpu::clear(), we had

SkASSERT(renderTarget);	
SkASSERT(SkIRect::MakeWH(renderTarget->width(),
renderTarget->height()).contains(rect));

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/batches/GrCopyS...
File src/gpu/batches/GrCopySurfaceBatch.h (right):

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/batches/GrCopyS...
src/gpu/batches/GrCopySurfaceBatch.h:39: GrRenderTarget* renderTarget() const
override { return nullptr; }
On 2016/06/17 15:57:35, egdaniel wrote:
> On 2016/06/17 15:25:16, jvanverth1 wrote:
> > It's not clear to me why this is okay.
> 
> Before my change, this call was only called by drawBatches for drawing the
> bounding rectangle around a batch. So changing this has no side effect
(outside
> of not drawing bounds around a copy which we can live with). I then use this
> function as a trigger of whether to use a command buffer or not in the
> individual batch draw calls.
> 
> This really needs Robs MDB changes to clean it up nicely since he has special
> handling of copies which I believe will not be part of the normal drawTargets.


Just so I understand: so if renderTarget() returns nullptr you don't use a
command buffer, right?

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/batches/GrCopyS...
src/gpu/batches/GrCopySurfaceBatch.h:70: // currently we are not sending copies
through the GrGpuCommandBuffer
On 2016/06/17 15:57:35, egdaniel wrote:
> On 2016/06/17 15:25:16, jvanverth1 wrote:
> > Does this mean that copies will be performed immediately on Vulkan? Is that
> > safe?
> 
> Yes it is safe. Since the renderTarget above returns nullptr, it will trigger
us
> to end the current command buffer we are writing to and submit it. Thus this
> gets inlined in the correct place.

Acknowledged.

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/batches/GrStenc...
File src/gpu/batches/GrStencilPathBatch.h (right):

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/batches/GrStenc...
src/gpu/batches/GrStencilPathBatch.h:71:
state->gpu()->pathRendering()->stencilPath(args, fPath.get());
On 2016/06/17 15:57:35, egdaniel wrote:
> On 2016/06/17 15:25:16, jvanverth1 wrote:
> > What happens with pathRendering? Does it get inlined with the
GpuCommandBuffer
> > somehow?
> 
> Well in Vulkan we never get Path Batches. In GL the command buffers just
> immediately forward the calls to the gpu anyways. So I figured this was easier
> than having command buffer need to be able to return a PathRendering instance
to
> send this calls to.

Acknowledged.

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/vk/GrVkGpuComma...
File src/gpu/vk/GrVkGpuCommandBuffer.cpp (right):

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/vk/GrVkGpuComma...
src/gpu/vk/GrVkGpuCommandBuffer.cpp:63: , fRenderTarget(target) {
On 2016/06/17 15:57:35, egdaniel wrote:
> On 2016/06/17 15:25:16, jvanverth1 wrote:
> > Why a non-const pointer rather than a const reference?
> 
> setImageLayout call on the RT is non const

Acknowledged.

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/vk/GrVkGpuComma...
src/gpu/vk/GrVkGpuCommandBuffer.cpp:185: // We do this since some clears
currently can come in the must happen outside a render pass
On 2016/06/17 15:57:35, egdaniel wrote:
> On 2016/06/17 15:25:16, jvanverth1 wrote:
> > Nit: that must happen
> > 
> > Also: seems like this might be a temporary perf hit, as a clear on a tiled
> > renderer is practically free.
> 
> Yes I believe this is a perf hit (though it hopefully will at least be grouped
> up in a render pass with other draws to the same RT). Once we get clears set
up
> on the load/stores I expect to see a perf gain.

Acknowledged.

[egdaniel, 2016-06-17 17:06:49]

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/batches/GrCopyS...
File src/gpu/batches/GrCopySurfaceBatch.h (right):

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/batches/GrCopyS...
src/gpu/batches/GrCopySurfaceBatch.h:39: GrRenderTarget* renderTarget() const
override { return nullptr; }
On 2016/06/17 16:49:13, jvanverth1 wrote:
> On 2016/06/17 15:57:35, egdaniel wrote:
> > On 2016/06/17 15:25:16, jvanverth1 wrote:
> > > It's not clear to me why this is okay.
> > 
> > Before my change, this call was only called by drawBatches for drawing the
> > bounding rectangle around a batch. So changing this has no side effect
> (outside
> > of not drawing bounds around a copy which we can live with). I then use this
> > function as a trigger of whether to use a command buffer or not in the
> > individual batch draw calls.
> > 
> > This really needs Robs MDB changes to clean it up nicely since he has
special
> > handling of copies which I believe will not be part of the normal
drawTargets.
> 
> 
> Just so I understand: so if renderTarget() returns nullptr you don't use a
> command buffer, right?

Yes

[egdaniel, 2016-06-17 18:49:14]

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (right):

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/GrDrawTarget.cp...
src/gpu/GrDrawTarget.cpp:219: if (fBatches[i]->renderTarget() != currentRT /* ||
commandBuffer*/) {
On 2016/06/17 15:25:15, jvanverth1 wrote:
> Remove /* || commandBuffer */?

Done.

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/GrGpuCommandBuf...
File src/gpu/GrGpuCommandBuffer.h (right):

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/GrGpuCommandBuf...
src/gpu/GrGpuCommandBuffer.h:55: this->onClear(renderTarget, rect, color);
On 2016/06/17 16:49:13, jvanverth1 wrote:
> On 2016/06/17 15:57:35, egdaniel wrote:
> > On 2016/06/17 15:25:15, jvanverth1 wrote:
> > > No asserts from GrGpu?
> > 
> > What assert are you referring to here? GrGpu is not involved in clear calls
> > anymore. GrGLGpu happens to have a clear command but that is only called
from
> > GrGLGpuCommandBuffer.
> 
> In the old GrGpu::clear(), we had
> 
> SkASSERT(renderTarget);	
> SkASSERT(SkIRect::MakeWH(renderTarget->width(),
> renderTarget->height()).contains(rect));

Done.

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/GrGpuCommandBuf...
src/gpu/GrGpuCommandBuffer.h:59: this->onClearStencilClip(renderTarget, rect,
insideClip);
On 2016/06/17 15:25:15, jvanverth1 wrote:
> No assert?

Done.

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/vk/GrVkGpuComma...
File src/gpu/vk/GrVkGpuCommandBuffer.cpp (right):

https://codereview.chromium.org/2078483002/diff/20001/src/gpu/vk/GrVkGpuComma...
src/gpu/vk/GrVkGpuCommandBuffer.cpp:185: // We do this since some clears
currently can come in the must happen outside a render pass
On 2016/06/17 16:49:13, jvanverth1 wrote:
> On 2016/06/17 15:57:35, egdaniel wrote:
> > On 2016/06/17 15:25:16, jvanverth1 wrote:
> > > Nit: that must happen
> > > 
> > > Also: seems like this might be a temporary perf hit, as a clear on a tiled
> > > renderer is practically free.
> > 
> > Yes I believe this is a perf hit (though it hopefully will at least be
grouped
> > up in a render pass with other draws to the same RT). Once we get clears set
> up
> > on the load/stores I expect to see a perf gain.
> 
> Acknowledged.

Done.

[bsalomon, 2016-06-20 16:51:14]

https://codereview.chromium.org/2078483002/diff/40001/src/gpu/GrGpu.h
File src/gpu/GrGpu.h (right):

https://codereview.chromium.org/2078483002/diff/40001/src/gpu/GrGpu.h#newcode581
src/gpu/GrGpu.h:581: friend class GrGpuCommandBuffer;
why does this need to be a friend?

https://codereview.chromium.org/2078483002/diff/40001/src/gpu/GrGpuCommandBuf...
File src/gpu/GrGpuCommandBuffer.h (right):

https://codereview.chromium.org/2078483002/diff/40001/src/gpu/GrGpuCommandBuf...
src/gpu/GrGpuCommandBuffer.h:61: // TODO: This should be removed in the future
to favor using the load and store ops for discard
Go ahead and remove this whenever you feel like it. We aren't really using
discard in a useful way currently.

https://codereview.chromium.org/2078483002/diff/40001/src/gpu/batches/GrBatch.h
File src/gpu/batches/GrBatch.h (right):

https://codereview.chromium.org/2078483002/diff/40001/src/gpu/batches/GrBatch...
src/gpu/batches/GrBatch.h:114: void draw(GrBatchFlushState* state,
GrGpuCommandBuffer* buffer) { this->onDraw(state, buffer); }
Could buffer be on the state?

https://codereview.chromium.org/2078483002/diff/40001/src/gpu/batches/GrCopyS...
File src/gpu/batches/GrCopySurfaceBatch.h (right):

https://codereview.chromium.org/2078483002/diff/40001/src/gpu/batches/GrCopyS...
src/gpu/batches/GrCopySurfaceBatch.h:70: // currently we are not sending copies
through the GrGpuCommandBuffer
Does this break anything with GL?

https://codereview.chromium.org/2078483002/diff/40001/src/gpu/gl/GrGLGpu.h
File src/gpu/gl/GrGLGpu.h (right):

https://codereview.chromium.org/2078483002/diff/40001/src/gpu/gl/GrGLGpu.h#ne...
src/gpu/gl/GrGLGpu.h:98: void draw(const GrPipeline&,
Maybe a comment on these that they are the passthrough functions that the faux
GL command buffer uses as its implementation?

https://codereview.chromium.org/2078483002/diff/40001/src/gpu/gl/GrGLGpuComma...
File src/gpu/gl/GrGLGpuCommandBuffer.h (right):

https://codereview.chromium.org/2078483002/diff/40001/src/gpu/gl/GrGLGpuComma...
src/gpu/gl/GrGLGpuCommandBuffer.h:15: class GrGLGpuCommandBuffer : public
GrGpuCommandBuffer {
Comment that this is a passthrough back to the GL gpu object?

[egdaniel, 2016-06-22 15:26:57]

Fixed comments in review.

I've also added support clears in the vulkan command buffer. If the first
command we see is a full clear, we will change the render pass to use a clear
load op. Otherwise we always use the partial clear path since we are inside of a
render pass. The check for the first command being a clear will hopefully be
bubbled up to the drawTarget level once MDB in.

The clear change is needed, because when doing clears as draws, viewer gets
really messed up since it is always try to load old data sitting in the
swapchain causing lots of errors.

https://codereview.chromium.org/2078483002/diff/40001/src/gpu/GrGpu.h
File src/gpu/GrGpu.h (right):

https://codereview.chromium.org/2078483002/diff/40001/src/gpu/GrGpu.h#newcode581
src/gpu/GrGpu.h:581: friend class GrGpuCommandBuffer;
On 2016/06/20 16:51:14, bsalomon wrote:
> why does this need to be a friend?

it is for the handleDirtyContext call on GrGpuCommandBuffer::submit

https://codereview.chromium.org/2078483002/diff/40001/src/gpu/GrGpuCommandBuf...
File src/gpu/GrGpuCommandBuffer.h (right):

https://codereview.chromium.org/2078483002/diff/40001/src/gpu/GrGpuCommandBuf...
src/gpu/GrGpuCommandBuffer.h:61: // TODO: This should be removed in the future
to favor using the load and store ops for discard
On 2016/06/20 16:51:14, bsalomon wrote:
> Go ahead and remove this whenever you feel like it. We aren't really using
> discard in a useful way currently.

Ill do this in a follow up.

https://codereview.chromium.org/2078483002/diff/40001/src/gpu/batches/GrBatch.h
File src/gpu/batches/GrBatch.h (right):

https://codereview.chromium.org/2078483002/diff/40001/src/gpu/batches/GrBatch...
src/gpu/batches/GrBatch.h:114: void draw(GrBatchFlushState* state,
GrGpuCommandBuffer* buffer) { this->onDraw(state, buffer); }
On 2016/06/20 16:51:14, bsalomon wrote:
> Could buffer be on the state?

Done.

https://codereview.chromium.org/2078483002/diff/40001/src/gpu/batches/GrCopyS...
File src/gpu/batches/GrCopySurfaceBatch.h (right):

https://codereview.chromium.org/2078483002/diff/40001/src/gpu/batches/GrCopyS...
src/gpu/batches/GrCopySurfaceBatch.h:70: // currently we are not sending copies
through the GrGpuCommandBuffer
On 2016/06/20 16:51:14, bsalomon wrote:
> Does this break anything with GL?

it does not. All copySurfaceBatches return a nullptr for render target. Before
this was only used to draw the debug batch bounds if the RT existed which really
isn't that interesting with copies anyways.

In GrDrawTarget, if we don't have a render target then we don't create a command
buffer. This means we get here and always call the copy directly on the gpu. See
comments in previous patch to why this is also safe for vulkan

https://codereview.chromium.org/2078483002/diff/40001/src/gpu/gl/GrGLGpu.h
File src/gpu/gl/GrGLGpu.h (right):

https://codereview.chromium.org/2078483002/diff/40001/src/gpu/gl/GrGLGpu.h#ne...
src/gpu/gl/GrGLGpu.h:98: void draw(const GrPipeline&,
On 2016/06/20 16:51:14, bsalomon wrote:
> Maybe a comment on these that they are the passthrough functions that the faux
> GL command buffer uses as its implementation?

Done.

https://codereview.chromium.org/2078483002/diff/40001/src/gpu/gl/GrGLGpuComma...
File src/gpu/gl/GrGLGpuCommandBuffer.h (right):

https://codereview.chromium.org/2078483002/diff/40001/src/gpu/gl/GrGLGpuComma...
src/gpu/gl/GrGLGpuCommandBuffer.h:15: class GrGLGpuCommandBuffer : public
GrGpuCommandBuffer {
On 2016/06/20 16:51:14, bsalomon wrote:
> Comment that this is a passthrough back to the GL gpu object?

Done.

[bsalomon, 2016-06-22 15:41:15]

https://codereview.chromium.org/2078483002/diff/40001/src/gpu/GrGpu.h
File src/gpu/GrGpu.h (right):

https://codereview.chromium.org/2078483002/diff/40001/src/gpu/GrGpu.h#newcode581
src/gpu/GrGpu.h:581: friend class GrGpuCommandBuffer;
On 2016/06/22 15:26:57, egdaniel wrote:
> On 2016/06/20 16:51:14, bsalomon wrote:
> > why does this need to be a friend?
> 
> it is for the handleDirtyContext call on GrGpuCommandBuffer::submit

I'd just make handleDirtyContext() public.

Should the dirty context stuff be pushed down to the GL layer?

https://codereview.chromium.org/2078483002/diff/40001/src/gpu/batches/GrCopyS...
File src/gpu/batches/GrCopySurfaceBatch.h (right):

https://codereview.chromium.org/2078483002/diff/40001/src/gpu/batches/GrCopyS...
src/gpu/batches/GrCopySurfaceBatch.h:70: // currently we are not sending copies
through the GrGpuCommandBuffer
On 2016/06/22 15:26:57, egdaniel wrote:
> On 2016/06/20 16:51:14, bsalomon wrote:
> > Does this break anything with GL?
> 
> it does not. All copySurfaceBatches return a nullptr for render target. Before
> this was only used to draw the debug batch bounds if the RT existed which
really
> isn't that interesting with copies anyways.
> 
> In GrDrawTarget, if we don't have a render target then we don't create a
command
> buffer. This means we get here and always call the copy directly on the gpu.
See
> comments in previous patch to why this is also safe for vulkan

This feels a little gnarly. Is there a different long term plan here? Do we
expect post-MDB that we will always have a command buffer?

https://codereview.chromium.org/2078483002/diff/80001/src/gpu/GrGpuCommandBuf...
File src/gpu/GrGpuCommandBuffer.h (right):

https://codereview.chromium.org/2078483002/diff/80001/src/gpu/GrGpuCommandBuf...
src/gpu/GrGpuCommandBuffer.h:20: class GrGpuCommandBuffer {
I think we need a good description of exactly what this is. It seems like it is
a series of draw, clear, discard commands all targeting the same render target.
It may or may not execute immediately. And maybe something about how it relates
to GrBatchs?

https://codereview.chromium.org/2078483002/diff/80001/src/gpu/GrGpuCommandBuf...
src/gpu/GrGpuCommandBuffer.h:22: enum LoadAndStoreOp {
Why make this enum a cross product rather than two separate enums?

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

https://codereview.chromium.org/2078483002/diff/80001/src/gpu/vk/GrVkGpu.cpp#...
src/gpu/vk/GrVkGpu.cpp:140: // and we can't allocate more. Until we get a fix
moving this to SingleAlloc.
I think this is from your other CL

[egdaniel, 2016-06-22 21:13:34]

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

[egdaniel, 2016-06-22 21:13:36]

Updated. Also note that I will move the NVPR stuff onto command buffer in a
future patch.

Adding Rob so he is aware of this change with his stuff.

https://codereview.chromium.org/2078483002/diff/40001/src/gpu/GrGpu.h
File src/gpu/GrGpu.h (right):

https://codereview.chromium.org/2078483002/diff/40001/src/gpu/GrGpu.h#newcode581
src/gpu/GrGpu.h:581: friend class GrGpuCommandBuffer;
On 2016/06/22 15:41:15, bsalomon wrote:
> On 2016/06/22 15:26:57, egdaniel wrote:
> > On 2016/06/20 16:51:14, bsalomon wrote:
> > > why does this need to be a friend?
> > 
> > it is for the handleDirtyContext call on GrGpuCommandBuffer::submit
> 
> I'd just make handleDirtyContext() public.
> 
> Should the dirty context stuff be pushed down to the GL layer?

Made public. Previously we decided not to move it all to GL since we may add
another backend that also will use it.

https://codereview.chromium.org/2078483002/diff/40001/src/gpu/batches/GrCopyS...
File src/gpu/batches/GrCopySurfaceBatch.h (right):

https://codereview.chromium.org/2078483002/diff/40001/src/gpu/batches/GrCopyS...
src/gpu/batches/GrCopySurfaceBatch.h:70: // currently we are not sending copies
through the GrGpuCommandBuffer
On 2016/06/22 15:41:15, bsalomon wrote:
> On 2016/06/22 15:26:57, egdaniel wrote:
> > On 2016/06/20 16:51:14, bsalomon wrote:
> > > Does this break anything with GL?
> > 
> > it does not. All copySurfaceBatches return a nullptr for render target.
Before
> > this was only used to draw the debug batch bounds if the RT existed which
> really
> > isn't that interesting with copies anyways.
> > 
> > In GrDrawTarget, if we don't have a render target then we don't create a
> command
> > buffer. This means we get here and always call the copy directly on the gpu.
> See
> > comments in previous patch to why this is also safe for vulkan
> 
> This feels a little gnarly. Is there a different long term plan here? Do we
> expect post-MDB that we will always have a command buffer?

As per discussion, copies should be moved away from Batches and thus not go
through command buffers.

https://codereview.chromium.org/2078483002/diff/80001/src/gpu/GrGpuCommandBuf...
File src/gpu/GrGpuCommandBuffer.h (right):

https://codereview.chromium.org/2078483002/diff/80001/src/gpu/GrGpuCommandBuf...
src/gpu/GrGpuCommandBuffer.h:22: enum LoadAndStoreOp {
On 2016/06/22 15:41:15, bsalomon wrote:
> Why make this enum a cross product rather than two separate enums?

Done.

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

https://codereview.chromium.org/2078483002/diff/80001/src/gpu/vk/GrVkGpu.cpp#...
src/gpu/vk/GrVkGpu.cpp:140: // and we can't allocate more. Until we get a fix
moving this to SingleAlloc.
On 2016/06/22 15:41:15, bsalomon wrote:
> I think this is from your other CL

Yes it is only showing up here cause it is rebased since the change has landed.
Code review is just showing this as a diff since previous patch, if you don't
use the diff view it shouldn't show up.

[bsalomon, 2016-06-23 02:12:55]

lgtm w/ some inline comments that you can ignore if you like.

https://codereview.chromium.org/2078483002/diff/100001/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (right):

https://codereview.chromium.org/2078483002/diff/100001/src/gpu/GrDrawTarget.c...
src/gpu/GrDrawTarget.cpp:230: static const GrGpuCommandBuffer::LoadAndStoreInfo
kBasicLoadStoreInfo =
A slightly more terse version of this would be:

static const GrGpuCommandBuffer::LoadAndStoreInfo kBasicLoadStoreInfo
    { GrGpuCommandBuffer::kLoad_LoadOp, GrGpuCommandBuffer::kStore_StoreOp };

https://codereview.chromium.org/2078483002/diff/100001/src/gpu/GrGpuCommandBu...
File src/gpu/GrGpuCommandBuffer.h (right):

https://codereview.chromium.org/2078483002/diff/100001/src/gpu/GrGpuCommandBu...
src/gpu/GrGpuCommandBuffer.h:23: * buffered up for later execution (Vulkan).
Currently GrBatches will execute their draw commands
Why "Currently"? Is this likely to change?

https://codereview.chromium.org/2078483002/diff/100001/src/gpu/GrGpuCommandBu...
src/gpu/GrGpuCommandBuffer.h:28: enum LoadOp {
enum classes? No strong preference but it is the future.

https://codereview.chromium.org/2078483002/diff/100001/src/gpu/GrGpuCommandBu...
src/gpu/GrGpuCommandBuffer.h:44: LoadAndStoreInfo(LoadOp loadOp, StoreOp
storeOp, GrColor color = GrColor_ILLEGAL)
It seems like you have this constructor in order to check for consistency
between fLoadOp and fClearColor, but with public members it's a pretty weak
guard. I'd advocate for either going whole hog with setters and accessors or
just making it a struct with no methods (or maybe a default cons). Personally,
I'd go with the latter. It seems any bug where fLoadOp was set to kClear without
specifying the color would be caught by a memory checker or a test.

[egdaniel, 2016-06-23 13:57:08]

https://codereview.chromium.org/2078483002/diff/100001/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (right):

https://codereview.chromium.org/2078483002/diff/100001/src/gpu/GrDrawTarget.c...
src/gpu/GrDrawTarget.cpp:230: static const GrGpuCommandBuffer::LoadAndStoreInfo
kBasicLoadStoreInfo =
On 2016/06/23 02:12:55, bsalomon wrote:
> A slightly more terse version of this would be:
> 
> static const GrGpuCommandBuffer::LoadAndStoreInfo kBasicLoadStoreInfo
>     { GrGpuCommandBuffer::kLoad_LoadOp, GrGpuCommandBuffer::kStore_StoreOp };
> 

Done.

https://codereview.chromium.org/2078483002/diff/100001/src/gpu/GrGpuCommandBu...
File src/gpu/GrGpuCommandBuffer.h (right):

https://codereview.chromium.org/2078483002/diff/100001/src/gpu/GrGpuCommandBu...
src/gpu/GrGpuCommandBuffer.h:23: * buffered up for later execution (Vulkan).
Currently GrBatches will execute their draw commands
On 2016/06/23 02:12:55, bsalomon wrote:
> Why "Currently"? Is this likely to change?

Removed. I put currently at first, cause technically this class doesn't rely on
anything specific from Batch. Thus it gives the idea that the upstack calles of
this are free to send in commands however they like.

https://codereview.chromium.org/2078483002/diff/100001/src/gpu/GrGpuCommandBu...
src/gpu/GrGpuCommandBuffer.h:28: enum LoadOp {
On 2016/06/23 02:12:55, bsalomon wrote:
> enum classes? No strong preference but it is the future.

Done.

https://codereview.chromium.org/2078483002/diff/100001/src/gpu/GrGpuCommandBu...
src/gpu/GrGpuCommandBuffer.h:44: LoadAndStoreInfo(LoadOp loadOp, StoreOp
storeOp, GrColor color = GrColor_ILLEGAL)
On 2016/06/23 02:12:55, bsalomon wrote:
> It seems like you have this constructor in order to check for consistency
> between fLoadOp and fClearColor, but with public members it's a pretty weak
> guard. I'd advocate for either going whole hog with setters and accessors or
> just making it a struct with no methods (or maybe a default cons). Personally,
> I'd go with the latter. It seems any bug where fLoadOp was set to kClear
without
> specifying the color would be caught by a memory checker or a test.

when with pure struct.

[egdaniel, 2016-06-23 15:11:02]

The CQ bit was checked by egdaniel@google.com

[egdaniel, 2016-06-23 15:11:03]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/2078483002/#ps140001
(title: "nits")

[commit-bot: I haz the power, 2016-06-23 15:11:13]

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

[egdaniel, 2016-06-23 15:11:28]

The CQ bit was unchecked by egdaniel@google.com

[egdaniel, 2016-06-23 15:12:46]

The CQ bit was checked by egdaniel@google.com

[egdaniel, 2016-06-23 15:12:46]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/2078483002/#ps160001
(title: "remove errant lines")

[commit-bot: I haz the power, 2016-06-23 15:12:53]

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

[commit-bot: I haz the power, 2016-06-23 15:37:11]

Description was changed from

==========
Start using GrGpuCommandBuffer in GrDrawTarget.

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

to

==========
Start using GrGpuCommandBuffer in GrDrawTarget.

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

Committed:
https://skia.googlesource.com/skia/+/9cb6340a62a5d748e4189d50e51fa527c8c80c03
==========

[commit-bot: I haz the power, 2016-06-23 15:37:12]

Committed patchset #9 (id:160001) as
https://skia.googlesource.com/skia/+/9cb6340a62a5d748e4189d50e51fa527c8c80c03