[PATCH] drm/xe/uapi: Add DRM_XE_QUERY_CONFIG_FLAG_HAS_DIS_NULL_QUERY

Wed Jun 25 13:35:33 UTC 2025

On Wed, Jun 25, 2025 at 03:23:29AM -0600, Gote, Nitin R wrote:
> Hi Matthew,
> > 
> > On Mon, Jun 23, 2025 at 12:10:36PM +0530, Nitin Gote wrote:
> > > Add the DRM_XE_QUERY_CONFIG_FLAG_HAS_DIS_NULL_QUERY device query
> > flag
> > > which indicates whether a device supports DIS_NULL_QUERY (Disable null
> > 
> > s/DIS_NULL_QUERY/DISABLE_NULL_QUERY
> 
> The DIS_NULL_QUERY naming convention is followed as per Bspec 57496.
> 

The bspec has nothing to do with uAPI naming. uAPI should be as clear as
possible. Leave the register as DIS_NULL_QUERY but change the uAPI.

> > 
> > > anyhit shader query mechanism). The intent is for UMDs to use this
> > > query and opt-in DRM_XE_EXEC_QUEUE_DIS_NULL_QUERY flag to disable null
> > > query mechanism for anyhit shader by setting DIS_NULL_QUERY bit of
> > > RT_CTRL register for Xe2 IP.
> > >
> > > Signed-off-by: Nitin Gote <nitin.r.gote at intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_exec_queue.c | 9 +++++++++
> > >  drivers/gpu/drm/xe/xe_query.c      | 3 ++-
> > >  include/uapi/drm/xe_drm.h          | 8 ++++++++
> > >  3 files changed, 19 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c
> > > b/drivers/gpu/drm/xe/xe_exec_queue.c
> > > index fee22358cc09..519f36db7cd0 100644
> > > --- a/drivers/gpu/drm/xe/xe_exec_queue.c
> > > +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
> > > @@ -26,6 +26,8 @@
> > >  #include "xe_trace.h"
> > >  #include "xe_vm.h"
> > >  #include "xe_pxp.h"
> > > +#include "xe_gt_mcr.h"
> > > +#include "regs/xe_gt_regs.h"
> > >
> > >  enum xe_exec_queue_sched_prop {
> > >  	XE_EXEC_QUEUE_JOB_TIMEOUT = 0,
> > > @@ -693,6 +695,13 @@ int xe_exec_queue_create_ioctl(struct drm_device
> > *dev, void *data,
> > >  		}
> > >  	}
> > >
> > > +	if (((GRAPHICS_VER(xe) >= 20) && (GRAPHICS_VER(xe) < 30)) &&
> > > +	    (args->flags & DRM_XE_EXEC_QUEUE_DIS_NULL_QUERY) &&
> > > +	    (eci[0].engine_class == DRM_XE_ENGINE_CLASS_RENDER ||
> > > +	    eci[0].engine_class == DRM_XE_ENGINE_CLASS_COMPUTE)) {
> > > +		xe_gt_mcr_multicast_write(q->gt, RT_CTRL, DIS_NULL_QUERY);
> > > +	}
> > 
> > You will hit an error before you get here because of the if statement above (not
> > shown in the diff) that sanitizes args->flags.
> > 
> 
> Yes. Agree with you. I will add it.
> 
> > You don’t need {} here.
> > 
> Thanks, I will remove it.
> 
> > Also, a register write implies this would affect the entire GT, not just the exec
> > queue flagged with “disable NULL query.” Is that really the intent? It doesn’t
> > seem correct to me, but honestly, I’m not entirely sure what this patch is trying to
> > do. If the intent is for this to be per-exec queue, then I think this needs to be
> > implemented as some ring instructions in the LRC WA BB.
> > 
> 
> Here, the intention is that for the already released platforms Xe2, 
> we need to add an opt-in flag via uapi to disable null query for anyhit shader.
> So that userspace can use this flag to disable anyhit shader query mechanism, and I think
> that it is not possible in the LRC WA BB. And to me, this placement looks ok as this setting is 
> effective post xe_exec call.
> 

Way can't you write a WA BB program that sets this?
xe_gt_mcr_multicast_write function seemly could be implemented in a WA
BB - the mcr_lock function indicates these registers are shared with
firmware, no different than WA BB.

The way you have it written it is a global (e.g., all user processes)
change which once is set is never cleared. You might as well just this
write at driver load and be done with it the way you it written.

Field a 2nd opinion here if you like, but this looks very incorrect to
me unless I'm completely missing something.

Matt

> > > +
> > >  	q->xef = xe_file_get(xef);
> > >
> > >  	/* user id alloc must always be last in ioctl to prevent UAF */ diff
> > > --git a/drivers/gpu/drm/xe/xe_query.c b/drivers/gpu/drm/xe/xe_query.c
> > > index e8e1743dcb1e..c5002b8c9ac0 100644
> > > --- a/drivers/gpu/drm/xe/xe_query.c
> > > +++ b/drivers/gpu/drm/xe/xe_query.c
> > > @@ -344,7 +344,8 @@ static int query_config(struct xe_device *xe, struct
> > drm_xe_device_query *query)
> > >  		config->info[DRM_XE_QUERY_CONFIG_FLAGS] |=
> > >
> > 	DRM_XE_QUERY_CONFIG_FLAG_HAS_CPU_ADDR_MIRROR;
> > >  	config->info[DRM_XE_QUERY_CONFIG_FLAGS] |=
> > > -			DRM_XE_QUERY_CONFIG_FLAG_HAS_LOW_LATENCY;
> > > +			(DRM_XE_QUERY_CONFIG_FLAG_HAS_LOW_LATENCY
> > |
> > > +
> > 	DRM_XE_QUERY_CONFIG_FLAG_HAS_DIS_NULL_QUERY);
> > >  	config->info[DRM_XE_QUERY_CONFIG_MIN_ALIGNMENT] =
> > >  		xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K ? SZ_64K :
> > SZ_4K;
> > >  	config->info[DRM_XE_QUERY_CONFIG_VA_BITS] = xe->info.va_bits; diff
> > > --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h index
> > > 8e8bbdec8c5c..a5bfba121360 100644
> > > --- a/include/uapi/drm/xe_drm.h
> > > +++ b/include/uapi/drm/xe_drm.h
> > > @@ -397,6 +397,8 @@ struct drm_xe_query_mem_regions {
> > >   *      has low latency hint support
> > >   *    - %DRM_XE_QUERY_CONFIG_FLAG_HAS_CPU_ADDR_MIRROR - Flag is
> > set if the
> > >   *      device has CPU address mirroring support
> > > + *    - %DRM_XE_QUERY_CONFIG_FLAG_HAS_DIS_NULL_QUERY - Flag is set if
> > the
> > > + *      device has null query support for anyhit shader.
> > >   *  - %DRM_XE_QUERY_CONFIG_MIN_ALIGNMENT - Minimal memory
> > alignment
> > >   *    required by this device, typically SZ_4K or SZ_64K
> > >   *  - %DRM_XE_QUERY_CONFIG_VA_BITS - Maximum bits of a virtual
> > > address @@ -415,6 +417,7 @@ struct drm_xe_query_config {
> > >  	#define DRM_XE_QUERY_CONFIG_FLAG_HAS_VRAM	(1 << 0)
> > >  	#define DRM_XE_QUERY_CONFIG_FLAG_HAS_LOW_LATENCY	(1 <<
> > 1)
> > >  	#define DRM_XE_QUERY_CONFIG_FLAG_HAS_CPU_ADDR_MIRROR
> > 	(1 << 2)
> > > +	#define DRM_XE_QUERY_CONFIG_FLAG_HAS_DIS_NULL_QUERY	(1 <<
> > 3)
> > >  #define DRM_XE_QUERY_CONFIG_MIN_ALIGNMENT		2
> > >  #define DRM_XE_QUERY_CONFIG_VA_BITS			3
> > >  #define DRM_XE_QUERY_CONFIG_MAX_EXEC_QUEUE_PRIORITY	4
> > > @@ -1271,6 +1274,11 @@ struct drm_xe_exec_queue_create {
> > >  	__u32 vm_id;
> > >
> > >  #define DRM_XE_EXEC_QUEUE_LOW_LATENCY_HINT	(1 << 0)
> > > +	/*
> > > +	 * DRM_XE_EXEC_QUEUE_DIS_NULL_QUERY flag is
> > > +	 * use to disable null query check for Anyhit shader
> > > +	 */
> > > +#define DRM_XE_EXEC_QUEUE_DIS_NULL_QUERY	(1 << 1)
> > 
> > We should add proper kernel-doc for this flag.
> > 
> > It looks like we’re also missing proper kernel-doc for
> > DRM_XE_EXEC_QUEUE_LOW_LATENCY_HINT. While you’re at it, could you add
> > that as well?
> > 
> Sure. I will add kernel-doc for both macros.
> 
> > Matt
> > 
> > >  	/** @flags: flags to use for this exec queue */
> > >  	__u32 flags;
> > >
> > > --
> > > 2.25.1
> > >