[Mesa-dev] [PATCH] i965: perform 2 uploads with dual slot *64*PASSTHRU formats on gen<8

[Antia Puentes]

Hi,

the patch looks good to me. Some small comments:

On 29/01/18 01:26, Andres Gomez wrote:
> The emission of vertex attributes corresponding to dvec3 and dvec4
> vertex shader input variables was not correct when the <size> passed
> to the VertexAttribL* commands was <= 2.
>
> In 61a8a55f557 ("i965/gen8: Fix vertex attrib upload for dvec3/4
> shader inputs"), for gen8+ we needed to determine if the attrib was
> dual slot to emit 128 or 256-bit, independently of the VAO size.
>
> Similarly, for gen < 8 we also need to determine whether the attrib is
> dual slot to force the emission of 265-bits through 2 uploads.
                                                     ^^^
                                                     256-bits
> Additionally, we make use of the ISL_FORMAT_R32_FLOAT format in this
> second upload to fill these unspecified components with zeros, as we
> also do for gen8+.
>
> Fixes the following test on Haswell:
> KHR-GL46.vertex_attrib_binding.basic-inputL-case1
>
> Fixes: 75968a668e4 ("i965/gen7: expose OpenGL 4.2 on Haswell when
> supported")
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103006
> Cc: Alejandro Piñeiro <apinheiro at igalia.com>
> Cc: Juan A. Suarez Romero <jasuarez at igalia.com>
> Cc: Antia Puentes <apuentes at igalia.com>
> Cc: Rafael Antognolli <rafael.antognolli at intel.com>
> Cc: Kenneth Graunke <kenneth at whitecape.org>
> Signed-off-by: Andres Gomez <agomez at igalia.com>
> ---
>   src/mesa/drivers/dri/i965/genX_state_upload.c | 19 ++++++++++++++-----
>   1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/genX_state_upload.c b/src/mesa/drivers/dri/i965/genX_state_upload.c
> index aa4d64d08e2..d4c89c05b41 100644
> --- a/src/mesa/drivers/dri/i965/genX_state_upload.c
> +++ b/src/mesa/drivers/dri/i965/genX_state_upload.c
> @@ -364,11 +364,15 @@ is_passthru_format(uint32_t format)
>   }
>   
>   UNUSED static int
> -uploads_needed(uint32_t format)
> +uploads_needed(uint32_t format,
> +	       bool is_dual_slot)
>   {
>      if (!is_passthru_format(format))
>         return 1;
>   
> +   if (is_dual_slot)
> +      return 2;
> +
>      switch (format) {
>      case ISL_FORMAT_R64_PASSTHRU:
>      case ISL_FORMAT_R64G64_PASSTHRU:
> @@ -399,9 +403,11 @@ downsize_format_if_needed(uint32_t format,
>   
>      switch (format) {
>      case ISL_FORMAT_R64_PASSTHRU:
> -      return ISL_FORMAT_R32G32_FLOAT;
> +      return !upload ? ISL_FORMAT_R32G32_FLOAT
> +                     : ISL_FORMAT_R32_FLOAT;
>      case ISL_FORMAT_R64G64_PASSTHRU:
> -      return ISL_FORMAT_R32G32B32A32_FLOAT;
> +      return !upload ? ISL_FORMAT_R32G32B32A32_FLOAT
> +                     : ISL_FORMAT_R32_FLOAT;
>      case ISL_FORMAT_R64G64B64_PASSTHRU:
>         return !upload ? ISL_FORMAT_R32G32B32A32_FLOAT
>                        : ISL_FORMAT_R32G32_FLOAT;
> @@ -420,6 +426,8 @@ static int
>   upload_format_size(uint32_t upload_format)
>   {
>      switch (upload_format) {
> +   case ISL_FORMAT_R32_FLOAT:
> +      return 0;
I agree with Piñeiro about at least adding a comment here.
>      case ISL_FORMAT_R32G32_FLOAT:
>         return 2;
>      case ISL_FORMAT_R32G32B32A32_FLOAT:
> @@ -517,7 +525,7 @@ genX(emit_vertices)(struct brw_context *brw)
>         struct brw_vertex_element *input = brw->vb.enabled[i];
>         uint32_t format = brw_get_vertex_surface_type(brw, input->glarray);
>   
> -      if (uploads_needed(format) > 1)
> +      if (uploads_needed(format, input->is_dual_slot) > 1)
>            nr_elements++;
>      }
>   #endif
> @@ -613,7 +621,8 @@ genX(emit_vertices)(struct brw_context *brw)
>         uint32_t comp1 = VFCOMP_STORE_SRC;
>         uint32_t comp2 = VFCOMP_STORE_SRC;
>         uint32_t comp3 = VFCOMP_STORE_SRC;
> -      const unsigned num_uploads = GEN_GEN < 8 ? uploads_needed(format) : 1;
> +      const unsigned num_uploads = GEN_GEN < 8 ?
> +	 uploads_needed(format, input->is_dual_slot) : 1;
>   
>   #if GEN_GEN >= 8
>         /* From the BDW PRM, Volume 2d, page 588 (VERTEX_ELEMENT_STATE):
Thanks for fixing this.

Reviewed-by: Antia Puentes <apuentes at igalia.com>