Re: [Mesa-dev] [PATCH 2/3] radv: only copy the dynamic states that changed

[Samuel Pitoiset]

On 09/15/2017 05:40 PM, Fredrik Höglund wrote:
On Thursday 14 September 2017, Samuel Pitoiset wrote:

On 09/13/2017 06:34 PM, Fredrik Höglund wrote:
On Wednesday 13 September 2017, Samuel Pitoiset wrote:
When binding a new pipeline, we applied all dynamic states
without checking if they really need to be re-emitted. This
doesn't seem to be useful for the meta operations because only
the viewports/scissors are updated.

This should reduce the number of commands added to the IB
when a new graphics pipeline is bound.

Signed-off-by: Samuel Pitoiset <samuel.pitoi...@gmail.com>
---
   src/amd/vulkan/radv_cmd_buffer.c | 100 
+++++++++++++++++++++++++++++----------
   src/amd/vulkan/radv_private.h    |   7 +--
   2 files changed, 79 insertions(+), 28 deletions(-)

diff --git a/src/amd/vulkan/radv_cmd_buffer.c b/src/amd/vulkan/radv_cmd_buffer.c
index 561ca2fbce..9e76cf6c2c 100644
--- a/src/amd/vulkan/radv_cmd_buffer.c
+++ b/src/amd/vulkan/radv_cmd_buffer.c
@@ -78,43 +78,92 @@ const struct radv_dynamic_state default_dynamic_state = {
        },
   };
   
-void
+uint32_t
   radv_dynamic_state_copy(struct radv_dynamic_state *dest,
                        const struct radv_dynamic_state *src,
                        uint32_t copy_mask)
   {
+       uint32_t dest_mask = 0;
+
        if (copy_mask & (1 << VK_DYNAMIC_STATE_VIEWPORT)) {
-               dest->viewport.count = src->viewport.count;
-               typed_memcpy(dest->viewport.viewports, src->viewport.viewports,
-                            src->viewport.count);
+               if (memcmp(&dest->viewport, &src->viewport,
+                          sizeof(src->viewport))) {

I think only the first src->viewport.count viewports should be compared,
since those are the only ones used by the pipeline, and the only ones
that are copied.

Yeah, looks better.


The dest->viewport.count assignment should probably also be done
outside of the if (copy_mask & ...) block, since the viewport count is
fixed at pipeline creation time, and does not depend on whether the state
is dynamic or not.

The number of viewports can also be updated in vkCmdSetViewport() if the
initial value is less than the total count.

The implementation does that, but I don't think the specification
supports it.

The Vulkan specification (1.0.61) says:

"The number of viewports used by a pipeline is controlled by the
viewportCount member of the VkPipelineViewportStateCreateInfo structure
used in pipeline creation"

pViewports is ignored when the state is dynamic, but viewportCount
"must be between 1 and VkPhysicalDeviceLimits::maxViewports, inclusive"

So I don't think the count is meant to be dynamic.

Yeah, I think this is what the spec says as well. I have just sent 
patches which change that behaviour to reflect the spec.

Thanks!



This also goes for the scissor state below.

+                       dest->viewport.count = src->viewport.count;
+                       typed_memcpy(dest->viewport.viewports,
+                                    src->viewport.viewports,
+                                    src->viewport.count);
+                       dest_mask |= 1 << VK_DYNAMIC_STATE_VIEWPORT;
+               }
        }
   
   	if (copy_mask & (1 << VK_DYNAMIC_STATE_SCISSOR)) {
-               dest->scissor.count = src->scissor.count;
-               typed_memcpy(dest->scissor.scissors, src->scissor.scissors,
-                            src->scissor.count);
+               if (memcmp(&dest->scissor, &src->scissor,
+                          sizeof(src->scissor))) {
+                       dest->scissor.count = src->scissor.count;
+                       typed_memcpy(dest->scissor.scissors,
+                                    src->scissor.scissors, src->scissor.count);
+                       dest_mask |= 1 << VK_DYNAMIC_STATE_SCISSOR;
+               }
        }
   
-	if (copy_mask & (1 << VK_DYNAMIC_STATE_LINE_WIDTH))
-               dest->line_width = src->line_width;
+       if (copy_mask & (1 << VK_DYNAMIC_STATE_LINE_WIDTH)) {
+               if (dest->line_width != src->line_width) {
+                       dest->line_width = src->line_width;
+                       dest_mask |= 1 << VK_DYNAMIC_STATE_LINE_WIDTH;
+               }
+       }
   
-	if (copy_mask & (1 << VK_DYNAMIC_STATE_DEPTH_BIAS))
-               dest->depth_bias = src->depth_bias;
+       if (copy_mask & (1 << VK_DYNAMIC_STATE_DEPTH_BIAS)) {
+               if (memcmp(&dest->depth_bias, &src->depth_bias,
+                          sizeof(src->depth_bias))) {
+                       dest->depth_bias = src->depth_bias;
+                       dest_mask |= 1 << VK_DYNAMIC_STATE_DEPTH_BIAS;
+               }
+       }
   
-	if (copy_mask & (1 << VK_DYNAMIC_STATE_BLEND_CONSTANTS))
-               typed_memcpy(dest->blend_constants, src->blend_constants, 4);
+       if (copy_mask & (1 << VK_DYNAMIC_STATE_BLEND_CONSTANTS)) {
+               if (memcmp(&dest->blend_constants, &src->blend_constants,
+                          sizeof(src->blend_constants))) {
+                       typed_memcpy(dest->blend_constants,
+                                    src->blend_constants, 4);
+                       dest_mask |= 1 << VK_DYNAMIC_STATE_BLEND_CONSTANTS;
+               }
+       }
   
-	if (copy_mask & (1 << VK_DYNAMIC_STATE_DEPTH_BOUNDS))
-               dest->depth_bounds = src->depth_bounds;
+       if (copy_mask & (1 << VK_DYNAMIC_STATE_DEPTH_BOUNDS)) {
+               if (memcmp(&dest->depth_bounds, &src->depth_bounds,
+                          sizeof(src->depth_bounds))) {
+                       dest->depth_bounds = src->depth_bounds;
+                       dest_mask |= 1 << VK_DYNAMIC_STATE_DEPTH_BOUNDS;
+               }
+       }
   
-	if (copy_mask & (1 << VK_DYNAMIC_STATE_STENCIL_COMPARE_MASK))
-               dest->stencil_compare_mask = src->stencil_compare_mask;
+       if (copy_mask & (1 << VK_DYNAMIC_STATE_STENCIL_COMPARE_MASK)) {
+               if (memcmp(&dest->stencil_compare_mask,
+                          &src->stencil_compare_mask,
+                          sizeof(src->stencil_compare_mask))) {
+                       dest->stencil_compare_mask = src->stencil_compare_mask;
+                       dest_mask |= 1 << VK_DYNAMIC_STATE_STENCIL_COMPARE_MASK;
+               }
+       }
   
-	if (copy_mask & (1 << VK_DYNAMIC_STATE_STENCIL_WRITE_MASK))
-               dest->stencil_write_mask = src->stencil_write_mask;
+       if (copy_mask & (1 << VK_DYNAMIC_STATE_STENCIL_WRITE_MASK)) {
+               if (memcmp(&dest->stencil_write_mask, &src->stencil_write_mask,
+                          sizeof(src->stencil_write_mask))) {
+                       dest->stencil_write_mask = src->stencil_write_mask;
+                       dest_mask |= 1 << VK_DYNAMIC_STATE_STENCIL_WRITE_MASK;
+               }
+       }
+
+       if (copy_mask & (1 << VK_DYNAMIC_STATE_STENCIL_REFERENCE)) {
+               if (memcmp(&dest->stencil_reference, &src->stencil_reference,
+                          sizeof(src->stencil_reference))) {
+                       dest->stencil_reference = src->stencil_reference;
+                       dest_mask |= 1 << VK_DYNAMIC_STATE_STENCIL_REFERENCE;
+               }
+       }
   
-	if (copy_mask & (1 << VK_DYNAMIC_STATE_STENCIL_REFERENCE))
-               dest->stencil_reference = src->stencil_reference;
+       return dest_mask;
   }
   
   bool radv_cmd_buffer_uses_mec(struct radv_cmd_buffer *cmd_buffer)
@@ -2445,6 +2494,7 @@ void radv_CmdBindPipeline(
   {
        RADV_FROM_HANDLE(radv_cmd_buffer, cmd_buffer, commandBuffer);
        RADV_FROM_HANDLE(radv_pipeline, pipeline, _pipeline);
+       uint32_t dirty_mask;
   
   	radv_mark_descriptor_sets_dirty(cmd_buffer);
   
@@ -2462,10 +2512,10 @@ void radv_CmdBindPipeline(
                cmd_buffer->push_constant_stages |= pipeline->active_stages;
   
   		/* Apply the dynamic state from the pipeline */
-               cmd_buffer->state.dirty |= pipeline->dynamic_state.mask;
-               radv_dynamic_state_copy(&cmd_buffer->state.dynamic,
-                                       &pipeline->dynamic_state,
-                                       pipeline->dynamic_state.mask);
+               dirty_mask = radv_dynamic_state_copy(&cmd_buffer->state.dynamic,
+                                                    &pipeline->dynamic_state,
+                                                    
pipeline->dynamic_state.mask);
+               cmd_buffer->state.dirty |= dirty_mask;
   
   		if (pipeline->graphics.esgs_ring_size > cmd_buffer->esgs_ring_size_needed)
                        cmd_buffer->esgs_ring_size_needed = 
pipeline->graphics.esgs_ring_size;
diff --git a/src/amd/vulkan/radv_private.h b/src/amd/vulkan/radv_private.h
index 0cebb3e7e1..9c67da49a2 100644
--- a/src/amd/vulkan/radv_private.h
+++ b/src/amd/vulkan/radv_private.h
@@ -747,9 +747,10 @@ struct radv_dynamic_state {
   
   extern const struct radv_dynamic_state default_dynamic_state;
   
-void radv_dynamic_state_copy(struct radv_dynamic_state *dest,
-                            const struct radv_dynamic_state *src,
-                            uint32_t copy_mask);
+uint32_t
+radv_dynamic_state_copy(struct radv_dynamic_state *dest,
+                       const struct radv_dynamic_state *src,
+                       uint32_t copy_mask);
   
   const char *
   radv_get_debug_option_name(int id);




_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev