[Mesa-dev] [PATCH 11/22] anv/cmd_buffer: Ensure the fast clear values are correct

[Nanley Chery]

On Tue, May 02, 2017 at 04:54:55PM -0700, Jason Ekstrand wrote:
> On Thu, Apr 27, 2017 at 11:32 AM, Nanley Chery <nanleychery at gmail.com>
> wrote:
> 
> > Signed-off-by: Nanley Chery <nanley.g.chery at intel.com>
> > ---
> >  src/intel/vulkan/genX_cmd_buffer.c | 76 ++++++++++++++++++++++++++++++
> > ++++++++
> >  1 file changed, 76 insertions(+)
> >
> > diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> > b/src/intel/vulkan/genX_cmd_buffer.c
> > index e3b1687121..4698270abb 100644
> > --- a/src/intel/vulkan/genX_cmd_buffer.c
> > +++ b/src/intel/vulkan/genX_cmd_buffer.c
> > @@ -400,6 +400,57 @@ transition_depth_buffer(struct anv_cmd_buffer
> > *cmd_buffer,
> >  }
> >
> >
> > +/* Copies clear value dwords between a surface state object and an image's
> > + * clear value buffer.
> > + */
> > +static void
> > +genX(transfer_clear_value)(struct anv_cmd_buffer * const cmd_buffer,
> > +                           const struct anv_state surface_state,
> > +                           const struct anv_image * const image,
> > +                           const uint8_t level,
> > +                           const bool copy_to_buffer)
> > +{
> > +   assert(cmd_buffer && image);
> > +
> > +   /* The image and its subresource must have a color auxiliary buffer. */
> > +   assert(anv_image_has_color_aux(image));
> > +   assert(level < anv_color_aux_levels(image));
> > +
> > +   const uint32_t img_clear_offset =
> > +      image->offset + image->aux_surface.offset +
> > +      image->aux_surface.isl.size +
> > +      cmd_buffer->device->isl_dev.ss.size * level +
> > +      cmd_buffer->device->isl_dev.ss.clear_value_offset;
> >
> 
> I think it would be good to add an anv_image_clear_value_offset(anv_image
> *, unsigned level) helper for this.  This calculation feels very much like
> anv_image internals which are leaking into cmd_buffer code.
> 
> 

This issue is indirectly addressed in the last patch of the series.

> > +
> > +   struct anv_bo * const ss_bo =
> > +      &cmd_buffer->device->surface_state_block_pool.bo;
> > +   const uint32_t ss_clear_offset = surface_state.offset +
> > +      cmd_buffer->device->isl_dev.ss.clear_value_offset;
> > +
> > +   const uint8_t clear_value_size =
> > +      cmd_buffer->device->isl_dev.ss.clear_value_size;
> > +
> > +   if (copy_to_buffer) {
> > +      genX(cmd_buffer_gpu_memcpy)(cmd_buffer, image->bo,
> > img_clear_offset,
> > +                                  ss_bo, ss_clear_offset,
> > clear_value_size);
> > +   } else {
> > +      genX(cmd_buffer_gpu_memcpy)(cmd_buffer, ss_bo, ss_clear_offset,
> > +                                  image->bo, img_clear_offset,
> > clear_value_size);
> >
> 
> The streamout GPU memcpy stuff is a bit heavy-weight for this.  It's great
> if you have lots of data to copy but setting up a full 3D pipeline per
> surface state is a bit excessive.  Probably better to just use the command
> streamer for this instead.
> 
> 

This issue is addressed in the next patch.