| Message ID | 20211215110746.865-2-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915: remove writeback hook | expand |
On 15/12/2021 11:07, Matthew Auld wrote: > Add some proper flags for the different modes, and shorten the name to > something more snappy. Looks good to me - but since it touches TTM I leave for Thomas to approve. Regards, Tvrtko P.S. I hope writing the patch means you thought it is an improvement as well, rather than feeling I was asking for it to be done. > Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > --- > .../gpu/drm/i915/gem/i915_gem_object_types.h | 23 ++++++++++++++++--- > drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 8 +++---- > drivers/gpu/drm/i915/gem/i915_gem_shrinker.c | 16 +++++++++---- > drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 10 ++++---- > 4 files changed, 39 insertions(+), 18 deletions(-) > > 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 00c844caeabd..6f446cca4322 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > @@ -57,9 +57,26 @@ struct drm_i915_gem_object_ops { > void (*put_pages)(struct drm_i915_gem_object *obj, > struct sg_table *pages); > int (*truncate)(struct drm_i915_gem_object *obj); > - int (*shrinker_release_pages)(struct drm_i915_gem_object *obj, > - bool no_gpu_wait, > - bool should_writeback); > + /** > + * shrink - Perform further backend specific actions to facilate > + * shrinking. > + * @obj: The gem object > + * @flags: Extra flags to control shrinking behaviour in the backend > + * > + * Possible values for @flags: > + * > + * I915_GEM_OBJECT_SHRINK_WRITEBACK - Try to perform writeback of the > + * backing pages, if supported. > + * > + * I915_GEM_OBJECT_SHRINK_NO_GPU_WAIT - Don't wait for the object to > + * idle. Active objects can be considered later. The TTM backend for > + * example might have aync migrations going on, which don't use any > + * i915_vma to track the active GTT binding, and hence having an unbound > + * object might not be enough. > + */ > +#define I915_GEM_OBJECT_SHRINK_WRITEBACK BIT(0) > +#define I915_GEM_OBJECT_SHRINK_NO_GPU_WAIT BIT(1) > + int (*shrink)(struct drm_i915_gem_object *obj, unsigned int flags); > > int (*pread)(struct drm_i915_gem_object *obj, > const struct drm_i915_gem_pread *arg); > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > index 7fdf4fa10b0e..6c57b0a79c8a 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > @@ -331,9 +331,7 @@ shmem_writeback(struct drm_i915_gem_object *obj) > __shmem_writeback(obj->base.size, obj->base.filp->f_mapping); > } > > -static int shmem_shrinker_release_pages(struct drm_i915_gem_object *obj, > - bool no_gpu_wait, > - bool writeback) > +static int shmem_shrink(struct drm_i915_gem_object *obj, unsigned int flags) > { > switch (obj->mm.madv) { > case I915_MADV_DONTNEED: > @@ -342,7 +340,7 @@ static int shmem_shrinker_release_pages(struct drm_i915_gem_object *obj, > return 0; > } > > - if (writeback) > + if (flags & I915_GEM_OBJECT_SHRINK_WRITEBACK) > shmem_writeback(obj); > > return 0; > @@ -520,7 +518,7 @@ const struct drm_i915_gem_object_ops i915_gem_shmem_ops = { > .get_pages = shmem_get_pages, > .put_pages = shmem_put_pages, > .truncate = shmem_truncate, > - .shrinker_release_pages = shmem_shrinker_release_pages, > + .shrink = shmem_shrink, > > .pwrite = shmem_pwrite, > .pread = shmem_pread, > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c > index fd54e05521f6..968ca0fdd57b 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c > @@ -57,10 +57,18 @@ static bool unsafe_drop_pages(struct drm_i915_gem_object *obj, > > static int try_to_writeback(struct drm_i915_gem_object *obj, unsigned int flags) > { > - if (obj->ops->shrinker_release_pages) > - return obj->ops->shrinker_release_pages(obj, > - !(flags & I915_SHRINK_ACTIVE), > - flags & I915_SHRINK_WRITEBACK); > + if (obj->ops->shrink) { > + unsigned int shrink_flags = 0; > + > + if (!(flags & I915_SHRINK_ACTIVE)) > + shrink_flags |= I915_GEM_OBJECT_SHRINK_NO_GPU_WAIT; > + > + if (flags & I915_SHRINK_WRITEBACK) > + shrink_flags |= I915_GEM_OBJECT_SHRINK_WRITEBACK; > + > + return obj->ops->shrink(obj, shrink_flags); > + } > + > return 0; > } > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > index 923cc7ad8d70..21277f3c64e7 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > @@ -424,16 +424,14 @@ int i915_ttm_purge(struct drm_i915_gem_object *obj) > return 0; > } > > -static int i915_ttm_shrinker_release_pages(struct drm_i915_gem_object *obj, > - bool no_wait_gpu, > - bool should_writeback) > +static int i915_ttm_shrink(struct drm_i915_gem_object *obj, unsigned int flags) > { > struct ttm_buffer_object *bo = i915_gem_to_ttm(obj); > struct i915_ttm_tt *i915_tt = > container_of(bo->ttm, typeof(*i915_tt), ttm); > struct ttm_operation_ctx ctx = { > .interruptible = true, > - .no_wait_gpu = no_wait_gpu, > + .no_wait_gpu = flags & I915_GEM_OBJECT_SHRINK_NO_GPU_WAIT, > }; > struct ttm_placement place = {}; > int ret; > @@ -467,7 +465,7 @@ static int i915_ttm_shrinker_release_pages(struct drm_i915_gem_object *obj, > return ret; > } > > - if (should_writeback) > + if (flags & I915_GEM_OBJECT_SHRINK_WRITEBACK) > __shmem_writeback(obj->base.size, i915_tt->filp->f_mapping); > > return 0; > @@ -953,7 +951,7 @@ static const struct drm_i915_gem_object_ops i915_gem_ttm_obj_ops = { > .get_pages = i915_ttm_get_pages, > .put_pages = i915_ttm_put_pages, > .truncate = i915_ttm_purge, > - .shrinker_release_pages = i915_ttm_shrinker_release_pages, > + .shrink = i915_ttm_shrink, > > .adjust_lru = i915_ttm_adjust_lru, > .delayed_free = i915_ttm_delayed_free, >
On 15/12/2021 15:55, Tvrtko Ursulin wrote: > > On 15/12/2021 11:07, Matthew Auld wrote: >> Add some proper flags for the different modes, and shorten the name to >> something more snappy. > > Looks good to me - but since it touches TTM I leave for Thomas to approve. > > Regards, > > Tvrtko > > P.S. I hope writing the patch means you thought it is an improvement as > well, rather than feeling I was asking for it to be done. Yes, I do see both patches as an improvement :) > >> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> >> Signed-off-by: Matthew Auld <matthew.auld@intel.com> >> --- >> .../gpu/drm/i915/gem/i915_gem_object_types.h | 23 ++++++++++++++++--- >> drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 8 +++---- >> drivers/gpu/drm/i915/gem/i915_gem_shrinker.c | 16 +++++++++---- >> drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 10 ++++---- >> 4 files changed, 39 insertions(+), 18 deletions(-) >> >> 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 00c844caeabd..6f446cca4322 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h >> @@ -57,9 +57,26 @@ struct drm_i915_gem_object_ops { >> void (*put_pages)(struct drm_i915_gem_object *obj, >> struct sg_table *pages); >> int (*truncate)(struct drm_i915_gem_object *obj); >> - int (*shrinker_release_pages)(struct drm_i915_gem_object *obj, >> - bool no_gpu_wait, >> - bool should_writeback); >> + /** >> + * shrink - Perform further backend specific actions to facilate >> + * shrinking. >> + * @obj: The gem object >> + * @flags: Extra flags to control shrinking behaviour in the backend >> + * >> + * Possible values for @flags: >> + * >> + * I915_GEM_OBJECT_SHRINK_WRITEBACK - Try to perform writeback of >> the >> + * backing pages, if supported. >> + * >> + * I915_GEM_OBJECT_SHRINK_NO_GPU_WAIT - Don't wait for the object to >> + * idle. Active objects can be considered later. The TTM backend >> for >> + * example might have aync migrations going on, which don't use any >> + * i915_vma to track the active GTT binding, and hence having an >> unbound >> + * object might not be enough. >> + */ >> +#define I915_GEM_OBJECT_SHRINK_WRITEBACK BIT(0) >> +#define I915_GEM_OBJECT_SHRINK_NO_GPU_WAIT BIT(1) >> + int (*shrink)(struct drm_i915_gem_object *obj, unsigned int flags); >> int (*pread)(struct drm_i915_gem_object *obj, >> const struct drm_i915_gem_pread *arg); >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c >> b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c >> index 7fdf4fa10b0e..6c57b0a79c8a 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c >> @@ -331,9 +331,7 @@ shmem_writeback(struct drm_i915_gem_object *obj) >> __shmem_writeback(obj->base.size, obj->base.filp->f_mapping); >> } >> -static int shmem_shrinker_release_pages(struct drm_i915_gem_object *obj, >> - bool no_gpu_wait, >> - bool writeback) >> +static int shmem_shrink(struct drm_i915_gem_object *obj, unsigned int >> flags) >> { >> switch (obj->mm.madv) { >> case I915_MADV_DONTNEED: >> @@ -342,7 +340,7 @@ static int shmem_shrinker_release_pages(struct >> drm_i915_gem_object *obj, >> return 0; >> } >> - if (writeback) >> + if (flags & I915_GEM_OBJECT_SHRINK_WRITEBACK) >> shmem_writeback(obj); >> return 0; >> @@ -520,7 +518,7 @@ const struct drm_i915_gem_object_ops >> i915_gem_shmem_ops = { >> .get_pages = shmem_get_pages, >> .put_pages = shmem_put_pages, >> .truncate = shmem_truncate, >> - .shrinker_release_pages = shmem_shrinker_release_pages, >> + .shrink = shmem_shrink, >> .pwrite = shmem_pwrite, >> .pread = shmem_pread, >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c >> b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c >> index fd54e05521f6..968ca0fdd57b 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c >> @@ -57,10 +57,18 @@ static bool unsafe_drop_pages(struct >> drm_i915_gem_object *obj, >> static int try_to_writeback(struct drm_i915_gem_object *obj, >> unsigned int flags) >> { >> - if (obj->ops->shrinker_release_pages) >> - return obj->ops->shrinker_release_pages(obj, >> - !(flags & I915_SHRINK_ACTIVE), >> - flags & I915_SHRINK_WRITEBACK); >> + if (obj->ops->shrink) { >> + unsigned int shrink_flags = 0; >> + >> + if (!(flags & I915_SHRINK_ACTIVE)) >> + shrink_flags |= I915_GEM_OBJECT_SHRINK_NO_GPU_WAIT; >> + >> + if (flags & I915_SHRINK_WRITEBACK) >> + shrink_flags |= I915_GEM_OBJECT_SHRINK_WRITEBACK; >> + >> + return obj->ops->shrink(obj, shrink_flags); >> + } >> + >> return 0; >> } >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c >> b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c >> index 923cc7ad8d70..21277f3c64e7 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c >> @@ -424,16 +424,14 @@ int i915_ttm_purge(struct drm_i915_gem_object *obj) >> return 0; >> } >> -static int i915_ttm_shrinker_release_pages(struct drm_i915_gem_object >> *obj, >> - bool no_wait_gpu, >> - bool should_writeback) >> +static int i915_ttm_shrink(struct drm_i915_gem_object *obj, unsigned >> int flags) >> { >> struct ttm_buffer_object *bo = i915_gem_to_ttm(obj); >> struct i915_ttm_tt *i915_tt = >> container_of(bo->ttm, typeof(*i915_tt), ttm); >> struct ttm_operation_ctx ctx = { >> .interruptible = true, >> - .no_wait_gpu = no_wait_gpu, >> + .no_wait_gpu = flags & I915_GEM_OBJECT_SHRINK_NO_GPU_WAIT, >> }; >> struct ttm_placement place = {}; >> int ret; >> @@ -467,7 +465,7 @@ static int i915_ttm_shrinker_release_pages(struct >> drm_i915_gem_object *obj, >> return ret; >> } >> - if (should_writeback) >> + if (flags & I915_GEM_OBJECT_SHRINK_WRITEBACK) >> __shmem_writeback(obj->base.size, i915_tt->filp->f_mapping); >> return 0; >> @@ -953,7 +951,7 @@ static const struct drm_i915_gem_object_ops >> i915_gem_ttm_obj_ops = { >> .get_pages = i915_ttm_get_pages, >> .put_pages = i915_ttm_put_pages, >> .truncate = i915_ttm_purge, >> - .shrinker_release_pages = i915_ttm_shrinker_release_pages, >> + .shrink = i915_ttm_shrink, >> .adjust_lru = i915_ttm_adjust_lru, >> .delayed_free = i915_ttm_delayed_free, >>
Hi, Matthew On 12/15/21 12:07, Matthew Auld wrote: > Add some proper flags for the different modes, and shorten the name to > something more snappy. > > Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > Signed-off-by: Matthew Auld <matthew.auld@intel.com> LGTM. Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
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 00c844caeabd..6f446cca4322 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h @@ -57,9 +57,26 @@ struct drm_i915_gem_object_ops { void (*put_pages)(struct drm_i915_gem_object *obj, struct sg_table *pages); int (*truncate)(struct drm_i915_gem_object *obj); - int (*shrinker_release_pages)(struct drm_i915_gem_object *obj, - bool no_gpu_wait, - bool should_writeback); + /** + * shrink - Perform further backend specific actions to facilate + * shrinking. + * @obj: The gem object + * @flags: Extra flags to control shrinking behaviour in the backend + * + * Possible values for @flags: + * + * I915_GEM_OBJECT_SHRINK_WRITEBACK - Try to perform writeback of the + * backing pages, if supported. + * + * I915_GEM_OBJECT_SHRINK_NO_GPU_WAIT - Don't wait for the object to + * idle. Active objects can be considered later. The TTM backend for + * example might have aync migrations going on, which don't use any + * i915_vma to track the active GTT binding, and hence having an unbound + * object might not be enough. + */ +#define I915_GEM_OBJECT_SHRINK_WRITEBACK BIT(0) +#define I915_GEM_OBJECT_SHRINK_NO_GPU_WAIT BIT(1) + int (*shrink)(struct drm_i915_gem_object *obj, unsigned int flags); int (*pread)(struct drm_i915_gem_object *obj, const struct drm_i915_gem_pread *arg); diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c index 7fdf4fa10b0e..6c57b0a79c8a 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c @@ -331,9 +331,7 @@ shmem_writeback(struct drm_i915_gem_object *obj) __shmem_writeback(obj->base.size, obj->base.filp->f_mapping); } -static int shmem_shrinker_release_pages(struct drm_i915_gem_object *obj, - bool no_gpu_wait, - bool writeback) +static int shmem_shrink(struct drm_i915_gem_object *obj, unsigned int flags) { switch (obj->mm.madv) { case I915_MADV_DONTNEED: @@ -342,7 +340,7 @@ static int shmem_shrinker_release_pages(struct drm_i915_gem_object *obj, return 0; } - if (writeback) + if (flags & I915_GEM_OBJECT_SHRINK_WRITEBACK) shmem_writeback(obj); return 0; @@ -520,7 +518,7 @@ const struct drm_i915_gem_object_ops i915_gem_shmem_ops = { .get_pages = shmem_get_pages, .put_pages = shmem_put_pages, .truncate = shmem_truncate, - .shrinker_release_pages = shmem_shrinker_release_pages, + .shrink = shmem_shrink, .pwrite = shmem_pwrite, .pread = shmem_pread, diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c index fd54e05521f6..968ca0fdd57b 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c @@ -57,10 +57,18 @@ static bool unsafe_drop_pages(struct drm_i915_gem_object *obj, static int try_to_writeback(struct drm_i915_gem_object *obj, unsigned int flags) { - if (obj->ops->shrinker_release_pages) - return obj->ops->shrinker_release_pages(obj, - !(flags & I915_SHRINK_ACTIVE), - flags & I915_SHRINK_WRITEBACK); + if (obj->ops->shrink) { + unsigned int shrink_flags = 0; + + if (!(flags & I915_SHRINK_ACTIVE)) + shrink_flags |= I915_GEM_OBJECT_SHRINK_NO_GPU_WAIT; + + if (flags & I915_SHRINK_WRITEBACK) + shrink_flags |= I915_GEM_OBJECT_SHRINK_WRITEBACK; + + return obj->ops->shrink(obj, shrink_flags); + } + return 0; } diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c index 923cc7ad8d70..21277f3c64e7 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c @@ -424,16 +424,14 @@ int i915_ttm_purge(struct drm_i915_gem_object *obj) return 0; } -static int i915_ttm_shrinker_release_pages(struct drm_i915_gem_object *obj, - bool no_wait_gpu, - bool should_writeback) +static int i915_ttm_shrink(struct drm_i915_gem_object *obj, unsigned int flags) { struct ttm_buffer_object *bo = i915_gem_to_ttm(obj); struct i915_ttm_tt *i915_tt = container_of(bo->ttm, typeof(*i915_tt), ttm); struct ttm_operation_ctx ctx = { .interruptible = true, - .no_wait_gpu = no_wait_gpu, + .no_wait_gpu = flags & I915_GEM_OBJECT_SHRINK_NO_GPU_WAIT, }; struct ttm_placement place = {}; int ret; @@ -467,7 +465,7 @@ static int i915_ttm_shrinker_release_pages(struct drm_i915_gem_object *obj, return ret; } - if (should_writeback) + if (flags & I915_GEM_OBJECT_SHRINK_WRITEBACK) __shmem_writeback(obj->base.size, i915_tt->filp->f_mapping); return 0; @@ -953,7 +951,7 @@ static const struct drm_i915_gem_object_ops i915_gem_ttm_obj_ops = { .get_pages = i915_ttm_get_pages, .put_pages = i915_ttm_put_pages, .truncate = i915_ttm_purge, - .shrinker_release_pages = i915_ttm_shrinker_release_pages, + .shrink = i915_ttm_shrink, .adjust_lru = i915_ttm_adjust_lru, .delayed_free = i915_ttm_delayed_free,
Add some proper flags for the different modes, and shorten the name to something more snappy. Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Signed-off-by: Matthew Auld <matthew.auld@intel.com> --- .../gpu/drm/i915/gem/i915_gem_object_types.h | 23 ++++++++++++++++--- drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 8 +++---- drivers/gpu/drm/i915/gem/i915_gem_shrinker.c | 16 +++++++++---- drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 10 ++++---- 4 files changed, 39 insertions(+), 18 deletions(-)