[PATCH 08/43] drm/xe: Adjust mmio code to pass VF substructure to SRIOV code
Matt Roper
matthew.d.roper at intel.com
Fri Sep 6 19:44:57 UTC 2024
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(>_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 at 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 = >->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 = >->sriov.vf;
> >
> > /*
> > * FIXME: At the moment multi-tile and standalone media are
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
More information about the Intel-xe
mailing list