Hi Francisco, On 9 September 2015 at 18:04, Ian Romanick <i...@freedesktop.org> wrote: > On 09/09/2015 05:30 AM, Francisco Jerez wrote: >> Ian Romanick <i...@freedesktop.org> writes: >> >>> On 09/03/2015 06:03 AM, Francisco Jerez wrote: >>>> IVB and VLV hang sporadically when an untyped surface read or write >>>> message is used to access a surface of format other than RAW, as may >>>> happen when there is a mismatch between the format qualifier of the >>>> image uniform and the format of the actual image bound to the >>>> pipeline. According to the spec this condition gives undefined >>>> results but may not lead to program termination (which is one of the >>>> possible outcomes of the hang). Fix it by checking at runtime whether >>>> the surface is of the right type. >>>> >>>> Fixes the "arb_shader_image_load_store.invalid/format mismatch" piglit >>>> subtest. >>>> >>>> Reported-by: Mark Janes <mark.a.ja...@intel.com> >>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91718 >>>> CC: mesa-sta...@lists.freedesktop.org >>>> --- >>>> .../drivers/dri/i965/brw_fs_surface_builder.cpp | 42 >>>> +++++++++++++++++++--- >>>> 1 file changed, 38 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_surface_builder.cpp >>>> b/src/mesa/drivers/dri/i965/brw_fs_surface_builder.cpp >>>> index f60afc9..57ce87f 100644 >>>> --- a/src/mesa/drivers/dri/i965/brw_fs_surface_builder.cpp >>>> +++ b/src/mesa/drivers/dri/i965/brw_fs_surface_builder.cpp >>>> @@ -313,12 +313,42 @@ namespace { >>>> >>>> namespace image_validity { >>>> /** >>>> + * Check whether the bound image is suitable for untyped access. >>>> + */ >>>> + brw_predicate >>>> + emit_untyped_image_check(const fs_builder &bld, const fs_reg &image, >>>> + brw_predicate pred) >>>> + { >>>> + const brw_device_info *devinfo = bld.shader->devinfo; >>>> + const fs_reg stride = offset(image, bld, >>>> BRW_IMAGE_PARAM_STRIDE_OFFSET); >>>> + >>>> + if (devinfo->gen == 7 && !devinfo->is_haswell) { >>>> + /* Check whether the first stride component (i.e. the Bpp >>>> value) >>>> + * is greater than four, what on Gen7 indicates that a >>>> surface of >>>> + * type RAW has been bound for untyped access. Reading or >>>> writing >>>> + * to a surface of type other than RAW using untyped surface >>>> + * messages causes a hang on IVB and VLV. >>>> + */ >>>> + set_predicate(pred, >>>> + bld.CMP(bld.null_reg_ud(), stride, fs_reg(4), >>>> + BRW_CONDITIONAL_G)); >>>> + >>>> + return BRW_PREDICATE_NORMAL; >>>> + } else { >>>> + /* More recent generations handle the format mismatch >>>> + * gracefully. >>>> + */ >>>> + return pred; >>>> + } >>>> + } >>>> + >>>> + /** >>>> * Check whether there is an image bound at the given index and >>>> write >>>> * the comparison result to f0.0. Returns an appropriate >>>> predication >>>> * mode to use on subsequent image operations. >>>> */ >>>> brw_predicate >>>> - emit_surface_check(const fs_builder &bld, const fs_reg &image) >>>> + emit_typed_atomic_check(const fs_builder &bld, const fs_reg &image) >>> >>> This change seems spurious (and also reasonable). >>> >> The problem is that this patch introduces a new kind of surface check >> applicable to untyped surface reads and writes only, so it would have >> been confusing to keep the other surface check which is applicable to >> atomics only with its previous rather unspecific name. >> >>>> { >>>> const brw_device_info *devinfo = bld.shader->devinfo; >>>> const fs_reg size = offset(image, bld, >>>> BRW_IMAGE_PARAM_SIZE_OFFSET); >>>> @@ -895,7 +925,9 @@ namespace brw { >>>> * surface read on the result, >>>> */ >>>> const brw_predicate pred = >>>> - emit_bounds_check(bld, image, saddr, dims); >>>> + emit_untyped_image_check(bld, image, >>>> + emit_bounds_check(bld, image, >>>> + saddr, dims)); >>> >>> These appear to be the only two users of emit_bounds_check... shouldn't >>> the bounds still be tested? >>> >> Yes, they are still. > > Ah... I completely missed that emit_bounds_check was moved into a > parameter of the call to emit_untyped_image_check. > > Reviewed-by: Ian Romanick <ian.d.roman...@intel.com> > Considering Ian's r-b, are there any obstacles why this hasn't landed in master yet ?
Thanks Emil _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
