[Mesa-dev] [PATCH v2 11/20] i965/cs: Add brw_cs_prog_data, brw_cs_prog_key and brw_context::cs.

[Jordan Justen]

On 2015-04-24 23:42:58, Kenneth Graunke wrote:
> On Friday, April 24, 2015 04:33:03 PM Jordan Justen wrote:
> > From: Paul Berry <stereotype441 at gmail.com>
> > 
> > jordan.l.justen at intel.com:
> >  * Added brw_cs_prog_key structure
> >  * Added brw_cs_prog_data::dispatch_grf_start_reg_16
> >  * Added brw_cs_prog_data::no_8
> >  * Added brw_cs_prog_data::local_size
> >  * Added brw_cs_prog_data::simd_size
> > 
> > Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>
> > Reviewed-by: Kristian Høgsberg <krh at bitplanet.net>
> > ---
> >  src/mesa/drivers/dri/i965/brw_context.h | 17 ++++++++++++
> >  src/mesa/drivers/dri/i965/brw_cs.h      | 46 +++++++++++++++++++++++++++++++++
> >  2 files changed, 63 insertions(+)
> >  create mode 100644 src/mesa/drivers/dri/i965/brw_cs.h
> > 
> > diff --git a/src/mesa/drivers/dri/i965/brw_context.h b/src/mesa/drivers/dri/i965/brw_context.h
> > index 134040e..07847cc 100644
> > --- a/src/mesa/drivers/dri/i965/brw_context.h
> > +++ b/src/mesa/drivers/dri/i965/brw_context.h
> > @@ -148,6 +148,7 @@ struct brw_vs_prog_key;
> >  struct brw_vue_prog_key;
> >  struct brw_wm_prog_key;
> >  struct brw_wm_prog_data;
> > +struct brw_cs_prog_data;
> >  
> >  enum brw_pipeline {
> >     BRW_RENDER_PIPELINE,
> > @@ -429,6 +430,18 @@ struct brw_wm_prog_data {
> >     int urb_setup[VARYING_SLOT_MAX];
> >  };
> >  
> > +/* Note: brw_cs_prog_data_compare() must be updated when adding fields to this
> > + * struct!
> > + */
> > +struct brw_cs_prog_data {
> > +   struct brw_stage_prog_data base;
> > +
> > +   GLuint dispatch_grf_start_reg_16;
> > +   bool no_8;
> > +   unsigned local_size[3];
> > +   unsigned simd_size;
> > +};
> 
> Hey Jordan,
> 
> Could you clarify something for me?  For fragment shaders, we specify
> SIMD8 and SIMD16 programs, and let the hardware pick which one it thinks
> is more appropriate.
> 
> For compute, we may have to use SIMD8/SIMD16/SIMD32, but we only specify
> one of those programs.  Am I right?
> 
> If so, I suspect we don't need the no_8 flag - we should just generate
> the one program we need, and set simd_size to indicate whether it's
> SIMD8/SIMD16/SIMD32.  (At least, I assume that's what this is for...)

Yeah, you are right. We can still support INTEL_DEBUG=no8 at program
generation time, but we shouldn't need this field in brw_cs_prog_data
since there will only be one program, and simd_size will tell us the
size.

-Jordan

> > +
> >  /**
> >   * Enum representing the i965-specific vertex results that don't correspond
> >   * exactly to any element of gl_varying_slot.  The values of this enum are
> > @@ -1361,6 +1374,10 @@ struct brw_context
> >        uint32_t fast_clear_op;
> >     } wm;
> >  
> > +   struct {
> > +      struct brw_stage_state base;
> > +      struct brw_cs_prog_data *prog_data;
> > +   } cs;
> >  
> >     struct {
> >        uint32_t state_offset;
> > diff --git a/src/mesa/drivers/dri/i965/brw_cs.h b/src/mesa/drivers/dri/i965/brw_cs.h
> > new file mode 100644
> > index 0000000..0e9e65a
> > --- /dev/null
> > +++ b/src/mesa/drivers/dri/i965/brw_cs.h
> > @@ -0,0 +1,46 @@
> > +/*
> > + * Copyright © 2014 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the "Software"),
> > + * to deal in the Software without restriction, including without limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the next
> > + * paragraph) shall be included in all copies or substantial portions of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> > + * DEALINGS IN THE SOFTWARE.
> > + */
> > +
> > +
> > +#ifndef BRW_CS_H
> > +#define BRW_CS_H
> > +
> > +#include "brw_program.h"
> > +
> > +struct brw_cs_prog_key {
> > +   GLuint program_string_id:32;
> 
> We should just do:
> 
>    uint32_t program_string_id;
> 
> or unsigned, if you prefer...
> 
> > +   struct brw_sampler_prog_key_data tex;
> > +};
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +void
> > +brw_upload_cs_prog(struct brw_context *brw);
> > +
> > +#ifdef __cplusplus
> > +}
> > +#endif
> > +
> > +#endif /* BRW_CS_H */
> >