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

[Iago Toral]

On Tue, 2018-06-26 at 10:59 -0700, Jason Ekstrand wrote:
> On Tue, Jun 26, 2018 at 4:08 AM, Iago Toral Quiroga <itoral at igalia.co
> m> 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.

No, no secondaries are involved. I did some more investigation and I
think my explanation of the problem was not good, this is what is
really happening:
First, I found the problem in the compute pipeline and I only extended
the fix to the graphics pipeline because it looked like the same
rationale would apply, so I'll explain what happens in compute and then
we can discuss whether the same problem applies to graphics.
The test does something like this:
for (...) {   clear ssbos / storage images   dispatch compute}
The first iteration of this loop will find that the compute pipeline
and descriptors are dirty and proceed to emit binding tables. We have
storage images, so during that process the push constant buffer is
amended to include storage images. Specifically, we call
anv_cmd_buffer_ensure_push_constants_size() for the images field. This
gives us a size of 624.
We move on to the second iteration of the loop. When we clear images
and ssbos via blorp, we again mark the push constant buffer as dirty.
Now we execute the compute dispatch and the first thing we do there is
anv_cmd_buffer_push_base_group_id() which calls
anv_cmd_buffer_ensure_push_constants_size() for the base group id,
which gives as a size of 144. This is smaller than what we computed in
the previous iteration, because we haven't called the same function for
the images field yet. Unfortunately, we will never call that again,
because we only do that during binding table emission and we only do
that if the compute pipeline is dirty (it is not) or our descriptors
are  dirty (they are not). So we don't re-emit binding table and we
don't ensure push constant space for the image data, but because we
come from a blorp execution our push constant dirty flag is true, so we
re-emit push constant data, only that this time we won't emit the push
constant data we need for the storage images, which leads to the
problem.
I thought that maybe making anv_cmd_buffer_ensure_push_constants_size()
only update the size if we alloc or realloc would fix this, but that
can cause GPU hangs in some cases when I run multiple tests in parallel
, so I guess it isn't that simple.

I hope I am making more sense now.
Iago
> --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/20180627/fbc6a29f/attachment.html>