[Mesa-stable] [PATCH] i965: Tell intel_get_memcpy() which direction the memcpy() is going.

Jason Ekstrand jason at jlekstrand.net
Wed Mar 4 16:28:11 PST 2015


On Wed, Mar 4, 2015 at 4:19 PM, Matt Turner <mattst88 at gmail.com> wrote:

> The SSSE3 swizzling code was written for fast uploads to the GPU and
> assumed the destination was always 16-byte aligned. When we began using
> this code for fast downloads as well we didn't do anything to account
> for the fact that the destination pointer given by glReadPixels() or
> glGetTexImage() is not guaranteed to be suitably aligned.
>
> With SSSE3 enabled (at compile-time), some applications would crash when
> an SSE aligned-store instruction tried to store to an unaligned
> destination (or an assertion that the destination is aligned would
> trigger).
>
> To remedy this, tell intel_get_memcpy() whether we're uploading or
> downloading so that it can select whether to assume the destination or
> source is aligned, respectively.
>
> Cc: 10.5 <mesa-stable at lists.freedesktop.org>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89416
> ---
> I don't know what benchmarks I should run to test this code?
>
>  src/mesa/drivers/dri/i965/intel_pixel_read.c   |   3 +-
>  src/mesa/drivers/dri/i965/intel_tex_image.c    |   3 +-
>  src/mesa/drivers/dri/i965/intel_tex_subimage.c |   3 +-
>  src/mesa/drivers/dri/i965/intel_tiled_memcpy.c | 156
> +++++++++++++++++++------
>  src/mesa/drivers/dri/i965/intel_tiled_memcpy.h |  15 ++-
>  5 files changed, 138 insertions(+), 42 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/intel_pixel_read.c
> b/src/mesa/drivers/dri/i965/intel_pixel_read.c
> index df22a63..0972121 100644
> --- a/src/mesa/drivers/dri/i965/intel_pixel_read.c
> +++ b/src/mesa/drivers/dri/i965/intel_pixel_read.c
> @@ -139,7 +139,8 @@ intel_readpixels_tiled_memcpy(struct gl_context * ctx,
>         rb->Format == MESA_FORMAT_R8G8B8X8_UNORM)
>        return false;
>
> -   if (!intel_get_memcpy(rb->Format, format, type, &mem_copy, &cpp))
> +   if (!intel_get_memcpy(rb->Format, format, type, &mem_copy, &cpp,
> +                         INTEL_DOWNLOAD))
>        return false;
>
>     if (!irb->mt ||
> diff --git a/src/mesa/drivers/dri/i965/intel_tex_image.c
> b/src/mesa/drivers/dri/i965/intel_tex_image.c
> index da42fdd..a3312d2 100644
> --- a/src/mesa/drivers/dri/i965/intel_tex_image.c
> +++ b/src/mesa/drivers/dri/i965/intel_tex_image.c
> @@ -408,7 +408,8 @@ intel_gettexsubimage_tiled_memcpy(struct gl_context
> *ctx,
>         texImage->TexFormat == MESA_FORMAT_R8G8B8X8_UNORM)
>        return false;
>
> -   if (!intel_get_memcpy(texImage->TexFormat, format, type, &mem_copy,
> &cpp))
> +   if (!intel_get_memcpy(texImage->TexFormat, format, type, &mem_copy,
> &cpp,
> +                         INTEL_UPLOAD))
>

Should be INTEL_DOWNLOAD


>        return false;
>
>     /* If this is a nontrivial texture view, let another path handle it
> instead. */
> diff --git a/src/mesa/drivers/dri/i965/intel_tex_subimage.c
> b/src/mesa/drivers/dri/i965/intel_tex_subimage.c
> index 4262f71..909ff25 100644
> --- a/src/mesa/drivers/dri/i965/intel_tex_subimage.c
> +++ b/src/mesa/drivers/dri/i965/intel_tex_subimage.c
> @@ -118,7 +118,8 @@ intel_texsubimage_tiled_memcpy(struct gl_context * ctx,
>         packing->Invert)
>        return false;
>
> -   if (!intel_get_memcpy(texImage->TexFormat, format, type, &mem_copy,
> &cpp))
> +   if (!intel_get_memcpy(texImage->TexFormat, format, type, &mem_copy,
> &cpp,
> +                         INTEL_UPLOAD))
>        return false;
>
>     /* If this is a nontrivial texture view, let another path handle it
> instead. */
> diff --git a/src/mesa/drivers/dri/i965/intel_tiled_memcpy.c
> b/src/mesa/drivers/dri/i965/intel_tiled_memcpy.c
> index f2b35cb..c43383b 100644
> --- a/src/mesa/drivers/dri/i965/intel_tiled_memcpy.c
> +++ b/src/mesa/drivers/dri/i965/intel_tiled_memcpy.c
> @@ -60,42 +60,79 @@ static const uint32_t ytile_span = 16;
>  static const uint8_t rgba8_permutation[16] =
>     { 2,1,0,3, 6,5,4,7, 10,9,8,11, 14,13,12,15 };
>
> -/* NOTE: dst must be 16 byte aligned */
> -#define rgba8_copy_16(dst, src)                     \
> -   *(__m128i *)(dst) = _mm_shuffle_epi8(            \
> -      (__m128i) _mm_loadu_ps((float *)(src)),       \
> -      *(__m128i *) rgba8_permutation                \
> -   )
> +/* NOTE: dst must be 16-byte aligned. src may be unaligned. */
> +#define rgba8_copy_16_aligned_dst(dst, src)                            \
> +   _mm_store_si128((__m128i *)(dst),                                   \
> +                   _mm_shuffle_epi8(_mm_loadu_si128((__m128i *)(src)), \
> +                                    *(__m128i *) rgba8_permutation))
> +
> +/* NOTE: src must be 16-byte aligned. dst may be unaligned. */
> +#define rgba8_copy_16_aligned_src(dst, src)                            \
> +   _mm_storeu_si128((__m128i *)(dst),                                  \
> +                    _mm_shuffle_epi8(_mm_load_si128((__m128i *)(src)), \
> +                                     *(__m128i *) rgba8_permutation))
>  #endif
>
>  /**
> - * Copy RGBA to BGRA - swap R and B.
> + * Copy RGBA to BGRA - swap R and B, with the destination 16-byte aligned.
>   */
>  static inline void *
> -rgba8_copy(void *dst, const void *src, size_t bytes)
> +rgba8_copy_aligned_dst(void *dst, const void *src, size_t bytes)
>  {
>     uint8_t *d = dst;
>     uint8_t const *s = src;
>
>  #ifdef __SSSE3__
> -   /* Fast copying for tile spans.
> -    *
> -    * As long as the destination texture is 16 aligned,
> -    * any 16 or 64 spans we get here should also be 16 aligned.
> -    */
> -
>     if (bytes == 16) {
>        assert(!(((uintptr_t)dst) & 0xf));
> -      rgba8_copy_16(d+ 0, s+ 0);
> +      rgba8_copy_16_aligned_dst(d+ 0, s+ 0);
>        return dst;
>     }
>
>     if (bytes == 64) {
>        assert(!(((uintptr_t)dst) & 0xf));
> -      rgba8_copy_16(d+ 0, s+ 0);
> -      rgba8_copy_16(d+16, s+16);
> -      rgba8_copy_16(d+32, s+32);
> -      rgba8_copy_16(d+48, s+48);
> +      rgba8_copy_16_aligned_dst(d+ 0, s+ 0);
> +      rgba8_copy_16_aligned_dst(d+16, s+16);
> +      rgba8_copy_16_aligned_dst(d+32, s+32);
> +      rgba8_copy_16_aligned_dst(d+48, s+48);
> +      return dst;
> +   }
> +#endif
> +
> +   while (bytes >= 4) {
> +      d[0] = s[2];
> +      d[1] = s[1];
> +      d[2] = s[0];
> +      d[3] = s[3];
> +      d += 4;
> +      s += 4;
> +      bytes -= 4;
> +   }
> +   return dst;
> +}
> +
> +/**
> + * Copy RGBA to BGRA - swap R and B, with the source 16-byte aligned.
> + */
> +static inline void *
> +rgba8_copy_aligned_src(void *dst, const void *src, size_t bytes)
> +{
> +   uint8_t *d = dst;
> +   uint8_t const *s = src;
> +
> +#ifdef __SSSE3__
> +   if (bytes == 16) {
> +      assert(!(((uintptr_t)src) & 0xf));
> +      rgba8_copy_16_aligned_src(d+ 0, s+ 0);
> +      return dst;
> +   }
> +
> +   if (bytes == 64) {
> +      assert(!(((uintptr_t)src) & 0xf));
> +      rgba8_copy_16_aligned_src(d+ 0, s+ 0);
> +      rgba8_copy_16_aligned_src(d+16, s+16);
> +      rgba8_copy_16_aligned_src(d+32, s+32);
> +      rgba8_copy_16_aligned_src(d+48, s+48);
>        return dst;
>     }
>  #endif
> @@ -357,16 +394,26 @@ linear_to_xtiled_faster(uint32_t x0, uint32_t x1,
> uint32_t x2, uint32_t x3,
>        if (mem_copy == memcpy)
>           return linear_to_xtiled(0, 0, xtile_width, xtile_width, 0,
> xtile_height,
>                                   dst, src, src_pitch, swizzle_bit,
> memcpy);
> -      else if (mem_copy == rgba8_copy)
> +      else if (mem_copy == rgba8_copy_aligned_dst)
> +         return linear_to_xtiled(0, 0, xtile_width, xtile_width, 0,
> xtile_height,
> +                                 dst, src, src_pitch, swizzle_bit,
> +                                 rgba8_copy_aligned_dst);
> +      else if (mem_copy == rgba8_copy_aligned_src)
>           return linear_to_xtiled(0, 0, xtile_width, xtile_width, 0,
> xtile_height,
> -                                 dst, src, src_pitch, swizzle_bit,
> rgba8_copy);
> +                                 dst, src, src_pitch, swizzle_bit,
> +                                 rgba8_copy_aligned_src);
>

linear_to_xtiled will always be called with an aligned destination so you
only need the one case.


>     } else {
>        if (mem_copy == memcpy)
>           return linear_to_xtiled(x0, x1, x2, x3, y0, y1,
>                                   dst, src, src_pitch, swizzle_bit,
> memcpy);
> -      else if (mem_copy == rgba8_copy)
> +      else if (mem_copy == rgba8_copy_aligned_dst)
>           return linear_to_xtiled(x0, x1, x2, x3, y0, y1,
> -                                 dst, src, src_pitch, swizzle_bit,
> rgba8_copy);
> +                                 dst, src, src_pitch, swizzle_bit,
> +                                 rgba8_copy_aligned_dst);
> +      else if (mem_copy == rgba8_copy_aligned_src)
> +         return linear_to_xtiled(x0, x1, x2, x3, y0, y1,
> +                                 dst, src, src_pitch, swizzle_bit,
> +                                 rgba8_copy_aligned_src);
>     }
>     linear_to_xtiled(x0, x1, x2, x3, y0, y1,
>                      dst, src, src_pitch, swizzle_bit, mem_copy);
> @@ -393,16 +440,26 @@ linear_to_ytiled_faster(uint32_t x0, uint32_t x1,
> uint32_t x2, uint32_t x3,
>        if (mem_copy == memcpy)
>           return linear_to_ytiled(0, 0, ytile_width, ytile_width, 0,
> ytile_height,
>                                   dst, src, src_pitch, swizzle_bit,
> memcpy);
> -      else if (mem_copy == rgba8_copy)
> +      else if (mem_copy == rgba8_copy_aligned_dst)
> +         return linear_to_ytiled(0, 0, ytile_width, ytile_width, 0,
> ytile_height,
> +                                 dst, src, src_pitch, swizzle_bit,
> +                                 rgba8_copy_aligned_dst);
> +      else if (mem_copy == rgba8_copy_aligned_src)
>           return linear_to_ytiled(0, 0, ytile_width, ytile_width, 0,
> ytile_height,
> -                                 dst, src, src_pitch, swizzle_bit,
> rgba8_copy);
> +                                 dst, src, src_pitch, swizzle_bit,
> +                                 rgba8_copy_aligned_src);
>

Same for linear_to_ytiled


>     } else {
>        if (mem_copy == memcpy)
>           return linear_to_ytiled(x0, x1, x2, x3, y0, y1,
>                                   dst, src, src_pitch, swizzle_bit,
> memcpy);
> -      else if (mem_copy == rgba8_copy)
> +      else if (mem_copy == rgba8_copy_aligned_dst)
>           return linear_to_ytiled(x0, x1, x2, x3, y0, y1,
> -                                 dst, src, src_pitch, swizzle_bit,
> rgba8_copy);
> +                                 dst, src, src_pitch, swizzle_bit,
> +                                 rgba8_copy_aligned_dst);
> +      else if (mem_copy == rgba8_copy_aligned_src)
> +         return linear_to_ytiled(x0, x1, x2, x3, y0, y1,
> +                                 dst, src, src_pitch, swizzle_bit,
> +                                 rgba8_copy_aligned_src);
>     }
>     linear_to_ytiled(x0, x1, x2, x3, y0, y1,
>                      dst, src, src_pitch, swizzle_bit, mem_copy);
> @@ -429,16 +486,26 @@ xtiled_to_linear_faster(uint32_t x0, uint32_t x1,
> uint32_t x2, uint32_t x3,
>        if (mem_copy == memcpy)
>           return xtiled_to_linear(0, 0, xtile_width, xtile_width, 0,
> xtile_height,
>                                   dst, src, dst_pitch, swizzle_bit,
> memcpy);
> -      else if (mem_copy == rgba8_copy)
> +      else if (mem_copy == rgba8_copy_aligned_dst)
> +         return xtiled_to_linear(0, 0, xtile_width, xtile_width, 0,
> xtile_height,
> +                                 dst, src, dst_pitch, swizzle_bit,
> +                                 rgba8_copy_aligned_dst);
> +      else if (mem_copy == rgba8_copy_aligned_src)
>           return xtiled_to_linear(0, 0, xtile_width, xtile_width, 0,
> xtile_height,
> -                                 dst, src, dst_pitch, swizzle_bit,
> rgba8_copy);
> +                                 dst, src, dst_pitch, swizzle_bit,
> +                                 rgba8_copy_aligned_src);
>     } else {
>        if (mem_copy == memcpy)
>           return xtiled_to_linear(x0, x1, x2, x3, y0, y1,
>                                   dst, src, dst_pitch, swizzle_bit,
> memcpy);
> -      else if (mem_copy == rgba8_copy)
> +      else if (mem_copy == rgba8_copy_aligned_dst)
> +         return xtiled_to_linear(x0, x1, x2, x3, y0, y1,
> +                                 dst, src, dst_pitch, swizzle_bit,
> +                                 rgba8_copy_aligned_dst);
> +      else if (mem_copy == rgba8_copy_aligned_src)
>           return xtiled_to_linear(x0, x1, x2, x3, y0, y1,
> -                                 dst, src, dst_pitch, swizzle_bit,
> rgba8_copy);
> +                                 dst, src, dst_pitch, swizzle_bit,
> +                                 rgba8_copy_aligned_src);


xtiled_to_linear will only ever be called with an aligned source


>     }
>     xtiled_to_linear(x0, x1, x2, x3, y0, y1,
>                      dst, src, dst_pitch, swizzle_bit, mem_copy);
> @@ -465,16 +532,26 @@ ytiled_to_linear_faster(uint32_t x0, uint32_t x1,
> uint32_t x2, uint32_t x3,
>        if (mem_copy == memcpy)
>           return ytiled_to_linear(0, 0, ytile_width, ytile_width, 0,
> ytile_height,
>                                   dst, src, dst_pitch, swizzle_bit,
> memcpy);
> -      else if (mem_copy == rgba8_copy)
> +      else if (mem_copy == rgba8_copy_aligned_dst)
>           return ytiled_to_linear(0, 0, ytile_width, ytile_width, 0,
> ytile_height,
> -                                 dst, src, dst_pitch, swizzle_bit,
> rgba8_copy);
> +                                 dst, src, dst_pitch, swizzle_bit,
> +                                 rgba8_copy_aligned_dst);
> +      else if (mem_copy == rgba8_copy_aligned_src)
> +         return ytiled_to_linear(0, 0, ytile_width, ytile_width, 0,
> ytile_height,
> +                                 dst, src, dst_pitch, swizzle_bit,
> +                                 rgba8_copy_aligned_src);
>

same for ytiled_to_linear.

Kill off the uneeded cases and fix the typo and this patch is

Reviewed-by: Jason Ekstrand <jason.ekstrand at intel.com>


>     } else {
>        if (mem_copy == memcpy)
>           return ytiled_to_linear(x0, x1, x2, x3, y0, y1,
>                                   dst, src, dst_pitch, swizzle_bit,
> memcpy);
> -      else if (mem_copy == rgba8_copy)
> +      else if (mem_copy == rgba8_copy_aligned_dst)
> +         return ytiled_to_linear(x0, x1, x2, x3, y0, y1,
> +                                 dst, src, dst_pitch, swizzle_bit,
> +                                 rgba8_copy_aligned_dst);
> +      else if (mem_copy == rgba8_copy_aligned_src)
>           return ytiled_to_linear(x0, x1, x2, x3, y0, y1,
> -                                 dst, src, dst_pitch, swizzle_bit,
> rgba8_copy);
> +                                 dst, src, dst_pitch, swizzle_bit,
> +                                 rgba8_copy_aligned_src);
>     }
>     ytiled_to_linear(x0, x1, x2, x3, y0, y1,
>                      dst, src, dst_pitch, swizzle_bit, mem_copy);
> @@ -684,7 +761,8 @@ tiled_to_linear(uint32_t xt1, uint32_t xt2,
>   * \return true if the format and type combination are valid
>   */
>  bool intel_get_memcpy(mesa_format tiledFormat, GLenum format,
> -                      GLenum type, mem_copy_fn* mem_copy, uint32_t* cpp)
> +                      GLenum type, mem_copy_fn *mem_copy, uint32_t *cpp,
> +                      enum intel_memcpy_direction direction)
>  {
>     if (type == GL_UNSIGNED_INT_8_8_8_8_REV &&
>         !(format == GL_RGBA || format == GL_BGRA))
> @@ -700,7 +778,8 @@ bool intel_get_memcpy(mesa_format tiledFormat, GLenum
> format,
>        if (format == GL_BGRA) {
>           *mem_copy = memcpy;
>        } else if (format == GL_RGBA) {
> -         *mem_copy = rgba8_copy;
> +         *mem_copy = direction == INTEL_UPLOAD ? rgba8_copy_aligned_dst
> +                                               : rgba8_copy_aligned_src;
>        }
>     } else if ((tiledFormat == MESA_FORMAT_R8G8B8A8_UNORM) ||
>                (tiledFormat == MESA_FORMAT_R8G8B8X8_UNORM)) {
> @@ -709,7 +788,8 @@ bool intel_get_memcpy(mesa_format tiledFormat, GLenum
> format,
>           /* Copying from RGBA to BGRA is the same as BGRA to RGBA so we
> can
>            * use the same function.
>            */
> -         *mem_copy = rgba8_copy;
> +         *mem_copy = direction == INTEL_UPLOAD ? rgba8_copy_aligned_dst
> +                                               : rgba8_copy_aligned_src;
>        } else if (format == GL_RGBA) {
>           *mem_copy = memcpy;
>        }
> diff --git a/src/mesa/drivers/dri/i965/intel_tiled_memcpy.h
> b/src/mesa/drivers/dri/i965/intel_tiled_memcpy.h
> index 3ff0d71..9dc1088 100644
> --- a/src/mesa/drivers/dri/i965/intel_tiled_memcpy.h
> +++ b/src/mesa/drivers/dri/i965/intel_tiled_memcpy.h
> @@ -55,7 +55,20 @@ tiled_to_linear(uint32_t xt1, uint32_t xt2,
>                  uint32_t tiling,
>                  mem_copy_fn mem_copy);
>
> +/* Tells intel_get_memcpy() whether the memcpy() is
> + *
> + *  - an upload to the GPU with an aligned destination and a potentially
> + *    unaligned source; or
> + *  - a download from the GPU with an aligned source and a potentially
> + *    unaligned destination.
> + */
> +enum intel_memcpy_direction {
> +   INTEL_UPLOAD,
> +   INTEL_DOWNLOAD
> +};
> +
>  bool intel_get_memcpy(mesa_format tiledFormat, GLenum format,
> -                      GLenum type, mem_copy_fn* mem_copy, uint32_t* cpp);
> +                      GLenum type, mem_copy_fn *mem_copy, uint32_t *cpp,
> +                      enum intel_memcpy_direction direction);
>
>  #endif /* INTEL_TILED_MEMCPY */
> --
> 2.0.5
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-stable/attachments/20150304/8412b6e5/attachment-0001.html>


More information about the mesa-stable mailing list