[Mesa-dev] [PATCH 1/3] i965: Make a better helper function for UBO/SSBO/ABO surface handling.

[Jason Ekstrand]

On Thu, Nov 9, 2017 at 2:42 PM, Kenneth Graunke <kenneth at whitecape.org>
wrote:

> On Thursday, November 9, 2017 12:31:18 PM PST Jason Ekstrand wrote:
> > On Thu, Nov 9, 2017 at 12:45 AM, Kenneth Graunke <kenneth at whitecape.org>
> > wrote:
> >
> > > This fixes the missing AutomaticSize handling in the ABO code, removes
> > > a bunch of duplicated code, and drops an extra layer of wrapping around
> > > brw_emit_buffer_surface_state().
> > > ---
> > >  src/mesa/drivers/dri/i965/brw_context.h          |  10 --
> > >  src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 113
> > > +++++++----------------
> > >  src/mesa/drivers/dri/i965/gen6_constant_state.c  |   7 +-
> > >  3 files changed, 36 insertions(+), 94 deletions(-)
> > >
> > > diff --git a/src/mesa/drivers/dri/i965/brw_context.h
> > > b/src/mesa/drivers/dri/i965/brw_context.h
> > > index 8aa0c5ff64c..5d19a6bfc9a 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_context.h
> > > +++ b/src/mesa/drivers/dri/i965/brw_context.h
> > > @@ -1395,16 +1395,6 @@ brw_get_index_type(unsigned index_size)
> > >  void brw_prepare_vertices(struct brw_context *brw);
> > >
> > >  /* brw_wm_surface_state.c */
> > > -void brw_create_constant_surface(struct brw_context *brw,
> > > -                                 struct brw_bo *bo,
> > > -                                 uint32_t offset,
> > > -                                 uint32_t size,
> > > -                                 uint32_t *out_offset);
> > > -void brw_create_buffer_surface(struct brw_context *brw,
> > > -                               struct brw_bo *bo,
> > > -                               uint32_t offset,
> > > -                               uint32_t size,
> > > -                               uint32_t *out_offset);
> > >  void brw_update_buffer_texture_surface(struct gl_context *ctx,
> > >                                         unsigned unit,
> > >                                         uint32_t *surf_offset);
> > > diff --git a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > > b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > > index 27c241a87af..a483ba34151 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > > +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > > @@ -672,44 +672,6 @@ brw_update_buffer_texture_surface(struct
> gl_context
> > > *ctx,
> > >                                   0);
> > >  }
> > >
> > > -/**
> > > - * Create the constant buffer surface.  Vertex/fragment shader
> constants
> > > will be
> > > - * read from this buffer with Data Port Read instructions/messages.
> > > - */
> > > -void
> > > -brw_create_constant_surface(struct brw_context *brw,
> > > -                           struct brw_bo *bo,
> > > -                           uint32_t offset,
> > > -                           uint32_t size,
> > > -                           uint32_t *out_offset)
> > > -{
> > > -   brw_emit_buffer_surface_state(brw, out_offset, bo, offset,
> > > -                                 ISL_FORMAT_R32G32B32A32_FLOAT,
> > > -                                 size, 1, 0);
> > > -}
> > > -
> > > -/**
> > > - * Create the buffer surface. Shader buffer variables will be
> > > - * read from / write to this buffer with Data Port Read/Write
> > > - * instructions/messages.
> > > - */
> > > -void
> > > -brw_create_buffer_surface(struct brw_context *brw,
> > > -                          struct brw_bo *bo,
> > > -                          uint32_t offset,
> > > -                          uint32_t size,
> > > -                          uint32_t *out_offset)
> > > -{
> > > -   /* Use a raw surface so we can reuse existing untyped
> read/write/atomic
> > > -    * messages. We need these specifically for the fragment shader
> since
> > > they
> > > -    * include a pixel mask header that we need to ensure correct
> behavior
> > > -    * with helper invocations, which cannot write to the buffer.
> > > -    */
> > > -   brw_emit_buffer_surface_state(brw, out_offset, bo, offset,
> > > -                                 ISL_FORMAT_RAW,
> > > -                                 size, 1, RELOC_WRITE);
> > > -}
> > > -
> > >  /**
> > >   * Set up a binding table entry for use by stream output logic
> (transform
> > >   * feedback).
> > > @@ -1271,6 +1233,31 @@ const struct brw_tracked_state
> > > brw_cs_texture_surfaces = {
> > >     .emit = brw_update_cs_texture_surfaces,
> > >  };
> > >
> > > +static void
> > > +upload_buffer_surface(struct brw_context *brw,
> > > +                      struct gl_buffer_binding *binding,
> > > +                      uint32_t *out_offset,
> > > +                      enum isl_format format,
> > > +                      unsigned reloc_flags)
> > > +{
> > > +   struct gl_context *ctx = &brw->ctx;
> > > +
> > > +   if (binding->BufferObject == ctx->Shared->NullBufferObj) {
> > > +      emit_null_surface_state(brw, NULL, out_offset);
> > > +   } else {
> > > +      ptrdiff_t size = binding->BufferObject->Size - binding->Offset;
> > > +      if (!binding->AutomaticSize)
> > > +         size = MIN2(size, binding->Size);
> > > +
> > > +      struct intel_buffer_object *iobj =
> > > +         intel_buffer_object(binding->BufferObject);
> > > +      struct brw_bo *bo =
> > > +         intel_bufferobj_buffer(brw, iobj, binding->Offset, size,
> false);
> > >
> >
> > You're using this for both reads and writes.  I think you need another
> > boolean parameter instead of simply passing false all the time.  Other
> than
> > that, looks good.
>
> Whoops, thanks for catching that!  I've changed it from 'false' to
> (reloc_flags & RELOC_WRITE) != 0.
>
> With that change, should I add your R-b?
>

Yeah, that's probably ok.  I don't know how I feel about keying it off of
the reloc flag as opposed to the other way around but I don't care too
much.  With that, it would be

Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171109/cae63330/attachment-0001.html>