Re: [PATCH] drm/xe/xe2: Enable Priority Mem Read

[Matt Roper <matthew.d.roper@intel.com>]

On Sat, May 25, 2024 at 06:19:33AM +0530, Pallavi Mishra wrote:
> Enable feature to allow memory reads to take a
> priority memory path. This will reduce latency
> on the read path, but may introduce RAW hazards.

You might want to elaborate on what "may introduce RAW hazards" means.
I.e., it's worth noting that both kernel *and* userspace-submitted batch
buffers may now be subject to read-after-write hazards if they don't use
MI_MEM_FENCE in cases where there are assumptions about operations
happening in-order, but where a read could potentially leapfrog a
preceding write now that we've enabled this.

Presumably since there aren't any other changes in this patch you've
already confirmed that there's nowhere in the KMD itself that needs
extra fences added to avoid read-after-write hazards...you might want to
mention that in the commit message too.

This could expose previously-harmless bugs in userspace drivers and/or
IGT, so it will be important to make sure we land this before lifting
force_probe on these platforms, otherwise the appearance of those bugs
would be seen as a KMD regression, even though the bugs themselves live
in userspace.

> Bspec: 45845, 60237, 60187, 60188

45845 is a page for pre-Xe2 platforms, so I don't think that's the one
you want here.  I think you want 60298 for the Xe2 equivalent page that
documents the register bit.

> 
> Signed-off-by: Pallavi Mishra <pallavi.mishra@intel.com>
> ---
>  drivers/gpu/drm/xe/regs/xe_engine_regs.h | 1 +
>  drivers/gpu/drm/xe/xe_hw_engine.c        | 7 +++++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/regs/xe_engine_regs.h b/drivers/gpu/drm/xe/regs/xe_engine_regs.h
> index 263ffc7bc2ef..4e8f9a61f0bf 100644
> --- a/drivers/gpu/drm/xe/regs/xe_engine_regs.h
> +++ b/drivers/gpu/drm/xe/regs/xe_engine_regs.h
> @@ -104,6 +104,7 @@
>  #define CSFE_CHICKEN1(base)			XE_REG((base) + 0xd4, XE_REG_OPTION_MASKED)
>  #define   GHWSP_CSB_REPORT_DIS			REG_BIT(15)
>  #define   PPHWSP_CSB_AND_TIMESTAMP_REPORT_DIS	REG_BIT(14)
> +#define   CS_PRIORITY_MEM_READ			REG_BIT(7)
>  
>  #define FF_SLICE_CS_CHICKEN1(base)		XE_REG((base) + 0xe0, XE_REG_OPTION_MASKED)
>  #define   FFSC_PERCTX_PREEMPT_CTRL		REG_BIT(14)
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> index de1aefaa2335..34aa8b5270b0 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> @@ -424,6 +424,13 @@ hw_engine_setup_default_state(struct xe_hw_engine *hwe)
>  					   0xA,
>  					   XE_RTP_ACTION_FLAG(ENGINE_BASE)))
>  		},
> +		/* Enable Priority Mem Read */
> +		{ XE_RTP_NAME("Priority_Mem_Read"),
> +		  XE_RTP_RULES(GRAPHICS_VERSION_RANGE(2000, 2004)),

The current platforms that currently support this are BMG (graphics
version 20.01) and LNL (20.04), so the starting point of this range
should be 2001 rather than 2000.  Also, since this is a new feature
rather than a workaround, the expectation is that it will (probably)
continue to exist on future platforms as well, so it's best to just use
XE_RTP_END_VERSION_UNDEFINED as the end point so that we don't need to
keep coming back here and updating this for every new platform in the
future.

Also, RTP rules using GRAPHICS_VERSION[_RANGE] only get applied on the
primary GT.  If this feature is also available on the media engines (I
think it is since I don't see anything in the bspec implying it wouldn't
be supported), then we'll also want a second table entry for those
(i.e., pretty much a copy of what you have here, but using a
MEDIA_VERSION_RANGE(1301, XE_RTP_END_VERSION_UNDEFINED) to capture the
media IP.


Matt

> +		  XE_RTP_ACTIONS(SET(CSFE_CHICKEN1(0),
> +				     CS_PRIORITY_MEM_READ)),
> +		  XE_RTP_ACTION_FLAG(ENGINE_BASE),
> +		},
>  		{}
>  	};
>  
> -- 
> 2.25.1
> 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation