Use texture barriers to read directly from the RT

Use texture barriers to read directly from the RT

Updates GrXferProcessor to read directly from the RT texture when
texture barriers are supported and it needs to know the dst color.
Also adds the notion of an Xfer barrier and uses it to issue texture
barriers when the XP will read the RT.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/9954bc38c498f6b9e9d8c0bcc5cd00d45bfc6e23

[Chris Dalton, 2015-03-31 05:40:24]

cdalton@nvidia.com changed reviewers:
+ bsalomon@google.com, egdaniel@google.com, markkilgard@gmail.com

[Chris Dalton, 2015-03-31 05:40:25]

Depends on https://codereview.chromium.org/1035083004/

[egdaniel, 2015-03-31 13:33:03]

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

[egdaniel, 2015-03-31 13:33:04]

adding josh for batching stuff and where Ganesh is heading

[egdaniel, 2015-03-31 14:23:41]

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

https://codereview.chromium.org/1040303002/diff/20001/src/gpu/GrDrawTarget.cp...
src/gpu/GrDrawTarget.cpp:473: if
(!pipelineBuilder->willBlendCoherently(*this->caps())) {
So I feel like somewhere we need to be checking the caps to make sure we support
adding a Barrier. I know there is an assert right before the GL call but I think
we should protect ourselves from adding a barrier into the command buffer if we
don't support it. So maybe here we also check the the caps along with
willBlendCoherently(). If we don't have support and willBlendCohernetly returns
false then we need to figure out what our fall back is. I don't think it is safe
enough to assume willBlendCoherently() will only return false if there is caps
support (unless maybe it is checked in a non virtual version of it but that kind
of changes the meaning of the function call if you check the caps support).

Maybe a better name for willBlendCoherently is
requiresBarrierToBlendCoherently() and then a caller is responsible for making
sure they can insert a Barrier if needed.

[Chris Dalton, 2015-03-31 17:06:42]

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

https://codereview.chromium.org/1040303002/diff/20001/src/gpu/GrDrawTarget.cp...
src/gpu/GrDrawTarget.cpp:473: if
(!pipelineBuilder->willBlendCoherently(*this->caps())) {
On 2015/03/31 14:23:41, egdaniel wrote:
> So I feel like somewhere we need to be checking the caps to make sure we
support
> adding a Barrier. I know there is an assert right before the GL call but I
think
> we should protect ourselves from adding a barrier into the command buffer if
we
> don't support it. So maybe here we also check the the caps along with
> willBlendCoherently(). If we don't have support and willBlendCohernetly
returns
> false then we need to figure out what our fall back is. I don't think it is
safe
> enough to assume willBlendCoherently() will only return false if there is caps
> support (unless maybe it is checked in a non virtual version of it but that
kind
> of changes the meaning of the function call if you check the caps support).
> 
> Maybe a better name for willBlendCoherently is
> requiresBarrierToBlendCoherently() and then a caller is responsible for making
> sure they can insert a Barrier if needed.

That seems fine. What about "requiresBlendBarrier()"? Do you have a strong
preference for the name? We can just bail from onBlendBarrier in the event that
we don't have support. We shouldn't need a fallback because if we don't have
blend barriers, the xpf is obviously mistaken as it couldn't possibly be using a
blend mode that requires a barrier.

Just brainstorming here, but another option could be a virtual method where the
subclass inserts its blend barrier instead returning whether it needs one? That
could potentially be useful if we start having different types of blend barriers
(e.g. glTextureBarrier)...

[joshualitt, 2015-03-31 18:29:37]

Chris, this looks like a good change, however, all of our draws, outside of
text, go through batch.  In the next few weeks I will hopefully convert text
over to GrBatch.  I think you might want to wait until everything goes through
batch, and we can figure out how to handle the device rects.  I don't think it
will be too hard, GrBatch will need to manage the device rects for reordering,
but its probably worth grabbing Brian's input too so we can do this cleanly with
GrBatch.

[bsalomon, 2015-04-01 13:32:26]

Is it possible that we could automatically insert the barriers when flushing,
perhaps in the GPU object, rather than pushing them through the command queue?
We'd still need a hint from the XP that it can't merge overlapping draws to
avoid batching issues.

Is it worth looking at bounding boxes to avoid barriers (when draws can't batch
for other reasons, e.g. different primitive types) or is the barrier cheap
enough?

[egdaniel, 2015-04-01 13:39:22]

On 2015/04/01 13:32:26, bsalomon wrote:
> Is it possible that we could automatically insert the barriers when flushing,
> perhaps in the GPU object, rather than pushing them through the command queue?
> We'd still need a hint from the XP that it can't merge overlapping draws to
> avoid batching issues.
> 
> Is it worth looking at bounding boxes to avoid barriers (when draws can't
batch
> for other reasons, e.g. different primitive types) or is the barrier cheap
> enough?

Another thought I had is when we start doing reordering it will get complicated
if we need to keep knowledge about what draw command a barrier in the command
buffer connects with. It would probably be easiest if the draw that needed the
barrier held the knowledge internally and could correctly return whether or not
it can be batched with given draws.

[bsalomon, 2015-04-01 13:55:57]

On 2015/04/01 13:39:22, egdaniel wrote:
> On 2015/04/01 13:32:26, bsalomon wrote:
> > Is it possible that we could automatically insert the barriers when
flushing,
> > perhaps in the GPU object, rather than pushing them through the command
queue?
> > We'd still need a hint from the XP that it can't merge overlapping draws to
> > avoid batching issues.
> > 
> > Is it worth looking at bounding boxes to avoid barriers (when draws can't
> batch
> > for other reasons, e.g. different primitive types) or is the barrier cheap
> > enough?
> 
> Another thought I had is when we start doing reordering it will get
complicated
> if we need to keep knowledge about what draw command a barrier in the command
> buffer connects with. It would probably be easiest if the draw that needed the
> barrier held the knowledge internally and could correctly return whether or
not
> it can be batched with given draws.

I've had the same thought about dst copies as well. I think the XP should convey
info about copies/barriers that need to be issued before the draw is executed
and also provide input to batchability of draws.

[Mark Kilgard, 2015-04-02 23:02:38]

are my comments useful?

otherwise lgtm

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

https://codereview.chromium.org/1040303002/diff/20001/src/gpu/GrDrawTarget.cp...
src/gpu/GrDrawTarget.cpp:473: if
(!pipelineBuilder->willBlendCoherently(*this->caps())) {
Two cents: I like the shorter requiresBlendBarrier to
requiresBarrierToBlendCoherently.

Chris, you don't want or need to use blend barriers for "conventional"
(non-advanced) blend modes.

Maybe I'll see that the pipelineBuilder has this knowledge...

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

https://codereview.chromium.org/1040303002/diff/20001/src/gpu/GrGpu.h#newcode309
src/gpu/GrGpu.h:309: virtual void blendBarrier() = 0;
seems like you could just provide a no-op implementation {} rather than a base
virtual method

having a no-op default blendBarrier makes sense to me

long term, I don't expect hardware to need the barrier to do anything

https://codereview.chromium.org/1040303002/diff/20001/src/gpu/GrPipelineBuild...
File src/gpu/GrPipelineBuilder.cpp (right):

https://codereview.chromium.org/1040303002/diff/20001/src/gpu/GrPipelineBuild...
src/gpu/GrPipelineBuilder.cpp:128: return
this->getXPFactory()->willBlendCoherently(caps);
this seems a lot of indirection to just return a boolean that hopefully will be
false most of the time

could the knowledge be cached within the GrPipelineBuilder?

https://codereview.chromium.org/1040303002/diff/20001/src/gpu/GrTargetCommand...
File src/gpu/GrTargetCommands.cpp (right):

https://codereview.chromium.org/1040303002/diff/20001/src/gpu/GrTargetCommand...
src/gpu/GrTargetCommands.cpp:110: fCmdBuffer.pop_back();
does something avoid all this checking to eliminate blend barriers if blend
barriers aren't actually necessary?

I'd had to be looking for work to eliminate that doesn't exist

[Chris Dalton, 2015-04-16 10:43:18]

Reworked to make the barriers more generic. Can you guys take a look?

The code to join barriers is here:

https://codereview.chromium.org/1073943004/

[Chris Dalton, 2015-04-17 08:06:06]

Ok, one more major rework :)

I decided to scratch the dst copy stuff. It is too complex for now and needs
more tweaking, since combining dst copies can increase your mem bandwidth and
isn't always a clear win.

Texture barriers, on the other hand, are definitely always a clear win. And I
think you will be pleased with how simple the patch is now.

https://codereview.chromium.org/1040303002/diff/60001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1040303002/diff/60001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:2774: SkASSERT(this->caps()->textureBarrierSupport());
I can update this to an if instead of an assert, to protect from Xfer barriers
that request nonsupported barriers.

[bsalomon, 2015-04-21 15:19:28]

lgtm

https://codereview.chromium.org/1040303002/diff/60001/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1040303002/diff/60001/src/gpu/gl/GrGLGpu.cpp#...
src/gpu/gl/GrGLGpu.cpp:2774: SkASSERT(this->caps()->textureBarrierSupport());
On 2015/04/17 08:06:05, Chris Dalton wrote:
> I can update this to an if instead of an assert, to protect from Xfer barriers
> that request nonsupported barriers.

I think an assert is fine. Technically a client could derive their own XP
classes, and if that was common it might be nice to fail in a friendlier manner,
but it's not. Asserting seems ok.

[Chris Dalton, 2015-04-29 18:42:02]

https://codereview.chromium.org/1040303002/diff/120001/src/gpu/GrTargetComman...
File src/gpu/GrTargetCommands.cpp (right):

https://codereview.chromium.org/1040303002/diff/120001/src/gpu/GrTargetComman...
src/gpu/GrTargetCommands.cpp:422: 
This part of the rebase was nontrivial. I removed the call to this->closeBatch()

[Chris Dalton, 2015-04-29 19:47:49]

The CQ bit was checked by cdalton@nvidia.com

[Chris Dalton, 2015-04-29 19:47:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
markkilgard@gmail.com, bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/1040303002/#ps140001
(title: "Rename setXferBarrier to xferBarrier")

[commit-bot: I haz the power, 2015-04-29 19:48:05]

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

[commit-bot: I haz the power, 2015-04-29 20:01:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-29 20:01:41]

Try jobs failed on following builders:
  Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[Chris Dalton, 2015-04-29 21:13:42]

The CQ bit was checked by cdalton@nvidia.com

[Chris Dalton, 2015-04-29 21:13:42]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com,
markkilgard@gmail.com
Link to the patchset: https://codereview.chromium.org/1040303002/#ps160001
(title: "fix windows build")

[commit-bot: I haz the power, 2015-04-29 21:14:00]

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

[commit-bot: I haz the power, 2015-04-29 21:17:05]

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