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

[Francisco Jerez]

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.

>>  
>>              /* 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150909/7915e9f6/attachment.sig>