* [PATCH 1/6] drm/i915: Clearing buffer objects via CPU/GTT
2015-10-08 6:24 [PATCH v8 0/6] Support for creating/using Stolen memory backed objects ankitprasad.r.sharma
@ 2015-10-08 6:24 ` ankitprasad.r.sharma
2015-10-08 6:24 ` [PATCH 2/6] drm/i915: Support for creating Stolen memory backed objects ankitprasad.r.sharma
` (4 subsequent siblings)
5 siblings, 0 replies; 36+ messages in thread
From: ankitprasad.r.sharma @ 2015-10-08 6:24 UTC (permalink / raw)
To: intel-gfx; +Cc: Ankitprasad Sharma, akash.goel, shashidhar.hiremath
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)
Testcase: igt/gem_stolen
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 | 1 +
drivers/gpu/drm/i915/i915_gem.c | 39 +++++++++++++++++++++++++++++++++++++++
2 files changed, 40 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2b5d587..f962425 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2848,6 +2848,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_clear_object(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 bf5ef7a..e6b3bc7 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5143,3 +5143,42 @@ fail:
drm_gem_object_unreference(&obj->base);
return ERR_PTR(ret);
}
+
+/**
+ * i915_gem_clear_object() - Clear buffer object via CPU/GTT
+ * @obj: Buffer object to be cleared
+ *
+ * Return: 0 - success, non-zero - failure
+ */
+int i915_gem_clear_object(struct drm_i915_gem_object *obj)
+{
+ int ret;
+ char __iomem *base;
+ size_t size = obj->base.size;
+ struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
+
+ WARN_ON(!mutex_is_locked(&obj->base.dev->struct_mutex));
+ ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
+ if (ret)
+ return ret;
+
+ ret = i915_gem_object_put_fence(obj);
+ if (ret)
+ goto unpin;
+
+ /* Get the CPU virtual address of the buffer */
+ base = ioremap_wc(dev_priv->gtt.mappable_base +
+ i915_gem_obj_ggtt_offset(obj), size);
+ if (base == NULL) {
+ DRM_ERROR("Mapping of gem object to CPU failed!\n");
+ ret = -ENOSPC;
+ goto unpin;
+ }
+
+ memset_io(base, 0, size);
+
+ iounmap(base);
+unpin:
+ i915_gem_object_ggtt_unpin(obj);
+ return ret;
+}
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH 2/6] drm/i915: Support for creating Stolen memory backed objects
2015-10-08 6:24 [PATCH v8 0/6] Support for creating/using Stolen memory backed objects ankitprasad.r.sharma
2015-10-08 6:24 ` [PATCH 1/6] drm/i915: Clearing buffer objects via CPU/GTT ankitprasad.r.sharma
@ 2015-10-08 6:24 ` ankitprasad.r.sharma
2015-10-08 6:24 ` [PATCH 3/6] drm/i915: Propagating correct error codes to the userspace ankitprasad.r.sharma
` (3 subsequent siblings)
5 siblings, 0 replies; 36+ messages in thread
From: ankitprasad.r.sharma @ 2015-10-08 6:24 UTC (permalink / raw)
To: intel-gfx; +Cc: Ankitprasad Sharma, akash.goel, shashidhar.hiremath
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)
Testcase: igt/gem_stolen
Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/i915_dma.c | 3 +++
drivers/gpu/drm/i915/i915_drv.h | 2 +-
drivers/gpu/drm/i915/i915_gem.c | 30 +++++++++++++++++++++++++++---
drivers/gpu/drm/i915/i915_gem_stolen.c | 4 ++--
include/uapi/drm/i915_drm.h | 16 ++++++++++++++++
5 files changed, 49 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 817b05c..0a25f23 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -170,6 +170,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
case I915_PARAM_HAS_RESOURCE_STREAMER:
value = HAS_RESOURCE_STREAMER(dev);
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 f962425..bca1572 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3191,7 +3191,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 e6b3bc7..d3f4321 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -375,6 +375,7 @@ static int
i915_gem_create(struct drm_file *file,
struct drm_device *dev,
uint64_t size,
+ uint32_t flags,
uint32_t *handle_p)
{
struct drm_i915_gem_object *obj;
@@ -385,8 +386,31 @@ 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);
+ if (flags & I915_CREATE_PLACEMENT_STOLEN) {
+ mutex_lock(&dev->struct_mutex);
+ obj = i915_gem_object_create_stolen(dev, size);
+ if (!obj) {
+ mutex_unlock(&dev->struct_mutex);
+ return -ENOMEM;
+ }
+
+ /* Always clear fresh buffers before handing to userspace */
+ ret = i915_gem_clear_object(obj);
+ if (ret) {
+ drm_gem_object_unreference(&obj->base);
+ mutex_unlock(&dev->struct_mutex);
+ return ret;
+ }
+
+ mutex_unlock(&dev->struct_mutex);
+ } else {
+ obj = i915_gem_alloc_object(dev, size);
+ }
+
if (obj == NULL)
return -ENOMEM;
@@ -409,7 +433,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);
}
/**
@@ -422,7 +446,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 1520779..5ec2190 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -497,7 +497,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;
@@ -507,7 +507,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 fd5aa47..ee60459 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -356,6 +356,7 @@ typedef struct drm_i915_irq_wait {
#define I915_PARAM_EU_TOTAL 34
#define I915_PARAM_HAS_GPU_RESET 35
#define I915_PARAM_HAS_RESOURCE_STREAMER 36
+#define I915_PARAM_CREATE_VERSION 37
typedef struct drm_i915_getparam {
__s32 param;
@@ -455,6 +456,21 @@ 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
+ */
+ __u32 flags;
+#define I915_CREATE_PLACEMENT_STOLEN (1<<0) /* Cannot use CPU mmaps */
+#define __I915_CREATE_UNKNOWN_FLAGS -(I915_CREATE_PLACEMENT_STOLEN << 1)
};
struct drm_i915_gem_pread {
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH 3/6] drm/i915: Propagating correct error codes to the userspace
2015-10-08 6:24 [PATCH v8 0/6] Support for creating/using Stolen memory backed objects ankitprasad.r.sharma
2015-10-08 6:24 ` [PATCH 1/6] drm/i915: Clearing buffer objects via CPU/GTT ankitprasad.r.sharma
2015-10-08 6:24 ` [PATCH 2/6] drm/i915: Support for creating Stolen memory backed objects ankitprasad.r.sharma
@ 2015-10-08 6:24 ` ankitprasad.r.sharma
2015-10-08 10:22 ` Tvrtko Ursulin
2015-10-08 10:49 ` Chris Wilson
2015-10-08 6:24 ` [PATCH 4/6] drm/i915: Add support for stealing purgable stolen pages ankitprasad.r.sharma
` (2 subsequent siblings)
5 siblings, 2 replies; 36+ messages in thread
From: ankitprasad.r.sharma @ 2015-10-08 6:24 UTC (permalink / raw)
To: intel-gfx; +Cc: Ankitprasad Sharma, akash.goel, shashidhar.hiremath
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)
Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
---
drivers/gpu/drm/i915/i915_gem.c | 15 +++++-----
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 | 4 +--
drivers/gpu/drm/i915/i915_gem_stolen.c | 43 ++++++++++++++++------------
drivers/gpu/drm/i915/i915_guc_submission.c | 4 +--
drivers/gpu/drm/i915/intel_display.c | 2 +-
drivers/gpu/drm/i915/intel_fbdev.c | 6 ++--
drivers/gpu/drm/i915/intel_lrc.c | 8 +++---
drivers/gpu/drm/i915/intel_overlay.c | 4 +--
drivers/gpu/drm/i915/intel_pm.c | 2 +-
drivers/gpu/drm/i915/intel_ringbuffer.c | 20 ++++++-------
12 files changed, 61 insertions(+), 55 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d3f4321..54f7df1 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -393,9 +393,9 @@ i915_gem_create(struct drm_file *file,
if (flags & I915_CREATE_PLACEMENT_STOLEN) {
mutex_lock(&dev->struct_mutex);
obj = i915_gem_object_create_stolen(dev, size);
- if (!obj) {
+ if (IS_ERR(obj)) {
mutex_unlock(&dev->struct_mutex);
- return -ENOMEM;
+ return PTR_ERR(obj);
}
/* Always clear fresh buffers before handing to userspace */
@@ -411,8 +411,8 @@ i915_gem_create(struct drm_file *file,
obj = i915_gem_alloc_object(dev, size);
}
- 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 */
@@ -4273,14 +4273,15 @@ 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 = 0;
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) {
+ if ((ret = drm_gem_object_init(dev, &obj->base, size)) != 0) {
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 74aa0c9..603600c 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -157,8 +157,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 5026a62..50d010e 100644
--- a/drivers/gpu/drm/i915/i915_gem_render_state.c
+++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
@@ -58,8 +58,8 @@ 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))
+ return PTR_ERR(so->obj);
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 5ec2190..d3e6813 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -406,6 +406,7 @@ i915_pages_create_for_stolen(struct drm_device *dev,
struct drm_i915_private *dev_priv = dev->dev_private;
struct sg_table *st;
struct scatterlist *sg;
+ int ret;
DRM_DEBUG_DRIVER("offset=0x%x, size=%d\n", offset, size);
BUG_ON(offset > dev_priv->gtt.stolen_size - size);
@@ -417,11 +418,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;
@@ -470,18 +472,21 @@ _i915_gem_object_create_stolen(struct drm_device *dev,
struct drm_mm_node *stolen)
{
struct drm_i915_gem_object *obj;
+ int ret = 0;
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)
+ if (IS_ERR(obj->pages)) {
+ ret = PTR_ERR(obj->pages);
goto cleanup;
+ }
i915_gem_object_pin_pages(obj);
obj->stolen = stolen;
@@ -493,7 +498,7 @@ _i915_gem_object_create_stolen(struct drm_device *dev,
cleanup:
i915_gem_object_free(obj);
- return NULL;
+ return ERR_PTR(ret);
}
struct drm_i915_gem_object *
@@ -505,29 +510,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 *
@@ -544,7 +549,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);
DRM_DEBUG_KMS("creating preallocated stolen object: stolen_offset=%x, gtt_offset=%x, size=%x\n",
stolen_offset, gtt_offset, size);
@@ -552,11 +557,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;
@@ -566,15 +571,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 */
@@ -612,5 +617,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 792d0b9..3901698 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -646,8 +646,8 @@ static struct drm_i915_gem_object *gem_allocate_guc_obj(struct drm_device *dev,
struct drm_i915_gem_object *obj;
obj = i915_gem_alloc_object(dev, size);
- if (!obj)
- return NULL;
+ if (IS_ERR(obj))
+ return obj;
if (i915_gem_object_get_pages(obj)) {
drm_gem_object_unreference(&obj->base);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1847257..d08989a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2514,7 +2514,7 @@ intel_alloc_initial_plane_obj(struct intel_crtc *crtc,
base_aligned,
base_aligned,
size_aligned);
- if (!obj)
+ if (IS_ERR(obj))
return false;
obj->tiling_mode = plane_config->tiling;
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 6532912..67de958 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -140,11 +140,11 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
size = mode_cmd.pitches[0] * mode_cmd.height;
size = PAGE_ALIGN(size);
obj = i915_gem_object_create_stolen(dev, size);
- if (obj == NULL)
+ if (IS_ERR(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 256167b..d9bd9a1 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1377,9 +1377,9 @@ static int lrc_setup_wa_ctx_obj(struct intel_engine_cs *ring, u32 size)
int ret;
ring->wa_ctx.obj = i915_gem_alloc_object(ring->dev, PAGE_ALIGN(size));
- if (!ring->wa_ctx.obj) {
+ if (IS_ERR(ring->wa_ctx.obj)) {
DRM_DEBUG_DRIVER("alloc LRC WA ctx backing obj failed.\n");
- return -ENOMEM;
+ return PTR_ERR(ring->wa_ctx.obj);
}
ret = i915_gem_obj_ggtt_pin(ring->wa_ctx.obj, PAGE_SIZE, 0);
@@ -2459,9 +2459,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(ring, 4 * PAGE_SIZE);
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index 4445426..bfb95a2 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -1392,9 +1392,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 ab5ac5e..0d35d7f 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5387,7 +5387,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");
return;
}
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 16a4ead..a928602 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -676,9 +676,9 @@ intel_init_pipe_control(struct intel_engine_cs *ring)
WARN_ON(ring->scratch.obj);
ring->scratch.obj = i915_gem_alloc_object(ring->dev, 4096);
- if (ring->scratch.obj == NULL) {
+ if (IS_ERR(ring->scratch.obj)) {
DRM_ERROR("Failed to allocate seqno page\n");
- ret = -ENOMEM;
+ ret = PTR_ERR(ring->scratch.obj);
goto err;
}
@@ -1913,9 +1913,9 @@ static int init_status_page(struct intel_engine_cs *ring)
int ret;
obj = i915_gem_alloc_object(ring->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);
@@ -2020,10 +2020,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;
@@ -2607,7 +2607,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 {
@@ -2713,9 +2713,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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 3/6] drm/i915: Propagating correct error codes to the userspace
2015-10-08 6:24 ` [PATCH 3/6] drm/i915: Propagating correct error codes to the userspace ankitprasad.r.sharma
@ 2015-10-08 10:22 ` Tvrtko Ursulin
2015-10-09 15:13 ` Dave Gordon
2015-10-08 10:49 ` Chris Wilson
1 sibling, 1 reply; 36+ messages in thread
From: Tvrtko Ursulin @ 2015-10-08 10:22 UTC (permalink / raw)
To: ankitprasad.r.sharma, intel-gfx; +Cc: akash.goel, shashidhar.hiremath
Hi,
On 08/10/15 07:24, ankitprasad.r.sharma@intel.com wrote:
> 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)
>
> Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem.c | 15 +++++-----
> 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 | 4 +--
> drivers/gpu/drm/i915/i915_gem_stolen.c | 43 ++++++++++++++++------------
> drivers/gpu/drm/i915/i915_guc_submission.c | 4 +--
> drivers/gpu/drm/i915/intel_display.c | 2 +-
> drivers/gpu/drm/i915/intel_fbdev.c | 6 ++--
> drivers/gpu/drm/i915/intel_lrc.c | 8 +++---
> drivers/gpu/drm/i915/intel_overlay.c | 4 +--
> drivers/gpu/drm/i915/intel_pm.c | 2 +-
> drivers/gpu/drm/i915/intel_ringbuffer.c | 20 ++++++-------
> 12 files changed, 61 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index d3f4321..54f7df1 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -393,9 +393,9 @@ i915_gem_create(struct drm_file *file,
> if (flags & I915_CREATE_PLACEMENT_STOLEN) {
> mutex_lock(&dev->struct_mutex);
> obj = i915_gem_object_create_stolen(dev, size);
> - if (!obj) {
> + if (IS_ERR(obj)) {
> mutex_unlock(&dev->struct_mutex);
> - return -ENOMEM;
> + return PTR_ERR(obj);
> }
>
> /* Always clear fresh buffers before handing to userspace */
> @@ -411,8 +411,8 @@ i915_gem_create(struct drm_file *file,
> obj = i915_gem_alloc_object(dev, size);
> }
>
> - 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 */
> @@ -4273,14 +4273,15 @@ 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 = 0;
>
> 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) {
> + if ((ret = drm_gem_object_init(dev, &obj->base, size)) != 0) {
> 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 74aa0c9..603600c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -157,8 +157,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 5026a62..50d010e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_render_state.c
> +++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
> @@ -58,8 +58,8 @@ 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))
> + return PTR_ERR(so->obj);
>
> 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 5ec2190..d3e6813 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -406,6 +406,7 @@ i915_pages_create_for_stolen(struct drm_device *dev,
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct sg_table *st;
> struct scatterlist *sg;
> + int ret;
>
> DRM_DEBUG_DRIVER("offset=0x%x, size=%d\n", offset, size);
> BUG_ON(offset > dev_priv->gtt.stolen_size - size);
> @@ -417,11 +418,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;
> @@ -470,18 +472,21 @@ _i915_gem_object_create_stolen(struct drm_device *dev,
> struct drm_mm_node *stolen)
> {
> struct drm_i915_gem_object *obj;
> + int ret = 0;
>
> 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)
> + if (IS_ERR(obj->pages)) {
> + ret = PTR_ERR(obj->pages);
> goto cleanup;
> + }
>
> i915_gem_object_pin_pages(obj);
> obj->stolen = stolen;
> @@ -493,7 +498,7 @@ _i915_gem_object_create_stolen(struct drm_device *dev,
>
> cleanup:
> i915_gem_object_free(obj);
> - return NULL;
> + return ERR_PTR(ret);
> }
>
> struct drm_i915_gem_object *
> @@ -505,29 +510,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 *
> @@ -544,7 +549,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);
>
> DRM_DEBUG_KMS("creating preallocated stolen object: stolen_offset=%x, gtt_offset=%x, size=%x\n",
> stolen_offset, gtt_offset, size);
> @@ -552,11 +557,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;
> @@ -566,15 +571,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 */
> @@ -612,5 +617,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 792d0b9..3901698 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -646,8 +646,8 @@ static struct drm_i915_gem_object *gem_allocate_guc_obj(struct drm_device *dev,
> struct drm_i915_gem_object *obj;
>
> obj = i915_gem_alloc_object(dev, size);
> - if (!obj)
> - return NULL;
> + if (IS_ERR(obj))
> + return obj;
You have to change kerneldoc for this function now.
And even more importantly update error handling in the callers!
The rest looks fine to me.
One possible class of improvements could be, now that error codes are
not swallowed, to print them out in all the existing DRM_DEBUG_ logging
after the relevant call sites. Up to you.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 3/6] drm/i915: Propagating correct error codes to the userspace
2015-10-08 10:22 ` Tvrtko Ursulin
@ 2015-10-09 15:13 ` Dave Gordon
0 siblings, 0 replies; 36+ messages in thread
From: Dave Gordon @ 2015-10-09 15:13 UTC (permalink / raw)
To: intel-gfx
On 08/10/15 11:22, Tvrtko Ursulin wrote:
>
> Hi,
>
> On 08/10/15 07:24, ankitprasad.r.sharma@intel.com wrote:
>> 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)
>>
>> Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_gem.c | 15 +++++-----
>> 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 | 4 +--
>> drivers/gpu/drm/i915/i915_gem_stolen.c | 43
>> ++++++++++++++++------------
>> drivers/gpu/drm/i915/i915_guc_submission.c | 4 +--
>> drivers/gpu/drm/i915/intel_display.c | 2 +-
>> drivers/gpu/drm/i915/intel_fbdev.c | 6 ++--
>> drivers/gpu/drm/i915/intel_lrc.c | 8 +++---
>> drivers/gpu/drm/i915/intel_overlay.c | 4 +--
>> drivers/gpu/drm/i915/intel_pm.c | 2 +-
>> drivers/gpu/drm/i915/intel_ringbuffer.c | 20 ++++++-------
>> 12 files changed, 61 insertions(+), 55 deletions(-)
[snip]
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index 792d0b9..3901698 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -646,8 +646,8 @@ static struct drm_i915_gem_object
>> *gem_allocate_guc_obj(struct drm_device *dev,
>> struct drm_i915_gem_object *obj;
>>
>> obj = i915_gem_alloc_object(dev, size);
>> - if (!obj)
>> - return NULL;
>> + if (IS_ERR(obj))
>> + return obj;
>
> You have to change kerneldoc for this function now.
> And even more importantly update error handling in the callers!
Or more simply, stop propagating the specific error at this level (it
doesn't really help and probably can't even happen here), making this:
>> obj = i915_gem_alloc_object(dev, size);
>> - if (!obj)
>> + if (IS_ERR(obj))
leaving the "return NULL;" unchanged.
.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 3/6] drm/i915: Propagating correct error codes to the userspace
2015-10-08 6:24 ` [PATCH 3/6] drm/i915: Propagating correct error codes to the userspace ankitprasad.r.sharma
2015-10-08 10:22 ` Tvrtko Ursulin
@ 2015-10-08 10:49 ` Chris Wilson
1 sibling, 0 replies; 36+ messages in thread
From: Chris Wilson @ 2015-10-08 10:49 UTC (permalink / raw)
To: ankitprasad.r.sharma; +Cc: intel-gfx, akash.goel, shashidhar.hiremath
On Thu, Oct 08, 2015 at 11:54:26AM +0530, ankitprasad.r.sharma@intel.com wrote:
> 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)
>
> Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem.c | 15 +++++-----
> 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 | 4 +--
> drivers/gpu/drm/i915/i915_gem_stolen.c | 43 ++++++++++++++++------------
> drivers/gpu/drm/i915/i915_guc_submission.c | 4 +--
> drivers/gpu/drm/i915/intel_display.c | 2 +-
> drivers/gpu/drm/i915/intel_fbdev.c | 6 ++--
> drivers/gpu/drm/i915/intel_lrc.c | 8 +++---
> drivers/gpu/drm/i915/intel_overlay.c | 4 +--
> drivers/gpu/drm/i915/intel_pm.c | 2 +-
> drivers/gpu/drm/i915/intel_ringbuffer.c | 20 ++++++-------
> 12 files changed, 61 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index d3f4321..54f7df1 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4273,14 +4273,15 @@ 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 = 0;
Pointless initialisation.
>
> 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) {
> + if ((ret = drm_gem_object_init(dev, &obj->base, size)) != 0) {
> 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 74aa0c9..603600c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -157,8 +157,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 5026a62..50d010e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_render_state.c
> +++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
> @@ -58,8 +58,8 @@ 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))
> + return PTR_ERR(so->obj);
>
> 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 5ec2190..d3e6813 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -406,6 +406,7 @@ i915_pages_create_for_stolen(struct drm_device *dev,
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct sg_table *st;
> struct scatterlist *sg;
> + int ret;
>
> DRM_DEBUG_DRIVER("offset=0x%x, size=%d\n", offset, size);
> BUG_ON(offset > dev_priv->gtt.stolen_size - size);
> @@ -417,11 +418,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;
> @@ -470,18 +472,21 @@ _i915_gem_object_create_stolen(struct drm_device *dev,
> struct drm_mm_node *stolen)
> {
> struct drm_i915_gem_object *obj;
> + int ret = 0;
>
> 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)
> + if (IS_ERR(obj->pages)) {
> + ret = PTR_ERR(obj->pages);
> goto cleanup;
> + }
>
> i915_gem_object_pin_pages(obj);
> obj->stolen = stolen;
> @@ -493,7 +498,7 @@ _i915_gem_object_create_stolen(struct drm_device *dev,
>
> cleanup:
> i915_gem_object_free(obj);
> - return NULL;
> + return ERR_PTR(ret);
> }
>
> struct drm_i915_gem_object *
> @@ -505,29 +510,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 *
> @@ -544,7 +549,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);
>
> DRM_DEBUG_KMS("creating preallocated stolen object: stolen_offset=%x, gtt_offset=%x, size=%x\n",
> stolen_offset, gtt_offset, size);
> @@ -552,11 +557,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;
> @@ -566,15 +571,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 */
> @@ -612,5 +617,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 792d0b9..3901698 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -646,8 +646,8 @@ static struct drm_i915_gem_object *gem_allocate_guc_obj(struct drm_device *dev,
> struct drm_i915_gem_object *obj;
>
> obj = i915_gem_alloc_object(dev, size);
> - if (!obj)
> - return NULL;
> + if (IS_ERR(obj))
> + return obj;
Did you check the caller?
>
> if (i915_gem_object_get_pages(obj)) {
> drm_gem_object_unreference(&obj->base);
New error code return here.
The call to get_pages() is redundant anyway.
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1847257..d08989a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2514,7 +2514,7 @@ intel_alloc_initial_plane_obj(struct intel_crtc *crtc,
> base_aligned,
> base_aligned,
> size_aligned);
> - if (!obj)
> + if (IS_ERR(obj))
> return false;
>
> obj->tiling_mode = plane_config->tiling;
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 6532912..67de958 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -140,11 +140,11 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
> size = mode_cmd.pitches[0] * mode_cmd.height;
> size = PAGE_ALIGN(size);
> obj = i915_gem_object_create_stolen(dev, size);
> - if (obj == NULL)
> + if (IS_ERR(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 256167b..d9bd9a1 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1377,9 +1377,9 @@ static int lrc_setup_wa_ctx_obj(struct intel_engine_cs *ring, u32 size)
> int ret;
>
> ring->wa_ctx.obj = i915_gem_alloc_object(ring->dev, PAGE_ALIGN(size));
> - if (!ring->wa_ctx.obj) {
> + if (IS_ERR(ring->wa_ctx.obj)) {
> DRM_DEBUG_DRIVER("alloc LRC WA ctx backing obj failed.\n");
> - return -ENOMEM;
> + return PTR_ERR(ring->wa_ctx.obj);
Safer not to store error objects inside structs - we have a habit of
using drm_gem_object_unreference() which will then explode.
> }
>
> ret = i915_gem_obj_ggtt_pin(ring->wa_ctx.obj, PAGE_SIZE, 0);
> @@ -2459,9 +2459,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(ring, 4 * PAGE_SIZE);
> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> index 4445426..bfb95a2 100644
> --- a/drivers/gpu/drm/i915/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/intel_overlay.c
> @@ -1392,9 +1392,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 ab5ac5e..0d35d7f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5387,7 +5387,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");
> return;
> }
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 16a4ead..a928602 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -676,9 +676,9 @@ intel_init_pipe_control(struct intel_engine_cs *ring)
> WARN_ON(ring->scratch.obj);
>
> ring->scratch.obj = i915_gem_alloc_object(ring->dev, 4096);
> - if (ring->scratch.obj == NULL) {
> + if (IS_ERR(ring->scratch.obj)) {
> DRM_ERROR("Failed to allocate seqno page\n");
> - ret = -ENOMEM;
> + ret = PTR_ERR(ring->scratch.obj);
Beware the caller subsequently calling drm_gem_object_unreference on the
error pointer.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 4/6] drm/i915: Add support for stealing purgable stolen pages
2015-10-08 6:24 [PATCH v8 0/6] Support for creating/using Stolen memory backed objects ankitprasad.r.sharma
` (2 preceding siblings ...)
2015-10-08 6:24 ` [PATCH 3/6] drm/i915: Propagating correct error codes to the userspace ankitprasad.r.sharma
@ 2015-10-08 6:24 ` ankitprasad.r.sharma
2015-10-08 10:43 ` Tvrtko Ursulin
2015-10-08 6:24 ` [PATCH 5/6] drm/i915: Support for pread/pwrite from/to non shmem backed objects ankitprasad.r.sharma
2015-10-08 6:24 ` [PATCH 6/6] drm/i915: Migrate stolen objects before hibernation ankitprasad.r.sharma
5 siblings, 1 reply; 36+ messages in thread
From: ankitprasad.r.sharma @ 2015-10-08 6:24 UTC (permalink / raw)
To: intel-gfx; +Cc: Ankitprasad Sharma, akash.goel, shashidhar.hiremath
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()
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>
---
drivers/gpu/drm/i915/i915_debugfs.c | 4 +-
drivers/gpu/drm/i915/i915_drv.h | 17 +++-
drivers/gpu/drm/i915/i915_gem.c | 16 ++++
drivers/gpu/drm/i915/i915_gem_stolen.c | 169 +++++++++++++++++++++++++++++----
drivers/gpu/drm/i915/intel_pm.c | 4 +-
5 files changed, 186 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 8797717..13762c1 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,7 +253,7 @@ 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);
- return a->stolen->start - b->stolen->start;
+ return a->stolen->base.start - b->stolen->base.start;
}
static int i915_gem_stolen_list_info(struct seq_file *m, void *data)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index bca1572..5612df3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -850,6 +850,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
@@ -1279,6 +1285,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) */
@@ -2047,7 +2060,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 ring_list[I915_NUM_RINGS];
@@ -2055,6 +2068,7 @@ struct drm_i915_gem_object {
struct list_head obj_exec_link;
struct list_head batch_pool_link;
+ struct list_head tmp_link;
/**
* This is set if the object is on the active lists (has pending
@@ -2171,6 +2185,7 @@ struct drm_i915_gem_object {
};
};
#define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base)
+#define I915_BO_IS_ACTIVE(__obj) (__obj->active)
void i915_gem_track_fb(struct drm_i915_gem_object *old,
struct drm_i915_gem_object *new,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 54f7df1..91a2e97 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4233,6 +4233,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:
@@ -4253,6 +4267,7 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
INIT_LIST_HEAD(&obj->obj_exec_link);
INIT_LIST_HEAD(&obj->vma_list);
INIT_LIST_HEAD(&obj->batch_pool_link);
+ INIT_LIST_HEAD(&obj->tmp_link);
obj->ops = ops;
@@ -4895,6 +4910,7 @@ i915_gem_load(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_RINGS; i++)
init_ring_lists(&dev_priv->ring[i]);
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index d3e6813..bd5b2ea 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -456,7 +456,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;
}
@@ -469,7 +470,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;
int ret = 0;
@@ -478,11 +479,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);
obj->pages = i915_pages_create_for_stolen(dev,
- stolen->start, stolen->size);
+ stolen->base.start,
+ stolen->base.size);
if (IS_ERR(obj->pages)) {
ret = PTR_ERR(obj->pages);
goto cleanup;
@@ -491,6 +493,9 @@ _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;
@@ -501,18 +506,102 @@ cleanup:
return ERR_PTR(ret);
}
-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);
+ 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 (I915_BO_IS_ACTIVE(iter->obj) != 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,
+ vma_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);
@@ -520,17 +609,59 @@ 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, 0);
+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;
+
+ 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_PTR(-ENOMEM);
+
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;
}
@@ -544,7 +675,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
struct drm_i915_private *dev_priv = dev->dev_private;
struct i915_address_space *ggtt = &dev_priv->gtt.base;
struct drm_i915_gem_object *obj;
- struct drm_mm_node *stolen;
+ struct i915_stolen_node *stolen;
struct i915_vma *vma;
int ret;
@@ -563,10 +694,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");
@@ -577,7 +708,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 0d35d7f..12bf162 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5318,7 +5318,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);
}
@@ -5392,7 +5392,7 @@ static void valleyview_setup_pctx(struct drm_device *dev)
return;
}
- 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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 4/6] drm/i915: Add support for stealing purgable stolen pages
2015-10-08 6:24 ` [PATCH 4/6] drm/i915: Add support for stealing purgable stolen pages ankitprasad.r.sharma
@ 2015-10-08 10:43 ` Tvrtko Ursulin
2015-10-08 11:09 ` Chris Wilson
2015-10-09 8:11 ` Daniel Vetter
0 siblings, 2 replies; 36+ messages in thread
From: Tvrtko Ursulin @ 2015-10-08 10:43 UTC (permalink / raw)
To: ankitprasad.r.sharma, intel-gfx; +Cc: akash.goel, shashidhar.hiremath
Hi,
On 08/10/15 07:24, ankitprasad.r.sharma@intel.com wrote:
> 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()
>
> 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>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 4 +-
> drivers/gpu/drm/i915/i915_drv.h | 17 +++-
> drivers/gpu/drm/i915/i915_gem.c | 16 ++++
> drivers/gpu/drm/i915/i915_gem_stolen.c | 169 +++++++++++++++++++++++++++++----
> drivers/gpu/drm/i915/intel_pm.c | 4 +-
> 5 files changed, 186 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 8797717..13762c1 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,7 +253,7 @@ 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);
>
> - return a->stolen->start - b->stolen->start;
> + return a->stolen->base.start - b->stolen->base.start;
> }
>
> static int i915_gem_stolen_list_info(struct seq_file *m, void *data)
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index bca1572..5612df3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -850,6 +850,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
>
> @@ -1279,6 +1285,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) */
>
> @@ -2047,7 +2060,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 ring_list[I915_NUM_RINGS];
> @@ -2055,6 +2068,7 @@ struct drm_i915_gem_object {
> struct list_head obj_exec_link;
>
> struct list_head batch_pool_link;
> + struct list_head tmp_link;
>
> /**
> * This is set if the object is on the active lists (has pending
> @@ -2171,6 +2185,7 @@ struct drm_i915_gem_object {
> };
> };
> #define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base)
> +#define I915_BO_IS_ACTIVE(__obj) (__obj->active)
>
> void i915_gem_track_fb(struct drm_i915_gem_object *old,
> struct drm_i915_gem_object *new,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 54f7df1..91a2e97 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4233,6 +4233,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:
> @@ -4253,6 +4267,7 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
> INIT_LIST_HEAD(&obj->obj_exec_link);
> INIT_LIST_HEAD(&obj->vma_list);
> INIT_LIST_HEAD(&obj->batch_pool_link);
> + INIT_LIST_HEAD(&obj->tmp_link);
>
> obj->ops = ops;
>
> @@ -4895,6 +4910,7 @@ i915_gem_load(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_RINGS; i++)
> init_ring_lists(&dev_priv->ring[i]);
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index d3e6813..bd5b2ea 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -456,7 +456,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;
> }
> @@ -469,7 +470,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;
> int ret = 0;
> @@ -478,11 +479,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);
>
> obj->pages = i915_pages_create_for_stolen(dev,
> - stolen->start, stolen->size);
> + stolen->base.start,
> + stolen->base.size);
> if (IS_ERR(obj->pages)) {
> ret = PTR_ERR(obj->pages);
> goto cleanup;
> @@ -491,6 +493,9 @@ _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;
>
> @@ -501,18 +506,102 @@ cleanup:
> return ERR_PTR(ret);
> }
>
> -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);
I am fundamentally opposed to BUG_ONs which can be avoided. In this I
see no value in hanging the machine while we could WARN_ON and return
false.
> + 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);
> + 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 (I915_BO_IS_ACTIVE(iter->obj) != 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,
> + vma_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);
>
> @@ -520,17 +609,59 @@ 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);
I have raised this question of struct_mutex in the previous round.
Correct me if I am wrong, but I thought there was some effort made to
make stolen object allocation run without struct mutex?
With this change it requires it again. At the moment callers seem to
hold it anyway. But I think lockdep_assert_held is needed now at least
to document the requirement, probably in top level
i915_gem_object_create_stolen.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 4/6] drm/i915: Add support for stealing purgable stolen pages
2015-10-08 10:43 ` Tvrtko Ursulin
@ 2015-10-08 11:09 ` Chris Wilson
2015-10-08 14:31 ` Tvrtko Ursulin
2015-10-09 8:11 ` Daniel Vetter
1 sibling, 1 reply; 36+ messages in thread
From: Chris Wilson @ 2015-10-08 11:09 UTC (permalink / raw)
To: Tvrtko Ursulin
Cc: ankitprasad.r.sharma, intel-gfx, akash.goel, shashidhar.hiremath
On Thu, Oct 08, 2015 at 11:43:29AM +0100, Tvrtko Ursulin wrote:
> >-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);
>
> I am fundamentally opposed to BUG_ONs which can be avoided. In this
> I see no value in hanging the machine while we could WARN_ON and
> return false.
Don't bother with the WARN_ON. Either take the BUG_ON or accept that to
get to this point the machine is dead anyway and a warning here doesn't
help identify the root cause (better off with list debugging and memory
debugging). I have personally been converting these asserts over to a
dev-only compiletime option as I still find the BUGs more useful than
WARNs in the GEM code.
> >+ 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);
> >+}
> >@@ -520,17 +609,59 @@ 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);
>
> I have raised this question of struct_mutex in the previous round.
>
> Correct me if I am wrong, but I thought there was some effort made
> to make stolen object allocation run without struct mutex?
Correct. But note that we do GEM operations inside the eviction logic
and the struct_mutex is the only one we have for them.
> With this change it requires it again. At the moment callers seem to
> hold it anyway. But I think lockdep_assert_held is needed now at
> least to document the requirement, probably in top level
> i915_gem_object_create_stolen.
And a comment as to why, might as well also try and document the logic
behind such decisions.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 4/6] drm/i915: Add support for stealing purgable stolen pages
2015-10-08 11:09 ` Chris Wilson
@ 2015-10-08 14:31 ` Tvrtko Ursulin
2015-10-08 15:08 ` Chris Wilson
0 siblings, 1 reply; 36+ messages in thread
From: Tvrtko Ursulin @ 2015-10-08 14:31 UTC (permalink / raw)
To: Chris Wilson, ankitprasad.r.sharma, intel-gfx, akash.goel,
shashidhar.hiremath
On 08/10/15 12:09, Chris Wilson wrote:
> On Thu, Oct 08, 2015 at 11:43:29AM +0100, Tvrtko Ursulin wrote:
>>> -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);
>>
>> I am fundamentally opposed to BUG_ONs which can be avoided. In this
>> I see no value in hanging the machine while we could WARN_ON and
>> return false.
>
> Don't bother with the WARN_ON. Either take the BUG_ON or accept that to
> get to this point the machine is dead anyway and a warning here doesn't
> help identify the root cause (better off with list debugging and memory
> debugging). I have personally been converting these asserts over to a
> dev-only compiletime option as I still find the BUGs more useful than
> WARNs in the GEM code.
This is one of the ones which are to be expected in development only. At
that time I much prefer a WARN_ON since it potentially saves you one
reboot cycle and there aren't really any downsides to it. Especially if,
as you say, machine is dead already.
>>> + 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);
>>> +}
>
>>> @@ -520,17 +609,59 @@ 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);
>>
>> I have raised this question of struct_mutex in the previous round.
>>
>> Correct me if I am wrong, but I thought there was some effort made
>> to make stolen object allocation run without struct mutex?
>
> Correct. But note that we do GEM operations inside the eviction logic
> and the struct_mutex is the only one we have for them.
>
>> With this change it requires it again. At the moment callers seem to
>> hold it anyway. But I think lockdep_assert_held is needed now at
>> least to document the requirement, probably in top level
>> i915_gem_object_create_stolen.
>
> And a comment as to why, might as well also try and document the logic
> behind such decisions.
Agreed.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 4/6] drm/i915: Add support for stealing purgable stolen pages
2015-10-08 14:31 ` Tvrtko Ursulin
@ 2015-10-08 15:08 ` Chris Wilson
2015-10-09 8:13 ` Daniel Vetter
0 siblings, 1 reply; 36+ messages in thread
From: Chris Wilson @ 2015-10-08 15:08 UTC (permalink / raw)
To: Tvrtko Ursulin
Cc: ankitprasad.r.sharma, intel-gfx, akash.goel, shashidhar.hiremath
On Thu, Oct 08, 2015 at 03:31:08PM +0100, Tvrtko Ursulin wrote:
>
> On 08/10/15 12:09, Chris Wilson wrote:
> >On Thu, Oct 08, 2015 at 11:43:29AM +0100, Tvrtko Ursulin wrote:
> >>>-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);
> >>
> >>I am fundamentally opposed to BUG_ONs which can be avoided. In this
> >>I see no value in hanging the machine while we could WARN_ON and
> >>return false.
> >
> >Don't bother with the WARN_ON. Either take the BUG_ON or accept that to
> >get to this point the machine is dead anyway and a warning here doesn't
> >help identify the root cause (better off with list debugging and memory
> >debugging). I have personally been converting these asserts over to a
> >dev-only compiletime option as I still find the BUGs more useful than
> >WARNs in the GEM code.
>
> This is one of the ones which are to be expected in development
> only. At that time I much prefer a WARN_ON since it potentially
> saves you one reboot cycle and there aren't really any downsides to
> it. Especially if, as you say, machine is dead already.
panic-on-oops ftw :-p
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 4/6] drm/i915: Add support for stealing purgable stolen pages
2015-10-08 15:08 ` Chris Wilson
@ 2015-10-09 8:13 ` Daniel Vetter
0 siblings, 0 replies; 36+ messages in thread
From: Daniel Vetter @ 2015-10-09 8:13 UTC (permalink / raw)
To: Chris Wilson, Tvrtko Ursulin, ankitprasad.r.sharma, intel-gfx,
akash.goel, shashidhar.hiremath
On Thu, Oct 08, 2015 at 04:08:52PM +0100, Chris Wilson wrote:
> On Thu, Oct 08, 2015 at 03:31:08PM +0100, Tvrtko Ursulin wrote:
> >
> > On 08/10/15 12:09, Chris Wilson wrote:
> > >On Thu, Oct 08, 2015 at 11:43:29AM +0100, Tvrtko Ursulin wrote:
> > >>>-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);
> > >>
> > >>I am fundamentally opposed to BUG_ONs which can be avoided. In this
> > >>I see no value in hanging the machine while we could WARN_ON and
> > >>return false.
> > >
> > >Don't bother with the WARN_ON. Either take the BUG_ON or accept that to
> > >get to this point the machine is dead anyway and a warning here doesn't
> > >help identify the root cause (better off with list debugging and memory
> > >debugging). I have personally been converting these asserts over to a
> > >dev-only compiletime option as I still find the BUGs more useful than
> > >WARNs in the GEM code.
> >
> > This is one of the ones which are to be expected in development
> > only. At that time I much prefer a WARN_ON since it potentially
> > saves you one reboot cycle and there aren't really any downsides to
> > it. Especially if, as you say, machine is dead already.
>
> panic-on-oops ftw :-p
We killed drm panic handling, and if it gets resurrect it will be
super-minimal to avoid any kind of "my real oops scrolled off the screen
because i915.ko was dying even harder ..." bug reports. We've done this
because it's pretty much impossible to avoid piles of WARN_ON, lockdep
splats and other crap once you're trying to do a full modeset from panic
context.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 4/6] drm/i915: Add support for stealing purgable stolen pages
2015-10-08 10:43 ` Tvrtko Ursulin
2015-10-08 11:09 ` Chris Wilson
@ 2015-10-09 8:11 ` Daniel Vetter
1 sibling, 0 replies; 36+ messages in thread
From: Daniel Vetter @ 2015-10-09 8:11 UTC (permalink / raw)
To: Tvrtko Ursulin
Cc: ankitprasad.r.sharma, intel-gfx, akash.goel, shashidhar.hiremath
On Thu, Oct 08, 2015 at 11:43:29AM +0100, Tvrtko Ursulin wrote:
>
> Hi,
>
> On 08/10/15 07:24, ankitprasad.r.sharma@intel.com wrote:
> >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()
> >
> >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>
> >---
> > drivers/gpu/drm/i915/i915_debugfs.c | 4 +-
> > drivers/gpu/drm/i915/i915_drv.h | 17 +++-
> > drivers/gpu/drm/i915/i915_gem.c | 16 ++++
> > drivers/gpu/drm/i915/i915_gem_stolen.c | 169 +++++++++++++++++++++++++++++----
> > drivers/gpu/drm/i915/intel_pm.c | 4 +-
> > 5 files changed, 186 insertions(+), 24 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> >index 8797717..13762c1 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,7 +253,7 @@ 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);
> >
> >- return a->stolen->start - b->stolen->start;
> >+ return a->stolen->base.start - b->stolen->base.start;
> > }
> >
> > static int i915_gem_stolen_list_info(struct seq_file *m, void *data)
> >diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >index bca1572..5612df3 100644
> >--- a/drivers/gpu/drm/i915/i915_drv.h
> >+++ b/drivers/gpu/drm/i915/i915_drv.h
> >@@ -850,6 +850,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
> >
> >@@ -1279,6 +1285,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) */
> >
> >@@ -2047,7 +2060,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 ring_list[I915_NUM_RINGS];
> >@@ -2055,6 +2068,7 @@ struct drm_i915_gem_object {
> > struct list_head obj_exec_link;
> >
> > struct list_head batch_pool_link;
> >+ struct list_head tmp_link;
> >
> > /**
> > * This is set if the object is on the active lists (has pending
> >@@ -2171,6 +2185,7 @@ struct drm_i915_gem_object {
> > };
> > };
> > #define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base)
> >+#define I915_BO_IS_ACTIVE(__obj) (__obj->active)
> >
> > void i915_gem_track_fb(struct drm_i915_gem_object *old,
> > struct drm_i915_gem_object *new,
> >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >index 54f7df1..91a2e97 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -4233,6 +4233,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:
> >@@ -4253,6 +4267,7 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
> > INIT_LIST_HEAD(&obj->obj_exec_link);
> > INIT_LIST_HEAD(&obj->vma_list);
> > INIT_LIST_HEAD(&obj->batch_pool_link);
> >+ INIT_LIST_HEAD(&obj->tmp_link);
> >
> > obj->ops = ops;
> >
> >@@ -4895,6 +4910,7 @@ i915_gem_load(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_RINGS; i++)
> > init_ring_lists(&dev_priv->ring[i]);
> >diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> >index d3e6813..bd5b2ea 100644
> >--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> >+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> >@@ -456,7 +456,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;
> > }
> >@@ -469,7 +470,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;
> > int ret = 0;
> >@@ -478,11 +479,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);
> >
> > obj->pages = i915_pages_create_for_stolen(dev,
> >- stolen->start, stolen->size);
> >+ stolen->base.start,
> >+ stolen->base.size);
> > if (IS_ERR(obj->pages)) {
> > ret = PTR_ERR(obj->pages);
> > goto cleanup;
> >@@ -491,6 +493,9 @@ _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;
> >
> >@@ -501,18 +506,102 @@ cleanup:
> > return ERR_PTR(ret);
> > }
> >
> >-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);
>
> I am fundamentally opposed to BUG_ONs which can be avoided. In this I see no
> value in hanging the machine while we could WARN_ON and return false.
>
> >+ 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);
> >+ 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 (I915_BO_IS_ACTIVE(iter->obj) != 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,
> >+ vma_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);
> >
> >@@ -520,17 +609,59 @@ 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);
>
> I have raised this question of struct_mutex in the previous round.
>
> Correct me if I am wrong, but I thought there was some effort made to make
> stolen object allocation run without struct mutex?
>
> With this change it requires it again. At the moment callers seem to hold it
> anyway. But I think lockdep_assert_held is needed now at least to document
> the requirement, probably in top level i915_gem_object_create_stolen.
Please use WARN_ON(!mutex_is_locked()) because lockdep_assert_held is
compiled out when lockdep is disabled. And that's for almost everyone. The
reason for that is that the semantics aren't a perfect match -
lockdep_assert_held also makes sure that it's the calling process holding
the lock, not just that it's locked.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 5/6] drm/i915: Support for pread/pwrite from/to non shmem backed objects
2015-10-08 6:24 [PATCH v8 0/6] Support for creating/using Stolen memory backed objects ankitprasad.r.sharma
` (3 preceding siblings ...)
2015-10-08 6:24 ` [PATCH 4/6] drm/i915: Add support for stealing purgable stolen pages ankitprasad.r.sharma
@ 2015-10-08 6:24 ` ankitprasad.r.sharma
2015-10-08 13:56 ` Tvrtko Ursulin
2015-10-08 6:24 ` [PATCH 6/6] drm/i915: Migrate stolen objects before hibernation ankitprasad.r.sharma
5 siblings, 1 reply; 36+ messages in thread
From: ankitprasad.r.sharma @ 2015-10-08 6:24 UTC (permalink / raw)
To: intel-gfx; +Cc: Ankitprasad Sharma, akash.goel, shashidhar.hiremath
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 prime objects as well as stolen memory backed objects
but for userptr objects it is still forbidden.
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)
Testcase: igt/gem_stolen
Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
---
drivers/gpu/drm/i915/i915_gem.c | 125 +++++++++++++++++++++++++++++++++-------
1 file changed, 104 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 91a2e97..2c94e22 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -614,6 +614,99 @@ shmem_pread_slow(struct page *page, int shmem_page_offset, int page_length,
return ret ? - EFAULT : 0;
}
+static inline uint64_t
+slow_user_access(struct io_mapping *mapping,
+ uint64_t page_base, int page_offset,
+ char __user *user_data,
+ int length, bool pwrite)
+{
+ void __iomem *vaddr_inatomic;
+ void *vaddr;
+ uint64_t unwritten;
+
+ vaddr_inatomic = io_mapping_map_wc(mapping, page_base);
+ /* We can use the cpu mem copy function because this is X86. */
+ vaddr = (void __force *)vaddr_inatomic + page_offset;
+ if (pwrite)
+ unwritten = __copy_from_user(vaddr, user_data, length);
+ else
+ unwritten = __copy_to_user(user_data, vaddr, length);
+
+ io_mapping_unmap(vaddr_inatomic);
+ 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;
+ char __user *user_data;
+ uint64_t remain;
+ uint64_t offset, page_base;
+ int page_offset, page_length, ret = 0;
+
+ ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
+ if (ret)
+ goto out;
+
+ ret = i915_gem_object_set_to_gtt_domain(obj, false);
+ if (ret)
+ goto out_unpin;
+
+ ret = i915_gem_object_put_fence(obj);
+ if (ret)
+ goto out_unpin;
+
+ user_data = to_user_ptr(data_ptr);
+ remain = size;
+ offset = i915_gem_obj_ggtt_offset(obj) + data_offset;
+
+ mutex_unlock(&dev->struct_mutex);
+ if (likely(!i915.prefault_disable))
+ ret = fault_in_multipages_writeable(user_data, remain);
+
+ /*
+ * page_offset = offset within page
+ * page_base = page offset within aperture
+ */
+ page_offset = offset_in_page(offset);
+ page_base = offset & PAGE_MASK;
+
+ while (remain > 0) {
+ /* page_length = bytes to copy for this page */
+ page_length = remain;
+ if ((page_offset + remain) > PAGE_SIZE)
+ page_length = PAGE_SIZE - page_offset;
+
+ /* This is a slow read/write as it tries to read from
+ * and write to user memory which may result into page
+ * faults
+ */
+ ret = slow_user_access(dev_priv->gtt.mappable, page_base,
+ page_offset, user_data,
+ page_length, false);
+
+ if (ret) {
+ ret = -EFAULT;
+ break;
+ }
+
+ remain -= page_length;
+ user_data += page_length;
+ page_base += page_length;
+ page_offset = 0;
+ }
+
+ mutex_lock(&dev->struct_mutex);
+
+out_unpin:
+ 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,
@@ -737,17 +830,14 @@ 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 (!obj->base.filp && obj->tiling_mode == I915_TILING_NONE)
+ ret = i915_gem_gtt_pread(dev, obj, args->size,
+ args->offset, args->data_ptr);
+ else
+ ret = i915_gem_shmem_pread(dev, obj, args, file);
out:
drm_gem_object_unreference(&obj->base);
@@ -795,7 +885,7 @@ i915_gem_gtt_pwrite_fast(struct drm_device *dev,
char __user *user_data;
int page_offset, page_length, ret;
- ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE | PIN_NONBLOCK);
+ ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
if (ret)
goto out;
@@ -1090,14 +1180,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;
@@ -1108,8 +1190,9 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
* 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)) {
+ (!obj->base.filp ||
+ (obj->base.write_domain != I915_GEM_DOMAIN_CPU &&
+ cpu_write_needs_clflush(obj)))) {
ret = i915_gem_gtt_pwrite_fast(dev, obj, args, file);
/* Note that the gtt paths might fail with non-page-backed user
* pointers (e.g. gtt mappings when moving data between
@@ -1119,7 +1202,7 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
if (ret == -EFAULT || ret == -ENOSPC) {
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);
}
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 5/6] drm/i915: Support for pread/pwrite from/to non shmem backed objects
2015-10-08 6:24 ` [PATCH 5/6] drm/i915: Support for pread/pwrite from/to non shmem backed objects ankitprasad.r.sharma
@ 2015-10-08 13:56 ` Tvrtko Ursulin
2015-10-28 11:18 ` Ankitprasad Sharma
0 siblings, 1 reply; 36+ messages in thread
From: Tvrtko Ursulin @ 2015-10-08 13:56 UTC (permalink / raw)
To: ankitprasad.r.sharma, intel-gfx; +Cc: akash.goel, shashidhar.hiremath
Hi,
On 08/10/15 07:24, ankitprasad.r.sharma@intel.com wrote:
> 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 prime objects as well as stolen memory backed objects
> but for userptr objects it is still forbidden.
Where is the part which forbids it for userptr 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)
>
> Testcase: igt/gem_stolen
>
> Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem.c | 125 +++++++++++++++++++++++++++++++++-------
> 1 file changed, 104 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 91a2e97..2c94e22 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -614,6 +614,99 @@ shmem_pread_slow(struct page *page, int shmem_page_offset, int page_length,
> return ret ? - EFAULT : 0;
> }
>
> +static inline uint64_t
> +slow_user_access(struct io_mapping *mapping,
> + uint64_t page_base, int page_offset,
> + char __user *user_data,
> + int length, bool pwrite)
> +{
> + void __iomem *vaddr_inatomic;
> + void *vaddr;
> + uint64_t unwritten;
> +
> + vaddr_inatomic = io_mapping_map_wc(mapping, page_base);
> + /* We can use the cpu mem copy function because this is X86. */
> + vaddr = (void __force *)vaddr_inatomic + page_offset;
> + if (pwrite)
> + unwritten = __copy_from_user(vaddr, user_data, length);
> + else
> + unwritten = __copy_to_user(user_data, vaddr, length);
> +
> + io_mapping_unmap(vaddr_inatomic);
> + 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;
> + char __user *user_data;
> + uint64_t remain;
> + uint64_t offset, page_base;
> + int page_offset, page_length, ret = 0;
> +
> + ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
> + if (ret)
> + goto out;
> +
> + ret = i915_gem_object_set_to_gtt_domain(obj, false);
> + if (ret)
> + goto out_unpin;
> +
> + ret = i915_gem_object_put_fence(obj);
> + if (ret)
> + goto out_unpin;
> +
> + user_data = to_user_ptr(data_ptr);
> + remain = size;
> + offset = i915_gem_obj_ggtt_offset(obj) + data_offset;
> +
> + mutex_unlock(&dev->struct_mutex);
> + if (likely(!i915.prefault_disable))
> + ret = fault_in_multipages_writeable(user_data, remain);
> +
> + /*
> + * page_offset = offset within page
> + * page_base = page offset within aperture
> + */
> + page_offset = offset_in_page(offset);
> + page_base = offset & PAGE_MASK;
> +
> + while (remain > 0) {
> + /* page_length = bytes to copy for this page */
> + page_length = remain;
> + if ((page_offset + remain) > PAGE_SIZE)
> + page_length = PAGE_SIZE - page_offset;
> +
> + /* This is a slow read/write as it tries to read from
> + * and write to user memory which may result into page
> + * faults
> + */
> + ret = slow_user_access(dev_priv->gtt.mappable, page_base,
> + page_offset, user_data,
> + page_length, false);
> +
> + if (ret) {
> + ret = -EFAULT;
> + break;
> + }
> +
> + remain -= page_length;
> + user_data += page_length;
> + page_base += page_length;
> + page_offset = 0;
> + }
> +
> + mutex_lock(&dev->struct_mutex);
> +
> +out_unpin:
> + 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,
> @@ -737,17 +830,14 @@ 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 (!obj->base.filp && obj->tiling_mode == I915_TILING_NONE)
> + ret = i915_gem_gtt_pread(dev, obj, args->size,
> + args->offset, args->data_ptr);
> + else
> + ret = i915_gem_shmem_pread(dev, obj, args, file);
>
> out:
> drm_gem_object_unreference(&obj->base);
> @@ -795,7 +885,7 @@ i915_gem_gtt_pwrite_fast(struct drm_device *dev,
> char __user *user_data;
> int page_offset, page_length, ret;
>
> - ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE | PIN_NONBLOCK);
> + ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
Why is this needed?
> if (ret)
> goto out;
>
> @@ -1090,14 +1180,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;
> @@ -1108,8 +1190,9 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
> * 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)) {
> + (!obj->base.filp ||
> + (obj->base.write_domain != I915_GEM_DOMAIN_CPU &&
> + cpu_write_needs_clflush(obj)))) {
> ret = i915_gem_gtt_pwrite_fast(dev, obj, args, file);
So the pwrite path will fail if a page fault happens, as opposed to the
pread path which makes an effort to handle it. What is the reason for
this asymmetry in the API? Or I am missing something?
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 5/6] drm/i915: Support for pread/pwrite from/to non shmem backed objects
2015-10-08 13:56 ` Tvrtko Ursulin
@ 2015-10-28 11:18 ` Ankitprasad Sharma
0 siblings, 0 replies; 36+ messages in thread
From: Ankitprasad Sharma @ 2015-10-28 11:18 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx, akash.goel, shashidhar.hiremath
On Thu, 2015-10-08 at 14:56 +0100, Tvrtko Ursulin wrote:
> Hi,
>
> On 08/10/15 07:24, ankitprasad.r.sharma@intel.com wrote:
> > 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 prime objects as well as stolen memory backed objects
> > but for userptr objects it is still forbidden.
>
> Where is the part which forbids it for userptr objects?
In version 5, updated the patch handle pwrite/pread for all non-shmem
backed objects, including userptr objects
Will update the Commit message
>
> > 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)
> >
> > Testcase: igt/gem_stolen
> >
> > Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_gem.c | 125 +++++++++++++++++++++++++++++++++-------
> > 1 file changed, 104 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 91a2e97..2c94e22 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -614,6 +614,99 @@ shmem_pread_slow(struct page *page, int shmem_page_offset, int page_length,
> > return ret ? - EFAULT : 0;
> > }
> >
> > +static inline uint64_t
> > +slow_user_access(struct io_mapping *mapping,
> > + uint64_t page_base, int page_offset,
> > + char __user *user_data,
> > + int length, bool pwrite)
> > +{
> > + void __iomem *vaddr_inatomic;
> > + void *vaddr;
> > + uint64_t unwritten;
> > +
> > + vaddr_inatomic = io_mapping_map_wc(mapping, page_base);
> > + /* We can use the cpu mem copy function because this is X86. */
> > + vaddr = (void __force *)vaddr_inatomic + page_offset;
> > + if (pwrite)
> > + unwritten = __copy_from_user(vaddr, user_data, length);
> > + else
> > + unwritten = __copy_to_user(user_data, vaddr, length);
> > +
> > + io_mapping_unmap(vaddr_inatomic);
> > + 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;
> > + char __user *user_data;
> > + uint64_t remain;
> > + uint64_t offset, page_base;
> > + int page_offset, page_length, ret = 0;
> > +
> > + ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
> > + if (ret)
> > + goto out;
> > +
> > + ret = i915_gem_object_set_to_gtt_domain(obj, false);
> > + if (ret)
> > + goto out_unpin;
> > +
> > + ret = i915_gem_object_put_fence(obj);
> > + if (ret)
> > + goto out_unpin;
> > +
> > + user_data = to_user_ptr(data_ptr);
> > + remain = size;
> > + offset = i915_gem_obj_ggtt_offset(obj) + data_offset;
> > +
> > + mutex_unlock(&dev->struct_mutex);
> > + if (likely(!i915.prefault_disable))
> > + ret = fault_in_multipages_writeable(user_data, remain);
> > +
> > + /*
> > + * page_offset = offset within page
> > + * page_base = page offset within aperture
> > + */
> > + page_offset = offset_in_page(offset);
> > + page_base = offset & PAGE_MASK;
> > +
> > + while (remain > 0) {
> > + /* page_length = bytes to copy for this page */
> > + page_length = remain;
> > + if ((page_offset + remain) > PAGE_SIZE)
> > + page_length = PAGE_SIZE - page_offset;
> > +
> > + /* This is a slow read/write as it tries to read from
> > + * and write to user memory which may result into page
> > + * faults
> > + */
> > + ret = slow_user_access(dev_priv->gtt.mappable, page_base,
> > + page_offset, user_data,
> > + page_length, false);
> > +
> > + if (ret) {
> > + ret = -EFAULT;
> > + break;
> > + }
> > +
> > + remain -= page_length;
> > + user_data += page_length;
> > + page_base += page_length;
> > + page_offset = 0;
> > + }
> > +
> > + mutex_lock(&dev->struct_mutex);
> > +
> > +out_unpin:
> > + 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,
> > @@ -737,17 +830,14 @@ 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 (!obj->base.filp && obj->tiling_mode == I915_TILING_NONE)
> > + ret = i915_gem_gtt_pread(dev, obj, args->size,
> > + args->offset, args->data_ptr);
> > + else
> > + ret = i915_gem_shmem_pread(dev, obj, args, file);
> >
> > out:
> > drm_gem_object_unreference(&obj->base);
> > @@ -795,7 +885,7 @@ i915_gem_gtt_pwrite_fast(struct drm_device *dev,
> > char __user *user_data;
> > int page_offset, page_length, ret;
> >
> > - ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE | PIN_NONBLOCK);
> > + ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
>
> Why is this needed?
This was Chris' suggestion. This change can go as a separate patch, if
needed. I do not think pwrite/pread has any dependency on this.
Need Chris to respond on this.
>
> > if (ret)
> > goto out;
> >
> > @@ -1090,14 +1180,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;
> > @@ -1108,8 +1190,9 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
> > * 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)) {
> > + (!obj->base.filp ||
> > + (obj->base.write_domain != I915_GEM_DOMAIN_CPU &&
> > + cpu_write_needs_clflush(obj)))) {
> > ret = i915_gem_gtt_pwrite_fast(dev, obj, args, file);
>
> So the pwrite path will fail if a page fault happens, as opposed to the
> pread path which makes an effort to handle it. What is the reason for
> this asymmetry in the API? Or I am missing something?
I had earlier implemented the pwrite and pread maintaining the symmetry
in the API. After couple of revisions we landed on this implementation.
Need Chris to respond on this.
Thanks,
Ankit
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 6/6] drm/i915: Migrate stolen objects before hibernation
2015-10-08 6:24 [PATCH v8 0/6] Support for creating/using Stolen memory backed objects ankitprasad.r.sharma
` (4 preceding siblings ...)
2015-10-08 6:24 ` [PATCH 5/6] drm/i915: Support for pread/pwrite from/to non shmem backed objects ankitprasad.r.sharma
@ 2015-10-08 6:24 ` ankitprasad.r.sharma
2015-10-08 11:02 ` Chris Wilson
5 siblings, 1 reply; 36+ messages in thread
From: ankitprasad.r.sharma @ 2015-10-08 6:24 UTC (permalink / raw)
To: intel-gfx; +Cc: Ankitprasad Sharma, akash.goel, shashidhar.hiremath
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)
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
---
drivers/gpu/drm/i915/i915_drv.c | 17 ++-
drivers/gpu/drm/i915/i915_drv.h | 7 +
drivers/gpu/drm/i915/i915_gem.c | 230 ++++++++++++++++++++++++++++++--
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 +
7 files changed, 259 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e6d7a69..7663fb4 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -968,6 +968,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;
@@ -1621,7 +1636,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 5612df3..1efa3b6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2094,6 +2094,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.
@@ -2981,6 +2987,7 @@ 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_ringbuffer(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_suspend(struct drm_device *dev);
void __i915_add_request(struct drm_i915_gem_request *req,
struct drm_i915_gem_object *batch_obj,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 2c94e22..843f3d1 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4365,12 +4365,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 = 0;
obj = i915_gem_object_alloc(dev);
@@ -4382,15 +4397,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);
@@ -4567,6 +4574,207 @@ i915_gem_stop_ringbuffers(struct drm_device *dev)
dev_priv->gt.stop_ring(ring);
}
+static 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 drm_mm_node node;
+ struct file *file;
+ struct address_space *mapping;
+ struct sg_table *stolen_pages, *shmemfs_pages;
+ int ret, i;
+
+ if (WARN_ON(i915_gem_object_needs_bit17_swizzle(obj)))
+ return -EINVAL;
+
+ ret = i915_gem_object_set_to_gtt_domain(obj, false);
+ if (ret)
+ return ret;
+
+ 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, vma_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;
+ goto swap_pages;
+ }
+
+ /* stolen objects are already pinned to prevent shrinkage */
+ memset(&node, 0, sizeof(node));
+ ret = drm_mm_insert_node_in_range_generic(&i915->gtt.base.mm,
+ &node,
+ 4096, 0, I915_CACHE_NONE,
+ 0, i915->gtt.mappable_end,
+ DRM_MM_SEARCH_DEFAULT,
+ DRM_MM_CREATE_DEFAULT);
+ if (ret)
+ return ret;
+
+ i915->gtt.base.insert_entries(&i915->gtt.base, obj->pages,
+ node.start, I915_CACHE_NONE, 0);
+
+ for (i = 0; i < obj->base.size / PAGE_SIZE; i++) {
+ struct page *page;
+ void *__iomem src;
+ void *dst;
+
+ page = shmem_read_mapping_page(mapping, i);
+ if (IS_ERR(page)) {
+ ret = PTR_ERR(page);
+ goto err_node;
+ }
+
+ src = io_mapping_map_atomic_wc(i915->gtt.mappable, node.start + PAGE_SIZE * i);
+ dst = kmap_atomic(page);
+ memcpy_fromio(dst, src, PAGE_SIZE);
+ kunmap_atomic(dst);
+ io_mapping_unmap_atomic(src);
+
+ page_cache_release(page);
+ }
+
+ wmb();
+ i915->gtt.base.clear_range(&i915->gtt.base,
+ node.start, node.size,
+ true);
+ drm_mm_remove_node(&node);
+
+swap_pages:
+ stolen_pages = obj->pages;
+ obj->pages = NULL;
+
+ obj->base.filp = file;
+ obj->base.read_domains = I915_GEM_DOMAIN_CPU;
+ obj->base.write_domain = I915_GEM_DOMAIN_CPU;
+
+ /* 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;
+ }
+
+ ret = i915_gem_gtt_prepare_object(obj);
+ if (ret) {
+ i915_gem_object_put_pages_gtt(obj);
+ obj->pages = stolen_pages;
+ goto err_file;
+ }
+
+ ret = i915_gem_object_set_to_gtt_domain(obj, true);
+ if (ret) {
+ i915_gem_gtt_finish_object(obj);
+ i915_gem_object_put_pages_gtt(obj);
+ 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, vma_link) {
+ if (!drm_mm_node_allocated(&vma->node))
+ continue;
+
+ WARN_ON(i915_vma_bind(vma,
+ obj->cache_level,
+ PIN_UPDATE));
+ }
+ } else
+ 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_node:
+ wmb();
+ i915->gtt.base.clear_range(&i915->gtt.base,
+ node.start, node.size,
+ true);
+ drm_mm_remove_node(&node);
+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);
+ struct drm_i915_gem_object *obj, *tmp;
+ struct list_head *phase[] = {
+ &i915->mm.unbound_list, &i915->mm.bound_list, NULL
+ }, **p;
+
+ /* Across hibernation, the stolen area is not preserved.
+ * Anything inside stolen must copied back to normal
+ * memory if we wish to preserve it.
+ */
+ 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)
+ return ret;
+ }
+
+ return 0;
+}
+
int
i915_gem_suspend(struct drm_device *dev)
{
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d08989a..6791c18 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2517,6 +2517,9 @@ intel_alloc_initial_plane_obj(struct intel_crtc *crtc,
if (IS_ERR(obj))
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 67de958..dd2ce4d 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -148,6 +148,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)) {
ret = PTR_ERR(fb);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 12bf162..eec1131 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5396,6 +5396,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;
}
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index a928602..cd19776 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2025,6 +2025,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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH 6/6] drm/i915: Migrate stolen objects before hibernation
2015-10-08 6:24 ` [PATCH 6/6] drm/i915: Migrate stolen objects before hibernation ankitprasad.r.sharma
@ 2015-10-08 11:02 ` Chris Wilson
2015-10-13 5:25 ` Ankitprasad Sharma
2015-10-28 11:22 ` Ankitprasad Sharma
0 siblings, 2 replies; 36+ messages in thread
From: Chris Wilson @ 2015-10-08 11:02 UTC (permalink / raw)
To: ankitprasad.r.sharma; +Cc: intel-gfx, akash.goel, shashidhar.hiremath
On Thu, Oct 08, 2015 at 11:54:29AM +0530, ankitprasad.r.sharma@intel.com wrote:
> + /* stolen objects are already pinned to prevent shrinkage */
> + memset(&node, 0, sizeof(node));
> + ret = drm_mm_insert_node_in_range_generic(&i915->gtt.base.mm,
> + &node,
> + 4096, 0, I915_CACHE_NONE,
> + 0, i915->gtt.mappable_end,
> + DRM_MM_SEARCH_DEFAULT,
> + DRM_MM_CREATE_DEFAULT);
> + if (ret)
> + return ret;
> +
> + i915->gtt.base.insert_entries(&i915->gtt.base, obj->pages,
> + node.start, I915_CACHE_NONE, 0);
This was written using an insert_page() function you don't have. Either
grab that as well, or you need to pin the entire object into the GGTT,
i.e. i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE); note that to do so
will also need to be very careful to handle the pinning of obj->pages
and the introduction of a new GGTT vma.
> +
> + for (i = 0; i < obj->base.size / PAGE_SIZE; i++) {
> + struct page *page;
> + void *__iomem src;
> + void *dst;
> +
> + page = shmem_read_mapping_page(mapping, i);
> + if (IS_ERR(page)) {
> + ret = PTR_ERR(page);
> + goto err_node;
> + }
> +
> + src = io_mapping_map_atomic_wc(i915->gtt.mappable, node.start + PAGE_SIZE * i);
> + dst = kmap_atomic(page);
> + memcpy_fromio(dst, src, PAGE_SIZE);
> + kunmap_atomic(dst);
> + io_mapping_unmap_atomic(src);
> +
> + page_cache_release(page);
> + }
> +
> + wmb();
> + i915->gtt.base.clear_range(&i915->gtt.base,
> + node.start, node.size,
> + true);
> + drm_mm_remove_node(&node);
> +
> +swap_pages:
> + stolen_pages = obj->pages;
> + obj->pages = NULL;
> +
> + obj->base.filp = file;
> + obj->base.read_domains = I915_GEM_DOMAIN_CPU;
> + obj->base.write_domain = I915_GEM_DOMAIN_CPU;
> +
> + /* 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;
> + }
> +
> + ret = i915_gem_gtt_prepare_object(obj);
> + if (ret) {
> + i915_gem_object_put_pages_gtt(obj);
> + obj->pages = stolen_pages;
> + goto err_file;
> + }
> +
> + ret = i915_gem_object_set_to_gtt_domain(obj, true);
> + if (ret) {
> + i915_gem_gtt_finish_object(obj);
> + i915_gem_object_put_pages_gtt(obj);
> + 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, vma_link) {
> + if (!drm_mm_node_allocated(&vma->node))
> + continue;
> +
> + WARN_ON(i915_vma_bind(vma,
> + obj->cache_level,
> + PIN_UPDATE));
> + }
> + } else
> + 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_node:
> + wmb();
> + i915->gtt.base.clear_range(&i915->gtt.base,
> + node.start, node.size,
> + true);
> + drm_mm_remove_node(&node);
> +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);
> + struct drm_i915_gem_object *obj, *tmp;
> + struct list_head *phase[] = {
> + &i915->mm.unbound_list, &i915->mm.bound_list, NULL
> + }, **p;
> +
> + /* Across hibernation, the stolen area is not preserved.
> + * Anything inside stolen must copied back to normal
> + * memory if we wish to preserve it.
> + */
> + for (p = phase; *p; p++) {
Didn't we introduce a list of stolen objects in one of the other
patches?
> + 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)
> + return ret;
> + }
> +
> + return 0;
> +}
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 6/6] drm/i915: Migrate stolen objects before hibernation
2015-10-08 11:02 ` Chris Wilson
@ 2015-10-13 5:25 ` Ankitprasad Sharma
2015-10-28 11:22 ` Ankitprasad Sharma
1 sibling, 0 replies; 36+ messages in thread
From: Ankitprasad Sharma @ 2015-10-13 5:25 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx, akash.goel, shashidhar.hiremath
On Thu, 2015-10-08 at 12:02 +0100, Chris Wilson wrote:
> On Thu, Oct 08, 2015 at 11:54:29AM +0530, ankitprasad.r.sharma@intel.com wrote:
> > + /* stolen objects are already pinned to prevent shrinkage */
> > + memset(&node, 0, sizeof(node));
> > + ret = drm_mm_insert_node_in_range_generic(&i915->gtt.base.mm,
> > + &node,
> > + 4096, 0, I915_CACHE_NONE,
> > + 0, i915->gtt.mappable_end,
> > + DRM_MM_SEARCH_DEFAULT,
> > + DRM_MM_CREATE_DEFAULT);
> > + if (ret)
> > + return ret;
> > +
> > + i915->gtt.base.insert_entries(&i915->gtt.base, obj->pages,
> > + node.start, I915_CACHE_NONE, 0);
>
> This was written using an insert_page() function you don't have. Either
> grab that as well, or you need to pin the entire object into the GGTT,
> i.e. i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE); note that to do so
> will also need to be very careful to handle the pinning of obj->pages
> and the introduction of a new GGTT vma.
>
> > +
> > + for (i = 0; i < obj->base.size / PAGE_SIZE; i++) {
> > + struct page *page;
> > + void *__iomem src;
> > + void *dst;
> > +
> > + page = shmem_read_mapping_page(mapping, i);
> > + if (IS_ERR(page)) {
> > + ret = PTR_ERR(page);
> > + goto err_node;
> > + }
> > +
> > + src = io_mapping_map_atomic_wc(i915->gtt.mappable, node.start + PAGE_SIZE * i);
> > + dst = kmap_atomic(page);
> > + memcpy_fromio(dst, src, PAGE_SIZE);
> > + kunmap_atomic(dst);
> > + io_mapping_unmap_atomic(src);
> > +
> > + page_cache_release(page);
> > + }
> > +
> > + wmb();
> > + i915->gtt.base.clear_range(&i915->gtt.base,
> > + node.start, node.size,
> > + true);
> > + drm_mm_remove_node(&node);
> > +
> > +swap_pages:
> > + stolen_pages = obj->pages;
> > + obj->pages = NULL;
> > +
> > + obj->base.filp = file;
> > + obj->base.read_domains = I915_GEM_DOMAIN_CPU;
> > + obj->base.write_domain = I915_GEM_DOMAIN_CPU;
> > +
> > + /* 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;
> > + }
> > +
> > + ret = i915_gem_gtt_prepare_object(obj);
> > + if (ret) {
> > + i915_gem_object_put_pages_gtt(obj);
> > + obj->pages = stolen_pages;
> > + goto err_file;
> > + }
> > +
> > + ret = i915_gem_object_set_to_gtt_domain(obj, true);
> > + if (ret) {
> > + i915_gem_gtt_finish_object(obj);
> > + i915_gem_object_put_pages_gtt(obj);
> > + 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, vma_link) {
> > + if (!drm_mm_node_allocated(&vma->node))
> > + continue;
> > +
> > + WARN_ON(i915_vma_bind(vma,
> > + obj->cache_level,
> > + PIN_UPDATE));
> > + }
> > + } else
> > + 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_node:
> > + wmb();
> > + i915->gtt.base.clear_range(&i915->gtt.base,
> > + node.start, node.size,
> > + true);
> > + drm_mm_remove_node(&node);
> > +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);
> > + struct drm_i915_gem_object *obj, *tmp;
> > + struct list_head *phase[] = {
> > + &i915->mm.unbound_list, &i915->mm.bound_list, NULL
> > + }, **p;
> > +
> > + /* Across hibernation, the stolen area is not preserved.
> > + * Anything inside stolen must copied back to normal
> > + * memory if we wish to preserve it.
> > + */
> > + for (p = phase; *p; p++) {
>
> Didn't we introduce a list of stolen objects in one of the other
> patches?
Yes, but that list is only for purgeable objects.
+ /**
+ * 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;
+
Thanks,
Ankit
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH 6/6] drm/i915: Migrate stolen objects before hibernation
2015-10-08 11:02 ` Chris Wilson
2015-10-13 5:25 ` Ankitprasad Sharma
@ 2015-10-28 11:22 ` Ankitprasad Sharma
1 sibling, 0 replies; 36+ messages in thread
From: Ankitprasad Sharma @ 2015-10-28 11:22 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx, akash.goel, shashidhar.hiremath
On Thu, 2015-10-08 at 12:02 +0100, Chris Wilson wrote:
> On Thu, Oct 08, 2015 at 11:54:29AM +0530, ankitprasad.r.sharma@intel.com wrote:
> > + /* stolen objects are already pinned to prevent shrinkage */
> > + memset(&node, 0, sizeof(node));
> > + ret = drm_mm_insert_node_in_range_generic(&i915->gtt.base.mm,
> > + &node,
> > + 4096, 0, I915_CACHE_NONE,
> > + 0, i915->gtt.mappable_end,
> > + DRM_MM_SEARCH_DEFAULT,
> > + DRM_MM_CREATE_DEFAULT);
> > + if (ret)
> > + return ret;
> > +
> > + i915->gtt.base.insert_entries(&i915->gtt.base, obj->pages,
> > + node.start, I915_CACHE_NONE, 0);
>
> This was written using an insert_page() function you don't have. Either
> grab that as well, or you need to pin the entire object into the GGTT,
> i.e. i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE); note that to do so
> will also need to be very careful to handle the pinning of obj->pages
> and the introduction of a new GGTT vma.
We thought to implement the second alternative, but as you mentioned
handling the pinning of obj->pages and the introduction of a new GGTT
vma, is a bit messy.
Can you please share the insert_page() function?
Thanks,
Ankit
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 36+ messages in thread