[Intel-gfx] [PATCH v4 1/6] drm/i915/pxp: Make gt and pxp init/fini aware of PXP-owning-GT

[Teres Alexis, Alan Previn]

On Mon, 2022-11-21 at 14:06 +0000, Vivi, Rodrigo wrote:
> On Mon, 2022-11-21 at 11:39 +0000, Tvrtko Ursulin wrote:
> > 
> > On 17/11/2022 22:34, Teres Alexis, Alan Previn wrote:
> > > On Thu, 2022-11-17 at 11:02 -0500, Vivi, Rodrigo wrote:
> > > > On Wed, Nov 16, 2022 at 04:30:13PM -0800, Alan Previn wrote:
> > > > > In preparation for future MTL-PXP feature support, PXP control
> > > > > context should only valid on the correct gt tile. Depending on
> > > > > the
> > > > > +bool intel_pxp_supported_on_gt(const struct intel_pxp *pxp)
> > > > 
> > > > If we are asking if it is supported on gt, then the argument must
> > > > be a gt struct.
> > > > 
> > > I agree with you but Daniele said above is more consistent with
> > > existing ways that is considered the standard.
> > > Respectfully and humbly I would like to request both yourself and
> > > Daniele to show me the coding guidelines somewhere.
> > > 
> > > Honestly, this is one of the first few hunks of the first patch of
> > > the first series in a very large complex design to
> > > enable PXP on MTL and it only a helper utility function.
> > > Respecfully and humbly, I rather we focus our energy for review
> > > + redo  on more critical things like the e2e usage and top-to-
> > > bottom design or coding logic flows or find actual bugs
> > > instead of debating about coding styles for internal only helper
> > > functions.
> > 
> > My 2c/p is that the intel_pxp_supported_on_gt the "on_gt" part reads
> > a 
> > bit clumsy because it makes it sound like the two are independent 
> > objects. Like I could pass one pxp to different GTs and ask if that
> > one 
> > works here, or maybe there.
> > 
> > I am though a fan of intel_pxp_ prefix meaning function operates on 
> > struct intel_pxp.
> > 
> > In this case I don't know what is the correct relationship. If it is
> > 1:1 
> > between intel_pxp:intel_gt
> 
> Yeap, this is my main point here. It is not clear what is the correct
> relationship here.
> 
> Or we make the intel_pxp under the drm_i915_private, and then have the
> associated gt (always gt0?!) link inside the intel_pxp.
> 
> Or we keep it inside intel_gt as is today, but then we run all the
> functions gt agnostically and then skip when not enabled (!gt0?).
> 
> But all these functions to check "on_gt" makes the code hard to
> understand the relation and hard to maintain long term.
> 
> The argument that we have more patches in the pipeline to come on top
> of this series here just make it stronger the need for a very clean
> start.
> 
> 

Rodrigo, this is the jist of the contention - something Daniele and i discussed months back, and the direction we picked
with Option-1 below.


We have Option-1 where we stick with the current structure hierarchy - i.e. pxp is a gt-substructure. But for MTL, gt0's
pxp is not usable and media-tile's pxp is the control point. This means that backend code that accesses hw will be clean
(always already on the correct tile) - but then gem-execbuf / gem-create / gem-context (which are tile-agnostic layers
right?) are forced to pick the correct gt or have a dedicated helper to redirect from i915 layer to correct gt-tile
depending on platform. HW wise any context / buffer / request on any tile can still access PXP feature via batch level
commands.

Or for Option-2, we elevate pxp to a global level (or use ptr-to-pxp in gt) so tile-agnostic-layers call pxp without
needing to care about which tile. However, since pxp requires the ability to send commands or touch registers of the
media tile and not there other tile, such backend-code will be need that additional layer to pick the right gt. Also, we
will need to ensure the flow of init/fini and suspend/resume are "aligned" with the gt-tile level init/fini and
suspend/resume. So that presents another level of convoluted code to follow (when looking from a top-down e2e flow and
what params are being passed into different functions). 

I personally would prefer Option-2 where we elevate "intel_pxp" to a i915 level sub-structure. Based on my discussions
with the architects, they assured me that for the foreseeable future, there would always be a single "control-point" for
pxp feature and access to the backend firmware - i.e. "single" here meaning one tile only. 

In either case (option-1 or option-2) we will always be presented with one form readability confusion or another. Also
in either case we will likely have both types of function names : intel_pxp_is_enabled(pxp) vs
intel_gt_has_enabled_pxp(gt) - where one is a wrapper over the other - so we ought to keep the "enabled" part of the
name the same.

I havent thought about using a gt->pxp_ptr (like MTL's interrupt code). Lets consider this Option-3. If you think that
is an even better alternative, let me know, only the "pxp-to-gt" helpers would then need a rewrite but the init/fini
code would also have a tiny bit of kludginess since we will have to skip the allocation of said sub-structure for gt0
for MTL but not for ADL.

...alan