Re: [PATCH v5] drm/i915: Use SSE4.1 movntdqa to accelerate reads from WC memory

Tvrtko Ursulin
2016-07-18T09:31:07Z

On 16/07/16 16:44, Chris Wilson wrote:
> This patch provides the infrastructure for performing a 16-byte aligned
> read from WC memory using non-temporal instructions introduced with sse4.1.
> Using movntdqa we can bypass the CPU caches and read directly from memory
> and ignoring the page attributes set on the CPU PTE i.e. negating the
> impact of an otherwise UC access. Copying using movntqda from WC is almost
> as fast as reading from WB memory, modulo the possibility of both hitting
> the CPU cache or leaving the data in the CPU cache for the next consumer.
> (The CPU cache itself my be flushed for the region of the movntdqa and on
> later access the movntdqa reads from a separate internal buffer for the
> cacheline.) The write back to the memory is however cached.
>
> This will be used in later patches to accelerate accessing WC memory.
>
> v2: Report whether the accelerated copy is successful/possible.
> v3: Function alignment override was only necessary when using the
> function target("sse4.1") - which is not necessary for emitting movntdqa
> from __asm__.
> v4: Improve notes on CPU cache behaviour vs non-temporal stores.
> v5: Fix byte offsets for unrolled moves.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Akash Goel <akash.goel@intel.com>
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
> drivers/gpu/drm/i915/Makefile | 3 ++
> drivers/gpu/drm/i915/i915_drv.c | 2 +
> drivers/gpu/drm/i915/i915_drv.h | 3 ++
> drivers/gpu/drm/i915/i915_memcpy.c | 75 ++++++++++++++++++++++++++++++++++++++
> 4 files changed, 83 insertions(+)
> create mode 100644 drivers/gpu/drm/i915/i915_memcpy.c
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 75318ebb8d25..a53853daa998 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -3,12 +3,15 @@
> # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
>
> subdir-ccflags-$(CONFIG_DRM_I915_WERROR) := -Werror
> +subdir-ccflags-y += \
> + $(call as-instr,movntdqa (%eax)$(comma)%xmm0,-DCONFIG_AS_MOVNTDQA)
>
> # Please keep these build lists sorted!
>
> # core driver code
> i915-y := i915_drv.o \
> i915_irq.o \
> + i915_memcpy.o \
> i915_params.o \
> i915_pci.o \
> i915_suspend.o \
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index c5b7b8e0678a..16612542496a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -848,6 +848,8 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
> mutex_init(&dev_priv->wm.wm_mutex);
> mutex_init(&dev_priv->pps_mutex);
>
> + i915_memcpy_init_early(dev_priv);
> +
> ret = i915_workqueues_init(dev_priv);
> if (ret < 0)
> return ret;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 27d9b2c374b3..3c266e7866ba 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -4070,4 +4070,7 @@ static inline bool __i915_request_irq_complete(struct drm_i915_gem_request *req)
> return false;
> }
>
> +void i915_memcpy_init_early(struct drm_i915_private *dev_priv);
> +bool i915_memcpy_from_wc(void *dst, const void *src, unsigned long len);
> +
> #endif
> diff --git a/drivers/gpu/drm/i915/i915_memcpy.c b/drivers/gpu/drm/i915/i915_memcpy.c
> new file mode 100644
> index 000000000000..4ed4d3bb2f3e
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_memcpy.c
> @@ -0,0 +1,75 @@
> +#include "i915_drv.h"
> +
> +DEFINE_STATIC_KEY_FALSE(has_movntqa);
> +
> +#ifdef CONFIG_AS_MOVNTDQA
> +static void __movntqda(void *dst, const void *src, unsigned long len)
> +{
> + len >>= 4;
> + while (len >= 4) {
> + __asm__ __volatile__(
> + "movntdqa (%0), %%xmm0\n"
> + "movntdqa 16(%0), %%xmm1\n"
> + "movntdqa 32(%0), %%xmm2\n"
> + "movntdqa 48(%0), %%xmm3\n"
> + "movaps %%xmm0, (%1)\n"
> + "movaps %%xmm1, 16(%1)\n"
> + "movaps %%xmm2, 32(%1)\n"
> + "movaps %%xmm3, 48(%1)\n"
> + : : "r" (src), "r" (dst) : "memory");
> + src += 64;
> + dst += 64;
> + len -= 4;
> + }
> + while (len--) {
> + __asm__ __volatile__(
> + "movntdqa (%0), %%xmm0\n"
> + "movaps %%xmm0, (%1)\n"
> + : : "r" (src), "r" (dst) : "memory");
> + src += 16;
> + dst += 16;
> + }
> +}
> +#endif

Is it okay nowadays to just use these registers in the kernel?

Many years ago when I last looked into this FPU and MMX registers were
discouraged against and needed explicit kernel_gpu_begin/end around the
block. Since they were not saved/restored by the kernel and doing
otherwise would mess up the userspace context.

Perhaps these new registers are different or the things have generally
changed since then?

Regards,

Tvrtko

> +
> +/**
> + * i915_memcpy_from_wc: perform an accelerated *aligned* read from WC
> + * @dst: destination pointer
> + * @src: source pointer
> + * @len: how many bytes to copy
> + *
> + * i915_memcpy_from_wc copies @len bytes from @src to @dst using
> + * non-temporal instructions where available. Note that all arguments
> + * (@src, @dst) must be aligned to 16 bytes and @len must be a multiple
> + * of 16.
> + *
> + * To test whether accelerated reads from WC are supported, use
> + * i915_memcpy_from_wc(NULL, NULL, 0);
> + *
> + * Returns true if the copy was successful, false if the preconditions
> + * are not met.
> + */
> +bool i915_memcpy_from_wc(void *dst, const void *src, unsigned long len)
> +{
> + GEM_BUG_ON((unsigned long)dst & 15);
> + GEM_BUG_ON((unsigned long)src & 15);
> +
> + if (unlikely(len & 15))
> + return false;
> +
> +#ifdef CONFIG_AS_MOVNTDQA
> + if (static_branch_likely(&has_movntqa)) {
> + if (len)
> + __movntqda(dst, src, len);
> + return true;
> + }
> +#endif
> +
> + return false;
> +}
> +
> +void i915_memcpy_init_early(struct drm_i915_private *dev_priv)
> +{
> + if (static_cpu_has(X86_FEATURE_XMM4_1))
> + static_branch_enable(&has_movntqa);
> +}