++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)
>
