Re: [PATCH v4 5/6] drm/xe/lrc: Allow to add user commands on context switch

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Sep 16, 2025 at 04:07:20PM -0500, Lucas De Marchi wrote:
> On Tue, Sep 16, 2025 at 04:27:45PM -0400, Rodrigo Vivi wrote:
> > On Thu, Sep 11, 2025 at 12:36:29PM -0700, Lucas De Marchi wrote:
> > > During validation it's useful to allows additional commands to be
> > > executed on context switch. Fetch the commands from configfs (to be
> > > added) and add them to the WA BB.
> > > 
> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > > ---
> > > v2: Fix warning when building without configfs
> > > ---
> > >  drivers/gpu/drm/xe/xe_configfs.c | 13 +++++++++++++
> > >  drivers/gpu/drm/xe/xe_configfs.h |  6 ++++++
> > >  drivers/gpu/drm/xe/xe_lrc.c      | 25 +++++++++++++++++++++++++
> > >  3 files changed, 44 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_configfs.c b/drivers/gpu/drm/xe/xe_configfs.c
> > > index 45fdb6cf26b5d..d86c75af03278 100644
> > > --- a/drivers/gpu/drm/xe/xe_configfs.c
> > > +++ b/drivers/gpu/drm/xe/xe_configfs.c
> > > @@ -633,6 +633,19 @@ bool xe_configfs_get_psmi_enabled(struct pci_dev *pdev)
> > >  	return ret;
> > >  }
> > > 
> > > +/**
> > > + * xe_configfs_get_ctx_restore_post_bb - get configfs ctx_restore_post_bb setting
> > > + * @pdev: pci device
> > > + *
> > > + * Return: post_ctx_restore setting in configfs
> > > + */
> > > +u32 xe_configfs_get_ctx_restore_post_bb(struct pci_dev *pdev,
> > > +					enum xe_engine_class class,
> > > +					const u32 **cs)
> > > +{
> > > +	return 0;
> > > +}
> > > +
> > >  int __init xe_configfs_init(void)
> > >  {
> > >  	int ret;
> > > diff --git a/drivers/gpu/drm/xe/xe_configfs.h b/drivers/gpu/drm/xe/xe_configfs.h
> > > index 1402e863b71c0..eff2645b5f593 100644
> > > --- a/drivers/gpu/drm/xe/xe_configfs.h
> > > +++ b/drivers/gpu/drm/xe/xe_configfs.h
> > > @@ -8,6 +8,8 @@
> > >  #include <linux/limits.h>
> > >  #include <linux/types.h>
> > > 
> > > +#include <xe_hw_engine_types.h>
> > > +
> > >  struct pci_dev;
> > > 
> > >  #if IS_ENABLED(CONFIG_CONFIGFS_FS)
> > > @@ -17,6 +19,8 @@ void xe_configfs_check_device(struct pci_dev *pdev);
> > >  bool xe_configfs_get_survivability_mode(struct pci_dev *pdev);
> > >  u64 xe_configfs_get_engines_allowed(struct pci_dev *pdev);
> > >  bool xe_configfs_get_psmi_enabled(struct pci_dev *pdev);
> > > +u32 xe_configfs_get_ctx_restore_post_bb(struct pci_dev *pdev, enum xe_engine_class,
> > > +					const u32 **cs);
> > >  #else
> > >  static inline int xe_configfs_init(void) { return 0; }
> > >  static inline void xe_configfs_exit(void) { }
> > > @@ -24,6 +28,8 @@ static inline void xe_configfs_check_device(struct pci_dev *pdev) { }
> > >  static inline bool xe_configfs_get_survivability_mode(struct pci_dev *pdev) { return false; }
> > >  static inline u64 xe_configfs_get_engines_allowed(struct pci_dev *pdev) { return U64_MAX; }
> > >  static inline bool xe_configfs_get_psmi_enabled(struct pci_dev *pdev) { return false; }
> > > +static inline u32 xe_configfs_get_ctx_restore_post_bb(struct pci_dev *pdev, enum xe_engine_class,
> > > +						      const u32 **cs) { return 0; }
> > >  #endif
> > > 
> > >  #endif
> > > diff --git a/drivers/gpu/drm/xe/xe_lrc.c b/drivers/gpu/drm/xe/xe_lrc.c
> > > index 6d52e0eb97f54..f337954066f55 100644
> > > --- a/drivers/gpu/drm/xe/xe_lrc.c
> > > +++ b/drivers/gpu/drm/xe/xe_lrc.c
> > > @@ -16,6 +16,7 @@
> > >  #include "regs/xe_lrc_layout.h"
> > >  #include "xe_bb.h"
> > >  #include "xe_bo.h"
> > > +#include "xe_configfs.h"
> > >  #include "xe_device.h"
> > >  #include "xe_drm_client.h"
> > >  #include "xe_exec_queue_types.h"
> > > @@ -1102,6 +1103,29 @@ static ssize_t setup_timestamp_wa(struct xe_lrc *lrc, struct xe_hw_engine *hwe,
> > >  	return cmd - batch;
> > >  }
> > > 
> > > +static ssize_t setup_configfs_post_ctx_restore_bb(struct xe_lrc *lrc,
> > > +						  struct xe_hw_engine *hwe,
> > > +						  u32 *batch, size_t max_len)
> > > +{
> > > +	struct xe_device *xe = gt_to_xe(lrc->gt);
> > > +	const u32 *user_batch;
> > > +	u32 *cmd = batch;
> > > +	u32 count;
> > > +
> > > +	count = xe_configfs_get_ctx_restore_post_bb(to_pci_dev(xe->drm.dev),
> > > +						    hwe->class, &user_batch);
> > > +	if (!count)
> > > +		return 0;
> > > +
> > > +	if (count > max_len)
> > > +		return -ENOSPC;
> > > +
> > > +	memcpy(cmd, user_batch, count * sizeof(u32));
> > 
> > shouldn't we use copy_from_user here?
> 
> no. When the user wrote to the file, configfs did the copy and
> called us in xe_configfs.c:
> 
> 	configfs_write_iter()
> 		copy_from_iter()
> 			...
> 				raw_copy_from_user()
> 		flush_write_buffer()
> 			buffer->attr->store()
> 				ctx_restore_post_bb_store()
> 
> ctx_restore_post_bb_store() will parse the lines, and save it in the
> correct format to this instance of xe_config_device.
> 
> What xe_configfs_get_ctx_restore_post_bb() does is basically return
> that already-parsed-and-saved data.
> 
> So... here we are not copying anything from user. We are copying the
> previously saved buffer that is already in the kernel.

so, it does look right

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> 
> Lucas De Marchi
> 
> > 
> > > +	cmd += count;
> > > +
> > > +	return cmd - batch;
> > > +}
> > > +
> > >  static ssize_t setup_invalidate_state_cache_wa(struct xe_lrc *lrc,
> > >  					       struct xe_hw_engine *hwe,
> > >  					       u32 *batch, size_t max_len)
> > > @@ -1203,6 +1227,7 @@ int xe_lrc_setup_wa_bb_with_scratch(struct xe_lrc *lrc, struct xe_hw_engine *hwe
> > >  		{ .setup = setup_timestamp_wa },
> > >  		{ .setup = setup_invalidate_state_cache_wa },
> > >  		{ .setup = setup_utilization_wa },
> > > +		{ .setup = setup_configfs_post_ctx_restore_bb },
> > >  	};
> > >  	struct bo_setup_state state = {
> > >  		.lrc = lrc,
> > > 
> > > --
> > > 2.50.1
> > >