Re: [PATCH v4 13/17] drm/i915/pxp: Enable PXP power management

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Jun 10, 2021 at 03:58:13PM -0700, Daniele Ceraolo Spurio wrote:
> 
> 
> On 6/2/2021 9:20 AM, Rodrigo Vivi wrote:
> > On Mon, May 24, 2021 at 10:47:59PM -0700, Daniele Ceraolo Spurio wrote:
> > > From: "Huang, Sean Z" <sean.z.huang@intel.com>
> > > 
> > > During the power event S3+ sleep/resume, hardware will lose all the
> > > encryption keys for every hardware session, even though the
> > > session state might still be marked as alive after resume. Therefore,
> > > we should consider the session as dead on suspend and invalidate all the
> > > objects. The session will be automatically restarted on the first
> > > protected submission on resume.
> > > 
> > > v2: runtime suspend also invalidates the keys
> > > v3: fix return codes, simplify rpm ops (Chris), use the new worker func
> > > v4: invalidate the objects on suspend, don't re-create the arb sesson on
> > > resume (delayed to first submission).
> > > 
> > > Signed-off-by: Huang, Sean Z <sean.z.huang@intel.com>
> > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/Makefile                |  1 +
> > >   drivers/gpu/drm/i915/gt/intel_gt_pm.c        | 15 +++++++-
> > >   drivers/gpu/drm/i915/i915_drv.c              |  2 +
> > >   drivers/gpu/drm/i915/pxp/intel_pxp_irq.c     | 11 ++++--
> > >   drivers/gpu/drm/i915/pxp/intel_pxp_pm.c      | 40 ++++++++++++++++++++
> > >   drivers/gpu/drm/i915/pxp/intel_pxp_pm.h      | 23 +++++++++++
> > >   drivers/gpu/drm/i915/pxp/intel_pxp_session.c | 38 ++++++++++++++-----
> > >   drivers/gpu/drm/i915/pxp/intel_pxp_tee.c     |  9 +++++
> > >   8 files changed, 124 insertions(+), 15 deletions(-)
> > >   create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> > >   create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_pm.h
> > > 
> > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> > > index 29331bbb3e98..9cce0bf9a50f 100644
> > > --- a/drivers/gpu/drm/i915/Makefile
> > > +++ b/drivers/gpu/drm/i915/Makefile
> > > @@ -278,6 +278,7 @@ i915-$(CONFIG_DRM_I915_PXP) += \
> > >   	pxp/intel_pxp.o \
> > >   	pxp/intel_pxp_cmd.o \
> > >   	pxp/intel_pxp_irq.o \
> > > +	pxp/intel_pxp_pm.o \
> > >   	pxp/intel_pxp_session.o \
> > >   	pxp/intel_pxp_tee.o
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> > > index aef3084e8b16..91151a02f7a2 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> > > @@ -19,6 +19,7 @@
> > >   #include "intel_rc6.h"
> > >   #include "intel_rps.h"
> > >   #include "intel_wakeref.h"
> > > +#include "pxp/intel_pxp_pm.h"
> > >   static void user_forcewake(struct intel_gt *gt, bool suspend)
> > >   {
> > > @@ -265,6 +266,8 @@ int intel_gt_resume(struct intel_gt *gt)
> > >   	intel_uc_resume(&gt->uc);
> > > +	intel_pxp_resume(&gt->pxp);
> > > +
> > >   	user_forcewake(gt, false);
> > >   out_fw:
> > > @@ -299,6 +302,7 @@ void intel_gt_suspend_prepare(struct intel_gt *gt)
> > >   	user_forcewake(gt, true);
> > >   	wait_for_suspend(gt);
> > > +	intel_pxp_suspend(&gt->pxp);
> > >   	intel_uc_suspend(&gt->uc);
> > >   }
> > > @@ -349,6 +353,7 @@ void intel_gt_suspend_late(struct intel_gt *gt)
> > >   void intel_gt_runtime_suspend(struct intel_gt *gt)
> > >   {
> > > +	intel_pxp_suspend(&gt->pxp);
> > >   	intel_uc_runtime_suspend(&gt->uc);
> > >   	GT_TRACE(gt, "\n");
> > > @@ -356,11 +361,19 @@ void intel_gt_runtime_suspend(struct intel_gt *gt)
> > >   int intel_gt_runtime_resume(struct intel_gt *gt)
> > >   {
> > > +	int ret;
> > > +
> > >   	GT_TRACE(gt, "\n");
> > >   	intel_gt_init_swizzling(gt);
> > >   	intel_ggtt_restore_fences(gt->ggtt);
> > > -	return intel_uc_runtime_resume(&gt->uc);
> > > +	ret = intel_uc_runtime_resume(&gt->uc);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	intel_pxp_resume(&gt->pxp);
> > > +
> > > +	return 0;
> > >   }
> > >   static ktime_t __intel_gt_get_awake_time(const struct intel_gt *gt)
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > index 2f06bb7b3ed2..6543e5577709 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -68,6 +68,8 @@
> > >   #include "gt/intel_gt_pm.h"
> > >   #include "gt/intel_rc6.h"
> > > +#include "pxp/intel_pxp_pm.h"
> > > +
> > >   #include "i915_debugfs.h"
> > >   #include "i915_drv.h"
> > >   #include "i915_ioc32.h"
> > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
> > > index a230d0034e50..9e5847c653f2 100644
> > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
> > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
> > > @@ -9,6 +9,7 @@
> > >   #include "gt/intel_gt_irq.h"
> > >   #include "i915_irq.h"
> > >   #include "i915_reg.h"
> > > +#include "intel_runtime_pm.h"
> > >   /**
> > >    * intel_pxp_irq_handler - Handles PXP interrupts.
> > > @@ -62,11 +63,13 @@ void intel_pxp_irq_enable(struct intel_pxp *pxp)
> > >   	struct intel_gt *gt = pxp_to_gt(pxp);
> > >   	spin_lock_irq(&gt->irq_lock);
> > > -	if (!pxp->irq_enabled) {
> > > +
> > > +	if (!pxp->irq_enabled)
> > >   		WARN_ON_ONCE(gen11_gt_reset_one_iir(gt, 0, GEN11_KCR));
> > > -		__pxp_set_interrupts(gt, GEN12_PXP_INTERRUPTS);
> > > -		pxp->irq_enabled = true;
> > > -	}
> > > +
> > > +	__pxp_set_interrupts(gt, GEN12_PXP_INTERRUPTS);
> > > +	pxp->irq_enabled = true;
> > why?
> > and if we really need this maybe worth a squash on the other patch or a separated new one?
> 
> I had some troubles with the driver resetting all interrupts on S3 exit
> behind the PXP code, so wanted to make sure we always enabled them when we
> entered this function.  I can squash this change in the patch that adds the
> irq logic if you think that's better.

yeap, please...
squash there or a separated patch explaining your issue.

> 
> > 
> > > +
> > >   	spin_unlock_irq(&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
> > > new file mode 100644
> > > index 000000000000..400b3a9944c8
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> > > @@ -0,0 +1,40 @@
> > > +// SPDX-License-Identifier: MIT
> > > +/*
> > > + * Copyright(c) 2020 Intel Corporation.
> > > + */
> > > +
> > > +#include "intel_pxp.h"
> > > +#include "intel_pxp_irq.h"
> > > +#include "intel_pxp_pm.h"
> > > +#include "intel_pxp_session.h"
> > > +
> > > +void intel_pxp_suspend(struct intel_pxp *pxp)
> > > +{
> > > +	if (!intel_pxp_is_enabled(pxp))
> > > +		return;
> > > +
> > > +	pxp->arb_is_valid = false;
> > > +
> > > +	/* invalidate protected objects */
> > > +	intel_pxp_invalidate(pxp);
> > > +
> > > +	intel_pxp_fini_hw(pxp);
> > > +
> > > +	pxp->global_state_attacked = false;
> > this attacked var name is really bad :)
> 
> pxp->hw_state_invalidated?

I like this more... but up to you!

> 
> > 
> > and if we are setting this any time we disable, maybe the it would deserve to be in the fini function...
> > but not sure...
> 
> I'd prefer to keep it out of the fini_hw, but I'll double check if I can
> wrap it up more nicely.

ack!

> 
> > 
> > > +}
> > > +
> > > +void intel_pxp_resume(struct intel_pxp *pxp)
> > > +{
> > > +	if (!intel_pxp_is_enabled(pxp))
> > > +		return;
> > > +
> > > +	/*
> > > +	 * The PXP component gets automatically unbound when we go into S3 and
> > > +	 * re-bound after we come out, so in that scenario we can defer the
> > > +	 * hw init to the bind call.
> > > +	 */
> > > +	if (!pxp->pxp_component)
> > > +		return;
> > > +
> > > +	intel_pxp_init_hw(pxp);
> > > +}
> > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h
> > > new file mode 100644
> > > index 000000000000..6f488789db6a
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h
> > > @@ -0,0 +1,23 @@
> > > +/* SPDX-License-Identifier: MIT */
> > // SPDX-License-Identifier: MIT
> 
> Don't we use /* for SPDX in headers?

Duh! (facepalm)
you are right /**/ for .h and // for .c
https://www.kernel.org/doc/html/latest/process/license-rules.html

sorry

> 
> Daniele
> 
> > > +/*
> > > + * Copyright(c) 2020, Intel Corporation. All rights reserved.
> > > + */
> > > +
> > > +#ifndef __INTEL_PXP_PM_H__
> > > +#define __INTEL_PXP_PM_H__
> > > +
> > > +#include "i915_drv.h"
> > > +
> > > +#ifdef CONFIG_DRM_I915_PXP
> > > +void intel_pxp_suspend(struct intel_pxp *pxp);
> > > +void intel_pxp_resume(struct intel_pxp *pxp);
> > > +#else
> > > +static inline void intel_pxp_suspend(struct intel_pxp *pxp)
> > > +{
> > > +}
> > > +static inline void intel_pxp_resume(struct intel_pxp *pxp)
> > > +{
> > > +}
> > > +#endif
> > > +
> > > +#endif /* __INTEL_PXP_PM_H__ */
> > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> > > index c21620916710..893a2bf61b5c 100644
> > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> > > @@ -21,29 +21,36 @@
> > >   static bool intel_pxp_session_is_in_play(struct intel_pxp *pxp, u32 id)
> > >   {
> > > -	struct intel_gt *gt = pxp_to_gt(pxp);
> > > +	struct intel_uncore *uncore = pxp_to_gt(pxp)->uncore;
> > >   	intel_wakeref_t wakeref;
> > >   	u32 sip = 0;
> > > -	with_intel_runtime_pm(gt->uncore->rpm, wakeref)
> > > -		sip = intel_uncore_read(gt->uncore, GEN12_KCR_SIP);
> > > +	/* if we're suspended the session is considered off */
> > > +	with_intel_runtime_pm_if_in_use(uncore->rpm, wakeref)
> > > +		sip = intel_uncore_read(uncore, GEN12_KCR_SIP);
> > >   	return sip & BIT(id);
> > >   }
> > >   static int pxp_wait_for_session_state(struct intel_pxp *pxp, u32 id, bool in_play)
> > >   {
> > > -	struct intel_gt *gt = pxp_to_gt(pxp);
> > > +	struct intel_uncore *uncore = pxp_to_gt(pxp)->uncore;
> > >   	intel_wakeref_t wakeref;
> > >   	u32 mask = BIT(id);
> > >   	int ret;
> > > -	with_intel_runtime_pm(gt->uncore->rpm, wakeref)
> > > -		ret = intel_wait_for_register(gt->uncore,
> > > -					      GEN12_KCR_SIP,
> > > -					      mask,
> > > -					      in_play ? mask : 0,
> > > -					      100);
> > > +	/* if we're suspended the session is considered off */
> > > +	wakeref = intel_runtime_pm_get_if_in_use(uncore->rpm);
> > > +	if (!wakeref)
> > > +		return in_play ? -ENODEV : 0;
> > > +
> > > +	ret = intel_wait_for_register(uncore,
> > > +				      GEN12_KCR_SIP,
> > > +				      mask,
> > > +				      in_play ? mask : 0,
> > > +				      100);
> > > +
> > > +	intel_runtime_pm_put(uncore->rpm, wakeref);
> > >   	return ret;
> > >   }
> > > @@ -132,6 +139,7 @@ void intel_pxp_session_work(struct work_struct *work)
> > >   {
> > >   	struct intel_pxp *pxp = container_of(work, typeof(*pxp), session_work);
> > >   	struct intel_gt *gt = pxp_to_gt(pxp);
> > > +	intel_wakeref_t wakeref;
> > >   	u32 events = 0;
> > >   	spin_lock_irq(&gt->irq_lock);
> > > @@ -144,6 +152,14 @@ void intel_pxp_session_work(struct work_struct *work)
> > >   	if (events & PXP_INVAL_REQUIRED)
> > >   		intel_pxp_invalidate(pxp);
> > > +	/*
> > > +	 * If we're processing an event while suspending then don't bother,
> > > +	 * we're going to re-init everything on resume anyway.
> > > +	 */
> > > +	wakeref = intel_runtime_pm_get_if_in_use(gt->uncore->rpm);
> > > +	if (!wakeref)
> > > +		return;
> > > +
> > >   	if (events & PXP_TERMINATION_REQUEST) {
> > >   		events &= ~PXP_TERMINATION_COMPLETE;
> > >   		pxp_terminate(pxp);
> > > @@ -151,4 +167,6 @@ void intel_pxp_session_work(struct work_struct *work)
> > >   	if (events & PXP_TERMINATION_COMPLETE)
> > >   		pxp_terminate_complete(pxp);
> > > +
> > > +	intel_runtime_pm_put(gt->uncore->rpm, wakeref);
> > >   }
> > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> > > index 35b3fed4ca2f..0b6098a7b75c 100644
> > > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> > > @@ -63,14 +63,23 @@ static int intel_pxp_tee_io_message(struct intel_pxp *pxp,
> > >   static int i915_pxp_tee_component_bind(struct device *i915_kdev,
> > >   				       struct device *tee_kdev, void *data)
> > >   {
> > > +	struct drm_i915_private *i915 = kdev_to_i915(i915_kdev);
> > >   	struct intel_pxp *pxp = i915_dev_to_pxp(i915_kdev);
> > > +	intel_wakeref_t wakeref;
> > >   	pxp->pxp_component = data;
> > >   	pxp->pxp_component->tee_dev = tee_kdev;
> > > +	/* if we are suspended, the HW will be re-initialized on resume */
> > > +	wakeref = intel_runtime_pm_get_if_in_use(&i915->runtime_pm);
> > > +	if (!wakeref)
> > > +		return 0;
> > > +
> > >   	/* the component is required to fully start the PXP HW */
> > >   	intel_pxp_init_hw(pxp);
> > > +	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
> > > +
> > >   	return 0;
> > >   }
> > > -- 
> > > 2.29.2
> > > 
>