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

[Eleni Maria Stea]

On Thu, 14 Mar 2019 20:00:45 -0500
Jason Ekstrand <jason at jlekstrand.net> wrote:
> >
> >  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
> 
> >  

Hi Jason,

If I understand well (sorry if I misunderstood), you want to make sure
that in every case we will have locations set either the default or
custom, and that we emit the default locations when a pipeline (or
subpass, but we don't have locations per subpass, only per pipeline) is
bound after a pipeline for which custom locations were set dynamically?

I didn't find any reference to this problem in the spec, the solution I
thought myself was to use the variable bool custom_locations to decide
if we are going to emit custom locations or the default in
emit_ms_state and always emit some locations when the extension is
enabled. 

So:

When the emit_ms_state is called for each new pipeline at rasterization,
we check if 1- the custom locations are TRUE and if 2- the flag for the
dynamic locations (VK_DYNAMIC_STATE_SAMPLE_LOCATIONS_EXT) is set to
false. If both apply we emit the custom locations. In every other case
(when locations_enabled are false, or when dynamic state is true) we
emit the default locations.

This way, in the non-dynamic case (no VK_DYNAMIC_STATE... set):
If a pipeline has the locations enabled, we emit the custom.
If a pipeline doesn't have locations enabled, we emit the default.
So, we always have some locations set for the pipeline.

Similarly in the dynamic case:
When the user sets locations with vkCmdSetSampleLocations we use these
locations.
As we have locations per pipeline not per subpass (variable sample
locations = false) next pipeline that will be bound will have either
custom locations (from emit_ms_state) or the default (emitted by
the emit_ms_state), unless if the DYNAMIC_STATE flag is set and the user
calls the vkCmdSetSampleLocationsEXT again to override the default,
that we'll set the user's.
So, again we'll always have some locations set (and these will be the
default when the user doesn't chain the locations info struct, or
disables the locations, or sets the DYNAMIC flag but doesn't override
the locations with the VkCmdSetLocationsEXT).

So, I think we are fine.

If we didn't emit always the default pipelines created after setting
locations with any of the 2 possible ways would have garbage locations
set. 

I verified this would happen like that: If you don't make use of the
bool custom_locations and run all the multisample.* vulkancts tests at
once, you will notice that tests that don't set the sample locations in
the pipeline and run after some tests that set them fail. I had spotted
the following failures:

dEQP-VK.glsl.builtin_var.fragcoord_msaa.*
dEQP-VK.pipeline.multisample.sample_mask_with_depth_test.samples_.*
dEQP-VK.pipeline.multisample.sample_mask_with_depth_test.samples_.*_post_depth_coverage
dEQP-VK.pipeline.multisample_interpolation.sample_interpolate_at_distinct_values.128_128_1.samples_.*
dEQP-VK.pipeline.multisample_interpolation.sample_interpolate_at_distinct_values.137_191_1.samples_.*
dEQP-VK.pipeline.multisample_interpolation.sample_qualifier_distinct_values.128_128_1.samples_.*
dEQP-VK.pipeline.multisample_interpolation.sample_qualifier_distinct_values.137_191_1.samples_.*
dEQP-VK.pipeline.multisample_shader_builtin.sample_position.distribution.128_128_1.samples_.*
dEQP-VK.pipeline.multisample_shader_builtin.sample_position.distribution.137_191_1.samples_.*

Apart from this problem I described, I coudn't think of any other
scenario that could leave us with undefined locations. Do you have any
other case in mind?

Thank you in advance :)
Eleni