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

["Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>]

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(&gt->uc.huc) && intel_uc_uses_huc(&gt->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.