[Mesa-dev] [PATCH 03/27] i965: Enable hardware-generated binding tables on render path.

[Pohjolainen, Topi]

On Thu, May 07, 2015 at 04:43:21PM +0300, Pohjolainen, Topi wrote:
> On Tue, Apr 28, 2015 at 11:08:00PM +0300, Abdiel Janulgue wrote:
> > This patch implements the binding table enable command which is also
> > used to allocate a binding table pool where hardware-generated
> > binding table entries are flushed into. Each binding table offset in
> > the binding table pool is unique per each shader stage that are
> > enabled within a batch.
> > 
> > Also insert the required brw_tracked_state objects to enable
> > hw-generated binding tables in normal render path.
> > 
> > Signed-off-by: Abdiel Janulgue <abdiel.janulgue at linux.intel.com>
> > ---
> >  src/mesa/drivers/dri/i965/brw_binding_tables.c | 70 ++++++++++++++++++++++++++
> >  src/mesa/drivers/dri/i965/brw_context.c        |  4 ++
> >  src/mesa/drivers/dri/i965/brw_context.h        |  5 ++
> >  src/mesa/drivers/dri/i965/brw_state.h          |  7 +++
> >  src/mesa/drivers/dri/i965/brw_state_upload.c   |  2 +
> >  src/mesa/drivers/dri/i965/intel_batchbuffer.c  |  4 ++
> >  6 files changed, 92 insertions(+)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/brw_binding_tables.c b/src/mesa/drivers/dri/i965/brw_binding_tables.c
> > index 459165a..a58e32e 100644
> > --- a/src/mesa/drivers/dri/i965/brw_binding_tables.c
> > +++ b/src/mesa/drivers/dri/i965/brw_binding_tables.c
> > @@ -44,6 +44,11 @@
> >  #include "brw_state.h"
> >  #include "intel_batchbuffer.h"
> >  
> > +/* Somehow the hw-binding table pool offset must start here, otherwise
> > + * the GPU will hang
> > + */
> > +#define HW_BT_START_OFFSET 256;
> 
> I think we want to understand this a little better before enabling...
> 
> > +
> >  /**
> >   * Upload a shader stage's binding table as indirect state.
> >   *
> > @@ -163,6 +168,71 @@ const struct brw_tracked_state brw_gs_binding_table = {
> >     .emit = brw_gs_upload_binding_table,
> >  };
> >  
> > +/**
> > + * Hardware-generated binding tables for the resource streamer
> > + */
> > +void
> > +gen7_disable_hw_binding_tables(struct brw_context *brw)
> > +{
> > +   BEGIN_BATCH(3);
> > +   OUT_BATCH(_3DSTATE_BINDING_TABLE_POOL_ALLOC << 16 | (3 - 2));
> > +   OUT_BATCH(SET_FIELD(BRW_HW_BINDING_TABLE_OFF, BRW_HW_BINDING_TABLE_ENABLE) |
> > +             brw->is_haswell ? HSW_HW_BINDING_TABLE_RESERVED : 0);
> > +   OUT_BATCH(0);
> > +   ADVANCE_BATCH();
> > +
> > +   /* Pipe control workaround */
> > +   brw_emit_pipe_control_flush(brw, PIPE_CONTROL_STATE_CACHE_INVALIDATE);
> > +}
> > +
> > +void
> > +gen7_enable_hw_binding_tables(struct brw_context *brw)
> > +{
> > +   if (!brw->has_resource_streamer) {
> > +      gen7_disable_hw_binding_tables(brw);
> 
> I started wondering why we really need this - RS is disabled by default and
> we haven't needed to do anything to disable it before.
> 
> > +      return;
> > +   }
> > +
> > +   if (!brw->hw_bt_pool.bo) {
> > +      /* From the BSpec, 3D Pipeline > Resource Streamer > Hardware Binding Tables:
> > +       *
> > +       *  "A maximum of 16,383 Binding tables are allowed in any batch buffer."
> > +       */
> > +      int max_size = 16383 * 4;
> 
> But does it really need this much all the time? I guess I need to go and
> read the spec.

I haven't read through the entire series but it seems that we can calculate
(at least for gather constants) pretty accurately how much we need space.
Could we do it also here based on the program data of all stages? I maybe
missing something and just throwing questions up in the air, so bare with me...