Re: [Intel-gfx] [PATCH v6] drm/i915: Add soft-pinning API for execbuffer
On Wed, Dec 02, 2015 at 11:28:52AM +, Tvrtko Ursulin wrote: > This will return ENOSPC to userspace when they have passed in an > address outside the (PP)GTT range which is misleading. Reviewing the wrong patch. -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 v6] drm/i915: Add soft-pinning API for execbuffer
On 16/10/15 11:59, Thomas Daniel wrote:
From: Chris Wilson
Userspace can pass in an offset that it presumes the object is located
at. The kernel will then do its utmost to fit the object into that
location. The assumption is that userspace is handling its own object
locations (for example along with full-ppgtt) and that the kernel will
rarely have to make space for the user's requests.
Signed-off-by: Chris Wilson
v2: Fixed incorrect eviction found by Michal Winiarski - fix suggested by Chris
Wilson. Fixed incorrect error paths causing crash found by Michal Winiarski.
(Not published externally)
v3: Rebased because of trivial conflict in object_bind_to_vm. Fixed eviction
to allow eviction of soft-pinned objects when another soft-pinned object used
by a subsequent execbuffer overlaps reported by Michal Winiarski.
(Not published externally)
v4: Moved soft-pinned objects to the front of ordered_vmas so that they are
pinned first after an address conflict happens to avoid repeated conflicts in
rare cases (Suggested by Chris Wilson). Expanded comment on
drm_i915_gem_exec_object2.offset to cover this new API.
v5: Added I915_PARAM_HAS_EXEC_SOFTPIN parameter for detecting this capability
(Kristian). Added check for multiple pinnings on eviction (Akash). Made sure
buffers are not considered misplaced without the user specifying
EXEC_OBJECT_SUPPORTS_48B_ADDRESS. User must assume responsibility for any
addressing workarounds. Updated object2.offset field comment again to clarify
NO_RELOC case (Chris). checkpatch cleanup.
v6: Trivial rebase on latest drm-intel-nightly
Cc: Chris Wilson
Cc: Akash Goel
Cc: Vinay Belgaumkar
Cc: Michal Winiarski
Cc: Zou Nanhai
Cc: Kristian Høgsberg
Signed-off-by: Thomas Daniel
---
drivers/gpu/drm/i915/i915_dma.c| 3 ++
drivers/gpu/drm/i915/i915_drv.h| 2 +
drivers/gpu/drm/i915/i915_gem.c| 64 --
drivers/gpu/drm/i915/i915_gem_evict.c | 39 ++
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 19 +++--
include/uapi/drm/i915_drm.h| 12 --
6 files changed, 113 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 2336af9..824c6c3 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_HAS_EXEC_SOFTPIN:
+ value = 1;
+ 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 1396af9..73c3acf 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2816,6 +2816,7 @@ void i915_gem_vma_destroy(struct i915_vma *vma);
#define PIN_UPDATE(1<<5)
#define PIN_ZONE_4G (1<<6)
#define PIN_HIGH (1<<7)
+#define PIN_OFFSET_FIXED (1<<8)
#define PIN_OFFSET_MASK (~4095)
int __must_check
i915_gem_object_pin(struct drm_i915_gem_object *obj,
@@ -3163,6 +3164,7 @@ int __must_check i915_gem_evict_something(struct
drm_device *dev,
unsigned long start,
unsigned long end,
unsigned flags);
+int __must_check i915_gem_evict_for_vma(struct i915_vma *target);
int i915_gem_evict_vm(struct i915_address_space *vm, bool do_idle);
/* belongs in i915_gem_gtt.h */
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1e67484..c3453bd 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3450,30 +3450,50 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object
*obj,
if (IS_ERR(vma))
goto err_unpin;
- if (flags & PIN_HIGH) {
- search_flag = DRM_MM_SEARCH_BELOW;
- alloc_flag = DRM_MM_CREATE_TOP;
+ if (flags & PIN_OFFSET_FIXED) {
+ uint64_t offset = flags & PIN_OFFSET_MASK;
+
+ if (offset & (alignment - 1)) {
+ ret = -EINVAL;
+ goto err_free_vma;
+ }
+ vma->node.start = offset;
+ vma->node.size = size;
+ vma->node.color = obj->cache_level;
+ ret = drm_mm_reserve_node(&vm->mm, &vma->node);
This will return ENOSPC to userspace when they have passed in an address
outside the (PP)GTT range which is misleading.
I would suggest explicit checking against the 'end' (available a bit
further up in the same function) and returning something more
appropriate. At least EINVAL, although execbuf overloads that one so
much I would even rather steal some other semi-appro
Re: [Intel-gfx] [PATCH v6] drm/i915: Add soft-pinning API for execbuffer
On Fri, Oct 16, 2015 at 07:39:20PM +0530, Goel, Akash wrote: > > Discussed sometime back about this patch with Chris. He mainly has 2 > concerns with it. > 1. The linear walk used by the patch to detect the overlapping > objects would be expensive. > 2. Restriction to disallow !RCS submissions for non-default > contexts, which could lead to lot of conflicts for the placements, > once multiple engines like media/blit/vebox are used > > Both of them can be addressed by the subsequent patches. > Chris already has the patch ready to reduce the validation overhead > with the use of rbtree and there should be no implications of > allowing the non-default contexts for !RCS submissions in > execbuffer. > > In the standalone form, the patch looks good, so > Reviewed-by: "Akash Goel " I am sorry, but this patch does not reflect the final version that I wrote, in particular does not provide the support I actually use inside the kernel. -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 v6] drm/i915: Add soft-pinning API for execbuffer
Discussed sometime back about this patch with Chris. He mainly has 2
concerns with it.
1. The linear walk used by the patch to detect the overlapping objects
would be expensive.
2. Restriction to disallow !RCS submissions for non-default contexts,
which could lead to lot of conflicts for the placements, once multiple
engines like media/blit/vebox are used
Both of them can be addressed by the subsequent patches.
Chris already has the patch ready to reduce the validation overhead with
the use of rbtree and there should be no implications of allowing the
non-default contexts for !RCS submissions in execbuffer.
In the standalone form, the patch looks good, so
Reviewed-by: "Akash Goel "
On 10/16/2015 4:29 PM, Thomas Daniel wrote:
From: Chris Wilson
Userspace can pass in an offset that it presumes the object is located
at. The kernel will then do its utmost to fit the object into that
location. The assumption is that userspace is handling its own object
locations (for example along with full-ppgtt) and that the kernel will
rarely have to make space for the user's requests.
Signed-off-by: Chris Wilson
v2: Fixed incorrect eviction found by Michal Winiarski - fix suggested by Chris
Wilson. Fixed incorrect error paths causing crash found by Michal Winiarski.
(Not published externally)
v3: Rebased because of trivial conflict in object_bind_to_vm. Fixed eviction
to allow eviction of soft-pinned objects when another soft-pinned object used
by a subsequent execbuffer overlaps reported by Michal Winiarski.
(Not published externally)
v4: Moved soft-pinned objects to the front of ordered_vmas so that they are
pinned first after an address conflict happens to avoid repeated conflicts in
rare cases (Suggested by Chris Wilson). Expanded comment on
drm_i915_gem_exec_object2.offset to cover this new API.
v5: Added I915_PARAM_HAS_EXEC_SOFTPIN parameter for detecting this capability
(Kristian). Added check for multiple pinnings on eviction (Akash). Made sure
buffers are not considered misplaced without the user specifying
EXEC_OBJECT_SUPPORTS_48B_ADDRESS. User must assume responsibility for any
addressing workarounds. Updated object2.offset field comment again to clarify
NO_RELOC case (Chris). checkpatch cleanup.
v6: Trivial rebase on latest drm-intel-nightly
Cc: Chris Wilson
Cc: Akash Goel
Cc: Vinay Belgaumkar
Cc: Michal Winiarski
Cc: Zou Nanhai
Cc: Kristian Høgsberg
Signed-off-by: Thomas Daniel
---
drivers/gpu/drm/i915/i915_dma.c| 3 ++
drivers/gpu/drm/i915/i915_drv.h| 2 +
drivers/gpu/drm/i915/i915_gem.c| 64 --
drivers/gpu/drm/i915/i915_gem_evict.c | 39 ++
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 19 +++--
include/uapi/drm/i915_drm.h| 12 --
6 files changed, 113 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 2336af9..824c6c3 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_HAS_EXEC_SOFTPIN:
+ value = 1;
+ 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 1396af9..73c3acf 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2816,6 +2816,7 @@ void i915_gem_vma_destroy(struct i915_vma *vma);
#define PIN_UPDATE(1<<5)
#define PIN_ZONE_4G (1<<6)
#define PIN_HIGH (1<<7)
+#define PIN_OFFSET_FIXED (1<<8)
#define PIN_OFFSET_MASK (~4095)
int __must_check
i915_gem_object_pin(struct drm_i915_gem_object *obj,
@@ -3163,6 +3164,7 @@ int __must_check i915_gem_evict_something(struct
drm_device *dev,
unsigned long start,
unsigned long end,
unsigned flags);
+int __must_check i915_gem_evict_for_vma(struct i915_vma *target);
int i915_gem_evict_vm(struct i915_address_space *vm, bool do_idle);
/* belongs in i915_gem_gtt.h */
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1e67484..c3453bd 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3450,30 +3450,50 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object
*obj,
if (IS_ERR(vma))
goto err_unpin;
- if (flags & PIN_HIGH) {
- search_flag = DRM_MM_SEARCH_BELOW;
- alloc_flag = DRM_MM_CREATE_TOP;
+ if (flags & PIN_OFFSET_FIXED) {
+ uint64_t offset = flags & PIN_OFFSET_MASK;
+
+ if
[Intel-gfx] [PATCH v6] drm/i915: Add soft-pinning API for execbuffer
From: Chris Wilson
Userspace can pass in an offset that it presumes the object is located
at. The kernel will then do its utmost to fit the object into that
location. The assumption is that userspace is handling its own object
locations (for example along with full-ppgtt) and that the kernel will
rarely have to make space for the user's requests.
Signed-off-by: Chris Wilson
v2: Fixed incorrect eviction found by Michal Winiarski - fix suggested by Chris
Wilson. Fixed incorrect error paths causing crash found by Michal Winiarski.
(Not published externally)
v3: Rebased because of trivial conflict in object_bind_to_vm. Fixed eviction
to allow eviction of soft-pinned objects when another soft-pinned object used
by a subsequent execbuffer overlaps reported by Michal Winiarski.
(Not published externally)
v4: Moved soft-pinned objects to the front of ordered_vmas so that they are
pinned first after an address conflict happens to avoid repeated conflicts in
rare cases (Suggested by Chris Wilson). Expanded comment on
drm_i915_gem_exec_object2.offset to cover this new API.
v5: Added I915_PARAM_HAS_EXEC_SOFTPIN parameter for detecting this capability
(Kristian). Added check for multiple pinnings on eviction (Akash). Made sure
buffers are not considered misplaced without the user specifying
EXEC_OBJECT_SUPPORTS_48B_ADDRESS. User must assume responsibility for any
addressing workarounds. Updated object2.offset field comment again to clarify
NO_RELOC case (Chris). checkpatch cleanup.
v6: Trivial rebase on latest drm-intel-nightly
Cc: Chris Wilson
Cc: Akash Goel
Cc: Vinay Belgaumkar
Cc: Michal Winiarski
Cc: Zou Nanhai
Cc: Kristian Høgsberg
Signed-off-by: Thomas Daniel
---
drivers/gpu/drm/i915/i915_dma.c| 3 ++
drivers/gpu/drm/i915/i915_drv.h| 2 +
drivers/gpu/drm/i915/i915_gem.c| 64 --
drivers/gpu/drm/i915/i915_gem_evict.c | 39 ++
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 19 +++--
include/uapi/drm/i915_drm.h| 12 --
6 files changed, 113 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 2336af9..824c6c3 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_HAS_EXEC_SOFTPIN:
+ value = 1;
+ 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 1396af9..73c3acf 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2816,6 +2816,7 @@ void i915_gem_vma_destroy(struct i915_vma *vma);
#define PIN_UPDATE (1<<5)
#define PIN_ZONE_4G(1<<6)
#define PIN_HIGH (1<<7)
+#define PIN_OFFSET_FIXED (1<<8)
#define PIN_OFFSET_MASK (~4095)
int __must_check
i915_gem_object_pin(struct drm_i915_gem_object *obj,
@@ -3163,6 +3164,7 @@ int __must_check i915_gem_evict_something(struct
drm_device *dev,
unsigned long start,
unsigned long end,
unsigned flags);
+int __must_check i915_gem_evict_for_vma(struct i915_vma *target);
int i915_gem_evict_vm(struct i915_address_space *vm, bool do_idle);
/* belongs in i915_gem_gtt.h */
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1e67484..c3453bd 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3450,30 +3450,50 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object
*obj,
if (IS_ERR(vma))
goto err_unpin;
- if (flags & PIN_HIGH) {
- search_flag = DRM_MM_SEARCH_BELOW;
- alloc_flag = DRM_MM_CREATE_TOP;
+ if (flags & PIN_OFFSET_FIXED) {
+ uint64_t offset = flags & PIN_OFFSET_MASK;
+
+ if (offset & (alignment - 1)) {
+ ret = -EINVAL;
+ goto err_free_vma;
+ }
+ vma->node.start = offset;
+ vma->node.size = size;
+ vma->node.color = obj->cache_level;
+ ret = drm_mm_reserve_node(&vm->mm, &vma->node);
+ if (ret) {
+ ret = i915_gem_evict_for_vma(vma);
+ if (ret == 0)
+ ret = drm_mm_reserve_node(&vm->mm, &vma->node);
+ }
+ if (ret)
+ goto err_free_vma;
} else {
- search_flag = DRM_MM_SEARCH_DEFAULT;
- alloc_flag = DRM_MM_CREATE_DEFAULT;
- }
+
