Re: [Intel-gfx] [PATCH 3/6] drm/i915: Add support for stealing purgable stolen pages

2014-04-14 Thread Gupta, Sourab 
On Fri, 2014-04-11 at 08:55 +, Daniel Vetter wrote:
 On Thu, Apr 10, 2014 at 10:12:39AM +, Gupta, Sourab wrote:
  On Wed, 2014-04-09 at 13:06 +, Daniel Vetter wrote:
   On Tue, Apr 08, 2014 at 06:53:03AM +, Gupta, Sourab wrote:
On Tue, 2014-04-08 at 06:45 +, Chris Wilson wrote:
 On Tue, Apr 08, 2014 at 04:32:02AM +, Gupta, Sourab wrote:
  Hi Rodrigo,
  In this patch, while freeing the purgeable stolen object, the memory
  node also has to be freed, so as to make space for new object. We 
  need
  to call drm_mm_remove_node while freeing obj.
  
  The below modification patch was floated earlier for this purpose:
  http://lists.freedesktop.org/archives/intel-gfx/2014-March/041282.html
 
 Right, I have a v2 locally with the fix you identified.
 -Chris
 
Ok, Thanks Chris.
   
   I'd really prefer if someone would pick up all the
   stolen/create2_ioctl/whatever patches, pack them up into a polished
   series, add the testcases and submit this all for review and merging.
   
   Otherwise this will linger forever and we'll get nowhere. Chris seems
   swamped with other stuff, so Sourab could you please take a look at this?
   
   Please check with your manager that you have sufficient bandwidth to pull
   this through.
   
  
  I'll be on vacation from next week, so I'll be able to gauge this better
  after coming back.
  Nevertheless, I have some questions regarding the expectation of
  userspace code changes required for these patches (i.e. libdrm changes
  and igt testcases)
  
  1) For libdrm , I am assuming, a counterpart of
  drm_intel_gem_bo_alloc_tiled() function would call the create2 ioctl and
  take in the parameters needed. 
  Should the caching of objects from libdrm need to take care of both the
  placement domains seperately (as in different sets of bo buckets)?
  Should libdrm be transparent to all the combinations of different
  parameters being passed by user or should the prohibited combinations be
  disallowed from libdrm side?
 
 I'm not sure whether we need a cache implemented in libdrm. Since stolen
 objects are fairly special it's probably easier to just have a simple
 linear cache tailor-made in the respective UMD. So just exposing
 create2_ioctl should be good enough.
 
  2) For the igt, since we have a lot of parameters exposed to user, the
  number of subtests required may be huge and still they may not test out
  everything. 
  So, Is the expectation here to have exhaustive test cases for all the
  parameters (tiling/cache/domain/madvise/offset etc.) going in as input
  to the create2 ioctl?
  For eg. let us say we are going to check the render copy operation of
  src and dest bo's. Do we need to provide all possible combinations of
  different (create2 ioctl) input parameters to these src and dest bo's
  and then run the render copy test for all these combinations.
  Any guiding pointers from your side as to how we may go about the igt
  testcases?
 
 At a high-level there's two parts for igt tests:
 - First is functional tests, where we try to make sure that the feature
   actually works. I.e. allocate some stolen memory and then do something
   with it, making sure that data access for the gpu and similar things
   work. For this we just want some reasonable base coverage so that when
   we hit a bug somewhere it's easy to extend the testcase to cover that
   bug with a specific subtest.
 
 - Then there's interface testing. kernel/userspace is a trust barrier, so
   we need to make sure that evil userspace can't make the kernel crash
   with some crazy invalid combination of flags or operations (like create
   a stolen object and then try to mmap it). Since this is security
   relevant and also since we can't ever change established userspace ABI I
   want full coverage of all cases. But this is just about detecting abuse
   correctly, no functional tests here.
 
 For details see my two blog posts on the topic:
 
 http://blog.ffwll.ch/2013/11/botching-up-ioctls.html
 
 http://blog.ffwll.ch/2013/11/testing-requirements-for-drmi915.html
 
 Cheers, Daniel

Thanks Daniel,
We'll take care of the above points for libdrm changes and igt.

Regards,
Sourab

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/6] drm/i915: Add support for stealing purgable stolen pages

2014-04-11 Thread Daniel Vetter 
On Thu, Apr 10, 2014 at 10:12:39AM +, Gupta, Sourab wrote:
 On Wed, 2014-04-09 at 13:06 +, Daniel Vetter wrote:
  On Tue, Apr 08, 2014 at 06:53:03AM +, Gupta, Sourab wrote:
   On Tue, 2014-04-08 at 06:45 +, Chris Wilson wrote:
On Tue, Apr 08, 2014 at 04:32:02AM +, Gupta, Sourab wrote:
 Hi Rodrigo,
 In this patch, while freeing the purgeable stolen object, the memory
 node also has to be freed, so as to make space for new object. We need
 to call drm_mm_remove_node while freeing obj.
 
 The below modification patch was floated earlier for this purpose:
 http://lists.freedesktop.org/archives/intel-gfx/2014-March/041282.html

Right, I have a v2 locally with the fix you identified.
-Chris

   Ok, Thanks Chris.
  
  I'd really prefer if someone would pick up all the
  stolen/create2_ioctl/whatever patches, pack them up into a polished
  series, add the testcases and submit this all for review and merging.
  
  Otherwise this will linger forever and we'll get nowhere. Chris seems
  swamped with other stuff, so Sourab could you please take a look at this?
  
  Please check with your manager that you have sufficient bandwidth to pull
  this through.
  
 
 I'll be on vacation from next week, so I'll be able to gauge this better
 after coming back.
 Nevertheless, I have some questions regarding the expectation of
 userspace code changes required for these patches (i.e. libdrm changes
 and igt testcases)
 
 1) For libdrm , I am assuming, a counterpart of
 drm_intel_gem_bo_alloc_tiled() function would call the create2 ioctl and
 take in the parameters needed. 
 Should the caching of objects from libdrm need to take care of both the
 placement domains seperately (as in different sets of bo buckets)?
 Should libdrm be transparent to all the combinations of different
 parameters being passed by user or should the prohibited combinations be
 disallowed from libdrm side?

I'm not sure whether we need a cache implemented in libdrm. Since stolen
objects are fairly special it's probably easier to just have a simple
linear cache tailor-made in the respective UMD. So just exposing
create2_ioctl should be good enough.

 2) For the igt, since we have a lot of parameters exposed to user, the
 number of subtests required may be huge and still they may not test out
 everything. 
 So, Is the expectation here to have exhaustive test cases for all the
 parameters (tiling/cache/domain/madvise/offset etc.) going in as input
 to the create2 ioctl?
 For eg. let us say we are going to check the render copy operation of
 src and dest bo's. Do we need to provide all possible combinations of
 different (create2 ioctl) input parameters to these src and dest bo's
 and then run the render copy test for all these combinations.
 Any guiding pointers from your side as to how we may go about the igt
 testcases?

At a high-level there's two parts for igt tests:
- First is functional tests, where we try to make sure that the feature
  actually works. I.e. allocate some stolen memory and then do something
  with it, making sure that data access for the gpu and similar things
  work. For this we just want some reasonable base coverage so that when
  we hit a bug somewhere it's easy to extend the testcase to cover that
  bug with a specific subtest.

- Then there's interface testing. kernel/userspace is a trust barrier, so
  we need to make sure that evil userspace can't make the kernel crash
  with some crazy invalid combination of flags or operations (like create
  a stolen object and then try to mmap it). Since this is security
  relevant and also since we can't ever change established userspace ABI I
  want full coverage of all cases. But this is just about detecting abuse
  correctly, no functional tests here.

For details see my two blog posts on the topic:

http://blog.ffwll.ch/2013/11/botching-up-ioctls.html

http://blog.ffwll.ch/2013/11/testing-requirements-for-drmi915.html

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/6] drm/i915: Add support for stealing purgable stolen pages

2014-04-10 Thread Gupta, Sourab 
On Wed, 2014-04-09 at 13:06 +, Daniel Vetter wrote:
 On Tue, Apr 08, 2014 at 06:53:03AM +, Gupta, Sourab wrote:
  On Tue, 2014-04-08 at 06:45 +, Chris Wilson wrote:
   On Tue, Apr 08, 2014 at 04:32:02AM +, Gupta, Sourab wrote:
Hi Rodrigo,
In this patch, while freeing the purgeable stolen object, the memory
node also has to be freed, so as to make space for new object. We need
to call drm_mm_remove_node while freeing obj.

The below modification patch was floated earlier for this purpose:
http://lists.freedesktop.org/archives/intel-gfx/2014-March/041282.html
   
   Right, I have a v2 locally with the fix you identified.
   -Chris
   
  Ok, Thanks Chris.
 
 I'd really prefer if someone would pick up all the
 stolen/create2_ioctl/whatever patches, pack them up into a polished
 series, add the testcases and submit this all for review and merging.
 
 Otherwise this will linger forever and we'll get nowhere. Chris seems
 swamped with other stuff, so Sourab could you please take a look at this?
 
 Please check with your manager that you have sufficient bandwidth to pull
 this through.
 

I'll be on vacation from next week, so I'll be able to gauge this better
after coming back.
Nevertheless, I have some questions regarding the expectation of
userspace code changes required for these patches (i.e. libdrm changes
and igt testcases)

1) For libdrm , I am assuming, a counterpart of
drm_intel_gem_bo_alloc_tiled() function would call the create2 ioctl and
take in the parameters needed. 
Should the caching of objects from libdrm need to take care of both the
placement domains seperately (as in different sets of bo buckets)?
Should libdrm be transparent to all the combinations of different
parameters being passed by user or should the prohibited combinations be
disallowed from libdrm side?

2) For the igt, since we have a lot of parameters exposed to user, the
number of subtests required may be huge and still they may not test out
everything. 
So, Is the expectation here to have exhaustive test cases for all the
parameters (tiling/cache/domain/madvise/offset etc.) going in as input
to the create2 ioctl?
For eg. let us say we are going to check the render copy operation of
src and dest bo's. Do we need to provide all possible combinations of
different (create2 ioctl) input parameters to these src and dest bo's
and then run the render copy test for all these combinations.
Any guiding pointers from your side as to how we may go about the igt
testcases?

Thanks,
sourab

 Rodrigo, I think you can drop this patch from -collector, it only really
 makes sense in the context of all the other stolen work.
 
 Thanks, Daniel

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/6] drm/i915: Add support for stealing purgable stolen pages

2014-04-08 Thread Chris Wilson 
On Tue, Apr 08, 2014 at 04:32:02AM +, Gupta, Sourab wrote:
 Hi Rodrigo,
 In this patch, while freeing the purgeable stolen object, the memory
 node also has to be freed, so as to make space for new object. We need
 to call drm_mm_remove_node while freeing obj.
 
 The below modification patch was floated earlier for this purpose:
 http://lists.freedesktop.org/archives/intel-gfx/2014-March/041282.html

Right, I have a v2 locally with the fix you identified.
-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

Re: [Intel-gfx] [PATCH 3/6] drm/i915: Add support for stealing purgable stolen pages

2014-04-08 Thread Gupta, Sourab 
On Tue, 2014-04-08 at 06:45 +, Chris Wilson wrote:
 On Tue, Apr 08, 2014 at 04:32:02AM +, Gupta, Sourab wrote:
  Hi Rodrigo,
  In this patch, while freeing the purgeable stolen object, the memory
  node also has to be freed, so as to make space for new object. We need
  to call drm_mm_remove_node while freeing obj.
  
  The below modification patch was floated earlier for this purpose:
  http://lists.freedesktop.org/archives/intel-gfx/2014-March/041282.html
 
 Right, I have a v2 locally with the fix you identified.
 -Chris
 
Ok, Thanks Chris.

Regards,
Sourab

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 3/6] drm/i915: Add support for stealing purgable stolen pages

2014-04-07 Thread Rodrigo Vivi 
From: Chris Wilson ch...@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.

Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Gupta, Sourab sourab.gu...@intel.com
Cc: Goel, Akash akash.g...@intel.com
Signed-off-by: Rodrigo Vivi rodrigo.v...@gmail.com
---
 drivers/gpu/drm/i915/i915_gem_stolen.c | 119 ++---
 1 file changed, 108 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c 
b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 62ef55b..1fc1986 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -329,18 +329,25 @@ cleanup:
return NULL;
 }
 
-struct drm_i915_gem_object *
-i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
+static bool mark_free(struct drm_i915_gem_object *obj, struct list_head 
*unwind)
+{
+   if (obj-stolen == NULL)
+   return false;
+
+   if (obj-madv != I915_MADV_DONTNEED)
+   return false;
+
+   list_add(obj-obj_exec_link, unwind);
+   return drm_mm_scan_add_block(obj-stolen);
+}
+
+static struct drm_mm_node *stolen_alloc(struct drm_i915_private *dev_priv, u32 
size)
 {
-   struct drm_i915_private *dev_priv = dev-dev_private;
-   struct drm_i915_gem_object *obj;
struct drm_mm_node *stolen;
+   struct drm_i915_gem_object *obj;
+   struct list_head unwind, evict;
int ret;
 
-   if (!drm_mm_initialized(dev_priv-mm.stolen))
-   return NULL;
-
-   DRM_DEBUG_KMS(creating stolen object: size=%x\n, size);
if (size == 0)
return NULL;
 
@@ -350,11 +357,101 @@ i915_gem_object_create_stolen(struct drm_device *dev, 
u32 size)
 
ret = drm_mm_insert_node(dev_priv-mm.stolen, stolen, size,
 4096, DRM_MM_SEARCH_DEFAULT);
-   if (ret) {
-   kfree(stolen);
-   return NULL;
+   if (ret == 0)
+   return stolen;
+
+   /* No more stolen memory available, or too fragmented.
+* Try evicting purgeable objects and search again.
+*/
+
+   drm_mm_init_scan(dev_priv-mm.stolen, size, 4096, 0);
+   INIT_LIST_HEAD(unwind);
+
+   list_for_each_entry(obj, dev_priv-mm.unbound_list, global_list)
+   if (mark_free(obj, unwind))
+   goto found;
+
+   list_for_each_entry(obj, dev_priv-mm.bound_list, global_list)
+   if (mark_free(obj, unwind))
+   goto found;
+
+found:
+   INIT_LIST_HEAD(evict);
+   while (!list_empty(unwind)) {
+   obj = list_first_entry(unwind,
+  struct drm_i915_gem_object,
+  obj_exec_link);
+   list_del_init(obj-obj_exec_link);
+
+   if (drm_mm_scan_remove_block(obj-stolen)) {
+   list_add(obj-obj_exec_link, evict);
+   drm_gem_object_reference(obj-base);
+   }
}
 
+   ret = 0;
+   while (!list_empty(evict)) {
+   obj = list_first_entry(evict,
+  struct drm_i915_gem_object,
+  obj_exec_link);
+   list_del_init(obj-obj_exec_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) {
+   obj-madv = __I915_MADV_PURGED;
+
+   kfree(obj-stolen);
+   obj-stolen = NULL;
+   } else
+   i915_gem_object_pin_pages(obj);
+   }
+
+

Re: [Intel-gfx] [PATCH 3/6] drm/i915: Add support for stealing purgable stolen pages

2014-04-07 Thread Gupta, Sourab 
On Mon, 2014-04-07 at 20:01 +, Rodrigo Vivi wrote:
 From: Chris Wilson ch...@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.
 
 Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
 Cc: Gupta, Sourab sourab.gu...@intel.com
 Cc: Goel, Akash akash.g...@intel.com
 Signed-off-by: Rodrigo Vivi rodrigo.v...@gmail.com
 ---
  drivers/gpu/drm/i915/i915_gem_stolen.c | 119 
 ++---
  1 file changed, 108 insertions(+), 11 deletions(-)
 
 diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c 
 b/drivers/gpu/drm/i915/i915_gem_stolen.c
 index 62ef55b..1fc1986 100644
 --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
 +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
 @@ -329,18 +329,25 @@ cleanup:
   return NULL;
  }
  
 -struct drm_i915_gem_object *
 -i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
 +static bool mark_free(struct drm_i915_gem_object *obj, struct list_head 
 *unwind)
 +{
 + if (obj-stolen == NULL)
 + return false;
 +
 + if (obj-madv != I915_MADV_DONTNEED)
 + return false;
 +
 + list_add(obj-obj_exec_link, unwind);
 + return drm_mm_scan_add_block(obj-stolen);
 +}
 +
 +static struct drm_mm_node *stolen_alloc(struct drm_i915_private *dev_priv, 
 u32 size)
  {
 - struct drm_i915_private *dev_priv = dev-dev_private;
 - struct drm_i915_gem_object *obj;
   struct drm_mm_node *stolen;
 + struct drm_i915_gem_object *obj;
 + struct list_head unwind, evict;
   int ret;
  
 - if (!drm_mm_initialized(dev_priv-mm.stolen))
 - return NULL;
 -
 - DRM_DEBUG_KMS(creating stolen object: size=%x\n, size);
   if (size == 0)
   return NULL;
  
 @@ -350,11 +357,101 @@ i915_gem_object_create_stolen(struct drm_device *dev, 
 u32 size)
  
   ret = drm_mm_insert_node(dev_priv-mm.stolen, stolen, size,
4096, DRM_MM_SEARCH_DEFAULT);
 - if (ret) {
 - kfree(stolen);
 - return NULL;
 + if (ret == 0)
 + return stolen;
 +
 + /* No more stolen memory available, or too fragmented.
 +  * Try evicting purgeable objects and search again.
 +  */
 +
 + drm_mm_init_scan(dev_priv-mm.stolen, size, 4096, 0);
 + INIT_LIST_HEAD(unwind);
 +
 + list_for_each_entry(obj, dev_priv-mm.unbound_list, global_list)
 + if (mark_free(obj, unwind))
 + goto found;
 +
 + list_for_each_entry(obj, dev_priv-mm.bound_list, global_list)
 + if (mark_free(obj, unwind))
 + goto found;
 +
 +found:
 + INIT_LIST_HEAD(evict);
 + while (!list_empty(unwind)) {
 + obj = list_first_entry(unwind,
 +struct drm_i915_gem_object,
 +obj_exec_link);
 + list_del_init(obj-obj_exec_link);
 +
 + if (drm_mm_scan_remove_block(obj-stolen)) {
 + list_add(obj-obj_exec_link, evict);
 + drm_gem_object_reference(obj-base);
 + }
   }
  
 + ret = 0;
 + while (!list_empty(evict)) {
 + obj = list_first_entry(evict,
 +struct drm_i915_gem_object,
 +obj_exec_link);
 + list_del_init(obj-obj_exec_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) {
 + obj-madv = __I915_MADV_PURGED;
 +
 + kfree(obj-stolen);
 + obj-stolen = NULL;
 + } else
 +