Re: [Intel-gfx] [PATCH v4 2/6] drm/i915/pxp: Make intel_pxp_is_enabled implicitly sort PXP-owning-GT

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

On Thu, 2022-11-17 at 11:04 -0500, Vivi, Rodrigo wrote:
> On Wed, Nov 16, 2022 at 04:30:14PM -0800, Alan Previn wrote:
> > Make intel_pxp_is_enabled a global check and implicitly find the
> > PXP-owning-GT.
> > 
> > PXP feature support is a device-config flag. In preparation for MTL
> > PXP control-context shall reside on of the two GT's. That said,
> > update intel_pxp_is_enabled to take in i915 as its input and internally
> > find the right gt to check if PXP is enabled so its transparent to
> > callers of this functions.
> > 
> > However we also need to expose the per-gt variation of this internal
> > pxp files to use (like what intel_pxp_enabled was prior) so also expose
> > a new intel_gtpxp_is_enabled function for replacement.
> > 
> > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> > ---
> >  drivers/gpu/drm/i915/gem/i915_gem_context.c  |  2 +-
> >  drivers/gpu/drm/i915/gem/i915_gem_create.c   |  2 +-
> >  drivers/gpu/drm/i915/pxp/intel_pxp.c         | 28 ++++++++++++++++++--
> >  drivers/gpu/drm/i915/pxp/intel_pxp.h         |  4 ++-
> >  drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c     |  2 +-
> >  drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c |  2 +-
> >  drivers/gpu/drm/i915/pxp/intel_pxp_irq.c     |  2 +-
> >  drivers/gpu/drm/i915/pxp/intel_pxp_pm.c      |  8 +++---
> >  drivers/gpu/drm/i915/pxp/intel_pxp_tee.c     |  4 +--
> >  9 files changed, 40 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > index 7f2831efc798..c123f4847b19 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > @@ -257,7 +257,7 @@ static int proto_context_set_protected(struct drm_i915_private *i915,
> >  
> >  	if (!protected) {
> >  		pc->uses_protected_content = false;
> > -	} else if (!intel_pxp_is_enabled(&to_gt(i915)->pxp)) {
> > +	} else if (!intel_pxp_is_enabled(i915)) {
> 
> if we are asking about pxp we should pass pxp, not i915...
> 
> 
The function is being called by gem-exec / gem-context / gem-create about the availibility of this feature globally.
I had previously discussed this with Daniele with the goal to have 2 versions (one a wrapper over the other) where u can
query "is the pxp feature available on this hw?" vs "does this gt have the enabled pxp controls"? where the latter is
more for internal PXP usage while the former is for external (gem-exec, gem-context, etc). So the naming above was
decided by Daniele. Or perhaps this might work better?

Another direction is to have the external callers not change at all (so gem-exec would continue call with either the
render-gt-pxp or the media-gt-pxp and have the internal subsystem sort out which is the correct subsystem. Internally in
our display code, when we have shared functions like clocks, buffers and such where i've seen code that takes in the
caller's crtc the top level and then internally parse across all crtcs to take the proper global actions (where
sometimes the control unit might reside on only 1 crtc). Actually, this was where rev1 was originally heading but
Daniele said that was convoluted (the internal rerouting from callers gt-pxp to the correct gt-pxp).

Respectfully and humbly, i would like to request where is the coding guideline for function naming when u have 2nd level
subsystem IPs owning control over global hw features so that we dont need to have this back and forth of conflicting
direction from different reviewers especially so long after initial reviews have started. (internally reworking future
MTL PXP series end up getting impacted here).

...alan