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

[Fredrik Höglund]

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.pitoiset at 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.

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.

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);
>