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

[Emil Velikov]

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 ?

Thanks
Emil