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.