Thank you Jani - this was clearly my mistake (apologies to Daniele/Rodrigo) - didn't realize we had this documented. I will go through that first. ...alan On Mon, 2022-11-21 at 14:12 +0200, Jani Nikula wrote: > On Mon, 21 Nov 2022, Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote: > > On 17/11/2022 22:34, Teres Alexis, Alan Previn wrote: > > > 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? > > > > Separating refactoring from functional changes is one of the most basic > > principles we follow (and not just us) and there should never be > > pushback on the former due lack of functional changes. > > It's also documented [1][2] but that never seems to make a difference. > > BR, > Jani. > > > [1] https://docs.kernel.org/process/submitting-patches.html#separate-your-changes > [2] https://docs.kernel.org/process/5.Posting.html#patch-preparation > > > > > > > > On the basic level following this guideline makes it trivial to review > > the refactoring patch, and in the vast majority of cases much easier to > > review the functional change patch as well. > > > > And easy to review means happy reviewers, faster turnaround time (easier > > to carry a rebase), happier authors, easier reverts when things go bad > > (only small functional patch needs to be reverted), sometimes even > > easier backporting if code diverged a lot. > > > > Oh, and it even has potential to decrease internal technical debt. Often > > you can push refactoring upstream and keep a smaller delta behind the > > closed doors, when that is required. > > > > > > > Signed-off-by: Alan Previn <alan.previn.teres.alexis@xxxxxxxxx> > > > > > --- > > > > > 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. > > > > My 2c/p is that the intel_pxp_supported_on_gt the "on_gt" part reads a > > bit clumsy because it makes it sound like the two are independent > > objects. Like I could pass one pxp to different GTs and ask if that one > > works here, or maybe there. > > > > I am though a fan of intel_pxp_ prefix meaning function operates on > > struct intel_pxp. > > > > In this case I don't know what is the correct relationship. If it is 1:1 > > between intel_pxp:intel_gt then intel_pxp_supported(pxp) sounds fine. > > Even if a singleton that works for me. If we do have 1:1 but only want > > to init the first one, but PXP truly lives in the GT block, we could > > have pointers per GT, with only the gt0 one being initialized, and > > perhaps a shadow i915->pxp pointer which points to gt[0]->pxp, if that > > makes for better code organisation? > > > > Regards, > > > > Tvrtko > > > > > > > > > > > > > +{ > > > > > + /* 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 > > > > > > > > > > -- > Jani Nikula, Intel Open Source Graphics Center
