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

[Francisco Jerez]

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

> Hi Francisco,
>
> On 9 September 2015 at 18:04, Ian Romanick <idr at freedesktop.org> wrote:
>> 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>
>>
> 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
-------------- 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/20150926/6e06d62d/attachment.sig>