Emil Velikov <emil.l.veli...@gmail.com> writes: > 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 ? > Nope, sorry, I've been on vacation and with intermittent connectivity since it was reviewed, I'll push the patch on Monday if no-one beats me to it.
> Thanks > Emil
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
