[PATCH] drm/xe: Add support for object purgeability

[PATCH] drm/xe: Add support for object purgeability

[Pallavi Mishra]

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.

Signed-off-by: Pallavi Mishra <pallavi.mishra@intel.com>
---
 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;
+	}
+
 	/* 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);
+	struct xe_bo *bo;
+	int err;
+
+	if (XE_IOCTL_DBG(xe, (!(args->madv & DRM_XE_MADV_DONTNEED) &&
+			!(args->madv & DRM_XE_MADV_WILLNEED))))
+		return -EINVAL;
+
+	if (XE_IOCTL_DBG(xe, !gem_obj))
+		return -EINVAL;
+
+	bo = gem_to_xe_bo(gem_obj);
+	if (!bo)
+		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)))) {
+		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;
+
+	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 {
+	/** 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.
+	 */
+#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
-- 
2.25.1

✗ CI.Patch_applied: failure for drm/xe: Add support for object purgeability

[Patchwork]

== Series Details ==

Series: drm/xe: Add support for object purgeability
URL   : https://patchwork.freedesktop.org/series/139264/
State : failure

== Summary ==

=== Applying kernel patches on branch 'drm-tip' with base: ===
Base commit: 1accf1380ac2 drm-tip: 2024y-09m-30d-04h-27m-01s UTC integration manifest
=== git am output follows ===
error: patch failed: drivers/gpu/drm/xe/xe_bo_types.h:76
error: drivers/gpu/drm/xe/xe_bo_types.h: patch does not apply
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Applying: drm/xe: Add support for object purgeability
Patch failed at 0001 drm/xe: Add support for object purgeability
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Re: [PATCH] drm/xe: Add support for object purgeability

[Matthew Auld]

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@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

RE: [PATCH] drm/xe: Add support for object purgeability

[Mishra, Pallavi]

> -----Original Message-----
> From: Auld, Matthew <matthew.auld@intel.com>
> Sent: Monday, September 30, 2024 2:15 AM
> To: Mishra, Pallavi <pallavi.mishra@intel.com>; intel-xe@lists.freedesktop.org
> Cc: thomas.hellstrom@linux.intel.com; Zuo, Alex <alex.zuo@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@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