On Wed, 30 Nov 2022, "Teres Alexis, Alan Previn" <alan.previn.teres.alexis@xxxxxxxxx> wrote:
> ++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?
In general, one of two approaches should be used:
1) The caller takes care of storing/clearing the pointer:
struct intel_pxp *intel_pxp_init(void);
void intel_pxp_fini(struct intel_pxp *pxp);
2) The callee takes care of storing/clearing the pointer:
int intel_pxp_init(struct drm_i915_private *i915);
void intel_pxp_fini(struct drm_i915_private *i915);
Passing pointers to pointers is not as clean.
In this case, I'd choose 2) just because it's being called at the high
level, and it's pretty much in line with everything else.
BR,
Jani.
>
>> >
>> >
> 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)
>>
--
Jani Nikula, Intel Open Source Graphics Center
