[Mesa-dev,62/70] i965: Prevent coordinate overflow in intel_emit_linear_blit

Submitted by Chris Wilson on Aug. 7, 2015, 8:14 p.m.

Details

Message ID 1438978454-30139-63-git-send-email-chris@chris-wilson.co.uk
State Accepted
Commit d38a5601068ae1d923efece8f28757777f4474e4
Headers show

Not browsing as part of any series.

Commit Message

Chris Wilson Aug. 7, 2015, 8:14 p.m.
Fixes regression from
commit 8c17d53823c77ac1c56b0548e4e54f69a33285f1
Author: Kenneth Graunke <kenneth@whitecape.org>
Date:   Wed Apr 15 03:04:33 2015 -0700

    i965: Make intel_emit_linear_blit handle Gen8+ alignment restrictions.

which adjusted the coordinates to be relative to the nearest cacheline.
However, this then offsets the coordinates by up to 63 and this may then
cause them to overflow the BLT limits. For the well aligned large
transfer case, we can use 32bpp pixels and so reduce the coordinates by
4 (versus the current 8bpp pixels). We also have to be more careful
doing the last line just in case it may exceed the coordinate limit.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90734
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Kenneth Graunke <kenneth@whitecape.org>
Cc: Ian Romanick <ian.d.romanick@intel.com>
Cc: Anuj Phogat <anuj.phogat@gmail.com>
Cc: mesa-stable@lists.freedesktop.org
---
 src/mesa/drivers/dri/i965/intel_blit.c | 72 ++++++++++++++++------------------
 1 file changed, 34 insertions(+), 38 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/mesa/drivers/dri/i965/intel_blit.c b/src/mesa/drivers/dri/i965/intel_blit.c
index 4d1defc..845fcde 100644
--- a/src/mesa/drivers/dri/i965/intel_blit.c
+++ b/src/mesa/drivers/dri/i965/intel_blit.c
@@ -765,47 +765,43 @@  intel_emit_linear_blit(struct brw_context *brw,
    int16_t src_x, dst_x;
    bool ok;
 
-   /* The pitch given to the GPU must be DWORD aligned, and
-    * we want width to match pitch. Max width is (1 << 15 - 1),
-    * rounding that down to the nearest DWORD is 1 << 15 - 4
-    */
-   pitch = ROUND_DOWN_TO(MIN2(size, (1 << 15) - 1), 4);
-   height = (pitch == 0) ? 1 : size / pitch;
-   src_x = src_offset % 64;
-   dst_x = dst_offset % 64;
-   ok = intelEmitCopyBlit(brw, 1,
-			  pitch, src_bo, src_offset - src_x, I915_TILING_NONE,
-                          INTEL_MIPTREE_TRMODE_NONE,
-			  pitch, dst_bo, dst_offset - dst_x, I915_TILING_NONE,
-                          INTEL_MIPTREE_TRMODE_NONE,
-			  src_x, 0, /* src x/y */
-			  dst_x, 0, /* dst x/y */
-			  pitch, height, /* w, h */
-			  GL_COPY);
-   if (!ok)
-      _mesa_problem(ctx, "Failed to linear blit %dx%d\n", pitch, height);
-
-   src_offset += pitch * height;
-   dst_offset += pitch * height;
-   src_x = src_offset % 64;
-   dst_x = dst_offset % 64;
-   size -= pitch * height;
-   assert (size < (1 << 15));
-   pitch = ALIGN(size, 4);
-
-   if (size != 0) {
+   do {
+      /* The pitch given to the GPU must be DWORD aligned, and
+       * we want width to match pitch. Max width is (1 << 15 - 1),
+       * rounding that down to the nearest DWORD is 1 << 15 - 4
+       */
+      pitch = ROUND_DOWN_TO(MIN2(size, (1 << 15) - 64), 4);
+      height = (size < pitch || pitch == 0) ? 1 : size / pitch;
+
+      src_x = src_offset % 64;
+      dst_x = dst_offset % 64;
+      pitch = ALIGN(MIN2(size, (1 << 15) - 64), 4);
+      assert(src_x + pitch < 1 << 15);
+      assert(dst_x + pitch < 1 << 15);
+
       ok = intelEmitCopyBlit(brw, 1,
-			     pitch, src_bo, src_offset - src_x, I915_TILING_NONE,
+                             pitch, src_bo, src_offset - src_x, I915_TILING_NONE,
                              INTEL_MIPTREE_TRMODE_NONE,
-			     pitch, dst_bo, dst_offset - dst_x, I915_TILING_NONE,
+                             pitch, dst_bo, dst_offset - dst_x, I915_TILING_NONE,
                              INTEL_MIPTREE_TRMODE_NONE,
-			     src_x, 0, /* src x/y */
-			     dst_x, 0, /* dst x/y */
-			     size, 1, /* w, h */
-			     GL_COPY);
-      if (!ok)
-         _mesa_problem(ctx, "Failed to linear blit %dx%d\n", size, 1);
-   }
+                             src_x, 0, /* src x/y */
+                             dst_x, 0, /* dst x/y */
+                             MIN2(size, pitch), height, /* w, h */
+                             GL_COPY);
+      if (!ok) {
+         _mesa_problem(ctx, "Failed to linear blit %dx%d\n",
+                       MIN2(size, pitch), height);
+         return;
+      }
+
+      pitch *= height;
+      if (size <= pitch)
+         return;
+
+      src_offset += pitch;
+      dst_offset += pitch;
+      size -= pitch;
+   } while (1);
 }
 
 /**

Comments

On Fri, Aug 7, 2015 at 1:14 PM, Chris Wilson <chris@chris-wilson.co.uk>
wrote:

> Fixes regression from
> commit 8c17d53823c77ac1c56b0548e4e54f69a33285f1
> Author: Kenneth Graunke <kenneth@whitecape.org>
> Date:   Wed Apr 15 03:04:33 2015 -0700
>
>     i965: Make intel_emit_linear_blit handle Gen8+ alignment restrictions.
>
> which adjusted the coordinates to be relative to the nearest cacheline.
> However, this then offsets the coordinates by up to 63 and this may then
> cause them to overflow the BLT limits. For the well aligned large
> transfer case, we can use 32bpp pixels and so reduce the coordinates by
> 4 (versus the current 8bpp pixels). We also have to be more careful
> doing the last line just in case it may exceed the coordinate limit.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90734
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Kenneth Graunke <kenneth@whitecape.org>
> Cc: Ian Romanick <ian.d.romanick@intel.com>
> Cc: Anuj Phogat <anuj.phogat@gmail.com>
> Cc: mesa-stable@lists.freedesktop.org
> ---
>  src/mesa/drivers/dri/i965/intel_blit.c | 72
> ++++++++++++++++------------------
>  1 file changed, 34 insertions(+), 38 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/intel_blit.c
> b/src/mesa/drivers/dri/i965/intel_blit.c
> index 4d1defc..845fcde 100644
> --- a/src/mesa/drivers/dri/i965/intel_blit.c
> +++ b/src/mesa/drivers/dri/i965/intel_blit.c
> @@ -765,47 +765,43 @@ intel_emit_linear_blit(struct brw_context *brw,
>     int16_t src_x, dst_x;
>     bool ok;
>
> -   /* The pitch given to the GPU must be DWORD aligned, and
> -    * we want width to match pitch. Max width is (1 << 15 - 1),
> -    * rounding that down to the nearest DWORD is 1 << 15 - 4
> -    */
> -   pitch = ROUND_DOWN_TO(MIN2(size, (1 << 15) - 1), 4);
> -   height = (pitch == 0) ? 1 : size / pitch;
> -   src_x = src_offset % 64;
> -   dst_x = dst_offset % 64;
> -   ok = intelEmitCopyBlit(brw, 1,
> -                         pitch, src_bo, src_offset - src_x,
> I915_TILING_NONE,
> -                          INTEL_MIPTREE_TRMODE_NONE,
> -                         pitch, dst_bo, dst_offset - dst_x,
> I915_TILING_NONE,
> -                          INTEL_MIPTREE_TRMODE_NONE,
> -                         src_x, 0, /* src x/y */
> -                         dst_x, 0, /* dst x/y */
> -                         pitch, height, /* w, h */
> -                         GL_COPY);
> -   if (!ok)
> -      _mesa_problem(ctx, "Failed to linear blit %dx%d\n", pitch, height);
> -
> -   src_offset += pitch * height;
> -   dst_offset += pitch * height;
> -   src_x = src_offset % 64;
> -   dst_x = dst_offset % 64;
> -   size -= pitch * height;
> -   assert (size < (1 << 15));
> -   pitch = ALIGN(size, 4);
> -
> -   if (size != 0) {
> +   do {
> +      /* The pitch given to the GPU must be DWORD aligned, and
> +       * we want width to match pitch. Max width is (1 << 15 - 1),
> +       * rounding that down to the nearest DWORD is 1 << 15 - 4
> +       */
> +      pitch = ROUND_DOWN_TO(MIN2(size, (1 << 15) - 64), 4);
> +      height = (size < pitch || pitch == 0) ? 1 : size / pitch;
> +
> +      src_x = src_offset % 64;
> +      dst_x = dst_offset % 64;
> +      pitch = ALIGN(MIN2(size, (1 << 15) - 64), 4);
> +      assert(src_x + pitch < 1 << 15);
> +      assert(dst_x + pitch < 1 << 15);
> +
>        ok = intelEmitCopyBlit(brw, 1,
> -                            pitch, src_bo, src_offset - src_x,
> I915_TILING_NONE,
> +                             pitch, src_bo, src_offset - src_x,
> I915_TILING_NONE,
>                               INTEL_MIPTREE_TRMODE_NONE,
> -                            pitch, dst_bo, dst_offset - dst_x,
> I915_TILING_NONE,
> +                             pitch, dst_bo, dst_offset - dst_x,
> I915_TILING_NONE,
>                               INTEL_MIPTREE_TRMODE_NONE,
> -                            src_x, 0, /* src x/y */
> -                            dst_x, 0, /* dst x/y */
> -                            size, 1, /* w, h */
> -                            GL_COPY);
> -      if (!ok)
> -         _mesa_problem(ctx, "Failed to linear blit %dx%d\n", size, 1);
> -   }
> +                             src_x, 0, /* src x/y */
> +                             dst_x, 0, /* dst x/y */
> +                             MIN2(size, pitch), height, /* w, h */
> +                             GL_COPY);
> +      if (!ok) {
> +         _mesa_problem(ctx, "Failed to linear blit %dx%d\n",
> +                       MIN2(size, pitch), height);
> +         return;
> +      }
> +
> +      pitch *= height;
> +      if (size <= pitch)
> +         return;
> +
> +      src_offset += pitch;
> +      dst_offset += pitch;
> +      size -= pitch;
> +   } while (1);
>  }
>
>  /**
> --
> 2.5.0
>
> _______________________________________________
> mesa-stable mailing list
> mesa-stable@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-stable
>

I don't see any new changes as compared to the original patch you sent
earlier.

Reviewed-by: Anuj Phogat <anuj.phogat@gmail.com>
On Fri, Aug 07, 2015 at 05:22:27PM -0700, Anuj Phogat wrote:
>    I don't see any new changes as compared to the original patch you sent
>    earlier.
>    Reviewed-by: Anuj Phogat <[11]anuj.phogat@gmail.com>

Sorry, there were none, I just forgot about pushing to the head of the
queue and commiting it.

Thanks for the review and gentle poke.
-Chris