Make stencil buffers uncached for uncached render target textures

Make stencil buffers uncached for uncached render target textures

Make new stencil buffers of uncached render target textures not affect the
cache budgets. This is consistent with render buffer storage of uncached
render target textures.

Affects only newly created stencil buffers. An uncached render target
might still receive a cached stencil buffer if such is available from
cache.

BUG=skia:3119
BUG=skia:3301
Committed: https://skia.googlesource.com/skia/+/36c57dfb4fe9bbaca436942d5eaa75b142ba251d

[Kimmo Kinnunen, 2015-01-20 12:25:08]

kkinnunen@nvidia.com changed reviewers:
+ bsalomon@google.com

[Kimmo Kinnunen, 2015-01-20 12:25:09]

Was this intentionally left out from https://codereview.chromium.org/846303002 ?

[reed1, 2015-01-20 13:54:35]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2015-01-20 13:54:36]

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

https://codereview.chromium.org/859013002/diff/1/src/gpu/GrGpu.h#newcode481
src/gpu/GrGpu.h:481: virtual bool
createStencilBufferForRenderTarget(GrRenderTarget*, bool budgeted,
Why not keep the enum that was created for this, rather than just a bool (which
is slightly harder to understand at a call-site)?

[Kimmo Kinnunen, 2015-01-20 14:05:48]

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

https://codereview.chromium.org/859013002/diff/1/src/gpu/GrGpu.h#newcode481
src/gpu/GrGpu.h:481: virtual bool
createStencilBufferForRenderTarget(GrRenderTarget*, bool budgeted,
On 2015/01/20 13:54:36, reed1 wrote:
> Why not keep the enum that was created for this, rather than just a bool
(which
> is slightly harder to understand at a call-site)?

So it's consistent with the call that does the same thing with the texture.

[bsalomon, 2015-01-20 17:20:50]

This does disable the sharing of sb's across rts for uncached rts, right?

How does this come up for skbug 3119? Are the uncached RTs the tiles? Won't it
only be one SB shared by all the tiles?

[Kimmo Kinnunen, 2015-01-21 10:48:27]

On 2015/01/20 17:20:50, bsalomon wrote:
> This does disable the sharing of sb's across rts for uncached rts, right?

Yes, in the sense that there's the check "if (kCached_LifeCycle == lifeCycle)",
the same one as with textures.

The intention is to affect the cache budgeting, so would it be more correct to
use ... != kWrapped_LifeCycle ?

I'm confused about expectation of sharing SBs across RTs -- maybe I don't
understand the code enough.

If you have time to specify how it is supposed to work, I can try to debug why
it doesn't work with current Skia and fix it.. E.g if you have 16 canvases, all
which can receive commands at any given moment, how is the stencil buffer
supposed to jump between them?

I understood that it is the intention that if there's a ref to the SB, the cache
will not give the SB out for other render target to use. The render targets will
not release the ref, since they're canvas surfaces and do not go out of scope
until the canvas is destroyed.

> How does this come up for skbug 3119?

The 16 256x256x1bitx16 SBs for the SKPBench tile surfaces consume 16% of the
cache.
The 1 1000x1000x1bitx16 SB for the SKPBench result surface consumes 16% of the
cache.

> Are the uncached RTs the tiles?

Yes. SKPBench tiles are uncached texture RTs.

> Won't it only be one SB shared by all the tiles?

At the moment, as far as my debugging / code reading indicates, no.

[bsalomon, 2015-01-21 14:14:32]

On 2015/01/21 10:48:27, Kimmo Kinnunen wrote:
> On 2015/01/20 17:20:50, bsalomon wrote:
> > This does disable the sharing of sb's across rts for uncached rts, right?
> 
> Yes, in the sense that there's the check "if (kCached_LifeCycle ==
lifeCycle)",
> the same one as with textures.
> 
> The intention is to affect the cache budgeting, so would it be more correct to
> use ... != kWrapped_LifeCycle ?
> 
> I'm confused about expectation of sharing SBs across RTs -- maybe I don't
> understand the code enough.
> 
> If you have time to specify how it is supposed to work, I can try to debug why
> it doesn't work with current Skia and fix it.. E.g if you have 16 canvases,
all
> which can receive commands at any given moment, how is the stencil buffer
> supposed to jump between them?
> 
> I understood that it is the intention that if there's a ref to the SB, the
cache
> will not give the SB out for other render target to use. The render targets
will
> not release the ref, since they're canvas surfaces and do not go out of scope
> until the canvas is destroyed.
> 
> > How does this come up for skbug 3119?
> 
> The 16 256x256x1bitx16 SBs for the SKPBench tile surfaces consume 16% of the
> cache.
> The 1 1000x1000x1bitx16 SB for the SKPBench result surface consumes 16% of the
> cache.
> 
> > Are the uncached RTs the tiles?
> 
> Yes. SKPBench tiles are uncached texture RTs.
> 
> > Won't it only be one SB shared by all the tiles?
> 
> At the moment, as far as my debugging / code reading indicates, no.

Ah, you're right I forgot that when we moved to the new cache system this got
broken. The RT's ref does prevent SB recycling. I have a system in mind to allow
resources to have "subresources" that they own but are still independently
accessible in the cache, but haven't implemented that yet. In the old cache a
ref on a resource didn't prevent the cache from returning it, which led to a
much larger set of problems.

Perhaps we should land this as is and then revisit when SB recycling is working.
Will take a closer look at the code.

[bsalomon, 2015-01-21 14:18:00]

lgtm

[bsalomon, 2015-01-21 14:18:18]

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

https://codereview.chromium.org/859013002/diff/1/src/gpu/GrGpu.h#newcode481
src/gpu/GrGpu.h:481: virtual bool
createStencilBufferForRenderTarget(GrRenderTarget*, bool budgeted,
On 2015/01/20 14:05:48, Kimmo Kinnunen wrote:
> On 2015/01/20 13:54:36, reed1 wrote:
> > Why not keep the enum that was created for this, rather than just a bool
> (which
> > is slightly harder to understand at a call-site)?
> 
> So it's consistent with the call that does the same thing with the texture.

I'm slightly hesitant to start depending on an enum from SkSurface all over the
gpu code. My pref is to leave it as a bool for now and perhaps the Budgeted enum
should graduate from the school of SkSurface to be in a pervasive Sk header in
the future (e.g. SkTypes.h) in a future CL. I'll be adding more Budgeted params
soon and can look at doing that.

[Kimmo Kinnunen, 2015-01-27 08:23:45]

The CQ bit was checked by kkinnunen@nvidia.com

[commit-bot: I haz the power, 2015-01-27 08:24:43]

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

[commit-bot: I haz the power, 2015-01-27 08:30:22]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/36c57dfb4fe9bbaca436942d5eaa75b142ba251d