[Mesa-stable] [Mesa-dev] [PATCH] i965/fs: Fix hang on IVB and VLV with image format mismatch.

[Ian Romanick]

On 09/09/2015 05:30 AM, Francisco Jerez wrote:
> Ian Romanick <idr at 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.janes at intel.com>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91718
>>> CC: mesa-stable at 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.romanick at 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-stable at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-stable