| Message ID | 20210525054803.7387-14-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Introduce Intel PXP | expand |
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(>->uc); > > + intel_pxp_resume(>->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(>->pxp); > intel_uc_suspend(>->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(>->pxp); > intel_uc_runtime_suspend(>->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(>->uc); > + ret = intel_uc_runtime_resume(>->uc); > + if (ret) > + return ret; > + > + intel_pxp_resume(>->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(>->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? > + > spin_unlock_irq(>->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 :) and if we are setting this any time we disable, maybe the it would deserve to be in the fini function... but not sure... > +} > + > +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 > +/* > + * 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(>->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 >
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(>->uc); >> >> + intel_pxp_resume(>->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(>->pxp); >> intel_uc_suspend(>->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(>->pxp); >> intel_uc_runtime_suspend(>->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(>->uc); >> + ret = intel_uc_runtime_resume(>->uc); >> + if (ret) >> + return ret; >> + >> + intel_pxp_resume(>->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(>->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. > >> + >> spin_unlock_irq(>->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? > > 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. > >> +} >> + >> +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? 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(>->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 >>
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(>->uc); > > > + intel_pxp_resume(>->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(>->pxp); > > > intel_uc_suspend(>->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(>->pxp); > > > intel_uc_runtime_suspend(>->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(>->uc); > > > + ret = intel_uc_runtime_resume(>->uc); > > > + if (ret) > > > + return ret; > > > + > > > + intel_pxp_resume(>->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(>->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(>->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(>->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 > > > >
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(>->uc); + intel_pxp_resume(>->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(>->pxp); intel_uc_suspend(>->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(>->pxp); intel_uc_runtime_suspend(>->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(>->uc); + ret = intel_uc_runtime_resume(>->uc); + if (ret) + return ret; + + intel_pxp_resume(>->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(>->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; + spin_unlock_irq(>->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; +} + +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 */ +/* + * 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(>->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; }