We want to be able to explicitly migrate objects between gem memory regions, initially for display and dma-buf, but there might be more use-cases coming up.
Introduce a gem migrate interface, add a selftest and use it for display fb pinning and dma-buf mapping.
This series should make the desktop light up on DG1 with DG1-enabled mesa.
Matthew Auld (1): drm/i915/gem: Introduce a selftest for the gem object migrate functionality
Thomas Hellström (3): drm/i915/gem: Implement object migration drm/i915/display: Migrate objects to LMEM if possible for display drm/i915/gem: Migrate to system at dma-buf map time
drivers/gpu/drm/i915/display/intel_display.c | 5 +- drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 9 +- drivers/gpu/drm/i915/gem/i915_gem_domain.c | 2 +- drivers/gpu/drm/i915/gem/i915_gem_lmem.c | 21 -- drivers/gpu/drm/i915/gem/i915_gem_object.c | 92 +++++++ drivers/gpu/drm/i915/gem/i915_gem_object.h | 12 +- .../gpu/drm/i915/gem/i915_gem_object_types.h | 9 + drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 69 +++-- drivers/gpu/drm/i915/gem/i915_gem_wait.c | 19 ++ .../drm/i915/gem/selftests/i915_gem_migrate.c | 237 ++++++++++++++++++ .../drm/i915/selftests/i915_live_selftests.h | 1 + 11 files changed, 434 insertions(+), 42 deletions(-) create mode 100644 drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c
Introduce an interface to migrate objects between regions. This is primarily intended to migrate objects to LMEM for display and to SYSTEM for dma-buf, but might be reused in one form or another for performande-based migration.
Signed-off-by: Thomas Hellström thomas.hellstrom@linux.intel.com --- drivers/gpu/drm/i915/gem/i915_gem_object.c | 91 +++++++++++++++++++ drivers/gpu/drm/i915/gem/i915_gem_object.h | 12 +++ .../gpu/drm/i915/gem/i915_gem_object_types.h | 9 ++ drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 69 ++++++++++---- drivers/gpu/drm/i915/gem/i915_gem_wait.c | 19 ++++ 5 files changed, 183 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c index 07e8ff9a8aae..6421c3a8b2f3 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c @@ -513,6 +513,97 @@ bool i915_gem_object_has_iomem(const struct drm_i915_gem_object *obj) return obj->mem_flags & I915_BO_FLAG_IOMEM; }
+/** + * i915_gem_object_can_migrate - Whether an object likely can be migrated + * + * @obj: The object to migrate + * @id: The region intended to migrate to + * + * Check whether the object backend supports migration to the + * given region. Note that pinning may affect the ability to migrate. + * + * Return: true if migration is possible, false otherwise. + */ +bool i915_gem_object_can_migrate(struct drm_i915_gem_object *obj, + enum intel_region_id id) +{ + struct drm_i915_private *i915 = to_i915(obj->base.dev); + unsigned int num_allowed = obj->mm.n_placements; + struct intel_memory_region *mr; + unsigned int i; + + GEM_BUG_ON(id >= INTEL_REGION_UNKNOWN); + GEM_BUG_ON(obj->mm.madv != I915_MADV_WILLNEED); + + if (!obj->ops->migrate) + return -EOPNOTSUPP; + + mr = i915->mm.regions[id]; + if (obj->mm.region == mr) + return true; + + if (!i915_gem_object_evictable(obj)) + return false; + + if (!(obj->flags & I915_BO_ALLOC_USER)) + return true; + + if (num_allowed == 0) + return false; + + for (i = 0; i < num_allowed; ++i) { + if (mr == obj->mm.placements[i]) + return true; + } + + return false; +} + +/** + * i915_gem_object_migrate - Migrate an object to the desired region id + * @obj: The object to migrate. + * @ww: An optional struct i915_gem_ww_ctx. If NULL, the backend may + * not be successful in evicting other objects to make room for this object. + * @id: The region id to migrate to. + * + * Attempt to migrate the object to the desired memory region. The + * object backend must support migration and the object may not be + * pinned, (explicitly pinned pages or pinned vmas). The object must + * be locked. + * On successful completion, the object will have pages pointing to + * memory in the new region, but an async migration task may not have + * completed yet, and to accomplish that, i915_gem_object_wait_migration() + * must be called. + * + * Return: 0 on success. Negative error code on failure. In particular may + * return -ENXIO on lack of region space, -EDEADLK for deadlock avoidance + * if @ww is set, -EINTR or -ERESTARTSYS if signal pending, and + * -EBUSY if the object is pinned. + */ +int i915_gem_object_migrate(struct drm_i915_gem_object *obj, + struct i915_gem_ww_ctx *ww, + enum intel_region_id id) +{ + struct drm_i915_private *i915 = to_i915(obj->base.dev); + struct intel_memory_region *mr; + + GEM_BUG_ON(id >= INTEL_REGION_UNKNOWN); + GEM_BUG_ON(obj->mm.madv != I915_MADV_WILLNEED); + assert_object_held(obj); + + mr = i915->mm.regions[id]; + if (obj->mm.region == mr) + return 0; + + if (!i915_gem_object_evictable(obj)) + return -EBUSY; + + if (!obj->ops->migrate) + return -EOPNOTSUPP; + + return obj->ops->migrate(obj, mr); +} + void i915_gem_init__objects(struct drm_i915_private *i915) { INIT_WORK(&i915->mm.free_work, __i915_gem_free_work); diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h index ea3224a480c4..8cbd7a5334e2 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h @@ -17,6 +17,8 @@ #include "i915_gem_ww.h" #include "i915_vma_types.h"
+enum intel_region_id; + /* * XXX: There is a prevalence of the assumption that we fit the * object's page count inside a 32bit _signed_ variable. Let's document @@ -597,6 +599,16 @@ bool i915_gem_object_migratable(struct drm_i915_gem_object *obj);
bool i915_gem_object_validates_to_lmem(struct drm_i915_gem_object *obj);
+int i915_gem_object_migrate(struct drm_i915_gem_object *obj, + struct i915_gem_ww_ctx *ww, + enum intel_region_id id); + +bool i915_gem_object_can_migrate(struct drm_i915_gem_object *obj, + enum intel_region_id id); + +int i915_gem_object_wait_migration(struct drm_i915_gem_object *obj, + unsigned int flags); + #ifdef CONFIG_MMU_NOTIFIER static inline bool i915_gem_object_is_userptr(struct drm_i915_gem_object *obj) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h index 441f913c87e6..ef3de2ae9723 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h @@ -18,6 +18,7 @@
struct drm_i915_gem_object; struct intel_fronbuffer; +struct intel_memory_region;
/* * struct i915_lut_handle tracks the fast lookups from handle to vma used @@ -77,6 +78,14 @@ struct drm_i915_gem_object_ops { * delayed_free - Override the default delayed free implementation */ void (*delayed_free)(struct drm_i915_gem_object *obj); + + /** + * migrate - Migrate object to a different region either for + * pinning or for as long as the object lock is held. + */ + int (*migrate)(struct drm_i915_gem_object *obj, + struct intel_memory_region *mr); + void (*release)(struct drm_i915_gem_object *obj);
const struct vm_operations_struct *mmap_ops; diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c index c39d982c4fa6..b01bf0650ab0 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c @@ -617,7 +617,8 @@ struct ttm_device_funcs *i915_ttm_driver(void) return &i915_ttm_bo_driver; }
-static int i915_ttm_get_pages(struct drm_i915_gem_object *obj) +static int __i915_ttm_get_pages(struct drm_i915_gem_object *obj, + struct ttm_placement *placement) { struct ttm_buffer_object *bo = i915_gem_to_ttm(obj); struct ttm_operation_ctx ctx = { @@ -625,19 +626,12 @@ static int i915_ttm_get_pages(struct drm_i915_gem_object *obj) .no_wait_gpu = false, }; struct sg_table *st; - struct ttm_place requested, busy[I915_TTM_MAX_PLACEMENTS]; - struct ttm_placement placement; int real_num_busy; int ret;
- GEM_BUG_ON(obj->mm.n_placements > I915_TTM_MAX_PLACEMENTS); - - /* Move to the requested placement. */ - i915_ttm_placement_from_obj(obj, &requested, busy, &placement); - /* First try only the requested placement. No eviction. */ - real_num_busy = fetch_and_zero(&placement.num_busy_placement); - ret = ttm_bo_validate(bo, &placement, &ctx); + real_num_busy = fetch_and_zero(&placement->num_busy_placement); + ret = ttm_bo_validate(bo, placement, &ctx); if (ret) { ret = i915_ttm_err_to_gem(ret); /* @@ -652,8 +646,8 @@ static int i915_ttm_get_pages(struct drm_i915_gem_object *obj) * If the initial attempt fails, allow all accepted placements, * evicting if necessary. */ - placement.num_busy_placement = real_num_busy; - ret = ttm_bo_validate(bo, &placement, &ctx); + placement->num_busy_placement = real_num_busy; + ret = ttm_bo_validate(bo, placement, &ctx); if (ret) return i915_ttm_err_to_gem(ret); } @@ -668,16 +662,56 @@ static int i915_ttm_get_pages(struct drm_i915_gem_object *obj) i915_ttm_adjust_gem_after_move(obj); }
- /* Object either has a page vector or is an iomem object */ - st = bo->ttm ? i915_ttm_tt_get_st(bo->ttm) : obj->ttm.cached_io_st; - if (IS_ERR(st)) - return PTR_ERR(st); + if (!obj->mm.pages) { + /* Object either has a page vector or is an iomem object */ + st = bo->ttm ? i915_ttm_tt_get_st(bo->ttm) : obj->ttm.cached_io_st; + if (IS_ERR(st)) + return PTR_ERR(st);
- __i915_gem_object_set_pages(obj, st, i915_sg_dma_sizes(st->sgl)); + __i915_gem_object_set_pages(obj, st, i915_sg_dma_sizes(st->sgl)); + }
return ret; }
+static int i915_ttm_get_pages(struct drm_i915_gem_object *obj) +{ + struct ttm_place requested, busy[I915_TTM_MAX_PLACEMENTS]; + struct ttm_placement placement; + + GEM_BUG_ON(obj->mm.n_placements > I915_TTM_MAX_PLACEMENTS); + + /* Move to the requested placement. */ + i915_ttm_placement_from_obj(obj, &requested, busy, &placement); + + return __i915_ttm_get_pages(obj, &placement); +} + +static int i915_ttm_migrate(struct drm_i915_gem_object *obj, + struct intel_memory_region *mr) +{ + struct ttm_place requested; + struct ttm_placement placement; + int ret; + + i915_ttm_place_from_region(mr, &requested, obj->flags); + placement.num_placement = 1; + placement.num_busy_placement = 1; + placement.placement = &requested; + placement.busy_placement = &requested; + + ret = __i915_ttm_get_pages(obj, &placement); + if (ret) + return ret; + + if (obj->mm.region != mr) { + intel_memory_region_put(obj->mm.region); + obj->mm.region = intel_memory_region_get(mr); + } + + return 0; +} + static void i915_ttm_put_pages(struct drm_i915_gem_object *obj, struct sg_table *st) { @@ -814,6 +848,7 @@ static const struct drm_i915_gem_object_ops i915_gem_ttm_obj_ops = { .truncate = i915_ttm_purge, .adjust_lru = i915_ttm_adjust_lru, .delayed_free = i915_ttm_delayed_free, + .migrate = i915_ttm_migrate, .mmap_offset = i915_ttm_mmap_offset, .mmap_ops = &vm_ops_ttm, }; diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c b/drivers/gpu/drm/i915/gem/i915_gem_wait.c index 1070d3afdce7..e9f7a8d9f0e2 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c @@ -290,3 +290,22 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file) i915_gem_object_put(obj); return ret; } + +/** + * i915_gem_object_wait_migrate - Sync an accelerated migration operation + * @obj: The migrating object. + * @flags: waiting flags. Currently supports only I915_WAIT_INTERRUPTIBLE. + * + * Wait for any pending async migration operation on the object, + * whether it's explicitly (i915_gem_object_migrate()) or implicitly + * (swapin, initial clearing) initiated. + * + * Return: 0 if successful, -ERESTARTSYS if a signal was hit during waiting. + */ +int i915_gem_object_wait_migration(struct drm_i915_gem_object *obj, + unsigned int flags) +{ + might_sleep(); + /* NOP for now. */ + return 0; +}
On 24/06/2021 19:31, Thomas Hellström wrote:
Introduce an interface to migrate objects between regions. This is primarily intended to migrate objects to LMEM for display and to SYSTEM for dma-buf, but might be reused in one form or another for performande-based migration.
Signed-off-by: Thomas Hellström thomas.hellstrom@linux.intel.com
drivers/gpu/drm/i915/gem/i915_gem_object.c | 91 +++++++++++++++++++ drivers/gpu/drm/i915/gem/i915_gem_object.h | 12 +++ .../gpu/drm/i915/gem/i915_gem_object_types.h | 9 ++ drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 69 ++++++++++---- drivers/gpu/drm/i915/gem/i915_gem_wait.c | 19 ++++ 5 files changed, 183 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c index 07e8ff9a8aae..6421c3a8b2f3 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c @@ -513,6 +513,97 @@ bool i915_gem_object_has_iomem(const struct drm_i915_gem_object *obj) return obj->mem_flags & I915_BO_FLAG_IOMEM; }
+/**
- i915_gem_object_can_migrate - Whether an object likely can be migrated
- @obj: The object to migrate
- @id: The region intended to migrate to
- Check whether the object backend supports migration to the
- given region. Note that pinning may affect the ability to migrate.
- Return: true if migration is possible, false otherwise.
- */
+bool i915_gem_object_can_migrate(struct drm_i915_gem_object *obj,
enum intel_region_id id)+{
- struct drm_i915_private *i915 = to_i915(obj->base.dev);
- unsigned int num_allowed = obj->mm.n_placements;
- struct intel_memory_region *mr;
- unsigned int i;
- GEM_BUG_ON(id >= INTEL_REGION_UNKNOWN);
- GEM_BUG_ON(obj->mm.madv != I915_MADV_WILLNEED);
- if (!obj->ops->migrate)
return -EOPNOTSUPP;- mr = i915->mm.regions[id];
if (!mr) return false;
?
- if (obj->mm.region == mr)
return true;- if (!i915_gem_object_evictable(obj))
return false;- if (!(obj->flags & I915_BO_ALLOC_USER))
return true;- if (num_allowed == 0)
return false;- for (i = 0; i < num_allowed; ++i) {
if (mr == obj->mm.placements[i])return true;- }
- return false;
+}
+/**
- i915_gem_object_migrate - Migrate an object to the desired region id
- @obj: The object to migrate.
- @ww: An optional struct i915_gem_ww_ctx. If NULL, the backend may
- not be successful in evicting other objects to make room for this object.
- @id: The region id to migrate to.
- Attempt to migrate the object to the desired memory region. The
- object backend must support migration and the object may not be
- pinned, (explicitly pinned pages or pinned vmas). The object must
- be locked.
- On successful completion, the object will have pages pointing to
- memory in the new region, but an async migration task may not have
- completed yet, and to accomplish that, i915_gem_object_wait_migration()
- must be called.
- Return: 0 on success. Negative error code on failure. In particular may
- return -ENXIO on lack of region space, -EDEADLK for deadlock avoidance
- if @ww is set, -EINTR or -ERESTARTSYS if signal pending, and
- -EBUSY if the object is pinned.
- */
+int i915_gem_object_migrate(struct drm_i915_gem_object *obj,
struct i915_gem_ww_ctx *ww,enum intel_region_id id)+{
- struct drm_i915_private *i915 = to_i915(obj->base.dev);
- struct intel_memory_region *mr;
- GEM_BUG_ON(id >= INTEL_REGION_UNKNOWN);
- GEM_BUG_ON(obj->mm.madv != I915_MADV_WILLNEED);
- assert_object_held(obj);
- mr = i915->mm.regions[id];
GEM_BUG_ON(!mr) ?
- if (obj->mm.region == mr)
return 0;- if (!i915_gem_object_evictable(obj))
return -EBUSY;- if (!obj->ops->migrate)
return -EOPNOTSUPP;- return obj->ops->migrate(obj, mr);
+}
- void i915_gem_init__objects(struct drm_i915_private *i915) { INIT_WORK(&i915->mm.free_work, __i915_gem_free_work);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h index ea3224a480c4..8cbd7a5334e2 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h @@ -17,6 +17,8 @@ #include "i915_gem_ww.h" #include "i915_vma_types.h"
+enum intel_region_id;
- /*
- XXX: There is a prevalence of the assumption that we fit the
- object's page count inside a 32bit _signed_ variable. Let's document
@@ -597,6 +599,16 @@ bool i915_gem_object_migratable(struct drm_i915_gem_object *obj);
bool i915_gem_object_validates_to_lmem(struct drm_i915_gem_object *obj);
+int i915_gem_object_migrate(struct drm_i915_gem_object *obj,
struct i915_gem_ww_ctx *ww,enum intel_region_id id);+bool i915_gem_object_can_migrate(struct drm_i915_gem_object *obj,
enum intel_region_id id);+int i915_gem_object_wait_migration(struct drm_i915_gem_object *obj,
unsigned int flags);- #ifdef CONFIG_MMU_NOTIFIER static inline bool i915_gem_object_is_userptr(struct drm_i915_gem_object *obj)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h index 441f913c87e6..ef3de2ae9723 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h @@ -18,6 +18,7 @@
struct drm_i915_gem_object; struct intel_fronbuffer; +struct intel_memory_region;
/*
- struct i915_lut_handle tracks the fast lookups from handle to vma used
@@ -77,6 +78,14 @@ struct drm_i915_gem_object_ops { * delayed_free - Override the default delayed free implementation */ void (*delayed_free)(struct drm_i915_gem_object *obj);
/**
* migrate - Migrate object to a different region either for* pinning or for as long as the object lock is held.*/int (*migrate)(struct drm_i915_gem_object *obj,
struct intel_memory_region *mr);void (*release)(struct drm_i915_gem_object *obj);
const struct vm_operations_struct *mmap_ops;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c index c39d982c4fa6..b01bf0650ab0 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c @@ -617,7 +617,8 @@ struct ttm_device_funcs *i915_ttm_driver(void) return &i915_ttm_bo_driver; }
-static int i915_ttm_get_pages(struct drm_i915_gem_object *obj) +static int __i915_ttm_get_pages(struct drm_i915_gem_object *obj,
struct ttm_placement *placement)@@ -625,19 +626,12 @@ static int i915_ttm_get_pages(struct drm_i915_gem_object *obj) .no_wait_gpu = false, }; struct sg_table *st;
struct ttm_place requested, busy[I915_TTM_MAX_PLACEMENTS];
struct ttm_placement placement; int real_num_busy; int ret;
GEM_BUG_ON(obj->mm.n_placements > I915_TTM_MAX_PLACEMENTS);
/* Move to the requested placement. */
i915_ttm_placement_from_obj(obj, &requested, busy, &placement);
/* First try only the requested placement. No eviction. */
real_num_busy = fetch_and_zero(&placement.num_busy_placement);
ret = ttm_bo_validate(bo, &placement, &ctx);
- real_num_busy = fetch_and_zero(&placement->num_busy_placement);
- ret = ttm_bo_validate(bo, placement, &ctx); if (ret) { ret = i915_ttm_err_to_gem(ret); /*
@@ -652,8 +646,8 @@ static int i915_ttm_get_pages(struct drm_i915_gem_object *obj) * If the initial attempt fails, allow all accepted placements, * evicting if necessary. */
placement.num_busy_placement = real_num_busy;ret = ttm_bo_validate(bo, &placement, &ctx);
placement->num_busy_placement = real_num_busy;ret = ttm_bo_validate(bo, placement, &ctx);@@ -668,16 +662,56 @@ static int i915_ttm_get_pages(struct drm_i915_gem_object *obj) i915_ttm_adjust_gem_after_move(obj); }
- /* Object either has a page vector or is an iomem object */
- st = bo->ttm ? i915_ttm_tt_get_st(bo->ttm) : obj->ttm.cached_io_st;
- if (IS_ERR(st))
return PTR_ERR(st);
- if (!obj->mm.pages) {
/* Object either has a page vector or is an iomem object */st = bo->ttm ? i915_ttm_tt_get_st(bo->ttm) : obj->ttm.cached_io_st;if (IS_ERR(st))return PTR_ERR(st);
- __i915_gem_object_set_pages(obj, st, i915_sg_dma_sizes(st->sgl));
__i915_gem_object_set_pages(obj, st, i915_sg_dma_sizes(st->sgl));}
return ret; }
+static int i915_ttm_get_pages(struct drm_i915_gem_object *obj) +{
- struct ttm_place requested, busy[I915_TTM_MAX_PLACEMENTS];
- struct ttm_placement placement;
- GEM_BUG_ON(obj->mm.n_placements > I915_TTM_MAX_PLACEMENTS);
- /* Move to the requested placement. */
- i915_ttm_placement_from_obj(obj, &requested, busy, &placement);
- return __i915_ttm_get_pages(obj, &placement);
+}
+static int i915_ttm_migrate(struct drm_i915_gem_object *obj,
struct intel_memory_region *mr)+{
- struct ttm_place requested;
- struct ttm_placement placement;
- int ret;
- i915_ttm_place_from_region(mr, &requested, obj->flags);
- placement.num_placement = 1;
- placement.num_busy_placement = 1;
- placement.placement = &requested;
- placement.busy_placement = &requested;
- ret = __i915_ttm_get_pages(obj, &placement);
- if (ret)
return ret;- if (obj->mm.region != mr) {
intel_memory_region_put(obj->mm.region);obj->mm.region = intel_memory_region_get(mr);
i915_gem_object_{init,release}_memory_region?
- }
- return 0;
+}
- static void i915_ttm_put_pages(struct drm_i915_gem_object *obj, struct sg_table *st) {
@@ -814,6 +848,7 @@ static const struct drm_i915_gem_object_ops i915_gem_ttm_obj_ops = { .truncate = i915_ttm_purge, .adjust_lru = i915_ttm_adjust_lru, .delayed_free = i915_ttm_delayed_free,
- .migrate = i915_ttm_migrate, .mmap_offset = i915_ttm_mmap_offset, .mmap_ops = &vm_ops_ttm, };
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c b/drivers/gpu/drm/i915/gem/i915_gem_wait.c index 1070d3afdce7..e9f7a8d9f0e2 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c @@ -290,3 +290,22 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file) i915_gem_object_put(obj); return ret; }
+/**
- i915_gem_object_wait_migrate - Sync an accelerated migration operation
- @obj: The migrating object.
- @flags: waiting flags. Currently supports only I915_WAIT_INTERRUPTIBLE.
- Wait for any pending async migration operation on the object,
- whether it's explicitly (i915_gem_object_migrate()) or implicitly
- (swapin, initial clearing) initiated.
- Return: 0 if successful, -ERESTARTSYS if a signal was hit during waiting.
- */
+int i915_gem_object_wait_migration(struct drm_i915_gem_object *obj,
unsigned int flags)+{
- might_sleep();
- /* NOP for now. */
- return 0;
+}
From: Matthew Auld matthew.auld@intel.com
A selftest for the gem object migrate functionality. Slightly adapted from the original by Matthew to the new interface and new fill blit code.
Co-developed-by: Thomas Hellström thomas.hellstrom@linux.intel.com Signed-off-by: Thomas Hellström thomas.hellstrom@linux.intel.com Signed-off-by: Matthew Auld matthew.auld@intel.com --- drivers/gpu/drm/i915/gem/i915_gem_object.c | 1 + .../drm/i915/gem/selftests/i915_gem_migrate.c | 237 ++++++++++++++++++ .../drm/i915/selftests/i915_live_selftests.h | 1 + 3 files changed, 239 insertions(+) create mode 100644 drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c index 6421c3a8b2f3..24f4395bf387 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c @@ -644,6 +644,7 @@ static const struct drm_gem_object_funcs i915_gem_object_funcs = { #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) #include "selftests/huge_gem_object.c" #include "selftests/huge_pages.c" +#include "selftests/i915_gem_migrate.c" #include "selftests/i915_gem_object.c" #include "selftests/i915_gem_coherency.c" #endif diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c new file mode 100644 index 000000000000..a437b66f64d9 --- /dev/null +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c @@ -0,0 +1,237 @@ +// SPDX-License-Identifier: MIT +/* + * Copyright © 2020-2021 Intel Corporation + */ + +#include "gt/intel_migrate.h" + +static int igt_smem_create_migrate(void *arg) +{ + struct intel_gt *gt = arg; + struct drm_i915_private *i915 = gt->i915; + struct drm_i915_gem_object *obj; + struct i915_gem_ww_ctx ww; + int err = 0; + + /* Switch object backing-store on create */ + obj = i915_gem_object_create_lmem(i915, PAGE_SIZE, 0); + if (IS_ERR(obj)) + return PTR_ERR(obj); + + for_i915_gem_ww(&ww, err, true) { + err = i915_gem_object_lock(obj, &ww); + if (err) + continue; + + if (!i915_gem_object_can_migrate(obj, INTEL_REGION_SMEM)) { + err = -EINVAL; + continue; + } + + err = i915_gem_object_migrate(obj, &ww, INTEL_REGION_SMEM); + if (err) + continue; + + err = i915_gem_object_pin_pages(obj); + if (err) + continue; + + if (i915_gem_object_can_migrate(obj, INTEL_REGION_LMEM)) + err = -EINVAL; + + i915_gem_object_unpin_pages(obj); + } + i915_gem_object_put(obj); + + return err; +} + +static int igt_lmem_create_migrate(void *arg) +{ + struct intel_gt *gt = arg; + struct drm_i915_private *i915 = gt->i915; + struct drm_i915_gem_object *obj; + struct i915_gem_ww_ctx ww; + int err = 0; + + /* Switch object backing-store on create */ + obj = i915_gem_object_create_shmem(i915, PAGE_SIZE); + if (IS_ERR(obj)) + return PTR_ERR(obj); + + for_i915_gem_ww(&ww, err, true) { + err = i915_gem_object_lock(obj, &ww); + if (err) + continue; + + if (!i915_gem_object_can_migrate(obj, INTEL_REGION_LMEM)) { + err = -EINVAL; + continue; + } + + err = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM); + if (err) + continue; + + err = i915_gem_object_pin_pages(obj); + if (err) + continue; + + if (i915_gem_object_can_migrate(obj, INTEL_REGION_SMEM)) + err = -EINVAL; + + i915_gem_object_unpin_pages(obj); + } + i915_gem_object_put(obj); + + return err; +} + +static int lmem_pages_migrate_one(struct i915_gem_ww_ctx *ww, + struct drm_i915_gem_object *obj) +{ + int err; + + err = i915_gem_object_lock(obj, ww); + if (err) + return err; + + err = i915_gem_object_wait(obj, + I915_WAIT_INTERRUPTIBLE | + I915_WAIT_PRIORITY | + I915_WAIT_ALL, + MAX_SCHEDULE_TIMEOUT); + if (err) + return err; + + if (i915_gem_object_is_lmem(obj)) { + if (!i915_gem_object_can_migrate(obj, INTEL_REGION_SMEM)) { + pr_err("object can't migrate to smem.\n"); + return -EINVAL; + } + + err = i915_gem_object_migrate(obj, ww, INTEL_REGION_SMEM); + if (err) { + pr_err("Object failed migration to smem\n"); + if (err) + return err; + } + + if (i915_gem_object_is_lmem(obj)) { + pr_err("object still backed by lmem\n"); + err = -EINVAL; + } + + if (!i915_gem_object_has_struct_page(obj)) { + pr_err("object not backed by struct page\n"); + err = -EINVAL; + } + + } else { + if (!i915_gem_object_can_migrate(obj, INTEL_REGION_LMEM)) { + pr_err("object can't migrate to lmem.\n"); + return -EINVAL; + } + + err = i915_gem_object_migrate(obj, ww, INTEL_REGION_LMEM); + if (err) { + pr_err("Object failed migration to lmem\n"); + if (err) + return err; + } + + if (i915_gem_object_has_struct_page(obj)) { + pr_err("object still backed by struct page\n"); + err = -EINVAL; + } + + if (!i915_gem_object_is_lmem(obj)) { + pr_err("object not backed by lmem\n"); + err = -EINVAL; + } + } + + return err; +} + +static int igt_lmem_pages_migrate(void *arg) +{ + struct intel_gt *gt = arg; + struct drm_i915_private *i915 = gt->i915; + struct drm_i915_gem_object *obj; + struct i915_gem_ww_ctx ww; + struct i915_request *rq; + int err; + int i; + + /* From LMEM to shmem and back again */ + + obj = i915_gem_object_create_lmem(i915, SZ_2M, 0); + if (IS_ERR(obj)) + return PTR_ERR(obj); + + err = i915_gem_object_lock(obj, NULL); + if (err) + goto out_put; + + err = ____i915_gem_object_get_pages(obj); + if (err) { + i915_gem_object_unlock(obj); + goto out_put; + } + + err = intel_context_migrate_clear(gt->migrate.context, NULL, + obj->mm.pages->sgl, obj->cache_level, + i915_gem_object_is_lmem(obj), + 0, &rq); + if (rq) { + dma_resv_add_excl_fence(obj->base.resv, &rq->fence); + i915_request_put(rq); + } + i915_gem_object_unlock(obj); + if (err) + goto out_put; + + for (i = 1; i <= 4; ++i) { + for_i915_gem_ww(&ww, err, true) { + err = lmem_pages_migrate_one(&ww, obj); + if (err) + continue; + + err = i915_gem_object_wait_migration(obj, true); + if (err) + continue; + + err = intel_migrate_clear(>->migrate, &ww, NULL, + obj->mm.pages->sgl, + obj->cache_level, + i915_gem_object_is_lmem(obj), + 0xdeadbeaf, &rq); + if (rq) { + dma_resv_add_excl_fence(obj->base.resv, + &rq->fence); + i915_request_put(rq); + } + } + if (err) + break; + } +out_put: + i915_gem_object_put(obj); + + return err; +} + +int i915_gem_migrate_live_selftests(struct drm_i915_private *i915) +{ + static const struct i915_subtest tests[] = { + SUBTEST(igt_smem_create_migrate), + SUBTEST(igt_lmem_create_migrate), + SUBTEST(igt_lmem_pages_migrate), + }; + + if (!HAS_LMEM(i915)) + return 0; + + return intel_gt_live_subtests(tests, &i915->gt); +} diff --git a/drivers/gpu/drm/i915/selftests/i915_live_selftests.h b/drivers/gpu/drm/i915/selftests/i915_live_selftests.h index a68197cf1044..e2fd1b61af71 100644 --- a/drivers/gpu/drm/i915/selftests/i915_live_selftests.h +++ b/drivers/gpu/drm/i915/selftests/i915_live_selftests.h @@ -40,6 +40,7 @@ selftest(hugepages, i915_gem_huge_page_live_selftests) selftest(gem_contexts, i915_gem_context_live_selftests) selftest(gem_execbuf, i915_gem_execbuffer_live_selftests) selftest(client, i915_gem_client_blt_live_selftests) +selftest(gem_migrate, i915_gem_migrate_live_selftests) selftest(reset, intel_reset_live_selftests) selftest(memory_region, intel_memory_region_live_selftests) selftest(hangcheck, intel_hangcheck_live_selftests)
Objects intended to be used as display framebuffers must reside in LMEM for discrete. If they happen to not do that, migrate them to LMEM before pinning.
Signed-off-by: Thomas Hellström thomas.hellstrom@linux.intel.com --- drivers/gpu/drm/i915/display/intel_display.c | 5 ++++- drivers/gpu/drm/i915/gem/i915_gem_domain.c | 2 +- drivers/gpu/drm/i915/gem/i915_gem_lmem.c | 21 -------------------- drivers/gpu/drm/i915/gem/i915_gem_object.h | 2 -- 4 files changed, 5 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 4524dbfa5e42..83a4aba54d67 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -1331,6 +1331,9 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, ret = i915_gem_object_lock(obj, &ww); if (!ret && phys_cursor) ret = i915_gem_object_attach_phys(obj, alignment); + else if (!ret && HAS_LMEM(dev_priv)) + ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM); + /* TODO: Do we need to sync when migration becomes async? */ if (!ret) ret = i915_gem_object_pin_pages(obj); if (ret) @@ -11770,7 +11773,7 @@ intel_user_framebuffer_create(struct drm_device *dev,
/* object is backed with LMEM for discrete */ i915 = to_i915(obj->base.dev); - if (HAS_LMEM(i915) && !i915_gem_object_validates_to_lmem(obj)) { + if (HAS_LMEM(i915) && !i915_gem_object_can_migrate(obj, INTEL_REGION_LMEM)) { /* object is "remote", not in local memory */ i915_gem_object_put(obj); return ERR_PTR(-EREMOTE); diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c b/drivers/gpu/drm/i915/gem/i915_gem_domain.c index 073822100da7..7d1400b13429 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c @@ -375,7 +375,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj, struct i915_vma *vma; int ret;
- /* Frame buffer must be in LMEM (no migration yet) */ + /* Frame buffer must be in LMEM */ if (HAS_LMEM(i915) && !i915_gem_object_is_lmem(obj)) return ERR_PTR(-EINVAL);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_lmem.c b/drivers/gpu/drm/i915/gem/i915_gem_lmem.c index 41d5182cd367..be1d122574af 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_lmem.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_lmem.c @@ -23,27 +23,6 @@ i915_gem_object_lmem_io_map(struct drm_i915_gem_object *obj, return io_mapping_map_wc(&obj->mm.region->iomap, offset, size); }
-/** - * i915_gem_object_validates_to_lmem - Whether the object is resident in - * lmem when pages are present. - * @obj: The object to check. - * - * Migratable objects residency may change from under us if the object is - * not pinned or locked. This function is intended to be used to check whether - * the object can only reside in lmem when pages are present. - * - * Return: Whether the object is always resident in lmem when pages are - * present. - */ -bool i915_gem_object_validates_to_lmem(struct drm_i915_gem_object *obj) -{ - struct intel_memory_region *mr = READ_ONCE(obj->mm.region); - - return !i915_gem_object_migratable(obj) && - mr && (mr->type == INTEL_MEMORY_LOCAL || - mr->type == INTEL_MEMORY_STOLEN_LOCAL); -} - /** * i915_gem_object_is_lmem - Whether the object is resident in * lmem diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h index 8cbd7a5334e2..d423d8cac4f2 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h @@ -597,8 +597,6 @@ bool i915_gem_object_evictable(struct drm_i915_gem_object *obj);
bool i915_gem_object_migratable(struct drm_i915_gem_object *obj);
-bool i915_gem_object_validates_to_lmem(struct drm_i915_gem_object *obj); - int i915_gem_object_migrate(struct drm_i915_gem_object *obj, struct i915_gem_ww_ctx *ww, enum intel_region_id id);
Until we support p2p dma or as a complement to that, migrate data to system memory at dma-buf map time if possible.
Signed-off-by: Thomas Hellström thomas.hellstrom@linux.intel.com --- drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c index 616c3a2f1baf..a52f885bc09a 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c @@ -25,7 +25,14 @@ static struct sg_table *i915_gem_map_dma_buf(struct dma_buf_attachment *attachme struct scatterlist *src, *dst; int ret, i;
- ret = i915_gem_object_pin_pages_unlocked(obj); + ret = i915_gem_object_lock_interruptible(obj, NULL); + if (ret) + return ERR_PTR(ret); + + ret = i915_gem_object_migrate(obj, NULL, INTEL_REGION_SMEM); + if (!ret) + ret = i915_gem_object_pin_pages(obj); + i915_gem_object_unlock(obj); if (ret) goto err;
-----Original Message----- From: dri-devel dri-devel-bounces@lists.freedesktop.org On Behalf Of Thomas Hellström Sent: Thursday, June 24, 2021 2:31 PM To: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Cc: Thomas Hellström thomas.hellstrom@linux.intel.com; Auld, Matthew matthew.auld@intel.com Subject: [PATCH 4/4] drm/i915/gem: Migrate to system at dma-buf map time
Until we support p2p dma or as a complement to that, migrate data to system memory at dma-buf map time if possible.
Signed-off-by: Thomas Hellström thomas.hellstrom@linux.intel.com
drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c index 616c3a2f1baf..a52f885bc09a 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c @@ -25,7 +25,14 @@ static struct sg_table *i915_gem_map_dma_buf(struct dma_buf_attachment *attachme struct scatterlist *src, *dst; int ret, i;
- ret = i915_gem_object_pin_pages_unlocked(obj);
- ret = i915_gem_object_lock_interruptible(obj, NULL);
Hmm, I believe in most cases that the caller should be holding the lock (object dma-resv) on this object already.
I know for the dynamic version of dma-buf, there is a check to make sure that the lock is held when called.
I think you will run into some issues if you try to get it here as well.
Mike
- if (ret)
return ERR_PTR(ret);- ret = i915_gem_object_migrate(obj, NULL, INTEL_REGION_SMEM);
- if (!ret)
ret = i915_gem_object_pin_pages(obj);- i915_gem_object_unlock(obj); if (ret) goto err;
-- 2.31.1
Hi, Michael,
thanks for looking at this.
On 6/25/21 6:02 PM, Ruhl, Michael J wrote:
-----Original Message----- From: dri-devel dri-devel-bounces@lists.freedesktop.org On Behalf Of Thomas Hellström Sent: Thursday, June 24, 2021 2:31 PM To: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Cc: Thomas Hellström thomas.hellstrom@linux.intel.com; Auld, Matthew matthew.auld@intel.com Subject: [PATCH 4/4] drm/i915/gem: Migrate to system at dma-buf map time
Until we support p2p dma or as a complement to that, migrate data to system memory at dma-buf map time if possible.
Signed-off-by: Thomas Hellström thomas.hellstrom@linux.intel.com
drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c index 616c3a2f1baf..a52f885bc09a 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c @@ -25,7 +25,14 @@ static struct sg_table *i915_gem_map_dma_buf(struct dma_buf_attachment *attachme struct scatterlist *src, *dst; int ret, i;
- ret = i915_gem_object_pin_pages_unlocked(obj);
- ret = i915_gem_object_lock_interruptible(obj, NULL);
Hmm, I believe in most cases that the caller should be holding the lock (object dma-resv) on this object already.
Yes, I agree, In particular for other instances of our own driver, at least since the dma_resv introduction.
But I also think that's a pre-existing bug, since i915_gem_object_pin_pages_unlocked() will also take the lock.
I Think we need to initially make the exporter dynamic-capable to resolve this, and drop the locking here completely, as dma-buf docs says that we're then guaranteed to get called with the object lock held.
I figure if we make the exporter dynamic, we need to migrate already at dma_buf_pin time so we don't pin the object in the wrong location.
/Thomas
I know for the dynamic version of dma-buf, there is a check to make sure that the lock is held when called.
I think you will run into some issues if you try to get it here as well.
Mike
- if (ret)
return ERR_PTR(ret);- ret = i915_gem_object_migrate(obj, NULL, INTEL_REGION_SMEM);
- if (!ret)
ret = i915_gem_object_pin_pages(obj);- i915_gem_object_unlock(obj); if (ret) goto err;
-- 2.31.1
-----Original Message----- From: Thomas Hellström thomas.hellstrom@linux.intel.com Sent: Friday, June 25, 2021 12:18 PM To: Ruhl, Michael J michael.j.ruhl@intel.com; intel- gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Cc: Auld, Matthew matthew.auld@intel.com Subject: Re: [PATCH 4/4] drm/i915/gem: Migrate to system at dma-buf map time
Hi, Michael,
thanks for looking at this.
On 6/25/21 6:02 PM, Ruhl, Michael J wrote:
-----Original Message----- From: dri-devel dri-devel-bounces@lists.freedesktop.org On Behalf Of Thomas Hellström Sent: Thursday, June 24, 2021 2:31 PM To: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Cc: Thomas Hellström thomas.hellstrom@linux.intel.com; Auld,
Matthew
matthew.auld@intel.com Subject: [PATCH 4/4] drm/i915/gem: Migrate to system at dma-buf map
time
Until we support p2p dma or as a complement to that, migrate data to system memory at dma-buf map time if possible.
Signed-off-by: Thomas Hellström thomas.hellstrom@linux.intel.com
drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c index 616c3a2f1baf..a52f885bc09a 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c @@ -25,7 +25,14 @@ static struct sg_table
*i915_gem_map_dma_buf(struct
dma_buf_attachment *attachme struct scatterlist *src, *dst; int ret, i;
- ret = i915_gem_object_pin_pages_unlocked(obj);
- ret = i915_gem_object_lock_interruptible(obj, NULL);
Hmm, I believe in most cases that the caller should be holding the lock (object dma-resv) on this object already.
Yes, I agree, In particular for other instances of our own driver, at least since the dma_resv introduction.
But I also think that's a pre-existing bug, since i915_gem_object_pin_pages_unlocked() will also take the lock.
Ouch yes. Missed that.
I Think we need to initially make the exporter dynamic-capable to resolve this, and drop the locking here completely, as dma-buf docs says that we're then guaranteed to get called with the object lock held.
I figure if we make the exporter dynamic, we need to migrate already at dma_buf_pin time so we don't pin the object in the wrong location.
The exporter as dynamic (ops->pin is available) is optional, but importer dynamic (ops->move_notify) is required.
With that in mind, it would seem that there are three possible combinations for the migrate to be attempted:
1) in the ops->pin function (export_dynamic != import_dynamic, during attach) 2) in the ops->pin function (export_dynamic and !CONFIG_DMABUF_MOVE_NOTIFY) during mapping 3) and possibly in ops->map_dma_buf (exort_dynamic iand CONFIG_DMABUF_MOVE_NOTIFY)
Since one possibility has to be in the mapping function, it seems that if we can figure out the locking, that the migrate should probably be available here.
Mike
/Thomas
I know for the dynamic version of dma-buf, there is a check to make sure that the lock is held when called.
I think you will run into some issues if you try to get it here as well.
Mike
- if (ret)
return ERR_PTR(ret);- ret = i915_gem_object_migrate(obj, NULL, INTEL_REGION_SMEM);
- if (!ret)
ret = i915_gem_object_pin_pages(obj);- i915_gem_object_unlock(obj); if (ret) goto err;
-- 2.31.1
On 6/25/21 7:38 PM, Ruhl, Michael J wrote:
-----Original Message----- From: Thomas Hellström thomas.hellstrom@linux.intel.com Sent: Friday, June 25, 2021 12:18 PM To: Ruhl, Michael J michael.j.ruhl@intel.com; intel- gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Cc: Auld, Matthew matthew.auld@intel.com Subject: Re: [PATCH 4/4] drm/i915/gem: Migrate to system at dma-buf map time
Hi, Michael,
thanks for looking at this.
On 6/25/21 6:02 PM, Ruhl, Michael J wrote:
-----Original Message----- From: dri-devel dri-devel-bounces@lists.freedesktop.org On Behalf Of Thomas Hellström Sent: Thursday, June 24, 2021 2:31 PM To: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Cc: Thomas Hellström thomas.hellstrom@linux.intel.com; Auld,
Matthew
matthew.auld@intel.com Subject: [PATCH 4/4] drm/i915/gem: Migrate to system at dma-buf map
time
Until we support p2p dma or as a complement to that, migrate data to system memory at dma-buf map time if possible.
Signed-off-by: Thomas Hellström thomas.hellstrom@linux.intel.com
drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c index 616c3a2f1baf..a52f885bc09a 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c @@ -25,7 +25,14 @@ static struct sg_table
*i915_gem_map_dma_buf(struct
dma_buf_attachment *attachme struct scatterlist *src, *dst; int ret, i;
- ret = i915_gem_object_pin_pages_unlocked(obj);
- ret = i915_gem_object_lock_interruptible(obj, NULL);
Hmm, I believe in most cases that the caller should be holding the lock (object dma-resv) on this object already.
Yes, I agree, In particular for other instances of our own driver, at least since the dma_resv introduction.
But I also think that's a pre-existing bug, since i915_gem_object_pin_pages_unlocked() will also take the lock.
Ouch yes. Missed that.
I Think we need to initially make the exporter dynamic-capable to resolve this, and drop the locking here completely, as dma-buf docs says that we're then guaranteed to get called with the object lock held.
I figure if we make the exporter dynamic, we need to migrate already at dma_buf_pin time so we don't pin the object in the wrong location.
The exporter as dynamic (ops->pin is available) is optional, but importer dynamic (ops->move_notify) is required.
With that in mind, it would seem that there are three possible combinations for the migrate to be attempted:
- in the ops->pin function (export_dynamic != import_dynamic, during attach)
- in the ops->pin function (export_dynamic and !CONFIG_DMABUF_MOVE_NOTIFY) during mapping
- and possibly in ops->map_dma_buf (exort_dynamic iand CONFIG_DMABUF_MOVE_NOTIFY)
Since one possibility has to be in the mapping function, it seems that if we can figure out the locking, that the migrate should probably be available here.
Mike
So perhaps just to initially fix the bug, we could just implement NOP pin() and unpin() callbacks and drop the locking in map_attach() and replace it with an assert_object_held();
/Thomas
-----Original Message----- From: Thomas Hellström thomas.hellstrom@linux.intel.com Sent: Friday, June 25, 2021 1:52 PM To: Ruhl, Michael J michael.j.ruhl@intel.com; intel- gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Cc: Auld, Matthew matthew.auld@intel.com Subject: Re: [PATCH 4/4] drm/i915/gem: Migrate to system at dma-buf map time
On 6/25/21 7:38 PM, Ruhl, Michael J wrote:
-----Original Message----- From: Thomas Hellström thomas.hellstrom@linux.intel.com Sent: Friday, June 25, 2021 12:18 PM To: Ruhl, Michael J michael.j.ruhl@intel.com; intel- gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Cc: Auld, Matthew matthew.auld@intel.com Subject: Re: [PATCH 4/4] drm/i915/gem: Migrate to system at dma-buf
map
time
Hi, Michael,
thanks for looking at this.
On 6/25/21 6:02 PM, Ruhl, Michael J wrote:
-----Original Message----- From: dri-devel dri-devel-bounces@lists.freedesktop.org On Behalf
Of
Thomas Hellström Sent: Thursday, June 24, 2021 2:31 PM To: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Cc: Thomas Hellström thomas.hellstrom@linux.intel.com; Auld,
Matthew
matthew.auld@intel.com Subject: [PATCH 4/4] drm/i915/gem: Migrate to system at dma-buf map
time
Until we support p2p dma or as a complement to that, migrate data to system memory at dma-buf map time if possible.
Signed-off-by: Thomas Hellström thomas.hellstrom@linux.intel.com
drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c index 616c3a2f1baf..a52f885bc09a 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c @@ -25,7 +25,14 @@ static struct sg_table
*i915_gem_map_dma_buf(struct
dma_buf_attachment *attachme struct scatterlist *src, *dst; int ret, i;
- ret = i915_gem_object_pin_pages_unlocked(obj);
- ret = i915_gem_object_lock_interruptible(obj, NULL);
Hmm, I believe in most cases that the caller should be holding the lock (object dma-resv) on this object already.
Yes, I agree, In particular for other instances of our own driver, at least since the dma_resv introduction.
But I also think that's a pre-existing bug, since i915_gem_object_pin_pages_unlocked() will also take the lock.
Ouch yes. Missed that.
I Think we need to initially make the exporter dynamic-capable to resolve this, and drop the locking here completely, as dma-buf docs says that we're then guaranteed to get called with the object lock held.
I figure if we make the exporter dynamic, we need to migrate already at dma_buf_pin time so we don't pin the object in the wrong location.
The exporter as dynamic (ops->pin is available) is optional, but importer dynamic (ops->move_notify) is required.
With that in mind, it would seem that there are three possible combinations for the migrate to be attempted:
- in the ops->pin function (export_dynamic != import_dynamic, during
attach)
- in the ops->pin function (export_dynamic and
!CONFIG_DMABUF_MOVE_NOTIFY) during mapping
- and possibly in ops->map_dma_buf (exort_dynamic iand
CONFIG_DMABUF_MOVE_NOTIFY)
Since one possibility has to be in the mapping function, it seems that if we can figure out the locking, that the migrate should probably be available
here.
Mike
So perhaps just to initially fix the bug, we could just implement NOP pin() and unpin() callbacks and drop the locking in map_attach() and replace it with an assert_object_held();
That is the sticky part of the move notify API.
If you do the attach_dynamic you have to have an ops with move_notify.
(https://elixir.bootlin.com/linux/v5.13-rc7/source/drivers/dma-buf/dma-buf.c#...)
If you don't have that, i.e. just the pin interface, the attach will be rejected, and you will not get the callbacks.
So I think that the only thing we can do for now is to dop the locking and add the
assert_object_held();
M
/Thomas
Hi, Mike,
On 6/25/21 7:57 PM, Ruhl, Michael J wrote:
-----Original Message----- From: Thomas Hellström thomas.hellstrom@linux.intel.com Sent: Friday, June 25, 2021 1:52 PM To: Ruhl, Michael J michael.j.ruhl@intel.com; intel- gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Cc: Auld, Matthew matthew.auld@intel.com Subject: Re: [PATCH 4/4] drm/i915/gem: Migrate to system at dma-buf map time
On 6/25/21 7:38 PM, Ruhl, Michael J wrote:
-----Original Message----- From: Thomas Hellström thomas.hellstrom@linux.intel.com Sent: Friday, June 25, 2021 12:18 PM To: Ruhl, Michael J michael.j.ruhl@intel.com; intel- gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Cc: Auld, Matthew matthew.auld@intel.com Subject: Re: [PATCH 4/4] drm/i915/gem: Migrate to system at dma-buf
map
time
Hi, Michael,
thanks for looking at this.
On 6/25/21 6:02 PM, Ruhl, Michael J wrote:
-----Original Message----- From: dri-devel dri-devel-bounces@lists.freedesktop.org On Behalf
Of
Thomas Hellström Sent: Thursday, June 24, 2021 2:31 PM To: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Cc: Thomas Hellström thomas.hellstrom@linux.intel.com; Auld,
Matthew
matthew.auld@intel.com Subject: [PATCH 4/4] drm/i915/gem: Migrate to system at dma-buf map
time
Until we support p2p dma or as a complement to that, migrate data to system memory at dma-buf map time if possible.
Signed-off-by: Thomas Hellström thomas.hellstrom@linux.intel.com
drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c index 616c3a2f1baf..a52f885bc09a 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c @@ -25,7 +25,14 @@ static struct sg_table
*i915_gem_map_dma_buf(struct
dma_buf_attachment *attachme struct scatterlist *src, *dst; int ret, i;
- ret = i915_gem_object_pin_pages_unlocked(obj);
- ret = i915_gem_object_lock_interruptible(obj, NULL);
Hmm, I believe in most cases that the caller should be holding the lock (object dma-resv) on this object already.
Yes, I agree, In particular for other instances of our own driver, at least since the dma_resv introduction.
But I also think that's a pre-existing bug, since i915_gem_object_pin_pages_unlocked() will also take the lock.
Ouch yes. Missed that.
I Think we need to initially make the exporter dynamic-capable to resolve this, and drop the locking here completely, as dma-buf docs says that we're then guaranteed to get called with the object lock held.
I figure if we make the exporter dynamic, we need to migrate already at dma_buf_pin time so we don't pin the object in the wrong location.
The exporter as dynamic (ops->pin is available) is optional, but importer dynamic (ops->move_notify) is required.
With that in mind, it would seem that there are three possible combinations for the migrate to be attempted:
- in the ops->pin function (export_dynamic != import_dynamic, during
attach)
- in the ops->pin function (export_dynamic and
!CONFIG_DMABUF_MOVE_NOTIFY) during mapping
- and possibly in ops->map_dma_buf (exort_dynamic iand
CONFIG_DMABUF_MOVE_NOTIFY)
Since one possibility has to be in the mapping function, it seems that if we can figure out the locking, that the migrate should probably be available
here.
Mike
So perhaps just to initially fix the bug, we could just implement NOP pin() and unpin() callbacks and drop the locking in map_attach() and replace it with an assert_object_held();
That is the sticky part of the move notify API.
If you do the attach_dynamic you have to have an ops with move_notify.
(https://elixir.bootlin.com/linux/v5.13-rc7/source/drivers/dma-buf/dma-buf.c#...)
If you don't have that, i.e. just the pin interface, the attach will be rejected, and you will not get the callbacks.
I understood that as the requirement for move_notify is only if the *importer* declares dynamic. A dynamic exporter could choose whether to call move_notify() on eviction or to pin and never evict. If the importer is non-dynamic, the core calls pin() and the only choice is to pin and never evict.
So if we temporarily choose to pin and never evict for *everything*, (as the current code does now), I think we should be good for now, and then we can implement all fancy p2p and move_notify stuff on top of that.
/Thomas
So I think that the only thing we can do for now is to dop the locking and add the
assert_object_held();
M
/Thomas
-----Original Message----- From: Thomas Hellström thomas.hellstrom@linux.intel.com Sent: Friday, June 25, 2021 2:50 PM To: Ruhl, Michael J michael.j.ruhl@intel.com; intel- gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Cc: Auld, Matthew matthew.auld@intel.com Subject: Re: [PATCH 4/4] drm/i915/gem: Migrate to system at dma-buf map time
Hi, Mike,
On 6/25/21 7:57 PM, Ruhl, Michael J wrote:
-----Original Message----- From: Thomas Hellström thomas.hellstrom@linux.intel.com Sent: Friday, June 25, 2021 1:52 PM To: Ruhl, Michael J michael.j.ruhl@intel.com; intel- gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Cc: Auld, Matthew matthew.auld@intel.com Subject: Re: [PATCH 4/4] drm/i915/gem: Migrate to system at dma-buf
map
time
On 6/25/21 7:38 PM, Ruhl, Michael J wrote:
-----Original Message----- From: Thomas Hellström thomas.hellstrom@linux.intel.com Sent: Friday, June 25, 2021 12:18 PM To: Ruhl, Michael J michael.j.ruhl@intel.com; intel- gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Cc: Auld, Matthew matthew.auld@intel.com Subject: Re: [PATCH 4/4] drm/i915/gem: Migrate to system at dma-buf
map
time
Hi, Michael,
thanks for looking at this.
On 6/25/21 6:02 PM, Ruhl, Michael J wrote:
> -----Original Message----- > From: dri-devel dri-devel-bounces@lists.freedesktop.org On
Behalf
Of
> Thomas Hellström > Sent: Thursday, June 24, 2021 2:31 PM > To: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org > Cc: Thomas Hellström thomas.hellstrom@linux.intel.com; Auld,
Matthew
> matthew.auld@intel.com > Subject: [PATCH 4/4] drm/i915/gem: Migrate to system at dma-buf
map
time
> Until we support p2p dma or as a complement to that, migrate data > to system memory at dma-buf map time if possible. > > Signed-off-by: Thomas Hellström
thomas.hellstrom@linux.intel.com
> --- > drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > index 616c3a2f1baf..a52f885bc09a 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > @@ -25,7 +25,14 @@ static struct sg_table
*i915_gem_map_dma_buf(struct
> dma_buf_attachment *attachme > struct scatterlist *src, *dst; > int ret, i; > > - ret = i915_gem_object_pin_pages_unlocked(obj); > + ret = i915_gem_object_lock_interruptible(obj, NULL); Hmm, I believe in most cases that the caller should be holding the lock (object dma-resv) on this object already.
Yes, I agree, In particular for other instances of our own driver, at least since the dma_resv introduction.
But I also think that's a pre-existing bug, since i915_gem_object_pin_pages_unlocked() will also take the lock.
Ouch yes. Missed that.
I Think we need to initially make the exporter dynamic-capable to resolve this, and drop the locking here completely, as dma-buf docs says that we're then guaranteed to get called with the object lock held.
I figure if we make the exporter dynamic, we need to migrate already at dma_buf_pin time so we don't pin the object in the wrong location.
The exporter as dynamic (ops->pin is available) is optional, but importer dynamic (ops->move_notify) is required.
With that in mind, it would seem that there are three possible
combinations
for the migrate to be attempted:
- in the ops->pin function (export_dynamic != import_dynamic, during
attach)
- in the ops->pin function (export_dynamic and
!CONFIG_DMABUF_MOVE_NOTIFY) during mapping
- and possibly in ops->map_dma_buf (exort_dynamic iand
CONFIG_DMABUF_MOVE_NOTIFY)
Since one possibility has to be in the mapping function, it seems that if we can figure out the locking, that the migrate should probably be available
here.
Mike
So perhaps just to initially fix the bug, we could just implement NOP pin() and unpin() callbacks and drop the locking in map_attach() and replace it with an assert_object_held();
That is the sticky part of the move notify API.
If you do the attach_dynamic you have to have an ops with move_notify.
(https://elixir.bootlin.com/linux/v5.13-rc7/source/drivers/dma-buf/dma-
buf.c#L730)
If you don't have that, i.e. just the pin interface, the attach will be rejected, and you will not get the callbacks.
I understood that as the requirement for move_notify is only if the *importer* declares dynamic. A dynamic exporter could choose whether to call move_notify() on eviction or to pin and never evict. If the importer is non-dynamic, the core calls pin() and the only choice is to pin and never evict.
So if we temporarily choose to pin and never evict for *everything*, (as the current code does now), I think we should be good for now, and then we can implement all fancy p2p and move_notify stuff on top of that.
/sigh.
You are correct. I was mistakenly placing the pin API (dma_buf_ops) in the attach_ops. 😐 Must be Friday.
Upon further reflection, I think that your path will work.
However, is doing a pin (with no locking) from the dma_buf_mapping any different from using the pin API + export_dynamic?
M
/Thomas
So I think that the only thing we can do for now is to dop the locking and add
the
assert_object_held();
M
/Thomas
On 6/25/21 9:07 PM, Ruhl, Michael J wrote:
-----Original Message----- From: Thomas Hellström thomas.hellstrom@linux.intel.com Sent: Friday, June 25, 2021 2:50 PM To: Ruhl, Michael J michael.j.ruhl@intel.com; intel- gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Cc: Auld, Matthew matthew.auld@intel.com Subject: Re: [PATCH 4/4] drm/i915/gem: Migrate to system at dma-buf map time
Hi, Mike,
On 6/25/21 7:57 PM, Ruhl, Michael J wrote:
-----Original Message----- From: Thomas Hellström thomas.hellstrom@linux.intel.com Sent: Friday, June 25, 2021 1:52 PM To: Ruhl, Michael J michael.j.ruhl@intel.com; intel- gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Cc: Auld, Matthew matthew.auld@intel.com Subject: Re: [PATCH 4/4] drm/i915/gem: Migrate to system at dma-buf
map
time
On 6/25/21 7:38 PM, Ruhl, Michael J wrote:
-----Original Message----- From: Thomas Hellström thomas.hellstrom@linux.intel.com Sent: Friday, June 25, 2021 12:18 PM To: Ruhl, Michael J michael.j.ruhl@intel.com; intel- gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Cc: Auld, Matthew matthew.auld@intel.com Subject: Re: [PATCH 4/4] drm/i915/gem: Migrate to system at dma-buf
map
time
Hi, Michael,
thanks for looking at this.
On 6/25/21 6:02 PM, Ruhl, Michael J wrote: >> -----Original Message----- >> From: dri-devel dri-devel-bounces@lists.freedesktop.org On
Behalf
Of
>> Thomas Hellström >> Sent: Thursday, June 24, 2021 2:31 PM >> To: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org >> Cc: Thomas Hellström thomas.hellstrom@linux.intel.com; Auld, Matthew >> matthew.auld@intel.com >> Subject: [PATCH 4/4] drm/i915/gem: Migrate to system at dma-buf
map
time >> Until we support p2p dma or as a complement to that, migrate data >> to system memory at dma-buf map time if possible. >> >> Signed-off-by: Thomas Hellström
thomas.hellstrom@linux.intel.com
>> --- >> drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c >> b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c >> index 616c3a2f1baf..a52f885bc09a 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c >> @@ -25,7 +25,14 @@ static struct sg_table *i915_gem_map_dma_buf(struct >> dma_buf_attachment *attachme >> struct scatterlist *src, *dst; >> int ret, i; >> >> - ret = i915_gem_object_pin_pages_unlocked(obj); >> + ret = i915_gem_object_lock_interruptible(obj, NULL); > Hmm, I believe in most cases that the caller should be holding the > lock (object dma-resv) on this object already. Yes, I agree, In particular for other instances of our own driver, at least since the dma_resv introduction.
But I also think that's a pre-existing bug, since i915_gem_object_pin_pages_unlocked() will also take the lock.
Ouch yes. Missed that.
I Think we need to initially make the exporter dynamic-capable to resolve this, and drop the locking here completely, as dma-buf docs says that we're then guaranteed to get called with the object lock held.
I figure if we make the exporter dynamic, we need to migrate already at dma_buf_pin time so we don't pin the object in the wrong location.
The exporter as dynamic (ops->pin is available) is optional, but importer dynamic (ops->move_notify) is required.
With that in mind, it would seem that there are three possible
combinations
for the migrate to be attempted:
- in the ops->pin function (export_dynamic != import_dynamic, during
attach)
- in the ops->pin function (export_dynamic and
!CONFIG_DMABUF_MOVE_NOTIFY) during mapping
- and possibly in ops->map_dma_buf (exort_dynamic iand
CONFIG_DMABUF_MOVE_NOTIFY)
Since one possibility has to be in the mapping function, it seems that if we can figure out the locking, that the migrate should probably be available
here.
Mike
So perhaps just to initially fix the bug, we could just implement NOP pin() and unpin() callbacks and drop the locking in map_attach() and replace it with an assert_object_held();
That is the sticky part of the move notify API.
If you do the attach_dynamic you have to have an ops with move_notify.
(https://elixir.bootlin.com/linux/v5.13-rc7/source/drivers/dma-buf/dma-
buf.c#L730)
If you don't have that, i.e. just the pin interface, the attach will be rejected, and you will not get the callbacks.
I understood that as the requirement for move_notify is only if the *importer* declares dynamic. A dynamic exporter could choose whether to call move_notify() on eviction or to pin and never evict. If the importer is non-dynamic, the core calls pin() and the only choice is to pin and never evict.
So if we temporarily choose to pin and never evict for *everything*, (as the current code does now), I think we should be good for now, and then we can implement all fancy p2p and move_notify stuff on top of that.
/sigh.
You are correct. I was mistakenly placing the pin API (dma_buf_ops) in the attach_ops. 😐 Must be Friday.
Upon further reflection, I think that your path will work.
However, is doing a pin (with no locking) from the dma_buf_mapping any different from using the pin API + export_dynamic?
M
Yes, it's different for dynamic importers only that would otherwise never pin, and we could mistakenly evict the object without having implemented calling move_notify. If we pin, we never evict.
/Thomas
/Thomas
So I think that the only thing we can do for now is to dop the locking and add
the
assert_object_held();
M
/Thomas
-----Original Message----- From: Thomas Hellström thomas.hellstrom@linux.intel.com Sent: Friday, June 25, 2021 3:10 PM>To: Ruhl, Michael J michael.j.ruhl@intel.com; intel- gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Cc: Auld, Matthew matthew.auld@intel.com Subject: Re: [PATCH 4/4] drm/i915/gem: Migrate to system at dma-buf map time
On 6/25/21 9:07 PM, Ruhl, Michael J wrote:
-----Original Message----- From: Thomas Hellström thomas.hellstrom@linux.intel.com Sent: Friday, June 25, 2021 2:50 PM To: Ruhl, Michael J michael.j.ruhl@intel.com; intel- gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Cc: Auld, Matthew matthew.auld@intel.com Subject: Re: [PATCH 4/4] drm/i915/gem: Migrate to system at dma-buf
map
time
Hi, Mike,
On 6/25/21 7:57 PM, Ruhl, Michael J wrote:
-----Original Message----- From: Thomas Hellström thomas.hellstrom@linux.intel.com Sent: Friday, June 25, 2021 1:52 PM To: Ruhl, Michael J michael.j.ruhl@intel.com; intel- gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Cc: Auld, Matthew matthew.auld@intel.com Subject: Re: [PATCH 4/4] drm/i915/gem: Migrate to system at dma-buf
map
time
On 6/25/21 7:38 PM, Ruhl, Michael J wrote:
> -----Original Message----- > From: Thomas Hellström thomas.hellstrom@linux.intel.com > Sent: Friday, June 25, 2021 12:18 PM > To: Ruhl, Michael J michael.j.ruhl@intel.com; intel- > gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org > Cc: Auld, Matthew matthew.auld@intel.com > Subject: Re: [PATCH 4/4] drm/i915/gem: Migrate to system at dma-
buf
map
> time > > Hi, Michael, > > thanks for looking at this. > > On 6/25/21 6:02 PM, Ruhl, Michael J wrote: >>> -----Original Message----- >>> From: dri-devel dri-devel-bounces@lists.freedesktop.org On
Behalf
Of
>>> Thomas Hellström >>> Sent: Thursday, June 24, 2021 2:31 PM >>> To: intel-gfx@lists.freedesktop.org; dri-
devel@lists.freedesktop.org
>>> Cc: Thomas Hellström thomas.hellstrom@linux.intel.com; Auld, > Matthew >>> matthew.auld@intel.com >>> Subject: [PATCH 4/4] drm/i915/gem: Migrate to system at dma-buf
map
> time >>> Until we support p2p dma or as a complement to that, migrate data >>> to system memory at dma-buf map time if possible. >>> >>> Signed-off-by: Thomas Hellström
thomas.hellstrom@linux.intel.com
>>> --- >>> drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 9 ++++++++- >>> 1 file changed, 8 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c >>> b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c >>> index 616c3a2f1baf..a52f885bc09a 100644 >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c >>> @@ -25,7 +25,14 @@ static struct sg_table > *i915_gem_map_dma_buf(struct >>> dma_buf_attachment *attachme >>> struct scatterlist *src, *dst; >>> int ret, i; >>> >>> - ret = i915_gem_object_pin_pages_unlocked(obj); >>> + ret = i915_gem_object_lock_interruptible(obj, NULL); >> Hmm, I believe in most cases that the caller should be holding the >> lock (object dma-resv) on this object already. > Yes, I agree, In particular for other instances of our own driver, at > least since the dma_resv introduction. > > But I also think that's a pre-existing bug, since > i915_gem_object_pin_pages_unlocked() will also take the lock. Ouch yes. Missed that.
> I Think we need to initially make the exporter dynamic-capable to > resolve this, and drop the locking here completely, as dma-buf docs
says
> that we're then guaranteed to get called with the object lock held. > > I figure if we make the exporter dynamic, we need to migrate already
at
> dma_buf_pin time so we don't pin the object in the wrong location. The exporter as dynamic (ops->pin is available) is optional, but
importer
dynamic (ops->move_notify) is required.
With that in mind, it would seem that there are three possible
combinations
for the migrate to be attempted:
- in the ops->pin function (export_dynamic != import_dynamic,
during
attach)
- in the ops->pin function (export_dynamic and
!CONFIG_DMABUF_MOVE_NOTIFY) during mapping
- and possibly in ops->map_dma_buf (exort_dynamic iand
CONFIG_DMABUF_MOVE_NOTIFY)
Since one possibility has to be in the mapping function, it seems that if
we
can figure out the locking, that the migrate should probably be
available
here.
Mike
So perhaps just to initially fix the bug, we could just implement NOP pin() and unpin() callbacks and drop the locking in map_attach() and replace it with an assert_object_held();
That is the sticky part of the move notify API.
If you do the attach_dynamic you have to have an ops with move_notify.
(https://elixir.bootlin.com/linux/v5.13-rc7/source/drivers/dma-buf/dma-
buf.c#L730)
If you don't have that, i.e. just the pin interface, the attach will be rejected, and you will not get the callbacks.
I understood that as the requirement for move_notify is only if the *importer* declares dynamic. A dynamic exporter could choose whether
to
call move_notify() on eviction or to pin and never evict. If the importer is non-dynamic, the core calls pin() and the only choice is to pin and never evict.
So if we temporarily choose to pin and never evict for *everything*, (as the current code does now), I think we should be good for now, and then we can implement all fancy p2p and move_notify stuff on top of that.
/sigh.
You are correct. I was mistakenly placing the pin API (dma_buf_ops) in the attach_ops. 😐 Must be Friday.
Upon further reflection, I think that your path will work.
However, is doing a pin (with no locking) from the dma_buf_mapping any
different
from using the pin API + export_dynamic?
M
Yes, it's different for dynamic importers only that would otherwise never pin, and we could mistakenly evict the object without having implemented calling move_notify. If we pin, we never evict.
Ahh. Got it. That is an interesting nuance. I need to remember that there are other things than i915... 😊
So that would definitely put the migrate code in the pin path.
M
/Thomas
/Thomas
So I think that the only thing we can do for now is to dop the locking and
add
the
assert_object_held();
M
/Thomas
dri-devel@lists.freedesktop.org
-
Matthew Auld -
Ruhl, Michael J -
Thomas Hellström