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

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

++Nikula if he has suggestions on the bottom most comment.

On Tue, 2022-11-29 at 16:28 -0500, Vivi, Rodrigo wrote:
> On Mon, Nov 28, 2022 at 04:31:52PM -0800, Alan Previn wrote:
> > Starting with MTL, there will be two GT-tiles, a render and media
> > tile. PXP as a service for supporting workloads with protected
> > contexts and protected buffers can be subscribed by process
> > workloads on any tile. However, depending on the platform,
> > only one of the tiles is used for control events pertaining to PXP
> > operation (such as creating the arbitration session and session
> > tear-down). In the case of MTL, this is the media-tile.
> 
> Imho this patch shows that having the pxp under i915 instead of gt
> is the right way to go.
> 
Alan: yes, agreed. 

> but I have a few comments and doubts below...
> 
> > 
> > 
Alan: [snip]

> > @@ -138,31 +144,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_kcr_owner_gt(struct drm_i915_private *i915)
> 
> pxp_get_ctrl_gt or pxp_get_serving_gt sounds better in my opinion...
> what's "owner"?
> 
Alan: Sure- will change to pxp_get_ctrl_gt (as per the name in the header file).

> >  {
> > -	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;
> > +	}
> >  
> >  	/* 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 intel_pxp **pxp_store_ptr)
> 
> Please let's avoid the ** here and everywhere.
> 
Alan: In order to to avoid causing the entire driver into a rebuild because of any change in the intel_pxp structure,
the only way to accomplish that is to use a ptr in i915. But using a ptr means we allocate the memory at init time and
free it at fini time and those 2 cases would require the ptr-to-ptr to ensure we get the correct store. The only way i
can avoid the ** is be passing i915 as the param and then populating the ptr via i915->pxp. Would this work?

> > 
> > 
Alan:[snip]

> > @@ -12,12 +12,23 @@
> >  #include <linux/workqueue.h>
> >  
> >  struct intel_context;
> > +struct intel_gt;
> >  struct i915_pxp_component;
> > +struct drm_i915_private;
> >  
> >  /**
> >   * struct intel_pxp - pxp state
> >   */
> >  struct intel_pxp {
> > +	/** @i915: back poiner to i915*/
> > +	struct drm_i915_private *i915;
> 
> do you really need this pointer back here?
> or using a container_of should be enough?
> 
Alan: this is the same thing for above. We can use container_of if the caller passes the ptr-to-ptr ... if caller only
passes the pxp ptr, it will be passing, by reference, an allocated address. The only way I can think of to avoid this
is by dropping the ptr-to-ptr method and therefore pulling in the pxp type header into drm_i915_private header file -
which is againts the direction we are trying to head towards. (cc-ing Nikula is he has some ideas on this)
>