[Mesa-dev] [PATCH] i965: Drop render_target_start from binding table struct.

[Kenneth Graunke]

On Thursday, January 18, 2018 11:47:33 PM PST Iago Toral wrote:
> On Thu, 2018-01-18 at 15:49 -0800, Kenneth Graunke wrote:
> > We have to start render targets at binding table index 0 in order to
> > use
> > headerless FB write messages, and in fact already assume this in a
> > bunch
> > of places in the code.  Let's finish that off, and not bother storing
> > 0
> > in a struct to pretend to add it in a few places.
> > ---
> >  src/intel/blorp/blorp.c                 | 1 -
> >  src/intel/compiler/brw_compiler.h       | 1 -
> >  src/intel/compiler/brw_fs_generator.cpp | 6 ++----
> >  src/mesa/drivers/dri/i965/brw_wm.c      | 1 -
> >  4 files changed, 2 insertions(+), 7 deletions(-)
> > 
> > diff --git a/src/intel/blorp/blorp.c b/src/intel/blorp/blorp.c
> > index e8a2c6135f5..b7238306eb1 100644
> > --- a/src/intel/blorp/blorp.c
> > +++ b/src/intel/blorp/blorp.c
> > @@ -177,7 +177,6 @@ blorp_compile_fs(struct blorp_context *blorp,
> > void *mem_ctx,
> >     wm_prog_data->base.param = NULL;
> >  
> >     /* BLORP always just uses the first two binding table entries */
> > -   wm_prog_data->biable.render_target_start =
> > BLORP_RENDERBUFFER_BT_INDEX;
> >     wm_prog_data->base.binding_table.texture_start =
> > BLORP_TEXTURE_BT_INDEX;
> 
> Since the comment says that blorp uses the first two entries but it
> then only assigns one, maybe it would be nice to update the comment
> above to state that we assume render targets start at binding table
> index 0.

Thanks, updated to:

   /* BLORP always uses the first two binding table entries:
    * - Surface 0 is the render target (which always start from 0)
    * - Surface 1 is the source texture
    */

> >     nir = brw_preprocess
> > Wouldn't it be better to keep the assert? We still needs
> > render_target_start to be 0_nir(compiler, nir);
> > diff --git a/src/intel/compiler/brw_compiler.h
> > b/src/intel/compiler/brw_compiler.h
> > index 0060c381c0d..b1086bbcee5 100644
> > --- a/src/intel/compiler/brw_compiler.h
> > +++ b/src/intel/compiler/brw_compiler.h
> > @@ -681,7 +681,6 @@ struct brw_wm_prog_data {
> >        /** @{
> >         * surface indices the WM-specific surfaces
> >         */
> > -      uint32_t render_target_start;
> >        uint32_t render_target_read_start;
> >        /** @} */
> >     } binding_table;
> > diff --git a/src/intel/compiler/brw_fs_generator.cpp
> > b/src/intel/compiler/brw_fs_generator.cpp
> > index 91bf0643084..cd5be054f69 100644
> > --- a/src/intel/compiler/brw_fs_generator.cpp
> > +++ b/src/intel/compiler/brw_fs_generator.cpp
> > @@ -287,8 +287,6 @@ fs_generator::fire_fb_write(fs_inst *inst,
> >      * messages set "Render Target Index" to 0.  Using a different
> > binding
> >      * table index would make it impossible to use headerless
> > messages.
> >      */
> > -   assert(prog_data->binding_table.render_target_start == 0);
> > -
> >     const uint32_t surf_index = inst->target;
> >  
> >     bool last_render_target = inst->eot ||
> > @@ -427,8 +425,8 @@ fs_generator::generate_fb_read(fs_inst *inst,
> > struct brw_reg dst,
> >  {
> >     assert(inst->size_written % REG_SIZE == 0);
> >     struct brw_wm_prog_data *prog_data = brw_wm_prog_data(this-
> > >prog_data);
> > -   const unsigned surf_index =
> > -      prog_data->binding_table.render_target_start + inst->target;
> > +   /* We assume that render targets start at binding table index 0.
> > */
> > +   const unsigned surf_index = inst->target;
> >  
> >     gen9_fb_READ(p, dst, payload, surf_index,
> >                  inst->header_size, inst->size_written / REG_SIZE,
> > diff --git a/src/mesa/drivers/dri/i965/brw_wm.c
> > b/src/mesa/drivers/dri/i965/brw_wm.c
> > index 08bacebd571..755a76eec71 100644
> > --- a/src/mesa/drivers/dri/i965/brw_wm.c
> > +++ b/src/mesa/drivers/dri/i965/brw_wm.c
> > @@ -51,7 +51,6 @@ assign_fs_binding_table_offsets(const struct
> > gen_device_info *devinfo,
> >     /* If there are no color regions, we still perform an FB write to
> > a null
> >      * renderbuffer, which we place at surface index 0.
> >      */
> > -   prog_data->binding_table.render_target_start =
> > next_binding_table_offset;
> >     next_binding_table_offset += MAX2(key->nr_color_regions, 1);
> 
> Since we are no longer assigning next_binding_table_offset with value
> of  0, we might as well drop the initialization to that value (or just
> initialize the variable directly to the value right after the line you
> remove here).
> 
> Either way:
> Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>

Good call, updated to:

   /* Render targets implicitly start at surface index 0.  Even if there are
    * no color regions, we still perform an FB write to a null render target,
    * which will be surface 0.
    */
   uint32_t next_binding_table_offset = MAX2(key->nr_color_regions, 1);

and pushed:

To ssh://git.freedesktop.org/git/mesa/mesa
   a9bb067e27c..60f15477dad  master -> master

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180122/c428d21a/attachment.sig>