[Intel-xe] [PATCH 2/2] drm/xe: Set default MOCS value for copy cs instructions

[Souza, Jose]

On Wed, 2023-04-26 at 23:39 -0400, Rodrigo Vivi wrote:
> On Tue, Apr 25, 2023 at 05:58:54PM +0000, Souza, Jose wrote:
> > On Mon, 2023-04-24 at 15:59 -0700, Matt Roper wrote:
> > > On Fri, Apr 14, 2023 at 09:09:06PM -0700, José Roberto de Souza wrote:
> > > > copy cs instructions that dont have a explict MOCS field will use this
> > > > default MOCS value.
> > > > 
> > > > This was mainly copied from i915 source code.
> > > > 
> > > > BSpec: 45807
> > > > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > > > ---
> > > >  drivers/gpu/drm/xe/regs/xe_engine_regs.h |  9 +++++++++
> > > >  drivers/gpu/drm/xe/xe_wa.c               | 13 +++++++++++++
> > > >  2 files changed, 22 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/xe/regs/xe_engine_regs.h b/drivers/gpu/drm/xe/regs/xe_engine_regs.h
> > > > index cc874348f76bc..dbc4b9f15b4e8 100644
> > > > --- a/drivers/gpu/drm/xe/regs/xe_engine_regs.h
> > > > +++ b/drivers/gpu/drm/xe/regs/xe_engine_regs.h
> > > > @@ -105,6 +105,15 @@
> > > >  		(REG_FIELD_PREP(CMD_CCTL_WRITE_OVERRIDE_MASK, (write) << 1) | \
> > > >  		 REG_FIELD_PREP(CMD_CCTL_READ_OVERRIDE_MASK, (read) << 1))
> > > >  
> > > > +#define BLIT_CCTL(base)				_MMIO((base) + 0x204)
> > > > +#define   BLIT_CCTL_DST_MOCS_MASK		REG_GENMASK(14, 8)
> > > > +#define   BLIT_CCTL_SRC_MOCS_MASK		REG_GENMASK(6, 0)
> > > > +#define   BLIT_CCTL_MASK (BLIT_CCTL_DST_MOCS_MASK | \
> > > > +			  BLIT_CCTL_SRC_MOCS_MASK)
> > > > +#define   BLIT_CCTL_MOCS(dst, src)				       \
> > > > +		(REG_FIELD_PREP(BLIT_CCTL_DST_MOCS_MASK, (dst) << 1) | \
> > > > +		 REG_FIELD_PREP(BLIT_CCTL_SRC_MOCS_MASK, (src) << 1))
> > > 
> > > Same general comments from last patch about file position, mask
> > > definition, and potentially dropping last two macros apply to this patch
> > > as well.
> > > 
> > > > +
> > > >  #define VDBOX_CGCTL3F10(base)			_MMIO((base) + 0x3f10)
> > > >  #define   IECPUNIT_CLKGATE_DIS			REG_BIT(22)
> > > >  
> > > > diff --git a/drivers/gpu/drm/xe/xe_wa.c b/drivers/gpu/drm/xe/xe_wa.c
> > > > index 5ef3561101466..0c12700bedde5 100644
> > > > --- a/drivers/gpu/drm/xe/xe_wa.c
> > > > +++ b/drivers/gpu/drm/xe/xe_wa.c
> > > > @@ -660,6 +660,19 @@ xe_wa_process_runtime_was_engine(struct xe_reg_sr *sr,
> > > >  					   CMD_CCTL_MOCS_OVERRIDE(mocs_w, mocs_r),
> > > >  					   XE_RTP_ACTION_FLAG(MASKED_REG, ENGINE_BASE))),
> > > >  		},
> > > > +		/*
> > > > +		 * Some blitter commands do not have a field for MOCS, those
> > > > +		 * commands will use MOCS index pointed by BLIT_CCTL.
> > > > +		 * BLIT_CCTL registers are needed to be programmed to un-cached.
> > > > +		 */
> > > > +		{ XE_RTP_NAME("BLIT_CCTL_default_MOCS"),
> > > > +		  XE_RTP_RULES(GRAPHICS_VERSION_RANGE(1200, XE_RTP_END_VERSION_UNDEFINED),
> > > > +			       ENGINE_CLASS(COPY)),
> > > > +		  XE_RTP_ACTIONS(FIELD_SET(BLIT_CCTL(0),
> > > > +				 BLIT_CCTL_MASK,
> > > > +				 BLIT_CCTL_MOCS(mocs_w, mocs_r),
> > > > +				 XE_RTP_ACTION_FLAG(MASKED_REG, ENGINE_BASE)))
> > > 
> > > Even though BLIT_CCTL looks a lot like CMD_CCTL on the surface, there
> > > are some important differences between the two that need to be noted
> > > here:
> > > 
> > >  - BLIT_CCTL is not a masked register.
> > >  - BLIT_CCTL is part of an engine context (e.g., bspec 45585), so it
> > >    would need to eventually wind up on an LRC list, not a HWE list.
> > > 
> > > The inconsistency in hardware design here is frustrating...
> > > 
> > > Also note that BLIT_CCTL is directly writable by userspace (bspec 45546)
> > > so I question whether it's even our responsibility to do this in Xe.
> > > It's too late to fix i915 now (at least for current platforms), but I'd
> > > suggest just dropping this second patch for Xe and leaving it to
> > > userspace to program whatever value they wish for their own context.
> 
> one might argue that the MOCS was created exactly to be an implicit
> API with kernel always responsible for programming. But I see your point...
> 
> > 
> > Cc: Rodrigo
> > 
> > Okay will send the first patch with comments addressed.
> > 
> > Then if decided to not program this in KMD it needs to be documented publicly.
> 
> Yeap, I fully agree.
> 
> > Together with all other small behavior changes from i915, MI_BATCH_BUFFER_START is other case where behavior changed.
> 
> What's the behavior difference in this case?
> 
> Could you please file a gitlab issue for this documentation of the differences
> and include the details you already know? So I will add this to the doc when
> I get some time to start organizing the docs.

Done: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/259

While we discuss patch 2, can someone please review patch 1 ?
Version with comments addressed here: https://patchwork.freedesktop.org/series/116943/

MattR is out for a few days.

> 
> Thanks,
> Rodrigo.
> 
> > 
> > This will save a lot of debug time in the other UMDs.
> > 
> > > 
> > > 
> > > Matt
> > > 
> > > > +		},
> > > >  		{}
> > > >  	};
> > > >  
> > > > -- 
> > > > 2.40.0
> > > > 
> > > 
> >