[Intel-gfx] [PATCH v8 1/1] drm/i915/pxp: Promote pxp subsystem to top-level of i915

[Teres Alexis, Alan Previn]

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(&gt->uc, root);
> > -	intel_pxp_debugfs_register(&gt->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(&gt->i915->drm, "Protected Xe Path (PXP) protected content support initialized\n");
> > +	drm_info(&gt->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.