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>
>>>
>>> /* and they don't know about surface coordinates, we need to
>>> * convert them to a raw memory offset.
>>> @@ -1041,7 +1073,9 @@ namespace brw {
>>> * the surface write 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));
>>>
>>> /* and, phew, they don't know about surface coordinates, we
>>> * need to convert them to a raw memory offset.
>>> @@ -1072,7 +1106,7 @@ namespace brw {
>>> using namespace image_coordinates;
>>> using namespace surface_access;
>>> /* Avoid performing an atomic operation on an unbound surface. */
>>> - const brw_predicate pred = emit_surface_check(bld, image);
>>> + const brw_predicate pred = emit_typed_atomic_check(bld, image);
>>>
>>> /* Transform the image coordinates into actual surface
>>> coordinates. */
>>> const fs_reg saddr =
>>
>> _______________________________________________
>> mesa-stable mailing list
>> mesa-sta...@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-stable
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
