[Mesa-dev] [PATCH 10/20] i965: Include UBO parameter sizes in push constant parameters

[Francisco Jerez]

Abdiel Janulgue <abdiel.janulgue at linux.intel.com> writes:

> Now that we consider UBO constants as push constants, we need to include
> the sizes of the UBO's constant slots in the visitor's uniform slot sizes.
> This information is needed to properly pack vector constants tightly next to
> each other.
>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue at linux.intel.com>
> ---
>  src/mesa/drivers/dri/i965/brw_gs.c      |  1 +
>  src/mesa/drivers/dri/i965/brw_program.h |  3 +++
>  src/mesa/drivers/dri/i965/brw_vs.c      |  3 +++
>  src/mesa/drivers/dri/i965/brw_wm.c      | 22 ++++++++++++++++++++++
>  4 files changed, 29 insertions(+)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_gs.c b/src/mesa/drivers/dri/i965/brw_gs.c
> index 17e87b8..7641cc5 100644
> --- a/src/mesa/drivers/dri/i965/brw_gs.c
> +++ b/src/mesa/drivers/dri/i965/brw_gs.c
> @@ -72,6 +72,7 @@ brw_codegen_gs_prog(struct brw_context *brw,
>        rzalloc_array(NULL, struct brw_image_param, gs->NumImages);
>     c.prog_data.base.base.nr_params = param_count;
>     c.prog_data.base.base.nr_image_params = gs->NumImages;
> +   c.prog_data.base.base.nr_ubo_params = brw_count_ubo_params(gs);
>     c.prog_data.base.base.nr_gather_table = 0;
>     c.prog_data.base.base.gather_table =
>        rzalloc_size(NULL, sizeof(*c.prog_data.base.base.gather_table) *
> diff --git a/src/mesa/drivers/dri/i965/brw_program.h b/src/mesa/drivers/dri/i965/brw_program.h
> index 00e8f3f..20f5371 100644
> --- a/src/mesa/drivers/dri/i965/brw_program.h
> +++ b/src/mesa/drivers/dri/i965/brw_program.h
> @@ -182,6 +182,9 @@ void
>  brw_dump_ir(const char *stage, struct gl_shader_program *shader_prog,
>              struct gl_shader *shader, struct gl_program *prog);
>  
> +int
> +brw_count_ubo_params(struct gl_shader *fs);
> +
>  #ifdef __cplusplus
>  } /* extern "C" */
>  #endif
> diff --git a/src/mesa/drivers/dri/i965/brw_vs.c b/src/mesa/drivers/dri/i965/brw_vs.c
> index 8501796..1ec2bc9 100644
> --- a/src/mesa/drivers/dri/i965/brw_vs.c
> +++ b/src/mesa/drivers/dri/i965/brw_vs.c
> @@ -141,6 +141,9 @@ brw_codegen_vs_prog(struct brw_context *brw,
>                      stage_prog_data->nr_image_params);
>     stage_prog_data->nr_params = param_count;
>  
> +   stage_prog_data->nr_ubo_params = 0;

This assignment seems redundant, the prog data struct is already memset
to zero at the top of the function.

> +   if (vs)
> +      stage_prog_data->nr_ubo_params = brw_count_ubo_params(vs);

You could move this assignment into the other 'if (vs)' conditional
above.

>     stage_prog_data->nr_gather_table = 0;
>     stage_prog_data->gather_table =
>        rzalloc_size(NULL, sizeof(*stage_prog_data->gather_table) *
> diff --git a/src/mesa/drivers/dri/i965/brw_wm.c b/src/mesa/drivers/dri/i965/brw_wm.c
> index 204baa6..44efba0 100644
> --- a/src/mesa/drivers/dri/i965/brw_wm.c
> +++ b/src/mesa/drivers/dri/i965/brw_wm.c
> @@ -33,6 +33,7 @@
>  #include "main/framebuffer.h"
>  #include "program/prog_parameter.h"
>  #include "program/program.h"
> +#include "glsl/nir/nir_types.h"
>  #include "intel_mipmap_tree.h"
>  
>  #include "util/ralloc.h"
> @@ -149,6 +150,23 @@ brw_wm_prog_data_compare(const void *in_a, const void *in_b)
>     return true;
>  }
>  
> +int

The result of this function is always non-negative so I guess its return
type should be unsigned.

> +brw_count_ubo_params(struct gl_shader *shader)
> +{
> +   int nr_ubo = 0;

Same here.

> +   for (int i = 0; i < shader->NumUniformBlocks; i++) {
> +      for (int p = 0; p < shader->UniformBlocks[i].NumUniforms; p++) {
> +         const struct glsl_type *type = shader->UniformBlocks[i].Uniforms[p].Type;
> +         int array_sz = glsl_get_array_size(type);
> +         array_sz = MAX2(array_sz, 1);
> +         int components = glsl_get_components(glsl_get_type_without_array(type));
> +         nr_ubo += components * array_sz;

Why don't you just do 'nr_ubo += shader->UniformBlocks[i].Uniforms[p].Type->component_slots();'?

> +      }
> +   }
> +
> +   return nr_ubo;
> +}
> +

I guess brw_shader.cpp would be a more appropriate place for this
function rather than brw_wm.c since it doesn't do anything specific to
fragment shaders.  If anything you'd be able to use the C++ methods of
glsl_type and the last two patches would become unnecessary.

>  /**
>   * All Mesa program -> GPU code generation goes through this function.
>   * Depending on the instructions used (i.e. flow control instructions)
> @@ -208,6 +226,10 @@ brw_codegen_wm_prog(struct brw_context *brw,
>                      prog_data.base.nr_image_params);
>     prog_data.base.nr_params = param_count;
>  
> +   prog_data.base.nr_ubo_params = 0;
> +   if (fs)
> +      prog_data.base.nr_ubo_params = brw_count_ubo_params(fs);
> +
Same comments as for brw_codegen_vs_prog().  And I guess you're missing
an analogous change for compute shaders?

>     prog_data.base.nr_gather_table = 0;
>     prog_data.base.gather_table =
>        rzalloc_size(NULL, sizeof(*prog_data.base.gather_table) *
> -- 
> 1.9.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20151023/0e990406/attachment.sig>