[Mesa-dev] [PATCH 2/3] radv: only copy the dynamic states that changed
Samuel Pitoiset
samuel.pitoiset at gmail.com
Mon Sep 18 09:53:07 UTC 2017
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.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.
>>
>> 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);
>>>>
>>>
>>
>
More information about the mesa-dev
mailing list