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

Submitted by Francisco Jerez on Sept. 3, 2015, 1:03 p.m.

Details

Message ID 1441285417-18012-1-git-send-email-currojerez@riseup.net
State Accepted
Commit b61292296bd7e1876fdb64725a783a7e96f6c4c1
Headers show

Not browsing as part of any series.

Commit Message

Francisco Jerez Sept. 3, 2015, 1:03 p.m.
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@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91718
CC: mesa-stable@lists.freedesktop.org
---
 .../drivers/dri/i965/brw_fs_surface_builder.cpp    | 42 +++++++++++++++++++---
 1 file changed, 38 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

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)
       {
          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));
 
             /* 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 =

Comments

This fixes a GPU hang on IVB, which is triggered with
piglit.spec.arb_shader_image_load_store.invalid

Tested-by: Mark Janes <mark.a.janes@intel.com>

Francisco Jerez <currojerez@riseup.net> writes:

> 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@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91718
> CC: mesa-stable@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)
>        {
>           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));
>  
>              /* 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 =
> -- 
> 2.4.6
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@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91718
> CC: mesa-stable@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).

>        {
>           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?

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

On 9 September 2015 at 18:04, Ian Romanick <idr@freedesktop.org> wrote:
> On 09/09/2015 05:30 AM, Francisco Jerez wrote:
>> Ian Romanick <idr@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@intel.com>
>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91718
>>>> CC: mesa-stable@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@intel.com>
>
Considering Ian's r-b, are there any obstacles why this hasn't landed
in master yet ?

Thanks
Emil
Emil Velikov <emil.l.velikov@gmail.com> writes:

> Hi Francisco,
>
> On 9 September 2015 at 18:04, Ian Romanick <idr@freedesktop.org> wrote:
>> On 09/09/2015 05:30 AM, Francisco Jerez wrote:
>>> Ian Romanick <idr@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@intel.com>
>>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91718
>>>>> CC: mesa-stable@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@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