[Mesa-dev] [PATCH 1/3] anv/descriptor_set: Fix binding partly undefined descriptor sets

[Nanley Chery]

On Wed, Jul 13, 2016 at 03:56:42PM -0700, Jason Ekstrand wrote:
> On Wed, Jul 13, 2016 at 3:34 PM, Nanley Chery <nanleychery at gmail.com> wrote:
> 
> > Section 13.2.3. of the Vulkan spec requires that implementations be able to
> > bind sparsely-defined Descriptor Sets without any errors or exceptions.
> >
> > When binding a descriptor set that contains a dynamic buffer
> > binding/descriptor,
> > the driver attempts to dereference the descriptor's buffer_view field if
> > it is
> > non-NULL. It currently segfaults on undefined descriptors as this field is
> > never
> > zero-initialized. Zero undefined descriptors to avoid segfaulting. This
> > solution was suggested by Jason Ekstrand.
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96850
> > Cc: 12.0 <mesa-stable at lists.freedesktop.org>
> > Signed-off-by: Nanley Chery <nanley.g.chery at intel.com>
> > ---
> >  src/intel/vulkan/anv_descriptor_set.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/intel/vulkan/anv_descriptor_set.c
> > b/src/intel/vulkan/anv_descriptor_set.c
> > index 448ae0e..f06d2e4 100644
> > --- a/src/intel/vulkan/anv_descriptor_set.c
> > +++ b/src/intel/vulkan/anv_descriptor_set.c
> > @@ -412,8 +412,8 @@ anv_descriptor_set_create(struct anv_device *device,
> >     /* Go through and fill out immutable samplers if we have any */
> >     struct anv_descriptor *desc = set->descriptors;
> >     for (uint32_t b = 0; b < layout->binding_count; b++) {
> > -      if (layout->binding[b].immutable_samplers) {
> > -         for (uint32_t i = 0; i < layout->binding[b].array_size; i++) {
> > +      for (uint32_t i = 0; i < layout->binding[b].array_size; i++) {
> > +         if (layout->binding[b].immutable_samplers) {
> >              /* The type will get changed to COMBINED_IMAGE_SAMPLER in
> >               * UpdateDescriptorSets if needed.  However, if the descriptor
> >               * set has an immutable sampler, UpdateDescriptorSets may
> > never
> > @@ -423,6 +423,11 @@ anv_descriptor_set_create(struct anv_device *device,
> >                 .type = VK_DESCRIPTOR_TYPE_SAMPLER,
> >                 .sampler = layout->binding[b].immutable_samplers[i],
> >              };
> > +         } else {
> > +            /* By defining the descriptors to be zero now, we can later
> > verify that
> > +             * the descriptor has not been populated with user data.
> > +             */
> > +            zero(desc[i]);
> >
> 
> I think I'd rather just zero the whole thing rather than one descriptor at
> a time.  Given that the memset will use vectorized memory operations, it's
> almost certainly just as fast if not faster to do so and I think it's more
> clear.
> 

I agree. I overlooked the fact that we could memset all descriptor array
elements in a binding.

- Nanley

> --Jason
> 
> 
> >           }
> >        }
> >        desc += layout->binding[b].array_size;
> > --
> > 2.9.0
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >