Re: [Intel-gfx] [PATCH v2 2/7] 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-10-13 at 14:10 -0700, Ceraolo Spurio, Daniele wrote:
> 
> On 10/5/2022 9:38 PM, Alan Previn wrote:
> > Make intel_pxp_is_enabled 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.
> > 
Alan:[snip]

> >   
> > +struct intel_gt *intel_pxp_get_owning_gt(struct drm_i915_private *i915)
> 
> This seems to only be used inside this file, so it should be static.
> 
will fix this.

> > +{
> > +	struct intel_gt *gt = NULL;
> > +	int i = 0;
> > +
> > +	for_each_gt(gt, i915, i) {
> > +		if (gt && gt->pxptee_iface_owner)
> > +			return gt;
> > +	}
> > +	return NULL;
> > +}
> > +
> >   struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp)
> >   {
> >   	return container_of(pxp, struct intel_gt, pxp);
> >   }
> >   
> > -bool intel_pxp_is_enabled(const struct intel_pxp *pxp)
> > +static bool _pxp_is_enabled(struct intel_pxp *pxp)
> 
> I believe this is going to be needed outside this file (more comments 
> below, I'm going to refer to this as the per-gt checker).
> 
> >   {
> >   	return pxp->ce;
> >   }
> > 
> > 
Alan:[snip]

> >   
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c
> > index f41e45763d0d..1d409149c0e8 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd.c
> > @@ -99,7 +99,7 @@ int intel_pxp_terminate_session(struct intel_pxp *pxp, u32 id)
> >   	u32 *cs;
> >   	int err = 0;
> >   
> > -	if (!intel_pxp_is_enabled(pxp))
> > +	if (!intel_pxp_is_enabled(pxp_to_gt(pxp)->i915))
> 
> This is a gt-specific function, so it should use the per-gt checker.
> 
Understood - as per offline conversation, it looks like we need both a gt-version and a global-version of
intel_pxp_enabled depending on the caller (and whether its being driven out of a callstack/subsystem that is gt specific
or not).

> >   		return 0;
> >   
> >   	rq = i915_request_create(ce);
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> > index 7b37f061044d..907d3aba7a9c 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> > @@ -18,7 +18,7 @@ static int pxp_info_show(struct seq_file *m, void *data)
> >   {
> >   	struct intel_pxp *pxp = m->private;
> >   	struct drm_printer p = drm_seq_file_printer(m);
> > -	bool enabled = intel_pxp_is_enabled(pxp);
> > +	bool enabled = intel_pxp_is_enabled(pxp_to_gt(pxp)->i915);
> 
> This is a gt-specific function, so it should use the per-gt checker.
agreed (same as above)
> 
> >   
> >   	if (!enabled) {
> >   		drm_printf(&p, "pxp disabled\n");
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
> > index c28be430718a..6f515c163d2f 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
> > @@ -22,7 +22,10 @@ void intel_pxp_irq_handler(struct intel_pxp *pxp, u16 iir)
> >   {
> >   	struct intel_gt *gt = pxp_to_gt(pxp);
> >   
> > -	if (GEM_WARN_ON(!intel_pxp_is_enabled(pxp)))
> > +	if (!gt->pxptee_iface_owner)
> > +		return;
> 
> Do you need this? the if below should include this case.
> 
You are right but let me get back to you coz of future MTL support preparation where we might need both these checks
(but perhaps we wont to debate this once we roll out the two versions of 'enabled' ( intel_pxp_enabled vs
intel_pxp_gt_enabled" then the irq handler would be calling ONLY the latter newer function that would be enough. 

> > +
> > +	if (GEM_WARN_ON(!intel_pxp_is_enabled(gt->i915)))
> >   		return;
> >   
> >   	lockdep_assert_held(gt->irq_lock);
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> > index 6a7d4e2ee138..5f713ac5c3ce 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> > @@ -11,7 +11,7 @@
> >   
> >   void intel_pxp_suspend_prepare(struct intel_pxp *pxp)
> >   {
> > -	if (!intel_pxp_is_enabled(pxp))
> > +	if (!intel_pxp_is_enabled(pxp_to_gt(pxp)->i915))
> >   		return;
> >   
> 
> This is called from a gt-specific function, so it should use the per-gt 
> checker. Same for all the other suspend/resume calls.
yeah - undestood
> 
> >   	pxp->arb_is_valid = false;
> > @@ -23,7 +23,7 @@ void intel_pxp_suspend(struct intel_pxp *pxp)
> >   {
> >   	intel_wakeref_t wakeref;
> >   
> > -	if (!intel_pxp_is_enabled(pxp))
> > +	if (!intel_pxp_is_enabled(pxp_to_gt(pxp)->i915))
> >   		return;
> >   
> >   	with_intel_runtime_pm(&pxp_to_gt(pxp)->i915->runtime_pm, wakeref) {
> > @@ -34,7 +34,7 @@ void intel_pxp_suspend(struct intel_pxp *pxp)
> >   
> >   void intel_pxp_resume(struct intel_pxp *pxp)
> >   {
> > -	if (!intel_pxp_is_enabled(pxp))
> > +	if (!intel_pxp_is_enabled(pxp_to_gt(pxp)->i915))
> >   		return;
> >   
> >   	/*
> > @@ -50,7 +50,7 @@ void intel_pxp_resume(struct intel_pxp *pxp)
> >   
> >   void intel_pxp_runtime_suspend(struct intel_pxp *pxp)
> >   {
> > -	if (!intel_pxp_is_enabled(pxp))
> > +	if (!intel_pxp_is_enabled(pxp_to_gt(pxp)->i915))
> >   		return;
> >   
> >   	pxp->arb_is_valid = false;
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> > index 052fd2f9a583..792a56edfde7 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> > @@ -152,7 +152,7 @@ static int i915_pxp_tee_component_bind(struct device *i915_kdev,
> >   		return 0;
> >   
> >   	/* the component is required to fully start the PXP HW */
> > -	if (intel_pxp_is_enabled(pxp))
> > +	if (intel_pxp_is_enabled(i915))
> >   		intel_pxp_init_hw(pxp);
> 
> This is now using the PXP from the root GT. I'd suggest to update 
> i915_dev_to_pxp:
> 
> static inline struct intel_pxp *i915_dev_to_pxp(struct device *i915_kdev)
> {
>           struct drm_i915_private *i915 = kdev_to_i915(i915_kdev);
>           struct intel_gt *gt = intel_pxp_get_owning_gt(i915);
>           return &gt->pxp;
> }
> 
> and then use the per-gt checker for pxp_enabled() with the pxp structure.
> Same below with the unbind.
> 
> Daniele
> 
> >   
> >   	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
> > @@ -167,7 +167,7 @@ static void i915_pxp_tee_component_unbind(struct device *i915_kdev,
> >   	struct intel_pxp *pxp = i915_dev_to_pxp(i915_kdev);
> >   	intel_wakeref_t wakeref;
> >   
> > -	if (intel_pxp_is_enabled(pxp))
> > +	if (intel_pxp_is_enabled(i915))
> >   		with_intel_runtime_pm_if_in_use(&i915->runtime_pm, wakeref)
> >   			intel_pxp_fini_hw(pxp);
> >   
>