Re: [PATCH v2 11/11] drm/xe: Add debugfs support for page reclamation

[Matthew Brost <matthew.brost@intel.com>]

On Tue, Dec 02, 2025 at 03:51:53PM -0700, Nguyen, Brian3 wrote:
> On Tuesday, December 2, 2025 2:29 PM, Matthew Brost wrote:
> > On Thu, Nov 27, 2025 at 07:02:12AM +0800, Brian Nguyen wrote:
> > > Allow for runtime modification to page reclamation feature through
> > > debugfs configuration. This parameter will only take effect if the
> > > platform supports the page reclamation feature by default.
> > >
> > > v2:
> > >  - Minor comment tweaks. (Shuicheng)
> > >  - Convert to kstrtobool_from_user. (Michal)
> > >  - Only expose page reclaim file if page reclaim flag
> > >    initially supported and with that, remove
> > >    xe_match_desc usage. (Michal)
> > >
> > > Signed-off-by: Brian Nguyen <brian3.nguyen@intel.com>
> > > Cc: Shuicheng Lin <shuicheng.lin@intel.com>
> > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_debugfs.c | 41
> > > +++++++++++++++++++++++++++++++++
> > >  1 file changed, 41 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/xe/xe_debugfs.c
> > > b/drivers/gpu/drm/xe/xe_debugfs.c index 1d5a2a43a9d7..296b8136511b
> > > 100644
> > > --- a/drivers/gpu/drm/xe/xe_debugfs.c
> > > +++ b/drivers/gpu/drm/xe/xe_debugfs.c
> > > @@ -291,6 +291,39 @@ static const struct file_operations wedged_mode_fops = {
> > >  	.write = wedged_mode_set,
> > >  };
> > >
> > > +static ssize_t page_reclaim_hw_assist_show(struct file *f, char __user *ubuf,
> > > +					   size_t size, loff_t *pos)
> > > +{
> > > +	struct xe_device *xe = file_inode(f)->i_private;
> > > +	char buf[8];
> > > +	int len;
> > > +
> > > +	len = scnprintf(buf, sizeof(buf), "%d\n", xe->info.has_page_reclaim_hw_assist);
> > > +	return simple_read_from_buffer(ubuf, size, pos, buf, len); }
> > > +
> > > +static ssize_t page_reclaim_hw_assist_set(struct file *f, const char __user *ubuf,
> > > +					  size_t size, loff_t *pos)
> > > +{
> > > +	struct xe_device *xe = file_inode(f)->i_private;
> > > +	bool val;
> > > +	ssize_t ret;
> > > +
> > > +	ret = kstrtobool_from_user(ubuf, size, &val);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	xe->info.has_page_reclaim_hw_assist = val;
> > 
> > This does seem a little dangerous as someone could set this on unsupported platform or GuC firmware. Should we maybe store
> > page_reclaim_hw_assist_supported + page_reclaim_hw_assist_enabled in the Xe device and only let page_reclaim_hw_assist_enabled
> > move to 1 if page_reclaim_hw_assist_supported is set?
> > 
> > Matt
> > 
> 
> I had a similar check in my first patch, but I believe this is prevented now by not exposing the file on initial xe_debugfs_register.
> This should handle both the unsupported platform who is always false and GuC FW version check, which is done currently at
> xe_guc_init, earlier in initialization than xe_debugfs_register. Is there a missing edge case here?
> 

Ah, yes. I missed the file only gets exposed if
has_page_reclaim_hw_assist is set during the late part of the driver
load. My mistake this patch if fine then.

Sorry for the noise:
Reviewed-by: Matthew Brost <matthew.brost@intel.com> 

> Brian
> 
> > > +
> > > +	return size;
> > > +}
> > > +
> > > +static const struct file_operations page_reclaim_hw_assist_fops = {
> > > +	.owner = THIS_MODULE,
> > > +	.read = page_reclaim_hw_assist_show,
> > > +	.write = page_reclaim_hw_assist_set, };
> > > +
> > >  static ssize_t atomic_svm_timeslice_ms_show(struct file *f, char __user *ubuf,
> > >  					    size_t size, loff_t *pos)
> > >  {
> > > @@ -397,6 +430,14 @@ void xe_debugfs_register(struct xe_device *xe)
> > >  	debugfs_create_file("disable_late_binding", 0600, root, xe,
> > >  			    &disable_late_binding_fops);
> > >
> > > +	/*
> > > +	 * Don't expose page reclaim configuration file if not supported by the
> > > +	 * hardware initially.
> > > +	 */
> > > +	if (xe->info.has_page_reclaim_hw_assist)
> > > +		debugfs_create_file("page_reclaim_hw_assist", 0600, root, xe,
> > > +				    &page_reclaim_hw_assist_fops);
> > > +
> > >  	for (mem_type = XE_PL_VRAM0; mem_type <= XE_PL_VRAM1; ++mem_type) {
> > >  		man = ttm_manager_type(bdev, mem_type);
> > >
> > > --
> > > 2.52.0
> > >