On Tue, 2022-12-06 at 10:04 +0000, 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 &i915->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(&i915->pxp->ctrl_gt->uc.huc) &&
> > + intel_uc_uses_huc(&i915->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,
> > +};
>
> DEFINE_SHOW_ATTRIBUTE?
>
Alan: okay.
> > /**
> > @@ -20,7 +24,7 @@
> > */
> > void intel_pxp_irq_handler(struct intel_pxp *pxp, u16 iir)
> > {
> > - struct intel_gt *gt = pxp_to_gt(pxp);
> > + struct intel_gt *gt = pxp->ctrl_gt;
> >
> > if (GEM_WARN_ON(!intel_pxp_is_enabled(pxp))) > return;
>
> The early return is now less effective with spurious interrupts because
> potentially NULL pxp has already been dereferenced to get the gt.
>
Alan: Good catch - i will fix this by not doing the dereference first until
after the enabled check is called.
>
>
> I haven't read it all in detail but just a gut feel init flow is not
> easy enough to understand, feels like it should be streamlined and
> simplified to become as self-documenting as possible. Plus some minor
> details.
>
Alan: The init flow is mostly identical to existing codes except for bringing
the contents of HAS_PXP into the init
codes since that macro is not needed to be included from i915_drv.h (not used
externally). I can add more comments but i
don't think it would help much without understanding all of the quirks of the
PXP subsystem feature and framework. But i
can at least add some more comments.
