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

[Iago Toral]

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.

>     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>


>     next_binding_table_offset =