[PATCH 1/6] drm/i915/gtt: Add read only pages to gen8_pte_encode

[PATCH 1/6] drm/i915/gtt: Add read only pages to gen8_pte_encode

[Chris Wilson]

From: Jon Bloomfield <jon.bloomfield@intel.com>

We can set a bit inside the ppGTT PTE to indicate a page is read-only;
writes from the GPU will be discarded. We can use this to protect pages
and in particular support read-only userptr mappings (necessary for
importing PROT_READ vma).

Signed-off-by: Jon Bloomfield <jon.bloomfield@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Matthew Auld <matthew.william.auld@gmail.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Matthew Auld <matthew.william.auld@gmail.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index d0acef299b9c..7292f41eb752 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -244,10 +244,13 @@ static void clear_pages(struct i915_vma *vma)
 }
 
 static gen8_pte_t gen8_pte_encode(dma_addr_t addr,
-				  enum i915_cache_level level)
+				  enum i915_cache_level level,
+				  u32 flags)
 {
-	gen8_pte_t pte = _PAGE_PRESENT | _PAGE_RW;
-	pte |= addr;
+	gen8_pte_t pte = addr | _PAGE_PRESENT | _PAGE_RW;
+
+	if (unlikely(flags & PTE_READ_ONLY))
+		pte &= ~_PAGE_RW;
 
 	switch (level) {
 	case I915_CACHE_NONE:
@@ -721,7 +724,7 @@ static void gen8_initialize_pt(struct i915_address_space *vm,
 			       struct i915_page_table *pt)
 {
 	fill_px(vm, pt,
-		gen8_pte_encode(vm->scratch_page.daddr, I915_CACHE_LLC));
+		gen8_pte_encode(vm->scratch_page.daddr, I915_CACHE_LLC, 0));
 }
 
 static void gen6_initialize_pt(struct gen6_hw_ppgtt *ppgtt,
@@ -869,7 +872,7 @@ static bool gen8_ppgtt_clear_pt(struct i915_address_space *vm,
 	unsigned int pte = gen8_pte_index(start);
 	unsigned int pte_end = pte + num_entries;
 	const gen8_pte_t scratch_pte =
-		gen8_pte_encode(vm->scratch_page.daddr, I915_CACHE_LLC);
+		gen8_pte_encode(vm->scratch_page.daddr, I915_CACHE_LLC, 0);
 	gen8_pte_t *vaddr;
 
 	GEM_BUG_ON(num_entries > pt->used_ptes);
@@ -1044,7 +1047,7 @@ gen8_ppgtt_insert_pte_entries(struct i915_hw_ppgtt *ppgtt,
 			      enum i915_cache_level cache_level)
 {
 	struct i915_page_directory *pd;
-	const gen8_pte_t pte_encode = gen8_pte_encode(0, cache_level);
+	const gen8_pte_t pte_encode = gen8_pte_encode(0, cache_level, 0);
 	gen8_pte_t *vaddr;
 	bool ret;
 
@@ -1112,7 +1115,7 @@ static void gen8_ppgtt_insert_huge_entries(struct i915_vma *vma,
 					   struct sgt_dma *iter,
 					   enum i915_cache_level cache_level)
 {
-	const gen8_pte_t pte_encode = gen8_pte_encode(0, cache_level);
+	const gen8_pte_t pte_encode = gen8_pte_encode(0, cache_level, 0);
 	u64 start = vma->node.start;
 	dma_addr_t rem = iter->sg->length;
 
@@ -1578,7 +1581,7 @@ static void gen8_dump_ppgtt(struct i915_hw_ppgtt *ppgtt, struct seq_file *m)
 {
 	struct i915_address_space *vm = &ppgtt->vm;
 	const gen8_pte_t scratch_pte =
-		gen8_pte_encode(vm->scratch_page.daddr, I915_CACHE_LLC);
+		gen8_pte_encode(vm->scratch_page.daddr, I915_CACHE_LLC, 0);
 	u64 start = 0, length = ppgtt->vm.total;
 
 	if (use_4lvl(vm)) {
@@ -2461,7 +2464,7 @@ static void gen8_ggtt_insert_page(struct i915_address_space *vm,
 	gen8_pte_t __iomem *pte =
 		(gen8_pte_t __iomem *)ggtt->gsm + (offset >> PAGE_SHIFT);
 
-	gen8_set_pte(pte, gen8_pte_encode(addr, level));
+	gen8_set_pte(pte, gen8_pte_encode(addr, level, 0));
 
 	ggtt->invalidate(vm->i915);
 }
@@ -2474,7 +2477,7 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
 	struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
 	struct sgt_iter sgt_iter;
 	gen8_pte_t __iomem *gtt_entries;
-	const gen8_pte_t pte_encode = gen8_pte_encode(0, level);
+	const gen8_pte_t pte_encode = gen8_pte_encode(0, level, 0);
 	dma_addr_t addr;
 
 	gtt_entries = (gen8_pte_t __iomem *)ggtt->gsm;
@@ -2542,7 +2545,7 @@ static void gen8_ggtt_clear_range(struct i915_address_space *vm,
 	unsigned first_entry = start >> PAGE_SHIFT;
 	unsigned num_entries = length >> PAGE_SHIFT;
 	const gen8_pte_t scratch_pte =
-		gen8_pte_encode(vm->scratch_page.daddr, I915_CACHE_LLC);
+		gen8_pte_encode(vm->scratch_page.daddr, I915_CACHE_LLC, 0);
 	gen8_pte_t __iomem *gtt_base =
 		(gen8_pte_t __iomem *)ggtt->gsm + first_entry;
 	const int max_entries = ggtt_total_entries(ggtt) - first_entry;
-- 
2.18.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/6] drm/i915/gtt: Read-only pages for insert_entries on bdw+

[Chris Wilson]

From: Jon Bloomfield <jon.bloomfield@intel.com>

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
v3: Don't duplicate cpu_check() as we can just reuse it, and even worse
don't wholesale copy the theory-of-operation comment from igt_ctx_exec
without changing it to explain the intention behind the new test!
v4: Joonas really likes magic mystery values

Signed-off-by: Jon Bloomfield <jon.bloomfield@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Matthew Auld <matthew.william.auld@gmail.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Matthew Auld <matthew.william.auld@gmail.com>
---
 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 | 112 +++++++++++++++++-
 4 files changed, 153 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 7292f41eb752..6c0b438afe46 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;
@@ -1044,10 +1044,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;
 
@@ -1098,14 +1099,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;
 }
@@ -1113,9 +1114,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;
 
@@ -1231,19 +1233,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;
@@ -1658,6 +1662,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;
+
 	i915_address_space_init(&ppgtt->vm, i915);
 
 	/* There are only few exceptions for gen >=6. chv and bxt.
@@ -2472,7 +2479,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;
@@ -2480,6 +2487,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)
@@ -2606,13 +2616,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;
@@ -2621,9 +2632,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);
 }
@@ -2714,7 +2725,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;
@@ -3594,6 +3605,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);
+
+	/* 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 14e62651010b..2a116a91420b 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -331,7 +331,12 @@ struct i915_address_space {
 	struct list_head unbound_list;
 
 	struct pagestash 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 f4bd185c9369..2106b191781d 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1087,6 +1087,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;
 
@@ -1096,10 +1097,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 ab2590242033..e51ca7b4b7ae 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"
@@ -252,9 +253,9 @@ static int cpu_check(struct drm_i915_gem_object *obj, unsigned int max)
 		}
 
 		for (; m < DW_PER_PAGE; m++) {
-			if (map[m] != 0xdeadbeef) {
+			if (map[m] != STACK_MAGIC) {
 				pr_err("Invalid value at page %d, offset %d: found %x expected %x\n",
-				       n, m, map[m], 0xdeadbeef);
+				       n, m, map[m], STACK_MAGIC);
 				err = -EINVAL;
 				goto out_unmap;
 			}
@@ -310,7 +311,7 @@ create_test_object(struct i915_gem_context *ctx,
 	if (err)
 		return ERR_PTR(err);
 
-	err = cpu_fill(obj, 0xdeadbeef);
+	err = cpu_fill(obj, STACK_MAGIC);
 	if (err) {
 		pr_err("Failed to fill object with cpu, err=%d\n",
 		       err);
@@ -432,6 +433,110 @@ 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 read-only objects (with the occasional writable object)
+	 * and try to write into these object checking that the GPU discards
+	 * any write to a read-only object.
+	 */
+
+	file = mock_file(i915);
+	if (IS_ERR(file))
+		return PTR_ERR(file);
+
+	mutex_lock(&i915->drm.struct_mutex);
+
+	ctx = i915_gem_create_context(i915, file->driver_priv);
+	if (IS_ERR(ctx)) {
+		err = PTR_ERR(ctx);
+		goto out_unlock;
+	}
+
+	ppgtt = ctx->ppgtt ?: i915->mm.aliasing_ppgtt;
+	if (!ppgtt || !ppgtt->vm.has_read_only) {
+		err = 0;
+		goto out_unlock;
+	}
+
+	ndwords = 0;
+	dw = 0;
+	while (!time_after(jiffies, end_time)) {
+		struct intel_engine_cs *engine;
+		unsigned int id;
+
+		for_each_engine(engine, i915, id) {
+			if (!intel_engine_can_store_dword(engine))
+				continue;
+
+			if (!obj) {
+				obj = create_test_object(ctx, file, &objects);
+				if (IS_ERR(obj)) {
+					err = PTR_ERR(obj);
+					goto out_unlock;
+				}
+
+				obj->gt_ro = prandom_u32_state(&prng);
+			}
+
+			intel_runtime_pm_get(i915);
+			err = gpu_fill(obj, ctx, engine, dw);
+			intel_runtime_pm_put(i915);
+			if (err) {
+				pr_err("Failed to fill dword %lu [%lu/%lu] with gpu (%s) in ctx %u [full-ppgtt? %s], err=%d\n",
+				       ndwords, dw, max_dwords(obj),
+				       engine->name, ctx->hw_id,
+				       yesno(!!ctx->ppgtt), err);
+				goto out_unlock;
+			}
+
+			if (++dw == max_dwords(obj)) {
+				obj = NULL;
+				dw = 0;
+			}
+			ndwords++;
+		}
+	}
+	pr_info("Submitted %lu dwords (across %u engines)\n",
+	       	ndwords, INTEL_INFO(i915)->num_rings);
+
+	dw = 0;
+	list_for_each_entry(obj, &objects, st_link) {
+		unsigned int rem =
+			min_t(unsigned int, ndwords - dw, max_dwords(obj));
+		unsigned int num_writes;
+
+		num_writes = rem;
+		if (obj->gt_ro)
+			num_writes = 0;
+
+		err = cpu_check(obj, num_writes);
+		if (err)
+			break;
+
+		dw += rem;
+	}
+
+out_unlock:
+	if (igt_flush_test(i915, I915_WAIT_LOCKED))
+		err = -EIO;
+	mutex_unlock(&i915->drm.struct_mutex);
+
+	mock_file_free(i915, file);
+	return err;
+}
+
 static __maybe_unused const char *
 __engine_name(struct drm_i915_private *i915, unsigned int engines)
 {
@@ -608,6 +713,7 @@ int i915_gem_context_live_selftests(struct drm_i915_private *dev_priv)
 	static const struct i915_subtest tests[] = {
 		SUBTEST(igt_switch_to_kernel_context),
 		SUBTEST(igt_ctx_exec),
+		SUBTEST(igt_ctx_readonly),
 	};
 	bool fake_alias = false;
 	int err;
-- 
2.18.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/6] drm/i915/gtt: Disable read-only support under GVT

[Chris Wilson]

GVT is not propagating the PTE bits, and is always setting the
read-write bit, thus breaking read-only support.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
Cc: Jon Bloomfield <jon.bloomfield@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Matthew Auld <matthew.william.auld@gmail.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 6c0b438afe46..8e70a45b8a90 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1662,8 +1662,12 @@ 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;
+	/*
+	 * From bdw, there is support for read-only pages in the PPGTT.
+	 *
+	 * XXX GVT is not setting honouring the PTE bits.
+	 */
+	ppgtt->vm.has_read_only = !intel_vgpu_active(i915);
 
 	i915_address_space_init(&ppgtt->vm, i915);
 
-- 
2.18.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/6] drm/i915: Prevent writing into a read-only object via a GGTT mmap

[Chris Wilson]

If the user has created a read-only object, they should not be allowed
to circumvent the write protection by using a GGTT mmapping. Deny it.

Also most machines do not support read-only GGTT PTEs, so again we have
to reject attempted writes. Fortunately, this is known a priori, so we
can at least reject in the call to create the mmap (with a sanity check
in the fault handler).

v2: Check the vma->vm_flags during mmap() to allow readonly access.
v3: Remove VM_MAYWRITE to curtail mprotect()

Testcase: igt/gem_userptr_blits/readonly_mmap*
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jon Bloomfield <jon.bloomfield@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Matthew Auld <matthew.william.auld@gmail.com>
Cc: David Herrmann <dh.herrmann@gmail.com>
Reviewed-by: Matthew Auld <matthew.william.auld@gmail.com> #v1
Reviewed-by: Jon Bloomfield <jon.bloomfield@intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/drm_gem.c                         |  9 +++++++++
 drivers/gpu/drm/i915/i915_gem.c                   |  4 ++++
 drivers/gpu/drm/i915/i915_gem_gtt.c               | 12 +++++++-----
 drivers/gpu/drm/i915/i915_gem_object.h            | 13 ++++++++++++-
 drivers/gpu/drm/i915/intel_ringbuffer.c           |  2 +-
 drivers/gpu/drm/i915/selftests/i915_gem_context.c |  5 +++--
 include/drm/drm_vma_manager.h                     |  1 +
 7 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 4a16d7b26c89..bf90625df3c5 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1036,6 +1036,15 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
 		return -EACCES;
 	}
 
+	if (node->readonly) {
+		if (vma->vm_flags & VM_WRITE) {
+			drm_gem_object_put_unlocked(obj);
+			return -EINVAL;
+		}
+
+		vma->vm_flags &= ~VM_MAYWRITE;
+	}
+
 	ret = drm_gem_mmap_obj(obj, drm_vma_node_size(node) << PAGE_SHIFT,
 			       vma);
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ed2be33ec58a..1910c66f48e2 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2012,6 +2012,10 @@ vm_fault_t i915_gem_fault(struct vm_fault *vmf)
 	pgoff_t page_offset;
 	int ret;
 
+	/* Sanity check that we allow writing into this object */
+	if (i915_gem_object_is_readonly(obj) && write)
+		return VM_FAULT_SIGBUS;
+
 	/* We don't use vmf->pgoff since that has the fake offset */
 	page_offset = (vmf->address - area->vm_start) >> PAGE_SHIFT;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 8e70a45b8a90..da0e9870a66f 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -206,7 +206,7 @@ static int ppgtt_bind_vma(struct i915_vma *vma,
 
 	/* Applicable to VLV, and gen8+ */
 	pte_flags = 0;
-	if (vma->obj->gt_ro)
+	if (i915_gem_object_is_readonly(vma->obj))
 		pte_flags |= PTE_READ_ONLY;
 
 	vma->vm->insert_entries(vma->vm, vma, cache_level, pte_flags);
@@ -2491,8 +2491,10 @@ 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);
+	/*
+	 * Note that we ignore PTE_READ_ONLY here. The caller must be careful
+	 * not to allow the user to override access to a read only page.
+	 */
 
 	gtt_entries = (gen8_pte_t __iomem *)ggtt->gsm;
 	gtt_entries += vma->node.start >> PAGE_SHIFT;
@@ -2731,7 +2733,7 @@ static int ggtt_bind_vma(struct i915_vma *vma,
 
 	/* Applicable to VLV (gen8+ do not support RO in the GGTT) */
 	pte_flags = 0;
-	if (obj->gt_ro)
+	if (i915_gem_object_is_readonly(obj))
 		pte_flags |= PTE_READ_ONLY;
 
 	intel_runtime_pm_get(i915);
@@ -2769,7 +2771,7 @@ static int aliasing_gtt_bind_vma(struct i915_vma *vma,
 
 	/* Currently applicable only to VLV */
 	pte_flags = 0;
-	if (vma->obj->gt_ro)
+	if (i915_gem_object_is_readonly(vma->obj))
 		pte_flags |= PTE_READ_ONLY;
 
 	if (flags & I915_VMA_LOCAL_BIND) {
diff --git a/drivers/gpu/drm/i915/i915_gem_object.h b/drivers/gpu/drm/i915/i915_gem_object.h
index c3c6f2e588fb..56e9f00d2c4c 100644
--- a/drivers/gpu/drm/i915/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/i915_gem_object.h
@@ -141,7 +141,6 @@ struct drm_i915_gem_object {
 	 * Is the object to be mapped as read-only to the GPU
 	 * Only honoured if hardware has relevant pte bit
 	 */
-	unsigned long gt_ro:1;
 	unsigned int cache_level:3;
 	unsigned int cache_coherent:2;
 #define I915_BO_CACHE_COHERENT_FOR_READ BIT(0)
@@ -358,6 +357,18 @@ static inline void i915_gem_object_unlock(struct drm_i915_gem_object *obj)
 	reservation_object_unlock(obj->resv);
 }
 
+static inline void
+i915_gem_object_set_readonly(struct drm_i915_gem_object *obj)
+{
+	obj->base.vma_node.readonly = true;
+}
+
+static inline bool
+i915_gem_object_is_readonly(const struct drm_i915_gem_object *obj)
+{
+	return obj->base.vma_node.readonly;
+}
+
 static inline bool
 i915_gem_object_has_struct_page(const struct drm_i915_gem_object *obj)
 {
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 2106b191781d..33faad3197fe 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1102,7 +1102,7 @@ intel_ring_create_vma(struct drm_i915_private *dev_priv, int size)
 	 * if supported by the platform's GGTT.
 	 */
 	if (vm->has_read_only)
-		obj->gt_ro = 1;
+		i915_gem_object_set_readonly(obj);
 
 	vma = i915_vma_instance(obj, vm, NULL);
 	if (IS_ERR(vma))
diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/selftests/i915_gem_context.c
index e51ca7b4b7ae..b64a09a75b50 100644
--- a/drivers/gpu/drm/i915/selftests/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/selftests/i915_gem_context.c
@@ -487,7 +487,8 @@ static int igt_ctx_readonly(void *arg)
 					goto out_unlock;
 				}
 
-				obj->gt_ro = prandom_u32_state(&prng);
+				if (prandom_u32_state(&prng) & 1)
+					i915_gem_object_set_readonly(obj);
 			}
 
 			intel_runtime_pm_get(i915);
@@ -518,7 +519,7 @@ static int igt_ctx_readonly(void *arg)
 		unsigned int num_writes;
 
 		num_writes = rem;
-		if (obj->gt_ro)
+		if (i915_gem_object_is_readonly(obj))
 			num_writes = 0;
 
 		err = cpu_check(obj, num_writes);
diff --git a/include/drm/drm_vma_manager.h b/include/drm/drm_vma_manager.h
index 8758df94e9a0..c7987daeaed0 100644
--- a/include/drm/drm_vma_manager.h
+++ b/include/drm/drm_vma_manager.h
@@ -41,6 +41,7 @@ struct drm_vma_offset_node {
 	rwlock_t vm_lock;
 	struct drm_mm_node vm_node;
 	struct rb_root vm_files;
+	bool readonly:1;
 };
 
 struct drm_vma_offset_manager {
-- 
2.18.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/6] drm/i915: Reject attempted pwrites into a read-only object

[Chris Wilson]

If the user created a read-only object, they should not be allowed to
circumvent the write protection using the pwrite ioctl.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jon Bloomfield <jon.bloomfield@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Matthew Auld <matthew.william.auld@gmail.com>
Reviewed-by: Jon Bloomfield <jon.bloomfield@intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Matthew Auld <matthew.william.auld@gmail.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1910c66f48e2..42d24410a98c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1627,6 +1627,12 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
 		goto err;
 	}
 
+	/* Writes not allowed into this read-only object */
+	if (i915_gem_object_is_readonly(obj)) {
+		ret = -EINVAL;
+		goto err;
+	}
+
 	trace_i915_gem_object_pwrite(obj, args->offset, args->size);
 
 	ret = -ENODEV;
-- 
2.18.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 6/6] drm/i915/userptr: Enable read-only support on gen8+

[Chris Wilson]

On gen8 and onwards, we can mark GPU accesses through the ppGTT as being
read-only, that is cause any GPU write onto that page to be discarded
(not triggering a fault). This is all that we need to finally support
the read-only flag for userptr!

Testcase: igt/gem_userptr_blits/readonly*
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jon Bloomfield <jon.bloomfield@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Matthew Auld <matthew.william.auld@gmail.com>
Reviewed-by: Jon Bloomfield <jon.bloomfield@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_object.h  |  1 -
 drivers/gpu/drm/i915/i915_gem_userptr.c | 15 +++++++++------
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_object.h b/drivers/gpu/drm/i915/i915_gem_object.h
index 56e9f00d2c4c..83e5e01fa9ea 100644
--- a/drivers/gpu/drm/i915/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/i915_gem_object.h
@@ -267,7 +267,6 @@ struct drm_i915_gem_object {
 	union {
 		struct i915_gem_userptr {
 			uintptr_t ptr;
-			unsigned read_only :1;
 
 			struct i915_mm_struct *mm;
 			struct i915_mmu_object *mmu_object;
diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index 854bd51b9478..045db5ef17ac 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -507,7 +507,7 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
 		struct mm_struct *mm = obj->userptr.mm->mm;
 		unsigned int flags = 0;
 
-		if (!obj->userptr.read_only)
+		if (!i915_gem_object_is_readonly(obj))
 			flags |= FOLL_WRITE;
 
 		ret = -EFAULT;
@@ -643,7 +643,7 @@ static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
 		if (pvec) /* defer to worker if malloc fails */
 			pinned = __get_user_pages_fast(obj->userptr.ptr,
 						       num_pages,
-						       !obj->userptr.read_only,
+						       !i915_gem_object_is_readonly(obj),
 						       pvec);
 	}
 
@@ -789,10 +789,12 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
 		return -EFAULT;
 
 	if (args->flags & I915_USERPTR_READ_ONLY) {
-		/* On almost all of the current hw, we cannot tell the GPU that a
-		 * page is readonly, so this is just a placeholder in the uAPI.
+		/*
+		 * On almost all of the older hw, we cannot tell the GPU that
+		 * a page is readonly.
 		 */
-		return -ENODEV;
+		if (INTEL_GEN(dev_priv) < 8 || !USES_PPGTT(dev_priv))
+			return -ENODEV;
 	}
 
 	obj = i915_gem_object_alloc(dev_priv);
@@ -806,7 +808,8 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
 	i915_gem_object_set_cache_coherency(obj, I915_CACHE_LLC);
 
 	obj->userptr.ptr = args->user_ptr;
-	obj->userptr.read_only = !!(args->flags & I915_USERPTR_READ_ONLY);
+	if (args->flags & I915_USERPTR_READ_ONLY)
+		i915_gem_object_set_readonly(obj);
 
 	/* And keep a pointer to the current->mm for resolving the user pages
 	 * at binding. This means that we need to hook into the mmu_notifier
-- 
2.18.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/6] drm/i915/userptr: Enable read-only support on gen8+

[Chris Wilson]

Quoting Chris Wilson (2018-07-12 19:53:15)
> @@ -789,10 +789,12 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
>                 return -EFAULT;
>  
>         if (args->flags & I915_USERPTR_READ_ONLY) {
> -               /* On almost all of the current hw, we cannot tell the GPU that a
> -                * page is readonly, so this is just a placeholder in the uAPI.
> +               /*
> +                * On almost all of the older hw, we cannot tell the GPU that
> +                * a page is readonly.
>                  */
> -               return -ENODEV;
> +               if (INTEL_GEN(dev_priv) < 8 || !USES_PPGTT(dev_priv))
> +                       return -ENODEV;

Hmm, I need to be more careful here considering gvt.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/6] drm/i915/gtt: Add read only pages to gen8_pte_encode

[Patchwork]

== Series Details ==

Series: series starting with [1/6] drm/i915/gtt: Add read only pages to gen8_pte_encode
URL   : https://patchwork.freedesktop.org/series/46432/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
14eaf2339094 drm/i915/gtt: Add read only pages to gen8_pte_encode
81df64c8eb7c drm/i915/gtt: Read-only pages for insert_entries on bdw+
-:192: WARNING:BOOL_BITFIELD: Avoid using bool as bitfield.  Prefer bool bitfields as unsigned int or u<8|16|32>
#192: FILE: drivers/gpu/drm/i915/i915_gem_gtt.h:336:
+	bool pt_kmap_wc:1;

-:195: WARNING:BOOL_BITFIELD: Avoid using bool as bitfield.  Prefer bool bitfields as unsigned int or u<8|16|32>
#195: FILE: drivers/gpu/drm/i915/i915_gem_gtt.h:339:
+	bool has_read_only:1;

-:271: WARNING:LINE_SPACING: Missing a blank line after declarations
#271: FILE: drivers/gpu/drm/i915/selftests/i915_gem_context.c:441:
+	struct drm_file *file;
+	I915_RND_STATE(prng);

-:342: ERROR:CODE_INDENT: code indent should use tabs where possible
#342: FILE: drivers/gpu/drm/i915/selftests/i915_gem_context.c:512:
+^I       ^Indwords, INTEL_INFO(i915)->num_rings);$

-:342: WARNING:SPACE_BEFORE_TAB: please, no space before tabs
#342: FILE: drivers/gpu/drm/i915/selftests/i915_gem_context.c:512:
+^I       ^Indwords, INTEL_INFO(i915)->num_rings);$

-:342: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#342: FILE: drivers/gpu/drm/i915/selftests/i915_gem_context.c:512:
+	pr_info("Submitted %lu dwords (across %u engines)\n",
+	       	ndwords, INTEL_INFO(i915)->num_rings);

total: 1 errors, 4 warnings, 1 checks, 323 lines checked
b9303f1f2584 drm/i915/gtt: Disable read-only support under GVT
0dd3530a2408 drm/i915: Prevent writing into a read-only object via a GGTT mmap
-:182: WARNING:BOOL_BITFIELD: Avoid using bool as bitfield.  Prefer bool bitfields as unsigned int or u<8|16|32>
#182: FILE: include/drm/drm_vma_manager.h:44:
+	bool readonly:1;

total: 0 errors, 1 warnings, 0 checks, 118 lines checked
607bbdb2cc10 drm/i915: Reject attempted pwrites into a read-only object
eda0213a1a4e drm/i915/userptr: Enable read-only support on gen8+

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.SPARSE: warning for series starting with [1/6] drm/i915/gtt: Add read only pages to gen8_pte_encode

[Patchwork]

== Series Details ==

Series: series starting with [1/6] drm/i915/gtt: Add read only pages to gen8_pte_encode
URL   : https://patchwork.freedesktop.org/series/46432/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Commit: drm/i915/gtt: Add read only pages to gen8_pte_encode
Okay!

Commit: drm/i915/gtt: Read-only pages for insert_entries on bdw+
+drivers/gpu/drm/i915/selftests/i915_gem_context.c:517:25: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/i915_gem_context.c:517:25: warning: expression using sizeof(void)

Commit: drm/i915/gtt: Disable read-only support under GVT
Okay!

Commit: drm/i915: Prevent writing into a read-only object via a GGTT mmap
Okay!

Commit: drm/i915: Reject attempted pwrites into a read-only object
Okay!

Commit: drm/i915/userptr: Enable read-only support on gen8+
Okay!

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915/userptr: Enable read-only support on gen8+

[Chris Wilson]

On gen8 and onwards, we can mark GPU accesses through the ppGTT as being
read-only, that is cause any GPU write onto that page to be discarded
(not triggering a fault). This is all that we need to finally support
the read-only flag for userptr!

v2: Check default address space for read only support as a proxy for the
user context/ppgtt.

Testcase: igt/gem_userptr_blits/readonly*
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jon Bloomfield <jon.bloomfield@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Matthew Auld <matthew.william.auld@gmail.com>
Reviewed-by: Jon Bloomfield <jon.bloomfield@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_object.h  |  1 -
 drivers/gpu/drm/i915/i915_gem_userptr.c | 18 ++++++++++++------
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_object.h b/drivers/gpu/drm/i915/i915_gem_object.h
index 56e9f00d2c4c..83e5e01fa9ea 100644
--- a/drivers/gpu/drm/i915/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/i915_gem_object.h
@@ -267,7 +267,6 @@ struct drm_i915_gem_object {
 	union {
 		struct i915_gem_userptr {
 			uintptr_t ptr;
-			unsigned read_only :1;
 
 			struct i915_mm_struct *mm;
 			struct i915_mmu_object *mmu_object;
diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index 854bd51b9478..dcd6e230d16a 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -507,7 +507,7 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
 		struct mm_struct *mm = obj->userptr.mm->mm;
 		unsigned int flags = 0;
 
-		if (!obj->userptr.read_only)
+		if (!i915_gem_object_is_readonly(obj))
 			flags |= FOLL_WRITE;
 
 		ret = -EFAULT;
@@ -643,7 +643,7 @@ static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
 		if (pvec) /* defer to worker if malloc fails */
 			pinned = __get_user_pages_fast(obj->userptr.ptr,
 						       num_pages,
-						       !obj->userptr.read_only,
+						       !i915_gem_object_is_readonly(obj),
 						       pvec);
 	}
 
@@ -789,10 +789,15 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
 		return -EFAULT;
 
 	if (args->flags & I915_USERPTR_READ_ONLY) {
-		/* On almost all of the current hw, we cannot tell the GPU that a
-		 * page is readonly, so this is just a placeholder in the uAPI.
+		struct i915_hw_ppgtt *ppgtt;
+
+		/*
+		 * On almost all of the older hw, we cannot tell the GPU that
+		 * a page is readonly.
 		 */
-		return -ENODEV;
+		ppgtt = dev_priv->kernel_context->ppgtt;
+		if (!ppgtt || !ppgtt->vm.has_read_only)
+			return -ENODEV;
 	}
 
 	obj = i915_gem_object_alloc(dev_priv);
@@ -806,7 +811,8 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
 	i915_gem_object_set_cache_coherency(obj, I915_CACHE_LLC);
 
 	obj->userptr.ptr = args->user_ptr;
-	obj->userptr.read_only = !!(args->flags & I915_USERPTR_READ_ONLY);
+	if (args->flags & I915_USERPTR_READ_ONLY)
+		i915_gem_object_set_readonly(obj);
 
 	/* And keep a pointer to the current->mm for resolving the user pages
 	 * at binding. This means that we need to hook into the mmu_notifier
-- 
2.18.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/6] drm/i915/gtt: Add read only pages to gen8_pte_encode

[Patchwork]

== Series Details ==

Series: series starting with [1/6] drm/i915/gtt: Add read only pages to gen8_pte_encode
URL   : https://patchwork.freedesktop.org/series/46432/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4479 -> Patchwork_9637 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/46432/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_9637 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@debugfs_test@read_all_entries:
      fi-snb-2520m:       PASS -> INCOMPLETE (fdo#103713)

    
    ==== Possible fixes ====

    igt@drv_module_reload@basic-reload:
      fi-ilk-650:         DMESG-WARN (fdo#106387) -> PASS +2

    igt@kms_chamelium@hdmi-hpd-fast:
      fi-kbl-7500u:       FAIL (fdo#103841, fdo#102672) -> SKIP

    
  fdo#102672 https://bugs.freedesktop.org/show_bug.cgi?id=102672
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#103841 https://bugs.freedesktop.org/show_bug.cgi?id=103841
  fdo#106387 https://bugs.freedesktop.org/show_bug.cgi?id=106387


== Participating hosts (46 -> 42) ==

  Additional (1): fi-byt-j1900 
  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4479 -> Patchwork_9637

  CI_DRM_4479: c9c54a1c37adefd414d51ed919aab3f94794ee35 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4553: 9d25d62bb6ff62694e5226b02d79075eab304402 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9637: eda0213a1a4ed9dd213afef1071fe6653c16041b @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

eda0213a1a4e drm/i915/userptr: Enable read-only support on gen8+
607bbdb2cc10 drm/i915: Reject attempted pwrites into a read-only object
0dd3530a2408 drm/i915: Prevent writing into a read-only object via a GGTT mmap
b9303f1f2584 drm/i915/gtt: Disable read-only support under GVT
81df64c8eb7c drm/i915/gtt: Read-only pages for insert_entries on bdw+
14eaf2339094 drm/i915/gtt: Add read only pages to gen8_pte_encode

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9637/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/6] drm/i915/gtt: Add read only pages to gen8_pte_encode (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/6] drm/i915/gtt: Add read only pages to gen8_pte_encode (rev2)
URL   : https://patchwork.freedesktop.org/series/46432/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
7f97b3f09826 drm/i915/gtt: Add read only pages to gen8_pte_encode
e60d2578b1b0 drm/i915/gtt: Read-only pages for insert_entries on bdw+
-:192: WARNING:BOOL_BITFIELD: Avoid using bool as bitfield.  Prefer bool bitfields as unsigned int or u<8|16|32>
#192: FILE: drivers/gpu/drm/i915/i915_gem_gtt.h:336:
+	bool pt_kmap_wc:1;

-:195: WARNING:BOOL_BITFIELD: Avoid using bool as bitfield.  Prefer bool bitfields as unsigned int or u<8|16|32>
#195: FILE: drivers/gpu/drm/i915/i915_gem_gtt.h:339:
+	bool has_read_only:1;

-:271: WARNING:LINE_SPACING: Missing a blank line after declarations
#271: FILE: drivers/gpu/drm/i915/selftests/i915_gem_context.c:441:
+	struct drm_file *file;
+	I915_RND_STATE(prng);

-:342: ERROR:CODE_INDENT: code indent should use tabs where possible
#342: FILE: drivers/gpu/drm/i915/selftests/i915_gem_context.c:512:
+^I       ^Indwords, INTEL_INFO(i915)->num_rings);$

-:342: WARNING:SPACE_BEFORE_TAB: please, no space before tabs
#342: FILE: drivers/gpu/drm/i915/selftests/i915_gem_context.c:512:
+^I       ^Indwords, INTEL_INFO(i915)->num_rings);$

-:342: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#342: FILE: drivers/gpu/drm/i915/selftests/i915_gem_context.c:512:
+	pr_info("Submitted %lu dwords (across %u engines)\n",
+	       	ndwords, INTEL_INFO(i915)->num_rings);

total: 1 errors, 4 warnings, 1 checks, 323 lines checked
f63aea6e09de drm/i915/gtt: Disable read-only support under GVT
edf93a9f453c drm/i915: Prevent writing into a read-only object via a GGTT mmap
-:182: WARNING:BOOL_BITFIELD: Avoid using bool as bitfield.  Prefer bool bitfields as unsigned int or u<8|16|32>
#182: FILE: include/drm/drm_vma_manager.h:44:
+	bool readonly:1;

total: 0 errors, 1 warnings, 0 checks, 118 lines checked
059c35166df9 drm/i915: Reject attempted pwrites into a read-only object
b8de49af2a4b drm/i915/userptr: Enable read-only support on gen8+

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.SPARSE: warning for series starting with [1/6] drm/i915/gtt: Add read only pages to gen8_pte_encode (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/6] drm/i915/gtt: Add read only pages to gen8_pte_encode (rev2)
URL   : https://patchwork.freedesktop.org/series/46432/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Commit: drm/i915/gtt: Add read only pages to gen8_pte_encode
Okay!

Commit: drm/i915/gtt: Read-only pages for insert_entries on bdw+
+drivers/gpu/drm/i915/selftests/i915_gem_context.c:517:25: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/i915_gem_context.c:517:25: warning: expression using sizeof(void)

Commit: drm/i915/gtt: Disable read-only support under GVT
Okay!

Commit: drm/i915: Prevent writing into a read-only object via a GGTT mmap
Okay!

Commit: drm/i915: Reject attempted pwrites into a read-only object
Okay!

Commit: drm/i915/userptr: Enable read-only support on gen8+
Okay!

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/6] drm/i915/gtt: Add read only pages to gen8_pte_encode (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/6] drm/i915/gtt: Add read only pages to gen8_pte_encode (rev2)
URL   : https://patchwork.freedesktop.org/series/46432/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4479 -> Patchwork_9638 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/46432/revisions/2/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9638:

  === IGT changes ===

    ==== Possible regressions ====

    igt@gem_exec_suspend@basic-s3:
      {fi-cfl-8109u}:     PASS -> DMESG-WARN

    
== Known issues ==

  Here are the changes found in Patchwork_9638 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-bxt-dsi:         PASS -> INCOMPLETE (fdo#103927)

    
    ==== Possible fixes ====

    igt@drv_module_reload@basic-reload:
      fi-ilk-650:         DMESG-WARN (fdo#106387) -> PASS +2

    igt@kms_chamelium@hdmi-hpd-fast:
      fi-kbl-7500u:       FAIL (fdo#102672, fdo#103841) -> SKIP

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-snb-2520m:       INCOMPLETE (fdo#103713) -> PASS

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#102672 https://bugs.freedesktop.org/show_bug.cgi?id=102672
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#103841 https://bugs.freedesktop.org/show_bug.cgi?id=103841
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#106387 https://bugs.freedesktop.org/show_bug.cgi?id=106387


== Participating hosts (46 -> 42) ==

  Additional (1): fi-byt-j1900 
  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4479 -> Patchwork_9638

  CI_DRM_4479: c9c54a1c37adefd414d51ed919aab3f94794ee35 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4553: 9d25d62bb6ff62694e5226b02d79075eab304402 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9638: b8de49af2a4bb5b4276b9fd7a800647bf0f2587f @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

b8de49af2a4b drm/i915/userptr: Enable read-only support on gen8+
059c35166df9 drm/i915: Reject attempted pwrites into a read-only object
edf93a9f453c drm/i915: Prevent writing into a read-only object via a GGTT mmap
f63aea6e09de drm/i915/gtt: Disable read-only support under GVT
e60d2578b1b0 drm/i915/gtt: Read-only pages for insert_entries on bdw+
7f97b3f09826 drm/i915/gtt: Add read only pages to gen8_pte_encode

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9638/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/6] drm/i915/gtt: Disable read-only support under GVT

[Bloomfield, Jon]

> -----Original Message-----
> From: Chris Wilson <chris@chris-wilson.co.uk>
> Sent: Thursday, July 12, 2018 11:53 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: Chris Wilson <chris@chris-wilson.co.uk>; Zhenyu Wang
> <zhenyuw@linux.intel.com>; Bloomfield, Jon <jon.bloomfield@intel.com>;
> Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Matthew Auld
> <matthew.william.auld@gmail.com>
> Subject: [PATCH 3/6] drm/i915/gtt: Disable read-only support under GVT
> 
> GVT is not propagating the PTE bits, and is always setting the
> read-write bit, thus breaking read-only support.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Matthew Auld <matthew.william.auld@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 6c0b438afe46..8e70a45b8a90 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -1662,8 +1662,12 @@ 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;
> +	/*
> +	 * From bdw, there is support for read-only pages in the PPGTT.
> +	 *
> +	 * XXX GVT is not setting honouring the PTE bits.
> +	 */
> +	ppgtt->vm.has_read_only = !intel_vgpu_active(i915);
> 
>  	i915_address_space_init(&ppgtt->vm, i915);
> 
> --
> 2.18.0

Is there a blocker that prevents gvt respecting this bit? I can't think of an obvious
reason why it would be a bad thing to support.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/6] drm/i915/gtt: Disable read-only support under GVT

[Zhenyu Wang]

[-- Attachment #1.1: Type: text/plain, Size: 2285 bytes --]

On 2018.07.12 20:36:03 +0000, Bloomfield, Jon wrote:
> > -----Original Message-----
> > From: Chris Wilson <chris@chris-wilson.co.uk>
> > Sent: Thursday, July 12, 2018 11:53 AM
> > To: intel-gfx@lists.freedesktop.org
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>; Zhenyu Wang
> > <zhenyuw@linux.intel.com>; Bloomfield, Jon <jon.bloomfield@intel.com>;
> > Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Matthew Auld
> > <matthew.william.auld@gmail.com>
> > Subject: [PATCH 3/6] drm/i915/gtt: Disable read-only support under GVT
> > 
> > GVT is not propagating the PTE bits, and is always setting the
> > read-write bit, thus breaking read-only support.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
> > Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Matthew Auld <matthew.william.auld@gmail.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_gtt.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 6c0b438afe46..8e70a45b8a90 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -1662,8 +1662,12 @@ 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;
> > +	/*
> > +	 * From bdw, there is support for read-only pages in the PPGTT.
> > +	 *
> > +	 * XXX GVT is not setting honouring the PTE bits.
> > +	 */
> > +	ppgtt->vm.has_read_only = !intel_vgpu_active(i915);
> > 
> >  	i915_address_space_init(&ppgtt->vm, i915);
> > 
> > --
> > 2.18.0
> 
> Is there a blocker that prevents gvt respecting this bit? I can't think of an obvious
> reason why it would be a bad thing to support.

I don't think any blocker for gvt support, we can respect that bit when shadowing.
But we need capability check on host gvt when that support is ready.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/6] drm/i915/gtt: Disable read-only support under GVT

[Chris Wilson]

Quoting Zhenyu Wang (2018-07-13 03:03:10)
> On 2018.07.12 20:36:03 +0000, Bloomfield, Jon wrote:
> > > -----Original Message-----
> > > From: Chris Wilson <chris@chris-wilson.co.uk>
> > > Sent: Thursday, July 12, 2018 11:53 AM
> > > To: intel-gfx@lists.freedesktop.org
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>; Zhenyu Wang
> > > <zhenyuw@linux.intel.com>; Bloomfield, Jon <jon.bloomfield@intel.com>;
> > > Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Matthew Auld
> > > <matthew.william.auld@gmail.com>
> > > Subject: [PATCH 3/6] drm/i915/gtt: Disable read-only support under GVT
> > > 
> > > GVT is not propagating the PTE bits, and is always setting the
> > > read-write bit, thus breaking read-only support.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
> > > Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > Cc: Matthew Auld <matthew.william.auld@gmail.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem_gtt.c | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > index 6c0b438afe46..8e70a45b8a90 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > @@ -1662,8 +1662,12 @@ 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;
> > > +   /*
> > > +    * From bdw, there is support for read-only pages in the PPGTT.
> > > +    *
> > > +    * XXX GVT is not setting honouring the PTE bits.
> > > +    */
> > > +   ppgtt->vm.has_read_only = !intel_vgpu_active(i915);
> > > 
> > >     i915_address_space_init(&ppgtt->vm, i915);
> > > 
> > > --
> > > 2.18.0
> > 
> > Is there a blocker that prevents gvt respecting this bit? I can't think of an obvious
> > reason why it would be a bad thing to support.
> 
> I don't think any blocker for gvt support, we can respect that bit when shadowing.
> But we need capability check on host gvt when that support is ready.

Another cap bit required, so ack on both sides?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/6] drm/i915/gtt: Disable read-only support under GVT

[Bloomfield, Jon]

> -----Original Message-----
> From: Chris Wilson <chris@chris-wilson.co.uk>
> Sent: Friday, July 13, 2018 1:06 AM
> To: Bloomfield, Jon <jon.bloomfield@intel.com>; Zhenyu Wang
> <zhenyuw@linux.intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Zhenyu Wang
> <zhenyuw@linux.intel.com>; Joonas Lahtinen
> <joonas.lahtinen@linux.intel.com>; Matthew Auld
> <matthew.william.auld@gmail.com>
> Subject: Re: [PATCH 3/6] drm/i915/gtt: Disable read-only support under GVT
> 
> Quoting Zhenyu Wang (2018-07-13 03:03:10)
> > On 2018.07.12 20:36:03 +0000, Bloomfield, Jon wrote:
> > > > -----Original Message-----
> > > > From: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Sent: Thursday, July 12, 2018 11:53 AM
> > > > To: intel-gfx@lists.freedesktop.org
> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>; Zhenyu Wang
> > > > <zhenyuw@linux.intel.com>; Bloomfield, Jon
> <jon.bloomfield@intel.com>;
> > > > Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Matthew Auld
> > > > <matthew.william.auld@gmail.com>
> > > > Subject: [PATCH 3/6] drm/i915/gtt: Disable read-only support under
> GVT
> > > >
> > > > GVT is not propagating the PTE bits, and is always setting the
> > > > read-write bit, thus breaking read-only support.
> > > >
> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
> > > > Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > > Cc: Matthew Auld <matthew.william.auld@gmail.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_gem_gtt.c | 8 ++++++--
> > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > index 6c0b438afe46..8e70a45b8a90 100644
> > > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > @@ -1662,8 +1662,12 @@ 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;
> > > > +   /*
> > > > +    * From bdw, there is support for read-only pages in the PPGTT.
> > > > +    *
> > > > +    * XXX GVT is not setting honouring the PTE bits.
> > > > +    */
> > > > +   ppgtt->vm.has_read_only = !intel_vgpu_active(i915);
> > > >
> > > >     i915_address_space_init(&ppgtt->vm, i915);
> > > >
> > > > --
> > > > 2.18.0
> > >
> > > Is there a blocker that prevents gvt respecting this bit? I can't think of an
> obvious
> > > reason why it would be a bad thing to support.
> >
> > I don't think any blocker for gvt support, we can respect that bit when
> shadowing.
> > But we need capability check on host gvt when that support is ready.
> 
> Another cap bit required, so ack on both sides?
> -Chris
I see. Not as permanent disable, just more plumbing needed.
I'm happy then :-)
Reviewed-by: Jon Bloomfield <jon.bloomfield@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/6] drm/i915/gtt: Disable read-only support under GVT

[Chris Wilson]

Quoting Bloomfield, Jon (2018-07-13 14:51:04)
> > -----Original Message-----
> > From: Chris Wilson <chris@chris-wilson.co.uk>
> > Sent: Friday, July 13, 2018 1:06 AM
> > To: Bloomfield, Jon <jon.bloomfield@intel.com>; Zhenyu Wang
> > <zhenyuw@linux.intel.com>
> > Cc: intel-gfx@lists.freedesktop.org; Zhenyu Wang
> > <zhenyuw@linux.intel.com>; Joonas Lahtinen
> > <joonas.lahtinen@linux.intel.com>; Matthew Auld
> > <matthew.william.auld@gmail.com>
> > Subject: Re: [PATCH 3/6] drm/i915/gtt: Disable read-only support under GVT
> > 
> > Quoting Zhenyu Wang (2018-07-13 03:03:10)
> > > On 2018.07.12 20:36:03 +0000, Bloomfield, Jon wrote:
> > > > > -----Original Message-----
> > > > > From: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > Sent: Thursday, July 12, 2018 11:53 AM
> > > > > To: intel-gfx@lists.freedesktop.org
> > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>; Zhenyu Wang
> > > > > <zhenyuw@linux.intel.com>; Bloomfield, Jon
> > <jon.bloomfield@intel.com>;
> > > > > Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Matthew Auld
> > > > > <matthew.william.auld@gmail.com>
> > > > > Subject: [PATCH 3/6] drm/i915/gtt: Disable read-only support under
> > GVT
> > > > >
> > > > > GVT is not propagating the PTE bits, and is always setting the
> > > > > read-write bit, thus breaking read-only support.
> > > > >
> > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
> > > > > Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> > > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > > > Cc: Matthew Auld <matthew.william.auld@gmail.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_gem_gtt.c | 8 ++++++--
> > > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > > b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > > index 6c0b438afe46..8e70a45b8a90 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > > @@ -1662,8 +1662,12 @@ 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;
> > > > > +   /*
> > > > > +    * From bdw, there is support for read-only pages in the PPGTT.
> > > > > +    *
> > > > > +    * XXX GVT is not setting honouring the PTE bits.
> > > > > +    */
> > > > > +   ppgtt->vm.has_read_only = !intel_vgpu_active(i915);
> > > > >
> > > > >     i915_address_space_init(&ppgtt->vm, i915);
> > > > >
> > > > > --
> > > > > 2.18.0
> > > >
> > > > Is there a blocker that prevents gvt respecting this bit? I can't think of an
> > obvious
> > > > reason why it would be a bad thing to support.
> > >
> > > I don't think any blocker for gvt support, we can respect that bit when
> > shadowing.
> > > But we need capability check on host gvt when that support is ready.
> > 
> > Another cap bit required, so ack on both sides?
> > -Chris
> I see. Not as permanent disable, just more plumbing needed.
> I'm happy then :-)
> Reviewed-by: Jon Bloomfield <jon.bloomfield@intel.com>

With both a check in igt and selftests happy, let's go!

Thanks for your patience,
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx