Re: [Intel-gfx] [PATCH v3 3/6] drm/i915/pxp: Make intel_pxp_is_active implicitly sort PXP-owning-GT

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

On Mon, 2022-11-14 at 20:17 -0800, Ceraolo Spurio, Daniele wrote:
> 
> On 10/21/2022 10:39 AM, Alan Previn wrote:
> > Make intel_pxp_is_active a global check and implicitly find
> > the PXP-owning-GT.
> > 
> > As per prior two patches, callers of this function shall now
> > pass in i915 since PXP is a global GPU feature. Make
> > intel_pxp_is_active implicitly find the right gt so it's transparent
> > for global view callers (like display or gem-exec).
> > 
> > However we also need to expose the per-gt variation of this for internal
> > pxp files to use (like what intel_pxp_is_active was prior) so also expose
> > a new intel_gtpxp_is_active 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/pxp/intel_pxp.c         | 14 ++++++++++++--
> >   drivers/gpu/drm/i915/pxp/intel_pxp.h         |  3 ++-
> >   drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c |  4 ++--
> >   drivers/gpu/drm/i915/pxp/intel_pxp_irq.c     |  2 +-
> >   5 files changed, 18 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > index 72f47ebda75f..798e77398acc 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > @@ -271,7 +271,7 @@ static int proto_context_set_protected(struct drm_i915_private *i915,
> >   		 */
> >   		pc->pxp_wakeref = intel_runtime_pm_get(&i915->runtime_pm);
> >   
> > -		if (!intel_pxp_is_active(&to_gt(i915)->pxp))
> > +		if (!intel_pxp_is_active(i915))
> >   			ret = intel_pxp_start(&to_gt(i915)->pxp);
> >   	}
> >   
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> > index f7c909fce97c..15f7983f6da8 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> > @@ -97,11 +97,21 @@ bool intel_pxp_is_enabled(struct drm_i915_private *i915)
> >   	return intel_gtpxp_is_enabled(&gt->pxp);
> >   }
> >   
> > -bool intel_pxp_is_active(const struct intel_pxp *pxp)
> > +bool intel_gtpxp_is_active(const struct intel_pxp *pxp)
> >   {
> >   	return pxp->arb_is_valid;
> >   }
> >   
> > +bool intel_pxp_is_active(struct drm_i915_private *i915)
> 
> again I'd suggest a different name to differentiate the 2 checkers. 
> Considering the only calling of this is from outside the PXP code is to 
> decide whether to start the arb session or not, maybe rename this to 
> intel_pxp_has_started or intel_pxp_is_running and leave the old 
> intel_pxp_is_active as-is?
> 
Again, i humbly disagree - if one is a wrapper around the other, i rather keep the action specific part of the function
name to be exactly consistent. Perhaps like earlier, we can make intel_pxp_is_active as a wrapper round
intel_gt_has_active_pxp. But i want to maintain the "active" key word to enforce that symmetry and not decouple them
(since its a wrapper relationship).

> Daniele
> 
>