| Message ID | 20221117003018.1433115-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 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. > > 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 | 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. > +{ > + /* 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? > - > /* > * 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 >
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@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 > >
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. 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@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. 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 >>> >
On Mon, 21 Nov 2022, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> 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@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. > > 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 >>>> >>
On Mon, 2022-11-21 at 11:39 +0000, Tvrtko Ursulin 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. > > 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@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. > > 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 Yeap, this is my main point here. It is not clear what is the correct relationship here. Or we make the intel_pxp under the drm_i915_private, and then have the associated gt (always gt0?!) link inside the intel_pxp. Or we keep it inside intel_gt as is today, but then we run all the functions gt agnostically and then skip when not enabled (!gt0?). But all these functions to check "on_gt" makes the code hard to understand the relation and hard to maintain long term. The argument that we have more patches in the pipeline to come on top of this series here just make it stronger the need for a very clean start. > 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 > > > > > >
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@linux.intel.com> 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@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. > > > > 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
On Mon, 2022-11-21 at 11:39 +0000, Tvrtko Ursulin 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. > > 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. > > > > Okay got it - will remove that comment and ammend the commit msg to emphasis that this patch is for refactoring.
On Mon, 2022-11-21 at 14:06 +0000, Vivi, Rodrigo wrote: > On Mon, 2022-11-21 at 11:39 +0000, Tvrtko Ursulin 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 > > > > > +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 > > Yeap, this is my main point here. It is not clear what is the correct > relationship here. > > Or we make the intel_pxp under the drm_i915_private, and then have the > associated gt (always gt0?!) link inside the intel_pxp. > > Or we keep it inside intel_gt as is today, but then we run all the > functions gt agnostically and then skip when not enabled (!gt0?). > > But all these functions to check "on_gt" makes the code hard to > understand the relation and hard to maintain long term. > > The argument that we have more patches in the pipeline to come on top > of this series here just make it stronger the need for a very clean > start. > > Rodrigo, this is the jist of the contention - something Daniele and i discussed months back, and the direction we picked with Option-1 below. We have Option-1 where we stick with the current structure hierarchy - i.e. pxp is a gt-substructure. But for MTL, gt0's pxp is not usable and media-tile's pxp is the control point. This means that backend code that accesses hw will be clean (always already on the correct tile) - but then gem-execbuf / gem-create / gem-context (which are tile-agnostic layers right?) are forced to pick the correct gt or have a dedicated helper to redirect from i915 layer to correct gt-tile depending on platform. HW wise any context / buffer / request on any tile can still access PXP feature via batch level commands. Or for Option-2, we elevate pxp to a global level (or use ptr-to-pxp in gt) so tile-agnostic-layers call pxp without needing to care about which tile. However, since pxp requires the ability to send commands or touch registers of the media tile and not there other tile, such backend-code will be need that additional layer to pick the right gt. Also, we will need to ensure the flow of init/fini and suspend/resume are "aligned" with the gt-tile level init/fini and suspend/resume. So that presents another level of convoluted code to follow (when looking from a top-down e2e flow and what params are being passed into different functions). I personally would prefer Option-2 where we elevate "intel_pxp" to a i915 level sub-structure. Based on my discussions with the architects, they assured me that for the foreseeable future, there would always be a single "control-point" for pxp feature and access to the backend firmware - i.e. "single" here meaning one tile only. In either case (option-1 or option-2) we will always be presented with one form readability confusion or another. Also in either case we will likely have both types of function names : intel_pxp_is_enabled(pxp) vs intel_gt_has_enabled_pxp(gt) - where one is a wrapper over the other - so we ought to keep the "enabled" part of the name the same. I havent thought about using a gt->pxp_ptr (like MTL's interrupt code). Lets consider this Option-3. If you think that is an even better alternative, let me know, only the "pxp-to-gt" helpers would then need a rewrite but the init/fini code would also have a tiny bit of kludginess since we will have to skip the allocation of said sub-structure for gt0 for MTL but not for ADL. ...alan
On Mon, Nov 21, 2022 at 05:51:36PM +0000, Teres Alexis, Alan Previn wrote: > > > On Mon, 2022-11-21 at 14:06 +0000, Vivi, Rodrigo wrote: > > On Mon, 2022-11-21 at 11:39 +0000, Tvrtko Ursulin 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 > > > > > > +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 > > > > Yeap, this is my main point here. It is not clear what is the correct > > relationship here. > > > > Or we make the intel_pxp under the drm_i915_private, and then have the > > associated gt (always gt0?!) link inside the intel_pxp. > > > > Or we keep it inside intel_gt as is today, but then we run all the > > functions gt agnostically and then skip when not enabled (!gt0?). > > > > But all these functions to check "on_gt" makes the code hard to > > understand the relation and hard to maintain long term. > > > > The argument that we have more patches in the pipeline to come on top > > of this series here just make it stronger the need for a very clean > > start. > > > > > > Rodrigo, this is the jist of the contention - something Daniele and i discussed months back, and the direction we picked > with Option-1 below. > > > We have Option-1 where we stick with the current structure hierarchy - i.e. pxp is a gt-substructure. But for MTL, gt0's > pxp is not usable and media-tile's pxp is the control point. This means that backend code that accesses hw will be clean > (always already on the correct tile) - but then gem-execbuf / gem-create / gem-context (which are tile-agnostic layers > right?) are forced to pick the correct gt or have a dedicated helper to redirect from i915 layer to correct gt-tile > depending on platform. HW wise any context / buffer / request on any tile can still access PXP feature via batch level > commands. > > Or for Option-2, we elevate pxp to a global level (or use ptr-to-pxp in gt) so tile-agnostic-layers call pxp without > needing to care about which tile. However, since pxp requires the ability to send commands or touch registers of the > media tile and not there other tile, such backend-code will be need that additional layer to pick the right gt. Also, we > will need to ensure the flow of init/fini and suspend/resume are "aligned" with the gt-tile level init/fini and > suspend/resume. So that presents another level of convoluted code to follow (when looking from a top-down e2e flow and > what params are being passed into different functions). > > I personally would prefer Option-2 where we elevate "intel_pxp" to a i915 level sub-structure. Based on my discussions > with the architects, they assured me that for the foreseeable future, there would always be a single "control-point" for > pxp feature and access to the backend firmware - i.e. "single" here meaning one tile only. > > In either case (option-1 or option-2) we will always be presented with one form readability confusion or another. Also > in either case we will likely have both types of function names : intel_pxp_is_enabled(pxp) vs > intel_gt_has_enabled_pxp(gt) - where one is a wrapper over the other - so we ought to keep the "enabled" part of the > name the same. > > I havent thought about using a gt->pxp_ptr (like MTL's interrupt code). Lets consider this Option-3. If you think that > is an even better alternative, let me know, only the "pxp-to-gt" helpers would then need a rewrite but the init/fini > code would also have a tiny bit of kludginess since we will have to skip the allocation of said sub-structure for gt0 > for MTL but not for ADL. As I had told I don't have a strong preference, as long as it keep clean and without these many helpers of something "on_gt"... If this stays inside the gt, just make sure that you call for all the gts, but then you inside the functions you can skip if pxp not enabled... without asking if enabled on_gt... > > ...alan
On Tue, 2022-11-22 at 12:57 -0500, Vivi, Rodrigo wrote: > > [Alan:snip] > As I had told I don't have a strong preference, as long as it keep clean > and without these many helpers of something "on_gt"... > > If this stays inside the gt, just make sure that you call for all the gts, > but then you inside the functions you can skip if pxp not enabled... without > asking if enabled on_gt... > I think we have here conflicting requests. The "consumers" of pxp feature are gem-execbuf, gem-context, gem-create (and even display, for checking). Are we okay with making these callers be aware of "if mtl, ensure i call intel_pxp_foo with the media-tile's pxp, agnostic to the request, context or buffer i am dealing with now". If you are okay with this, then we can make this stay inside gt without "enabled on_gt" functions. But if dont want to polute such low level backend awareness into those upper layers, we need them to call something more global like "intel_gpu_has_pxp_enabled" or "intel_gpu_has_pxp_started" at the least with an i915 param. So that these callers dont need to worry about it. Or intel_pxp_enabled has to internally check with gt we are being passed with and verify we are on the correct gt to - but you said you dont want to have an "enabled on_gt" inside the pxp folder since pxp is a subset of gt. The only thing i can think of is just a rename - i.e. instead of "intel_pxp_enabled_on_gt", have a "intel_gpu_has_pxp_enabled(i915)" - but it would reside in the pxp folder. Would this work? > > > > ...alan
After a more comprehensive offline discussion with Daniele and Rodrigo, design direction was made to go with Option2 where we elevate pxp to a global subsystem and within it it establish a pointer to the correct gt for pxp-controls. This also reflects the current HW architectures where the PXP feature (when viewed as a service for contexts and buffers) is global to all subsystems including any workload on any tile, despite its single control-knobs being only in the media tile (because PXP controls needs to be global to the GPU to avoid any races). This would mean we move 'struct intel_pxp' to be under i915 so that all subsystems that need to call into PXP would now pass in i915 as its parameter. PXP internally would have a pointer to the correct GT (media-tile for MTL and gt0 for prior). It would also mean that certain code will still look a little kludgy or needs attention: - power-related operations like init/fini and suspend/resume would now need to called either before or after all-gt equivalents to ensure proper flow. - KCR IRQ will although being a gt level IRQ will now require passing i915 into the pxp subsystem. *NOTE: above list, even if i missed any, would still be nowhere near the amount of other external facing interfaces that would be called by global subsystems that would now look clean with i915->pxp as its param. ...alan On Tue, 2022-11-22 at 10:52 -0800, Alan Previn Teres Alexis wrote: > > On Tue, 2022-11-22 at 12:57 -0500, Vivi, Rodrigo wrote: > > > > > [Alan:snip] > > > As I had told I don't have a strong preference, as long as it keep clean > > and without these many helpers of something "on_gt"... > > > > If this stays inside the gt, just make sure that you call for all the gts, > > but then you inside the functions you can skip if pxp not enabled... without > > asking if enabled on_gt... > > > I think we have here conflicting requests. The "consumers" of pxp feature are gem-execbuf, gem-context, gem-create (and > even display, for checking). Are we okay with making these callers be aware of "if mtl, ensure i call intel_pxp_foo with > the media-tile's pxp, agnostic to the request, context or buffer i am dealing with now". If you are okay with this, then > we can make this stay inside gt without "enabled on_gt" functions. But if dont want to polute such low level backend > awareness into those upper layers, we need them to call something more global like "intel_gpu_has_pxp_enabled" or > "intel_gpu_has_pxp_started" at the least with an i915 param. So that these callers dont need to worry about it. Or > intel_pxp_enabled has to internally check with gt we are being passed with and verify we are on the correct gt to - but > you said you dont want to have an "enabled on_gt" inside the pxp folder since pxp is a subset of gt. The only thing i > can think of is just a rename - i.e. instead of "intel_pxp_enabled_on_gt", have a "intel_gpu_has_pxp_enabled(i915)" - > but it would reside in the pxp folder. Would this work? > > > > > > > ...alan >
On Tue, Nov 22, 2022 at 06:50:14PM +0000, Teres Alexis, Alan Previn wrote: > > > On Tue, 2022-11-22 at 12:57 -0500, Vivi, Rodrigo wrote: > > > > > [Alan:snip] > > > As I had told I don't have a strong preference, as long as it keep clean > > and without these many helpers of something "on_gt"... > > > > If this stays inside the gt, just make sure that you call for all the gts, > > but then you inside the functions you can skip if pxp not enabled... without > > asking if enabled on_gt... > > > I think we have here conflicting requests. The "consumers" of pxp feature are gem-execbuf, gem-context, gem-create (and > even display, for checking). Are we okay with making these callers be aware of "if mtl, ensure i call intel_pxp_foo with > the media-tile's pxp, agnostic to the request, context or buffer i am dealing with now". If you are okay with this, then > we can make this stay inside gt without "enabled on_gt" functions. But if dont want to polute such low level backend > awareness into those upper layers, we need them to call something more global like "intel_gpu_has_pxp_enabled" or > "intel_gpu_has_pxp_started" at the least with an i915 param. So that these callers dont need to worry about it. Or > intel_pxp_enabled has to internally check with gt we are being passed with and verify we are on the correct gt to - but > you said you dont want to have an "enabled on_gt" inside the pxp folder since pxp is a subset of gt. The only thing i > can think of is just a rename - i.e. instead of "intel_pxp_enabled_on_gt", have a "intel_gpu_has_pxp_enabled(i915)" - > but it would reside in the pxp folder. Would this work? In the end I believe that the pxp needs to be the one with knowledge of the serving_gt. Either gt0 on TGL or media_gt on MTL. So, if we keep the intel_pxp inside the intel_gt to make the initialization, irq, and pm flows cleaner, we need probably need to save the struct intel_gt *gt_serving_pxp; inside all the intel_pxp, or in drm_i915_private, and then have a helper that returns the ...gt_serving_pxp(...) but then skipping the init/fini functions if gt parent != gt_serving_pxp. > > > > > > > ...alan >
typo correction... On Tue, 2022-11-22 at 12:13 -0800, Alan Previn Teres Alexis wrote: > After a more comprehensive offline discussion with Daniele and Rodrigo, design direction was made to go with Option2 > where we elevate pxp to a global subsystem and within it it establish a pointer to the correct gt for pxp-controls. This > also reflects the current HW architectures where the PXP feature (when viewed as a service for contexts and buffers) is > global to all subsystems including any workload on any tile, despite its single control-knobs being only in the media > tile (because PXP controls needs to be global to the GPU to avoid any races). > > This would mean we move 'struct intel_pxp' to be under i915 so that all subsystems that need to call into PXP would now > pass in i915 as its parameter. PXP internally would have a pointer to the correct GT (media-tile for MTL and gt0 for typo: "pass in i915->pxp as its parameter" > prior). > > It would also mean that certain code will still look a little kludgy or needs attention: > - power-related operations like init/fini and suspend/resume would now need to called either before or after all-gt > equivalents to ensure proper flow. > - KCR IRQ will although being a gt level IRQ will now require passing i915 into the pxp subsystem. > > *NOTE: above list, even if i missed any, would still be nowhere near the amount of other external facing interfaces that > would be called by global subsystems that would now look clean with i915->pxp as its param. > > ...alan
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) +{ + /* 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; - /* * 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);
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 | 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(-)