<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Nov 9, 2017 at 2:42 PM, Kenneth Graunke <<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Thursday, November 9, 2017 12:31:18 PM PST Jason Ekstrand wrote: > On Thu, Nov 9, 2017 at 12:45 AM, Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> > 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. </div></div>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? </blockquote></div></div><div class="gmail_extra"> </div><div class="gmail_extra">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</div><div class="gmail_extra"> </div><div class="gmail_extra">Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> </div></div>