[Mesa-dev] [PATCH v5 08/11] anv: Added support for dynamic sample locations

[Jason Ekstrand]

On Thu, Mar 14, 2019 at 7:08 PM Jason Ekstrand <jason at jlekstrand.net> wrote:

> From: Eleni Maria Stea <estea at igalia.com>
>
> Added support for setting the locations when the pipeline has been
> created with the dynamic state bit enabled according to the Vulkan
> Specification section [26.5. Custom Sample Locations] for the function:
>
> 'vkCmdSetSampleLocationsEXT'
>
> The reason that we preferred to store the boolean valid inside the
> dynamic state struct for locations instead of using a dirty bit
> (ANV_CMD_DIRTY_SAMPLE_LOCATIONS for example) is that other functions
> can modify the value of the dirty bits causing unexpected behavior.
>
> v2: Removed all the anv* structs used with sample locations to store
>     the locations in order for dynamic case. (see also the patch for
>     the non-dynamic case. (Jason Ekstrand)
> ---
>  src/intel/vulkan/anv_cmd_buffer.c  | 17 +++++++++++++++++
>  src/intel/vulkan/anv_private.h     |  6 ++++++
>  src/intel/vulkan/genX_cmd_buffer.c | 11 +++++++++++
>  3 files changed, 34 insertions(+)
>
> diff --git a/src/intel/vulkan/anv_cmd_buffer.c
> b/src/intel/vulkan/anv_cmd_buffer.c
> index 1b34644a434..7f31300857b 100644
> --- a/src/intel/vulkan/anv_cmd_buffer.c
> +++ b/src/intel/vulkan/anv_cmd_buffer.c
> @@ -558,6 +558,23 @@ void anv_CmdSetStencilReference(
>     cmd_buffer->state.gfx.dirty |= ANV_CMD_DIRTY_DYNAMIC_STENCIL_REFERENCE;
>  }
>
> +void
> +anv_CmdSetSampleLocationsEXT(VkCommandBuffer commandBuffer,
> +                             const VkSampleLocationsInfoEXT
> *pSampleLocationsInfo)
> +{
> +   ANV_FROM_HANDLE(anv_cmd_buffer, cmd_buffer, commandBuffer);
> +
> +   struct anv_dynamic_state *dyn_state = &cmd_buffer->state.gfx.dynamic;
> +   uint32_t samples = pSampleLocationsInfo->sampleLocationsPerPixel;
> +
> +   assert(pSampleLocationsInfo);
> +   dyn_state->sample_locations.samples = samples;
> +   typed_memcpy(dyn_state->sample_locations.locations,
> +                pSampleLocationsInfo->pSampleLocations, samples);
> +
> +   dyn_state->sample_locations.valid = true;
> +}
> +
>  static void
>  anv_cmd_buffer_bind_descriptor_set(struct anv_cmd_buffer *cmd_buffer,
>                                     VkPipelineBindPoint bind_point,
> diff --git a/src/intel/vulkan/anv_private.h
> b/src/intel/vulkan/anv_private.h
> index a39195733cd..94c9db14028 100644
> --- a/src/intel/vulkan/anv_private.h
> +++ b/src/intel/vulkan/anv_private.h
> @@ -2124,6 +2124,12 @@ struct anv_dynamic_state {
>        uint32_t                                  front;
>        uint32_t                                  back;
>     } stencil_reference;
> +
> +   struct {
> +      bool                                      valid;
> +      uint32_t                                  samples;
> +      VkSampleLocationEXT
>  locations[MAX_SAMPLE_LOCATIONS];
> +   } sample_locations;
>  };
>
>  extern const struct anv_dynamic_state default_dynamic_state;
> diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> b/src/intel/vulkan/genX_cmd_buffer.c
> index 7687507e6b7..5d2b17cf8ae 100644
> --- a/src/intel/vulkan/genX_cmd_buffer.c
> +++ b/src/intel/vulkan/genX_cmd_buffer.c
> @@ -2796,6 +2796,17 @@ genX(cmd_buffer_flush_state)(struct anv_cmd_buffer
> *cmd_buffer)
>                                        ANV_CMD_DIRTY_RENDER_TARGETS))
>        gen7_cmd_buffer_emit_scissor(cmd_buffer);
>
> +   if (cmd_buffer->state.gfx.dynamic.sample_locations.valid) {
> +      uint32_t samples =
> cmd_buffer->state.gfx.dynamic.sample_locations.samples;
> +      const VkSampleLocationEXT *locations =
> +         cmd_buffer->state.gfx.dynamic.sample_locations.locations;
> +      genX(emit_multisample)(&cmd_buffer->batch, samples, locations);
> +#if GEN_GEN >= 8
> +      genX(emit_sample_pattern)(&cmd_buffer->batch, samples, locations);
> +#endif
> +      cmd_buffer->state.gfx.dynamic.sample_locations.valid = false;
>

I'm not sure this is actually going to be correct.  With dynamic state,
you're required to set it before you use it.  With pipeline state, it gets
set every time the pipeline is bound.  Effectively, the pipeline state is a
big bag of dynamic state.  With both of these, however, there are no
defaults and you're required to bind a pipeline containing the state or
explicitly set it on the command buffer before it gets used.
VK_EXT_sample_locations is different though because it does have a
default.  So the question I'm coming around to is: When does the default
get applied?  The only sensible thing I can think of is at the top of the
command buffer or maybe the top of the subpass.  If this is the case, then
we need to emit sample positions at the start of every subpass.  Does the
spec talk about this at all?

--Jason

>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20190314/06e4b386/attachment.html>