On Mon, 2015-07-27 at 16:01 +0300, Francisco Jerez wrote:
> This fixes the spec@arb_shader_image_load_store@invalid index bounds
> piglit tests on IVB, which were causing a GPU hang and then a crash
> due to the invalid binding table index result of the array index
> calculation. Other generations seem to behave sensibly when an
> invalid surface is provided so it doesn't look like we need to care.
> ---
> src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 25 +++++++++++++++++++++----
> 1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> index 1671595..b69e96b 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> @@ -1216,14 +1216,31 @@ fs_visitor::get_nir_image_deref(const nir_deref_var
> *deref)
> nir_deref_as_array(deref->deref.child);
> assert(deref->deref.child->deref_type == nir_deref_type_array &&
> deref_array->deref.child == NULL);
> + const unsigned size = glsl_get_length(deref->var->type);
> + const unsigned base = MIN2(deref_array->base_offset, size - 1);
>
> - image = offset(image, bld,
> - deref_array->base_offset * BRW_IMAGE_PARAM_SIZE);
> + image = offset(image, bld, base * BRW_IMAGE_PARAM_SIZE);
>
> if (deref_array->deref_array_type == nir_deref_array_type_indirect) {
> fs_reg *tmp = new(mem_ctx) fs_reg(vgrf(glsl_type::int_type));
> - bld.MUL(*tmp, get_nir_src(deref_array->indirect),
> - fs_reg(BRW_IMAGE_PARAM_SIZE));
> +
> + 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),
> + fs_reg(size - base - 1), BRW_CONDITIONAL_L);
I know this was just pushed but I'm working on AoA support for this, and notic
ed something. Isn't base always going to be 0 in the above line? As it will
only be set to another value for direct indexing.
> + } else {
> + bld.MOV(*tmp, get_nir_src(deref_array->indirect));
> + }
> +
> + bld.MUL(*tmp, *tmp, fs_reg(BRW_IMAGE_PARAM_SIZE));
> image.reladdr = tmp;
> }
> }
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
