[PATCH v19 00/12] Support for creating/using Stolen memory backed objects

[PATCH v19 00/12] Support for creating/using Stolen memory backed objects

[ankitprasad.r.sharma]

From: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>

This patch series adds support for creating/using Stolen memory backed
objects.

Despite being a unified memory architecture (UMA) some bits of memory
are more equal than others. In particular we have the thorny issue of
stolen memory, memory stolen from the system by the BIOS and reserved
for igfx use. Stolen memory is required for some functions of the GPU
and display engine, but in general it goes wasted. Whilst we cannot
return it back to the system, we need to find some other method for
utilising it. As we do not support direct access to the physical address
in the stolen region, it behaves like a different class of memory,
closer in kin to local GPU memory. This strongly suggests that we need a
placement model like TTM if we are to fully utilize these discrete
chunks of differing memory.

To add support for creating Stolen memory backed objects, we extend the
drm_i915_gem_create structure, by adding a new flag through which user
can specify the preference to allocate the object from stolen memory,
which if set, an attempt will be made to allocate the object from stolen
memory subject to the availability of free space in the stolen region.

This patch series adds support for clearing buffer objects via CPU/GTT.
This is particularly useful for clearing out the memory from stolen
region, but can also be used for other shmem allocated objects. Currently
being used for buffers allocated in the stolen region. Also adding support
for stealing purgable stolen pages, if we run out of stolen memory when
trying to allocate an object.

v2: Added support for read/write from/to objects not backed by
shmem using the pread/pwrite interface.
Also extended the current get_aperture ioctl to retrieve the
total and available size of the stolen region.

v3: Removed the extended get_aperture ioctl patch 5 (to be submitted as
part of other patch series), addressed comments by Chris about pread/pwrite
for non shmem backed objects.

v4: Rebased to the latest drm-intel-nightly.

v5: Addressed comments, replaced patch 1/4 "Clearing buffers via blitter
engine" by "Clearing buffers via CPU/GTT".

v6: Rebased to the latest drm-intel-nightly, Addressed comments, updated
stolen memory purging logic by maintaining a list for purgable stolen
memory objects, enabled pread/pwrite for all non-shmem backed objects
without tiling restrictions.

v7: Addressed comments, compiler optimization, new patch added for correct
error code propagation to the userspace.

v8: Added a new patch to the series to Migrate stolen objects before
hibernation, as stolen memory is not preserved across hibernation. Added
correct error propagation for shmem as well non-shmem backed object allocation.

v9: Addressed comments, use of insert_page helper function to map object page
by page which can be helpful in low aperture space availability.

v10: Addressed comments, use insert_page for clearing out the stolen memory

v11: Addressed comments, 3 new patches added to support allocation from Stolen
memory
1. Allow use of i915_gem_object_get_dma_address for stolen backed objects
2. Use insert_page for pwrite_fast
3. Fail the execbuff using stolen objects as batchbuffers

v12: Addressed comments, Removed patch "Fail the execbuff using stolen objects
as batchbuffers"

v13: Addressed comments, Added 2 patches to detect Intel RST and disable stolen
for persistent data if RST device found
1. acpi: Export acpi_bus_type
2. drm/i915: Disable use of stolen area by User when Intel RST is present

v14: Addressed comments, Added 2 base patches to the series
1. drm/i915: Add support for mapping an object page by page
2. drm/i915: Introduce i915_gem_object_get_dma_address()

v15: Addressed comments, Disabled stolen memory by default

v16: Addressed comments, Added low level rpm assertions, Enabled stolen
memory

v17: Addressed comments

v18: Rebased and fixed issue

v19: Rebased and added 2 more patches to report mappable and stolen size
numbers
1. drm/i915: Extend GET_APERTURE ioctl to report available map space
2. drm/i915: Extend GET_APERTURE ioctl to report size of the stolen region

This can be verified using IGT tests:
igt/gem_stolen, igt/gem_create, igt/gem_pread, igt/gem_pwrite

Ankitprasad Sharma (8):
  drm/i915: Use insert_page for pwrite_fast
  drm/i915: Clearing buffer objects via CPU/GTT
  drm/i915: Support for creating Stolen memory backed objects
  drm/i915: Propagating correct error codes to the userspace
  drm/i915: Support for pread/pwrite from/to non shmem backed objects
  drm/i915: Disable use of stolen area by User when Intel RST is present
  drm/i915: Extend GET_APERTURE ioctl to report available map space
  drm/i915: Extend GET_APERTURE ioctl to report size of the stolen
    region

Chris Wilson (4):
  drm/i915: Add support for mapping an object page by page
  drm/i915: Introduce i915_gem_object_get_dma_address()
  drm/i915: Add support for stealing purgable stolen pages
  drm/i915: Migrate stolen objects before hibernation

 drivers/char/agp/intel-gtt.c                 |   9 +
 drivers/gpu/drm/i915/i915_debugfs.c          | 149 ++++++-
 drivers/gpu/drm/i915/i915_dma.c              |   3 +
 drivers/gpu/drm/i915/i915_drv.c              |  17 +-
 drivers/gpu/drm/i915/i915_drv.h              |  61 ++-
 drivers/gpu/drm/i915/i915_gem.c              | 635 ++++++++++++++++++++++++---
 drivers/gpu/drm/i915/i915_gem_batch_pool.c   |   4 +-
 drivers/gpu/drm/i915/i915_gem_context.c      |   4 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c          |  67 +++
 drivers/gpu/drm/i915/i915_gem_gtt.h          |   5 +
 drivers/gpu/drm/i915/i915_gem_render_state.c |   7 +-
 drivers/gpu/drm/i915/i915_gem_stolen.c       | 320 ++++++++++++--
 drivers/gpu/drm/i915/i915_guc_submission.c   |  52 ++-
 drivers/gpu/drm/i915/intel_acpi.c            |   7 +
 drivers/gpu/drm/i915/intel_display.c         |   5 +-
 drivers/gpu/drm/i915/intel_fbdev.c           |  12 +-
 drivers/gpu/drm/i915/intel_lrc.c             |  10 +-
 drivers/gpu/drm/i915/intel_overlay.c         |   4 +-
 drivers/gpu/drm/i915/intel_pm.c              |  13 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c      |  27 +-
 include/drm/intel-gtt.h                      |   3 +
 include/uapi/drm/i915_drm.h                  |  52 +++
 22 files changed, 1300 insertions(+), 166 deletions(-)

-- 
1.9.1

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

[PATCH 01/12] drm/i915: Add support for mapping an object page by page

[ankitprasad.r.sharma]

From: Chris Wilson <chris@chris-wilson.co.uk>

Introduced a new vm specfic callback insert_page() to program a single pte in
ggtt or ppgtt. This allows us to map a single page in to the mappable aperture
space. This can be iterated over to access the whole object by using space as
meagre as page size.

v2: Added low level rpm assertions to insert_page routines (Chris)

v3: Added POSTING_READ post register write (Tvrtko)

v4: Rebase (Ankit)

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/char/agp/intel-gtt.c        |  9 +++++
 drivers/gpu/drm/i915/i915_gem_gtt.c | 67 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem_gtt.h |  5 +++
 include/drm/intel-gtt.h             |  3 ++
 4 files changed, 84 insertions(+)

diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
index aef87fd..cea0ae3 100644
--- a/drivers/char/agp/intel-gtt.c
+++ b/drivers/char/agp/intel-gtt.c
@@ -840,6 +840,15 @@ static bool i830_check_flags(unsigned int flags)
 	return false;
 }
 
+void intel_gtt_insert_page(dma_addr_t addr,
+			   unsigned int pg,
+			   unsigned int flags)
+{
+	intel_private.driver->write_entry(addr, pg, flags);
+	wmb();
+}
+EXPORT_SYMBOL(intel_gtt_insert_page);
+
 void intel_gtt_insert_sg_entries(struct sg_table *st,
 				 unsigned int pg_start,
 				 unsigned int flags)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 9f165fe..da1819d 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2353,6 +2353,29 @@ static void gen8_set_pte(void __iomem *addr, gen8_pte_t pte)
 #endif
 }
 
+static void gen8_ggtt_insert_page(struct i915_address_space *vm,
+				  dma_addr_t addr,
+				  uint64_t offset,
+				  enum i915_cache_level level,
+				  u32 unused)
+{
+	struct drm_i915_private *dev_priv = to_i915(vm->dev);
+	gen8_pte_t __iomem *pte =
+		(gen8_pte_t __iomem *)dev_priv->ggtt.gsm +
+		(offset >> PAGE_SHIFT);
+	int rpm_atomic_seq;
+
+	rpm_atomic_seq = assert_rpm_atomic_begin(dev_priv);
+
+	gen8_set_pte(pte, gen8_pte_encode(addr, level, true));
+	wmb();
+
+	I915_WRITE(GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
+	POSTING_READ(GFX_FLSH_CNTL_GEN6);
+
+	assert_rpm_atomic_end(dev_priv, rpm_atomic_seq);
+}
+
 static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
 				     struct sg_table *st,
 				     uint64_t start,
@@ -2425,6 +2448,29 @@ static void gen8_ggtt_insert_entries__BKL(struct i915_address_space *vm,
 	stop_machine(gen8_ggtt_insert_entries__cb, &arg, NULL);
 }
 
+static void gen6_ggtt_insert_page(struct i915_address_space *vm,
+				  dma_addr_t addr,
+				  uint64_t offset,
+				  enum i915_cache_level level,
+				  u32 flags)
+{
+	struct drm_i915_private *dev_priv = to_i915(vm->dev);
+	gen6_pte_t __iomem *pte =
+		(gen6_pte_t __iomem *)dev_priv->ggtt.gsm +
+		(offset >> PAGE_SHIFT);
+	int rpm_atomic_seq;
+
+	rpm_atomic_seq = assert_rpm_atomic_begin(dev_priv);
+
+	iowrite32(vm->pte_encode(addr, level, true, flags), pte);
+	wmb();
+
+	I915_WRITE(GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
+	POSTING_READ(GFX_FLSH_CNTL_GEN6);
+
+	assert_rpm_atomic_end(dev_priv, rpm_atomic_seq);
+}
+
 /*
  * Binds an object into the global gtt with the specified cache level. The object
  * will be accessible to the GPU via commands whose operands reference offsets
@@ -2539,6 +2585,24 @@ static void gen6_ggtt_clear_range(struct i915_address_space *vm,
 	assert_rpm_atomic_end(dev_priv, rpm_atomic_seq);
 }
 
+static void i915_ggtt_insert_page(struct i915_address_space *vm,
+				  dma_addr_t addr,
+				  uint64_t offset,
+				  enum i915_cache_level cache_level,
+				  u32 unused)
+{
+	struct drm_i915_private *dev_priv = to_i915(vm->dev);
+	unsigned int flags = (cache_level == I915_CACHE_NONE) ?
+		AGP_USER_MEMORY : AGP_USER_CACHED_MEMORY;
+	int rpm_atomic_seq;
+
+	rpm_atomic_seq = assert_rpm_atomic_begin(dev_priv);
+
+	intel_gtt_insert_page(addr, offset >> PAGE_SHIFT, flags);
+
+	assert_rpm_atomic_end(dev_priv, rpm_atomic_seq);
+}
+
 static void i915_ggtt_insert_entries(struct i915_address_space *vm,
 				     struct sg_table *pages,
 				     uint64_t start,
@@ -3071,6 +3135,7 @@ static int gen8_gmch_probe(struct i915_ggtt *ggtt)
 	ret = ggtt_probe_common(dev, ggtt->size);
 
 	ggtt->base.clear_range = gen8_ggtt_clear_range;
+	ggtt->base.insert_page = gen8_ggtt_insert_page;
 	if (IS_CHERRYVIEW(dev_priv))
 		ggtt->base.insert_entries = gen8_ggtt_insert_entries__BKL;
 	else
@@ -3109,6 +3174,7 @@ static int gen6_gmch_probe(struct i915_ggtt *ggtt)
 	ret = ggtt_probe_common(dev, ggtt->size);
 
 	ggtt->base.clear_range = gen6_ggtt_clear_range;
+	ggtt->base.insert_page = gen6_ggtt_insert_page;
 	ggtt->base.insert_entries = gen6_ggtt_insert_entries;
 	ggtt->base.bind_vma = ggtt_bind_vma;
 	ggtt->base.unbind_vma = ggtt_unbind_vma;
@@ -3140,6 +3206,7 @@ static int i915_gmch_probe(struct i915_ggtt *ggtt)
 		      &ggtt->mappable_base, &ggtt->mappable_end);
 
 	ggtt->do_idle_maps = needs_idle_maps(dev_priv->dev);
+	ggtt->base.insert_page = i915_ggtt_insert_page;
 	ggtt->base.insert_entries = i915_ggtt_insert_entries;
 	ggtt->base.clear_range = i915_ggtt_clear_range;
 	ggtt->base.bind_vma = ggtt_bind_vma;
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index d7dd3d8..2982a6a 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -316,6 +316,11 @@ struct i915_address_space {
 			    uint64_t start,
 			    uint64_t length,
 			    bool use_scratch);
+	void (*insert_page)(struct i915_address_space *vm,
+			    dma_addr_t addr,
+			    uint64_t offset,
+			    enum i915_cache_level cache_level,
+			    u32 flags);
 	void (*insert_entries)(struct i915_address_space *vm,
 			       struct sg_table *st,
 			       uint64_t start,
diff --git a/include/drm/intel-gtt.h b/include/drm/intel-gtt.h
index 9e9bddaa5..f49edec 100644
--- a/include/drm/intel-gtt.h
+++ b/include/drm/intel-gtt.h
@@ -13,6 +13,9 @@ void intel_gmch_remove(void);
 bool intel_enable_gtt(void);
 
 void intel_gtt_chipset_flush(void);
+void intel_gtt_insert_page(dma_addr_t addr,
+			   unsigned int pg,
+			   unsigned int flags);
 void intel_gtt_insert_sg_entries(struct sg_table *st,
 				 unsigned int pg_start,
 				 unsigned int flags);
-- 
1.9.1

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

[PATCH 02/12] drm/i915: Introduce i915_gem_object_get_dma_address()

[ankitprasad.r.sharma]

From: Chris Wilson <chris@chris-wilson.co.uk>

This utility function is a companion to i915_gem_object_get_page() that
uses the same cached iterator for the scatterlist to perform fast
sequential lookup of the dma address associated with any page within the
object.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b9ed1b3..883b706 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2977,6 +2977,23 @@ static inline int __sg_page_count(struct scatterlist *sg)
 struct page *
 i915_gem_object_get_dirty_page(struct drm_i915_gem_object *obj, int n);
 
+static inline dma_addr_t
+i915_gem_object_get_dma_address(struct drm_i915_gem_object *obj, int n)
+{
+	if (n < obj->get_page.last) {
+		obj->get_page.sg = obj->pages->sgl;
+		obj->get_page.last = 0;
+	}
+
+	while (obj->get_page.last + __sg_page_count(obj->get_page.sg) <= n) {
+		obj->get_page.last += __sg_page_count(obj->get_page.sg++);
+		if (unlikely(sg_is_chain(obj->get_page.sg)))
+			obj->get_page.sg = sg_chain_ptr(obj->get_page.sg);
+	}
+
+	return sg_dma_address(obj->get_page.sg) + ((n - obj->get_page.last) << PAGE_SHIFT);
+}
+
 static inline struct page *
 i915_gem_object_get_page(struct drm_i915_gem_object *obj, int n)
 {
-- 
1.9.1

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

[PATCH 03/12] drm/i915: Use insert_page for pwrite_fast

[ankitprasad.r.sharma]

From: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>

In pwrite_fast, map an object page by page if obj_ggtt_pin fails. First,
we try a nonblocking pin for the whole object (since that is fastest if
reused), then failing that we try to grab one page in the mappable
aperture. It also allows us to handle objects larger than the mappable
aperture (e.g. if we need to pwrite with vGPU restricting the aperture
to a measely 8MiB or something like that).

v2: Pin pages before starting pwrite, Combined duplicate loops (Chris)

v3: Combined loops based on local patch by Chris (Chris)

v4: Added i915 wrapper function for drm_mm_insert_node_in_range (Chris)

v5: Renamed wrapper function for drm_mm_insert_node_in_range (Chris)

v5: Added wrapper for drm_mm_remove_node() (Chris)

v6: Added get_pages call before pinning the pages (Tvrtko)
Added remove_mappable_node() wrapper for drm_mm_remove_node() (Chris)

v7: Added size argument for insert_mappable_node (Tvrtko)

v8: Do not put_pages after pwrite, do memset of node in the wrapper
function (insert_mappable_node) (Chris)

v9: Rebase (Ankit)

Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 90 +++++++++++++++++++++++++++++++----------
 1 file changed, 68 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6ce2c31..0b7ce9d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -60,6 +60,24 @@ static bool cpu_write_needs_clflush(struct drm_i915_gem_object *obj)
 	return obj->pin_display;
 }
 
+static int
+insert_mappable_node(struct drm_i915_private *i915,
+                     struct drm_mm_node *node, u32 size)
+{
+	memset(node, 0, sizeof(*node));
+	return drm_mm_insert_node_in_range_generic(&i915->ggtt.base.mm, node,
+						   size, 0, 0, 0,
+						   i915->ggtt.mappable_end,
+						   DRM_MM_SEARCH_DEFAULT,
+						   DRM_MM_CREATE_DEFAULT);
+}
+
+static void
+remove_mappable_node(struct drm_mm_node *node)
+{
+	drm_mm_remove_node(node);
+}
+
 /* some bookkeeping */
 static void i915_gem_info_add_obj(struct drm_i915_private *dev_priv,
 				  size_t size)
@@ -755,21 +773,34 @@ fast_user_write(struct io_mapping *mapping,
  * user into the GTT, uncached.
  */
 static int
-i915_gem_gtt_pwrite_fast(struct drm_device *dev,
+i915_gem_gtt_pwrite_fast(struct drm_i915_private *i915,
 			 struct drm_i915_gem_object *obj,
 			 struct drm_i915_gem_pwrite *args,
 			 struct drm_file *file)
 {
-	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct i915_ggtt *ggtt = &dev_priv->ggtt;
-	ssize_t remain;
-	loff_t offset, page_base;
+	struct i915_ggtt *ggtt = &i915->ggtt;
+	struct drm_mm_node node;
+	uint64_t remain, offset;
 	char __user *user_data;
-	int page_offset, page_length, ret;
+	int ret;
 
 	ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE | PIN_NONBLOCK);
-	if (ret)
-		goto out;
+	if (ret) {
+		ret = insert_mappable_node(i915, &node, PAGE_SIZE);
+		if (ret)
+			goto out;
+
+		ret = i915_gem_object_get_pages(obj);
+		if (ret) {
+			remove_mappable_node(&node);
+			goto out;
+		}
+
+		i915_gem_object_pin_pages(obj);
+	} else {
+		node.start = i915_gem_obj_ggtt_offset(obj);
+		node.allocated = false;
+	}
 
 	ret = i915_gem_object_set_to_gtt_domain(obj, true);
 	if (ret)
@@ -779,26 +810,32 @@ i915_gem_gtt_pwrite_fast(struct drm_device *dev,
 	if (ret)
 		goto out_unpin;
 
-	user_data = to_user_ptr(args->data_ptr);
-	remain = args->size;
-
-	offset = i915_gem_obj_ggtt_offset(obj) + args->offset;
-
 	intel_fb_obj_invalidate(obj, ORIGIN_GTT);
+	obj->dirty = true;
 
-	while (remain > 0) {
+	user_data = to_user_ptr(args->data_ptr);
+	offset = args->offset;
+	remain = args->size;
+	while (remain) {
 		/* Operation in this page
 		 *
 		 * page_base = page offset within aperture
 		 * page_offset = offset within page
 		 * page_length = bytes to copy for this page
 		 */
-		page_base = offset & PAGE_MASK;
-		page_offset = offset_in_page(offset);
-		page_length = remain;
-		if ((page_offset + remain) > PAGE_SIZE)
-			page_length = PAGE_SIZE - page_offset;
-
+		u32 page_base = node.start;
+		unsigned page_offset = offset_in_page(offset);
+		unsigned page_length = PAGE_SIZE - page_offset;
+		page_length = remain < page_length ? remain : page_length;
+		if (node.allocated) {
+			wmb();
+			ggtt->base.insert_page(&ggtt->base,
+					       i915_gem_object_get_dma_address(obj, offset >> PAGE_SHIFT),
+					       node.start, I915_CACHE_NONE, 0);
+			wmb();
+		} else {
+			page_base += offset & PAGE_MASK;
+		}
 		/* If we get a fault while copying data, then (presumably) our
 		 * source page isn't available.  Return the error and we'll
 		 * retry in the slow path.
@@ -817,7 +854,16 @@ i915_gem_gtt_pwrite_fast(struct drm_device *dev,
 out_flush:
 	intel_fb_obj_flush(obj, false, ORIGIN_GTT);
 out_unpin:
-	i915_gem_object_ggtt_unpin(obj);
+	if (node.allocated) {
+		wmb();
+		ggtt->base.clear_range(&ggtt->base,
+				       node.start, node.size,
+				       true);
+		i915_gem_object_unpin_pages(obj);
+		remove_mappable_node(&node);
+	} else {
+		i915_gem_object_ggtt_unpin(obj);
+	}
 out:
 	return ret;
 }
@@ -1082,7 +1128,7 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
 	if (obj->tiling_mode == I915_TILING_NONE &&
 	    obj->base.write_domain != I915_GEM_DOMAIN_CPU &&
 	    cpu_write_needs_clflush(obj)) {
-		ret = i915_gem_gtt_pwrite_fast(dev, obj, args, file);
+		ret = i915_gem_gtt_pwrite_fast(dev_priv, obj, args, file);
 		/* Note that the gtt paths might fail with non-page-backed user
 		 * pointers (e.g. gtt mappings when moving data between
 		 * textures). Fallback to the shmem path in that case. */
-- 
1.9.1

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

[PATCH 04/12] drm/i915: Clearing buffer objects via CPU/GTT

[ankitprasad.r.sharma]

From: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>

This patch adds support for clearing buffer objects via CPU/GTT. This
is particularly useful for clearing out the non shmem backed objects.
Currently intend to use this only for buffers allocated from stolen
region.

v2: Added kernel doc for i915_gem_clear_object(), corrected/removed
variable assignments (Tvrtko)

v3: Map object page by page to the gtt if the pinning of the whole object
to the ggtt fails, Corrected function name (Chris)

v4: Clear the buffer page by page, and not map the whole object in the gtt
aperture. Use i915 wrapper function in place of drm_mm_insert_node_in_range.

v5: Use renamed wrapper function for drm_mm_insert_node_in_range,
updated barrier positioning (Chris)

v6: Use PAGE_SIZE instead of 4096, use get_pages call before pinning pages
(Tvrtko)

v7: Fixed the onion (undo operation in reverse order) (Chris)

v8: Rebase (Ankit)

Testcase: igt/gem_stolen

Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/i915_gem.c | 45 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 883b706..3e41ac6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2968,6 +2968,7 @@ int i915_gem_obj_prepare_shmem_read(struct drm_i915_gem_object *obj,
 				    int *needs_clflush);
 
 int __must_check i915_gem_object_get_pages(struct drm_i915_gem_object *obj);
+int i915_gem_object_clear(struct drm_i915_gem_object *obj);
 
 static inline int __sg_page_count(struct scatterlist *sg)
 {
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 0b7ce9d..8adb962 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5446,3 +5446,48 @@ fail:
 	drm_gem_object_unreference(&obj->base);
 	return ERR_PTR(ret);
 }
+
+/**
+ * i915_gem_object_clear() - Clear buffer object via CPU/GTT
+ * @obj: Buffer object to be cleared
+ *
+ * Return: 0 - success, non-zero - failure
+ */
+int i915_gem_object_clear(struct drm_i915_gem_object *obj)
+{
+	struct drm_i915_private *i915 = to_i915(obj->base.dev);
+	struct i915_ggtt *ggtt = &i915->ggtt;
+	struct drm_mm_node node;
+	char __iomem *base;
+	uint64_t size = obj->base.size;
+	int ret, i;
+
+	lockdep_assert_held(&obj->base.dev->struct_mutex);
+	ret = insert_mappable_node(i915, &node, PAGE_SIZE);
+	if (ret)
+		return ret;
+
+	ret = i915_gem_object_get_pages(obj);
+	if (ret)
+		goto err_remove_node;
+
+	i915_gem_object_pin_pages(obj);
+	base = io_mapping_map_wc(ggtt->mappable, node.start);
+
+	for (i = 0; i < size/PAGE_SIZE; i++) {
+		ggtt->base.insert_page(&ggtt->base,
+				       i915_gem_object_get_dma_address(obj, i),
+				       node.start, I915_CACHE_NONE, 0);
+		wmb(); /* flush modifications to the GGTT (insert_page) */
+		memset_io(base, 0, PAGE_SIZE);
+		wmb(); /* flush the write before we modify the GGTT */
+	}
+
+	io_mapping_unmap(base);
+	ggtt->base.clear_range(&ggtt->base, node.start, node.size, true);
+	i915_gem_object_unpin_pages(obj);
+
+err_remove_node:
+	remove_mappable_node(&node);
+	return ret;
+}
-- 
1.9.1

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

[PATCH 05/12] drm/i915: Support for creating Stolen memory backed objects

[ankitprasad.r.sharma]

From: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>

Extend the drm_i915_gem_create structure to add support for
creating Stolen memory backed objects. Added a new flag through
which user can specify the preference to allocate the object from
stolen memory, which if set, an attempt will be made to allocate
the object from stolen memory subject to the availability of
free space in the stolen region.

v2: Rebased to the latest drm-intel-nightly (Ankit)

v3: Changed versioning of GEM_CREATE param, added new comments (Tvrtko)

v4: Changed size from 32b to 64b to prevent userspace overflow (Tvrtko)
Corrected function arguments ordering (Chris)

v5: Corrected function name (Chris)

v6: Updated datatype for flags to keep sizeof(drm_i915_gem_create) u64
aligned (Chris)

v7: Use first 8 bits of gem_create flags for placement (Chris), Add helper
function for object allocation from stolen region (Ankit)

v8: Added comment explaining STOLEN placement flag (Chris)

Testcase: igt/gem_stolen

Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_dma.c        |  3 +++
 drivers/gpu/drm/i915/i915_drv.h        |  2 +-
 drivers/gpu/drm/i915/i915_gem.c        | 45 +++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/i915_gem_stolen.c |  4 +--
 include/uapi/drm/i915_drm.h            | 41 +++++++++++++++++++++++++++++++
 5 files changed, 89 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index b377753..235670e 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -232,6 +232,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
 	case I915_PARAM_HAS_EXEC_SOFTPIN:
 		value = 1;
 		break;
+	case I915_PARAM_CREATE_VERSION:
+		value = 2;
+		break;
 	default:
 		DRM_DEBUG("Unknown parameter %d\n", param->param);
 		return -EINVAL;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3e41ac6..c0b8dd9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3383,7 +3383,7 @@ void i915_gem_stolen_remove_node(struct drm_i915_private *dev_priv,
 int i915_gem_init_stolen(struct drm_device *dev);
 void i915_gem_cleanup_stolen(struct drm_device *dev);
 struct drm_i915_gem_object *
-i915_gem_object_create_stolen(struct drm_device *dev, u32 size);
+i915_gem_object_create_stolen(struct drm_device *dev, u64 size);
 struct drm_i915_gem_object *
 i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
 					       u32 stolen_offset,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 8adb962..d0bd3c2 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -384,10 +384,36 @@ void i915_gem_object_free(struct drm_i915_gem_object *obj)
 	kmem_cache_free(dev_priv->objects, obj);
 }
 
+static struct drm_i915_gem_object *
+i915_gem_alloc_object_stolen(struct drm_device *dev, size_t size)
+{
+	struct drm_i915_gem_object *obj;
+	int ret;
+
+	mutex_lock(&dev->struct_mutex);
+	obj = i915_gem_object_create_stolen(dev, size);
+	if (!obj) {
+		mutex_unlock(&dev->struct_mutex);
+		return NULL;
+	}
+
+	/* Always clear fresh buffers before handing to userspace */
+	ret = i915_gem_object_clear(obj);
+	if (ret) {
+		drm_gem_object_unreference(&obj->base);
+		mutex_unlock(&dev->struct_mutex);
+		return NULL;
+	}
+
+	mutex_unlock(&dev->struct_mutex);
+	return obj;
+}
+
 static int
 i915_gem_create(struct drm_file *file,
 		struct drm_device *dev,
 		uint64_t size,
+		uint64_t flags,
 		uint32_t *handle_p)
 {
 	struct drm_i915_gem_object *obj;
@@ -398,8 +424,21 @@ i915_gem_create(struct drm_file *file,
 	if (size == 0)
 		return -EINVAL;
 
+	if (flags & __I915_CREATE_UNKNOWN_FLAGS)
+		return -EINVAL;
+
 	/* Allocate the new object */
-	obj = i915_gem_alloc_object(dev, size);
+	switch (flags & I915_CREATE_PLACEMENT_MASK) {
+	case I915_CREATE_PLACEMENT_NORMAL:
+		obj = i915_gem_alloc_object(dev, size);
+		break;
+	case I915_CREATE_PLACEMENT_STOLEN:
+		obj = i915_gem_alloc_object_stolen(dev, size);
+		break;
+	default:
+		return -EINVAL;
+	}
+
 	if (obj == NULL)
 		return -ENOMEM;
 
@@ -422,7 +461,7 @@ i915_gem_dumb_create(struct drm_file *file,
 	args->pitch = ALIGN(args->width * DIV_ROUND_UP(args->bpp, 8), 64);
 	args->size = args->pitch * args->height;
 	return i915_gem_create(file, dev,
-			       args->size, &args->handle);
+			       args->size, 0, &args->handle);
 }
 
 /**
@@ -435,7 +474,7 @@ i915_gem_create_ioctl(struct drm_device *dev, void *data,
 	struct drm_i915_gem_create *args = data;
 
 	return i915_gem_create(file, dev,
-			       args->size, &args->handle);
+			       args->size, args->flags, &args->handle);
 }
 
 static inline int
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index ea06da0..43a3246 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -595,7 +595,7 @@ cleanup:
 }
 
 struct drm_i915_gem_object *
-i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
+i915_gem_object_create_stolen(struct drm_device *dev, u64 size)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_gem_object *obj;
@@ -605,7 +605,7 @@ i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
 	if (!drm_mm_initialized(&dev_priv->mm.stolen))
 		return NULL;
 
-	DRM_DEBUG_KMS("creating stolen object: size=%x\n", size);
+	DRM_DEBUG_KMS("creating stolen object: size=%llx\n", size);
 	if (size == 0)
 		return NULL;
 
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index a5524cc..dae02d8 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -357,6 +357,7 @@ typedef struct drm_i915_irq_wait {
 #define I915_PARAM_HAS_GPU_RESET	 35
 #define I915_PARAM_HAS_RESOURCE_STREAMER 36
 #define I915_PARAM_HAS_EXEC_SOFTPIN	 37
+#define I915_PARAM_CREATE_VERSION	 38
 
 typedef struct drm_i915_getparam {
 	__s32 param;
@@ -456,6 +457,46 @@ struct drm_i915_gem_create {
 	 */
 	__u32 handle;
 	__u32 pad;
+	/**
+	 * Requested flags (currently used for placement
+	 * (which memory domain))
+	 *
+	 * You can request that the object be created from special memory
+	 * rather than regular system pages using this parameter. Such
+	 * irregular objects may have certain restrictions (such as CPU
+	 * access to a stolen object is verboten).
+	 *
+	 * This can be used in the future for other purposes too
+	 * e.g. specifying tiling/caching/madvise
+	 */
+	__u64 flags;
+#define I915_CREATE_PLACEMENT_NORMAL 	0 /* standard swappable bo  */
+/* Allocate the object from memory reserved for the igfx (stolen).
+ *
+ * Objects allocated from stolen are restricted in the API they can use,
+ * as direct CPU access to stolen memory is prohibited by the system.
+ * This means that you cannot use a regular CPU mmap (either using WB
+ * or with the WC extension). You can still use a GTT mmap, pwrite,
+ * pread and pass it around for use by execbuffer and between processes
+ * like normal.
+ *
+ * Stolen memory is a very limited resource and certain functions of the
+ * hardware can only work from within stolen memory. Userspace's
+ * allocations may be evicted from stolen and moved to normal memory as
+ * required. If the allocation is marked as purgeable (using madvise),
+ * the allocation will be dropped and further access to the object's
+ * backing storage will result in -EFAULT. Stolen objects will also be
+ * migrated to normal memory across suspend and resume, as the stolen
+ * memory is not preserved.
+ *
+ * Stolen memory is regarded as a resource placement hint, most suitable
+ * for medium-sized buffers that are only accessed by the GPU and can be
+ * discarded.
+ */
+#define I915_CREATE_PLACEMENT_STOLEN 	1 /* Cannot use CPU mmaps */
+
+#define I915_CREATE_PLACEMENT_MASK	0xff
+#define __I915_CREATE_UNKNOWN_FLAGS	~I915_CREATE_PLACEMENT_MASK
 };
 
 struct drm_i915_gem_pread {
-- 
1.9.1

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

[PATCH 06/12] drm/i915: Propagating correct error codes to the userspace

[ankitprasad.r.sharma]

From: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>

Propagating correct error codes to userspace by using ERR_PTR and
PTR_ERR macros for stolen memory based object allocation. We generally
return -ENOMEM to the user whenever there is a failure in object
allocation. This patch helps user to identify the correct reason for the
failure and not just -ENOMEM each time.

v2: Moved the patch up in the series, added error propagation for
i915_gem_alloc_object too (Chris)

v3: Removed storing of error pointer inside structs, Corrected error
propagation in caller functions (Chris)

v4: Remove assignments inside the predicate (Chris)

v5: Removed unnecessary initializations, updated kerneldoc for
i915_guc_client, corrected missed error pointer handling (Tvrtko)

v6: Use ERR_CAST/temporary variable to avoid storing invalid pointer
in a common field (Chris)

v7: Resolved rebasing conflicts (Ankit)

v8: Removed redundant code (Chris)

v9: Rebase

v10: Rebase, resolve merge conflicts

Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c              | 23 ++++++------
 drivers/gpu/drm/i915/i915_gem_batch_pool.c   |  4 +--
 drivers/gpu/drm/i915/i915_gem_context.c      |  4 +--
 drivers/gpu/drm/i915/i915_gem_render_state.c |  7 ++--
 drivers/gpu/drm/i915/i915_gem_stolen.c       | 53 +++++++++++++++-------------
 drivers/gpu/drm/i915/i915_guc_submission.c   | 52 +++++++++++++++++----------
 drivers/gpu/drm/i915/intel_display.c         |  2 +-
 drivers/gpu/drm/i915/intel_fbdev.c           |  6 ++--
 drivers/gpu/drm/i915/intel_lrc.c             | 10 +++---
 drivers/gpu/drm/i915/intel_overlay.c         |  4 +--
 drivers/gpu/drm/i915/intel_pm.c              |  7 ++--
 drivers/gpu/drm/i915/intel_ringbuffer.c      | 21 +++++------
 12 files changed, 110 insertions(+), 83 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d0bd3c2..f2affc6 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -392,19 +392,18 @@ i915_gem_alloc_object_stolen(struct drm_device *dev, size_t size)
 
 	mutex_lock(&dev->struct_mutex);
 	obj = i915_gem_object_create_stolen(dev, size);
-	if (!obj) {
-		mutex_unlock(&dev->struct_mutex);
-		return NULL;
-	}
+	if (IS_ERR(obj))
+		goto out;
 
 	/* Always clear fresh buffers before handing to userspace */
 	ret = i915_gem_object_clear(obj);
 	if (ret) {
 		drm_gem_object_unreference(&obj->base);
-		mutex_unlock(&dev->struct_mutex);
-		return NULL;
+		obj = ERR_PTR(ret);
+		goto out;
 	}
 
+out:
 	mutex_unlock(&dev->struct_mutex);
 	return obj;
 }
@@ -439,8 +438,8 @@ i915_gem_create(struct drm_file *file,
 		return -EINVAL;
 	}
 
-	if (obj == NULL)
-		return -ENOMEM;
+	if (IS_ERR(obj))
+		return PTR_ERR(obj);
 
 	ret = drm_gem_handle_create(file, &obj->base, &handle);
 	/* drop reference from allocate - handle holds it now */
@@ -4592,14 +4591,16 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
 	struct drm_i915_gem_object *obj;
 	struct address_space *mapping;
 	gfp_t mask;
+	int ret;
 
 	obj = i915_gem_object_alloc(dev);
 	if (obj == NULL)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
-	if (drm_gem_object_init(dev, &obj->base, size) != 0) {
+	ret = drm_gem_object_init(dev, &obj->base, size);
+	if (ret) {
 		i915_gem_object_free(obj);
-		return NULL;
+		return ERR_PTR(ret);
 	}
 
 	mask = GFP_HIGHUSER | __GFP_RECLAIMABLE;
diff --git a/drivers/gpu/drm/i915/i915_gem_batch_pool.c b/drivers/gpu/drm/i915/i915_gem_batch_pool.c
index 7bf2f3f..d79caa2 100644
--- a/drivers/gpu/drm/i915/i915_gem_batch_pool.c
+++ b/drivers/gpu/drm/i915/i915_gem_batch_pool.c
@@ -135,8 +135,8 @@ i915_gem_batch_pool_get(struct i915_gem_batch_pool *pool,
 		int ret;
 
 		obj = i915_gem_alloc_object(pool->dev, size);
-		if (obj == NULL)
-			return ERR_PTR(-ENOMEM);
+		if (IS_ERR(obj))
+			return obj;
 
 		ret = i915_gem_object_get_pages(obj);
 		if (ret)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index e5acc39..34f4930 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -179,8 +179,8 @@ i915_gem_alloc_context_obj(struct drm_device *dev, size_t size)
 	int ret;
 
 	obj = i915_gem_alloc_object(dev, size);
-	if (obj == NULL)
-		return ERR_PTR(-ENOMEM);
+	if (IS_ERR(obj))
+		return obj;
 
 	/*
 	 * Try to make the context utilize L3 as well as LLC.
diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c
index 71611bf..071c11e 100644
--- a/drivers/gpu/drm/i915/i915_gem_render_state.c
+++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
@@ -58,8 +58,11 @@ static int render_state_init(struct render_state *so, struct drm_device *dev)
 		return -EINVAL;
 
 	so->obj = i915_gem_alloc_object(dev, 4096);
-	if (so->obj == NULL)
-		return -ENOMEM;
+	if (IS_ERR(so->obj)) {
+		ret = PTR_ERR(so->obj);
+		so->obj = NULL;
+		return ret;
+	}
 
 	ret = i915_gem_obj_ggtt_pin(so->obj, 4096, 0);
 	if (ret)
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 43a3246..89ca151 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -501,6 +501,7 @@ i915_pages_create_for_stolen(struct drm_device *dev,
 	struct i915_ggtt *ggtt = &dev_priv->ggtt;
 	struct sg_table *st;
 	struct scatterlist *sg;
+	int ret;
 
 	DRM_DEBUG_DRIVER("offset=0x%x, size=%d\n", offset, size);
 	BUG_ON(offset > ggtt->stolen_size - size);
@@ -512,11 +513,12 @@ i915_pages_create_for_stolen(struct drm_device *dev,
 
 	st = kmalloc(sizeof(*st), GFP_KERNEL);
 	if (st == NULL)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
-	if (sg_alloc_table(st, 1, GFP_KERNEL)) {
+	ret = sg_alloc_table(st, 1, GFP_KERNEL);
+	if (ret) {
 		kfree(st);
-		return NULL;
+		return ERR_PTR(ret);
 	}
 
 	sg = st->sgl;
@@ -565,18 +567,23 @@ _i915_gem_object_create_stolen(struct drm_device *dev,
 			       struct drm_mm_node *stolen)
 {
 	struct drm_i915_gem_object *obj;
+	struct sg_table *pages;
 
 	obj = i915_gem_object_alloc(dev);
 	if (obj == NULL)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	drm_gem_private_object_init(dev, &obj->base, stolen->size);
 	i915_gem_object_init(obj, &i915_gem_object_stolen_ops);
 
-	obj->pages = i915_pages_create_for_stolen(dev,
-						  stolen->start, stolen->size);
-	if (obj->pages == NULL)
-		goto cleanup;
+	pages = i915_pages_create_for_stolen(dev,
+					     stolen->start, stolen->size);
+	if (IS_ERR(pages)) {
+		i915_gem_object_free(obj);
+		return ERR_CAST(pages);
+	}
+
+	obj->pages = pages;
 
 	obj->get_page.sg = obj->pages->sgl;
 	obj->get_page.last = 0;
@@ -588,10 +595,6 @@ _i915_gem_object_create_stolen(struct drm_device *dev,
 	obj->cache_level = HAS_LLC(dev) ? I915_CACHE_LLC : I915_CACHE_NONE;
 
 	return obj;
-
-cleanup:
-	i915_gem_object_free(obj);
-	return NULL;
 }
 
 struct drm_i915_gem_object *
@@ -603,29 +606,29 @@ i915_gem_object_create_stolen(struct drm_device *dev, u64 size)
 	int ret;
 
 	if (!drm_mm_initialized(&dev_priv->mm.stolen))
-		return NULL;
+		return ERR_PTR(-ENODEV);
 
 	DRM_DEBUG_KMS("creating stolen object: size=%llx\n", size);
 	if (size == 0)
-		return NULL;
+		return ERR_PTR(-EINVAL);
 
 	stolen = kzalloc(sizeof(*stolen), GFP_KERNEL);
 	if (!stolen)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	ret = i915_gem_stolen_insert_node(dev_priv, stolen, size, 4096);
 	if (ret) {
 		kfree(stolen);
-		return NULL;
+		return ERR_PTR(ret);
 	}
 
 	obj = _i915_gem_object_create_stolen(dev, stolen);
-	if (obj)
+	if (!IS_ERR(obj))
 		return obj;
 
 	i915_gem_stolen_remove_node(dev_priv, stolen);
 	kfree(stolen);
-	return NULL;
+	return obj;
 }
 
 struct drm_i915_gem_object *
@@ -642,7 +645,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
 	int ret;
 
 	if (!drm_mm_initialized(&dev_priv->mm.stolen))
-		return NULL;
+		return ERR_PTR(-ENODEV);
 
 	lockdep_assert_held(&dev->struct_mutex);
 
@@ -652,11 +655,11 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
 	/* KISS and expect everything to be page-aligned */
 	if (WARN_ON(size == 0) || WARN_ON(size & 4095) ||
 	    WARN_ON(stolen_offset & 4095))
-		return NULL;
+		return ERR_PTR(-EINVAL);
 
 	stolen = kzalloc(sizeof(*stolen), GFP_KERNEL);
 	if (!stolen)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	stolen->start = stolen_offset;
 	stolen->size = size;
@@ -666,15 +669,15 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
 	if (ret) {
 		DRM_DEBUG_KMS("failed to allocate stolen space\n");
 		kfree(stolen);
-		return NULL;
+		return ERR_PTR(ret);
 	}
 
 	obj = _i915_gem_object_create_stolen(dev, stolen);
-	if (obj == NULL) {
+	if (IS_ERR(obj)) {
 		DRM_DEBUG_KMS("failed to allocate stolen object\n");
 		i915_gem_stolen_remove_node(dev_priv, stolen);
 		kfree(stolen);
-		return NULL;
+		return obj;
 	}
 
 	/* Some objects just need physical mem from stolen space */
@@ -713,5 +716,5 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
 
 err:
 	drm_gem_object_unreference(&obj->base);
-	return NULL;
+	return ERR_PTR(ret);
 }
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index da86bdb..c0fab1b 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -608,27 +608,30 @@ int i915_guc_submit(struct i915_guc_client *client,
  * object needs to be pinned lifetime. Also we must pin it to gtt space other
  * than [0, GUC_WOPCM_TOP) because this range is reserved inside GuC.
  *
- * Return:	A drm_i915_gem_object if successful, otherwise NULL.
+ * Return:	A drm_i915_gem_object if successful, otherwise error pointer.
  */
 static struct drm_i915_gem_object *gem_allocate_guc_obj(struct drm_device *dev,
 							u32 size)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_gem_object *obj;
+	int ret;
 
 	obj = i915_gem_alloc_object(dev, size);
-	if (!obj)
-		return NULL;
+	if (IS_ERR(obj))
+		return obj;
 
-	if (i915_gem_object_get_pages(obj)) {
+	ret = i915_gem_object_get_pages(obj);
+	if (ret) {
 		drm_gem_object_unreference(&obj->base);
-		return NULL;
+		return ERR_PTR(ret);
 	}
 
-	if (i915_gem_obj_ggtt_pin(obj, PAGE_SIZE,
-			PIN_OFFSET_BIAS | GUC_WOPCM_TOP)) {
+	ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE,
+				    PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
+	if (ret) {
 		drm_gem_object_unreference(&obj->base);
-		return NULL;
+		return ERR_PTR(ret);
 	}
 
 	/* Invalidate GuC TLB to let GuC take the latest updates to GTT. */
@@ -696,7 +699,7 @@ static void guc_client_free(struct drm_device *dev,
  * @ctx:	the context that owns the client (we use the default render
  * 		context)
  *
- * Return:	An i915_guc_client object if success.
+ * Return:	An i915_guc_client object if success, error pointer on failure.
  */
 static struct i915_guc_client *guc_client_alloc(struct drm_device *dev,
 						uint32_t priority,
@@ -706,10 +709,11 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_guc *guc = &dev_priv->guc;
 	struct drm_i915_gem_object *obj;
+	int ret;
 
 	client = kzalloc(sizeof(*client), GFP_KERNEL);
 	if (!client)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	client->doorbell_id = GUC_INVALID_DOORBELL_ID;
 	client->priority = priority;
@@ -720,13 +724,16 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev,
 			GUC_MAX_GPU_CONTEXTS, GFP_KERNEL);
 	if (client->ctx_index >= GUC_MAX_GPU_CONTEXTS) {
 		client->ctx_index = GUC_INVALID_CTX_ID;
+		ret = -EINVAL;
 		goto err;
 	}
 
 	/* The first page is doorbell/proc_desc. Two followed pages are wq. */
 	obj = gem_allocate_guc_obj(dev, GUC_DB_SIZE + GUC_WQ_SIZE);
-	if (!obj)
+	if (IS_ERR(obj)) {
+		ret = PTR_ERR(obj);
 		goto err;
+	}
 
 	client->client_obj = obj;
 	client->wq_offset = GUC_DB_SIZE;
@@ -745,9 +752,11 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev,
 		client->proc_desc_offset = (GUC_DB_SIZE / 2);
 
 	client->doorbell_id = assign_doorbell(guc, client->priority);
-	if (client->doorbell_id == GUC_INVALID_DOORBELL_ID)
+	if (client->doorbell_id == GUC_INVALID_DOORBELL_ID) {
 		/* XXX: evict a doorbell instead */
+		ret = -EINVAL;
 		goto err;
+	}
 
 	guc_init_proc_desc(guc, client);
 	guc_init_ctx_desc(guc, client);
@@ -755,7 +764,8 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev,
 
 	/* XXX: Any cache flushes needed? General domain mgmt calls? */
 
-	if (host2guc_allocate_doorbell(guc, client))
+	ret = host2guc_allocate_doorbell(guc, client);
+	if (ret)
 		goto err;
 
 	DRM_DEBUG_DRIVER("new priority %u client %p: ctx_index %u db_id %u\n",
@@ -767,7 +777,7 @@ err:
 	DRM_ERROR("FAILED to create priority %u GuC client!\n", priority);
 
 	guc_client_free(dev, client);
-	return NULL;
+	return ERR_PTR(ret);
 }
 
 static void guc_create_log(struct intel_guc *guc)
@@ -792,7 +802,7 @@ static void guc_create_log(struct intel_guc *guc)
 	obj = guc->log_obj;
 	if (!obj) {
 		obj = gem_allocate_guc_obj(dev_priv->dev, size);
-		if (!obj) {
+		if (IS_ERR(obj)) {
 			/* logging will be off */
 			i915.guc_log_level = -1;
 			return;
@@ -912,6 +922,7 @@ int i915_guc_submission_init(struct drm_device *dev)
 	const size_t poolsize = GUC_MAX_GPU_CONTEXTS * ctxsize;
 	const size_t gemsize = round_up(poolsize, PAGE_SIZE);
 	struct intel_guc *guc = &dev_priv->guc;
+	int ret;
 
 	if (!i915.enable_guc_submission)
 		return 0; /* not enabled  */
@@ -920,8 +931,11 @@ int i915_guc_submission_init(struct drm_device *dev)
 		return 0; /* already allocated */
 
 	guc->ctx_pool_obj = gem_allocate_guc_obj(dev_priv->dev, gemsize);
-	if (!guc->ctx_pool_obj)
-		return -ENOMEM;
+	if (IS_ERR(guc->ctx_pool_obj)) {
+		ret = PTR_ERR(guc->ctx_pool_obj);
+		guc->ctx_pool_obj = NULL;
+		return ret;
+	}
 
 	ida_init(&guc->ctx_ids);
 
@@ -941,9 +955,9 @@ int i915_guc_submission_enable(struct drm_device *dev)
 
 	/* client for execbuf submission */
 	client = guc_client_alloc(dev, GUC_CTX_PRIORITY_KMD_NORMAL, ctx);
-	if (!client) {
+	if (IS_ERR(client)) {
 		DRM_ERROR("Failed to create execbuf guc_client\n");
-		return -ENOMEM;
+		return PTR_ERR(client);
 	}
 
 	guc->execbuf_client = client;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9397910..66072bf 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2474,7 +2474,7 @@ intel_alloc_initial_plane_obj(struct intel_crtc *crtc,
 							     base_aligned,
 							     base_aligned,
 							     size_aligned);
-	if (!obj) {
+	if (IS_ERR(obj)) {
 		mutex_unlock(&dev->struct_mutex);
 		return false;
 	}
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 79ac202..1686687 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -149,11 +149,11 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 	 * features. */
 	if (size * 2 < ggtt->stolen_usable_size)
 		obj = i915_gem_object_create_stolen(dev, size);
-	if (obj == NULL)
+	if (IS_ERR_OR_NULL(obj))
 		obj = i915_gem_alloc_object(dev, size);
-	if (!obj) {
+	if (IS_ERR(obj)) {
 		DRM_ERROR("failed to allocate framebuffer\n");
-		ret = -ENOMEM;
+		ret = PTR_ERR(obj);
 		goto out;
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 1562a75..c23b891 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1476,9 +1476,11 @@ static int lrc_setup_wa_ctx_obj(struct intel_engine_cs *engine, u32 size)
 
 	engine->wa_ctx.obj = i915_gem_alloc_object(engine->dev,
 						   PAGE_ALIGN(size));
-	if (!engine->wa_ctx.obj) {
+	if (IS_ERR(engine->wa_ctx.obj)) {
 		DRM_DEBUG_DRIVER("alloc LRC WA ctx backing obj failed.\n");
-		return -ENOMEM;
+		ret = PTR_ERR(engine->wa_ctx.obj);
+		engine->wa_ctx.obj = NULL;
+		return ret;
 	}
 
 	ret = i915_gem_obj_ggtt_pin(engine->wa_ctx.obj, PAGE_SIZE, 0);
@@ -2668,9 +2670,9 @@ int intel_lr_context_deferred_alloc(struct intel_context *ctx,
 	context_size += PAGE_SIZE * LRC_PPHWSP_PN;
 
 	ctx_obj = i915_gem_alloc_object(dev, context_size);
-	if (!ctx_obj) {
+	if (IS_ERR(ctx_obj)) {
 		DRM_DEBUG_DRIVER("Alloc LRC backing obj failed.\n");
-		return -ENOMEM;
+		return PTR_ERR(ctx_obj);
 	}
 
 	ringbuf = intel_engine_create_ringbuffer(engine, 4 * PAGE_SIZE);
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index bcc3b6a..7e4c389 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -1395,9 +1395,9 @@ void intel_setup_overlay(struct drm_device *dev)
 	reg_bo = NULL;
 	if (!OVERLAY_NEEDS_PHYSICAL(dev))
 		reg_bo = i915_gem_object_create_stolen(dev, PAGE_SIZE);
-	if (reg_bo == NULL)
+	if (IS_ERR_OR_NULL(reg_bo))
 		reg_bo = i915_gem_alloc_object(dev, PAGE_SIZE);
-	if (reg_bo == NULL)
+	if (IS_ERR(reg_bo))
 		goto out_free;
 	overlay->reg_bo = reg_bo;
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b7c2186..2863f01 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5342,10 +5342,13 @@ static void valleyview_setup_pctx(struct drm_device *dev)
 								      pcbr_offset,
 								      I915_GTT_OFFSET_NONE,
 								      pctx_size);
+		if (IS_ERR(pctx))
+			pctx = NULL;
+
 		goto out;
 	}
 
-	DRM_DEBUG_DRIVER("BIOS didn't set up PCBR, fixing up\n");
+	DRM_DEBUG_DRIVER("BIOS didn't set up PCBR or prealloc failed, fixing up\n");
 
 	/*
 	 * From the Gunit register HAS:
@@ -5356,7 +5359,7 @@ static void valleyview_setup_pctx(struct drm_device *dev)
 	 * memory, or any other relevant ranges.
 	 */
 	pctx = i915_gem_object_create_stolen(dev, pctx_size);
-	if (!pctx) {
+	if (IS_ERR(pctx)) {
 		DRM_DEBUG("not enough stolen space for PCTX, disabling\n");
 		goto out;
 	}
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 0d24494..fc5f8ae 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -672,9 +672,10 @@ intel_init_pipe_control(struct intel_engine_cs *engine)
 	WARN_ON(engine->scratch.obj);
 
 	engine->scratch.obj = i915_gem_alloc_object(engine->dev, 4096);
-	if (engine->scratch.obj == NULL) {
+	if (IS_ERR(engine->scratch.obj)) {
 		DRM_ERROR("Failed to allocate seqno page\n");
-		ret = -ENOMEM;
+		ret = PTR_ERR(engine->scratch.obj);
+		engine->scratch.obj = NULL;
 		goto err;
 	}
 
@@ -2022,9 +2023,9 @@ static int init_status_page(struct intel_engine_cs *engine)
 		int ret;
 
 		obj = i915_gem_alloc_object(engine->dev, 4096);
-		if (obj == NULL) {
+		if (IS_ERR(obj)) {
 			DRM_ERROR("Failed to allocate status page\n");
-			return -ENOMEM;
+			return PTR_ERR(obj);
 		}
 
 		ret = i915_gem_object_set_cache_level(obj, I915_CACHE_LLC);
@@ -2158,10 +2159,10 @@ static int intel_alloc_ringbuffer_obj(struct drm_device *dev,
 	obj = NULL;
 	if (!HAS_LLC(dev))
 		obj = i915_gem_object_create_stolen(dev, ringbuf->size);
-	if (obj == NULL)
+	if (IS_ERR_OR_NULL(obj))
 		obj = i915_gem_alloc_object(dev, ringbuf->size);
-	if (obj == NULL)
-		return -ENOMEM;
+	if (IS_ERR(obj))
+		return PTR_ERR(obj);
 
 	/* mark ring buffers as read-only from GPU side by default */
 	obj->gt_ro = 1;
@@ -2778,7 +2779,7 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
 	if (INTEL_INFO(dev)->gen >= 8) {
 		if (i915_semaphore_is_enabled(dev)) {
 			obj = i915_gem_alloc_object(dev, 4096);
-			if (obj == NULL) {
+			if (IS_ERR(obj)) {
 				DRM_ERROR("Failed to allocate semaphore bo. Disabling semaphores\n");
 				i915.semaphores = 0;
 			} else {
@@ -2887,9 +2888,9 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
 	/* Workaround batchbuffer to combat CS tlb bug. */
 	if (HAS_BROKEN_CS_TLB(dev)) {
 		obj = i915_gem_alloc_object(dev, I830_WA_SIZE);
-		if (obj == NULL) {
+		if (IS_ERR(obj)) {
 			DRM_ERROR("Failed to allocate batch bo\n");
-			return -ENOMEM;
+			return PTR_ERR(obj);
 		}
 
 		ret = i915_gem_obj_ggtt_pin(obj, 0, 0);
-- 
1.9.1

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

[PATCH 07/12] drm/i915: Add support for stealing purgable stolen pages

[ankitprasad.r.sharma]

From: Chris Wilson <chris at chris-wilson.co.uk>

If we run out of stolen memory when trying to allocate an object, see if
we can reap enough purgeable objects to free up enough contiguous free
space for the allocation. This is in principle very much like evicting
objects to free up enough contiguous space in the vma when binding
a new object - and you will be forgiven for thinking that the code looks
very similar.

At the moment, we do not allow userspace to allocate objects in stolen,
so there is neither the memory pressure to trigger stolen eviction nor
any purgeable objects inside the stolen arena. However, this will change
in the near future, and so better management and defragmentation of
stolen memory will become a real issue.

v2: Remember to remove the drm_mm_node.

v3: Rebased to the latest drm-intel-nightly (Ankit)

v4: corrected if-else braces format (Tvrtko/kerneldoc)

v5: Rebased to the latest drm-intel-nightly (Ankit)
Added a seperate list to maintain purgable objects from stolen memory
region (Chris/Daniel)

v6: Compiler optimization (merging 2 single loops into one for() loop),
corrected code for object eviction, retire_requests before starting
object eviction (Chris)

v7: Added kernel doc for i915_gem_object_create_stolen()

v8: Check for struct_mutex lock before creating object from stolen
region (Tvrtko)

v9: Renamed variables to make usage clear, added comment, removed onetime
used macro (Tvrtko)

v10: Avoid masking of error when stolen_alloc fails (Tvrtko)

v11: Renamed stolen_link to tmp_link, as it may be used for other
purposes too (Chris)
Used ERR_CAST to cast error pointers while returning

v12: Added lockdep_assert before starting stolen-backed object
eviction (Chris)

v13: Rebased

Testcase: igt/gem_stolen

Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c    |   6 +-
 drivers/gpu/drm/i915/i915_drv.h        |  17 +++-
 drivers/gpu/drm/i915/i915_gem.c        |  15 +++
 drivers/gpu/drm/i915/i915_gem_stolen.c | 171 +++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_pm.c        |   4 +-
 5 files changed, 188 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 46cc03b..7f94c6a 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -174,7 +174,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
 		seq_puts(m, ")");
 	}
 	if (obj->stolen)
-		seq_printf(m, " (stolen: %08llx)", obj->stolen->start);
+		seq_printf(m, " (stolen: %08llx)", obj->stolen->base.start);
 	if (obj->pin_display || obj->fault_mappable) {
 		char s[3], *t = s;
 		if (obj->pin_display)
@@ -253,9 +253,9 @@ static int obj_rank_by_stolen(void *priv,
 	struct drm_i915_gem_object *b =
 		container_of(B, struct drm_i915_gem_object, obj_exec_link);
 
-	if (a->stolen->start < b->stolen->start)
+	if (a->stolen->base.start < b->stolen->base.start)
 		return -1;
-	if (a->stolen->start > b->stolen->start)
+	if (a->stolen->base.start > b->stolen->base.start)
 		return 1;
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c0b8dd9..d998f6c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -818,6 +818,12 @@ struct i915_ctx_hang_stats {
 	bool banned;
 };
 
+struct i915_stolen_node {
+	struct drm_mm_node base;
+	struct list_head mm_link;
+	struct drm_i915_gem_object *obj;
+};
+
 /* This must match up with the value previously used for execbuf2.rsvd1. */
 #define DEFAULT_CONTEXT_HANDLE 0
 
@@ -1267,6 +1273,13 @@ struct i915_gem_mm {
 	 */
 	struct list_head unbound_list;
 
+	/**
+	 * List of stolen objects that have been marked as purgeable and
+	 * thus available for reaping if we need more space for a new
+	 * allocation. Ordered by time of marking purgeable.
+	 */
+	struct list_head stolen_list;
+
 	/** Usable portion of the GTT for GEM */
 	unsigned long stolen_base; /* limited to low memory (32-bit) */
 
@@ -2104,7 +2117,7 @@ struct drm_i915_gem_object {
 	struct list_head vma_list;
 
 	/** Stolen memory for this object, instead of being backed by shmem. */
-	struct drm_mm_node *stolen;
+	struct i915_stolen_node *stolen;
 	struct list_head global_list;
 
 	struct list_head engine_list[I915_NUM_ENGINES];
@@ -2112,6 +2125,8 @@ struct drm_i915_gem_object {
 	struct list_head obj_exec_link;
 
 	struct list_head batch_pool_link;
+	/** Used to link an object to a list temporarily */
+	struct list_head tmp_link;
 
 	/**
 	 * This is set if the object is on the active lists (has pending
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index f2affc6..c5ba1bb 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4550,6 +4550,20 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data,
 	if (obj->madv == I915_MADV_DONTNEED && obj->pages == NULL)
 		i915_gem_object_truncate(obj);
 
+	if (obj->stolen) {
+		switch (obj->madv) {
+		case I915_MADV_WILLNEED:
+			list_del_init(&obj->stolen->mm_link);
+			break;
+		case I915_MADV_DONTNEED:
+			list_move(&obj->stolen->mm_link,
+				  &dev_priv->mm.stolen_list);
+			break;
+		default:
+			break;
+		}
+	}
+
 	args->retained = obj->madv != __I915_MADV_PURGED;
 
 out:
@@ -5200,6 +5214,7 @@ i915_gem_load_init(struct drm_device *dev)
 	INIT_LIST_HEAD(&dev_priv->context_list);
 	INIT_LIST_HEAD(&dev_priv->mm.unbound_list);
 	INIT_LIST_HEAD(&dev_priv->mm.bound_list);
+	INIT_LIST_HEAD(&dev_priv->mm.stolen_list);
 	INIT_LIST_HEAD(&dev_priv->mm.fence_list);
 	for (i = 0; i < I915_NUM_ENGINES; i++)
 		init_engine_lists(&dev_priv->engine[i]);
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 89ca151..b8573a6 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -551,7 +551,8 @@ i915_gem_object_release_stolen(struct drm_i915_gem_object *obj)
 	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
 
 	if (obj->stolen) {
-		i915_gem_stolen_remove_node(dev_priv, obj->stolen);
+		list_del(&obj->stolen->mm_link);
+		i915_gem_stolen_remove_node(dev_priv, &obj->stolen->base);
 		kfree(obj->stolen);
 		obj->stolen = NULL;
 	}
@@ -564,7 +565,7 @@ static const struct drm_i915_gem_object_ops i915_gem_object_stolen_ops = {
 
 static struct drm_i915_gem_object *
 _i915_gem_object_create_stolen(struct drm_device *dev,
-			       struct drm_mm_node *stolen)
+			       struct i915_stolen_node *stolen)
 {
 	struct drm_i915_gem_object *obj;
 	struct sg_table *pages;
@@ -573,11 +574,12 @@ _i915_gem_object_create_stolen(struct drm_device *dev,
 	if (obj == NULL)
 		return ERR_PTR(-ENOMEM);
 
-	drm_gem_private_object_init(dev, &obj->base, stolen->size);
+	drm_gem_private_object_init(dev, &obj->base, stolen->base.size);
 	i915_gem_object_init(obj, &i915_gem_object_stolen_ops);
 
 	pages = i915_pages_create_for_stolen(dev,
-					     stolen->start, stolen->size);
+					     stolen->base.start,
+					     stolen->base.size);
 	if (IS_ERR(pages)) {
 		i915_gem_object_free(obj);
 		return ERR_CAST(pages);
@@ -591,24 +593,112 @@ _i915_gem_object_create_stolen(struct drm_device *dev,
 	i915_gem_object_pin_pages(obj);
 	obj->stolen = stolen;
 
+	stolen->obj = obj;
+	INIT_LIST_HEAD(&stolen->mm_link);
+
 	obj->base.read_domains = I915_GEM_DOMAIN_CPU | I915_GEM_DOMAIN_GTT;
 	obj->cache_level = HAS_LLC(dev) ? I915_CACHE_LLC : I915_CACHE_NONE;
 
 	return obj;
 }
 
-struct drm_i915_gem_object *
-i915_gem_object_create_stolen(struct drm_device *dev, u64 size)
+static bool
+mark_free(struct drm_i915_gem_object *obj, struct list_head *unwind)
+{
+	BUG_ON(obj->stolen == NULL);
+
+	if (obj->madv != I915_MADV_DONTNEED)
+		return false;
+
+	if (obj->pin_display)
+		return false;
+
+	list_add(&obj->tmp_link, unwind);
+	return drm_mm_scan_add_block(&obj->stolen->base);
+}
+
+static int
+stolen_evict(struct drm_i915_private *dev_priv, u64 size)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_gem_object *obj;
-	struct drm_mm_node *stolen;
-	int ret;
+	struct list_head unwind, evict;
+	struct i915_stolen_node *iter;
+	int ret, active;
 
-	if (!drm_mm_initialized(&dev_priv->mm.stolen))
-		return ERR_PTR(-ENODEV);
+	lockdep_assert_held(&dev_priv->dev->struct_mutex);
+	drm_mm_init_scan(&dev_priv->mm.stolen, size, 0, 0);
+	INIT_LIST_HEAD(&unwind);
+
+	/* Retire all requests before creating the evict list */
+	i915_gem_retire_requests(dev_priv->dev);
+
+	for (active = 0; active <= 1; active++) {
+		list_for_each_entry(iter, &dev_priv->mm.stolen_list, mm_link) {
+			if (iter->obj->active != active)
+				continue;
+
+			if (mark_free(iter->obj, &unwind))
+				goto found;
+		}
+	}
+
+found:
+	INIT_LIST_HEAD(&evict);
+	while (!list_empty(&unwind)) {
+		obj = list_first_entry(&unwind,
+				       struct drm_i915_gem_object,
+				       tmp_link);
+		list_del(&obj->tmp_link);
+
+		if (drm_mm_scan_remove_block(&obj->stolen->base)) {
+			list_add(&obj->tmp_link, &evict);
+			drm_gem_object_reference(&obj->base);
+		}
+	}
+
+	ret = 0;
+	while (!list_empty(&evict)) {
+		obj = list_first_entry(&evict,
+				       struct drm_i915_gem_object,
+				       tmp_link);
+		list_del(&obj->tmp_link);
+
+		if (ret == 0) {
+			struct i915_vma *vma, *vma_next;
+
+			list_for_each_entry_safe(vma, vma_next,
+						 &obj->vma_list,
+						 obj_link)
+				if (i915_vma_unbind(vma))
+					break;
+
+			/* Stolen pins its pages to prevent the
+			 * normal shrinker from processing stolen
+			 * objects.
+			 */
+			i915_gem_object_unpin_pages(obj);
+
+			ret = i915_gem_object_put_pages(obj);
+			if (ret == 0) {
+				i915_gem_object_release_stolen(obj);
+				obj->madv = __I915_MADV_PURGED;
+			} else {
+				i915_gem_object_pin_pages(obj);
+			}
+		}
+
+		drm_gem_object_unreference(&obj->base);
+	}
+
+	return ret;
+}
+
+static struct i915_stolen_node *
+stolen_alloc(struct drm_i915_private *dev_priv, u64 size)
+{
+	struct i915_stolen_node *stolen;
+	int ret;
 
-	DRM_DEBUG_KMS("creating stolen object: size=%llx\n", size);
 	if (size == 0)
 		return ERR_PTR(-EINVAL);
 
@@ -616,17 +706,60 @@ i915_gem_object_create_stolen(struct drm_device *dev, u64 size)
 	if (!stolen)
 		return ERR_PTR(-ENOMEM);
 
-	ret = i915_gem_stolen_insert_node(dev_priv, stolen, size, 4096);
+	ret = i915_gem_stolen_insert_node(dev_priv, &stolen->base, size, 4096);
+	if (ret == 0)
+		goto out;
+
+	/* No more stolen memory available, or too fragmented.
+	 * Try evicting purgeable objects and search again.
+	 */
+	ret = stolen_evict(dev_priv, size);
+	if (ret == 0)
+		ret = i915_gem_stolen_insert_node(dev_priv, &stolen->base,
+						  size, 4096);
+out:
 	if (ret) {
 		kfree(stolen);
 		return ERR_PTR(ret);
 	}
 
+	return stolen;
+}
+
+/**
+ * i915_gem_object_create_stolen() - creates object using the stolen memory
+ * @dev:	drm device
+ * @size:	size of the object requested
+ *
+ * i915_gem_object_create_stolen() tries to allocate memory for the object
+ * from the stolen memory region. If not enough memory is found, it tries
+ * evicting purgeable objects and searching again.
+ *
+ * Returns: Object pointer - success and error pointer - failure
+ */
+struct drm_i915_gem_object *
+i915_gem_object_create_stolen(struct drm_device *dev, u64 size)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_gem_object *obj;
+	struct i915_stolen_node *stolen;
+
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
+	if (!drm_mm_initialized(&dev_priv->mm.stolen))
+		return ERR_PTR(-ENODEV);
+
+	DRM_DEBUG_KMS("creating stolen object: size=%llx\n", size);
+
+	stolen = stolen_alloc(dev_priv, size);
+	if (IS_ERR(stolen))
+		return ERR_CAST(stolen);
+
 	obj = _i915_gem_object_create_stolen(dev, stolen);
 	if (!IS_ERR(obj))
 		return obj;
 
-	i915_gem_stolen_remove_node(dev_priv, stolen);
+	i915_gem_stolen_remove_node(dev_priv, &stolen->base);
 	kfree(stolen);
 	return obj;
 }
@@ -640,7 +773,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct i915_ggtt *ggtt = &dev_priv->ggtt;
 	struct drm_i915_gem_object *obj;
-	struct drm_mm_node *stolen;
+	struct i915_stolen_node *stolen;
 	struct i915_vma *vma;
 	int ret;
 
@@ -661,10 +794,10 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
 	if (!stolen)
 		return ERR_PTR(-ENOMEM);
 
-	stolen->start = stolen_offset;
-	stolen->size = size;
+	stolen->base.start = stolen_offset;
+	stolen->base.size = size;
 	mutex_lock(&dev_priv->mm.stolen_lock);
-	ret = drm_mm_reserve_node(&dev_priv->mm.stolen, stolen);
+	ret = drm_mm_reserve_node(&dev_priv->mm.stolen, &stolen->base);
 	mutex_unlock(&dev_priv->mm.stolen_lock);
 	if (ret) {
 		DRM_DEBUG_KMS("failed to allocate stolen space\n");
@@ -675,7 +808,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
 	obj = _i915_gem_object_create_stolen(dev, stolen);
 	if (IS_ERR(obj)) {
 		DRM_DEBUG_KMS("failed to allocate stolen object\n");
-		i915_gem_stolen_remove_node(dev_priv, stolen);
+		i915_gem_stolen_remove_node(dev_priv, &stolen->base);
 		kfree(stolen);
 		return obj;
 	}
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 2863f01..20739f3 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5289,7 +5289,7 @@ static void valleyview_check_pctx(struct drm_i915_private *dev_priv)
 	unsigned long pctx_addr = I915_READ(VLV_PCBR) & ~4095;
 
 	WARN_ON(pctx_addr != dev_priv->mm.stolen_base +
-			     dev_priv->vlv_pctx->stolen->start);
+			     dev_priv->vlv_pctx->stolen->base.start);
 }
 
 
@@ -5364,7 +5364,7 @@ static void valleyview_setup_pctx(struct drm_device *dev)
 		goto out;
 	}
 
-	pctx_paddr = dev_priv->mm.stolen_base + pctx->stolen->start;
+	pctx_paddr = dev_priv->mm.stolen_base + pctx->stolen->base.start;
 	I915_WRITE(VLV_PCBR, pctx_paddr);
 
 out:
-- 
1.9.1

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

[PATCH 08/12] drm/i915: Support for pread/pwrite from/to non shmem backed objects

[ankitprasad.r.sharma]

From: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>

This patch adds support for extending the pread/pwrite functionality
for objects not backed by shmem. The access will be made through
gtt interface. This will cover objects backed by stolen memory as well
as other non-shmem backed objects.

v2: Drop locks around slow_user_access, prefault the pages before
access (Chris)

v3: Rebased to the latest drm-intel-nightly (Ankit)

v4: Moved page base & offset calculations outside the copy loop,
corrected data types for size and offset variables, corrected if-else
braces format (Tvrtko/kerneldocs)

v5: Enabled pread/pwrite for all non-shmem backed objects including
without tiling restrictions (Ankit)

v6: Using pwrite_fast for non-shmem backed objects as well (Chris)

v7: Updated commit message, Renamed i915_gem_gtt_read to i915_gem_gtt_copy,
added pwrite slow path for non-shmem backed objects (Chris/Tvrtko)

v8: Updated v7 commit message, mutex unlock around pwrite slow path for
non-shmem backed objects (Tvrtko)

v9: Corrected check during pread_ioctl, to avoid shmem_pread being
called for non-shmem backed objects (Tvrtko)

v10: Moved the write_domain check to needs_clflush and tiling mode check
to pwrite_fast (Chris)

v11: Use pwrite_fast fallback for all objects (shmem and non-shmem backed),
call fast_user_write regardless of pagefault in previous iteration

v12: Use page-by-page copy for slow user access too (Chris)

v13: Handled EFAULT, Avoid use of WARN_ON, put_fence only if whole obj
pinned (Chris)

v14: Corrected datatypes/initializations (Tvrtko)

Testcase: igt/gem_stolen, igt/gem_pread, igt/gem_pwrite

Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 222 ++++++++++++++++++++++++++++++++++------
 1 file changed, 190 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c5ba1bb..3e72737 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -54,6 +54,9 @@ static bool cpu_cache_is_coherent(struct drm_device *dev,
 
 static bool cpu_write_needs_clflush(struct drm_i915_gem_object *obj)
 {
+	if (obj->base.write_domain == I915_GEM_DOMAIN_CPU)
+		return false;
+
 	if (!cpu_cache_is_coherent(obj->base.dev, obj->cache_level))
 		return true;
 
@@ -641,6 +644,142 @@ shmem_pread_slow(struct page *page, int shmem_page_offset, int page_length,
 	return ret ? - EFAULT : 0;
 }
 
+static inline unsigned long
+slow_user_access(struct io_mapping *mapping,
+		 uint64_t page_base, int page_offset,
+		 char __user *user_data,
+		 unsigned long length, bool pwrite)
+{
+	void __iomem *ioaddr;
+	void *vaddr;
+	uint64_t unwritten;
+
+	ioaddr = io_mapping_map_wc(mapping, page_base);
+	/* We can use the cpu mem copy function because this is X86. */
+	vaddr = (void __force *)ioaddr + page_offset;
+	if (pwrite)
+		unwritten = __copy_from_user(vaddr, user_data, length);
+	else
+		unwritten = __copy_to_user(user_data, vaddr, length);
+
+	io_mapping_unmap(ioaddr);
+	return unwritten;
+}
+
+static int
+i915_gem_gtt_pread(struct drm_device *dev,
+		   struct drm_i915_gem_object *obj, uint64_t size,
+		   uint64_t data_offset, uint64_t data_ptr)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_ggtt *ggtt = &dev_priv->ggtt;
+	struct drm_mm_node node;
+	char __user *user_data;
+	uint64_t remain;
+	uint64_t offset;
+	int ret;
+
+	ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
+	if (ret) {
+		ret = insert_mappable_node(dev_priv, &node, PAGE_SIZE);
+		if (ret)
+			goto out;
+
+		ret = i915_gem_object_get_pages(obj);
+		if (ret) {
+			remove_mappable_node(&node);
+			goto out;
+		}
+
+		i915_gem_object_pin_pages(obj);
+	} else {
+		node.start = i915_gem_obj_ggtt_offset(obj);
+		node.allocated = false;
+		ret = i915_gem_object_put_fence(obj);
+		if (ret)
+			goto out_unpin;
+	}
+
+	ret = i915_gem_object_set_to_gtt_domain(obj, false);
+	if (ret)
+		goto out_unpin;
+
+	user_data = to_user_ptr(data_ptr);
+	remain = size;
+	offset = data_offset;
+
+	mutex_unlock(&dev->struct_mutex);
+	if (likely(!i915.prefault_disable)) {
+		ret = fault_in_multipages_writeable(user_data, remain);
+		if (ret) {
+			mutex_lock(&dev->struct_mutex);
+			goto out_unpin;
+		}
+	}
+
+	while (remain > 0) {
+		/* Operation in this page
+		 *
+		 * page_base = page offset within aperture
+		 * page_offset = offset within page
+		 * page_length = bytes to copy for this page
+		 */
+		u32 page_base = node.start;
+		unsigned page_offset = offset_in_page(offset);
+		unsigned page_length = PAGE_SIZE - page_offset;
+		page_length = remain < page_length ? remain : page_length;
+		if (node.allocated) {
+			wmb();
+			ggtt->base.insert_page(&ggtt->base,
+					       i915_gem_object_get_dma_address(obj, offset >> PAGE_SHIFT),
+					       node.start,
+					       I915_CACHE_NONE, 0);
+			wmb();
+		} else {
+			page_base += offset & PAGE_MASK;
+		}
+		/* This is a slow read/write as it tries to read from
+		 * and write to user memory which may result into page
+		 * faults, and so we cannot perform this under struct_mutex.
+		 */
+		if (slow_user_access(ggtt->mappable, page_base,
+				     page_offset, user_data,
+				     page_length, false)) {
+			ret = -EFAULT;
+			break;
+		}
+
+		remain -= page_length;
+		user_data += page_length;
+		offset += page_length;
+	}
+
+	mutex_lock(&dev->struct_mutex);
+	if (ret == 0 && (obj->base.read_domains & I915_GEM_DOMAIN_GTT) == 0) {
+		/* The user has modified the object whilst we tried
+		 * reading from it, and we now have no idea what domain
+		 * the pages should be in. As we have just been touching
+		 * them directly, flush everything back to the GTT
+		 * domain.
+		 */
+		ret = i915_gem_object_set_to_gtt_domain(obj, false);
+	}
+
+out_unpin:
+	if (node.allocated) {
+		wmb();
+		ggtt->base.clear_range(&ggtt->base,
+				       node.start, node.size,
+				       true);
+		i915_gem_object_unpin_pages(obj);
+		remove_mappable_node(&node);
+	} else {
+		i915_gem_object_ggtt_unpin(obj);
+	}
+out:
+	return ret;
+}
+
 static int
 i915_gem_shmem_pread(struct drm_device *dev,
 		     struct drm_i915_gem_object *obj,
@@ -656,6 +795,9 @@ i915_gem_shmem_pread(struct drm_device *dev,
 	int needs_clflush = 0;
 	struct sg_page_iter sg_iter;
 
+	if (!obj->base.filp)
+		return -ENODEV;
+
 	user_data = to_user_ptr(args->data_ptr);
 	remain = args->size;
 
@@ -764,18 +906,15 @@ i915_gem_pread_ioctl(struct drm_device *dev, void *data,
 		goto out;
 	}
 
-	/* prime objects have no backing filp to GEM pread/pwrite
-	 * pages from.
-	 */
-	if (!obj->base.filp) {
-		ret = -EINVAL;
-		goto out;
-	}
-
 	trace_i915_gem_object_pread(obj, args->offset, args->size);
 
 	ret = i915_gem_shmem_pread(dev, obj, args, file);
 
+	/* pread for non shmem backed objects */
+	if (ret == -EFAULT || ret == -ENODEV)
+		ret = i915_gem_gtt_pread(dev, obj, args->size,
+					args->offset, args->data_ptr);
+
 out:
 	drm_gem_object_unreference(&obj->base);
 unlock:
@@ -817,10 +956,15 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_private *i915,
 			 struct drm_file *file)
 {
 	struct i915_ggtt *ggtt = &i915->ggtt;
+	struct drm_device *dev = obj->base.dev;
 	struct drm_mm_node node;
 	uint64_t remain, offset;
 	char __user *user_data;
 	int ret;
+	bool hit_slow_path = false;
+
+	if (obj->tiling_mode != I915_TILING_NONE)
+		return -EFAULT;
 
 	ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE | PIN_NONBLOCK);
 	if (ret) {
@@ -838,16 +982,15 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_private *i915,
 	} else {
 		node.start = i915_gem_obj_ggtt_offset(obj);
 		node.allocated = false;
+		ret = i915_gem_object_put_fence(obj);
+		if (ret)
+			goto out_unpin;
 	}
 
 	ret = i915_gem_object_set_to_gtt_domain(obj, true);
 	if (ret)
 		goto out_unpin;
 
-	ret = i915_gem_object_put_fence(obj);
-	if (ret)
-		goto out_unpin;
-
 	intel_fb_obj_invalidate(obj, ORIGIN_GTT);
 	obj->dirty = true;
 
@@ -866,22 +1009,34 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_private *i915,
 		unsigned page_length = PAGE_SIZE - page_offset;
 		page_length = remain < page_length ? remain : page_length;
 		if (node.allocated) {
-			wmb();
+			wmb(); /* flush the write before we modify the GGTT */
 			ggtt->base.insert_page(&ggtt->base,
 					       i915_gem_object_get_dma_address(obj, offset >> PAGE_SHIFT),
 					       node.start, I915_CACHE_NONE, 0);
-			wmb();
+			wmb(); /* flush modifications to the GGTT (insert_page) */
 		} else {
 			page_base += offset & PAGE_MASK;
 		}
 		/* If we get a fault while copying data, then (presumably) our
 		 * source page isn't available.  Return the error and we'll
 		 * retry in the slow path.
+		 * If the object is non-shmem backed, we retry again with the
+		 * path that handles page fault.
 		 */
 		if (fast_user_write(ggtt->mappable, page_base,
 				    page_offset, user_data, page_length)) {
-			ret = -EFAULT;
-			goto out_flush;
+			hit_slow_path = true;
+			mutex_unlock(&dev->struct_mutex);
+			if (slow_user_access(ggtt->mappable,
+					     page_base,
+					     page_offset, user_data,
+					     page_length, true)) {
+				ret = -EFAULT;
+				mutex_lock(&dev->struct_mutex);
+				goto out_flush;
+			}
+
+			mutex_lock(&dev->struct_mutex);
 		}
 
 		remain -= page_length;
@@ -890,6 +1045,19 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_private *i915,
 	}
 
 out_flush:
+	if (hit_slow_path) {
+		if (ret == 0 &&
+		    (obj->base.read_domains & I915_GEM_DOMAIN_GTT) == 0) {
+			/* The user has modified the object whilst we tried
+			 * reading from it, and we now have no idea what domain
+			 * the pages should be in. As we have just been touching
+			 * them directly, flush everything back to the GTT
+			 * domain.
+			 */
+			ret = i915_gem_object_set_to_gtt_domain(obj, false);
+		}
+	}
+
 	intel_fb_obj_flush(obj, false, ORIGIN_GTT);
 out_unpin:
 	if (node.allocated) {
@@ -1146,14 +1314,6 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
 		goto out;
 	}
 
-	/* prime objects have no backing filp to GEM pread/pwrite
-	 * pages from.
-	 */
-	if (!obj->base.filp) {
-		ret = -EINVAL;
-		goto out;
-	}
-
 	trace_i915_gem_object_pwrite(obj, args->offset, args->size);
 
 	ret = -EFAULT;
@@ -1163,20 +1323,20 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
 	 * pread/pwrite currently are reading and writing from the CPU
 	 * perspective, requiring manual detiling by the client.
 	 */
-	if (obj->tiling_mode == I915_TILING_NONE &&
-	    obj->base.write_domain != I915_GEM_DOMAIN_CPU &&
-	    cpu_write_needs_clflush(obj)) {
+	if (!obj->base.filp || cpu_write_needs_clflush(obj)) {
 		ret = i915_gem_gtt_pwrite_fast(dev_priv, obj, args, file);
 		/* Note that the gtt paths might fail with non-page-backed user
 		 * pointers (e.g. gtt mappings when moving data between
 		 * textures). Fallback to the shmem path in that case. */
 	}
 
-	if (ret == -EFAULT || ret == -ENOSPC) {
+	if (ret == -EFAULT) {
 		if (obj->phys_handle)
 			ret = i915_gem_phys_pwrite(obj, args, file);
-		else
+		else if (obj->base.filp)
 			ret = i915_gem_shmem_pwrite(dev, obj, args, file);
+		else
+			ret = -ENODEV;
 	}
 
 out:
@@ -4010,9 +4170,7 @@ out:
 	 * object is now coherent at its new cache level (with respect
 	 * to the access domain).
 	 */
-	if (obj->cache_dirty &&
-	    obj->base.write_domain != I915_GEM_DOMAIN_CPU &&
-	    cpu_write_needs_clflush(obj)) {
+	if (obj->cache_dirty && cpu_write_needs_clflush(obj)) {
 		if (i915_gem_clflush_object(obj, true))
 			i915_gem_chipset_flush(obj->base.dev);
 	}
-- 
1.9.1

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

[PATCH 09/12] drm/i915: Migrate stolen objects before hibernation

[ankitprasad.r.sharma]

From: Chris Wilson <chris@chris-wilson.co.uk>

Ville reminded us that stolen memory is not preserved across
hibernation, and a result of this was that context objects now being
allocated from stolen were being corrupted on S4 and promptly hanging
the GPU on resume.

We want to utilise stolen for as much as possible (nothing else will use
that wasted memory otherwise), so we need a strategy for handling
general objects allocated from stolen and hibernation. A simple solution
is to do a CPU copy through the GTT of the stolen object into a fresh
shmemfs backing store and thenceforth treat it as a normal objects. This
can be refined in future to either use a GPU copy to avoid the slow
uncached reads (though it's hibernation!) and recreate stolen objects
upon resume/first-use. For now, a simple approach should suffice for
testing the object migration.

v2:
Swap PTE for pinned bindings over to the shmemfs. This adds a
complicated dance, but is required as many stolen objects are likely to
be pinned for use by the hardware. Swapping the PTEs should not result
in externally visible behaviour, as each PTE update should be atomic and
the two pages identical. (danvet)

safe-by-default, or the principle of least surprise. We need a new flag
to mark objects that we can wilfully discard and recreate across
hibernation. (danvet)

Just use the global_list rather than invent a new stolen_list. This is
the slowpath hibernate and so adding a new list and the associated
complexity isn't worth it.

v3: Rebased on drm-intel-nightly (Ankit)

v4: Use insert_page to map stolen memory backed pages for migration to
shmem (Chris)

v5: Acquire mutex lock while copying stolen buffer objects to shmem (Chris)

v6: Handled file leak, Splitted object migration function, added kerneldoc
for migrate_stolen_to_shmemfs() function (Tvrtko)
Use i915 wrapper function for drm_mm_insert_node_in_range()

v7: Keep the object in cpu domain after get_pages, remove the object from
the unbound list only when marked PURGED, Corrected split of object migration
function (Chris)

v8: Split i915_gem_freeze(), removed redundant use of barrier, corrected
use of set_to_cpu_domain() (Chris)

v9: Replaced WARN_ON by BUG_ON and added a comment explaining it
(Daniel/Tvrtko)

v10: Document use of barriers (Chris)

v11: Resolved list corruption due to not removing obj from global_list
if no reference to pages is held, Rebased (Ankit)

v12: Rebase (Ankit)

Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c         |  17 ++-
 drivers/gpu/drm/i915/i915_drv.h         |  10 ++
 drivers/gpu/drm/i915/i915_gem.c         | 202 ++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/i915_gem_stolen.c  |  49 ++++++++
 drivers/gpu/drm/i915/intel_display.c    |   3 +
 drivers/gpu/drm/i915/intel_fbdev.c      |   6 +
 drivers/gpu/drm/i915/intel_pm.c         |   2 +
 drivers/gpu/drm/i915/intel_ringbuffer.c |   6 +
 8 files changed, 283 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 18eb3e6..9e411f1 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1020,6 +1020,21 @@ static int i915_pm_suspend(struct device *dev)
 	return i915_drm_suspend(drm_dev);
 }
 
+static int i915_pm_freeze(struct device *dev)
+{
+	int ret;
+
+	ret = i915_gem_freeze(pci_get_drvdata(to_pci_dev(dev)));
+	if (ret)
+		return ret;
+
+	ret = i915_pm_suspend(dev);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static int i915_pm_suspend_late(struct device *dev)
 {
 	struct drm_device *drm_dev = dev_to_i915(dev)->dev;
@@ -1667,7 +1682,7 @@ static const struct dev_pm_ops i915_pm_ops = {
 	 * @restore, @restore_early : called after rebooting and restoring the
 	 *                            hibernation image [PMSG_RESTORE]
 	 */
-	.freeze = i915_pm_suspend,
+	.freeze = i915_pm_freeze,
 	.freeze_late = i915_pm_suspend_late,
 	.thaw_early = i915_pm_resume_early,
 	.thaw = i915_pm_resume,
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d998f6c..c7fe863 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2152,6 +2152,12 @@ struct drm_i915_gem_object {
 	 * Advice: are the backing pages purgeable?
 	 */
 	unsigned int madv:2;
+	/**
+	 * Whereas madv is for userspace, there are certain situations
+	 * where we want I915_MADV_DONTNEED behaviour on internal objects
+	 * without conflating the userspace setting.
+	 */
+	unsigned int internal_volatile:1;
 
 	/**
 	 * Current tiling mode for the object.
@@ -3181,6 +3187,9 @@ int i915_gem_l3_remap(struct drm_i915_gem_request *req, int slice);
 void i915_gem_init_swizzling(struct drm_device *dev);
 void i915_gem_cleanup_engines(struct drm_device *dev);
 int __must_check i915_gpu_idle(struct drm_device *dev);
+int __must_check i915_gem_freeze(struct drm_device *dev);
+int __must_check
+i915_gem_object_migrate_stolen_to_shmemfs(struct drm_i915_gem_object *obj);
 int __must_check i915_gem_suspend(struct drm_device *dev);
 void __i915_add_request(struct drm_i915_gem_request *req,
 			struct drm_i915_gem_object *batch_obj,
@@ -3404,6 +3413,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
 					       u32 stolen_offset,
 					       u32 gtt_offset,
 					       u32 size);
+int __must_check i915_gem_stolen_freeze(struct drm_i915_private *i915);
 
 /* i915_gem_shrinker.c */
 unsigned long i915_gem_shrink(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 3e72737..48959cf 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4757,12 +4757,27 @@ static const struct drm_i915_gem_object_ops i915_gem_object_ops = {
 	.put_pages = i915_gem_object_put_pages_gtt,
 };
 
+static struct address_space *
+i915_gem_set_inode_gfp(struct drm_device *dev, struct file *file)
+{
+	struct address_space *mapping = file_inode(file)->i_mapping;
+	gfp_t mask;
+
+	mask = GFP_HIGHUSER | __GFP_RECLAIMABLE;
+	if (IS_CRESTLINE(dev) || IS_BROADWATER(dev)) {
+		/* 965gm cannot relocate objects above 4GiB. */
+		mask &= ~__GFP_HIGHMEM;
+		mask |= __GFP_DMA32;
+	}
+	mapping_set_gfp_mask(mapping, mask);
+
+	return mapping;
+}
+
 struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
 						  size_t size)
 {
 	struct drm_i915_gem_object *obj;
-	struct address_space *mapping;
-	gfp_t mask;
 	int ret;
 
 	obj = i915_gem_object_alloc(dev);
@@ -4775,15 +4790,7 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
 		return ERR_PTR(ret);
 	}
 
-	mask = GFP_HIGHUSER | __GFP_RECLAIMABLE;
-	if (IS_CRESTLINE(dev) || IS_BROADWATER(dev)) {
-		/* 965gm cannot relocate objects above 4GiB. */
-		mask &= ~__GFP_HIGHMEM;
-		mask |= __GFP_DMA32;
-	}
-
-	mapping = file_inode(obj->base.filp)->i_mapping;
-	mapping_set_gfp_mask(mapping, mask);
+	i915_gem_set_inode_gfp(dev, obj->base.filp);
 
 	i915_gem_object_init(obj, &i915_gem_object_ops);
 
@@ -4955,6 +4962,179 @@ i915_gem_stop_engines(struct drm_device *dev)
 		dev_priv->gt.stop_engine(engine);
 }
 
+static int
+copy_content(struct drm_i915_gem_object *obj,
+		struct drm_i915_private *i915,
+		struct address_space *mapping)
+{
+	struct i915_ggtt *ggtt = &i915->ggtt;
+	struct drm_mm_node node;
+	int ret, i;
+
+	ret = i915_gem_object_set_to_gtt_domain(obj, false);
+	if (ret)
+		return ret;
+
+	/* stolen objects are already pinned to prevent shrinkage */
+	memset(&node, 0, sizeof(node));
+	ret = insert_mappable_node(i915, &node, PAGE_SIZE);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < obj->base.size / PAGE_SIZE; i++) {
+		struct page *page;
+		void *__iomem src;
+		void *dst;
+
+		ggtt->base.insert_page(&ggtt->base,
+				       i915_gem_object_get_dma_address(obj, i),
+				       node.start,
+				       I915_CACHE_NONE, 0);
+
+		page = shmem_read_mapping_page(mapping, i);
+		if (IS_ERR(page)) {
+			ret = PTR_ERR(page);
+			break;
+		}
+
+		src = io_mapping_map_atomic_wc(ggtt->mappable, node.start);
+		dst = kmap_atomic(page);
+		wmb(); /* flush modifications to the GGTT (insert_page) */
+		memcpy_fromio(dst, src, PAGE_SIZE);
+		wmb(); 	/* flush the write before we modify the GGTT */
+		kunmap_atomic(dst);
+		io_mapping_unmap_atomic(src);
+
+		put_page(page);
+	}
+
+	ggtt->base.clear_range(&ggtt->base,
+			       node.start, node.size,
+			       true);
+	remove_mappable_node(&node);
+	if (ret)
+		return ret;
+
+	return i915_gem_object_set_to_cpu_domain(obj, true);
+}
+
+/**
+ * i915_gem_object_migrate_stolen_to_shmemfs() - migrates a stolen backed
+ * object to shmemfs
+ * @obj: stolen backed object to be migrated
+ *
+ * Returns: 0 on successful migration, errno on failure
+ */
+
+int
+i915_gem_object_migrate_stolen_to_shmemfs(struct drm_i915_gem_object *obj)
+{
+	struct drm_i915_private *i915 = to_i915(obj->base.dev);
+	struct i915_vma *vma, *vn;
+	struct file *file;
+	struct address_space *mapping;
+	struct sg_table *stolen_pages, *shmemfs_pages;
+	int ret;
+
+	if (WARN_ON_ONCE(i915_gem_object_needs_bit17_swizzle(obj)))
+		return -EINVAL;
+
+	file = shmem_file_setup("drm mm object", obj->base.size, VM_NORESERVE);
+	if (IS_ERR(file))
+		return PTR_ERR(file);
+	mapping = i915_gem_set_inode_gfp(obj->base.dev, file);
+
+	list_for_each_entry_safe(vma, vn, &obj->vma_list, obj_link)
+		if (i915_vma_unbind(vma))
+			continue;
+
+	if (obj->madv != I915_MADV_WILLNEED && list_empty(&obj->vma_list)) {
+		/* Discard the stolen reservation, and replace with
+		 * an unpopulated shmemfs object.
+		 */
+		obj->madv = __I915_MADV_PURGED;
+	} else {
+		ret = copy_content(obj, i915, mapping);
+		if (ret)
+			goto err_file;
+	}
+
+	stolen_pages = obj->pages;
+	obj->pages = NULL;
+
+	obj->base.filp = file;
+
+	/* Recreate any pinned binding with pointers to the new storage */
+	if (!list_empty(&obj->vma_list)) {
+		ret = i915_gem_object_get_pages_gtt(obj);
+		if (ret) {
+			obj->pages = stolen_pages;
+			goto err_file;
+		}
+
+		obj->get_page.sg = obj->pages->sgl;
+		obj->get_page.last = 0;
+
+		list_for_each_entry(vma, &obj->vma_list, obj_link) {
+			if (!drm_mm_node_allocated(&vma->node))
+				continue;
+
+			/* As vma is already allocated and only the PTEs
+			 * have to be reprogrammed, makes this vma_bind
+			 * call extremely unlikely to fail.
+			 */
+			BUG_ON(i915_vma_bind(vma,
+					      obj->cache_level,
+					      PIN_UPDATE));
+		}
+	} else {
+		/* Remove object from global list if no reference to the
+		 * pages is held.
+		 */
+		list_del(&obj->global_list);
+	}
+
+	/* drop the stolen pin and backing */
+	shmemfs_pages = obj->pages;
+	obj->pages = stolen_pages;
+
+	i915_gem_object_unpin_pages(obj);
+	obj->ops->put_pages(obj);
+	if (obj->ops->release)
+		obj->ops->release(obj);
+
+	obj->ops = &i915_gem_object_ops;
+	obj->pages = shmemfs_pages;
+
+	return 0;
+
+err_file:
+	fput(file);
+	obj->base.filp = NULL;
+	return ret;
+}
+
+int
+i915_gem_freeze(struct drm_device *dev)
+{
+	/* Called before i915_gem_suspend() when hibernating */
+	struct drm_i915_private *i915 = to_i915(dev);
+	int ret;
+
+	ret = i915_mutex_lock_interruptible(dev);
+	if (ret)
+		return ret;
+
+	/* Across hibernation, the stolen area is not preserved.
+	 * Anything inside stolen must copied back to normal
+	 * memory if we wish to preserve it.
+	 */
+	ret = i915_gem_stolen_freeze(i915);
+
+	mutex_unlock(&dev->struct_mutex);
+	return ret;
+}
+
 int
 i915_gem_suspend(struct drm_device *dev)
 {
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index b8573a6..d9756ee 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -851,3 +851,52 @@ err:
 	drm_gem_object_unreference(&obj->base);
 	return ERR_PTR(ret);
 }
+
+int i915_gem_stolen_freeze(struct drm_i915_private *i915)
+{
+	struct drm_i915_gem_object *obj, *tmp;
+	struct list_head *phase[] = {
+		&i915->mm.unbound_list, &i915->mm.bound_list, NULL
+	}, **p;
+	int ret = 0;
+
+	for (p = phase; *p; p++) {
+		struct list_head migrate;
+		int ret;
+
+		INIT_LIST_HEAD(&migrate);
+		list_for_each_entry_safe(obj, tmp, *p, global_list) {
+			if (obj->stolen == NULL)
+				continue;
+
+			if (obj->internal_volatile)
+				continue;
+
+			/* In the general case, this object may only be alive
+			 * due to an active reference, and that may disappear
+			 * when we unbind any of the objects (and so wait upon
+			 * the GPU and retire requests). To prevent one of the
+			 * objects from disappearing beneath us, we need to
+			 * take a reference to each as we build the migration
+			 * list.
+			 *
+			 * This is similar to the strategy required whilst
+			 * shrinking or evicting objects (for the same reason).
+			 */
+			drm_gem_object_reference(&obj->base);
+			list_move(&obj->global_list, &migrate);
+		}
+
+		ret = 0;
+		list_for_each_entry_safe(obj, tmp, &migrate, global_list) {
+			if (ret == 0)
+				ret = i915_gem_object_migrate_stolen_to_shmemfs(obj);
+			drm_gem_object_unreference(&obj->base);
+		}
+		list_splice(&migrate, *p);
+		if (ret)
+			break;
+	}
+
+	return ret;
+}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 66072bf..95b8d44 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2479,6 +2479,9 @@ intel_alloc_initial_plane_obj(struct intel_crtc *crtc,
 		return false;
 	}
 
+	/* Not to be preserved across hibernation */
+	obj->internal_volatile = true;
+
 	obj->tiling_mode = plane_config->tiling;
 	if (obj->tiling_mode == I915_TILING_X)
 		obj->stride = fb->pitches[0];
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 1686687..091fb6e 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -157,6 +157,12 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 		goto out;
 	}
 
+	/* Discard the contents of the BIOS fb across hibernation.
+	 * We really want to completely throwaway the earlier fbdev
+	 * and reconfigure it anyway.
+	 */
+	obj->internal_volatile = true;
+
 	fb = __intel_framebuffer_create(dev, &mode_cmd, obj);
 	if (IS_ERR(fb)) {
 		drm_gem_object_unreference(&obj->base);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 20739f3..5faeace 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5368,6 +5368,8 @@ static void valleyview_setup_pctx(struct drm_device *dev)
 	I915_WRITE(VLV_PCBR, pctx_paddr);
 
 out:
+	/* The power context need not be preserved across hibernation */
+	pctx->internal_volatile = true;
 	DRM_DEBUG_DRIVER("PCBR: 0x%08x\n", I915_READ(VLV_PCBR));
 	dev_priv->vlv_pctx = pctx;
 	mutex_unlock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index fc5f8ae..e38716f 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2164,6 +2164,12 @@ static int intel_alloc_ringbuffer_obj(struct drm_device *dev,
 	if (IS_ERR(obj))
 		return PTR_ERR(obj);
 
+	/* Ringbuffer objects are by definition volatile - only the commands
+	 * between HEAD and TAIL need to be preserved and whilst there are
+	 * any commands there, the ringbuffer is pinned by activity.
+	 */
+	obj->internal_volatile = true;
+
 	/* mark ring buffers as read-only from GPU side by default */
 	obj->gt_ro = 1;
 
-- 
1.9.1

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

[PATCH 10/12] drm/i915: Disable use of stolen area by User when Intel RST is present

[ankitprasad.r.sharma]

From: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>

The BIOS RapidStartTechnology may corrupt the stolen memory across S3
suspend due to unalarmed hibernation, in which case we will not be able
to preserve the User data stored in the stolen region. Hence this patch
tries to identify presence of the RST device on the ACPI bus, and
disables use of stolen memory (for persistent data) if found.

v2: Updated comment, updated/corrected new functions private to driver
(Chris/Tvrtko)

v3: Disabling stolen by default, wait till required acpi changes to
detect device presence are pulled in (Ankit)

v4: Enabled stolen by default as required acpi changes are merged
(Ankit)

v5: renamed variable, is IS_ENABLED() in place of #ifdef, use char*
instead of structures (Lukas)

Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
Cc: Lukas Wunner <lukas@wunner.de>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h        | 11 +++++++++++
 drivers/gpu/drm/i915/i915_gem.c        |  8 ++++++++
 drivers/gpu/drm/i915/i915_gem_stolen.c | 12 ++++++++++++
 drivers/gpu/drm/i915/intel_acpi.c      |  7 +++++++
 4 files changed, 38 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c7fe863..37f9de8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1326,6 +1326,16 @@ struct i915_gem_mm {
 	 */
 	bool busy;
 
+	/**
+	 * Stolen will be lost upon hibernate (as the memory is unpowered).
+	 * Across resume, we expect stolen to be intact - however, it may
+	 * also be utililised by third parties (e.g. Intel RapidStart
+	 * Technology) and if so we have to assume that any data stored in
+	 * stolen across resume is lost and we set this flag to indicate that
+	 * the stolen memory is volatile.
+	 */
+	bool volatile_stolen;
+
 	/* the indicator for dispatch video commands on two BSD rings */
 	unsigned int bsd_ring_dispatch_index;
 
@@ -3559,6 +3569,7 @@ static inline int intel_opregion_get_panel_type(struct drm_device *dev)
 #endif
 
 /* intel_acpi.c */
+bool intel_detect_acpi_rst(void);
 #ifdef CONFIG_ACPI
 extern void intel_register_dsm_handler(void);
 extern void intel_unregister_dsm_handler(void);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 48959cf..45fd049 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -391,8 +391,16 @@ static struct drm_i915_gem_object *
 i915_gem_alloc_object_stolen(struct drm_device *dev, size_t size)
 {
 	struct drm_i915_gem_object *obj;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
 
+	if (dev_priv->mm.volatile_stolen) {
+		/* Stolen may be overwritten by external parties
+		 * so unsuitable for persistent user data.
+		 */
+		return ERR_PTR(-ENODEV);
+	}
+
 	mutex_lock(&dev->struct_mutex);
 	obj = i915_gem_object_create_stolen(dev, size);
 	if (IS_ERR(obj))
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index d9756ee..ef28af6 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -490,6 +490,18 @@ int i915_gem_init_stolen(struct drm_device *dev)
 	 */
 	drm_mm_init(&dev_priv->mm.stolen, 0, ggtt->stolen_usable_size);
 
+	/* If the stolen region can be modified behind our backs upon suspend,
+	 * then we cannot use it to store nonvolatile contents (i.e user data)
+	 * as it will be corrupted upon resume.
+	 */
+	dev_priv->mm.volatile_stolen = false;
+	if (IS_ENABLED(CONFIG_SUSPEND)) {
+		/* BIOSes using RapidStart Technology have been reported
+		 * to overwrite stolen across S3, not just S4.
+		 */
+		dev_priv->mm.volatile_stolen = intel_detect_acpi_rst();
+	}
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_acpi.c b/drivers/gpu/drm/i915/intel_acpi.c
index eb638a1..05fd67f 100644
--- a/drivers/gpu/drm/i915/intel_acpi.c
+++ b/drivers/gpu/drm/i915/intel_acpi.c
@@ -23,6 +23,8 @@ static const u8 intel_dsm_guid[] = {
 	0x0f, 0x13, 0x17, 0xb0, 0x1c, 0x2c
 };
 
+static const char *irst_id = "INT3392";
+
 static char *intel_dsm_port_name(u8 id)
 {
 	switch (id) {
@@ -162,3 +164,8 @@ void intel_register_dsm_handler(void)
 void intel_unregister_dsm_handler(void)
 {
 }
+
+bool intel_detect_acpi_rst(void)
+{
+	return acpi_dev_present(irst_id);
+}
-- 
1.9.1

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

[PATCH 11/12] drm/i915: Extend GET_APERTURE ioctl to report available map space

[ankitprasad.r.sharma]

From: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>

When constructing a batchbuffer, it is sometimes crucial to know the
largest hole into which we can fit a fenceable buffer (for example when
handling very large objects on gen2 and gen3). This depends on the
fragmentation of pinned buffers inside the aperture, a question only the
kernel can easily answer.

This patch extends the current DRM_I915_GEM_GET_APERTURE ioctl to
include a couple of new fields in its reply to userspace - the total
amount of space available in the mappable region of the aperture and
also the single largest block available.

This is not quite what userspace wants to answer the question of whether
this batch will fit as fences are also required to meet severe alignment
constraints within the batch. For this purpose, a third conservative
estimate of largest fence available is also provided. For when userspace
needs more than one batch, we also provide the culmulative space
available for fences such that it has some additional guidance to how
much space it could allocate to fences. Conservatism still wins.

The patch also adds a debugfs file for convenient testing and reporting.

v2: The first object cannot end at offset 0, so we can use last==0 to
detect the empty list.

v3: Expand all values to 64bit, just in case.
    Report total mappable aperture size for userspace that cannot easily
    determine it by inspecting the PCI device.

v4: (Rodrigo) Fixed rebase conflicts.

v5: Keeping limits to get_aperture ioctl, and moved changing numbers to
debugfs, Addressed comments (Chris/Tvrtko)

Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 133 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem.c     |   1 +
 include/uapi/drm/i915_drm.h         |   5 ++
 3 files changed, 139 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 7f94c6a..d8d7994 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -524,6 +524,138 @@ static int i915_gem_object_info(struct seq_file *m, void* data)
 	return 0;
 }
 
+static int vma_rank_by_ggtt(void *priv,
+			    struct list_head *A,
+			    struct list_head *B)
+{
+	struct i915_vma *a = list_entry(A, typeof(*a), exec_list);
+	struct i915_vma *b = list_entry(B, typeof(*b), exec_list);
+
+	return a->node.start - b->node.start;
+}
+
+static u32 __fence_size(struct drm_i915_private *dev_priv, u32 start, u32 end)
+{
+	u32 size = end - start;
+	u32 fence_size;
+
+	if (INTEL_INFO(dev_priv)->gen < 4) {
+		u32 fence_max;
+		u32 fence_next;
+
+		if (IS_GEN3(dev_priv)) {
+			fence_max = I830_FENCE_MAX_SIZE_VAL << 20;
+			fence_next = 1024*1024;
+		} else {
+			fence_max = I830_FENCE_MAX_SIZE_VAL << 19;
+			fence_next = 512*1024;
+		}
+
+		fence_max = min(fence_max, size);
+		fence_size = 0;
+		/* Find fence_size less than fence_max and power of 2 */
+		while (fence_next <= fence_max) {
+			u32 base = ALIGN(start, fence_next);
+			if (base + fence_next > end)
+				break;
+
+			fence_size = fence_next;
+			fence_next <<= 1;
+		}
+	} else {
+		fence_size = size;
+	}
+
+	return fence_size;
+}
+
+static int i915_gem_aperture_info(struct seq_file *m, void *data)
+{
+	struct drm_info_node *node = m->private;
+	struct drm_device *dev = node->minor->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_ggtt *ggtt = &dev_priv->ggtt;
+	struct drm_i915_gem_get_aperture arg;
+	struct i915_vma *vma;
+	struct list_head map_list;
+	const uint64_t map_limit = ggtt->mappable_end;
+	uint64_t map_space, map_largest, fence_space, fence_largest;
+	uint64_t last, size;
+	int ret;
+
+	INIT_LIST_HEAD(&map_list);
+
+	map_space = map_largest = 0;
+	fence_space = fence_largest = 0;
+
+	ret = i915_gem_get_aperture_ioctl(node->minor->dev, &arg, NULL);
+	if (ret)
+		return ret;
+
+	mutex_lock(&dev->struct_mutex);
+	list_for_each_entry(vma, &ggtt->base.active_list, vm_link)
+		if (vma->pin_count &&
+			(vma->node.start + vma->node.size) <= map_limit)
+			list_add(&vma->exec_list, &map_list);
+	list_for_each_entry(vma, &ggtt->base.inactive_list, vm_link)
+		if (vma->pin_count &&
+			(vma->node.start + vma->node.size) <= map_limit)
+			list_add(&vma->exec_list, &map_list);
+
+	last = 0;
+	list_sort(NULL, &map_list, vma_rank_by_ggtt);
+	while (!list_empty(&map_list)) {
+		vma = list_first_entry(&map_list, typeof(*vma), exec_list);
+		list_del_init(&vma->exec_list);
+
+		if (last == 0)
+			goto skip_first;
+
+		size = vma->node.start - last;
+		if (size > map_largest)
+			map_largest = size;
+		map_space += size;
+
+		size = __fence_size(dev_priv, last, vma->node.start);
+		if (size > fence_largest)
+			fence_largest = size;
+		fence_space += size;
+
+skip_first:
+		last = vma->node.start + vma->node.size;
+	}
+	if (last < map_limit) {
+		size = map_limit - last;
+		if (size > map_largest)
+			map_largest = size;
+		map_space += size;
+
+		size = __fence_size(dev_priv, last, map_limit);
+		if (size > fence_largest)
+			fence_largest = size;
+		fence_space += size;
+	}
+
+	mutex_unlock(&dev->struct_mutex);
+
+	seq_printf(m, "Total size of the GTT: %llu bytes\n",
+		   arg.aper_size);
+	seq_printf(m, "Available space in the GTT: %llu bytes\n",
+		   arg.aper_available_size);
+	seq_printf(m, "Total space in the mappable aperture: %llu bytes\n",
+		   arg.map_total_size);
+	seq_printf(m, "Available space in the mappable aperture: %llu bytes\n",
+		   map_space);
+	seq_printf(m, "Single largest space in the mappable aperture: %llu bytes\n",
+		   map_largest);
+	seq_printf(m, "Available space for fences: %llu bytes\n",
+		   fence_space);
+	seq_printf(m, "Single largest fence available: %llu bytes\n",
+		   fence_largest);
+
+	return 0;
+}
+
 static int i915_gem_gtt_info(struct seq_file *m, void *data)
 {
 	struct drm_info_node *node = m->private;
@@ -5354,6 +5486,7 @@ static int i915_debugfs_create(struct dentry *root,
 static const struct drm_info_list i915_debugfs_list[] = {
 	{"i915_capabilities", i915_capabilities, 0},
 	{"i915_gem_objects", i915_gem_object_info, 0},
+	{"i915_gem_aperture", i915_gem_aperture_info, 0},
 	{"i915_gem_gtt", i915_gem_gtt_info, 0},
 	{"i915_gem_pinned", i915_gem_gtt_info, 0, (void *) PINNED_LIST},
 	{"i915_gem_active", i915_gem_object_list_info, 0, (void *) ACTIVE_LIST},
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 45fd049..a1be35f 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -165,6 +165,7 @@ i915_gem_get_aperture_ioctl(struct drm_device *dev, void *data,
 
 	args->aper_size = ggtt->base.total;
 	args->aper_available_size = args->aper_size - pinned;
+	args->map_total_size = ggtt->mappable_end;;
 
 	return 0;
 }
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index dae02d8..8f38407 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1007,6 +1007,11 @@ struct drm_i915_gem_get_aperture {
 	 * bytes
 	 */
 	__u64 aper_available_size;
+
+	/**
+	 * Total space in the mappable region of the aperture, in bytes
+	 */
+	__u64 map_total_size;
 };
 
 struct drm_i915_get_pipe_from_crtc_id {
-- 
1.9.1

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

[PATCH 12/12] drm/i915: Extend GET_APERTURE ioctl to report size of the stolen region

[ankitprasad.r.sharma]

From: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>

This patch extends the GET_APERTURE ioctl to add support
for getting total size and available size of the stolen region
as well as single largest block available in the stolen region.
Also adds debugfs support to retieve the size information of the
stolen area.

v2: respinned over Rodrigo's patch which extends the GET_APERTURE
too. Used drm_mm to get the size information of the stolen region
(Chris)
Added debugfs support for testing (Ankit)

v3: Rebased to the latest drm-intel-nightly (Ankit)

v4: Addressed comments, moved non-limits to debugfs.c (Chris/Tvrtko)

Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c    | 12 +++++++++-
 drivers/gpu/drm/i915/i915_drv.h        |  3 +++
 drivers/gpu/drm/i915/i915_gem.c        |  2 ++
 drivers/gpu/drm/i915/i915_gem_stolen.c | 41 ++++++++++++++++++++++++++++++++++
 include/uapi/drm/i915_drm.h            |  6 +++++
 5 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index d8d7994..55d110e 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -580,7 +580,7 @@ static int i915_gem_aperture_info(struct seq_file *m, void *data)
 	struct list_head map_list;
 	const uint64_t map_limit = ggtt->mappable_end;
 	uint64_t map_space, map_largest, fence_space, fence_largest;
-	uint64_t last, size;
+	uint64_t last, size, stolen_free, stolen_largest;
 	int ret;
 
 	INIT_LIST_HEAD(&map_list);
@@ -593,6 +593,10 @@ static int i915_gem_aperture_info(struct seq_file *m, void *data)
 		return ret;
 
 	mutex_lock(&dev->struct_mutex);
+
+	i915_gem_stolen_size_info(dev_priv, &stolen_free,
+				  &stolen_largest);
+
 	list_for_each_entry(vma, &ggtt->base.active_list, vm_link)
 		if (vma->pin_count &&
 			(vma->node.start + vma->node.size) <= map_limit)
@@ -652,6 +656,12 @@ skip_first:
 		   fence_space);
 	seq_printf(m, "Single largest fence available: %llu bytes\n",
 		   fence_largest);
+	seq_printf(m, "Total size of the stolen region: %llu bytes\n",
+		   arg.stolen_total_size);
+	seq_printf(m, "Available size of the stolen region: %llu bytes\n",
+		   stolen_free);
+	seq_printf(m, "Single largest area in the stolen region: %llu bytes\n",
+		   stolen_largest);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 37f9de8..d5c6b23 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3424,6 +3424,9 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
 					       u32 gtt_offset,
 					       u32 size);
 int __must_check i915_gem_stolen_freeze(struct drm_i915_private *i915);
+void i915_gem_stolen_size_info(struct drm_i915_private *dev_priv,
+			       uint64_t *stolen_free,
+			       uint64_t *stolen_largest);
 
 /* i915_gem_shrinker.c */
 unsigned long i915_gem_shrink(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a1be35f..e0f2259 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -166,6 +166,8 @@ i915_gem_get_aperture_ioctl(struct drm_device *dev, void *data,
 	args->aper_size = ggtt->base.total;
 	args->aper_available_size = args->aper_size - pinned;
 	args->map_total_size = ggtt->mappable_end;;
+	args->stolen_total_size =
+		dev_priv->mm.volatile_stolen ? 0 : ggtt->stolen_usable_size;
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index ef28af6..f2621c5 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -912,3 +912,44 @@ int i915_gem_stolen_freeze(struct drm_i915_private *i915)
 
 	return ret;
 }
+
+
+void i915_gem_stolen_size_info(struct drm_i915_private *dev_priv,
+			       uint64_t *stolen_free,
+			       uint64_t *stolen_largest)
+{
+	struct drm_mm *mm = &dev_priv->mm.stolen;
+	struct drm_mm_node *head_node = &mm->head_node;
+	struct drm_mm_node *entry;
+	uint64_t hole_size, hole_start, hole_end, largest_hole = 0;
+	uint64_t total_free = 0;
+
+	if (dev_priv->mm.volatile_stolen) {
+		*stolen_free = 0;
+		*stolen_largest = 0;
+		return;
+	}
+
+	if (head_node->hole_follows) {
+		hole_start = drm_mm_hole_node_start(head_node);
+		hole_end = drm_mm_hole_node_end(head_node);
+		hole_size = hole_end - hole_start;
+		total_free += hole_size;
+		if (largest_hole < hole_size)
+			largest_hole = hole_size;
+	}
+
+	drm_mm_for_each_node(entry, mm) {
+		if (entry->hole_follows) {
+			hole_start = drm_mm_hole_node_start(entry);
+			hole_end = drm_mm_hole_node_end(entry);
+			hole_size = hole_end - hole_start;
+			total_free += hole_size;
+			if (largest_hole < hole_size)
+				largest_hole = hole_size;
+		}
+	}
+
+	*stolen_free = total_free;
+	*stolen_largest = largest_hole;
+}
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 8f38407..424e57e 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1012,6 +1012,12 @@ struct drm_i915_gem_get_aperture {
 	 * Total space in the mappable region of the aperture, in bytes
 	 */
 	__u64 map_total_size;
+
+	/**
+	 * Total space in the stolen region, in bytes
+	 */
+	__u64 stolen_total_size;
+
 };
 
 struct drm_i915_get_pipe_from_crtc_id {
-- 
1.9.1

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

Re: [PATCH 01/12] drm/i915: Add support for mapping an object page by page

[Chris Wilson]

On Wed, Apr 20, 2016 at 04:47:28PM +0530, ankitprasad.r.sharma@intel.com wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Introduced a new vm specfic callback insert_page() to program a single pte in
> ggtt or ppgtt. This allows us to map a single page in to the mappable aperture
> space. This can be iterated over to access the whole object by using space as
> meagre as page size.
> 
> v2: Added low level rpm assertions to insert_page routines (Chris)
> 
> v3: Added POSTING_READ post register write (Tvrtko)
> 
> v4: Rebase (Ankit)
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/char/agp/intel-gtt.c        |  9 +++++
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 67 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_gem_gtt.h |  5 +++
>  include/drm/intel-gtt.h             |  3 ++
>  4 files changed, 84 insertions(+)
> 
> diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
> index aef87fd..cea0ae3 100644
> --- a/drivers/char/agp/intel-gtt.c
> +++ b/drivers/char/agp/intel-gtt.c
> @@ -840,6 +840,15 @@ static bool i830_check_flags(unsigned int flags)
>  	return false;
>  }
>  
> +void intel_gtt_insert_page(dma_addr_t addr,
> +			   unsigned int pg,
> +			   unsigned int flags)
> +{
> +	intel_private.driver->write_entry(addr, pg, flags);
> +	wmb();
> +}
> +EXPORT_SYMBOL(intel_gtt_insert_page);
> +
>  void intel_gtt_insert_sg_entries(struct sg_table *st,
>  				 unsigned int pg_start,
>  				 unsigned int flags)
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 9f165fe..da1819d 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2353,6 +2353,29 @@ static void gen8_set_pte(void __iomem *addr, gen8_pte_t pte)
>  #endif
>  }
>  
> +static void gen8_ggtt_insert_page(struct i915_address_space *vm,
> +				  dma_addr_t addr,
> +				  uint64_t offset,
> +				  enum i915_cache_level level,
> +				  u32 unused)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(vm->dev);
> +	gen8_pte_t __iomem *pte =
> +		(gen8_pte_t __iomem *)dev_priv->ggtt.gsm +
> +		(offset >> PAGE_SHIFT);
> +	int rpm_atomic_seq;
> +
> +	rpm_atomic_seq = assert_rpm_atomic_begin(dev_priv);
> +
> +	gen8_set_pte(pte, gen8_pte_encode(addr, level, true));
> +	wmb();
> +
> +	I915_WRITE(GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
> +	POSTING_READ(GFX_FLSH_CNTL_GEN6);

Actually, we don't need these at all for the insert-page API (current
users in pread/pwrite/reloc/gpu-error-capture) as they go through the
GTT and not the GPU TLB. I would much rather we punt these to the caller
with a vm->flush() which we can add later. When I say later, we already
have the GUC pretending to be the GGTT and adding code where it doesn't
belong...

All we need here is the wmb() to order the PTE write (and flush the WCB)
with the use afterwards. The caller has to provide the wmb() to order
its writes before a subsequent PTE change.

In a few patches time we can also kill the rpm_atomic asserts and the
dev_priv local.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/12] drm/i915: Introduce i915_gem_object_get_dma_address()

[Chris Wilson]

On Wed, Apr 20, 2016 at 04:47:29PM +0530, ankitprasad.r.sharma@intel.com wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> This utility function is a companion to i915_gem_object_get_page() that
> uses the same cached iterator for the scatterlist to perform fast
> sequential lookup of the dma address associated with any page within the
> object.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index b9ed1b3..883b706 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2977,6 +2977,23 @@ static inline int __sg_page_count(struct scatterlist *sg)
>  struct page *
>  i915_gem_object_get_dirty_page(struct drm_i915_gem_object *obj, int n);
>  
> +static inline dma_addr_t
> +i915_gem_object_get_dma_address(struct drm_i915_gem_object *obj, int n)

s/int n/unsigned long n/

> +{

Most recently, I felt this was too big to be inlined and stuck

	lockdep_assert_held(&obj->base.dev->struct_mutex);
	GEM_BUG_ON(n >= obj->base.size >> PAGE_SHIFT);
	GEM_BUG_ON(obj->pages_pin_count == 0);

in here as well.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/12] drm/i915: Support for pread/pwrite from/to non shmem backed objects

[Chris Wilson]

On Wed, Apr 20, 2016 at 04:47:35PM +0530, ankitprasad.r.sharma@intel.com wrote:
>  static int
>  i915_gem_shmem_pread(struct drm_device *dev,
>  		     struct drm_i915_gem_object *obj,
> @@ -656,6 +795,9 @@ i915_gem_shmem_pread(struct drm_device *dev,
>  	int needs_clflush = 0;
>  	struct sg_page_iter sg_iter;
>  
> +	if (!obj->base.filp)
> +		return -ENODEV;

This is now:

if ((obj->ops->flags & I915_GEM_OBJECT_HAS_STRUCT_PAGE) == 0)
	return -ENODEV;

> @@ -866,22 +1009,34 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_private *i915,
>  		unsigned page_length = PAGE_SIZE - page_offset;
>  		page_length = remain < page_length ? remain : page_length;
>  		if (node.allocated) {
> -			wmb();
> +			wmb(); /* flush the write before we modify the GGTT */
>  			ggtt->base.insert_page(&ggtt->base,
>  					       i915_gem_object_get_dma_address(obj, offset >> PAGE_SHIFT),
>  					       node.start, I915_CACHE_NONE, 0);
> -			wmb();
> +			wmb(); /* flush modifications to the GGTT (insert_page) */

We might as delete this wmb(); then we don't even need to document it ;)
Hmm. On reflection, we can punt wmb() (from insert_page()) to the caller
as well as it is much clearer, so go back to patch 1 and remove both the
wmb() and chipset flushes.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 03/12] drm/i915: Use insert_page for pwrite_fast

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 23039 bytes --]

Hi,

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on next-20160420]
[cannot apply to v4.6-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/ankitprasad-r-sharma-intel-com/Support-for-creating-using-Stolen-memory-backed-objects/20160420-194608
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   include/drm/drm_crtc.h:1549: warning: No description found for parameter 'helper_private'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'tile_idr'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'connector_ida'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'delayed_event'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'edid_property'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'dpms_property'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'path_property'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'tile_property'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'plane_type_property'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'rotation_property'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'prop_src_x'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'prop_src_y'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'prop_src_w'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'prop_src_h'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'prop_crtc_x'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'prop_crtc_y'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'prop_crtc_w'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'prop_crtc_h'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'prop_fb_id'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'prop_crtc_id'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'prop_active'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'prop_mode_id'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'dvi_i_subconnector_property'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'dvi_i_select_subconnector_property'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'tv_subconnector_property'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'tv_select_subconnector_property'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'tv_mode_property'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'tv_left_margin_property'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'tv_right_margin_property'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'tv_top_margin_property'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'tv_bottom_margin_property'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'tv_brightness_property'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'tv_contrast_property'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'tv_flicker_reduction_property'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'tv_overscan_property'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'tv_saturation_property'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'tv_hue_property'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'scaling_mode_property'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'aspect_ratio_property'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'dirty_info_property'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'suggested_x_property'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'suggested_y_property'
   include/drm/drm_crtc.h:2185: warning: No description found for parameter 'allow_fb_modifiers'
   drivers/gpu/drm/drm_atomic_helper.c:2913: warning: No description found for parameter 'start'
   drivers/gpu/drm/drm_atomic_helper.c:2913: warning: No description found for parameter 'start'
   drivers/gpu/drm/drm_atomic_helper.c:2913: warning: No description found for parameter 'start'
   drivers/gpu/drm/drm_atomic_helper.c:2913: warning: No description found for parameter 'start'
   include/drm/drm_dp_helper.h:749: warning: No description found for parameter 'i2c_nack_count'
   include/drm/drm_dp_helper.h:749: warning: No description found for parameter 'i2c_defer_count'
   drivers/gpu/drm/drm_dp_mst_topology.c:2356: warning: No description found for parameter 'connector'
   include/drm/drm_dp_mst_helper.h:92: warning: No description found for parameter 'cached_edid'
   include/drm/drm_dp_mst_helper.h:92: warning: No description found for parameter 'has_audio'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'max_dpcd_transaction_bytes'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'sink_count'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'total_slots'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'avail_slots'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'total_pbn'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'qlock'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_msg_downq'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_down_in_progress'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'payload_lock'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'proposed_vcpis'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'payloads'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'payload_mask'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'vcpi_mask'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_waitq'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'work'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'tx_work'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'destroy_connector_list'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'destroy_connector_lock'
   include/drm/drm_dp_mst_helper.h:466: warning: No description found for parameter 'destroy_connector_work'
   drivers/gpu/drm/drm_dp_mst_topology.c:2356: warning: No description found for parameter 'connector'
   drivers/gpu/drm/drm_irq.c:176: warning: No description found for parameter 'flags'
   include/drm/drmP.h:168: warning: No description found for parameter 'fmt'
   include/drm/drmP.h:184: warning: No description found for parameter 'fmt'
   include/drm/drmP.h:202: warning: No description found for parameter 'fmt'
   include/drm/drmP.h:247: warning: No description found for parameter 'dev'
   include/drm/drmP.h:247: warning: No description found for parameter 'data'
   include/drm/drmP.h:247: warning: No description found for parameter 'file_priv'
   include/drm/drmP.h:280: warning: No description found for parameter 'ioctl'
   include/drm/drmP.h:280: warning: No description found for parameter '_func'
   include/drm/drmP.h:280: warning: No description found for parameter '_flags'
   include/drm/drmP.h:362: warning: cannot understand function prototype: 'struct drm_lock_data '
   include/drm/drmP.h:415: warning: cannot understand function prototype: 'struct drm_driver '
   include/drm/drmP.h:672: warning: cannot understand function prototype: 'struct drm_info_list '
   include/drm/drmP.h:682: warning: cannot understand function prototype: 'struct drm_info_node '
   include/drm/drmP.h:692: warning: cannot understand function prototype: 'struct drm_minor '
   include/drm/drmP.h:740: warning: cannot understand function prototype: 'struct drm_device '
   drivers/gpu/drm/i915/intel_runtime_pm.c:2391: warning: No description found for parameter 'resume'
   drivers/gpu/drm/i915/intel_runtime_pm.c:2391: warning: No description found for parameter 'resume'
   drivers/gpu/drm/i915/i915_irq.c:2722: warning: No description found for parameter 'fmt'
   drivers/gpu/drm/i915/i915_irq.c:2722: warning: No description found for parameter 'fmt'
   drivers/gpu/drm/i915/i915_irq.c:2722: warning: No description found for parameter 'fmt'
   drivers/gpu/drm/i915/i915_irq.c:2722: warning: No description found for parameter 'fmt'
   drivers/gpu/drm/i915/i915_gem.c:434: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:434: warning: No description found for parameter 'data'
   drivers/gpu/drm/i915/i915_gem.c:434: warning: No description found for parameter 'file'
   drivers/gpu/drm/i915/i915_gem.c:699: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:699: warning: No description found for parameter 'data'
   drivers/gpu/drm/i915/i915_gem.c:699: warning: No description found for parameter 'file'
>> drivers/gpu/drm/i915/i915_gem.c:780: warning: No description found for parameter 'i915'
   drivers/gpu/drm/i915/i915_gem.c:780: warning: No description found for parameter 'obj'
   drivers/gpu/drm/i915/i915_gem.c:780: warning: No description found for parameter 'args'
   drivers/gpu/drm/i915/i915_gem.c:780: warning: No description found for parameter 'file'
   drivers/gpu/drm/i915/i915_gem.c:1071: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:1071: warning: No description found for parameter 'data'
   drivers/gpu/drm/i915/i915_gem.c:1071: warning: No description found for parameter 'file'
   drivers/gpu/drm/i915/i915_gem.c:1277: warning: No description found for parameter 'rps'
   drivers/gpu/drm/i915/i915_gem.c:1492: warning: No description found for parameter 'req'
   drivers/gpu/drm/i915/i915_gem.c:1516: warning: No description found for parameter 'obj'
   drivers/gpu/drm/i915/i915_gem.c:1516: warning: No description found for parameter 'readonly'
   drivers/gpu/drm/i915/i915_gem.c:1632: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:1632: warning: No description found for parameter 'data'
   drivers/gpu/drm/i915/i915_gem.c:1632: warning: No description found for parameter 'file'
   drivers/gpu/drm/i915/i915_gem.c:1695: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:1695: warning: No description found for parameter 'data'
   drivers/gpu/drm/i915/i915_gem.c:1695: warning: No description found for parameter 'file'
   drivers/gpu/drm/i915/i915_gem.c:1740: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:1740: warning: No description found for parameter 'data'
   drivers/gpu/drm/i915/i915_gem.c:1740: warning: No description found for parameter 'file'
   drivers/gpu/drm/i915/i915_gem.c:2045: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:2045: warning: No description found for parameter 'size'
   drivers/gpu/drm/i915/i915_gem.c:2045: warning: No description found for parameter 'tiling_mode'
   drivers/gpu/drm/i915/i915_gem.c:2045: warning: No description found for parameter 'fenced'
   drivers/gpu/drm/i915/i915_gem.c:2045: warning: Excess function parameter 'obj' description in 'i915_gem_get_gtt_alignment'
   drivers/gpu/drm/i915/i915_gem.c:3002: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/i915_gem.c:3128: warning: No description found for parameter 'obj'
   drivers/gpu/drm/i915/i915_gem.c:3178: warning: No description found for parameter 'dev'
   drivers/gpu/drm/i915/i915_gem.c:3178: warning: No description found for parameter 'data'
   drivers/gpu/drm/i915/i915_gem.c:3178: warning: No description found for parameter 'file'
   drivers/gpu/drm/i915/i915_gem.c:3178: warning: Excess function parameter 'DRM_IOCTL_ARGS' description in 'i915_gem_wait_ioctl'
   drivers/gpu/drm/i915/i915_gem.c:3540: warning: No description found for parameter 'obj'
   drivers/gpu/drm/i915/i915_gem.c:3540: warning: No description found for parameter 'vm'
   drivers/gpu/drm/i915/i915_gem.c:3540: warning: No description found for parameter 'ggtt_view'
   drivers/gpu/drm/i915/i915_gem.c:3540: warning: No description found for parameter 'alignment'
   drivers/gpu/drm/i915/i915_gem.c:3540: warning: No description found for parameter 'flags'
   drivers/gpu/drm/i915/i915_gem.c:3796: warning: No description found for parameter 'obj'
   drivers/gpu/drm/i915/i915_gem.c:3796: warning: No description found for parameter 'write'
   drivers/gpu/drm/i915/i915_gem.c:3874: warning: No description found for parameter 'obj'
   drivers/gpu/drm/i915/i915_gem.c:3874: warning: No description found for parameter 'cache_level'
   drivers/gpu/drm/i915/i915_gem.c:4148: warning: No description found for parameter 'obj'
   drivers/gpu/drm/i915/i915_gem.c:4148: warning: No description found for parameter 'write'
   drivers/gpu/drm/i915/i915_cmd_parser.c:748: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/i915_cmd_parser.c:748: warning: Excess function parameter 'ring' description in 'i915_cmd_parser_init_ring'
   drivers/gpu/drm/i915/i915_cmd_parser.c:838: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/i915_cmd_parser.c:838: warning: Excess function parameter 'ring' description in 'i915_cmd_parser_fini_ring'
   drivers/gpu/drm/i915/i915_cmd_parser.c:1034: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/i915_cmd_parser.c:1034: warning: Excess function parameter 'ring' description in 'i915_needs_cmd_parser'
   drivers/gpu/drm/i915/i915_cmd_parser.c:1186: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/i915_cmd_parser.c:1186: warning: Excess function parameter 'ring' description in 'i915_parse_cmds'
   drivers/gpu/drm/i915/i915_cmd_parser.c:748: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/i915_cmd_parser.c:748: warning: Excess function parameter 'ring' description in 'i915_cmd_parser_init_ring'
   drivers/gpu/drm/i915/i915_cmd_parser.c:838: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/i915_cmd_parser.c:838: warning: Excess function parameter 'ring' description in 'i915_cmd_parser_fini_ring'
   drivers/gpu/drm/i915/i915_cmd_parser.c:1034: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/i915_cmd_parser.c:1034: warning: Excess function parameter 'ring' description in 'i915_needs_cmd_parser'
   drivers/gpu/drm/i915/i915_cmd_parser.c:1186: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/i915_cmd_parser.c:1186: warning: Excess function parameter 'ring' description in 'i915_parse_cmds'
   drivers/gpu/drm/i915/intel_lrc.c:316: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/intel_lrc.c:316: warning: Excess function parameter 'ring' description in 'intel_lr_context_descriptor_update'
   drivers/gpu/drm/i915/intel_lrc.c:353: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/intel_lrc.c:353: warning: Excess function parameter 'ring' description in 'intel_execlists_ctx_id'
   drivers/gpu/drm/i915/intel_lrc.c:544: warning: No description found for parameter 'data'
   drivers/gpu/drm/i915/intel_lrc.c:544: warning: Excess function parameter 'engine' description in 'intel_lrc_irq_handler'
   drivers/gpu/drm/i915/intel_lrc.c:938: warning: No description found for parameter 'params'
   drivers/gpu/drm/i915/intel_lrc.c:938: warning: Excess function parameter 'dev' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:938: warning: Excess function parameter 'file' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:938: warning: Excess function parameter 'ring' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:938: warning: Excess function parameter 'ctx' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:938: warning: Excess function parameter 'batch_obj' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:938: warning: Excess function parameter 'exec_start' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:938: warning: Excess function parameter 'dispatch_flags' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:1323: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/intel_lrc.c:1323: warning: Excess function parameter 'ring' description in 'gen8_init_indirectctx_bb'
   drivers/gpu/drm/i915/intel_lrc.c:1386: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/intel_lrc.c:1386: warning: Excess function parameter 'ring' description in 'gen8_init_perctx_bb'
   drivers/gpu/drm/i915/intel_lrc.c:2029: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/intel_lrc.c:2029: warning: Excess function parameter 'ring' description in 'intel_logical_ring_cleanup'
   drivers/gpu/drm/i915/intel_lrc.c:2614: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/intel_lrc.c:2614: warning: Excess function parameter 'ring' description in 'intel_lr_context_size'
   drivers/gpu/drm/i915/intel_lrc.c:2653: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/intel_lrc.c:2653: warning: Excess function parameter 'ring' description in 'intel_lr_context_deferred_alloc'
   drivers/gpu/drm/i915/intel_lrc.c:316: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/intel_lrc.c:316: warning: Excess function parameter 'ring' description in 'intel_lr_context_descriptor_update'
   drivers/gpu/drm/i915/intel_lrc.c:353: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/intel_lrc.c:353: warning: Excess function parameter 'ring' description in 'intel_execlists_ctx_id'
   drivers/gpu/drm/i915/intel_lrc.c:544: warning: No description found for parameter 'data'
   drivers/gpu/drm/i915/intel_lrc.c:544: warning: Excess function parameter 'engine' description in 'intel_lrc_irq_handler'
   drivers/gpu/drm/i915/intel_lrc.c:938: warning: No description found for parameter 'params'
   drivers/gpu/drm/i915/intel_lrc.c:938: warning: Excess function parameter 'dev' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:938: warning: Excess function parameter 'file' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:938: warning: Excess function parameter 'ring' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:938: warning: Excess function parameter 'ctx' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:938: warning: Excess function parameter 'batch_obj' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:938: warning: Excess function parameter 'exec_start' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:938: warning: Excess function parameter 'dispatch_flags' description in 'intel_execlists_submission'
   drivers/gpu/drm/i915/intel_lrc.c:1323: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/intel_lrc.c:1323: warning: Excess function parameter 'ring' description in 'gen8_init_indirectctx_bb'
   drivers/gpu/drm/i915/intel_lrc.c:1386: warning: No description found for parameter 'engine'
   drivers/gpu/drm/i915/intel_lrc.c:1386: warning: Excess function parameter 'ring' description in 'gen8_init_perctx_bb'
   drivers/gpu/drm/i915/intel_lrc.c:2029: warning: No description found for parameter 'engine'

vim +/i915 +780 drivers/gpu/drm/i915/i915_gem.c

4f0c7cfb Ben Widawsky       2012-04-16  764  	vaddr = (void __force*)vaddr_atomic + page_offset;
4f0c7cfb Ben Widawsky       2012-04-16  765  	unwritten = __copy_from_user_inatomic_nocache(vaddr,
0839ccb8 Keith Packard      2008-10-30  766  						      user_data, length);
3e4d3af5 Peter Zijlstra     2010-10-26  767  	io_mapping_unmap_atomic(vaddr_atomic);
fbd5a26d Chris Wilson       2010-10-14  768  	return unwritten;
0839ccb8 Keith Packard      2008-10-30  769  }
0839ccb8 Keith Packard      2008-10-30  770  
3de09aa3 Eric Anholt        2009-03-09  771  /**
3de09aa3 Eric Anholt        2009-03-09  772   * This is the fast pwrite path, where we copy the data directly from the
3de09aa3 Eric Anholt        2009-03-09  773   * user into the GTT, uncached.
3de09aa3 Eric Anholt        2009-03-09  774   */
673a394b Eric Anholt        2008-07-30  775  static int
81e361f6 Ankitprasad Sharma 2016-04-20  776  i915_gem_gtt_pwrite_fast(struct drm_i915_private *i915,
05394f39 Chris Wilson       2010-11-08  777  			 struct drm_i915_gem_object *obj,
673a394b Eric Anholt        2008-07-30  778  			 struct drm_i915_gem_pwrite *args,
05394f39 Chris Wilson       2010-11-08  779  			 struct drm_file *file)
673a394b Eric Anholt        2008-07-30 @780  {
81e361f6 Ankitprasad Sharma 2016-04-20  781  	struct i915_ggtt *ggtt = &i915->ggtt;
81e361f6 Ankitprasad Sharma 2016-04-20  782  	struct drm_mm_node node;
81e361f6 Ankitprasad Sharma 2016-04-20  783  	uint64_t remain, offset;
673a394b Eric Anholt        2008-07-30  784  	char __user *user_data;
81e361f6 Ankitprasad Sharma 2016-04-20  785  	int ret;
935aaa69 Daniel Vetter      2012-03-25  786  
1ec9e26d Daniel Vetter      2014-02-14  787  	ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE | PIN_NONBLOCK);
81e361f6 Ankitprasad Sharma 2016-04-20  788  	if (ret) {

:::::: The code at line 780 was first introduced by commit
:::::: 673a394b1e3b69be886ff24abfd6df97c52e8d08 drm: Add GEM ("graphics execution manager") to i915 driver.

:::::: TO: Eric Anholt <eric@anholt.net>
:::::: CC: Dave Airlie <airlied@linux.ie>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 6302 bytes --]

[-- Attachment #3: 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 11/12] drm/i915: Extend GET_APERTURE ioctl to report available map space

[kbuild test robot]

Hi,

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on next-20160420]
[cannot apply to v4.6-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/ankitprasad-r-sharma-intel-com/Support-for-creating-using-Stolen-memory-backed-objects/20160420-194608
base:   git://anongit.freedesktop.org/drm-intel for-linux-next


coccinelle warnings: (new ones prefixed by >>)

>> drivers/gpu/drm/i915/i915_gem.c:168:43-44: Unneeded semicolon

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: fix semicolon.cocci warnings

[kbuild test robot]

drivers/gpu/drm/i915/i915_gem.c:168:43-44: Unneeded semicolon


 Remove unneeded semicolon.

Generated by: scripts/coccinelle/misc/semicolon.cocci

CC: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---

 i915_gem.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -165,7 +165,7 @@ i915_gem_get_aperture_ioctl(struct drm_d
 
 	args->aper_size = ggtt->base.total;
 	args->aper_available_size = args->aper_size - pinned;
-	args->map_total_size = ggtt->mappable_end;;
+	args->map_total_size = ggtt->mappable_end;
 
 	return 0;
 }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for Support for creating/using Stolen memory backed objects (rev13)

[Patchwork]

== Series Details ==

Series: Support for creating/using Stolen memory backed objects (rev13)
URL   : https://patchwork.freedesktop.org/series/659/
State : failure

== Summary ==

Series 659v13 Support for creating/using Stolen memory backed objects
http://patchwork.freedesktop.org/api/1.0/series/659/revisions/13/mbox/

Test drv_module_reload_basic:
                fail       -> PASS       (snb-dellxps)
Test gem_exec_basic:
        Subgroup gtt-bsd:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test gem_pread:
        Subgroup basic:
                pass       -> FAIL       (snb-dellxps)
Test gem_pwrite:
        Subgroup basic:
                pass       -> FAIL       (snb-dellxps)
Test kms_force_connector_basic:
        Subgroup force-edid:
                pass       -> SKIP       (ivb-t430s)

bdw-nuci7        total:192  pass:180  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:192  pass:169  dwarn:0   dfail:0   fail:0   skip:23 
bsw-nuc-2        total:191  pass:152  dwarn:0   dfail:0   fail:0   skip:39 
byt-nuc          total:191  pass:153  dwarn:0   dfail:0   fail:0   skip:38 
hsw-brixbox      total:192  pass:168  dwarn:0   dfail:0   fail:0   skip:24 
hsw-gt2          total:192  pass:173  dwarn:0   dfail:0   fail:0   skip:19 
ilk-hp8440p      total:192  pass:135  dwarn:0   dfail:0   fail:0   skip:57 
ivb-t430s        total:192  pass:163  dwarn:0   dfail:0   fail:0   skip:29 
skl-i7k-2        total:192  pass:167  dwarn:0   dfail:0   fail:0   skip:25 
skl-nuci5        total:192  pass:181  dwarn:0   dfail:0   fail:0   skip:11 
snb-dellxps      total:192  pass:152  dwarn:0   dfail:0   fail:2   skip:38 
snb-x220t        total:192  pass:154  dwarn:0   dfail:0   fail:1   skip:37 

Results at /archive/results/CI_IGT_test/Patchwork_1956/

5f13745078ab86d9833a181980afc7888157a9a4 drm-intel-nightly: 2016y-04m-20d-13h-46m-48s UTC integration manifest
568830a drm/i915: Support for creating Stolen memory backed objects
3b1dd9f drm/i915: Clearing buffer objects via CPU/GTT
6dd9ff9 drm/i915: Use insert_page for pwrite_fast
8208f24 drm/i915: Introduce i915_gem_object_get_dma_address()
0eeca57 drm/i915: Add support for mapping an object page by page

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

Re: [PATCH 10/12] drm/i915: Disable use of stolen area by User when Intel RST is present

[Lukas Wunner]

Hi Ankitprasad,

just a quick heads-up, Rafael asked that the function name
acpi_dev_present() be changed, so there's now a commit queued
for Linux 4.7 to rename it to acpi_dev_found():

https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=c68ae33e7fb4

It will therefore unfortunately be necessary that you respin
this patch with the new function name once the above-linked
commit has landed in Linus' tree (which I expect will happen
early in the merge window, sometime in mid-May).

My apologies for the inconvenience!

Lukas

On Wed, Apr 20, 2016 at 04:47:37PM +0530, ankitprasad.r.sharma@intel.com wrote:
> From: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
> 
> The BIOS RapidStartTechnology may corrupt the stolen memory across S3
> suspend due to unalarmed hibernation, in which case we will not be able
> to preserve the User data stored in the stolen region. Hence this patch
> tries to identify presence of the RST device on the ACPI bus, and
> disables use of stolen memory (for persistent data) if found.
> 
> v2: Updated comment, updated/corrected new functions private to driver
> (Chris/Tvrtko)
> 
> v3: Disabling stolen by default, wait till required acpi changes to
> detect device presence are pulled in (Ankit)
> 
> v4: Enabled stolen by default as required acpi changes are merged
> (Ankit)
> 
> v5: renamed variable, is IS_ENABLED() in place of #ifdef, use char*
> instead of structures (Lukas)
> 
> Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
> Cc: Lukas Wunner <lukas@wunner.de>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h        | 11 +++++++++++
>  drivers/gpu/drm/i915/i915_gem.c        |  8 ++++++++
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 12 ++++++++++++
>  drivers/gpu/drm/i915/intel_acpi.c      |  7 +++++++
>  4 files changed, 38 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c7fe863..37f9de8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1326,6 +1326,16 @@ struct i915_gem_mm {
>  	 */
>  	bool busy;
>  
> +	/**
> +	 * Stolen will be lost upon hibernate (as the memory is unpowered).
> +	 * Across resume, we expect stolen to be intact - however, it may
> +	 * also be utililised by third parties (e.g. Intel RapidStart
> +	 * Technology) and if so we have to assume that any data stored in
> +	 * stolen across resume is lost and we set this flag to indicate that
> +	 * the stolen memory is volatile.
> +	 */
> +	bool volatile_stolen;
> +
>  	/* the indicator for dispatch video commands on two BSD rings */
>  	unsigned int bsd_ring_dispatch_index;
>  
> @@ -3559,6 +3569,7 @@ static inline int intel_opregion_get_panel_type(struct drm_device *dev)
>  #endif
>  
>  /* intel_acpi.c */
> +bool intel_detect_acpi_rst(void);
>  #ifdef CONFIG_ACPI
>  extern void intel_register_dsm_handler(void);
>  extern void intel_unregister_dsm_handler(void);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 48959cf..45fd049 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -391,8 +391,16 @@ static struct drm_i915_gem_object *
>  i915_gem_alloc_object_stolen(struct drm_device *dev, size_t size)
>  {
>  	struct drm_i915_gem_object *obj;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int ret;
>  
> +	if (dev_priv->mm.volatile_stolen) {
> +		/* Stolen may be overwritten by external parties
> +		 * so unsuitable for persistent user data.
> +		 */
> +		return ERR_PTR(-ENODEV);
> +	}
> +
>  	mutex_lock(&dev->struct_mutex);
>  	obj = i915_gem_object_create_stolen(dev, size);
>  	if (IS_ERR(obj))
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index d9756ee..ef28af6 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -490,6 +490,18 @@ int i915_gem_init_stolen(struct drm_device *dev)
>  	 */
>  	drm_mm_init(&dev_priv->mm.stolen, 0, ggtt->stolen_usable_size);
>  
> +	/* If the stolen region can be modified behind our backs upon suspend,
> +	 * then we cannot use it to store nonvolatile contents (i.e user data)
> +	 * as it will be corrupted upon resume.
> +	 */
> +	dev_priv->mm.volatile_stolen = false;
> +	if (IS_ENABLED(CONFIG_SUSPEND)) {
> +		/* BIOSes using RapidStart Technology have been reported
> +		 * to overwrite stolen across S3, not just S4.
> +		 */
> +		dev_priv->mm.volatile_stolen = intel_detect_acpi_rst();
> +	}
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_acpi.c b/drivers/gpu/drm/i915/intel_acpi.c
> index eb638a1..05fd67f 100644
> --- a/drivers/gpu/drm/i915/intel_acpi.c
> +++ b/drivers/gpu/drm/i915/intel_acpi.c
> @@ -23,6 +23,8 @@ static const u8 intel_dsm_guid[] = {
>  	0x0f, 0x13, 0x17, 0xb0, 0x1c, 0x2c
>  };
>  
> +static const char *irst_id = "INT3392";
> +
>  static char *intel_dsm_port_name(u8 id)
>  {
>  	switch (id) {
> @@ -162,3 +164,8 @@ void intel_register_dsm_handler(void)
>  void intel_unregister_dsm_handler(void)
>  {
>  }
> +
> +bool intel_detect_acpi_rst(void)
> +{
> +	return acpi_dev_present(irst_id);
> +}
> -- 
> 1.9.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/12] drm/i915: Extend GET_APERTURE ioctl to report available map space

[Tvrtko Ursulin]

Hi,

On 20/04/16 12:17, ankitprasad.r.sharma@intel.com wrote:
> From: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>

Patch is mostly Rodrigo's, right? So I assumed he approved the 
authorship transfer.

> When constructing a batchbuffer, it is sometimes crucial to know the
> largest hole into which we can fit a fenceable buffer (for example when
> handling very large objects on gen2 and gen3). This depends on the
> fragmentation of pinned buffers inside the aperture, a question only the
> kernel can easily answer.
>
> This patch extends the current DRM_I915_GEM_GET_APERTURE ioctl to
> include a couple of new fields in its reply to userspace - the total
> amount of space available in the mappable region of the aperture and
> also the single largest block available.
>
> This is not quite what userspace wants to answer the question of whether
> this batch will fit as fences are also required to meet severe alignment
> constraints within the batch. For this purpose, a third conservative
> estimate of largest fence available is also provided. For when userspace
> needs more than one batch, we also provide the culmulative space
> available for fences such that it has some additional guidance to how
> much space it could allocate to fences. Conservatism still wins.
>
> The patch also adds a debugfs file for convenient testing and reporting.
>
> v2: The first object cannot end at offset 0, so we can use last==0 to
> detect the empty list.
>
> v3: Expand all values to 64bit, just in case.
>      Report total mappable aperture size for userspace that cannot easily
>      determine it by inspecting the PCI device.
>
> v4: (Rodrigo) Fixed rebase conflicts.
>
> v5: Keeping limits to get_aperture ioctl, and moved changing numbers to
> debugfs, Addressed comments (Chris/Tvrtko)

It is confusing that you already posted a different v5 on 1st of July 2015.

> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c | 133 ++++++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/i915_gem.c     |   1 +
>   include/uapi/drm/i915_drm.h         |   5 ++
>   3 files changed, 139 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 7f94c6a..d8d7994 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -524,6 +524,138 @@ static int i915_gem_object_info(struct seq_file *m, void* data)
>   	return 0;
>   }
>
> +static int vma_rank_by_ggtt(void *priv,
> +			    struct list_head *A,
> +			    struct list_head *B)
> +{
> +	struct i915_vma *a = list_entry(A, typeof(*a), exec_list);
> +	struct i915_vma *b = list_entry(B, typeof(*b), exec_list);
> +
> +	return a->node.start - b->node.start;
> +}
> +
> +static u32 __fence_size(struct drm_i915_private *dev_priv, u32 start, u32 end)
> +{
> +	u32 size = end - start;
> +	u32 fence_size;
> +
> +	if (INTEL_INFO(dev_priv)->gen < 4) {
> +		u32 fence_max;
> +		u32 fence_next;
> +
> +		if (IS_GEN3(dev_priv)) {
> +			fence_max = I830_FENCE_MAX_SIZE_VAL << 20;
> +			fence_next = 1024*1024;
> +		} else {
> +			fence_max = I830_FENCE_MAX_SIZE_VAL << 19;
> +			fence_next = 512*1024;
> +		}
> +
> +		fence_max = min(fence_max, size);
> +		fence_size = 0;
> +		/* Find fence_size less than fence_max and power of 2 */
> +		while (fence_next <= fence_max) {
> +			u32 base = ALIGN(start, fence_next);
> +			if (base + fence_next > end)
> +				break;
> +
> +			fence_size = fence_next;
> +			fence_next <<= 1;
> +		}
> +	} else {
> +		fence_size = size;
> +	}
> +
> +	return fence_size;
> +}
> +
> +static int i915_gem_aperture_info(struct seq_file *m, void *data)
> +{
> +	struct drm_info_node *node = m->private;
> +	struct drm_device *dev = node->minor->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct i915_ggtt *ggtt = &dev_priv->ggtt;
> +	struct drm_i915_gem_get_aperture arg;
> +	struct i915_vma *vma;
> +	struct list_head map_list;
> +	const uint64_t map_limit = ggtt->mappable_end;
> +	uint64_t map_space, map_largest, fence_space, fence_largest;
> +	uint64_t last, size;
> +	int ret;
> +
> +	INIT_LIST_HEAD(&map_list);
> +
> +	map_space = map_largest = 0;
> +	fence_space = fence_largest = 0;
> +
> +	ret = i915_gem_get_aperture_ioctl(node->minor->dev, &arg, NULL);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&dev->struct_mutex);
> +	list_for_each_entry(vma, &ggtt->base.active_list, vm_link)
> +		if (vma->pin_count &&
> +			(vma->node.start + vma->node.size) <= map_limit)
> +			list_add(&vma->exec_list, &map_list);
> +	list_for_each_entry(vma, &ggtt->base.inactive_list, vm_link)
> +		if (vma->pin_count &&
> +			(vma->node.start + vma->node.size) <= map_limit)
> +			list_add(&vma->exec_list, &map_list);

Could squash the two with an outer loop containing pointers to active 
and inactive list, like a pattern from the shrinker or something. 
Wouldn't save many lines of code though so not sure.

> +
> +	last = 0;
> +	list_sort(NULL, &map_list, vma_rank_by_ggtt);
> +	while (!list_empty(&map_list)) {
> +		vma = list_first_entry(&map_list, typeof(*vma), exec_list);
> +		list_del_init(&vma->exec_list);
> +
> +		if (last == 0)
> +			goto skip_first;
> +
> +		size = vma->node.start - last;

hole_size for readability? (took me some time to figure it out)

> +		if (size > map_largest)
> +			map_largest = size;
> +		map_space += size;
> +
> +		size = __fence_size(dev_priv, last, vma->node.start);
> +		if (size > fence_largest)
> +			fence_largest = size;
> +		fence_space += size;
> +
> +skip_first:
> +		last = vma->node.start + vma->node.size;
> +	}
> +	if (last < map_limit) {
> +		size = map_limit - last;
> +		if (size > map_largest)
> +			map_largest = size;
> +		map_space += size;
> +
> +		size = __fence_size(dev_priv, last, map_limit);
> +		if (size > fence_largest)
> +			fence_largest = size;
> +		fence_space += size;
> +	}
> +
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	seq_printf(m, "Total size of the GTT: %llu bytes\n",
> +		   arg.aper_size);
> +	seq_printf(m, "Available space in the GTT: %llu bytes\n",
> +		   arg.aper_available_size);
> +	seq_printf(m, "Total space in the mappable aperture: %llu bytes\n",
> +		   arg.map_total_size);
> +	seq_printf(m, "Available space in the mappable aperture: %llu bytes\n",
> +		   map_space);
> +	seq_printf(m, "Single largest space in the mappable aperture: %llu bytes\n",
> +		   map_largest);
> +	seq_printf(m, "Available space for fences: %llu bytes\n",
> +		   fence_space);
> +	seq_printf(m, "Single largest fence available: %llu bytes\n",
> +		   fence_largest);
> +
> +	return 0;
> +}
> +

In general I find this a lot of code for a feature of questionable 
utility. As such I would prefer someone really stated the need for this 
and explained how it really is useful - even though whetever number they 
get from this may be completely irrelevant by the time it is acted upon.

>   static int i915_gem_gtt_info(struct seq_file *m, void *data)
>   {
>   	struct drm_info_node *node = m->private;
> @@ -5354,6 +5486,7 @@ static int i915_debugfs_create(struct dentry *root,
>   static const struct drm_info_list i915_debugfs_list[] = {
>   	{"i915_capabilities", i915_capabilities, 0},
>   	{"i915_gem_objects", i915_gem_object_info, 0},
> +	{"i915_gem_aperture", i915_gem_aperture_info, 0},
>   	{"i915_gem_gtt", i915_gem_gtt_info, 0},
>   	{"i915_gem_pinned", i915_gem_gtt_info, 0, (void *) PINNED_LIST},
>   	{"i915_gem_active", i915_gem_object_list_info, 0, (void *) ACTIVE_LIST},
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 45fd049..a1be35f 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -165,6 +165,7 @@ i915_gem_get_aperture_ioctl(struct drm_device *dev, void *data,
>
>   	args->aper_size = ggtt->base.total;
>   	args->aper_available_size = args->aper_size - pinned;
> +	args->map_total_size = ggtt->mappable_end;;
>
>   	return 0;
>   }
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index dae02d8..8f38407 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1007,6 +1007,11 @@ struct drm_i915_gem_get_aperture {
>   	 * bytes
>   	 */
>   	__u64 aper_available_size;
> +
> +	/**
> +	 * Total space in the mappable region of the aperture, in bytes
> +	 */
> +	__u64 map_total_size;
>   };
>
>   struct drm_i915_get_pipe_from_crtc_id {
>

And I guess it is up to Daniel to approve any ABI extensions? (CCed)

Open source user for it exists?

Regards,

Tvrtko


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

Re: [PATCH 12/12] drm/i915: Extend GET_APERTURE ioctl to report size of the stolen region

[Tvrtko Ursulin]

On 20/04/16 12:17, ankitprasad.r.sharma@intel.com wrote:
> From: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
>
> This patch extends the GET_APERTURE ioctl to add support
> for getting total size and available size of the stolen region
> as well as single largest block available in the stolen region.
> Also adds debugfs support to retieve the size information of the
> stolen area.
>
> v2: respinned over Rodrigo's patch which extends the GET_APERTURE
> too. Used drm_mm to get the size information of the stolen region
> (Chris)
> Added debugfs support for testing (Ankit)
>
> v3: Rebased to the latest drm-intel-nightly (Ankit)
>
> v4: Addressed comments, moved non-limits to debugfs.c (Chris/Tvrtko)
>
> Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c    | 12 +++++++++-
>   drivers/gpu/drm/i915/i915_drv.h        |  3 +++
>   drivers/gpu/drm/i915/i915_gem.c        |  2 ++
>   drivers/gpu/drm/i915/i915_gem_stolen.c | 41 ++++++++++++++++++++++++++++++++++
>   include/uapi/drm/i915_drm.h            |  6 +++++
>   5 files changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index d8d7994..55d110e 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -580,7 +580,7 @@ static int i915_gem_aperture_info(struct seq_file *m, void *data)
>   	struct list_head map_list;
>   	const uint64_t map_limit = ggtt->mappable_end;
>   	uint64_t map_space, map_largest, fence_space, fence_largest;
> -	uint64_t last, size;
> +	uint64_t last, size, stolen_free, stolen_largest;
>   	int ret;
>
>   	INIT_LIST_HEAD(&map_list);
> @@ -593,6 +593,10 @@ static int i915_gem_aperture_info(struct seq_file *m, void *data)
>   		return ret;
>
>   	mutex_lock(&dev->struct_mutex);
> +
> +	i915_gem_stolen_size_info(dev_priv, &stolen_free,
> +				  &stolen_largest);
> +
>   	list_for_each_entry(vma, &ggtt->base.active_list, vm_link)
>   		if (vma->pin_count &&
>   			(vma->node.start + vma->node.size) <= map_limit)
> @@ -652,6 +656,12 @@ skip_first:
>   		   fence_space);
>   	seq_printf(m, "Single largest fence available: %llu bytes\n",
>   		   fence_largest);
> +	seq_printf(m, "Total size of the stolen region: %llu bytes\n",
> +		   arg.stolen_total_size);
> +	seq_printf(m, "Available size of the stolen region: %llu bytes\n",
> +		   stolen_free);
> +	seq_printf(m, "Single largest area in the stolen region: %llu bytes\n",
> +		   stolen_largest);
>
>   	return 0;
>   }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 37f9de8..d5c6b23 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3424,6 +3424,9 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
>   					       u32 gtt_offset,
>   					       u32 size);
>   int __must_check i915_gem_stolen_freeze(struct drm_i915_private *i915);
> +void i915_gem_stolen_size_info(struct drm_i915_private *dev_priv,
> +			       uint64_t *stolen_free,
> +			       uint64_t *stolen_largest);
>
>   /* i915_gem_shrinker.c */
>   unsigned long i915_gem_shrink(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index a1be35f..e0f2259 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -166,6 +166,8 @@ i915_gem_get_aperture_ioctl(struct drm_device *dev, void *data,
>   	args->aper_size = ggtt->base.total;
>   	args->aper_available_size = args->aper_size - pinned;
>   	args->map_total_size = ggtt->mappable_end;;
> +	args->stolen_total_size =
> +		dev_priv->mm.volatile_stolen ? 0 : ggtt->stolen_usable_size;
>
>   	return 0;
>   }
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index ef28af6..f2621c5 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -912,3 +912,44 @@ int i915_gem_stolen_freeze(struct drm_i915_private *i915)
>
>   	return ret;
>   }
> +
> +
> +void i915_gem_stolen_size_info(struct drm_i915_private *dev_priv,
> +			       uint64_t *stolen_free,
> +			       uint64_t *stolen_largest)
> +{
> +	struct drm_mm *mm = &dev_priv->mm.stolen;
> +	struct drm_mm_node *head_node = &mm->head_node;
> +	struct drm_mm_node *entry;
> +	uint64_t hole_size, hole_start, hole_end, largest_hole = 0;
> +	uint64_t total_free = 0;
> +
> +	if (dev_priv->mm.volatile_stolen) {
> +		*stolen_free = 0;
> +		*stolen_largest = 0;
> +		return;
> +	}
> +
> +	if (head_node->hole_follows) {
> +		hole_start = drm_mm_hole_node_start(head_node);
> +		hole_end = drm_mm_hole_node_end(head_node);
> +		hole_size = hole_end - hole_start;
> +		total_free += hole_size;
> +		if (largest_hole < hole_size)
> +			largest_hole = hole_size;
> +	}
> +

Why does this block need to be separately handled and the loop below 
would not cover it? On first iteration entry will be the head node below 
as well, no?

> +	drm_mm_for_each_node(entry, mm) {
> +		if (entry->hole_follows) {
> +			hole_start = drm_mm_hole_node_start(entry);
> +			hole_end = drm_mm_hole_node_end(entry);
> +			hole_size = hole_end - hole_start;
> +			total_free += hole_size;
> +			if (largest_hole < hole_size)
> +				largest_hole = hole_size;
> +		}
> +	}
> +
> +	*stolen_free = total_free;
> +	*stolen_largest = largest_hole;
> +}
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 8f38407..424e57e 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1012,6 +1012,12 @@ struct drm_i915_gem_get_aperture {
>   	 * Total space in the mappable region of the aperture, in bytes
>   	 */
>   	__u64 map_total_size;
> +
> +	/**
> +	 * Total space in the stolen region, in bytes
> +	 */
> +	__u64 stolen_total_size;
> +

How will the userspace detect existence of the new ioctl fields? Is it 
intended that they try to call it with a buffer of certain size and act 
on the failure when that fails? Is that good enough or we need something 
better like get_param or something?

Regards,

Tvrtko

>   };
>
>   struct drm_i915_get_pipe_from_crtc_id {
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 12/12] drm/i915: Extend GET_APERTURE ioctl to report size of the stolen region

[Chris Wilson]

On Thu, Apr 21, 2016 at 03:17:06PM +0100, Tvrtko Ursulin wrote:
> >+void i915_gem_stolen_size_info(struct drm_i915_private *dev_priv,
> >+			       uint64_t *stolen_free,
> >+			       uint64_t *stolen_largest)
> >+{
> >+	struct drm_mm *mm = &dev_priv->mm.stolen;
> >+	struct drm_mm_node *head_node = &mm->head_node;
> >+	struct drm_mm_node *entry;
> >+	uint64_t hole_size, hole_start, hole_end, largest_hole = 0;
> >+	uint64_t total_free = 0;
> >+
> >+	if (dev_priv->mm.volatile_stolen) {
> >+		*stolen_free = 0;
> >+		*stolen_largest = 0;
> >+		return;
> >+	}
> >+
> >+	if (head_node->hole_follows) {
> >+		hole_start = drm_mm_hole_node_start(head_node);
> >+		hole_end = drm_mm_hole_node_end(head_node);
> >+		hole_size = hole_end - hole_start;
> >+		total_free += hole_size;
> >+		if (largest_hole < hole_size)
> >+			largest_hole = hole_size;
> >+	}
> >+
> 
> Why does this block need to be separately handled and the loop below
> would not cover it? On first iteration entry will be the head node
> below as well, no?

Hmm, didn't I/somebody add drm_mm_for_each_hole() ?

> >+	*stolen_free = total_free;
> >+	*stolen_largest = largest_hole;
> >+}
> >diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> >index 8f38407..424e57e 100644
> >--- a/include/uapi/drm/i915_drm.h
> >+++ b/include/uapi/drm/i915_drm.h
> >@@ -1012,6 +1012,12 @@ struct drm_i915_gem_get_aperture {
> >  	 * Total space in the mappable region of the aperture, in bytes
> >  	 */
> >  	__u64 map_total_size;
> >+
> >+	/**
> >+	 * Total space in the stolen region, in bytes
> >+	 */
> >+	__u64 stolen_total_size;
> >+
> 
> How will the userspace detect existence of the new ioctl fields? Is
> it intended that they try to call it with a buffer of certain size
> and act on the failure when that fails? Is that good enough or we
> need something better like get_param or something?

As we are extending the structure:

1. Old userspace, old kernel: unaffacted

2. Old userspace, new kernel:
Kernel computes the new fields, but the struct is truncated in the copy
back to userspace. userspace only sees the fields it used to, no change.

3. New userspace, old kernel:
Userspace passes in a larger struct, kernel only copies back in the
fields it knows and zero fills the tail of the user's struct. userspace
sees a stolen_total_size of 0 and knows to avoid the new interface.

4. New userspace, new kernel:
Everything explodes^W just works.

In this case, the struct is only an out, so we don't need to do invalid
pad or flag rejection. We just have the ABI rule that anything the
kernel doesn't know about is ignored and zero-filled were applicable.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/12] drm/i915: Extend GET_APERTURE ioctl to report available map space

[Chris Wilson]

On Thu, Apr 21, 2016 at 03:04:52PM +0100, Tvrtko Ursulin wrote:
> 
> Hi,
> 
> On 20/04/16 12:17, ankitprasad.r.sharma@intel.com wrote:
> >+	mutex_unlock(&dev->struct_mutex);
> >+
> >+	seq_printf(m, "Total size of the GTT: %llu bytes\n",
> >+		   arg.aper_size);
> >+	seq_printf(m, "Available space in the GTT: %llu bytes\n",
> >+		   arg.aper_available_size);
> >+	seq_printf(m, "Total space in the mappable aperture: %llu bytes\n",
> >+		   arg.map_total_size);
> >+	seq_printf(m, "Available space in the mappable aperture: %llu bytes\n",
> >+		   map_space);
> >+	seq_printf(m, "Single largest space in the mappable aperture: %llu bytes\n",
> >+		   map_largest);
> >+	seq_printf(m, "Available space for fences: %llu bytes\n",
> >+		   fence_space);
> >+	seq_printf(m, "Single largest fence available: %llu bytes\n",
> >+		   fence_largest);
> >+
> >+	return 0;
> >+}
> >+
> 
> In general I find this a lot of code for a feature of questionable
> utility. As such I would prefer someone really stated the need for
> this and explained how it really is useful - even though whetever
> number they get from this may be completely irrelevant by the time
> it is acted upon.

Yes, with the exception of the size of the mappable aperture, this is
really is debug info. It will get automatically dumped by userspace
when it sees an ENOSPC, and that may prove enough to solve the riddle of
why it failed. However, this information is terrible outdated and now
longer of such relevance.

As for the mappable aperture size, there has been a request many years
ago! could we provide it without resorting to a privilege operation. I
guess by know that request has died out - but there is still the issue
with libpciassess that make it unsuitable for use inside a library where
one may want to avoid it and use a simple ioctl on the device you
already have open.

Yes, it is meh.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 12/12] drm/i915: Extend GET_APERTURE ioctl to report size of the stolen region

[Tvrtko Ursulin]

On 21/04/16 15:41, Chris Wilson wrote:
> On Thu, Apr 21, 2016 at 03:17:06PM +0100, Tvrtko Ursulin wrote:
>>> +void i915_gem_stolen_size_info(struct drm_i915_private *dev_priv,
>>> +			       uint64_t *stolen_free,
>>> +			       uint64_t *stolen_largest)
>>> +{
>>> +	struct drm_mm *mm = &dev_priv->mm.stolen;
>>> +	struct drm_mm_node *head_node = &mm->head_node;
>>> +	struct drm_mm_node *entry;
>>> +	uint64_t hole_size, hole_start, hole_end, largest_hole = 0;
>>> +	uint64_t total_free = 0;
>>> +
>>> +	if (dev_priv->mm.volatile_stolen) {
>>> +		*stolen_free = 0;
>>> +		*stolen_largest = 0;
>>> +		return;
>>> +	}
>>> +
>>> +	if (head_node->hole_follows) {
>>> +		hole_start = drm_mm_hole_node_start(head_node);
>>> +		hole_end = drm_mm_hole_node_end(head_node);
>>> +		hole_size = hole_end - hole_start;
>>> +		total_free += hole_size;
>>> +		if (largest_hole < hole_size)
>>> +			largest_hole = hole_size;
>>> +	}
>>> +
>>
>> Why does this block need to be separately handled and the loop below
>> would not cover it? On first iteration entry will be the head node
>> below as well, no?
>
> Hmm, didn't I/somebody add drm_mm_for_each_hole() ?

I see it in the header, yes.

>>> +	*stolen_free = total_free;
>>> +	*stolen_largest = largest_hole;
>>> +}
>>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
>>> index 8f38407..424e57e 100644
>>> --- a/include/uapi/drm/i915_drm.h
>>> +++ b/include/uapi/drm/i915_drm.h
>>> @@ -1012,6 +1012,12 @@ struct drm_i915_gem_get_aperture {
>>>   	 * Total space in the mappable region of the aperture, in bytes
>>>   	 */
>>>   	__u64 map_total_size;
>>> +
>>> +	/**
>>> +	 * Total space in the stolen region, in bytes
>>> +	 */
>>> +	__u64 stolen_total_size;
>>> +
>>
>> How will the userspace detect existence of the new ioctl fields? Is
>> it intended that they try to call it with a buffer of certain size
>> and act on the failure when that fails? Is that good enough or we
>> need something better like get_param or something?
>
> As we are extending the structure:
>
> 1. Old userspace, old kernel: unaffacted
>
> 2. Old userspace, new kernel:
> Kernel computes the new fields, but the struct is truncated in the copy
> back to userspace. userspace only sees the fields it used to, no change.
>
> 3. New userspace, old kernel:
> Userspace passes in a larger struct, kernel only copies back in the
> fields it knows and zero fills the tail of the user's struct. userspace
> sees a stolen_total_size of 0 and knows to avoid the new interface.

I suppose it doesn't make any practical difference to the driver between 
"there is no stolen memory" and "driver does not support stolen memory 
query / create".

For mappable region it is a bit weirder because it wouldn't be able to 
tell if there is no mappable or no query support so it would potentially 
incorrectly avoid mmap_gtt etc.

> 4. New userspace, new kernel:
> Everything explodes^W just works.
>
> In this case, the struct is only an out, so we don't need to do invalid
> pad or flag rejection. We just have the ABI rule that anything the
> kernel doesn't know about is ignored and zero-filled were applicable.
> -Chris
>

Regards,

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

Re: [PATCH 11/12] drm/i915: Extend GET_APERTURE ioctl to report available map space

[Tvrtko Ursulin]

On 21/04/16 15:46, Chris Wilson wrote:
> On Thu, Apr 21, 2016 at 03:04:52PM +0100, Tvrtko Ursulin wrote:
>>
>> Hi,
>>
>> On 20/04/16 12:17, ankitprasad.r.sharma@intel.com wrote:
>>> +	mutex_unlock(&dev->struct_mutex);
>>> +
>>> +	seq_printf(m, "Total size of the GTT: %llu bytes\n",
>>> +		   arg.aper_size);
>>> +	seq_printf(m, "Available space in the GTT: %llu bytes\n",
>>> +		   arg.aper_available_size);
>>> +	seq_printf(m, "Total space in the mappable aperture: %llu bytes\n",
>>> +		   arg.map_total_size);
>>> +	seq_printf(m, "Available space in the mappable aperture: %llu bytes\n",
>>> +		   map_space);
>>> +	seq_printf(m, "Single largest space in the mappable aperture: %llu bytes\n",
>>> +		   map_largest);
>>> +	seq_printf(m, "Available space for fences: %llu bytes\n",
>>> +		   fence_space);
>>> +	seq_printf(m, "Single largest fence available: %llu bytes\n",
>>> +		   fence_largest);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>
>> In general I find this a lot of code for a feature of questionable
>> utility. As such I would prefer someone really stated the need for
>> this and explained how it really is useful - even though whetever
>> number they get from this may be completely irrelevant by the time
>> it is acted upon.
>
> Yes, with the exception of the size of the mappable aperture, this is
> really is debug info. It will get automatically dumped by userspace
> when it sees an ENOSPC, and that may prove enough to solve the riddle of
> why it failed. However, this information is terrible outdated and now
> longer of such relevance.
>
> As for the mappable aperture size, there has been a request many years
> ago! could we provide it without resorting to a privilege operation. I
> guess by know that request has died out - but there is still the issue
> with libpciassess that make it unsuitable for use inside a library where
> one may want to avoid it and use a simple ioctl on the device you
> already have open.
>
> Yes, it is meh.

Aperture size in the ioctl is fine I think, just that detection caveat 
what I asked in the other reply.

Here I wanted to suggest dropping all the non-trivial debugfs stuff and 
just leave the info queried via i915_gem_get_aperture ioctl. So 
effectively dropping the list traversal and vma sorting bits.

Regards,

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

✓ Fi.CI.BAT: success for Support for creating/using Stolen memory backed objects (rev13)

[Patchwork]

== Series Details ==

Series: Support for creating/using Stolen memory backed objects (rev13)
URL   : https://patchwork.freedesktop.org/series/659/
State : success

== Summary ==

Series 659v13 Support for creating/using Stolen memory backed objects
http://patchwork.freedesktop.org/api/1.0/series/659/revisions/13/mbox/

Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> FAIL       (ilk-hp8440p) UNSTABLE

byt-nuc          total:192  pass:154  dwarn:0   dfail:0   fail:0   skip:38 
ilk-hp8440p      total:193  pass:135  dwarn:0   dfail:0   fail:1   skip:57 

Results at /archive/results/CI_IGT_test/Patchwork_2043/

1e81bacf1f7fdbdf83f46b55389713fa13cb1256 drm-intel-nightly: 2016y-04m-24d-10h-36m-11s UTC integration manifest
8d71fcc drm/i915: Support for creating Stolen memory backed objects
c3b4827 drm/i915: Clearing buffer objects via CPU/GTT
4c32deb drm/i915: Use insert_page for pwrite_fast
60691ca drm/i915: Introduce i915_gem_object_get_dma_address()
8d8d087 drm/i915: Add support for mapping an object page by page

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

Re: [PATCH 11/12] drm/i915: Extend GET_APERTURE ioctl to report available map space

[Ankitprasad Sharma]

On Thu, 2016-04-21 at 15:59 +0100, Tvrtko Ursulin wrote:
> On 21/04/16 15:46, Chris Wilson wrote:
> > On Thu, Apr 21, 2016 at 03:04:52PM +0100, Tvrtko Ursulin wrote:
> >>
> >> Hi,
> >>
> >> On 20/04/16 12:17, ankitprasad.r.sharma@intel.com wrote:
> >>> +	mutex_unlock(&dev->struct_mutex);
> >>> +
> >>> +	seq_printf(m, "Total size of the GTT: %llu bytes\n",
> >>> +		   arg.aper_size);
> >>> +	seq_printf(m, "Available space in the GTT: %llu bytes\n",
> >>> +		   arg.aper_available_size);
> >>> +	seq_printf(m, "Total space in the mappable aperture: %llu bytes\n",
> >>> +		   arg.map_total_size);
> >>> +	seq_printf(m, "Available space in the mappable aperture: %llu bytes\n",
> >>> +		   map_space);
> >>> +	seq_printf(m, "Single largest space in the mappable aperture: %llu bytes\n",
> >>> +		   map_largest);
> >>> +	seq_printf(m, "Available space for fences: %llu bytes\n",
> >>> +		   fence_space);
> >>> +	seq_printf(m, "Single largest fence available: %llu bytes\n",
> >>> +		   fence_largest);
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>
> >> In general I find this a lot of code for a feature of questionable
> >> utility. As such I would prefer someone really stated the need for
> >> this and explained how it really is useful - even though whetever
> >> number they get from this may be completely irrelevant by the time
> >> it is acted upon.
> >
> > Yes, with the exception of the size of the mappable aperture, this is
> > really is debug info. It will get automatically dumped by userspace
> > when it sees an ENOSPC, and that may prove enough to solve the riddle of
> > why it failed. However, this information is terrible outdated and now
> > longer of such relevance.
> >
> > As for the mappable aperture size, there has been a request many years
> > ago! could we provide it without resorting to a privilege operation. I
> > guess by know that request has died out - but there is still the issue
> > with libpciassess that make it unsuitable for use inside a library where
> > one may want to avoid it and use a simple ioctl on the device you
> > already have open.
> >
> > Yes, it is meh.
> 
> Aperture size in the ioctl is fine I think, just that detection caveat 
> what I asked in the other reply.
> 
> Here I wanted to suggest dropping all the non-trivial debugfs stuff and 
> just leave the info queried via i915_gem_get_aperture ioctl. So 
> effectively dropping the list traversal and vma sorting bits.
> 
I think, debug info regarding the mappable space is good to have for
debugging purpose as Chris mentioned.
Also, the list traversal and the vma sorting stuff will only be called
for debugging purposes, not slowing anything down or so.

Thanks,
Ankit

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

Re: [PATCH 11/12] drm/i915: Extend GET_APERTURE ioctl to report available map space

[Tvrtko Ursulin]

On 25/04/16 11:35, Ankitprasad Sharma wrote:
> On Thu, 2016-04-21 at 15:59 +0100, Tvrtko Ursulin wrote:
>> On 21/04/16 15:46, Chris Wilson wrote:
>>> On Thu, Apr 21, 2016 at 03:04:52PM +0100, Tvrtko Ursulin wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 20/04/16 12:17, ankitprasad.r.sharma@intel.com wrote:
>>>>> +	mutex_unlock(&dev->struct_mutex);
>>>>> +
>>>>> +	seq_printf(m, "Total size of the GTT: %llu bytes\n",
>>>>> +		   arg.aper_size);
>>>>> +	seq_printf(m, "Available space in the GTT: %llu bytes\n",
>>>>> +		   arg.aper_available_size);
>>>>> +	seq_printf(m, "Total space in the mappable aperture: %llu bytes\n",
>>>>> +		   arg.map_total_size);
>>>>> +	seq_printf(m, "Available space in the mappable aperture: %llu bytes\n",
>>>>> +		   map_space);
>>>>> +	seq_printf(m, "Single largest space in the mappable aperture: %llu bytes\n",
>>>>> +		   map_largest);
>>>>> +	seq_printf(m, "Available space for fences: %llu bytes\n",
>>>>> +		   fence_space);
>>>>> +	seq_printf(m, "Single largest fence available: %llu bytes\n",
>>>>> +		   fence_largest);
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>
>>>> In general I find this a lot of code for a feature of questionable
>>>> utility. As such I would prefer someone really stated the need for
>>>> this and explained how it really is useful - even though whetever
>>>> number they get from this may be completely irrelevant by the time
>>>> it is acted upon.
>>>
>>> Yes, with the exception of the size of the mappable aperture, this is
>>> really is debug info. It will get automatically dumped by userspace
>>> when it sees an ENOSPC, and that may prove enough to solve the riddle of
>>> why it failed. However, this information is terrible outdated and now
>>> longer of such relevance.
>>>
>>> As for the mappable aperture size, there has been a request many years
>>> ago! could we provide it without resorting to a privilege operation. I
>>> guess by know that request has died out - but there is still the issue
>>> with libpciassess that make it unsuitable for use inside a library where
>>> one may want to avoid it and use a simple ioctl on the device you
>>> already have open.
>>>
>>> Yes, it is meh.
>>
>> Aperture size in the ioctl is fine I think, just that detection caveat
>> what I asked in the other reply.
>>
>> Here I wanted to suggest dropping all the non-trivial debugfs stuff and
>> just leave the info queried via i915_gem_get_aperture ioctl. So
>> effectively dropping the list traversal and vma sorting bits.
>>
> I think, debug info regarding the mappable space is good to have for
> debugging purpose as Chris mentioned.
> Also, the list traversal and the vma sorting stuff will only be called
> for debugging purposes, not slowing anything down or so.

I am pretty indifferent on the topic of debugfs edition.

But for the ioctl extension, how about adding a version field as the 
first one in the extended area?

Regards,

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

Re: [PATCH 11/12] drm/i915: Extend GET_APERTURE ioctl to report available map space

[Chris Wilson]

On Mon, Apr 25, 2016 at 03:51:09PM +0100, Tvrtko Ursulin wrote:
> 
> On 25/04/16 11:35, Ankitprasad Sharma wrote:
> >On Thu, 2016-04-21 at 15:59 +0100, Tvrtko Ursulin wrote:
> >>On 21/04/16 15:46, Chris Wilson wrote:
> >>>On Thu, Apr 21, 2016 at 03:04:52PM +0100, Tvrtko Ursulin wrote:
> >>>>
> >>>>Hi,
> >>>>
> >>>>On 20/04/16 12:17, ankitprasad.r.sharma@intel.com wrote:
> >>>>>+	mutex_unlock(&dev->struct_mutex);
> >>>>>+
> >>>>>+	seq_printf(m, "Total size of the GTT: %llu bytes\n",
> >>>>>+		   arg.aper_size);
> >>>>>+	seq_printf(m, "Available space in the GTT: %llu bytes\n",
> >>>>>+		   arg.aper_available_size);
> >>>>>+	seq_printf(m, "Total space in the mappable aperture: %llu bytes\n",
> >>>>>+		   arg.map_total_size);
> >>>>>+	seq_printf(m, "Available space in the mappable aperture: %llu bytes\n",
> >>>>>+		   map_space);
> >>>>>+	seq_printf(m, "Single largest space in the mappable aperture: %llu bytes\n",
> >>>>>+		   map_largest);
> >>>>>+	seq_printf(m, "Available space for fences: %llu bytes\n",
> >>>>>+		   fence_space);
> >>>>>+	seq_printf(m, "Single largest fence available: %llu bytes\n",
> >>>>>+		   fence_largest);
> >>>>>+
> >>>>>+	return 0;
> >>>>>+}
> >>>>>+
> >>>>
> >>>>In general I find this a lot of code for a feature of questionable
> >>>>utility. As such I would prefer someone really stated the need for
> >>>>this and explained how it really is useful - even though whetever
> >>>>number they get from this may be completely irrelevant by the time
> >>>>it is acted upon.
> >>>
> >>>Yes, with the exception of the size of the mappable aperture, this is
> >>>really is debug info. It will get automatically dumped by userspace
> >>>when it sees an ENOSPC, and that may prove enough to solve the riddle of
> >>>why it failed. However, this information is terrible outdated and now
> >>>longer of such relevance.
> >>>
> >>>As for the mappable aperture size, there has been a request many years
> >>>ago! could we provide it without resorting to a privilege operation. I
> >>>guess by know that request has died out - but there is still the issue
> >>>with libpciassess that make it unsuitable for use inside a library where
> >>>one may want to avoid it and use a simple ioctl on the device you
> >>>already have open.
> >>>
> >>>Yes, it is meh.
> >>
> >>Aperture size in the ioctl is fine I think, just that detection caveat
> >>what I asked in the other reply.
> >>
> >>Here I wanted to suggest dropping all the non-trivial debugfs stuff and
> >>just leave the info queried via i915_gem_get_aperture ioctl. So
> >>effectively dropping the list traversal and vma sorting bits.
> >>
> >I think, debug info regarding the mappable space is good to have for
> >debugging purpose as Chris mentioned.
> >Also, the list traversal and the vma sorting stuff will only be called
> >for debugging purposes, not slowing anything down or so.
> 
> I am pretty indifferent on the topic of debugfs edition.
> 
> But for the ioctl extension, how about adding a version field as the
> first one in the extended area?

A version number only makes sense when you are changing the meaning of
an existing field. Adding one implies that we are planning to do so, are
we?

In the scenarios, I've run through I haven't found one where a caller
would behave any differently faced with "0 - ioctl version not
supported" and "0 - no available space (mappable/stolen)". Adding a
version doesn't help using the new fields afaict. The argument is the
same as whether a flags field is forward thinking or unthinkingly
forward.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/12] drm/i915: Extend GET_APERTURE ioctl to report available map space

[Tvrtko Ursulin]

On 26/04/16 10:44, Chris Wilson wrote:
> On Mon, Apr 25, 2016 at 03:51:09PM +0100, Tvrtko Ursulin wrote:
>>
>> On 25/04/16 11:35, Ankitprasad Sharma wrote:
>>> On Thu, 2016-04-21 at 15:59 +0100, Tvrtko Ursulin wrote:
>>>> On 21/04/16 15:46, Chris Wilson wrote:
>>>>> On Thu, Apr 21, 2016 at 03:04:52PM +0100, Tvrtko Ursulin wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On 20/04/16 12:17, ankitprasad.r.sharma@intel.com wrote:
>>>>>>> +	mutex_unlock(&dev->struct_mutex);
>>>>>>> +
>>>>>>> +	seq_printf(m, "Total size of the GTT: %llu bytes\n",
>>>>>>> +		   arg.aper_size);
>>>>>>> +	seq_printf(m, "Available space in the GTT: %llu bytes\n",
>>>>>>> +		   arg.aper_available_size);
>>>>>>> +	seq_printf(m, "Total space in the mappable aperture: %llu bytes\n",
>>>>>>> +		   arg.map_total_size);
>>>>>>> +	seq_printf(m, "Available space in the mappable aperture: %llu bytes\n",
>>>>>>> +		   map_space);
>>>>>>> +	seq_printf(m, "Single largest space in the mappable aperture: %llu bytes\n",
>>>>>>> +		   map_largest);
>>>>>>> +	seq_printf(m, "Available space for fences: %llu bytes\n",
>>>>>>> +		   fence_space);
>>>>>>> +	seq_printf(m, "Single largest fence available: %llu bytes\n",
>>>>>>> +		   fence_largest);
>>>>>>> +
>>>>>>> +	return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>
>>>>>> In general I find this a lot of code for a feature of questionable
>>>>>> utility. As such I would prefer someone really stated the need for
>>>>>> this and explained how it really is useful - even though whetever
>>>>>> number they get from this may be completely irrelevant by the time
>>>>>> it is acted upon.
>>>>>
>>>>> Yes, with the exception of the size of the mappable aperture, this is
>>>>> really is debug info. It will get automatically dumped by userspace
>>>>> when it sees an ENOSPC, and that may prove enough to solve the riddle of
>>>>> why it failed. However, this information is terrible outdated and now
>>>>> longer of such relevance.
>>>>>
>>>>> As for the mappable aperture size, there has been a request many years
>>>>> ago! could we provide it without resorting to a privilege operation. I
>>>>> guess by know that request has died out - but there is still the issue
>>>>> with libpciassess that make it unsuitable for use inside a library where
>>>>> one may want to avoid it and use a simple ioctl on the device you
>>>>> already have open.
>>>>>
>>>>> Yes, it is meh.
>>>>
>>>> Aperture size in the ioctl is fine I think, just that detection caveat
>>>> what I asked in the other reply.
>>>>
>>>> Here I wanted to suggest dropping all the non-trivial debugfs stuff and
>>>> just leave the info queried via i915_gem_get_aperture ioctl. So
>>>> effectively dropping the list traversal and vma sorting bits.
>>>>
>>> I think, debug info regarding the mappable space is good to have for
>>> debugging purpose as Chris mentioned.
>>> Also, the list traversal and the vma sorting stuff will only be called
>>> for debugging purposes, not slowing anything down or so.
>>
>> I am pretty indifferent on the topic of debugfs edition.
>>
>> But for the ioctl extension, how about adding a version field as the
>> first one in the extended area?
>
> A version number only makes sense when you are changing the meaning of
> an existing field. Adding one implies that we are planning to do so, are
> we?
>
> In the scenarios, I've run through I haven't found one where a caller
> would behave any differently faced with "0 - ioctl version not
> supported" and "0 - no available space (mappable/stolen)". Adding a
> version doesn't help using the new fields afaict. The argument is the
> same as whether a flags field is forward thinking or unthinkingly
> forward.

I was thinking that if 0 = no aperture or ioctl not supported userspace 
has to try one mmap_gtt to find out which is true, will it be ENODEV or 
ENOSPC (assuming, haven't checked). If we put a version in there then it 
can avoid doing that. Sounds like a better interface to me and I don't 
see any downsides to it.

Regards,

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

Re: ✗ Fi.CI.BAT: failure for Support for creating/using Stolen memory backed objects (rev13)

[Tvrtko Ursulin]

Hi Ankit,

On 20/04/16 17:28, Patchwork wrote:
> == Series Details ==
>
> Series: Support for creating/using Stolen memory backed objects (rev13)
> URL   : https://patchwork.freedesktop.org/series/659/
> State : failure
>
> == Summary ==
>
> Series 659v13 Support for creating/using Stolen memory backed objects
> http://patchwork.freedesktop.org/api/1.0/series/659/revisions/13/mbox/
>
> Test drv_module_reload_basic:
>                  fail       -> PASS       (snb-dellxps)
> Test gem_exec_basic:
>          Subgroup gtt-bsd:
>                  dmesg-warn -> PASS       (bsw-nuc-2)
> Test gem_pread:
>          Subgroup basic:
>                  pass       -> FAIL       (snb-dellxps)
> Test gem_pwrite:
>          Subgroup basic:
>                  pass       -> FAIL       (snb-dellxps)

Have you looked into these failures?

Regards,

Tvrtko

> Test kms_force_connector_basic:
>          Subgroup force-edid:
>                  pass       -> SKIP       (ivb-t430s)
>
> bdw-nuci7        total:192  pass:180  dwarn:0   dfail:0   fail:0   skip:12
> bdw-ultra        total:192  pass:169  dwarn:0   dfail:0   fail:0   skip:23
> bsw-nuc-2        total:191  pass:152  dwarn:0   dfail:0   fail:0   skip:39
> byt-nuc          total:191  pass:153  dwarn:0   dfail:0   fail:0   skip:38
> hsw-brixbox      total:192  pass:168  dwarn:0   dfail:0   fail:0   skip:24
> hsw-gt2          total:192  pass:173  dwarn:0   dfail:0   fail:0   skip:19
> ilk-hp8440p      total:192  pass:135  dwarn:0   dfail:0   fail:0   skip:57
> ivb-t430s        total:192  pass:163  dwarn:0   dfail:0   fail:0   skip:29
> skl-i7k-2        total:192  pass:167  dwarn:0   dfail:0   fail:0   skip:25
> skl-nuci5        total:192  pass:181  dwarn:0   dfail:0   fail:0   skip:11
> snb-dellxps      total:192  pass:152  dwarn:0   dfail:0   fail:2   skip:38
> snb-x220t        total:192  pass:154  dwarn:0   dfail:0   fail:1   skip:37
>
> Results at /archive/results/CI_IGT_test/Patchwork_1956/
>
> 5f13745078ab86d9833a181980afc7888157a9a4 drm-intel-nightly: 2016y-04m-20d-13h-46m-48s UTC integration manifest
> 568830a drm/i915: Support for creating Stolen memory backed objects
> 3b1dd9f drm/i915: Clearing buffer objects via CPU/GTT
> 6dd9ff9 drm/i915: Use insert_page for pwrite_fast
> 8208f24 drm/i915: Introduce i915_gem_object_get_dma_address()
> 0eeca57 drm/i915: Add support for mapping an object page by page
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/12] drm/i915: Extend GET_APERTURE ioctl to report available map space

[Chris Wilson]

On Thu, Apr 28, 2016 at 10:30:32AM +0100, Tvrtko Ursulin wrote:
> 
> On 26/04/16 10:44, Chris Wilson wrote:
> >On Mon, Apr 25, 2016 at 03:51:09PM +0100, Tvrtko Ursulin wrote:
> >>
> >>On 25/04/16 11:35, Ankitprasad Sharma wrote:
> >>>On Thu, 2016-04-21 at 15:59 +0100, Tvrtko Ursulin wrote:
> >>>>On 21/04/16 15:46, Chris Wilson wrote:
> >>>>>On Thu, Apr 21, 2016 at 03:04:52PM +0100, Tvrtko Ursulin wrote:
> >>>>>>
> >>>>>>Hi,
> >>>>>>
> >>>>>>On 20/04/16 12:17, ankitprasad.r.sharma@intel.com wrote:
> >>>>>>>+	mutex_unlock(&dev->struct_mutex);
> >>>>>>>+
> >>>>>>>+	seq_printf(m, "Total size of the GTT: %llu bytes\n",
> >>>>>>>+		   arg.aper_size);
> >>>>>>>+	seq_printf(m, "Available space in the GTT: %llu bytes\n",
> >>>>>>>+		   arg.aper_available_size);
> >>>>>>>+	seq_printf(m, "Total space in the mappable aperture: %llu bytes\n",
> >>>>>>>+		   arg.map_total_size);
> >>>>>>>+	seq_printf(m, "Available space in the mappable aperture: %llu bytes\n",
> >>>>>>>+		   map_space);
> >>>>>>>+	seq_printf(m, "Single largest space in the mappable aperture: %llu bytes\n",
> >>>>>>>+		   map_largest);
> >>>>>>>+	seq_printf(m, "Available space for fences: %llu bytes\n",
> >>>>>>>+		   fence_space);
> >>>>>>>+	seq_printf(m, "Single largest fence available: %llu bytes\n",
> >>>>>>>+		   fence_largest);
> >>>>>>>+
> >>>>>>>+	return 0;
> >>>>>>>+}
> >>>>>>>+
> >>>>>>
> >>>>>>In general I find this a lot of code for a feature of questionable
> >>>>>>utility. As such I would prefer someone really stated the need for
> >>>>>>this and explained how it really is useful - even though whetever
> >>>>>>number they get from this may be completely irrelevant by the time
> >>>>>>it is acted upon.
> >>>>>
> >>>>>Yes, with the exception of the size of the mappable aperture, this is
> >>>>>really is debug info. It will get automatically dumped by userspace
> >>>>>when it sees an ENOSPC, and that may prove enough to solve the riddle of
> >>>>>why it failed. However, this information is terrible outdated and now
> >>>>>longer of such relevance.
> >>>>>
> >>>>>As for the mappable aperture size, there has been a request many years
> >>>>>ago! could we provide it without resorting to a privilege operation. I
> >>>>>guess by know that request has died out - but there is still the issue
> >>>>>with libpciassess that make it unsuitable for use inside a library where
> >>>>>one may want to avoid it and use a simple ioctl on the device you
> >>>>>already have open.
> >>>>>
> >>>>>Yes, it is meh.
> >>>>
> >>>>Aperture size in the ioctl is fine I think, just that detection caveat
> >>>>what I asked in the other reply.
> >>>>
> >>>>Here I wanted to suggest dropping all the non-trivial debugfs stuff and
> >>>>just leave the info queried via i915_gem_get_aperture ioctl. So
> >>>>effectively dropping the list traversal and vma sorting bits.
> >>>>
> >>>I think, debug info regarding the mappable space is good to have for
> >>>debugging purpose as Chris mentioned.
> >>>Also, the list traversal and the vma sorting stuff will only be called
> >>>for debugging purposes, not slowing anything down or so.
> >>
> >>I am pretty indifferent on the topic of debugfs edition.
> >>
> >>But for the ioctl extension, how about adding a version field as the
> >>first one in the extended area?
> >
> >A version number only makes sense when you are changing the meaning of
> >an existing field. Adding one implies that we are planning to do so, are
> >we?
> >
> >In the scenarios, I've run through I haven't found one where a caller
> >would behave any differently faced with "0 - ioctl version not
> >supported" and "0 - no available space (mappable/stolen)". Adding a
> >version doesn't help using the new fields afaict. The argument is the
> >same as whether a flags field is forward thinking or unthinkingly
> >forward.
> 
> I was thinking that if 0 = no aperture or ioctl not supported
> userspace has to try one mmap_gtt to find out which is true, will it
> be ENODEV or ENOSPC (assuming, haven't checked). If we put a version
> in there then it can avoid doing that. Sounds like a better
> interface to me and I don't see any downsides to it.

I was thinking either userspace already cares and has a method for
finding the size of the PCI memory region or it doesn't. If it doesn't
and the ioctl reports 0 and its older second method fails with EPERM/EACCESS
then it is no worse off than before.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/12] drm/i915: Extend GET_APERTURE ioctl to report available map space

[Tvrtko Ursulin]

On 28/04/16 11:24, Chris Wilson wrote:
> On Thu, Apr 28, 2016 at 10:30:32AM +0100, Tvrtko Ursulin wrote:
>>
>> On 26/04/16 10:44, Chris Wilson wrote:
>>> On Mon, Apr 25, 2016 at 03:51:09PM +0100, Tvrtko Ursulin wrote:
>>>>
>>>> On 25/04/16 11:35, Ankitprasad Sharma wrote:
>>>>> On Thu, 2016-04-21 at 15:59 +0100, Tvrtko Ursulin wrote:
>>>>>> On 21/04/16 15:46, Chris Wilson wrote:
>>>>>>> On Thu, Apr 21, 2016 at 03:04:52PM +0100, Tvrtko Ursulin wrote:
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 20/04/16 12:17, ankitprasad.r.sharma@intel.com wrote:
>>>>>>>>> +	mutex_unlock(&dev->struct_mutex);
>>>>>>>>> +
>>>>>>>>> +	seq_printf(m, "Total size of the GTT: %llu bytes\n",
>>>>>>>>> +		   arg.aper_size);
>>>>>>>>> +	seq_printf(m, "Available space in the GTT: %llu bytes\n",
>>>>>>>>> +		   arg.aper_available_size);
>>>>>>>>> +	seq_printf(m, "Total space in the mappable aperture: %llu bytes\n",
>>>>>>>>> +		   arg.map_total_size);
>>>>>>>>> +	seq_printf(m, "Available space in the mappable aperture: %llu bytes\n",
>>>>>>>>> +		   map_space);
>>>>>>>>> +	seq_printf(m, "Single largest space in the mappable aperture: %llu bytes\n",
>>>>>>>>> +		   map_largest);
>>>>>>>>> +	seq_printf(m, "Available space for fences: %llu bytes\n",
>>>>>>>>> +		   fence_space);
>>>>>>>>> +	seq_printf(m, "Single largest fence available: %llu bytes\n",
>>>>>>>>> +		   fence_largest);
>>>>>>>>> +
>>>>>>>>> +	return 0;
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>
>>>>>>>> In general I find this a lot of code for a feature of questionable
>>>>>>>> utility. As such I would prefer someone really stated the need for
>>>>>>>> this and explained how it really is useful - even though whetever
>>>>>>>> number they get from this may be completely irrelevant by the time
>>>>>>>> it is acted upon.
>>>>>>>
>>>>>>> Yes, with the exception of the size of the mappable aperture, this is
>>>>>>> really is debug info. It will get automatically dumped by userspace
>>>>>>> when it sees an ENOSPC, and that may prove enough to solve the riddle of
>>>>>>> why it failed. However, this information is terrible outdated and now
>>>>>>> longer of such relevance.
>>>>>>>
>>>>>>> As for the mappable aperture size, there has been a request many years
>>>>>>> ago! could we provide it without resorting to a privilege operation. I
>>>>>>> guess by know that request has died out - but there is still the issue
>>>>>>> with libpciassess that make it unsuitable for use inside a library where
>>>>>>> one may want to avoid it and use a simple ioctl on the device you
>>>>>>> already have open.
>>>>>>>
>>>>>>> Yes, it is meh.
>>>>>>
>>>>>> Aperture size in the ioctl is fine I think, just that detection caveat
>>>>>> what I asked in the other reply.
>>>>>>
>>>>>> Here I wanted to suggest dropping all the non-trivial debugfs stuff and
>>>>>> just leave the info queried via i915_gem_get_aperture ioctl. So
>>>>>> effectively dropping the list traversal and vma sorting bits.
>>>>>>
>>>>> I think, debug info regarding the mappable space is good to have for
>>>>> debugging purpose as Chris mentioned.
>>>>> Also, the list traversal and the vma sorting stuff will only be called
>>>>> for debugging purposes, not slowing anything down or so.
>>>>
>>>> I am pretty indifferent on the topic of debugfs edition.
>>>>
>>>> But for the ioctl extension, how about adding a version field as the
>>>> first one in the extended area?
>>>
>>> A version number only makes sense when you are changing the meaning of
>>> an existing field. Adding one implies that we are planning to do so, are
>>> we?
>>>
>>> In the scenarios, I've run through I haven't found one where a caller
>>> would behave any differently faced with "0 - ioctl version not
>>> supported" and "0 - no available space (mappable/stolen)". Adding a
>>> version doesn't help using the new fields afaict. The argument is the
>>> same as whether a flags field is forward thinking or unthinkingly
>>> forward.
>>
>> I was thinking that if 0 = no aperture or ioctl not supported
>> userspace has to try one mmap_gtt to find out which is true, will it
>> be ENODEV or ENOSPC (assuming, haven't checked). If we put a version
>> in there then it can avoid doing that. Sounds like a better
>> interface to me and I don't see any downsides to it.
>
> I was thinking either userspace already cares and has a method for
> finding the size of the PCI memory region or it doesn't. If it doesn't
> and the ioctl reports 0 and its older second method fails with EPERM/EACCESS
> then it is no worse off than before.

I don't get it - if we are adding something why not add it in a way that 
makes it clear and self-contained - what is the downside of what I 
propose to meet such resistance?

Regards,

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

Re: [PATCH 11/12] drm/i915: Extend GET_APERTURE ioctl to report available map space

[Chris Wilson]

On Fri, Apr 29, 2016 at 11:06:49AM +0100, Tvrtko Ursulin wrote:
> I don't get it - if we are adding something why not add it in a way
> that makes it clear and self-contained - what is the downside of
> what I propose to meet such resistance?

You're suggesting to add a field I'm not going to use. Is any one?
Until there is an actual use (which would afaict be if one of the
existing values changed meaning, which itself would be an abi break) I'm
not convinced we should be designing for the unknowable. If we did need
to version would we not be better off with a new ioctl? 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/12] drm/i915: Extend GET_APERTURE ioctl to report available map space

[Tvrtko Ursulin]

On 29/04/16 11:18, Chris Wilson wrote:
> On Fri, Apr 29, 2016 at 11:06:49AM +0100, Tvrtko Ursulin wrote:
>> I don't get it - if we are adding something why not add it in a way
>> that makes it clear and self-contained - what is the downside of
>> what I propose to meet such resistance?
>
> You're suggesting to add a field I'm not going to use. Is any one?
> Until there is an actual use (which would afaict be if one of the
> existing values changed meaning, which itself would be an abi break) I'm
> not convinced we should be designing for the unknowable. If we did need
> to version would we not be better off with a new ioctl?

I am suggesting if we are adding aper_total_size, or whatever it is 
called, to make it usable in an self-contained matter.

My impression was you want aper_total_size so I was simply objecting to 
adding it without being able to directly tell if kernel actually 
supports that extension.

Regards,

Tvrtko

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

Re: [PATCH 11/12] drm/i915: Extend GET_APERTURE ioctl to report available map space

[Chris Wilson]

On Fri, Apr 29, 2016 at 11:26:45AM +0100, Tvrtko Ursulin wrote:
> 
> On 29/04/16 11:18, Chris Wilson wrote:
> >On Fri, Apr 29, 2016 at 11:06:49AM +0100, Tvrtko Ursulin wrote:
> >>I don't get it - if we are adding something why not add it in a way
> >>that makes it clear and self-contained - what is the downside of
> >>what I propose to meet such resistance?
> >
> >You're suggesting to add a field I'm not going to use. Is any one?
> >Until there is an actual use (which would afaict be if one of the
> >existing values changed meaning, which itself would be an abi break) I'm
> >not convinced we should be designing for the unknowable. If we did need
> >to version would we not be better off with a new ioctl?
> 
> I am suggesting if we are adding aper_total_size, or whatever it is
> called, to make it usable in an self-contained matter.
> 
> My impression was you want aper_total_size so I was simply objecting
> to adding it without being able to directly tell if kernel actually
> supports that extension.

The two additions that I thought we have reduced it to:

	u64 mappable_aperture_size
	u64 stolen_size;

Of which I agree that the first has some ambiguity over 0. But I don't
think it makes a difference in practice. I for one would not bother with
checking a version. I already have a cascade here to deal with not being
able to use various probes, with an eventual fallback to a safe value.

We know the mappable_aperture_size cannot be zero with the exception
that the device is disabled - but we have opened the device already.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/12] drm/i915: Extend GET_APERTURE ioctl to report available map space

[Tvrtko Ursulin]

On 29/04/16 11:39, Chris Wilson wrote:
> On Fri, Apr 29, 2016 at 11:26:45AM +0100, Tvrtko Ursulin wrote:
>>
>> On 29/04/16 11:18, Chris Wilson wrote:
>>> On Fri, Apr 29, 2016 at 11:06:49AM +0100, Tvrtko Ursulin wrote:
>>>> I don't get it - if we are adding something why not add it in a way
>>>> that makes it clear and self-contained - what is the downside of
>>>> what I propose to meet such resistance?
>>>
>>> You're suggesting to add a field I'm not going to use. Is any one?
>>> Until there is an actual use (which would afaict be if one of the
>>> existing values changed meaning, which itself would be an abi break) I'm
>>> not convinced we should be designing for the unknowable. If we did need
>>> to version would we not be better off with a new ioctl?
>>
>> I am suggesting if we are adding aper_total_size, or whatever it is
>> called, to make it usable in an self-contained matter.
>>
>> My impression was you want aper_total_size so I was simply objecting
>> to adding it without being able to directly tell if kernel actually
>> supports that extension.
>
> The two additions that I thought we have reduced it to:
>
> 	u64 mappable_aperture_size
> 	u64 stolen_size;
>
> Of which I agree that the first has some ambiguity over 0. But I don't
> think it makes a difference in practice. I for one would not bother with
> checking a version. I already have a cascade here to deal with not being
> able to use various probes, with an eventual fallback to a safe value.
>
> We know the mappable_aperture_size cannot be zero with the exception
> that the device is disabled - but we have opened the device already.

Since you agree there is ambiguity lets just add a version. You don't 
have to use it, but someone will.

	u32 version; // == 1
  	u64 mappable_aperture_size;

And then it is clear and self-contained.

Regards,

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

Re: [PATCH 11/12] drm/i915: Extend GET_APERTURE ioctl to report available map space

[Chris Wilson]

On Fri, Apr 29, 2016 at 11:56:28AM +0100, Tvrtko Ursulin wrote:
> 
> On 29/04/16 11:39, Chris Wilson wrote:
> >On Fri, Apr 29, 2016 at 11:26:45AM +0100, Tvrtko Ursulin wrote:
> >>
> >>On 29/04/16 11:18, Chris Wilson wrote:
> >>>On Fri, Apr 29, 2016 at 11:06:49AM +0100, Tvrtko Ursulin wrote:
> >>>>I don't get it - if we are adding something why not add it in a way
> >>>>that makes it clear and self-contained - what is the downside of
> >>>>what I propose to meet such resistance?
> >>>
> >>>You're suggesting to add a field I'm not going to use. Is any one?
> >>>Until there is an actual use (which would afaict be if one of the
> >>>existing values changed meaning, which itself would be an abi break) I'm
> >>>not convinced we should be designing for the unknowable. If we did need
> >>>to version would we not be better off with a new ioctl?
> >>
> >>I am suggesting if we are adding aper_total_size, or whatever it is
> >>called, to make it usable in an self-contained matter.
> >>
> >>My impression was you want aper_total_size so I was simply objecting
> >>to adding it without being able to directly tell if kernel actually
> >>supports that extension.
> >
> >The two additions that I thought we have reduced it to:
> >
> >	u64 mappable_aperture_size
> >	u64 stolen_size;
> >
> >Of which I agree that the first has some ambiguity over 0. But I don't
> >think it makes a difference in practice. I for one would not bother with
> >checking a version. I already have a cascade here to deal with not being
> >able to use various probes, with an eventual fallback to a safe value.
> >
> >We know the mappable_aperture_size cannot be zero with the exception
> >that the device is disabled - but we have opened the device already.
> 
> Since you agree there is ambiguity lets just add a version. You
> don't have to use it, but someone will.
> 
> 	u32 version; // == 1
>  	u64 mappable_aperture_size;
> 
> And then it is clear and self-contained.

	u64 version;

or

	u32 version;
	u32 flags;

uABI fields need to be naturally aligned.

Hah, you didn't mention the trump card you pulled on IRC. In light of
that issue ever being a problem (that we may be faced with 0 aperture),
yes.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/12] drm/i915: Add support for mapping an object page by page

[Ankitprasad Sharma]

On Wed, 2016-04-20 at 13:04 +0100, Chris Wilson wrote:
> On Wed, Apr 20, 2016 at 04:47:28PM +0530, ankitprasad.r.sharma@intel.com wrote:
> > From: Chris Wilson <chris@chris-wilson.co.uk>
> > 
> > Introduced a new vm specfic callback insert_page() to program a single pte in
> > ggtt or ppgtt. This allows us to map a single page in to the mappable aperture
> > space. This can be iterated over to access the whole object by using space as
> > meagre as page size.
> > 
> > v2: Added low level rpm assertions to insert_page routines (Chris)
> > 
> > v3: Added POSTING_READ post register write (Tvrtko)
> > 
> > v4: Rebase (Ankit)
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
> > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >  drivers/char/agp/intel-gtt.c        |  9 +++++
> >  drivers/gpu/drm/i915/i915_gem_gtt.c | 67 +++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/i915_gem_gtt.h |  5 +++
> >  include/drm/intel-gtt.h             |  3 ++
> >  4 files changed, 84 insertions(+)
> > 
> > diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
> > index aef87fd..cea0ae3 100644
> > --- a/drivers/char/agp/intel-gtt.c
> > +++ b/drivers/char/agp/intel-gtt.c
> > @@ -840,6 +840,15 @@ static bool i830_check_flags(unsigned int flags)
> >  	return false;
> >  }
> >  
> > +void intel_gtt_insert_page(dma_addr_t addr,
> > +			   unsigned int pg,
> > +			   unsigned int flags)
> > +{
> > +	intel_private.driver->write_entry(addr, pg, flags);
> > +	wmb();
> > +}
> > +EXPORT_SYMBOL(intel_gtt_insert_page);
> > +
> >  void intel_gtt_insert_sg_entries(struct sg_table *st,
> >  				 unsigned int pg_start,
> >  				 unsigned int flags)
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 9f165fe..da1819d 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -2353,6 +2353,29 @@ static void gen8_set_pte(void __iomem *addr, gen8_pte_t pte)
> >  #endif
> >  }
> >  
> > +static void gen8_ggtt_insert_page(struct i915_address_space *vm,
> > +				  dma_addr_t addr,
> > +				  uint64_t offset,
> > +				  enum i915_cache_level level,
> > +				  u32 unused)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(vm->dev);
> > +	gen8_pte_t __iomem *pte =
> > +		(gen8_pte_t __iomem *)dev_priv->ggtt.gsm +
> > +		(offset >> PAGE_SHIFT);
> > +	int rpm_atomic_seq;
> > +
> > +	rpm_atomic_seq = assert_rpm_atomic_begin(dev_priv);
> > +
> > +	gen8_set_pte(pte, gen8_pte_encode(addr, level, true));
> > +	wmb();
> > +
> > +	I915_WRITE(GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
> > +	POSTING_READ(GFX_FLSH_CNTL_GEN6);
> 
> Actually, we don't need these at all for the insert-page API (current
> users in pread/pwrite/reloc/gpu-error-capture) as they go through the
> GTT and not the GPU TLB. I would much rather we punt these to the caller
> with a vm->flush() which we can add later. When I say later, we already
> have the GUC pretending to be the GGTT and adding code where it doesn't
> belong...

I was trying to test the stolen memory patches on SKL with the removed
FLSH_CNTL write. Apparently, the insert_page routine does not seem to be
working. As I was verifying if the stolen-backed object is zeroed out
(using i915_gem_object_clear, which makes use of insert_page) on
creation but it failed (I was getting a garbage value instead of zeros).
And on adding this write it works as expected.

I think this write cannot be removed after all.

Thanks,
Ankit
> 
> All we need here is the wmb() to order the PTE write (and flush the WCB)
> with the use afterwards. The caller has to provide the wmb() to order
> its writes before a subsequent PTE change.
> 
> In a few patches time we can also kill the rpm_atomic asserts and the
> dev_priv local.
> -Chris
> 


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