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@xxxxxxxxx>
> > ---
> > 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(>->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
>
>
