[Intel-gfx] [PATCH v4 1/6] drm/i915/pxp: Make gt and pxp init/fini aware of PXP-owning-GT
Teres Alexis, Alan Previn
alan.previn.teres.alexis at intel.com
Thu Nov 17 22:34:50 UTC 2022
On Thu, 2022-11-17 at 11:02 -0500, Vivi, Rodrigo wrote:
> On Wed, Nov 16, 2022 at 04:30:13PM -0800, Alan Previn wrote:
> > In preparation for future MTL-PXP feature support, PXP control
> > context should only valid on the correct gt tile. Depending on the
> > device-info this depends on which tile owns the VEBOX and KCR.
> > PXP is still a global feature though (despite its control-context
> > located in the owning GT structure). Additionally, we find
> > that the HAS_PXP macro is only used within the pxp module,
> >
> > That said, lets drop that HAS_PXP macro altogether and replace it
> > with a more fitting named intel_gtpxp_is_supported and helpers
> > so that PXP init/fini can use to verify if the referenced gt supports
> > PXP or teelink.
>
> Yep, I understand you as I'm not fan of these macros, specially
> single usage. But we need to consider that we have multiple dependencies
> there and other cases like this in the driver... Well, but I'm not
> opposing, but probably better to first get rid of the macro,
> then change the behavior of the function on the next patch.
>
> >
> > Add TODO for Meteorlake that will come in future series.
>
> This refactor patch should be standalone, without poputing it with
> the changes that didn't came yet to this point.
>
Sure i can follow this rule, but it would then raise the question of "nothign is really changing anywhere for MTL, why
are we doing this" thats why i wanted to add that placeholder. I see "TODO"s are a common thing in the driver for larger
features that cant all be enabled at once. Respectfully and humbly, is there some documented rule? Can you show it to
me?
> >
> > Signed-off-by: Alan Previn <alan.previn.teres.alexis at intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 4 ----
> > drivers/gpu/drm/i915/pxp/intel_pxp.c | 22 ++++++++++++++------
> > drivers/gpu/drm/i915/pxp/intel_pxp.h | 3 +++
> > drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c | 2 +-
> > 4 files changed, 20 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 7e3820d2c404..0616e5f0bd31 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -933,10 +933,6 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
> >
> > #define HAS_GLOBAL_MOCS_REGISTERS(dev_priv) (INTEL_INFO(dev_priv)->has_global_mocs)
> >
> > -#define HAS_PXP(dev_priv) ((IS_ENABLED(CONFIG_DRM_I915_PXP) && \
> > - INTEL_INFO(dev_priv)->has_pxp) && \
> > - VDBOX_MASK(to_gt(dev_priv)))
> > -
> > #define HAS_GMCH(dev_priv) (INTEL_INFO(dev_priv)->display.has_gmch)
> >
> > #define HAS_GMD_ID(i915) (INTEL_INFO(i915)->has_gmd_id)
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> > index 5efe61f67546..d993e752bd36 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> > @@ -44,6 +44,20 @@ struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp)
> > return container_of(pxp, struct intel_gt, pxp);
> > }
> >
> > +static bool _gt_needs_teelink(struct intel_gt *gt)
> > +{
> > + /* TODO: MTL won't rely on CONFIG_INTEL_MEI_PXP but on GSC engine */
> > + return (IS_ENABLED(CONFIG_INTEL_MEI_PXP) && intel_huc_is_loaded_by_gsc(>->uc.huc) &&
> > + intel_uc_uses_huc(>->uc));
> > +}
> > +
> > +bool intel_pxp_supported_on_gt(const struct intel_pxp *pxp)
>
> If we are asking if it is supported on gt, then the argument must be a gt struct.
>
I agree with you but Daniele said above is more consistent with existing ways that is considered the standard.
Respectfully and humbly I would like to request both yourself and Daniele to show me the coding guidelines somewhere.
Honestly, this is one of the first few hunks of the first patch of the first series in a very large complex design to
enable PXP on MTL and it only a helper utility function. Respecfully and humbly, I rather we focus our energy for review
+ redo on more critical things like the e2e usage and top-to-bottom design or coding logic flows or find actual bugs
instead of debating about coding styles for internal only helper functions.
> > +{
> > + /* TODO: MTL won't rely on CONFIG_INTEL_MEI_PXP but on GSC engine */
> > + return (IS_ENABLED(CONFIG_INTEL_MEI_PXP) && IS_ENABLED(CONFIG_DRM_I915_PXP) &&
> > + INTEL_INFO((pxp_to_gt(pxp))->i915)->has_pxp && VDBOX_MASK(pxp_to_gt(pxp)));
> > +}
> > +
> > bool intel_pxp_is_enabled(const struct intel_pxp *pxp)
> > {
> > return pxp->ce;
> > @@ -142,17 +156,13 @@ void intel_pxp_init(struct intel_pxp *pxp)
> > {
> > struct intel_gt *gt = pxp_to_gt(pxp);
> >
> > - /* we rely on the mei PXP module */
> > - if (!IS_ENABLED(CONFIG_INTEL_MEI_PXP))
> > - return;
>
> I took a time to understand this movement based on the commit description.
> I have the feeling that this patch deserves further split in different patches.
>
> But also, looking a few lines above pxp_to_gt(pxp), I believe we
> have further refactor to do sooner. is is one pxp per gt, then we
> need to only enable in the gt0?
>
In our driver, PXP as reflected by the device info is being treated as a global feature.
PXP as a HW subsystem is "usable" device-wide from any workload on any engine on any tile (due to the internal mirror
component and additional plumbing across the tiles). So in line with that I rather not have the gem-exec-buf, gem-create
and gem-context calls be bothered about which GT to access to query of this global hw feature is enabled or active.
However the control point for allocating sessions, talking to the gsc firmware and doing global teardowns are only meant
to occur on and via the tile that owns the KCR engine which the media tile. This includes things like per-tile uncore
power gating controls of the GSC-CS. (although some aspects like IRQ for KCR global). So as u see its not a clean per-GT
feature.
I did speak to Daniele many months back when enabling the full feature set (on internal POC work) about whether we
should make PXP a global subsystem instead of hanging off gt but we both agreed that because the control engines are
only located on one tile, so you might face some its gonna be a trade off one way or the other:
- pxp a global structure, then all of the init / shutdown / suspend-resume flows would then have a different set of
convoluted functions that try to get access to gt specific controls from a top level function flow.
Additionally, humbly and respectfully, perhaps you can read through the internal arch HW specs through which it can be
infered that PXP will continue to have a single entity for control events despite the feature being usable / accessible
across all tiles.
> > -
> > /*
> > * If HuC is loaded by GSC but PXP is disabled, we can skip the init of
> > * the full PXP session/object management and just init the tee channel.
> > */
> > - if (HAS_PXP(gt->i915))
> > + if (intel_pxp_supported_on_gt(pxp))
> > pxp_init_full(pxp);
> > - else if (intel_huc_is_loaded_by_gsc(>->uc.huc) && intel_uc_uses_huc(>->uc))
> > + else if (_gt_needs_teelink(gt))
> > intel_pxp_tee_component_init(pxp);
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> > index 2da309088c6d..efa83f9d5e24 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> > @@ -13,6 +13,9 @@ struct intel_pxp;
> > struct drm_i915_gem_object;
> >
> > struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp);
> > +
> > +bool intel_pxp_supported_on_gt(const struct intel_pxp *pxp);
> > +
> > bool intel_pxp_is_enabled(const struct intel_pxp *pxp);
> > bool intel_pxp_is_active(const struct intel_pxp *pxp);
> >
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> > index 4359e8be4101..f0ad6f34624a 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> > @@ -70,7 +70,7 @@ void intel_pxp_debugfs_register(struct intel_pxp *pxp, struct dentry *gt_root)
> > if (!gt_root)
> > return;
> >
> > - if (!HAS_PXP((pxp_to_gt(pxp)->i915)))
> > + if (!intel_pxp_supported_on_gt(pxp))
> > return;
> >
> > root = debugfs_create_dir("pxp", gt_root);
> > --
> > 2.34.1
> >
More information about the Intel-gfx
mailing list