[Mesa-dev] [PATCH] i965/fs: Don't allow OOB array access of images

[Francisco Jerez]

Jason Ekstrand <jason at jlekstrand.net> writes:

> We have had a guard against OOB array access of images on IVB for a long
> time, but it can actually cause hangs on any GPU generation.  This can
> happen due to getting an untyped SURFACE_STATE for a typed message or
> vice-versa.

Are you sure about that?  AFAIK the
"arb_shader_image_load_store/invalid/format mismatch" subtest already
leads to a typed/untyped mismatch on HSW and I'm not aware of it causing
hangs -- Not saying that the fix below is wrong, the code looks
reasonable but I'm skeptical that the reason for the hang is simply a
surface format mismatch.

> We didn't used to hit this with the piglit test on anything
> other than IVB because the OOB in the test would cause us to go past the
> top of the push constant UBO and we would get a surface index of 0 which is

You probably meant the pull constant buffer?

> was always a valid surface.  Now that we're pushing small arrays, we can
> end up grabbing garbage from the GRF and going to some random index which
> causes a hang.  The solution is to just do the bounds check on all
> hardware.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94944
> ---
>  src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 26 +++++++++++---------------
>  1 file changed, 11 insertions(+), 15 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> index c16f1ed..e775049 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> @@ -1272,21 +1272,17 @@ fs_visitor::get_nir_image_deref(const nir_deref_var *deref)
>        if (deref_array->deref_array_type == nir_deref_array_type_indirect) {
>           fs_reg tmp = vgrf(glsl_type::uint_type);
>  
> -         if (devinfo->gen == 7 && !devinfo->is_haswell) {
> -            /* IVB hangs when trying to access an invalid surface index with
> -             * the dataport.  According to the spec "if the index used to
> -             * select an individual element is negative or greater than or
> -             * equal to the size of the array, the results of the operation
> -             * are undefined but may not lead to termination" -- which is one
> -             * of the possible outcomes of the hang.  Clamp the index to
> -             * prevent access outside of the array bounds.
> -             */
> -            bld.emit_minmax(tmp, retype(get_nir_src(deref_array->indirect),
> -                                        BRW_REGISTER_TYPE_UD),
> -                            brw_imm_ud(size - base - 1), BRW_CONDITIONAL_L);
> -         } else {
> -            bld.MOV(tmp, get_nir_src(deref_array->indirect));
> -         }
> +         /* Accessing an invalid surface index with the dataport can result
> +          * in a hang.  According to the spec "if the index used to
> +          * select an individual element is negative or greater than or
> +          * equal to the size of the array, the results of the operation
> +          * are undefined but may not lead to termination" -- which is one
> +          * of the possible outcomes of the hang.  Clamp the index to
> +          * prevent access outside of the array bounds.
> +          */
> +         bld.emit_minmax(tmp, retype(get_nir_src(deref_array->indirect),
> +                                     BRW_REGISTER_TYPE_UD),
> +                         brw_imm_ud(size - base - 1), BRW_CONDITIONAL_L);
>  
>           indirect_max += element_size * (tail->type->length - 1);
>  
> -- 
> 2.5.0.400.gff86faf
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://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: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160415/8eae15c4/attachment-0001.sig>