[Mesa-dev] [PATCH v2 29/42] i965: Enable shared local memory for CS shared variables

[Iago Toral]

On Mon, 2015-11-30 at 00:43 -0800, Jordan Justen wrote:
> On 2015-11-25 03:24:38, Iago Toral wrote:
> > On Tue, 2015-11-17 at 21:55 -0800, Jordan Justen wrote:
> > > Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>
> > > ---
> > >  src/mesa/drivers/dri/i965/brw_cs.c        |  2 ++
> > >  src/mesa/drivers/dri/i965/brw_defines.h   |  2 ++
> > >  src/mesa/drivers/dri/i965/gen7_cs_state.c | 12 ++++++++++++
> > >  3 files changed, 16 insertions(+)
> > > 
> > > diff --git a/src/mesa/drivers/dri/i965/brw_cs.c b/src/mesa/drivers/dri/i965/brw_cs.c
> > > index 263d224..704b00d 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_cs.c
> > > +++ b/src/mesa/drivers/dri/i965/brw_cs.c
> > > @@ -69,6 +69,8 @@ brw_codegen_cs_prog(struct brw_context *brw,
> > >  
> > >     memset(&prog_data, 0, sizeof(prog_data));
> > >  
> > > +   prog_data.base.total_shared = prog->Comp.SharedSize;
> > > +
> > >     assign_cs_binding_table_offsets(brw->intelScreen->devinfo, prog,
> > >                                     &cp->program.Base, &prog_data);
> > >  
> > > diff --git a/src/mesa/drivers/dri/i965/brw_defines.h b/src/mesa/drivers/dri/i965/brw_defines.h
> > > index 8189c08..ca5378a 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_defines.h
> > > +++ b/src/mesa/drivers/dri/i965/brw_defines.h
> > > @@ -2846,6 +2846,8 @@ enum brw_wm_barycentric_interp_mode {
> > >  /* GEN7 DW5, GEN8+ DW6 */
> > >  # define MEDIA_BARRIER_ENABLE_SHIFT             21
> > >  # define MEDIA_BARRIER_ENABLE_MASK              INTEL_MASK(21, 21)
> > > +# define MEDIA_SHARED_LOCAL_MEMORY_SIZE_SHIFT   16
> > > +# define MEDIA_SHARED_LOCAL_MEMORY_SIZE_MASK    INTEL_MASK(20, 16)
> > >  # define MEDIA_GPGPU_THREAD_COUNT_SHIFT         0
> > >  # define MEDIA_GPGPU_THREAD_COUNT_MASK          INTEL_MASK(7, 0)
> > >  # define GEN8_MEDIA_GPGPU_THREAD_COUNT_SHIFT    0
> > > diff --git a/src/mesa/drivers/dri/i965/gen7_cs_state.c b/src/mesa/drivers/dri/i965/gen7_cs_state.c
> > > index 2d7c04f..344ea5a 100644
> > > --- a/src/mesa/drivers/dri/i965/gen7_cs_state.c
> > > +++ b/src/mesa/drivers/dri/i965/gen7_cs_state.c
> > > @@ -164,8 +164,20 @@ brw_upload_cs_state(struct brw_context *brw)
> > >        SET_FIELD(threads, GEN8_MEDIA_GPGPU_THREAD_COUNT) :
> > >        SET_FIELD(threads, MEDIA_GPGPU_THREAD_COUNT);
> > >     assert(threads <= brw->max_cs_threads);
> > > +
> > > +   assert(prog_data->total_shared <= 64 * 1024);
> > > +   uint32_t slm_size = 0;
> > > +   if (prog_data->total_shared > 0) {
> > > +      /* slm_size is in 4k increments, but must be a power of 2. */
> > > +      slm_size = 4 * 1024;
> > > +      while (slm_size < 64 * 1024 && slm_size < prog_data->total_shared)
> > 
> > total_shared is guaranteed to be <= 64KB because of the assert above so
> > we should not need the "slm_size < 64 * 1024" part of the condition
> > here.
> 
> Well, in release builds, the assert won't help.

That's true

> This does point out that I ought to be checking this in
> brw_codegen_cs_prog, and refusing to generate a program if the size is
> more than 64KB. With that change, I agree that I don't need to check
> for 64KB in the while loop.

Yes, that sounds like a better idea.

> Thanks,
> 
> -Jordan
> 
> > 
> > Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>
> > 
> > > +         slm_size <<= 1;
> > > +      slm_size /= 4 * 1024;
> > > +   }
> > > +
> > >     desc[dw++] =
> > >        SET_FIELD(cs_prog_data->uses_barrier, MEDIA_BARRIER_ENABLE) |
> > > +      SET_FIELD(slm_size, MEDIA_SHARED_LOCAL_MEMORY_SIZE) |
> > >        media_threads;
> > >  
> > >     BEGIN_BATCH(4);
> > 
> > 
>