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

["Souza, Jose" <jose.souza@intel.com>]

On Fri, 2024-05-31 at 14:09 -0700, Matt Roper wrote:
> On Thu, May 30, 2024 at 10:00:36AM -0700, Souza, Jose wrote:
> > On Thu, 2024-05-30 at 11:55 +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 read after
> > > write (RAW) hazards as read and writes will no
> > > longer be ordered.
> > > 
> > > To avoid RAW hazards, SW can use the MI_MEM_FENCE
> > > command or any other MI command that generates
> > > non posted memory writes. This will ensure data
> > > is coherent in memory prior to execution of
> > > commands which read data from memory.
> > 
> > This has the potential to cause issues in UMDs, Mesa for example don't have any MI_MEM_FENCE instruction in the code base.
> > 
> > So please before merging this sync with all UMDs or merge it behind a run-time or build time parameter with it disabled by default.
> 
> Wouldn't it be better to make this the default ASAP so that we can start
> uncovering any hidden userspace bugs right away?  Once force_probe gets
> lifted and the platform becomes generally usable, it will be too late to
> fix since at that point any failures will need to be treated as kernel
> regressions, even if the mistake itself lies in userspace.  I don't see
> much benefit in artificially hiding problems when we don't have much
> time left to discover and fix them.

Changes like should not be merged this late on the program at least without acks from all UMDs.
Mesa for example is late on LNL upstreaming this additional feature could cause even more delays.


> 
> 
> Matt
> 
> > 
> > > 
> > > No pattern identified in KMD that could lead to a
> > > hazard.
> > > 
> > > v2: Modify commit message, enable priority mem
> > > read feature for media, modify version range,
> > > modify bspec detail (Matt Roper)
> > > 
> > > Bspec: 60298, 60237, 60187, 60188
> > > 
> > > 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        | 11 +++++++++++
> > >  2 files changed, 12 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 9eef789be897..f03e5cc9063e 100644
> > > --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> > > +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> > > @@ -424,6 +424,17 @@ 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(2001, XE_RTP_END_VERSION_UNDEFINED)),
> > > +		  XE_RTP_ACTIONS(SET(CSFE_CHICKEN1(0), CS_PRIORITY_MEM_READ,
> > > +				     XE_RTP_ACTION_FLAG(ENGINE_BASE)))
> > > +		},
> > > +		{ XE_RTP_NAME("Priority_Mem_Read_For_Media"),
> > > +		  XE_RTP_RULES(MEDIA_VERSION_RANGE(1301, XE_RTP_END_VERSION_UNDEFINED)),
> > > +		  XE_RTP_ACTIONS(SET(CSFE_CHICKEN1(0), CS_PRIORITY_MEM_READ,
> > > +				     XE_RTP_ACTION_FLAG(ENGINE_BASE)))
> > > +		},
> > >  		{}
> > >  	};
> > >  
> > 
>