Use dynamic state from vulkan pipelines

Use dynamic state from vulkan pipelines

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

Committed: https://skia.googlesource.com/skia/+/470d77a1d7dc286cdad98b145dac735cd7638e56

[egdaniel, 2016-03-17 18:21:07]

Description was changed from

==========
Use dynamic state from vulkan pipelines

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

to

==========
Use dynamic state from vulkan pipelines

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

[egdaniel, 2016-03-17 19:08:24]

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

[jvanverth1, 2016-03-18 15:02:26]

LGTM, plus some comments/suggestions.

https://codereview.chromium.org/1813913002/diff/40001/src/gpu/vk/GrVkCommandB...
File src/gpu/vk/GrVkCommandBuffer.cpp (right):

https://codereview.chromium.org/1813913002/diff/40001/src/gpu/vk/GrVkCommandB...
src/gpu/vk/GrVkCommandBuffer.cpp:358: void GrVkCommandBuffer::bindPipeline(const
GrVkGpu* gpu, const GrVkPipeline* pipeline) {
I'm curious why you moved this from the header.

https://codereview.chromium.org/1813913002/diff/40001/src/gpu/vk/GrVkPipeline...
File src/gpu/vk/GrVkPipeline.cpp (right):

https://codereview.chromium.org/1813913002/diff/40001/src/gpu/vk/GrVkPipeline...
src/gpu/vk/GrVkPipeline.cpp:351: // colorBlendInfo->blendConstants is set
dynamically
SkASSERT(usesBlendConstant);

https://codereview.chromium.org/1813913002/diff/40001/src/gpu/vk/GrVkPipeline...
src/gpu/vk/GrVkPipeline.cpp:401: // If we aren't using the blend constant we
don't set it on the command buffer where is an error
This comment is a little confusing. Suggestion: "If we aren't using the blend
constant, we don't include it here. Otherwise we may get an error on the command
buffer."

https://codereview.chromium.org/1813913002/diff/40001/src/gpu/vk/GrVkPipeline...
src/gpu/vk/GrVkPipeline.cpp:557: set_dynamic_scissor_state(gpu, cmdBuffer,
pipeline, target);
This is fine for now, but when you add the change to cache the pipeline, you may
want to make sure they only get set when they change. Other you're inserting
redundant instructions into the command buffer.

[egdaniel, 2016-03-18 19:29:59]

I've added caching of dynamic values to this CL. Can you quickly review the
recent changes?

https://codereview.chromium.org/1813913002/diff/40001/src/gpu/vk/GrVkCommandB...
File src/gpu/vk/GrVkCommandBuffer.cpp (right):

https://codereview.chromium.org/1813913002/diff/40001/src/gpu/vk/GrVkCommandB...
src/gpu/vk/GrVkCommandBuffer.cpp:358: void GrVkCommandBuffer::bindPipeline(const
GrVkGpu* gpu, const GrVkPipeline* pipeline) {
On 2016/03/18 15:02:26, jvanverth1 wrote:
> I'm curious why you moved this from the header.

all the other command buffer functions were in the cpp, this one didn't seem to
be any more special than the rest.

https://codereview.chromium.org/1813913002/diff/40001/src/gpu/vk/GrVkPipeline...
File src/gpu/vk/GrVkPipeline.cpp (right):

https://codereview.chromium.org/1813913002/diff/40001/src/gpu/vk/GrVkPipeline...
src/gpu/vk/GrVkPipeline.cpp:351: // colorBlendInfo->blendConstants is set
dynamically
On 2016/03/18 15:02:26, jvanverth1 wrote:
> SkASSERT(usesBlendConstant);

removed

https://codereview.chromium.org/1813913002/diff/40001/src/gpu/vk/GrVkPipeline...
src/gpu/vk/GrVkPipeline.cpp:401: // If we aren't using the blend constant we
don't set it on the command buffer where is an error
On 2016/03/18 15:02:26, jvanverth1 wrote:
> This comment is a little confusing. Suggestion: "If we aren't using the blend
> constant, we don't include it here. Otherwise we may get an error on the
command
> buffer."

always using dynamic blendConstant now

https://codereview.chromium.org/1813913002/diff/40001/src/gpu/vk/GrVkPipeline...
src/gpu/vk/GrVkPipeline.cpp:557: set_dynamic_scissor_state(gpu, cmdBuffer,
pipeline, target);
On 2016/03/18 15:02:26, jvanverth1 wrote:
> This is fine for now, but when you add the change to cache the pipeline, you
may
> want to make sure they only get set when they change. Other you're inserting
> redundant instructions into the command buffer.

added caching

[jvanverth1, 2016-03-18 19:36:45]

On 2016/03/18 19:29:59, egdaniel wrote:
> I've added caching of dynamic values to this CL. Can you quickly review the
> recent changes?
> 
>
https://codereview.chromium.org/1813913002/diff/40001/src/gpu/vk/GrVkCommandB...
> File src/gpu/vk/GrVkCommandBuffer.cpp (right):
> 
>
https://codereview.chromium.org/1813913002/diff/40001/src/gpu/vk/GrVkCommandB...
> src/gpu/vk/GrVkCommandBuffer.cpp:358: void
GrVkCommandBuffer::bindPipeline(const
> GrVkGpu* gpu, const GrVkPipeline* pipeline) {
> On 2016/03/18 15:02:26, jvanverth1 wrote:
> > I'm curious why you moved this from the header.
> 
> all the other command buffer functions were in the cpp, this one didn't seem
to
> be any more special than the rest.
> 
>
https://codereview.chromium.org/1813913002/diff/40001/src/gpu/vk/GrVkPipeline...
> File src/gpu/vk/GrVkPipeline.cpp (right):
> 
>
https://codereview.chromium.org/1813913002/diff/40001/src/gpu/vk/GrVkPipeline...
> src/gpu/vk/GrVkPipeline.cpp:351: // colorBlendInfo->blendConstants is set
> dynamically
> On 2016/03/18 15:02:26, jvanverth1 wrote:
> > SkASSERT(usesBlendConstant);
> 
> removed
> 
>
https://codereview.chromium.org/1813913002/diff/40001/src/gpu/vk/GrVkPipeline...
> src/gpu/vk/GrVkPipeline.cpp:401: // If we aren't using the blend constant we
> don't set it on the command buffer where is an error
> On 2016/03/18 15:02:26, jvanverth1 wrote:
> > This comment is a little confusing. Suggestion: "If we aren't using the
blend
> > constant, we don't include it here. Otherwise we may get an error on the
> command
> > buffer."
> 
> always using dynamic blendConstant now
> 
>
https://codereview.chromium.org/1813913002/diff/40001/src/gpu/vk/GrVkPipeline...
> src/gpu/vk/GrVkPipeline.cpp:557: set_dynamic_scissor_state(gpu, cmdBuffer,
> pipeline, target);
> On 2016/03/18 15:02:26, jvanverth1 wrote:
> > This is fine for now, but when you add the change to cache the pipeline, you
> may
> > want to make sure they only get set when they change. Other you're inserting
> > redundant instructions into the command buffer.
> 
> added caching

LGTM v2.0

[egdaniel, 2016-03-18 19:40:45]

The CQ bit was checked by egdaniel@google.com

[commit-bot: I haz the power, 2016-03-18 19:40:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1813913002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1813913002/60001

[commit-bot: I haz the power, 2016-03-18 19:50:29]

Description was changed from

==========
Use dynamic state from vulkan pipelines

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

to

==========
Use dynamic state from vulkan pipelines

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/+/470d77a1d7dc286cdad98b145dac735cd7638e56
==========

[commit-bot: I haz the power, 2016-03-18 19:50:30]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/470d77a1d7dc286cdad98b145dac735cd7638e56