[Mesa-dev] [PATCH] anv/cmd_buffer: emit binding tables always if push constants are dirty

[Jason Ekstrand]

On Tue, Jun 26, 2018 at 4:08 AM, Iago Toral Quiroga <itoral at igalia.com>
wrote:

> Storage images require to patch push constant stateto work, which happens
> during
> binding table emision. In the scenario where our pipeline and descriptors
> are
> not dirty, we don't re-emit the binding table, however, if our push
> constant
> state is dirty, we will re-emit the push constant state, trashing storage
> image setup.
>
> While that scenario is probably not very likely to happen in practice,
> there
> are some CTS tests that trigger this by clearing storage images and buffers
> and dispatching a compute shader in a loop. The clearing of the images
> and buffers will trigger a blorp execution which will dirty our push
> constant
> state, however, because  we don't alter the descriptors or the compute
> dispatch
> at all in the loop (we are basically execution the same program in a loop),
> our pipeline and descriptor state is not dirty. If the shader uses a
> storage
> image, then any iteration after the first will re-emit push constant state
> without re-emitting binding tables and the storage image will not be
> properly
> setup any more.
>

I don't see why that is a problem.  The only thing flush_descriptor_sets
does is fill out the binding and sampler tables and fill in the push
constant data for storage images/buffers.  The actual HW packets are filled
out by flush_push_constants and emit_descriptor_pointers.  Yes, blorp
trashes our descriptor pointers but the descriptor sets should be fine.
For push constants, it does emit 3DSTATE_CONSTANT_* but it doesn't actually
modify anv_cmd_state::push_constants.

Are secondary command buffers involved?  I could see something funny going
on with those.

--Jason



> Fixes multiple failures in some new CTS tests.
> ---
>  src/intel/vulkan/genX_cmd_buffer.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> b/src/intel/vulkan/genX_cmd_buffer.c
> index 97b321ccaeb..6e48aaedb9b 100644
> --- a/src/intel/vulkan/genX_cmd_buffer.c
> +++ b/src/intel/vulkan/genX_cmd_buffer.c
> @@ -2554,7 +2554,8 @@ genX(cmd_buffer_flush_state)(struct anv_cmd_buffer
> *cmd_buffer)
>      * 3DSTATE_BINDING_TABLE_POINTER_* for the push constants to take
> effect.
>      */
>     uint32_t dirty = 0;
> -   if (cmd_buffer->state.descriptors_dirty)
> +   if (cmd_buffer->state.descriptors_dirty ||
> +       cmd_buffer->state.push_constants_dirty)
>        dirty = flush_descriptor_sets(cmd_buffer);
>
>     if (dirty || cmd_buffer->state.push_constants_dirty) {
> @@ -2988,7 +2989,13 @@ genX(cmd_buffer_flush_compute_state)(struct
> anv_cmd_buffer *cmd_buffer)
>        anv_batch_emit_batch(&cmd_buffer->batch, &pipeline->batch);
>     }
>
> +   /* Storage images require push constant data, which is setup during the
> +    * binding table emision. If we have dirty push constants, we need to
> +    * re-emit the binding table so we get the push constant storage image
> setup
> +    * done, otherwise we trash it when we emit push constants below.
> +    */
>     if ((cmd_buffer->state.descriptors_dirty &
> VK_SHADER_STAGE_COMPUTE_BIT) ||
> +       (cmd_buffer->state.push_constants_dirty &
> VK_SHADER_STAGE_COMPUTE_BIT) ||
>         cmd_buffer->state.compute.pipeline_dirty) {
>        /* FIXME: figure out descriptors for gen7 */
>        result = flush_compute_descriptor_set(cmd_buffer);
> --
> 2.14.1
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180626/07e41c1b/attachment.html>