[Intel-gfx] [PATCH v4 1/6] drm/i915/pxp: Make gt and pxp init/fini aware of PXP-owning-GT
Jani Nikula
jani.nikula at linux.intel.com
Mon Nov 21 12:12:28 UTC 2022
On Mon, 21 Nov 2022, Tvrtko Ursulin <tvrtko.ursulin at 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 at intel.com>
>>>> ---
>>>> drivers/gpu/drm/i915/i915_drv.h | 4 ----
>>>> drivers/gpu/drm/i915/pxp/intel_pxp.c | 22 ++++++++++++++------
>>>> drivers/gpu/drm/i915/pxp/intel_pxp.h | 3 +++
>>>> drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c | 2 +-
>>>> 4 files changed, 20 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>> index 7e3820d2c404..0616e5f0bd31 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -933,10 +933,6 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
>>>>
>>>> #define HAS_GLOBAL_MOCS_REGISTERS(dev_priv) (INTEL_INFO(dev_priv)->has_global_mocs)
>>>>
>>>> -#define HAS_PXP(dev_priv) ((IS_ENABLED(CONFIG_DRM_I915_PXP) && \
>>>> - INTEL_INFO(dev_priv)->has_pxp) && \
>>>> - VDBOX_MASK(to_gt(dev_priv)))
>>>> -
>>>> #define HAS_GMCH(dev_priv) (INTEL_INFO(dev_priv)->display.has_gmch)
>>>>
>>>> #define HAS_GMD_ID(i915) (INTEL_INFO(i915)->has_gmd_id)
>>>> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
>>>> index 5efe61f67546..d993e752bd36 100644
>>>> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
>>>> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
>>>> @@ -44,6 +44,20 @@ struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp)
>>>> return container_of(pxp, struct intel_gt, pxp);
>>>> }
>>>>
>>>> +static bool _gt_needs_teelink(struct intel_gt *gt)
>>>> +{
>>>> + /* TODO: MTL won't rely on CONFIG_INTEL_MEI_PXP but on GSC engine */
>>>> + return (IS_ENABLED(CONFIG_INTEL_MEI_PXP) && intel_huc_is_loaded_by_gsc(>->uc.huc) &&
>>>> + intel_uc_uses_huc(>->uc));
>>>> +}
>>>> +
>>>> +bool intel_pxp_supported_on_gt(const struct intel_pxp *pxp)
>>>
>>> If we are asking if it is supported on gt, then the argument must be a gt struct.
>>>
>> I agree with you but Daniele said above is more consistent with existing ways that is considered the standard.
>> Respectfully and humbly I would like to request both yourself and Daniele to show me the coding guidelines somewhere.
>>
>> Honestly, this is one of the first few hunks of the first patch of the first series in a very large complex design to
>> enable PXP on MTL and it only a helper utility function. Respecfully and humbly, I rather we focus our energy for review
>> + redo on more critical things like the e2e usage and top-to-bottom design or coding logic flows or find actual bugs
>> instead of debating about coding styles for internal only helper functions.
>
> 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
More information about the Intel-gfx
mailing list