Re: [PATCH 08/43] drm/xe: Adjust mmio code to pass VF substructure to SRIOV code

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

On Fri, Sep 06, 2024 at 05:28:37PM +0200, Michal Wajdeczko wrote:
> 
> 
> On 04.09.2024 02:21, Matt Roper wrote:
> > Although we want to break the GT-centric nature of the MMIO code in the
> > general driver, the SRIOV handling still relies on data in a VF
> > substructure of the GT.  Pass this substructure, rather than the GT
> 
> hmm, I would prefer to keep GT as VF function param, this substructure
> is not used as explicit param in any other function by design
> 
> > itself, to the SRIOV code called from the MMIO operations.  Most of what
> > the SRIOV code needs is within this substructure, and container_of() can
> > be used to get back to the GT itself if necessary.
> > Also store a backpointer to this structure within the xe_mmio in
> > preparation for removal of xe_gt from the MMIO interface.
> 
> maybe in addition to backpointer to the xe_tile we can also keep more
> generic backpointer to the xe_gt?

My main worry was that if I added the GT itself, people might start
trying to use it for non-SRIOV things which defeats part of the purpose
here.  I guess I could name it something like 'sriov_vf_gt' to make it
clear that it can't be used (and likely isn't assigned) in other
settings.

> 
> but then the question would be whether the xe_mmio unification at GT
> level brings any value ...
> 
> maybe instead we should consider adding GT oriented MMIO functions that
> will do necessary address adjustments and then use the pure xe_mmio from
> the tile:

We sort of took this approach in i915 for a while (where we had
GT-specific read/write functions and they did some special stuff for MCR
registers implicitly), but I think the feedback at the time was that
most people preferred a unified MMIO interface and only wanted a
domain-specific MMIO interface in cases where the higher-level
operations (MCR in that case) were explicitly wanted/needed by the
caller.  So i915 went back to the (somewhat misnamed) "uncore" interface
for general GT register reads and only used a domain-specific MMIO
interface for the MCR cases where there were more options that needed to
be decided at the callsite.


Matt

> 
> u32 xe_gt_mmio_read32(gt, reg)
> {
> 	struct xe_reg adj = reg;
> 
> 	// maybe we should have XE_REG_OPTION_GT ?
> 	adj.addr += gt->mmio.adj_offset;
> 
> 	// we can place SRIOV VF hook here
> 	return xe_tile_mmio_read32(&gt_to_tile(gt), adj);
> }
> 
> and for the whole series introduce (or even keep):
> 
> #define xe_mmio_read32(p, reg) 					\
> 	_Generic(p,						\
> 		struct xe_mmio *: __xe_mmio_read32,		\
> 		struct xe_tile *: xe_tile mmio_read32,		\
> 		struct xe_gt *: xe_gt_mmio_read32)(p, reg)
> 
> > 
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_device_types.h | 3 +++
> >  drivers/gpu/drm/xe/xe_gt_sriov_vf.c  | 6 ++++--
> >  drivers/gpu/drm/xe/xe_gt_sriov_vf.h  | 5 +++--
> >  drivers/gpu/drm/xe/xe_mmio.c         | 4 ++--
> >  drivers/gpu/drm/xe/xe_pci.c          | 5 +++++
> >  5 files changed, 17 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> > index 19c21e55e153..15a371f81c60 100644
> > --- a/drivers/gpu/drm/xe/xe_device_types.h
> > +++ b/drivers/gpu/drm/xe/xe_device_types.h
> > @@ -121,6 +121,9 @@ struct xe_mmio {
> >  	/** @regs: Map used to access registers. */
> >  	void __iomem *regs;
> >  
> > +	/** @sriov_vf: For GT regions, backpointer to SRIOV VF structure */
> > +	struct xe_gt_sriov_vf *sriov_vf;
> > +
> >  	/**
> >  	 * @map_size: Length of the register region within the map.
> >  	 *
> > diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_vf.c b/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
> > index 4ebc82e607af..2c61882ea411 100644
> > --- a/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
> > +++ b/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
> > @@ -879,8 +879,9 @@ static struct vf_runtime_reg *vf_lookup_reg(struct xe_gt *gt, u32 addr)
> >   *
> >   * Return: register value obtained from the PF or 0 if not found.
> >   */
> > -u32 xe_gt_sriov_vf_read32(struct xe_gt *gt, struct xe_reg reg)
> > +u32 xe_gt_sriov_vf_read32(struct xe_gt_sriov_vf *vf, struct xe_reg reg)
> >  {
> > +	struct xe_gt *gt = container_of(vf, struct xe_gt, sriov.vf);
> >  	u32 addr = xe_mmio_adjusted_addr(gt, reg.addr);
> >  	struct vf_runtime_reg *rr;
> >  
> > @@ -915,8 +916,9 @@ u32 xe_gt_sriov_vf_read32(struct xe_gt *gt, struct xe_reg reg)
> >   * This function is for VF use only.
> >   * Currently it will trigger a WARN if running on debug build.
> >   */
> > -void xe_gt_sriov_vf_write32(struct xe_gt *gt, struct xe_reg reg, u32 val)
> > +void xe_gt_sriov_vf_write32(struct xe_gt_sriov_vf *vf, struct xe_reg reg, u32 val)
> >  {
> > +	struct xe_gt *gt = container_of(vf, struct xe_gt, sriov.vf);
> >  	u32 addr = xe_mmio_adjusted_addr(gt, reg.addr);
> >  
> >  	xe_gt_assert(gt, IS_SRIOV_VF(gt_to_xe(gt)));
> > diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_vf.h b/drivers/gpu/drm/xe/xe_gt_sriov_vf.h
> > index e541ce57bec2..0437c937f5b0 100644
> > --- a/drivers/gpu/drm/xe/xe_gt_sriov_vf.h
> > +++ b/drivers/gpu/drm/xe/xe_gt_sriov_vf.h
> > @@ -10,6 +10,7 @@
> >  
> >  struct drm_printer;
> >  struct xe_gt;
> > +struct xe_gt_sriov_vf;
> >  struct xe_reg;
> >  
> >  int xe_gt_sriov_vf_bootstrap(struct xe_gt *gt);
> > @@ -21,8 +22,8 @@ int xe_gt_sriov_vf_prepare_ggtt(struct xe_gt *gt);
> >  u32 xe_gt_sriov_vf_gmdid(struct xe_gt *gt);
> >  u16 xe_gt_sriov_vf_guc_ids(struct xe_gt *gt);
> >  u64 xe_gt_sriov_vf_lmem(struct xe_gt *gt);
> > -u32 xe_gt_sriov_vf_read32(struct xe_gt *gt, struct xe_reg reg);
> > -void xe_gt_sriov_vf_write32(struct xe_gt *gt, struct xe_reg reg, u32 val);
> > +u32 xe_gt_sriov_vf_read32(struct xe_gt_sriov_vf *vf, struct xe_reg reg);
> > +void xe_gt_sriov_vf_write32(struct xe_gt_sriov_vf *vf, struct xe_reg reg, u32 val);
> >  
> >  void xe_gt_sriov_vf_print_config(struct xe_gt *gt, struct drm_printer *p);
> >  void xe_gt_sriov_vf_print_runtime(struct xe_gt *gt, struct drm_printer *p);
> > diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
> > index dd2076b9003e..8068663e7d28 100644
> > --- a/drivers/gpu/drm/xe/xe_mmio.c
> > +++ b/drivers/gpu/drm/xe/xe_mmio.c
> > @@ -239,7 +239,7 @@ void xe_mmio_write32(struct xe_gt *gt, struct xe_reg reg, u32 val)
> >  	trace_xe_reg_rw(gt, true, addr, val, sizeof(val));
> >  
> >  	if (!reg.vf && IS_SRIOV_VF(gt_to_xe(gt)))
> > -		xe_gt_sriov_vf_write32(gt, reg, val);
> > +		xe_gt_sriov_vf_write32(gt->mmio.sriov_vf, reg, val);
> >  	else
> >  		writel(val, (reg.ext ? tile->mmio_ext.regs : tile->mmio.regs) + addr);
> >  }
> > @@ -254,7 +254,7 @@ u32 xe_mmio_read32(struct xe_gt *gt, struct xe_reg reg)
> >  	mmio_flush_pending_writes(gt);
> >  
> >  	if (!reg.vf && IS_SRIOV_VF(gt_to_xe(gt)))
> > -		val = xe_gt_sriov_vf_read32(gt, reg);
> > +		val = xe_gt_sriov_vf_read32(gt->mmio.sriov_vf, reg);
> >  	else
> >  		val = readl((reg.ext ? tile->mmio_ext.regs : tile->mmio.regs) + addr);
> >  
> > diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> > index e222539022d5..04d34fd015ce 100644
> > --- a/drivers/gpu/drm/xe/xe_pci.c
> > +++ b/drivers/gpu/drm/xe/xe_pci.c
> > @@ -715,6 +715,9 @@ static int xe_info_init(struct xe_device *xe,
> >  		gt->mmio.regs = tile->mmio.regs;
> >  		gt->mmio.regs_length = tile->mmio.regs_length;
> >  		gt->mmio.xe = xe;
> > +		if (IS_SRIOV_VF(xe))
> > +			gt->mmio.sriov_vf = &gt->sriov.vf;
> > +
> >  		if (MEDIA_VER(xe) < 13 && media_desc)
> >  			gt->info.engine_mask |= media_desc->hw_engine_mask;
> >  
> > @@ -738,6 +741,8 @@ static int xe_info_init(struct xe_device *xe,
> >  		gt->mmio.adj_offset = MEDIA_GT_GSI_OFFSET;
> >  		gt->mmio.adj_limit = MEDIA_GT_GSI_LENGTH;
> >  		gt->mmio.xe = xe;
> > +		if (IS_SRIOV_VF(xe))
> > +			gt->mmio.sriov_vf = &gt->sriov.vf;
> >  
> >  		/*
> >  		 * FIXME: At the moment multi-tile and standalone media are

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