On Mon, 2022-12-05 at 11:53 -0800, Ceraolo Spurio, Daniele wrote:
>
Alan:[snip]
> >
> > ext_data->flags |= I915_BO_PROTECTED;
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > index 29e9e8d5b6fe..ed74d173a092 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > @@ -869,7 +869,7 @@ static struct i915_vma *eb_lookup_vma(struct i915_execbuffer *eb, u32 handle)
> > */
> > if (i915_gem_context_uses_protected_content(eb->gem_context) &&
> > i915_gem_object_is_protected(obj)) {
> > - err = intel_pxp_key_check(&vm->gt->pxp, obj, true);
> > + err = intel_pxp_key_check(vm->gt->i915->pxp, obj, true);
>
> nit: eb->i915->pxp is one less pointer jump
>
Alan: okay
Alan:[snip]
>
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_debugfs.c b/drivers/gpu/drm/i915/gt/intel_gt_debugfs.c
> > index dd53641f3637..7876f4c3d0f1 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_debugfs.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_debugfs.c
> > @@ -99,7 +99,6 @@ void intel_gt_debugfs_register(struct intel_gt *gt)
> > intel_sseu_debugfs_register(gt, root);
> >
> > intel_uc_debugfs_register(>->uc, root);
> > - intel_pxp_debugfs_register(>->pxp, root);
>
> Nit: the pxp header inclusion can now be removed from this file.
>
Alan: okay - will fix.
Alan:[snip]
>
> > +
> > +bool intel_pxp_is_supported(const struct intel_pxp *pxp)
> > +{
> > + return (IS_ENABLED(CONFIG_DRM_I915_PXP) && pxp->ctrl_gt &&
> > + INTEL_INFO(pxp->ctrl_gt->i915)->has_pxp && VDBOX_MASK(pxp->ctrl_gt));
> > }
> >
> > bool intel_pxp_is_enabled(const struct intel_pxp *pxp)
> > @@ -130,7 +142,7 @@ static void pxp_init_full(struct intel_pxp *pxp)
> > if (ret)
> > goto out_context;
> >
> > - drm_info(>->i915->drm, "Protected Xe Path (PXP) protected content support initialized\n");
> > + drm_info(>->i915->drm, "Protected Xe Path v7B (PXP) protected content support initialized\n");
>
> This looks like a leftover debug addition
>
Alan: ooops - yeah this was not supposed to be part of the change - my mistake.
Alan:[snip]
> > -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;
> > +
> > + for_each_gt(gt, i915, i) {
> > + /* There can be only one GT that supports PXP */
> > + if (HAS_ENGINE(gt, GSC0))
> > + return gt;
> > + }
>
> Note that the GSC engine will be disabled if the GSC FW is not
> available, so in that case falling back to the root GT will be an error.
> Maybe just change the below check to be:
>
> /*
> * The GSC engine can be turned off, but on platform that
> * can have it we don't want to fall back to root GT.
> * On older platforms we rely instead on the mei PXP module.
> */
> if (IS_ENABLED(CONFIG_INTEL_MEI_PXP) && !915->media_gt)
>
Alan: okay - makes sense. will do.
Alan:[snip]
>
> >
> > /* we rely on the mei PXP module */
> > - if (!IS_ENABLED(CONFIG_INTEL_MEI_PXP))
> > - return;
> > + if (IS_ENABLED(CONFIG_INTEL_MEI_PXP))
> > + return &i915->gt0;
> > +
> > + return NULL;
> > +}
> > +
> > +int intel_pxp_init(struct drm_i915_private *i915)
> > +{
> > + i915->pxp = kzalloc(sizeof(*i915->pxp), GFP_KERNEL);
> > + if (!i915->pxp)
> > + return -ENOMEM;
>
> A failure in intel_pxp_init is non-fatal, so we can exit here without
> allocating i915->pxp and still complete driver load (although it's very
> unlikely). This means that functions that can be called from outside the
> PXP subsystem need to check if the pxp structure is allocated.
>
Alan: Okay - this is a good catch - but in that case of kernel memory allocation failure, would it make sense to fail
the driver load instead? (considering its obviously a more serious system wide issue?).
> > +
> > + i915->pxp->ctrl_gt = pxp_get_ctrl_gt(i915);
> > + if (!i915->pxp->ctrl_gt)
> > + return -ENODEV;
>
> I would do a kfree here, so the only pointer that needs to be checked is
> i915->pxp (i.e., guarantee that if i915->pxp is valid then
> i915->pxp->ctrl_gt is also valid), otherwise pxp_to_gt could return NULL
> when passing in a valid PXP pointer; although I think that all the calls
> to pxp_to_gt are guarded via a pxp_is_enabled/supported check, it's not
> intuitive for that function to return NULL (all other callers of that
> type that we have do always return a valid pointer).
>
Alan: okay sure - I guess this would also align well with a future where more and more subsystem structures are
allocated (as we try to reduce i915 build times by controlling the header-include-hierarchy) and therefore a null
subsystem structures would mean they are not enabled-or-supported.
Alan:[snip]
> > +void intel_pxp_debugfs_register(struct intel_pxp *pxp)
> > {
> > - static const struct intel_gt_debugfs_file files[] = {
> > - { "info", &pxp_info_fops, NULL },
> > - { "terminate_state", &pxp_terminate_fops, NULL },
> > - };
> > - struct dentry *root;
> > + struct drm_minor *minor;
> > + struct dentry *pxproot;
> >
> > - if (!gt_root)
> > + if (!intel_pxp_is_supported(pxp))
> > return;
> >
> > - if (!HAS_PXP((pxp_to_gt(pxp)->i915)))
> > + minor = pxp->ctrl_gt->i915->drm.primary;
> > + pxproot = debugfs_create_dir("pxp", minor->debugfs_root);
>
> can minor->debugfs_root be NULL ? in gt_debugfs_register we check for that.
>
Alan: I'm not sure but I'll add that check for consistency.
