[PATCH] drm/xe: Add support for object purgeability
Mishra, Pallavi
pallavi.mishra at intel.com
Thu Oct 3 22:11:37 UTC 2024
> -----Original Message-----
> From: Auld, Matthew <matthew.auld at intel.com>
> Sent: Monday, September 30, 2024 2:15 AM
> To: Mishra, Pallavi <pallavi.mishra at intel.com>; intel-xe at lists.freedesktop.org
> Cc: thomas.hellstrom at linux.intel.com; Zuo, Alex <alex.zuo at intel.com>
> Subject: Re: [PATCH] drm/xe: Add support for object purgeability
>
> On 30/09/2024 01:14, Pallavi Mishra wrote:
> > Introduce madvise ioctl in KMD for object level advice.
> > Userspace applications can utilize this feature to inform KMD whether
> > an object will be needed in near future or can be safely discarded
> > under memory pressure.
>
> So this allows re-using the object after being purged? If so needs to be
> mindful of bo->ccs_cleared [1] AFAICT. Also would need to reset bo->madv
> somewhere? I guess this is sightly different from i915 where the object
> becomes dead once PURGED. Not sure if we also want that or not.
Yes the contents of the bo are cleared, but bo can be re-used later after
changing madv to WILLNEED.
You mean reset bo->madv from purged to WILLNEED?
>
> [1] https://patchwork.freedesktop.org/series/138889/
Thanks for sharing this.
>
> >
> > Signed-off-by: Pallavi Mishra <pallavi.mishra at intel.com>
>
> Needs some UMD Cc.
Will add.
>
> > ---
> > drivers/gpu/drm/xe/xe_bo.c | 71 ++++++++++++++++++++++++++------
> > drivers/gpu/drm/xe/xe_bo.h | 7 ++++
> > drivers/gpu/drm/xe/xe_bo_types.h | 3 ++
> > drivers/gpu/drm/xe/xe_device.c | 1 +
> > include/uapi/drm/xe_drm.h | 20 +++++++++
> > 5 files changed, 90 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> > index 5f2f1ec46b57..0e7e7a624676 100644
> > --- a/drivers/gpu/drm/xe/xe_bo.c
> > +++ b/drivers/gpu/drm/xe/xe_bo.c
> > @@ -644,6 +644,22 @@ static int xe_bo_move_notify(struct xe_bo *bo,
> > return 0;
> > }
> >
> > +static void xe_ttm_bo_purge(struct ttm_buffer_object *ttm_bo, struct
> > +ttm_operation_ctx *ctx) {
> > + struct xe_device *xe = ttm_to_xe_device(ttm_bo->bdev);
> > + struct xe_bo *bo = ttm_to_xe_bo(ttm_bo);
> > +
> > + if (ttm_bo->ttm) {
> > + struct ttm_placement place = {};
> > + int ret = ttm_bo_validate(ttm_bo, &place, ctx);
> > +
> > + drm_WARN_ON(&xe->drm, ret);
> > +
> > + if (bo && !ret)
> > + bo->madv = XE_MADV_PURGED;
> > + }
> > +}
> > +
> > static int xe_bo_move(struct ttm_buffer_object *ttm_bo, bool evict,
> > struct ttm_operation_ctx *ctx,
> > struct ttm_resource *new_mem, @@ -663,6 +679,11 @@
> static
> > int xe_bo_move(struct ttm_buffer_object *ttm_bo, bool evict,
> > ttm && ttm_tt_is_populated(ttm)) ? true :
> false;
> > int ret = 0;
> >
> > + if (bo->madv == XE_MADV_DONTNEED) {
> > + xe_ttm_bo_purge(ttm_bo, ctx);
> > + return ret;
>
> So say user does DONTNEED on some non-populated bo and then tries to
> use that object in something like vm_bind, this here purges it instead of
> populating it which gives NULL bo->resource and returns success. But that
> would mean we have NULL bo->resource after calling bo_validate which I
> think will crash with NPD at some later point, since that shouldn't ever
> happen.
User would need to WILLNEED the bo before using it.
Can add an additional check in vm bind to ensure the madv is WILLNEED?
>
> It looks like we can potentially set empty placement in evict_flags() depending
> on the madv and then ttm will do the purge for us (see
> ttm_bo_evict()) which should then handle the eviction case.. And then for
Yes. Can call xe_ttm_bo_purge here based on madv status.
> swap case we already call this in swap_notify(). I think normally we only need
> to purge stuff when low on memory, like with eviction/swap.
> Not sure what to do here if not WILLNEED, do we just return an error or are
If not WILLNEED, that would be purged then. Purged objects should return.
> we meant to continue on, or perhaps something else?
>
> > + }
> > +
> > /* Bo creation path, moving to system or TT. */
> > if ((!old_mem && ttm) && !handle_system_ccs) {
> > if (new_mem->mem_type == XE_PL_TT) @@ -1084,18
> +1105,6 @@ static
> > void xe_ttm_bo_delete_mem_notify(struct ttm_buffer_object *ttm_bo)
> > }
> > }
> >
> > -static void xe_ttm_bo_purge(struct ttm_buffer_object *ttm_bo, struct
> > ttm_operation_ctx *ctx) -{
> > - struct xe_device *xe = ttm_to_xe_device(ttm_bo->bdev);
> > -
> > - if (ttm_bo->ttm) {
> > - struct ttm_placement place = {};
> > - int ret = ttm_bo_validate(ttm_bo, &place, ctx);
> > -
> > - drm_WARN_ON(&xe->drm, ret);
> > - }
> > -}
> > -
> > static void xe_ttm_bo_swap_notify(struct ttm_buffer_object *ttm_bo)
> > {
> > struct ttm_operation_ctx ctx = {
> > @@ -2416,6 +2425,44 @@ void
> xe_bo_runtime_pm_release_mmap_offset(struct xe_bo *bo)
> > list_del_init(&bo->vram_userfault_link);
> > }
> >
> > +int xe_vm_madvise_ioctl(struct drm_device *dev, void *data,
> > + struct drm_file *file_priv)
> > +{
> > + struct xe_device *xe = to_xe_device(dev);
> > + struct drm_xe_vm_madvise *args = data;
> > + struct drm_gem_object *gem_obj =
> drm_gem_object_lookup(file_priv,
> > +args->handle);
>
> This is also grabbing a bo ref underneath, which needs to be dropped
> otherwise the bo is leaked.
Will take care of this.
>
> > + struct xe_bo *bo;
> > + int err;
> > +
> > + if (XE_IOCTL_DBG(xe, (!(args->madv & DRM_XE_MADV_DONTNEED)
> &&
> > + !(args->madv & DRM_XE_MADV_WILLNEED))))
>
> madv doesn't seem to be flags, but rather a single value? Also user can do
> stuff like WILLNEED | DONTNEED and we don't reject it.
Right. Will modify.
>
> > + return -EINVAL;
> > +
> > + if (XE_IOCTL_DBG(xe, !gem_obj))
> > + return -EINVAL;
> > +
> > + bo = gem_to_xe_bo(gem_obj);
> > + if (!bo)
>
> I don't think bo can be NULL here. I think this is only possible with ghost
> objects, but those are internal to kmd.
>
> > + return -ENOENT;
> > +
> > + err = xe_bo_lock(bo, true);
> > + if (err)
> > + return err;
> > +
> > + if (XE_IOCTL_DBG(xe, ((bo->madv == XE_MADV_PURGED) && (args-
> >madv ==
> > +DRM_XE_MADV_WILLNEED)))) {
>
> Probably don't want to spam dmesg with this, since this is normal and
> expected if something got evicted?
Yes.
>
> Also should we validate that args->retained is zero above or set it here?
>
> > + xe_bo_unlock(bo);
> > + return -EINVAL;
> > + }
> > +
> > + args->retained = (bo->madv != XE_MADV_PURGED) ? 1 : 0;
> > +
> > + bo->madv = (args->madv == DRM_XE_MADV_WILLNEED) ?
> XE_MADV_WILLNEED :
> > +XE_MADV_DONTNEED;
>
> Is there a way to influence the LRU stuff here? It might make sense to bump
> DONTNEED so that stuff like eviction will victimize the stuff that can be freely
> discarded first.
Maybe change the ttm bo priority as well?
>
>
> > +
> > + xe_bo_unlock(bo);
> > +
> > + return 0;
> > +}
> > +
> > #if IS_ENABLED(CONFIG_DRM_XE_KUNIT_TEST)
> > #include "tests/xe_bo.c"
> > #endif
> > diff --git a/drivers/gpu/drm/xe/xe_bo.h b/drivers/gpu/drm/xe/xe_bo.h
> > index 6e4be52306df..515dea54a8e8 100644
> > --- a/drivers/gpu/drm/xe/xe_bo.h
> > +++ b/drivers/gpu/drm/xe/xe_bo.h
> > @@ -63,6 +63,10 @@
> >
> > #define XE_BO_PROPS_INVALID (-1)
> >
> > +#define XE_MADV_WILLNEED 0
> > +#define XE_MADV_DONTNEED 1
> > +#define XE_MADV_PURGED 2
> > +
> > struct sg_table;
> >
> > struct xe_bo *xe_bo_alloc(void);
> > @@ -223,6 +227,9 @@ int xe_gem_create_ioctl(struct drm_device *dev,
> void *data,
> > struct drm_file *file);
> > int xe_gem_mmap_offset_ioctl(struct drm_device *dev, void *data,
> > struct drm_file *file);
> > +int xe_vm_madvise_ioctl(struct drm_device *dev, void *data,
> > + struct drm_file *file_priv);
> > +
> > void xe_bo_runtime_pm_release_mmap_offset(struct xe_bo *bo);
> >
> > int xe_bo_dumb_create(struct drm_file *file_priv, diff --git
> > a/drivers/gpu/drm/xe/xe_bo_types.h b/drivers/gpu/drm/xe/xe_bo_types.h
> > index 2ed558ac2264..00b4de912dcf 100644
> > --- a/drivers/gpu/drm/xe/xe_bo_types.h
> > +++ b/drivers/gpu/drm/xe/xe_bo_types.h
> > @@ -76,6 +76,9 @@ struct xe_bo {
> >
> > /** @vram_userfault_link: Link into
> @mem_access.vram_userfault.list */
> > struct list_head vram_userfault_link;
> > +
> > + /** @madv: user space advise on obj purgeability */
> > + u16 madv;
> > };
> >
> > #define intel_bo_to_drm_bo(bo) (&(bo)->ttm.base) diff --git
> > a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> > index 8e9b551c7033..efeb0bf08b1b 100644
> > --- a/drivers/gpu/drm/xe/xe_device.c
> > +++ b/drivers/gpu/drm/xe/xe_device.c
> > @@ -197,6 +197,7 @@ static const struct drm_ioctl_desc xe_ioctls[] = {
> > DRM_IOCTL_DEF_DRV(XE_WAIT_USER_FENCE,
> xe_wait_user_fence_ioctl,
> > DRM_RENDER_ALLOW),
> > DRM_IOCTL_DEF_DRV(XE_OBSERVATION, xe_observation_ioctl,
> > DRM_RENDER_ALLOW),
> > + DRM_IOCTL_DEF_DRV(XE_VM_MADVISE, xe_vm_madvise_ioctl,
> > +DRM_RENDER_ALLOW),
> > };
> >
> > static long xe_drm_ioctl(struct file *file, unsigned int cmd,
> > unsigned long arg) diff --git a/include/uapi/drm/xe_drm.h
> > b/include/uapi/drm/xe_drm.h index b6fbe4988f2e..e32ba5598a8b 100644
> > --- a/include/uapi/drm/xe_drm.h
> > +++ b/include/uapi/drm/xe_drm.h
> > @@ -102,6 +102,7 @@ extern "C" {
> > #define DRM_XE_EXEC 0x09
> > #define DRM_XE_WAIT_USER_FENCE 0x0a
> > #define DRM_XE_OBSERVATION 0x0b
> > +#define DRM_XE_VM_MADVISE 0x0c
> >
> > /* Must be kept compact -- no holes */
> >
> > @@ -117,6 +118,7 @@ extern "C" {
> > #define DRM_IOCTL_XE_EXEC
> DRM_IOW(DRM_COMMAND_BASE + DRM_XE_EXEC, struct
> drm_xe_exec)
> > #define DRM_IOCTL_XE_WAIT_USER_FENCE
> DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_WAIT_USER_FENCE,
> struct drm_xe_wait_user_fence)
> > #define DRM_IOCTL_XE_OBSERVATION
> DRM_IOW(DRM_COMMAND_BASE + DRM_XE_OBSERVATION, struct
> drm_xe_observation_param)
> > +#define DRM_IOCTL_XE_VM_MADVISE
> DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_VM_MADVISE, struct
> drm_xe_vm_madvise)
> >
> > /**
> > * DOC: Xe IOCTL Extensions
> > @@ -1694,6 +1696,24 @@ struct drm_xe_oa_stream_info {
> > __u64 reserved[3];
> > };
> >
> > +/**
> > + * struct drm_xe_vm_madvise - Input of &DRM_IOCTL_XE_VM_MADVISE
> */
> > +struct drm_xe_vm_madvise {
>
> What is the relation to 'vm' here?
No relation actually. Will rename to drm_xe_bo_madvise
>
> > + /** Handle of the buffer to change the backing store advice */
> > + __u32 handle;
> > + /* Advice: either the buffer will be needed again in the near future,
> > + * or won't be and could be discarded under memory pressure.
> > + */
>
> I think needs properly formatted kernel-doc. Below also.
>
> > +#define DRM_XE_MADV_WILLNEED 0
> > +#define DRM_XE_MADV_DONTNEED 1
> > +
> > + __u32 madv;
> > +
> > + /** Whether the backing store still exists. */
> > + __u32 retained;
> > +};
> > +
> > #if defined(__cplusplus)
> > }
> > #endif
Thanks for the review!
Regards,
Pallavi
More information about the Intel-xe
mailing list