On 14 June 2018 at 20:24, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote:
> From: Jon Bloomfield <jon.bloomfield@xxxxxxxxx>
>
> Hook up the flags to allow read-only ppGTT mappings for gen8+
>
> v2: Include a selftest to check that writes to a readonly PTE are
> dropped
>
> Signed-off-by: Jon Bloomfield <jon.bloomfield@xxxxxxxxx>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> Cc: Matthew Auld <matthew.william.auld@xxxxxxxxx>
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx> #v1
> Reviewed-by: Matthew Auld <matthew.william.auld@xxxxxxxxx> #v1
> ---
> drivers/gpu/drm/i915/i915_gem_gtt.c | 45 ++++--
> drivers/gpu/drm/i915/i915_gem_gtt.h | 7 +-
> drivers/gpu/drm/i915/intel_ringbuffer.c | 11 +-
> .../gpu/drm/i915/selftests/i915_gem_context.c | 138 ++++++++++++++++++
> 4 files changed, 182 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index c3630abbd260..bfe23e10a127 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -204,7 +204,7 @@ static int ppgtt_bind_vma(struct i915_vma *vma,
> return err;
> }
>
> - /* Currently applicable only to VLV */
> + /* Applicable to VLV, and gen8+ */
> pte_flags = 0;
> if (vma->obj->gt_ro)
> pte_flags |= PTE_READ_ONLY;
> @@ -961,10 +961,11 @@ gen8_ppgtt_insert_pte_entries(struct i915_hw_ppgtt *ppgtt,
> struct i915_page_directory_pointer *pdp,
> struct sgt_dma *iter,
> struct gen8_insert_pte *idx,
> - enum i915_cache_level cache_level)
> + enum i915_cache_level cache_level,
> + u32 flags)
> {
> struct i915_page_directory *pd;
> - const gen8_pte_t pte_encode = gen8_pte_encode(0, cache_level, 0);
> + const gen8_pte_t pte_encode = gen8_pte_encode(0, cache_level, flags);
> gen8_pte_t *vaddr;
> bool ret;
>
> @@ -1015,14 +1016,14 @@ gen8_ppgtt_insert_pte_entries(struct i915_hw_ppgtt *ppgtt,
> static void gen8_ppgtt_insert_3lvl(struct i915_address_space *vm,
> struct i915_vma *vma,
> enum i915_cache_level cache_level,
> - u32 unused)
> + u32 flags)
> {
> struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm);
> struct sgt_dma iter = sgt_dma(vma);
> struct gen8_insert_pte idx = gen8_insert_pte(vma->node.start);
>
> gen8_ppgtt_insert_pte_entries(ppgtt, &ppgtt->pdp, &iter, &idx,
> - cache_level);
> + cache_level, flags);
>
> vma->page_sizes.gtt = I915_GTT_PAGE_SIZE;
> }
> @@ -1030,9 +1031,10 @@ static void gen8_ppgtt_insert_3lvl(struct i915_address_space *vm,
> static void gen8_ppgtt_insert_huge_entries(struct i915_vma *vma,
> struct i915_page_directory_pointer **pdps,
> struct sgt_dma *iter,
> - enum i915_cache_level cache_level)
> + enum i915_cache_level cache_level,
> + u32 flags)
> {
> - const gen8_pte_t pte_encode = gen8_pte_encode(0, cache_level, 0);
> + const gen8_pte_t pte_encode = gen8_pte_encode(0, cache_level, flags);
> u64 start = vma->node.start;
> dma_addr_t rem = iter->sg->length;
>
> @@ -1148,19 +1150,21 @@ static void gen8_ppgtt_insert_huge_entries(struct i915_vma *vma,
> static void gen8_ppgtt_insert_4lvl(struct i915_address_space *vm,
> struct i915_vma *vma,
> enum i915_cache_level cache_level,
> - u32 unused)
> + u32 flags)
> {
> struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm);
> struct sgt_dma iter = sgt_dma(vma);
> struct i915_page_directory_pointer **pdps = ppgtt->pml4.pdps;
>
> if (vma->page_sizes.sg > I915_GTT_PAGE_SIZE) {
> - gen8_ppgtt_insert_huge_entries(vma, pdps, &iter, cache_level);
> + gen8_ppgtt_insert_huge_entries(vma, pdps, &iter, cache_level,
> + flags);
> } else {
> struct gen8_insert_pte idx = gen8_insert_pte(vma->node.start);
>
> while (gen8_ppgtt_insert_pte_entries(ppgtt, pdps[idx.pml4e++],
> - &iter, &idx, cache_level))
> + &iter, &idx, cache_level,
> + flags))
> GEM_BUG_ON(idx.pml4e >= GEN8_PML4ES_PER_PML4);
>
> vma->page_sizes.gtt = I915_GTT_PAGE_SIZE;
> @@ -1573,6 +1577,9 @@ static struct i915_hw_ppgtt *gen8_ppgtt_create(struct drm_i915_private *i915)
> 1ULL << 48 :
> 1ULL << 32;
>
> + /* From bdw, there is support for read-only pages in the PPGTT */
> + ppgtt->vm.has_read_only = true;
> +
> /* There are only few exceptions for gen >=6. chv and bxt.
> * And we are not sure about the latter so play safe for now.
> */
> @@ -2408,7 +2415,7 @@ static void gen8_ggtt_insert_page(struct i915_address_space *vm,
> static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
> struct i915_vma *vma,
> enum i915_cache_level level,
> - u32 unused)
> + u32 flags)
> {
> struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
> struct sgt_iter sgt_iter;
> @@ -2416,6 +2423,9 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
> const gen8_pte_t pte_encode = gen8_pte_encode(0, level, 0);
> dma_addr_t addr;
>
> + /* The GTT does not support read-only mappings */
> + GEM_BUG_ON(flags & PTE_READ_ONLY);
> +
> gtt_entries = (gen8_pte_t __iomem *)ggtt->gsm;
> gtt_entries += vma->node.start >> PAGE_SHIFT;
> for_each_sgt_dma(addr, sgt_iter, vma->pages)
> @@ -2542,13 +2552,14 @@ struct insert_entries {
> struct i915_address_space *vm;
> struct i915_vma *vma;
> enum i915_cache_level level;
> + u32 flags;
> };
>
> static int bxt_vtd_ggtt_insert_entries__cb(void *_arg)
> {
> struct insert_entries *arg = _arg;
>
> - gen8_ggtt_insert_entries(arg->vm, arg->vma, arg->level, 0);
> + gen8_ggtt_insert_entries(arg->vm, arg->vma, arg->level, arg->flags);
> bxt_vtd_ggtt_wa(arg->vm);
>
> return 0;
> @@ -2557,9 +2568,9 @@ static int bxt_vtd_ggtt_insert_entries__cb(void *_arg)
> static void bxt_vtd_ggtt_insert_entries__BKL(struct i915_address_space *vm,
> struct i915_vma *vma,
> enum i915_cache_level level,
> - u32 unused)
> + u32 flags)
> {
> - struct insert_entries arg = { vm, vma, level };
> + struct insert_entries arg = { vm, vma, level, flags };
>
> stop_machine(bxt_vtd_ggtt_insert_entries__cb, &arg, NULL);
> }
> @@ -2650,7 +2661,7 @@ static int ggtt_bind_vma(struct i915_vma *vma,
> struct drm_i915_gem_object *obj = vma->obj;
> u32 pte_flags;
>
> - /* Currently applicable only to VLV */
> + /* Applicable to VLV (gen8+ do not support RO in the GGTT) */
> pte_flags = 0;
> if (obj->gt_ro)
> pte_flags |= PTE_READ_ONLY;
> @@ -3530,6 +3541,10 @@ int i915_ggtt_init_hw(struct drm_i915_private *dev_priv)
> */
> mutex_lock(&dev_priv->drm.struct_mutex);
> i915_address_space_init(&ggtt->vm, dev_priv, "[global]");
> +
> + /* Only VLV supports read-only GGTT mappings */
> + ggtt->vm.has_read_only = IS_VALLEYVIEW(dev_priv);
> +
> if (!HAS_LLC(dev_priv) && !USES_PPGTT(dev_priv))
> ggtt->vm.mm.color_adjust = i915_gtt_color_adjust;
> mutex_unlock(&dev_priv->drm.struct_mutex);
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 46c95d188580..67a13a0709fc 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -325,7 +325,12 @@ struct i915_address_space {
> struct list_head unbound_list;
>
> struct pagevec free_pages;
> - bool pt_kmap_wc;
> +
> + /* Some systems require uncached updates of the page directories */
> + bool pt_kmap_wc:1;
> +
> + /* Some systems support read-only mappings for GGTT and/or PPGTT */
> + bool has_read_only:1;
>
> /* FIXME: Need a more generic return type */
> gen6_pte_t (*pte_encode)(dma_addr_t addr,
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index ef3c76425843..8853a5e6d421 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1097,6 +1097,7 @@ void intel_ring_unpin(struct intel_ring *ring)
> static struct i915_vma *
> intel_ring_create_vma(struct drm_i915_private *dev_priv, int size)
> {
> + struct i915_address_space *vm = &dev_priv->ggtt.vm;
> struct drm_i915_gem_object *obj;
> struct i915_vma *vma;
>
> @@ -1106,10 +1107,14 @@ intel_ring_create_vma(struct drm_i915_private *dev_priv, int size)
> if (IS_ERR(obj))
> return ERR_CAST(obj);
>
> - /* mark ring buffers as read-only from GPU side by default */
> - obj->gt_ro = 1;
> + /*
> + * Mark ring buffers as read-only from GPU side (so no stray overwrites)
> + * if supported by the platform's GGTT.
> + */
> + if (vm->has_read_only)
> + obj->gt_ro = 1;
>
> - vma = i915_vma_instance(obj, &dev_priv->ggtt.vm, NULL);
> + vma = i915_vma_instance(obj, vm, NULL);
> if (IS_ERR(vma))
> goto err;
>
> diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/selftests/i915_gem_context.c
> index 836f1af8b833..5bae52068926 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_gem_context.c
> @@ -23,6 +23,7 @@
> */
>
> #include "../i915_selftest.h"
> +#include "i915_random.h"
> #include "igt_flush_test.h"
>
> #include "mock_drm.h"
> @@ -266,6 +267,41 @@ static int cpu_check(struct drm_i915_gem_object *obj, unsigned int max)
> return err;
> }
>
> +static int ro_check(struct drm_i915_gem_object *obj, unsigned int max)
> +{
> + unsigned int n, m, needs_flush;
> + int err;
> +
> + err = i915_gem_obj_prepare_shmem_read(obj, &needs_flush);
> + if (err)
> + return err;
> +
> + for (n = 0; n < real_page_count(obj); n++) {
> + u32 *map;
> +
> + map = kmap_atomic(i915_gem_object_get_page(obj, n));
> + if (needs_flush & CLFLUSH_BEFORE)
> + drm_clflush_virt_range(map, PAGE_SIZE);
> +
> + for (m = 0; m < DW_PER_PAGE; m++) {
m < max; or don't pass max?
> + if (map[m] != 0xdeadbeef) {
> + pr_err("Invalid value (overwritten) at page %d, offset %d: found %08x expected %08x\n",
> + n, m, map[m], 0xdeadbeef);
> + err = -EINVAL;
> + goto out_unmap;
> + }
> + }
> +
> +out_unmap:
> + kunmap_atomic(map);
> + if (err)
> + break;
> + }
> +
> + i915_gem_obj_finish_shmem_access(obj);
> + return err;
> +}
> +
> static int file_add_object(struct drm_file *file,
> struct drm_i915_gem_object *obj)
> {
> @@ -421,6 +457,107 @@ static int igt_ctx_exec(void *arg)
> return err;
> }
>
> +static int igt_ctx_readonly(void *arg)
> +{
> + struct drm_i915_private *i915 = arg;
> + struct drm_i915_gem_object *obj = NULL;
> + struct drm_file *file;
> + I915_RND_STATE(prng);
> + IGT_TIMEOUT(end_time);
> + LIST_HEAD(objects);
> + struct i915_gem_context *ctx;
> + struct i915_hw_ppgtt *ppgtt;
> + unsigned long ndwords, dw;
> + int err = -ENODEV;
> +
> + /* Create a few different contexts (with different mm) and write
> + * through each ctx/mm using the GPU making sure those writes end
> + * up in the expected pages of our obj.
> + */
/*\n
Reviewed-by: Matthew Auld <matthew.william.auld@xxxxxxxxx>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
