| Message ID | 20221021173946.366210-2-alan.previn.teres.alexis@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/pxp: Prepare intel_pxp entry points for MTL | expand |
On 10/21/2022 10:39 AM, 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. > > Add TODO for Meteorlake that will come in future series. > > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 4 --- > drivers/gpu/drm/i915/pxp/intel_pxp.c | 35 ++++++++++++++++---- > drivers/gpu/drm/i915/pxp/intel_pxp.h | 2 ++ > drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c | 2 +- > 4 files changed, 32 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 7c64f8a17493..0921d1107825 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -923,10 +923,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..545c075bf1aa 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c > @@ -44,6 +44,30 @@ 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)); > +} > + > +static bool _gt_supports_pxp(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) && IS_ENABLED(CONFIG_DRM_I915_PXP) && > + INTEL_INFO((gt)->i915)->has_pxp && VDBOX_MASK(gt)); > +} > + > +bool intel_gtpxp_is_supported(struct intel_pxp *pxp) > +{ > + struct intel_gt *gt = pxp_to_gt(pxp); > + > + if (_gt_needs_teelink(gt) || _gt_supports_pxp(gt)) > + return true; > + > + return false; > +} > + > bool intel_pxp_is_enabled(const struct intel_pxp *pxp) > { > return pxp->ce; > @@ -142,22 +166,21 @@ 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; > - > /* > * 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 (_gt_supports_pxp(gt)) > 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); > } > > void intel_pxp_fini(struct intel_pxp *pxp) > { > + if (!intel_gtpxp_is_supported(pxp)) > + return; Why do you need this? the fini below should already be smart enough to only cleanup when needed. > + > pxp->arb_is_valid = false; > > intel_pxp_tee_component_fini(pxp); > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h > index 2da309088c6d..c12e4d419c78 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h > @@ -13,6 +13,8 @@ struct intel_pxp; > struct drm_i915_gem_object; > > struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp); > +bool intel_gtpxp_is_supported(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..124663cf0047 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_gtpxp_is_supported(pxp)) > return; > This now returns true for DG2, but we don't want to register the PXP debugfs there as we don't support PXP aside from HuC loading. IMO a better approach would be to have intel_gtpxp_is_supported be what you currently have as _gt_supports_pxp(). Also, intel_gtpxp_is_supported is a bit confusing because of the new "gtpxp" prefix. Why not use just intel_pxp_is_supported? We already have per-gt checkers that refer only to the subcomponent, like intel_huc_is_supported(), which for MTL is false on the primary GT and true on the media one. I don't see why we can't do the same for PXP. Daniele > root = debugfs_create_dir("pxp", gt_root);
On Mon, 2022-11-14 at 20:00 -0800, Ceraolo Spurio, Daniele wrote: > > On 10/21/2022 10:39 AM, Alan Previn wrote: > > In preparation for future MTL-PXP feature support, PXP control > > @@ -142,22 +166,21 @@ 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; > > - > > /* > > * 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 (_gt_supports_pxp(gt)) > > 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); > > } > > > > void intel_pxp_fini(struct intel_pxp *pxp) > > { > > + if (!intel_gtpxp_is_supported(pxp)) > > + return; > > Why do you need this? the fini below should already be smart enough to > only cleanup when needed. Eventually i plan to create a backend abstraction for tee based vs mtl's gscccs based and rather keep as much of the checking on the front end to keep it cleaner. > > > + > > pxp->arb_is_valid = false; > > > > intel_pxp_tee_component_fini(pxp); > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h > > index 2da309088c6d..c12e4d419c78 100644 > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h > > @@ -13,6 +13,8 @@ struct intel_pxp; > > struct drm_i915_gem_object; > > > > struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp); > > +bool intel_gtpxp_is_supported(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..124663cf0047 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_gtpxp_is_supported(pxp)) > > return; > > > > This now returns true for DG2, but we don't want to register the PXP > debugfs there as we don't support PXP aside from HuC loading. > yeah - ok. > IMO a > better approach would be to have intel_gtpxp_is_supported be what you > currently have as _gt_supports_pxp(). > Okay, let me take a look at that since i recall that future patches would rely on intel_gtpxp_is_supported for the case where PXP is not supported but we just want to know if GT has backend tee for component binding or something - but i guess that could get a separate function as opposed to reusing intel_gtpxp_is_supported. > Also, intel_gtpxp_is_supported is a bit confusing because of the new > "gtpxp" prefix. Why not use just intel_pxp_is_supported? We already have > per-gt checkers that refer only to the subcomponent, like > intel_huc_is_supported(), which for MTL is false on the primary GT and > true on the media one. I don't see why we can't do the same for PXP. I think that existing method isn't a good way - i rather use this opportunity to set a precendence for pxp we can have a more standardized naming convention based on the global-vs-per-gt level checking (i also wish i had time to look at "intra-vs-inter function naming). So when something is called with _pxp_ its meant to be called as a global check (passing in i915 as its param) and if its using _gtpxp_, then its meant to be called as gt-level checker. And the similar function name should be okay if the check is similar (just at different hierarchy level). I prefer my way because it allows that understanding purely from the function name as opposed to having to look at the full definition before knowing if its a global check vs a gt level check. I think we really ought to have a more concise naming convention as opposed to "we do it like this, so why not just follow". An alternative would be instead of "intel_gtpxp_is_supported" then "intel_gt_supports_pxp". > > Daniele > > > root = debugfs_create_dir("pxp", gt_root); >
just recapping offline conversation summary - we agreed on: intel_pxp_enabled(i915) intel_pxp_enabled_on_gt(pxp) (where one is wrapper over the other, the action part of the function name stays the same). ...alan On Mon, 2022-11-14 at 21:13 -0800, Alan Previn Teres Alexis wrote: > > On Mon, 2022-11-14 at 20:00 -0800, Ceraolo Spurio, Daniele wrote: > > > > On 10/21/2022 10:39 AM, Alan Previn wrote: > > > In preparation for future MTL-PXP feature support, PXP control > > > @@ -142,22 +166,21 @@ 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; > > > - > > > /* > > > * 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 (_gt_supports_pxp(gt)) > > > 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); > > > } > > > > > > void intel_pxp_fini(struct intel_pxp *pxp) > > > { > > > + if (!intel_gtpxp_is_supported(pxp)) > > > + return; > > > > Why do you need this? the fini below should already be smart enough to > > only cleanup when needed. > Eventually i plan to create a backend abstraction for tee based vs mtl's gscccs based and rather keep as much of the > checking on the front end to keep it cleaner. > > > > > + > > > pxp->arb_is_valid = false; > > > > > > intel_pxp_tee_component_fini(pxp); > > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h > > > index 2da309088c6d..c12e4d419c78 100644 > > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h > > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h > > > @@ -13,6 +13,8 @@ struct intel_pxp; > > > struct drm_i915_gem_object; > > > > > > struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp); > > > +bool intel_gtpxp_is_supported(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..124663cf0047 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_gtpxp_is_supported(pxp)) > > > return; > > > > > > > This now returns true for DG2, but we don't want to register the PXP > > debugfs there as we don't support PXP aside from HuC loading. > > > > yeah - ok. > > > IMO a > > better approach would be to have intel_gtpxp_is_supported be what you > > currently have as _gt_supports_pxp(). > > > Okay, let me take a look at that since i recall that future patches would rely on intel_gtpxp_is_supported for the case > where PXP is not supported but we just want to know if GT has backend tee for component binding or something - but i > guess that could get a separate function as opposed to reusing intel_gtpxp_is_supported. > > > > Also, intel_gtpxp_is_supported is a bit confusing because of the new > > "gtpxp" prefix. Why not use just intel_pxp_is_supported? We already have > > per-gt checkers that refer only to the subcomponent, like > > intel_huc_is_supported(), which for MTL is false on the primary GT and > > true on the media one. I don't see why we can't do the same for PXP. > > I think that existing method isn't a good way - i rather use this opportunity to set a precendence for pxp we can have a > more standardized naming convention based on the global-vs-per-gt level checking (i also wish i had time to look at > "intra-vs-inter function naming). So when something is called with _pxp_ its meant to be called as a global check > (passing in i915 as its param) and if its using _gtpxp_, then its meant to be called as gt-level checker. And the > similar function name should be okay if the check is similar (just at different hierarchy level). I prefer my way > because it allows that understanding purely from the function name as opposed to having to look at the full definition > before knowing if its a global check vs a gt level check. I think we really ought to have a more concise naming > convention as opposed to "we do it like this, so why not just follow". An alternative would be instead of > "intel_gtpxp_is_supported" then "intel_gt_supports_pxp". > > > > > > Daniele > > > > > root = debugfs_create_dir("pxp", gt_root); > > >
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 7c64f8a17493..0921d1107825 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -923,10 +923,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..545c075bf1aa 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c @@ -44,6 +44,30 @@ 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)); +} + +static bool _gt_supports_pxp(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) && IS_ENABLED(CONFIG_DRM_I915_PXP) && + INTEL_INFO((gt)->i915)->has_pxp && VDBOX_MASK(gt)); +} + +bool intel_gtpxp_is_supported(struct intel_pxp *pxp) +{ + struct intel_gt *gt = pxp_to_gt(pxp); + + if (_gt_needs_teelink(gt) || _gt_supports_pxp(gt)) + return true; + + return false; +} + bool intel_pxp_is_enabled(const struct intel_pxp *pxp) { return pxp->ce; @@ -142,22 +166,21 @@ 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; - /* * 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 (_gt_supports_pxp(gt)) 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); } void intel_pxp_fini(struct intel_pxp *pxp) { + if (!intel_gtpxp_is_supported(pxp)) + return; + pxp->arb_is_valid = false; intel_pxp_tee_component_fini(pxp); diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h index 2da309088c6d..c12e4d419c78 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h @@ -13,6 +13,8 @@ struct intel_pxp; struct drm_i915_gem_object; struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp); +bool intel_gtpxp_is_supported(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..124663cf0047 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_gtpxp_is_supported(pxp)) return; root = debugfs_create_dir("pxp", gt_root);
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. Add TODO for Meteorlake that will come in future series. Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 4 --- drivers/gpu/drm/i915/pxp/intel_pxp.c | 35 ++++++++++++++++---- drivers/gpu/drm/i915/pxp/intel_pxp.h | 2 ++ drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c | 2 +- 4 files changed, 32 insertions(+), 11 deletions(-)