[PATCH] drm/xe: Add support for object purgeability
Matthew Auld
matthew.auld at intel.com
Mon Sep 30 09:14:47 UTC 2024
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.
[1] https://patchwork.freedesktop.org/series/138889/
>
> Signed-off-by: Pallavi Mishra <pallavi.mishra at intel.com>
Needs some UMD Cc.
> ---
> 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.
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 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 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.
> + 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.
> + 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?
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.
> +
> + 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?
> + /** 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
More information about the Intel-xe
mailing list