On Tue, Jun 26, 2018 at 4:08 AM, Iago Toral Quiroga <ito...@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
>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
