On Mon, 21 Nov 2022, Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote: > On 17/11/2022 22:34, Teres Alexis, Alan Previn wrote: >> On Thu, 2022-11-17 at 11:02 -0500, Vivi, Rodrigo wrote: >>> On Wed, Nov 16, 2022 at 04:30:13PM -0800, Alan Previn wrote: >>>> In preparation for future MTL-PXP feature support, PXP control >>>> context should only valid on the correct gt tile. Depending on the >>>> device-info this depends on which tile owns the VEBOX and KCR. >>>> PXP is still a global feature though (despite its control-context >>>> located in the owning GT structure). Additionally, we find >>>> that the HAS_PXP macro is only used within the pxp module, >>>> >>>> That said, lets drop that HAS_PXP macro altogether and replace it >>>> with a more fitting named intel_gtpxp_is_supported and helpers >>>> so that PXP init/fini can use to verify if the referenced gt supports >>>> PXP or teelink. >>> >>> Yep, I understand you as I'm not fan of these macros, specially >>> single usage. But we need to consider that we have multiple dependencies >>> there and other cases like this in the driver... Well, but I'm not >>> opposing, but probably better to first get rid of the macro, >>> then change the behavior of the function on the next patch. >>> >>>> >>>> Add TODO for Meteorlake that will come in future series. >>> >>> This refactor patch should be standalone, without poputing it with >>> the changes that didn't came yet to this point. >>> >> Sure i can follow this rule, but it would then raise the question of "nothign is really changing anywhere for MTL, why >> are we doing this" thats why i wanted to add that placeholder. I see "TODO"s are a common thing in the driver for larger >> features that cant all be enabled at once. Respectfully and humbly, is there some documented rule? Can you show it to >> me? > > Separating refactoring from functional changes is one of the most basic > principles we follow (and not just us) and there should never be > pushback on the former due lack of functional changes. It's also documented [1][2] but that never seems to make a difference. BR, Jani. [1] https://docs.kernel.org/process/submitting-patches.html#separate-your-changes [2] https://docs.kernel.org/process/5.Posting.html#patch-preparation > > On the basic level following this guideline makes it trivial to review > the refactoring patch, and in the vast majority of cases much easier to > review the functional change patch as well. > > And easy to review means happy reviewers, faster turnaround time (easier > to carry a rebase), happier authors, easier reverts when things go bad > (only small functional patch needs to be reverted), sometimes even > easier backporting if code diverged a lot. > > Oh, and it even has potential to decrease internal technical debt. Often > you can push refactoring upstream and keep a smaller delta behind the > closed doors, when that is required. > >>>> Signed-off-by: Alan Previn <alan.previn.teres.alexis@xxxxxxxxx> >>>> --- >>>> drivers/gpu/drm/i915/i915_drv.h | 4 ---- >>>> drivers/gpu/drm/i915/pxp/intel_pxp.c | 22 ++++++++++++++------ >>>> drivers/gpu/drm/i915/pxp/intel_pxp.h | 3 +++ >>>> drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c | 2 +- >>>> 4 files changed, 20 insertions(+), 11 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >>>> index 7e3820d2c404..0616e5f0bd31 100644 >>>> --- a/drivers/gpu/drm/i915/i915_drv.h >>>> +++ b/drivers/gpu/drm/i915/i915_drv.h >>>> @@ -933,10 +933,6 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, >>>> >>>> #define HAS_GLOBAL_MOCS_REGISTERS(dev_priv) (INTEL_INFO(dev_priv)->has_global_mocs) >>>> >>>> -#define HAS_PXP(dev_priv) ((IS_ENABLED(CONFIG_DRM_I915_PXP) && \ >>>> - INTEL_INFO(dev_priv)->has_pxp) && \ >>>> - VDBOX_MASK(to_gt(dev_priv))) >>>> - >>>> #define HAS_GMCH(dev_priv) (INTEL_INFO(dev_priv)->display.has_gmch) >>>> >>>> #define HAS_GMD_ID(i915) (INTEL_INFO(i915)->has_gmd_id) >>>> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c >>>> index 5efe61f67546..d993e752bd36 100644 >>>> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c >>>> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c >>>> @@ -44,6 +44,20 @@ struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp) >>>> return container_of(pxp, struct intel_gt, pxp); >>>> } >>>> >>>> +static bool _gt_needs_teelink(struct intel_gt *gt) >>>> +{ >>>> + /* TODO: MTL won't rely on CONFIG_INTEL_MEI_PXP but on GSC engine */ >>>> + return (IS_ENABLED(CONFIG_INTEL_MEI_PXP) && intel_huc_is_loaded_by_gsc(>->uc.huc) && >>>> + intel_uc_uses_huc(>->uc)); >>>> +} >>>> + >>>> +bool intel_pxp_supported_on_gt(const struct intel_pxp *pxp) >>> >>> If we are asking if it is supported on gt, then the argument must be a gt struct. >>> >> I agree with you but Daniele said above is more consistent with existing ways that is considered the standard. >> Respectfully and humbly I would like to request both yourself and Daniele to show me the coding guidelines somewhere. >> >> Honestly, this is one of the first few hunks of the first patch of the first series in a very large complex design to >> enable PXP on MTL and it only a helper utility function. Respecfully and humbly, I rather we focus our energy for review >> + redo on more critical things like the e2e usage and top-to-bottom design or coding logic flows or find actual bugs >> instead of debating about coding styles for internal only helper functions. > > My 2c/p is that the intel_pxp_supported_on_gt the "on_gt" part reads a > bit clumsy because it makes it sound like the two are independent > objects. Like I could pass one pxp to different GTs and ask if that one > works here, or maybe there. > > I am though a fan of intel_pxp_ prefix meaning function operates on > struct intel_pxp. > > In this case I don't know what is the correct relationship. If it is 1:1 > between intel_pxp:intel_gt then intel_pxp_supported(pxp) sounds fine. > Even if a singleton that works for me. If we do have 1:1 but only want > to init the first one, but PXP truly lives in the GT block, we could > have pointers per GT, with only the gt0 one being initialized, and > perhaps a shadow i915->pxp pointer which points to gt[0]->pxp, if that > makes for better code organisation? > > Regards, > > Tvrtko > >> >> >>>> +{ >>>> + /* TODO: MTL won't rely on CONFIG_INTEL_MEI_PXP but on GSC engine */ >>>> + return (IS_ENABLED(CONFIG_INTEL_MEI_PXP) && IS_ENABLED(CONFIG_DRM_I915_PXP) && >>>> + INTEL_INFO((pxp_to_gt(pxp))->i915)->has_pxp && VDBOX_MASK(pxp_to_gt(pxp))); >>>> +} >>>> + >>>> bool intel_pxp_is_enabled(const struct intel_pxp *pxp) >>>> { >>>> return pxp->ce; >>>> @@ -142,17 +156,13 @@ void intel_pxp_init(struct intel_pxp *pxp) >>>> { >>>> struct intel_gt *gt = pxp_to_gt(pxp); >>>> >>>> - /* we rely on the mei PXP module */ >>>> - if (!IS_ENABLED(CONFIG_INTEL_MEI_PXP)) >>>> - return; >>> >>> I took a time to understand this movement based on the commit description. >>> I have the feeling that this patch deserves further split in different patches. >>> >>> But also, looking a few lines above pxp_to_gt(pxp), I believe we >>> have further refactor to do sooner. is is one pxp per gt, then we >>> need to only enable in the gt0? >>> >> In our driver, PXP as reflected by the device info is being treated as a global feature. >> PXP as a HW subsystem is "usable" device-wide from any workload on any engine on any tile (due to the internal mirror >> component and additional plumbing across the tiles). So in line with that I rather not have the gem-exec-buf, gem-create >> and gem-context calls be bothered about which GT to access to query of this global hw feature is enabled or active. >> However the control point for allocating sessions, talking to the gsc firmware and doing global teardowns are only meant >> to occur on and via the tile that owns the KCR engine which the media tile. This includes things like per-tile uncore >> power gating controls of the GSC-CS. (although some aspects like IRQ for KCR global). So as u see its not a clean per-GT >> feature. >> >> I did speak to Daniele many months back when enabling the full feature set (on internal POC work) about whether we >> should make PXP a global subsystem instead of hanging off gt but we both agreed that because the control engines are >> only located on one tile, so you might face some its gonna be a trade off one way or the other: >> - pxp a global structure, then all of the init / shutdown / suspend-resume flows would then have a different set of >> convoluted functions that try to get access to gt specific controls from a top level function flow. >> >> >> Additionally, humbly and respectfully, perhaps you can read through the internal arch HW specs through which it can be >> infered that PXP will continue to have a single entity for control events despite the feature being usable / accessible >> across all tiles. >> >>>> - >>>> /* >>>> * If HuC is loaded by GSC but PXP is disabled, we can skip the init of >>>> * the full PXP session/object management and just init the tee channel. >>>> */ >>>> - if (HAS_PXP(gt->i915)) >>>> + if (intel_pxp_supported_on_gt(pxp)) >>>> pxp_init_full(pxp); >>>> - else if (intel_huc_is_loaded_by_gsc(>->uc.huc) && intel_uc_uses_huc(>->uc)) >>>> + else if (_gt_needs_teelink(gt)) >>>> intel_pxp_tee_component_init(pxp); >>>> } >>>> >>>> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h >>>> index 2da309088c6d..efa83f9d5e24 100644 >>>> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h >>>> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h >>>> @@ -13,6 +13,9 @@ struct intel_pxp; >>>> struct drm_i915_gem_object; >>>> >>>> struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp); >>>> + >>>> +bool intel_pxp_supported_on_gt(const struct intel_pxp *pxp); >>>> + >>>> bool intel_pxp_is_enabled(const struct intel_pxp *pxp); >>>> bool intel_pxp_is_active(const struct intel_pxp *pxp); >>>> >>>> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c >>>> index 4359e8be4101..f0ad6f34624a 100644 >>>> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c >>>> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c >>>> @@ -70,7 +70,7 @@ void intel_pxp_debugfs_register(struct intel_pxp *pxp, struct dentry *gt_root) >>>> if (!gt_root) >>>> return; >>>> >>>> - if (!HAS_PXP((pxp_to_gt(pxp)->i915))) >>>> + if (!intel_pxp_supported_on_gt(pxp)) >>>> return; >>>> >>>> root = debugfs_create_dir("pxp", gt_root); >>>> -- >>>> 2.34.1 >>>> >> -- Jani Nikula, Intel Open Source Graphics Center
