Message ID | 20210628144626.76126-2-thomas.hellstrom@linux.intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | drm/i915/gem: Introduce a migrate interface | expand |
On 28/06/2021 15:46, 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 > performance-based migration. > > v2: > - Verify that the memory region given as an id really exists. > (Reported by Matthew Auld) > - Call i915_gem_object_{init,release}_memory_region() when switching region > to handle also switching region lists. (Reported by Matthew Auld) > v3: > - Fix i915_gem_object_can_migrate() to return true if object is already in > the correct region, even if the object ops doesn't have a migrate() > callback. > - Update typo in commit message. > - Fix kerneldoc of i915_gem_object_wait_migration(). > > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_object.c | 96 +++++++++++++++++++ > 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, 188 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..1c18be067b58 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c > @@ -513,6 +513,102 @@ 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); > + > + 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->ops->migrate) > + 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..8f89185b6507 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) { Is this for the migration case? Where are we updating the mm.pages, assuming the migration step has to take care of pre-existing pages? What am I missing? > + /* 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) { > + i915_gem_object_release_memory_region(obj); > + i915_gem_object_init_memory_region(obj, mr); Hmm, so is this not already handled by adjust_gem_after_move()? > + } > + > + 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..f909aaa09d9c 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_migration - 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 6/28/21 6:28 PM, Matthew Auld wrote: > On 28/06/2021 15:46, 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 >> performance-based migration. >> >> v2: >> - Verify that the memory region given as an id really exists. >> (Reported by Matthew Auld) >> - Call i915_gem_object_{init,release}_memory_region() when switching >> region >> to handle also switching region lists. (Reported by Matthew Auld) >> v3: >> - Fix i915_gem_object_can_migrate() to return true if object is >> already in >> the correct region, even if the object ops doesn't have a migrate() >> callback. >> - Update typo in commit message. >> - Fix kerneldoc of i915_gem_object_wait_migration(). >> >> Reported-by: kernel test robot <lkp@intel.com> >> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> >> --- >> drivers/gpu/drm/i915/gem/i915_gem_object.c | 96 +++++++++++++++++++ >> 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, 188 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..1c18be067b58 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c >> @@ -513,6 +513,102 @@ 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); >> + >> + 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->ops->migrate) >> + 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..8f89185b6507 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) { > > Is this for the migration case? Where are we updating the mm.pages, > assuming the migration step has to take care of pre-existing pages? > What am I missing? Hmm, yes this is for a hypotetical migration case where ttm_bo_validate() decided to not change placement of the object, meaning we migrate to the very same region that we're already in. That should be blocked at the gem level, so I guess we could replace this with a GEM_BUG_ON(). > >> + /* 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) { >> + i915_gem_object_release_memory_region(obj); >> + i915_gem_object_init_memory_region(obj, mr); > > Hmm, so is this not already handled by adjust_gem_after_move()? No, not always. adjust_gem_after_move() only considers an object migrated and flips region if the new region is in one of the allowed placements. Otherwise it's considered evicted and the next get_pages() will revalidate into the preferred placement. the gem_migrate interface is more flexible, and offers a possibility to migrate to whatever it's told, so as long as we want that behaviour, we need the above. /Thomas > >> + } >> + >> + 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..f909aaa09d9c 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_migration - 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; >> +} >>
>-----Original Message----- >From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of >Thomas Hellström >Sent: Monday, June 28, 2021 10:46 AM >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>; lkp <lkp@intel.com> >Subject: [PATCH v3 1/5] drm/i915/gem: Implement object migration > >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 >performance-based migration. > >v2: >- Verify that the memory region given as an id really exists. > (Reported by Matthew Auld) >- Call i915_gem_object_{init,release}_memory_region() when switching >region > to handle also switching region lists. (Reported by Matthew Auld) >v3: >- Fix i915_gem_object_can_migrate() to return true if object is already in > the correct region, even if the object ops doesn't have a migrate() > callback. >- Update typo in commit message. >- Fix kerneldoc of i915_gem_object_wait_migration(). > >Reported-by: kernel test robot <lkp@intel.com> >Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> >--- > drivers/gpu/drm/i915/gem/i915_gem_object.c | 96 >+++++++++++++++++++ > 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, 188 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..1c18be067b58 100644 >--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c >+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c >@@ -513,6 +513,102 @@ 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); >+ >+ 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->ops->migrate) >+ 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; >+ } Hi Thomas, I am a little confused over the difference in checks between this function and i915_gem_object_migrate(). Why is the lack of an mr a BUG_ON in _object_migrate(), but here it just false? So that means that under certain circumstances, you could not have a mr? If that is the case, when? Would that be when the I915_BO_ALLOC_USER is set? If so, should there be a check for "non" user vs user? Or maybe document this function pointing out why there are differences and why? >+ 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. Is the ww for future consideration? (I don't see any use of it in the patch). >+ * @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; Why aren't you using _can_migrate here? >+ 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..8f89185b6507 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) { >+ i915_gem_object_release_memory_region(obj); >+ i915_gem_object_init_memory_region(obj, mr); >+ } Perhaps a minor nit: Doing this after we have done the _get_pages() just doesn't seem right. I.e. we do work on the object, and then we init some portion of it. Do we need to do this incase the migration/placement fails? If so, maybe a comment to that effect? Thanks, Mike >+ 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..f909aaa09d9c 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_migration - 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; >+} >-- >2.31.1
On 6/28/21 8:11 PM, Ruhl, Michael J wrote: >> -----Original Message----- >> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of >> Thomas Hellström >> Sent: Monday, June 28, 2021 10:46 AM >> 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>; lkp <lkp@intel.com> >> Subject: [PATCH v3 1/5] drm/i915/gem: Implement object migration >> >> 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 >> performance-based migration. >> >> v2: >> - Verify that the memory region given as an id really exists. >> (Reported by Matthew Auld) >> - Call i915_gem_object_{init,release}_memory_region() when switching >> region >> to handle also switching region lists. (Reported by Matthew Auld) >> v3: >> - Fix i915_gem_object_can_migrate() to return true if object is already in >> the correct region, even if the object ops doesn't have a migrate() >> callback. >> - Update typo in commit message. >> - Fix kerneldoc of i915_gem_object_wait_migration(). >> >> Reported-by: kernel test robot <lkp@intel.com> >> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> >> --- >> drivers/gpu/drm/i915/gem/i915_gem_object.c | 96 >> +++++++++++++++++++ >> 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, 188 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..1c18be067b58 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c >> @@ -513,6 +513,102 @@ 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); >> + >> + 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->ops->migrate) >> + 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; >> + } > Hi Thomas, > > I am a little confused over the difference in checks between this function > and i915_gem_object_migrate(). > > Why is the lack of an mr a BUG_ON in _object_migrate(), but here it just > false? > > So that means that under certain circumstances, you could not have a mr? > > If that is the case, when? > > Would that be when the I915_BO_ALLOC_USER is set? > > If so, should there be a check for "non" user vs user? > > Or maybe document this function pointing out why there are differences > and why? Yes, I'll give it some more documentation. The basic idea is that the above function also could be used to validate user supplied data, whereas there might be cases where we want to use the gem_object_migrate() function and override the above. > >> + 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. > Is the ww for future consideration? (I don't see any use of it in the patch). Yes, but it will remain optional. > >> + * @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; > Why aren't you using _can_migrate here? It's just in case we want to override. I'll add some more comments about this. > >> + 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..8f89185b6507 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) { >> + i915_gem_object_release_memory_region(obj); >> + i915_gem_object_init_memory_region(obj, mr); >> + } > Perhaps a minor nit: > > Doing this after we have done the _get_pages() just doesn't seem right. > > I.e. we do work on the object, and then we init some portion of it. > > Do we need to do this incase the migration/placement fails? If so, > maybe a comment to that effect? This is simply switching memory region under the lock, and to also move to another memory region list. Is it the naming _release_ and _init_ that is confusing? /Thomas > > Thanks, > > Mike > >> + 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..f909aaa09d9c 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_migration - 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; >> +} >> -- >> 2.31.1
>-----Original Message----- >From: Thomas Hellström <thomas.hellstrom@linux.intel.com> >Sent: Monday, June 28, 2021 3:03 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>; lkp <lkp@intel.com> >Subject: Re: [PATCH v3 1/5] drm/i915/gem: Implement object migration > > >On 6/28/21 8:11 PM, Ruhl, Michael J wrote: >>> -----Original Message----- >>> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of >>> Thomas Hellström >>> Sent: Monday, June 28, 2021 10:46 AM >>> 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>; lkp <lkp@intel.com> >>> Subject: [PATCH v3 1/5] drm/i915/gem: Implement object migration >>> >>> 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 >>> performance-based migration. >>> >>> v2: >>> - Verify that the memory region given as an id really exists. >>> (Reported by Matthew Auld) >>> - Call i915_gem_object_{init,release}_memory_region() when switching >>> region >>> to handle also switching region lists. (Reported by Matthew Auld) >>> v3: >>> - Fix i915_gem_object_can_migrate() to return true if object is already in >>> the correct region, even if the object ops doesn't have a migrate() >>> callback. >>> - Update typo in commit message. >>> - Fix kerneldoc of i915_gem_object_wait_migration(). >>> >>> Reported-by: kernel test robot <lkp@intel.com> >>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> >>> --- >>> drivers/gpu/drm/i915/gem/i915_gem_object.c | 96 >>> +++++++++++++++++++ >>> 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, 188 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..1c18be067b58 100644 >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c >>> @@ -513,6 +513,102 @@ 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); >>> + >>> + 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->ops->migrate) >>> + 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; >>> + } >> Hi Thomas, >> >> I am a little confused over the difference in checks between this function >> and i915_gem_object_migrate(). >> >> Why is the lack of an mr a BUG_ON in _object_migrate(), but here it just >> false? >> >> So that means that under certain circumstances, you could not have a mr? >> >> If that is the case, when? >> >> Would that be when the I915_BO_ALLOC_USER is set? >> >> If so, should there be a check for "non" user vs user? >> >> Or maybe document this function pointing out why there are differences >> and why? > >Yes, I'll give it some more documentation. The basic idea is that the >above function also could be >used to validate user supplied data, whereas there might be cases where >we want to use the gem_object_migrate() function and override the above. > > >> >>> + 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. >> Is the ww for future consideration? (I don't see any use of it in the patch). > >Yes, but it will remain optional. > > >> >>> + * @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; >> Why aren't you using _can_migrate here? > >It's just in case we want to override. I'll add some more comments about >this. > >> >>> + 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..8f89185b6507 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) { >>> + i915_gem_object_release_memory_region(obj); >>> + i915_gem_object_init_memory_region(obj, mr); >>> + } >> Perhaps a minor nit: >> >> Doing this after we have done the _get_pages() just doesn't seem right. >> >> I.e. we do work on the object, and then we init some portion of it. >> >> Do we need to do this incase the migration/placement fails? If so, >> maybe a comment to that effect? > >This is simply switching memory region under the lock, and to also move >to another memory region list. Is it the naming _release_ and _init_ >that is confusing? Hmm, re-reading my question, I am wondering if I was unclear. My "real" question was, can the release/init occur before the _get_pages()? But looking at this some more, I can see answer is probably no. I was going to suggest calling _init_ _set_, but when I looked at it, it was doing init things as well as setting things Maybe just a comment like: /* Complete the migration by updating the memory region info. */ if (object->mm...) ? M >/Thomas > > >> >> Thanks, >> >> Mike >> >>> + 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..f909aaa09d9c 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_migration - 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; >>> +} >>> -- >>> 2.31.1
On 6/28/21 9:50 PM, Ruhl, Michael J wrote: >> -----Original Message----- >> From: Thomas Hellström <thomas.hellstrom@linux.intel.com> >> Sent: Monday, June 28, 2021 3:03 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>; lkp <lkp@intel.com> >> Subject: Re: [PATCH v3 1/5] drm/i915/gem: Implement object migration >> >> >> On 6/28/21 8:11 PM, Ruhl, Michael J wrote: >>>> -----Original Message----- >>>> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of >>>> Thomas Hellström >>>> Sent: Monday, June 28, 2021 10:46 AM >>>> 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>; lkp <lkp@intel.com> >>>> Subject: [PATCH v3 1/5] drm/i915/gem: Implement object migration >>>> >>>> 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 >>>> performance-based migration. >>>> >>>> v2: >>>> - Verify that the memory region given as an id really exists. >>>> (Reported by Matthew Auld) >>>> - Call i915_gem_object_{init,release}_memory_region() when switching >>>> region >>>> to handle also switching region lists. (Reported by Matthew Auld) >>>> v3: >>>> - Fix i915_gem_object_can_migrate() to return true if object is already in >>>> the correct region, even if the object ops doesn't have a migrate() >>>> callback. >>>> - Update typo in commit message. >>>> - Fix kerneldoc of i915_gem_object_wait_migration(). >>>> >>>> Reported-by: kernel test robot <lkp@intel.com> >>>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> >>>> --- >>>> drivers/gpu/drm/i915/gem/i915_gem_object.c | 96 >>>> +++++++++++++++++++ >>>> 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, 188 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..1c18be067b58 100644 >>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c >>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c >>>> @@ -513,6 +513,102 @@ 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); >>>> + >>>> + 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->ops->migrate) >>>> + 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; >>>> + } >>> Hi Thomas, >>> >>> I am a little confused over the difference in checks between this function >>> and i915_gem_object_migrate(). >>> >>> Why is the lack of an mr a BUG_ON in _object_migrate(), but here it just >>> false? >>> >>> So that means that under certain circumstances, you could not have a mr? >>> >>> If that is the case, when? >>> >>> Would that be when the I915_BO_ALLOC_USER is set? >>> >>> If so, should there be a check for "non" user vs user? >>> >>> Or maybe document this function pointing out why there are differences >>> and why? >> Yes, I'll give it some more documentation. The basic idea is that the >> above function also could be >> used to validate user supplied data, whereas there might be cases where >> we want to use the gem_object_migrate() function and override the above. >> >> >>>> + 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. >>> Is the ww for future consideration? (I don't see any use of it in the patch). >> Yes, but it will remain optional. >> >> >>>> + * @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; >>> Why aren't you using _can_migrate here? >> It's just in case we want to override. I'll add some more comments about >> this. >> >>>> + 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..8f89185b6507 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) { >>>> + i915_gem_object_release_memory_region(obj); >>>> + i915_gem_object_init_memory_region(obj, mr); >>>> + } >>> Perhaps a minor nit: >>> >>> Doing this after we have done the _get_pages() just doesn't seem right. >>> >>> I.e. we do work on the object, and then we init some portion of it. >>> >>> Do we need to do this incase the migration/placement fails? If so, >>> maybe a comment to that effect? >> This is simply switching memory region under the lock, and to also move >> to another memory region list. Is it the naming _release_ and _init_ >> that is confusing? > Hmm, re-reading my question, I am wondering if I was unclear. > > My "real" question was, can the release/init occur before the _get_pages()? > > But looking at this some more, I can see answer is probably no. > > I was going to suggest calling _init_ _set_, but when I looked at it, it was > doing init things as well as setting things > > Maybe just a comment like: > > /* Complete the migration by updating the memory region info. */ > if (object->mm...) Sure, I'll add that. Note that another thread would have to take the object lock anyway to inspect both the region and the pages, so it shouldn't be confused by the order here. /Thomas
>-----Original Message----- >From: Thomas Hellström <thomas.hellstrom@linux.intel.com> >Sent: Monday, June 28, 2021 3:54 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>; lkp <lkp@intel.com> >Subject: Re: [PATCH v3 1/5] drm/i915/gem: Implement object migration > > >On 6/28/21 9:50 PM, Ruhl, Michael J wrote: >>> -----Original Message----- >>> From: Thomas Hellström <thomas.hellstrom@linux.intel.com> >>> Sent: Monday, June 28, 2021 3:03 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>; lkp <lkp@intel.com> >>> Subject: Re: [PATCH v3 1/5] drm/i915/gem: Implement object migration >>> >>> >>> On 6/28/21 8:11 PM, Ruhl, Michael J wrote: >>>>> -----Original Message----- >>>>> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf >Of >>>>> Thomas Hellström >>>>> Sent: Monday, June 28, 2021 10:46 AM >>>>> 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>; lkp <lkp@intel.com> >>>>> Subject: [PATCH v3 1/5] drm/i915/gem: Implement object migration >>>>> >>>>> 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 >>>>> performance-based migration. >>>>> >>>>> v2: >>>>> - Verify that the memory region given as an id really exists. >>>>> (Reported by Matthew Auld) >>>>> - Call i915_gem_object_{init,release}_memory_region() when switching >>>>> region >>>>> to handle also switching region lists. (Reported by Matthew Auld) >>>>> v3: >>>>> - Fix i915_gem_object_can_migrate() to return true if object is already >in >>>>> the correct region, even if the object ops doesn't have a migrate() >>>>> callback. >>>>> - Update typo in commit message. >>>>> - Fix kerneldoc of i915_gem_object_wait_migration(). >>>>> >>>>> Reported-by: kernel test robot <lkp@intel.com> >>>>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> >>>>> --- >>>>> drivers/gpu/drm/i915/gem/i915_gem_object.c | 96 >>>>> +++++++++++++++++++ >>>>> 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, 188 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..1c18be067b58 100644 >>>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c >>>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c >>>>> @@ -513,6 +513,102 @@ 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); >>>>> + >>>>> + 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->ops->migrate) >>>>> + 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; >>>>> + } >>>> Hi Thomas, >>>> >>>> I am a little confused over the difference in checks between this function >>>> and i915_gem_object_migrate(). >>>> >>>> Why is the lack of an mr a BUG_ON in _object_migrate(), but here it just >>>> false? >>>> >>>> So that means that under certain circumstances, you could not have a mr? >>>> >>>> If that is the case, when? >>>> >>>> Would that be when the I915_BO_ALLOC_USER is set? >>>> >>>> If so, should there be a check for "non" user vs user? >>>> >>>> Or maybe document this function pointing out why there are differences >>>> and why? >>> Yes, I'll give it some more documentation. The basic idea is that the >>> above function also could be >>> used to validate user supplied data, whereas there might be cases where >>> we want to use the gem_object_migrate() function and override the >above. >>> >>> >>>>> + 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. >>>> Is the ww for future consideration? (I don't see any use of it in the >patch). >>> Yes, but it will remain optional. >>> >>> >>>>> + * @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; >>>> Why aren't you using _can_migrate here? >>> It's just in case we want to override. I'll add some more comments about >>> this. >>> >>>>> + 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..8f89185b6507 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) { >>>>> + i915_gem_object_release_memory_region(obj); >>>>> + i915_gem_object_init_memory_region(obj, mr); >>>>> + } >>>> Perhaps a minor nit: >>>> >>>> Doing this after we have done the _get_pages() just doesn't seem right. >>>> >>>> I.e. we do work on the object, and then we init some portion of it. >>>> >>>> Do we need to do this incase the migration/placement fails? If so, >>>> maybe a comment to that effect? >>> This is simply switching memory region under the lock, and to also move >>> to another memory region list. Is it the naming _release_ and _init_ >>> that is confusing? >> Hmm, re-reading my question, I am wondering if I was unclear. >> >> My "real" question was, can the release/init occur before the _get_pages()? >> >> But looking at this some more, I can see answer is probably no. >> >> I was going to suggest calling _init_ _set_, but when I looked at it, it was >> doing init things as well as setting things >> >> Maybe just a comment like: >> >> /* Complete the migration by updating the memory region info. */ >> if (object->mm...) > >Sure, I'll add that. Note that another thread would have to take the >object lock anyway to inspect both the region and the pages, so it >shouldn't be confused by the order here. Understood. I think I was just disoriented by the pattern. :) With the added documentation, I think this looks good. Reviewed-by: Michael J. Ruhl <michael.j.ruhl@intel.com> Mike >/Thomas >
On Mon, Jun 28, 2021 at 04:46:22PM +0200, 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 > performance-based migration. > > v2: > - Verify that the memory region given as an id really exists. > (Reported by Matthew Auld) > - Call i915_gem_object_{init,release}_memory_region() when switching region > to handle also switching region lists. (Reported by Matthew Auld) > v3: > - Fix i915_gem_object_can_migrate() to return true if object is already in > the correct region, even if the object ops doesn't have a migrate() > callback. > - Update typo in commit message. > - Fix kerneldoc of i915_gem_object_wait_migration(). > > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> Looks reasonable to me as the main interface. I'm a bit on the fence on hiding everything behind obj->ops, since we're not going to have another implementation of migrate than the ttm one. But also while everything is in-flight it's probably good to at least try and maintain some boundaries, for otherwise the messiness might become unmanageable Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> But don't count this as real review :-) One suggestion at the very bottom. > --- > drivers/gpu/drm/i915/gem/i915_gem_object.c | 96 +++++++++++++++++++ > 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, 188 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..1c18be067b58 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c > @@ -513,6 +513,102 @@ 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); > + > + 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->ops->migrate) > + 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..8f89185b6507 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) { > + i915_gem_object_release_memory_region(obj); > + i915_gem_object_init_memory_region(obj, 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..f909aaa09d9c 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_migration - 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. */ Maybe dma_fence_might_wait() here, since that's a notch more nasty? Cheers, Daniel > + return 0; > +} > -- > 2.31.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c index 07e8ff9a8aae..1c18be067b58 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c @@ -513,6 +513,102 @@ 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); + + 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->ops->migrate) + 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..8f89185b6507 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) { + i915_gem_object_release_memory_region(obj); + i915_gem_object_init_memory_region(obj, 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..f909aaa09d9c 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_migration - 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; +}
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 performance-based migration. v2: - Verify that the memory region given as an id really exists. (Reported by Matthew Auld) - Call i915_gem_object_{init,release}_memory_region() when switching region to handle also switching region lists. (Reported by Matthew Auld) v3: - Fix i915_gem_object_can_migrate() to return true if object is already in the correct region, even if the object ops doesn't have a migrate() callback. - Update typo in commit message. - Fix kerneldoc of i915_gem_object_wait_migration(). Reported-by: kernel test robot <lkp@intel.com> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> --- drivers/gpu/drm/i915/gem/i915_gem_object.c | 96 +++++++++++++++++++ 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, 188 insertions(+), 17 deletions(-)