Re: [Intel-gfx] [PATCH v9 1/1] drm/i915/pxp: Promote pxp subsystem to top-level of i915
On Wed, 2022-12-07 at 13:46 +, Tvrtko Ursulin wrote:
> [Side note - your email client somehow manages to make a mess of line wraps
> so after a few replies it is super hard to follow the quote. Don't know
> how/what/why but I don't have this problem on the mailing list with other
> folks so at least I *think* it is something about your client or it's
> configuration.]
Alan: Yeah i apologize i've been struggling to find the wrong configuration -
which is why i use a lot of [snip]s
Alan: [snip]
> Null check is fine, I was a bit bothered by the helpers operating on pxp. But
> lets put this aside for now and focus on the init path.
Alan: Okay we can come back to this on next rev if we think it deserves more
scrutiny
Alan: [snip]
> >
> IS_ENABLED is always optimized away, be it 1 or 0, since it is a compile time
> constant. So it doesn't increase the number of runtime conditionals.
Alan: Right - my mistake.
Alan: [snip]
>
> > int intel_pxp_init(struct drm_i915_private *i915)
> > {
> > intel_gt *gt;
> > intel_pxp *pxp;
> > /*
> > * NOTE: Get a suitable ctrl_gt before checking intel_pxp_is_supported
Alan: ...[snip]...
> > /*
> > * 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 (intel_pxp_is_supported(pxp)) {
>
> How does the "full pxp init" branch handle the case of "have vdbox but huc fw
> is not available"? Presumably i915->pxp is not needed on that path too then?
> If so that could
> also be folded into pxp_find_suitable_ctrl_gt and then you wouldn't need the
> "else kfree" branch below.
Alan: No, on legacy platforms we do support PXP context/buffers without HuC
loaded. So we can't roll that in. But i get the intent.
> > Essentially, can you cram more of the static checks into
> > pxp_find_suitable_ctrl_gt and if that one returns a gt, then you definitely
> > know i915->pxp needs to be allocated
> > and just decide on the flavour of initialisation?
> I am not entirely sure about has_pxp but would this work:
>
> static struct intel_gt *pxp_find_suitable_ctrl_gt(struct drm_i915_private
> *i915)
> {
> ...
> if (!VDBOX_MASK(...))
> return NULL; /* Can't possibly need pxp */
Alan: For MTL, we will wrongly fail here if we check root gt (but must check
root-gt for pre-MTL), so this check would need to go into the "for_each_gt"
below to cover both.
> else if (!intel_uc_uses_huc(...))
> return NULL; /* Ditto? */
Alan: So we cant add this for the existing support legacy case as mentioned
above.
Alan: [snip]
> int intel_pxp_init(struct drm_i915_private *i915)
> {
> ...
> if (IS_ENABLED(CONFIG_DRM_I915_PXP) &&
> INTEL_INFO(pxp->ctrl_gt->i915)->has_pxp)
> pxp_init_full(pxp);
> else if (intel_huc_is_loaded_by_gsc(...))
> intel_pxp_tee_component_init(pxp);
> else
> WARN(1, "oopsie");
Alan: i get your point, we want to delay the allocation until we know for sure
so don't need to free it back. I'll think about this and get a rev-11 out with
the existing
fixes and we can continue from there. I'm assuming keeping intel_pxp_init
cleaner at the risk of rolling more of these quirky rules into helpers like
find_required_gt is the
preference.
Alan: [snip]
> P.S. s/pxp_find_suitable_ctrl_gt/pxp_find_required_ctrl_gt/, to signify it
> may not required even if it exists?
Alan: sounds good.
Re: [Intel-gfx] [PATCH v9 1/1] drm/i915/pxp: Promote pxp subsystem to top-level of i915
[Side note - your email client somehow manages to make a mess of line wraps so
after a few replies it is super hard to follow the quote. Don't know
how/what/why but I don't have this problem on the mailing list with other folks
so at least I *think* it is something about your client or it's configuration.]
On 07/12/2022 12:08, Teres Alexis, Alan Previn wrote:
On Wed, 2022-12-07 at 10:10 +, Tvrtko Ursulin wrote:
On 06/12/2022 18:26, Teres Alexis, Alan Previn wrote:
On Tue, 2022-12-06 at 10:04 +, Tvrtko Ursulin wrote:
On 06/12/2022 00:03, Alan Previn wrote:
Alan: [snip]
-struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp)
+bool intel_pxp_is_supported(const struct intel_pxp *pxp)
{
- return container_of(pxp, struct intel_gt, pxp);
+ if (!IS_ENABLED(CONFIG_DRM_I915_PXP))
+ return false;
+ else if (!pxp)
+ return false;
+ return (INTEL_INFO(pxp->ctrl_gt->i915)->has_pxp &&
VDBOX_MASK(pxp->ctrl_gt));
Intel_pxp_is_supported operating on the pxp reads a bit funny when one
of the checks is for NULL passed in object to start with.
And all callers pass in i915->pxp so my immediate thought is whether
i915_pxp_is_supported(i915) was considered?
Alan: I think you might need to track back through the last couple of months of
this patch (probably back to rev4 or
5)... I was told the coding practice is intel_subsystem_function(struct
subsystem...) so pxp should have pxp as its
input structure. We needed to make exceptions for init/fini because ptr-to-ptr
is worse - but we all agreed we dont want
viral include header hiearchys so dynamic allocation is the right way to go.
('we' included Jani + Rodrigo). As such i
I said nothing about dynamic allocation. I said, since you are promoting pxp to
be under i915, and have a this:
intel_pxp_is_supported(pxp)
{
...
if (!pxp)
return false;
(There's even a gt->i915->has_pxp in there, and a Kconfig based check, so it
really does not feel this should operate on the pxp.)
And callers to all these three function pass in, most even directly, i915->pxp,
that passing in a NULL pxp to function tell you you passed in NULL pxp reads
confused.
Alan: So prior to rev-9 i always allocated i915->pxp and would use the other
params such as the pxp->ce or those pxp-
is_arb_valid without checking the validity of the pxp ptr for those helpers.
Daniele said its very unlikely but
possible to have i915->pxp allocation fail and other functions get called and
so said its better to explicitily leave
i915->pxp as null for cases where the backend tee-hw was also not available
(and hence the ptr checking added in those
helpers).
Null check is fine, I was a bit bothered by the helpers operating on pxp. But
lets put this aside for now and focus on the init path.
I asked if this alternative was considered (function prefix follow target
object):
i915_pxp_is_supported(i915)
Or if you want to follow the alternative preference (function prefix follows
file prefix):
intel_pxp_is_supported(i915)
Alan: I believe somewhere in rev 5 or 6 this was discussed by Rodrigo/Janie ...
IIRC the summary was that coding
practice is to go with the function name + first param in the form of:
intel_[subsystem]_function(struct [subsystem]
*...) and we decided to stick with that for everything except the exceptions
for init/fini.
I can re-spin if you think its better to extend that exception to
intel_pxp_is_supported/active.
Hmmm... OR...
actually, what do you think about extending this for all other top-level entry
points that may called externally? (there
are several more: irq handler, huc-loading code, debugfs, pxp-key-check,
suspend-resume. Change all for consistency
(agnostic to whether they are prepend with a call to is_enabled/active first).
And then ... there is an completely different alternative method (building on
top of Rev8 instead):
Perhaps we can also always allocate i915->pxp to be valid and only use other params like
"pxp->component", "pxp->ce",
etc external facing functions. But i think this is not the right way - if the
far future were to see all subsystems get
dynamically allocated (and reducing header-file-triggered build times), then it
makes sense to expect nly valid
subsystems to be allocated.
I agree it is correct to have i915->pxp optionally present. That should stay.
Nothing about dropping the dynamic allocation in this question...
Oh would IS_ENABLED(CONFIG_DRM_I915_PXP) be right in
intel_pxp_is_enabled/active? If so maybe worth to add for some extra dead code
elimination.
Alan: Adding IS_ENABLED(CONFIG_DRM_I915_PXP) in intel_pxp_is_enabled/active
should be fine. But take note that on
platforms that DO support pxp, we would go thru that new line and then check 'pxp'
and 'pxp->ce' validity again so 3
cycles as opposed to 2 to accomplish the same thing - seems unnecessary to me.
NOTE: intel_pxp_is_supported is special
since it's
Re: [Intel-gfx] [PATCH v9 1/1] drm/i915/pxp: Promote pxp subsystem to top-level of i915
On Wed, 2022-12-07 at 10:10 +, Tvrtko Ursulin wrote:
> On 06/12/2022 18:26, Teres Alexis, Alan Previn wrote:
> >
> >
> > On Tue, 2022-12-06 at 10:04 +, Tvrtko Ursulin wrote:
> > > On 06/12/2022 00:03, Alan Previn wrote:
> > > >
> > Alan: [snip]
> > >
> > > >
> > > > -struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp)
> > > > +bool intel_pxp_is_supported(const struct intel_pxp *pxp)
> > > >{
> > > > - return container_of(pxp, struct intel_gt, pxp);
> > > > + if (!IS_ENABLED(CONFIG_DRM_I915_PXP))
> > > > + return false;
> > > > + else if (!pxp)
> > > > + return false;
> > > > + return (INTEL_INFO(pxp->ctrl_gt->i915)->has_pxp &&
> > > > VDBOX_MASK(pxp->ctrl_gt));
> > >
> > > Intel_pxp_is_supported operating on the pxp reads a bit funny when one
> > > of the checks is for NULL passed in object to start with.
> > >
> > > And all callers pass in i915->pxp so my immediate thought is whether
> > > i915_pxp_is_supported(i915) was considered?
> >
> >
> > Alan: I think you might need to track back through the last couple of
> > months of this patch (probably back to rev4 or
> > 5)... I was told the coding practice is intel_subsystem_function(struct
> > subsystem...) so pxp should have pxp as its
> > input structure. We needed to make exceptions for init/fini because
> > ptr-to-ptr is worse - but we all agreed we dont want
> > viral include header hiearchys so dynamic allocation is the right way to
> > go. ('we' included Jani + Rodrigo). As such i
>
> I said nothing about dynamic allocation. I said, since you are promoting pxp
> to be under i915, and have a this:
>
> intel_pxp_is_supported(pxp)
> {
> ...
> if (!pxp)
> return false;
>
> (There's even a gt->i915->has_pxp in there, and a Kconfig based check, so it
> really does not feel this should operate on the pxp.)
>
> And callers to all these three function pass in, most even directly,
> i915->pxp, that passing in a NULL pxp to function tell you you passed in NULL
> pxp reads confused.
>
Alan: So prior to rev-9 i always allocated i915->pxp and would use the other
params such as the pxp->ce or those pxp-
>is_arb_valid without checking the validity of the pxp ptr for those helpers.
>Daniele said its very unlikely but
possible to have i915->pxp allocation fail and other functions get called and
so said its better to explicitily leave
i915->pxp as null for cases where the backend tee-hw was also not available
(and hence the ptr checking added in those
helpers).
> I asked if this alternative was considered (function prefix follow target
> object):
>
> i915_pxp_is_supported(i915)
>
> Or if you want to follow the alternative preference (function prefix follows
> file prefix):
>
> intel_pxp_is_supported(i915)
>
Alan: I believe somewhere in rev 5 or 6 this was discussed by Rodrigo/Janie ...
IIRC the summary was that coding
practice is to go with the function name + first param in the form of:
intel_[subsystem]_function(struct [subsystem]
*...) and we decided to stick with that for everything except the exceptions
for init/fini.
I can re-spin if you think its better to extend that exception to
intel_pxp_is_supported/active.
Hmmm... OR...
actually, what do you think about extending this for all other top-level entry
points that may called externally? (there
are several more: irq handler, huc-loading code, debugfs, pxp-key-check,
suspend-resume. Change all for consistency
(agnostic to whether they are prepend with a call to is_enabled/active first).
And then ... there is an completely different alternative method (building on
top of Rev8 instead):
Perhaps we can also always allocate i915->pxp to be valid and only use other
params like "pxp->component", "pxp->ce",
etc external facing functions. But i think this is not the right way - if the
far future were to see all subsystems get
dynamically allocated (and reducing header-file-triggered build times), then it
makes sense to expect nly valid
subsystems to be allocated.
> Nothing about dropping the dynamic allocation in this question...
>
> Oh would IS_ENABLED(CONFIG_DRM_I915_PXP) be right in
> intel_pxp_is_enabled/active? If so maybe worth to add for some extra dead
> code elimination.
>
Alan: Adding IS_ENABLED(CONFIG_DRM_I915_PXP) in intel_pxp_is_enabled/active
should be fine. But take note that on
platforms that DO support pxp, we would go thru that new line and then check
'pxp' and 'pxp->ce' validity again so 3
cycles as opposed to 2 to accomplish the same thing - seems unnecessary to me.
NOTE: intel_pxp_is_supported is special
since it's called from within intel_pxp_init so it needs these extra checks for
the build config, has-pxp and engine-
mask.
> > wont change this - but i will wait for your confirmation before i re-rev.
> > Side note: with all due respect it would be
> > nice to have comments preceeded with "nack" or "nit" or "question".
>
> "Discussion in
Re: [Intel-gfx] [PATCH v9 1/1] drm/i915/pxp: Promote pxp subsystem to top-level of i915
On 06/12/2022 18:26, Teres Alexis, Alan Previn wrote:
On Tue, 2022-12-06 at 10:04 +, Tvrtko Ursulin wrote:
On 06/12/2022 00:03, Alan Previn wrote:
Alan: [snip]
-struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp)
+bool intel_pxp_is_supported(const struct intel_pxp *pxp)
{
- return container_of(pxp, struct intel_gt, pxp);
+ if (!IS_ENABLED(CONFIG_DRM_I915_PXP))
+ return false;
+ else if (!pxp)
+ return false;
+ return (INTEL_INFO(pxp->ctrl_gt->i915)->has_pxp &&
VDBOX_MASK(pxp->ctrl_gt));
Intel_pxp_is_supported operating on the pxp reads a bit funny when one
of the checks is for NULL passed in object to start with.
And all callers pass in i915->pxp so my immediate thought is whether
i915_pxp_is_supported(i915) was considered?
Alan: I think you might need to track back through the last couple of months of
this patch (probably back to rev4 or
5)... I was told the coding practice is intel_subsystem_function(struct
subsystem...) so pxp should have pxp as its
input structure. We needed to make exceptions for init/fini because ptr-to-ptr
is worse - but we all agreed we dont want
viral include header hiearchys so dynamic allocation is the right way to go.
('we' included Jani + Rodrigo). As such i
I said nothing about dynamic allocation. I said, since you are promoting pxp to
be under i915, and have a this:
intel_pxp_is_supported(pxp)
{
...
if (!pxp)
return false;
(There's even a gt->i915->has_pxp in there, and a Kconfig based check, so it
really does not feel this should operate on the pxp.)
And callers to all these three function pass in, most even directly, i915->pxp,
that passing in a NULL pxp to function tell you you passed in NULL pxp reads
confused.
I asked if this alternative was considered (function prefix follow target
object):
i915_pxp_is_supported(i915)
Or if you want to follow the alternative preference (function prefix follows
file prefix):
intel_pxp_is_supported(i915)
Nothing about dropping the dynamic allocation in this question...
Oh would IS_ENABLED(CONFIG_DRM_I915_PXP) be right in
intel_pxp_is_enabled/active? If so maybe worth to add for some extra dead code
elimination.
wont change this - but i will wait for your confirmation before i re-rev. Side
note: with all due respect it would be
nice to have comments preceeded with "nack" or "nit" or "question".
"Discussion in progress please hold".
Alan: [snip]
@@ -138,31 +152,63 @@ static void pxp_init_full(struct intel_pxp *pxp)
destroy_vcs_context(pxp);
}
-void intel_pxp_init(struct intel_pxp *pxp)
+static struct intel_gt *pxp_get_ctrl_gt(struct drm_i915_private *i915)
{
- struct intel_gt *gt = pxp_to_gt(pxp);
+ struct intel_gt *gt = NULL;
+ int i = 0;
No need to init.
Alan: Sorry - i hate not initing local vars - is this a nack?
Then you hate consistency as well? :) It is a very typical review feedback.
Look around the mailing list and the code and count how many needlessly
initialized locals we have. You just align it with the codebase and move on..
- /* we rely on the mei PXP module */
- if (!IS_ENABLED(CONFIG_INTEL_MEI_PXP))
- return;
+ for_each_gt(gt, i915, i) {
+ /* There can be only one GT with GSC-CS that supports PXP */
+ if (HAS_ENGINE(gt, GSC0))
+ return gt;
+ }
+
+ /* Else we rely on the GT-0 with mei PXP module */
+ if (IS_ENABLED(CONFIG_INTEL_MEI_PXP) && !i915->media_gt)
+ return >gt0;
+
None of this makes sense unless CONFIG_DRM_I915_PXP, right?
Alan: No - when we dont support PXP as a feature we still need the backend
Tee-link infrastructure that PXP provides for
GSC HuC authentication for DG2 - this existing code path. I can add some
additional comments. (im using Tee losely here
since its not actual Tee but an intel specific framework to provide access to
security firwmare).
Okay so is the answer i915->pxp is not the same PXP as CONFIG_DRM_I915_PXP. Latter
is kind of the user facing part, while i915->pxp is set when the PXP hardware
actually exists. I got this right? If so how about splitting intel_pxp_is_supported
into two (or more?) so it does not answer two separate questions?
+ return NULL;
+}
+
+int intel_pxp_init(struct drm_i915_private *i915)
+{
+ i915->pxp = kzalloc(sizeof(*i915->pxp), GFP_KERNEL);
+ if (!i915->pxp)
+ return -ENOMEM;
+
+ i915->pxp->ctrl_gt = pxp_get_ctrl_gt(i915);
+ if (!i915->pxp->ctrl_gt) {
+ kfree(i915->pxp);
+ i915->pxp = NULL;
+ return -ENODEV;
+ }
If you store ctrl_gt in a local then you don't have to allocate until
you'll know you need it, however..
Alan: see my reply below.
To be clear I was merely suggesting to avoid the need to free something just
allocated:
int intel_pxp_init(struct
Re: [Intel-gfx] [PATCH v9 1/1] drm/i915/pxp: Promote pxp subsystem to top-level of i915
Apologies, ignore this - typo on rev count - will resend with proper v10 subject (and cancel CI) On Tue, 2022-12-06 at 13:27 -0800, Teres Alexis, Alan Previn wrote: > Starting with MTL, there will be two GT-tiles, a render and media > tile. PXP as a service for supporting workloads with protected
[Intel-gfx] [PATCH v9 1/1] drm/i915/pxp: Promote pxp subsystem to top-level of i915
Starting with MTL, there will be two GT-tiles, a render and media tile. PXP as a service for supporting workloads with protected contexts and protected buffers can be subscribed by process workloads on any tile. However, depending on the platform, only one of the tiles is used for control events pertaining to PXP operation (such as creating the arbitration session and session tear-down). PXP as a global feature is accessible via batch buffer instructions on any engine/tile and the coherency across tiles is handled implicitly by the HW. In fact, for the foreseeable future, we are expecting this single-control-tile for the PXP subsystem. In MTL, it's the standalone media tile (not the root tile) because it contains the VDBOX and KCR engine (among the assets PXP relies on for those events). Looking at the current code design, each tile is represented by the intel_gt structure while the intel_pxp structure currently hangs off the intel_gt structure. Keeping the intel_pxp structure within the intel_gt structure makes some internal functionalities more straight forward but adds code complexity to code readibility and maintainibility to many external-to-pxp subsystems which may need to pick the correct intel_gt structure. An example of this would be the intel_pxp_is_active or intel_pxp_is_enabled functionality which should be viewed as a global level inquiry, not a per-gt inquiry. That said, this series promotes the intel_pxp structure into the drm_i915_private structure making it a top-level subsystem and the PXP subsystem will select the control gt internally and keep a pointer to it for internal reference. This promotion comes with two noteworthy changes: 1. Exported pxp functions that are called by external subsystems (such as intel_pxp_enabled/active) will have to check implicitly if i915->pxp is valid as that structure will not be allocated for HW that doesn't support PXP. 2. Since GT is now considered a soft-dependency of PXP we are ensuring that GT init happens before PXP init and vice versa for fini. This causes a minor ordering change whereby we previously called intel_pxp_suspend after intel_uc_suspend but now is before i915_gem_suspend_late but the change is required for correct dependency flows. Additionally, this re-order change doesn't have any impact because at that point in either case, the top level entry to i915 won't observe any PXP events (since the GPU was quiesced during suspend_prepare). Also, any PXP event doesn't really matter when we disable the PXP HW (global GT irqs are already off anyway, so even if there was a bug that generated spurious events we wouldn't see it and we would just clean it up on resume which is okay since the default fallback action for PXP would be to keep the sessions off at this suspend stage). Changes from prior revs: v9: - Cosmetic cleanups in supported/enabled/active. (Daniele). - Add comments for intel_pxp_init and pxp_get_ctrl_gt that explain the functional flow for when PXP is not supported but the backend-assets are needed for HuC authentication (Daniele and Tvrtko). - Fix two remaining functions that are accessible outside PXP that need to be checking pxp ptrs before using them: intel_pxp_irq_handler and intel_pxp_huc_load_and_auth (Tvrtko and Daniele). - User helper macro in pxp-debugfs (Tvrtko). v8: - Remove pxp_to_gt macro (Daniele). - Fix a bug in pxp_get_ctrl_gt for the case of MTL and we don't support GSC-FW on it. (Daniele). - Leave i915->pxp as NULL if we dont support PXP and in line with that, do additional validity check on i915->pxp for intel_pxp_is_supported/enabled/active (Daniele). - Remove unncessary include header from intel_gt_debugfs.c and check drm_minor i915->drm.primary (Daniele). - Other cosmetics / minor issues / more comments on suspend flow order change (Daniele). v7: - Drop i915_dev_to_pxp and in intel_pxp_init use 'i915->pxp' through out instead of local variable newpxp. (Rodrigo) - In the case intel_pxp_fini is called during driver unload but after i915 loading failed without pxp being allocated, check i915->pxp before referencing it. (Alan) v6: - Remove HAS_PXP macro and replace it with intel_pxp_is_supported because : [1] introduction of 'ctrl_gt' means we correct this for MTL's upcoming series now. [2] Also, this has little impact globally as its only used by PXP-internal callers at the moment. - Change intel_pxp_init/fini to take in i915 as its input to avoid ptr-to-ptr in init/fini calls.(Jani). - Remove the backpointer from pxp->i915 since we can use pxp->ctrl_gt->i915 if we need it. (Rodrigo). v5: - Switch from series to single patch (Rodrigo). - change function name from pxp_get_kcr_owner_gt to
Re: [Intel-gfx] [PATCH v9 1/1] drm/i915/pxp: Promote pxp subsystem to top-level of i915
On Mon, 2022-12-05 at 21:06 -0800, Ceraolo Spurio, Daniele wrote:
>
> On 12/5/2022 4:03 PM, Alan Previn wrote:
Alan:[snip]
> > @@ -39,18 +45,26 @@
> >* performed via the mei_pxp component module.
> >*/
> >
> > -struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp)
> > +bool intel_pxp_is_supported(const struct intel_pxp *pxp)
> > {
> > - return container_of(pxp, struct intel_gt, pxp);
> > + if (!IS_ENABLED(CONFIG_DRM_I915_PXP))
> > + return false;
> > + else if (!pxp)
> > + return false;
>
> nit: this could be squashed in a single line:
>
> if (!IS_ENABLED(CONFIG_DRM_I915_PXP) || !pxp)
>
> not a blocker
>
> > + return (INTEL_INFO(pxp->ctrl_gt->i915)->has_pxp &&
> > VDBOX_MASK(pxp->ctrl_gt));
> > }
> >
> > bool intel_pxp_is_enabled(const struct intel_pxp *pxp)
> > {
> > + if (!pxp)
> > + return false;
> > return pxp->ce;
>
> nit: this can be squashed as:
>
> return pxp && pxp->ce;
>
> same for the is_active below. not a blocker
>
> > }
> >
> > bool intel_pxp_is_active(const struct intel_pxp *pxp)
> > {
> > + if (!pxp)
> > + return false;
> > return pxp->arb_is_valid;
> > }
> >
> >
Alan: okay - will handle some of these nits since i need to re-rev anyway
Alan:[snip]
>
>
> This comment doesn't really explain why we exclude the case with
> media_gt. I would have preferred a line to explain that the module only
> works for pre-media_gt platforms. not a blocker.
>
Alan: Okay will add a comment.
Alan:[snip]
> > @@ -18,7 +18,7 @@
> >
> > int intel_pxp_huc_load_and_auth(struct intel_pxp *pxp)
> > {
> > - struct intel_gt *gt = pxp_to_gt(pxp);
> > + struct intel_gt *gt = pxp->ctrl_gt;
>
> This is called from outside the PXP code, so we need to check the pxp
> pointer before de-referencing it (see also pxp->pxp_component further
> down this function). It does look like the stack it's kind of circular
> so it should be safe (pxp bind -> huc load -> this function), but IMO
> better stick to the rule that all functions called from outside need a
> check.
>
> With this fixed:
>
> Reviewed-by: Daniele Ceraolo Spurio
>
Alan:Thanks will do - will respin rev10
Alan:[snip]
Re: [Intel-gfx] [PATCH v9 1/1] drm/i915/pxp: Promote pxp subsystem to top-level of i915
On Tue, 2022-12-06 at 10:04 +, Tvrtko Ursulin wrote:
> On 06/12/2022 00:03, Alan Previn wrote:
> >
Alan: [snip]
>
> >
> > -struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp)
> > +bool intel_pxp_is_supported(const struct intel_pxp *pxp)
> > {
> > - return container_of(pxp, struct intel_gt, pxp);
> > + if (!IS_ENABLED(CONFIG_DRM_I915_PXP))
> > + return false;
> > + else if (!pxp)
> > + return false;
> > + return (INTEL_INFO(pxp->ctrl_gt->i915)->has_pxp &&
> > VDBOX_MASK(pxp->ctrl_gt));
>
> Intel_pxp_is_supported operating on the pxp reads a bit funny when one
> of the checks is for NULL passed in object to start with.
>
> And all callers pass in i915->pxp so my immediate thought is whether
> i915_pxp_is_supported(i915) was considered?
Alan: I think you might need to track back through the last couple of months of
this patch (probably back to rev4 or
5)... I was told the coding practice is intel_subsystem_function(struct
subsystem...) so pxp should have pxp as its
input structure. We needed to make exceptions for init/fini because ptr-to-ptr
is worse - but we all agreed we dont want
viral include header hiearchys so dynamic allocation is the right way to go.
('we' included Jani + Rodrigo). As such i
wont change this - but i will wait for your confirmation before i re-rev. Side
note: with all due respect it would be
nice to have comments preceeded with "nack" or "nit" or "question".
Alan: [snip]
> >
> >
> >
> > @@ -138,31 +152,63 @@ static void pxp_init_full(struct intel_pxp *pxp)
> > destroy_vcs_context(pxp);
> > }
> >
> > -void intel_pxp_init(struct intel_pxp *pxp)
> > +static struct intel_gt *pxp_get_ctrl_gt(struct drm_i915_private *i915)
> > {
> > - struct intel_gt *gt = pxp_to_gt(pxp);
> > + struct intel_gt *gt = NULL;
> > + int i = 0;
>
> No need to init.
Alan: Sorry - i hate not initing local vars - is this a nack?
>
> >
> > - /* we rely on the mei PXP module */
> > - if (!IS_ENABLED(CONFIG_INTEL_MEI_PXP))
> > - return;
> > + for_each_gt(gt, i915, i) {
> > + /* There can be only one GT with GSC-CS that supports PXP */
> > + if (HAS_ENGINE(gt, GSC0))
> > + return gt;
> > + }
> > +
> > + /* Else we rely on the GT-0 with mei PXP module */
> > + if (IS_ENABLED(CONFIG_INTEL_MEI_PXP) && !i915->media_gt)
> > + return >gt0;
> > +
>
> None of this makes sense unless CONFIG_DRM_I915_PXP, right?
Alan: No - when we dont support PXP as a feature we still need the backend
Tee-link infrastructure that PXP provides for
GSC HuC authentication for DG2 - this existing code path. I can add some
additional comments. (im using Tee losely here
since its not actual Tee but an intel specific framework to provide access to
security firwmare).
>
> > + return NULL;
> > +}
> > +
> > +int intel_pxp_init(struct drm_i915_private *i915)
> > +{
> > + i915->pxp = kzalloc(sizeof(*i915->pxp), GFP_KERNEL);
> > + if (!i915->pxp)
> > + return -ENOMEM;
> > +
> > + i915->pxp->ctrl_gt = pxp_get_ctrl_gt(i915);
> > + if (!i915->pxp->ctrl_gt) {
> > + kfree(i915->pxp);
> > + i915->pxp = NULL;
> > + return -ENODEV;
> > + }
>
> If you store ctrl_gt in a local then you don't have to allocate until
> you'll know you need it, however..
Alan: see my reply below.
>
> >
> > /*
> > * 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))
> > - pxp_init_full(pxp);
> > - else if (intel_huc_is_loaded_by_gsc(>uc.huc) &&
> > intel_uc_uses_huc(>uc))
> > - intel_pxp_tee_component_init(pxp);
> > + if (intel_pxp_is_supported(i915->pxp))
> > + pxp_init_full(i915->pxp);
> > + else if (intel_huc_is_loaded_by_gsc(>pxp->ctrl_gt->uc.huc) &&
> > +intel_uc_uses_huc(>pxp->ctrl_gt->uc))
> > + intel_pxp_tee_component_init(i915->pxp);
>
> ... intel_pxp_is_supported() returnsed false so what is the purpose of
> the "else if" branch?
>
> Which of the conditions in intel_pxp_is_supported can it fail on to get
> here?
>
> And purpose of exiting init with supported = no but i915->pxp set?
>
Alan: So this was prior existing code flow i did not change - but i can add an
"else if (intel_pxp_tee_is_needed())" and
that can be a wrapper around those gsc-huc-authentication and tee backend
transport dependency needs.
> > -DEFINE_INTEL_GT_DEBUGFS_ATTRIBUTE(pxp_info);
> > +
> > +static int pxp_info_open(struct inode *inode, struct file *file)
> > +{
> > + return single_open(file, pxp_info_show, inode->i_private);
> > +}
> > +
> > +static const struct file_operations pxp_info_fops = {
> > + .owner = THIS_MODULE,
> > + .open = pxp_info_open,
> > + .read = seq_read,
> > + .llseek = seq_lseek,
> > + .release = single_release,
> > +};
>
>
Re: [Intel-gfx] [PATCH v9 1/1] drm/i915/pxp: Promote pxp subsystem to top-level of i915
On 06/12/2022 00:03, Alan Previn wrote: Starting with MTL, there will be two GT-tiles, a render and media tile. PXP as a service for supporting workloads with protected contexts and protected buffers can be subscribed by process workloads on any tile. However, depending on the platform, only one of the tiles is used for control events pertaining to PXP operation (such as creating the arbitration session and session tear-down). PXP as a global feature is accessible via batch buffer instructions on any engine/tile and the coherency across tiles is handled implicitly by the HW. In fact, for the foreseeable future, we are expecting this single-control-tile for the PXP subsystem. In MTL, it's the standalone media tile (not the root tile) because it contains the VDBOX and KCR engine (among the assets PXP relies on for those events). Looking at the current code design, each tile is represented by the intel_gt structure while the intel_pxp structure currently hangs off the intel_gt structure. Keeping the intel_pxp structure within the intel_gt structure makes some internal functionalities more straight forward but adds code complexity to code readibility and maintainibility to many external-to-pxp subsystems which may need to pick the correct intel_gt structure. An example of this would be the intel_pxp_is_active or intel_pxp_is_enabled functionality which should be viewed as a global level inquiry, not a per-gt inquiry. That said, this series promotes the intel_pxp structure into the drm_i915_private structure making it a top-level subsystem and the PXP subsystem will select the control gt internally and keep a pointer to it for internal reference. This promotion comes with two noteworthy changes: 1. Exported pxp functions that are called by external subsystems (such as intel_pxp_enabled/active) will have to check implicitly if i915->pxp is valid as that structure will not be allocated for HW that doesn't support PXP. 2. Since GT is now considered a soft-dependency of PXP we are ensuring that GT init happens before PXP init and vice versa for fini. This causes a minor ordering change whereby we previously called intel_pxp_suspend after intel_uc_suspend but now is before i915_gem_suspend_late but the change is required for correct dependency flows. Additionally, this re-order change doesn't have any impact because at that point in either case, the top level entry to i915 won't observe any PXP events (since the GPU was quiesced during suspend_prepare). Also, any PXP event doesn't really matter when we disable the PXP HW (global GT irqs are already off anyway, so even if there was a bug that generated spurious events we wouldn't see it and we would just clean it up on resume which is okay since the default fallback action for PXP would be to keep the sessions off at this suspend stage). Changes from prior revs: v8: - Remove pxp_to_gt macro (Daniele). Okay I guess, conceptually, just a pity since it would have made the diff smaller if kept. - Fix a bug in pxp_get_ctrl_gt for the case of MTL and we don't support GSC-FW on it. (Daniele). - Leave i915->pxp as NULL if we dont support PXP and in line with that, do additional validity check on i915->pxp for intel_pxp_is_supported/enabled/active (Daniele). - Remove unncessary include header from intel_gt_debugfs.c and check drm_minor i915->drm.primary (Daniele). - Other cosmetics / minor issues / more comments on suspend flow order change (Daniele). v7: - Drop i915_dev_to_pxp and in intel_pxp_init use 'i915->pxp' through out instead of local variable newpxp. (Rodrigo) - In the case intel_pxp_fini is called during driver unload but after i915 loading failed without pxp being allocated, check i915->pxp before referencing it. (Alan) v6: - Remove HAS_PXP macro and replace it with intel_pxp_is_supported because : [1] introduction of 'ctrl_gt' means we correct this for MTL's upcoming series now. [2] Also, this has little impact globally as its only used by PXP-internal callers at the moment. - Change intel_pxp_init/fini to take in i915 as its input to avoid ptr-to-ptr in init/fini calls.(Jani). - Remove the backpointer from pxp->i915 since we can use pxp->ctrl_gt->i915 if we need it. (Rodrigo). v5: - Switch from series to single patch (Rodrigo). - change function name from pxp_get_kcr_owner_gt to pxp_get_ctrl_gt. - Fix CI BAT failure by removing redundant call to intel_pxp_fini from driver-remove. - NOTE: remaining open still persists on using ptr-to-ptr and back-ptr. v4: - Instead of maintaining intel_pxp as an intel_gt structure member and creating a number of convoluted helpers that takes in i915 as input and redirects
Re: [Intel-gfx] [PATCH v9 1/1] drm/i915/pxp: Promote pxp subsystem to top-level of i915
On 12/5/2022 4:03 PM, Alan Previn wrote: Starting with MTL, there will be two GT-tiles, a render and media tile. PXP as a service for supporting workloads with protected contexts and protected buffers can be subscribed by process workloads on any tile. However, depending on the platform, only one of the tiles is used for control events pertaining to PXP operation (such as creating the arbitration session and session tear-down). PXP as a global feature is accessible via batch buffer instructions on any engine/tile and the coherency across tiles is handled implicitly by the HW. In fact, for the foreseeable future, we are expecting this single-control-tile for the PXP subsystem. In MTL, it's the standalone media tile (not the root tile) because it contains the VDBOX and KCR engine (among the assets PXP relies on for those events). Looking at the current code design, each tile is represented by the intel_gt structure while the intel_pxp structure currently hangs off the intel_gt structure. Keeping the intel_pxp structure within the intel_gt structure makes some internal functionalities more straight forward but adds code complexity to code readibility and maintainibility to many external-to-pxp subsystems which may need to pick the correct intel_gt structure. An example of this would be the intel_pxp_is_active or intel_pxp_is_enabled functionality which should be viewed as a global level inquiry, not a per-gt inquiry. That said, this series promotes the intel_pxp structure into the drm_i915_private structure making it a top-level subsystem and the PXP subsystem will select the control gt internally and keep a pointer to it for internal reference. This promotion comes with two noteworthy changes: 1. Exported pxp functions that are called by external subsystems (such as intel_pxp_enabled/active) will have to check implicitly if i915->pxp is valid as that structure will not be allocated for HW that doesn't support PXP. 2. Since GT is now considered a soft-dependency of PXP we are ensuring that GT init happens before PXP init and vice versa for fini. This causes a minor ordering change whereby we previously called intel_pxp_suspend after intel_uc_suspend but now is before i915_gem_suspend_late but the change is required for correct dependency flows. Additionally, this re-order change doesn't have any impact because at that point in either case, the top level entry to i915 won't observe any PXP events (since the GPU was quiesced during suspend_prepare). Also, any PXP event doesn't really matter when we disable the PXP HW (global GT irqs are already off anyway, so even if there was a bug that generated spurious events we wouldn't see it and we would just clean it up on resume which is okay since the default fallback action for PXP would be to keep the sessions off at this suspend stage). Changes from prior revs: v8: - Remove pxp_to_gt macro (Daniele). - Fix a bug in pxp_get_ctrl_gt for the case of MTL and we don't support GSC-FW on it. (Daniele). - Leave i915->pxp as NULL if we dont support PXP and in line with that, do additional validity check on i915->pxp for intel_pxp_is_supported/enabled/active (Daniele). - Remove unncessary include header from intel_gt_debugfs.c and check drm_minor i915->drm.primary (Daniele). - Other cosmetics / minor issues / more comments on suspend flow order change (Daniele). v7: - Drop i915_dev_to_pxp and in intel_pxp_init use 'i915->pxp' through out instead of local variable newpxp. (Rodrigo) - In the case intel_pxp_fini is called during driver unload but after i915 loading failed without pxp being allocated, check i915->pxp before referencing it. (Alan) v6: - Remove HAS_PXP macro and replace it with intel_pxp_is_supported because : [1] introduction of 'ctrl_gt' means we correct this for MTL's upcoming series now. [2] Also, this has little impact globally as its only used by PXP-internal callers at the moment. - Change intel_pxp_init/fini to take in i915 as its input to avoid ptr-to-ptr in init/fini calls.(Jani). - Remove the backpointer from pxp->i915 since we can use pxp->ctrl_gt->i915 if we need it. (Rodrigo). v5: - Switch from series to single patch (Rodrigo). - change function name from pxp_get_kcr_owner_gt to pxp_get_ctrl_gt. - Fix CI BAT failure by removing redundant call to intel_pxp_fini from driver-remove. - NOTE: remaining open still persists on using ptr-to-ptr and back-ptr. v4: - Instead of maintaining intel_pxp as an intel_gt structure member and creating a number of convoluted helpers that takes in i915 as input and redirects to the correct intel_gt or takes any intel_gt and internally replaces with the
[Intel-gfx] [PATCH v9 1/1] drm/i915/pxp: Promote pxp subsystem to top-level of i915
Starting with MTL, there will be two GT-tiles, a render and media tile. PXP as a service for supporting workloads with protected contexts and protected buffers can be subscribed by process workloads on any tile. However, depending on the platform, only one of the tiles is used for control events pertaining to PXP operation (such as creating the arbitration session and session tear-down). PXP as a global feature is accessible via batch buffer instructions on any engine/tile and the coherency across tiles is handled implicitly by the HW. In fact, for the foreseeable future, we are expecting this single-control-tile for the PXP subsystem. In MTL, it's the standalone media tile (not the root tile) because it contains the VDBOX and KCR engine (among the assets PXP relies on for those events). Looking at the current code design, each tile is represented by the intel_gt structure while the intel_pxp structure currently hangs off the intel_gt structure. Keeping the intel_pxp structure within the intel_gt structure makes some internal functionalities more straight forward but adds code complexity to code readibility and maintainibility to many external-to-pxp subsystems which may need to pick the correct intel_gt structure. An example of this would be the intel_pxp_is_active or intel_pxp_is_enabled functionality which should be viewed as a global level inquiry, not a per-gt inquiry. That said, this series promotes the intel_pxp structure into the drm_i915_private structure making it a top-level subsystem and the PXP subsystem will select the control gt internally and keep a pointer to it for internal reference. This promotion comes with two noteworthy changes: 1. Exported pxp functions that are called by external subsystems (such as intel_pxp_enabled/active) will have to check implicitly if i915->pxp is valid as that structure will not be allocated for HW that doesn't support PXP. 2. Since GT is now considered a soft-dependency of PXP we are ensuring that GT init happens before PXP init and vice versa for fini. This causes a minor ordering change whereby we previously called intel_pxp_suspend after intel_uc_suspend but now is before i915_gem_suspend_late but the change is required for correct dependency flows. Additionally, this re-order change doesn't have any impact because at that point in either case, the top level entry to i915 won't observe any PXP events (since the GPU was quiesced during suspend_prepare). Also, any PXP event doesn't really matter when we disable the PXP HW (global GT irqs are already off anyway, so even if there was a bug that generated spurious events we wouldn't see it and we would just clean it up on resume which is okay since the default fallback action for PXP would be to keep the sessions off at this suspend stage). Changes from prior revs: v8: - Remove pxp_to_gt macro (Daniele). - Fix a bug in pxp_get_ctrl_gt for the case of MTL and we don't support GSC-FW on it. (Daniele). - Leave i915->pxp as NULL if we dont support PXP and in line with that, do additional validity check on i915->pxp for intel_pxp_is_supported/enabled/active (Daniele). - Remove unncessary include header from intel_gt_debugfs.c and check drm_minor i915->drm.primary (Daniele). - Other cosmetics / minor issues / more comments on suspend flow order change (Daniele). v7: - Drop i915_dev_to_pxp and in intel_pxp_init use 'i915->pxp' through out instead of local variable newpxp. (Rodrigo) - In the case intel_pxp_fini is called during driver unload but after i915 loading failed without pxp being allocated, check i915->pxp before referencing it. (Alan) v6: - Remove HAS_PXP macro and replace it with intel_pxp_is_supported because : [1] introduction of 'ctrl_gt' means we correct this for MTL's upcoming series now. [2] Also, this has little impact globally as its only used by PXP-internal callers at the moment. - Change intel_pxp_init/fini to take in i915 as its input to avoid ptr-to-ptr in init/fini calls.(Jani). - Remove the backpointer from pxp->i915 since we can use pxp->ctrl_gt->i915 if we need it. (Rodrigo). v5: - Switch from series to single patch (Rodrigo). - change function name from pxp_get_kcr_owner_gt to pxp_get_ctrl_gt. - Fix CI BAT failure by removing redundant call to intel_pxp_fini from driver-remove. - NOTE: remaining open still persists on using ptr-to-ptr and back-ptr. v4: - Instead of maintaining intel_pxp as an intel_gt structure member and creating a number of convoluted helpers that takes in i915 as input and redirects to the correct intel_gt or takes any intel_gt and internally replaces with the correct intel_gt, promote it to be a top-level i915 structure. v3: - Rename gt level
