| Message ID | 20210106231223.8323-9-sean.z.huang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce Intel PXP component - Mesa single session | expand |
On Wed, 2021-01-06 at 15:12 -0800, Huang, Sean Z wrote: > During the power event S3+ sleep/resume, hardware will lose all the > encryption keys for every hardware session, even though the > software session state was marked as alive after resume. So to > handle such case, PXP should terminate all the hardware sessions > and cleanup all the software states after the power cycle. > > Signed-off-by: Huang, Sean Z <sean.z.huang@intel.com> > --- > drivers/gpu/drm/i915/Makefile | 1 + > drivers/gpu/drm/i915/gt/intel_gt_pm.c | 4 ++ > drivers/gpu/drm/i915/i915_drv.c | 4 ++ > drivers/gpu/drm/i915/pxp/intel_pxp_pm.c | 65 > ++++++++++++++++++++++ > drivers/gpu/drm/i915/pxp/intel_pxp_pm.h | 31 +++++++++++ > drivers/gpu/drm/i915/pxp/intel_pxp_types.h | 1 + > 6 files changed, 106 insertions(+) > 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 5599b92bea9b..7592fc8cbd89 100644 > --- a/drivers/gpu/drm/i915/Makefile > +++ b/drivers/gpu/drm/i915/Makefile > @@ -265,6 +265,7 @@ i915-$(CONFIG_DRM_I915_PXP) += \ > pxp/intel_pxp_arb.o \ > pxp/intel_pxp_cmd.o \ > pxp/intel_pxp_context.o \ > + pxp/intel_pxp_pm.o \ > pxp/intel_pxp_tee.o > > # Post-mortem debug and GPU hang state capture > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c > b/drivers/gpu/drm/i915/gt/intel_gt_pm.c > index c94e8ac884eb..ae0387e419a2 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c > +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c > @@ -20,6 +20,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) > { > @@ -266,6 +267,8 @@ int intel_gt_resume(struct intel_gt *gt) > > intel_uc_resume(>->uc); > > + intel_pxp_pm_resume(>->pxp); > + > user_forcewake(gt, false); > > out_fw: > @@ -300,6 +303,7 @@ void intel_gt_suspend_prepare(struct intel_gt > *gt) > user_forcewake(gt, true); > wait_for_suspend(gt); > > + intel_pxp_pm_prepare_suspend(>->pxp); > intel_uc_suspend(>->uc); > } > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > b/drivers/gpu/drm/i915/i915_drv.c > index 207d50226e64..5923db004d9b 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" > @@ -1338,6 +1340,8 @@ static int i915_drm_resume_early(struct > drm_device *dev) > > intel_power_domains_resume(dev_priv); > > + intel_pxp_pm_resume_early(&dev_priv->gt.pxp); > + > enable_rpm_wakeref_asserts(&dev_priv->runtime_pm); > > return ret; > 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..ebe89262485c > --- /dev/null > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c > @@ -0,0 +1,65 @@ > +// SPDX-License-Identifier: MIT > +/* > + * Copyright(c) 2020 Intel Corporation. > + */ > + > +#include "intel_pxp_context.h" > +#include "intel_pxp_arb.h" > +#include "intel_pxp_pm.h" > + > +void intel_pxp_pm_prepare_suspend(struct intel_pxp *pxp) > +{ > + if (!pxp->ctx.inited) > + return; > + > + mutex_lock(&pxp->ctx.mutex); > + > + /* Disable PXP-IOCTLs */ > + pxp->ctx.global_state_in_suspend = true; > + > + mutex_unlock(&pxp->ctx.mutex); > +} > + > +void intel_pxp_pm_resume_early(struct intel_pxp *pxp) > +{ > + if (!pxp->ctx.inited) > + return; > + > + mutex_lock(&pxp->ctx.mutex); > + > + if (pxp->ctx.global_state_in_suspend) { > + /* reset the attacked flag even there was a pending > */ > + pxp->ctx.global_state_attacked = false; > + > + pxp->ctx.flag_display_hm_surface_keys = false; > + } > + > + mutex_unlock(&pxp->ctx.mutex); > +} > + > +int intel_pxp_pm_resume(struct intel_pxp *pxp) > +{ > + int ret = 0; > + struct intel_gt *gt = container_of(pxp, typeof(*gt), pxp); > + > + if (!pxp->ctx.inited) > + return 0; > + > + mutex_lock(&pxp->ctx.mutex); > + > + /* Re-enable PXP-IOCTLs */ > + if (pxp->ctx.global_state_in_suspend) { > + ret = intel_pxp_arb_terminate_session(pxp); I'm confused. I was expecting we terminate the session at suspend and re-stablish at resume. But I'm missing or forgetting something > + if (ret) { > + drm_err(>->i915->drm, "Failed to terminate > the arb session\n"); > + goto end; > + } > + > + pxp->ctx.global_state_in_suspend = false; > + } > + > +end: > + mutex_unlock(&pxp->ctx.mutex); > + > + return ret; > +} > 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..135bfb59aaf7 > --- /dev/null > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h > @@ -0,0 +1,31 @@ > +/* 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_pm_prepare_suspend(struct intel_pxp *pxp); > + > +void intel_pxp_pm_resume_early(struct intel_pxp *pxp); > +int intel_pxp_pm_resume(struct intel_pxp *pxp); > +#else > +static inline void intel_pxp_pm_prepare_suspend(struct intel_pxp > *pxp) > +{ > +} > + > +static inline void intel_pxp_pm_resume_early(struct intel_pxp *pxp) > +{ > +} > + > +static inline int intel_pxp_pm_resume(struct intel_pxp *pxp) > +{ > + return 0; > +} > +#endif > + > +#endif /* __INTEL_PXP_PM_H__ */ > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h > b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h > index 0b140aaeb6ed..13d7b502b7f9 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h > @@ -44,6 +44,7 @@ struct pxp_context { > > bool flag_display_hm_surface_keys; > bool global_state_attacked; > + bool global_state_in_suspend; > }; > > struct intel_pxp {
Hi Rodrigo, According to our design we terminate the session after resume, and then re-create the session at i915_pxp_tee_component_bind() in intel_pxp_tee.c We only can terminate the session after resume, instead of during the intel_pxp_pm_prepare_suspend() call, because there is a change that display is stilling rendering/playing-back the protected buffer, and protected surfaces turn corruption garbage if we terminate the session during the suspend(). Best regards, Sean -----Original Message----- From: Vivi, Rodrigo <rodrigo.vivi@intel.com> Sent: Thursday, January 7, 2021 9:53 AM To: Huang, Sean Z <sean.z.huang@intel.com>; Intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [RFC-v19 08/13] drm/i915/pxp: Enable PXP power management On Wed, 2021-01-06 at 15:12 -0800, Huang, Sean Z wrote: > During the power event S3+ sleep/resume, hardware will lose all the > encryption keys for every hardware session, even though the software > session state was marked as alive after resume. So to handle such > case, PXP should terminate all the hardware sessions and cleanup all > the software states after the power cycle. > > Signed-off-by: Huang, Sean Z <sean.z.huang@intel.com> > --- > drivers/gpu/drm/i915/Makefile | 1 + > drivers/gpu/drm/i915/gt/intel_gt_pm.c | 4 ++ > drivers/gpu/drm/i915/i915_drv.c | 4 ++ > drivers/gpu/drm/i915/pxp/intel_pxp_pm.c | 65 > ++++++++++++++++++++++ > drivers/gpu/drm/i915/pxp/intel_pxp_pm.h | 31 +++++++++++ > drivers/gpu/drm/i915/pxp/intel_pxp_types.h | 1 + > 6 files changed, 106 insertions(+) > 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 5599b92bea9b..7592fc8cbd89 > 100644 > --- a/drivers/gpu/drm/i915/Makefile > +++ b/drivers/gpu/drm/i915/Makefile > @@ -265,6 +265,7 @@ i915-$(CONFIG_DRM_I915_PXP) += \ > pxp/intel_pxp_arb.o \ > pxp/intel_pxp_cmd.o \ > pxp/intel_pxp_context.o \ > + pxp/intel_pxp_pm.o \ > pxp/intel_pxp_tee.o > > # Post-mortem debug and GPU hang state capture diff --git > a/drivers/gpu/drm/i915/gt/intel_gt_pm.c > b/drivers/gpu/drm/i915/gt/intel_gt_pm.c > index c94e8ac884eb..ae0387e419a2 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c > +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c > @@ -20,6 +20,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) { @@ > -266,6 +267,8 @@ int intel_gt_resume(struct intel_gt *gt) > > intel_uc_resume(>->uc); > > + intel_pxp_pm_resume(>->pxp); > + > user_forcewake(gt, false); > > out_fw: > @@ -300,6 +303,7 @@ void intel_gt_suspend_prepare(struct intel_gt > *gt) > user_forcewake(gt, true); > wait_for_suspend(gt); > > + intel_pxp_pm_prepare_suspend(>->pxp); > intel_uc_suspend(>->uc); > } > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > b/drivers/gpu/drm/i915/i915_drv.c index 207d50226e64..5923db004d9b > 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" > @@ -1338,6 +1340,8 @@ static int i915_drm_resume_early(struct > drm_device *dev) > > intel_power_domains_resume(dev_priv); > > + intel_pxp_pm_resume_early(&dev_priv->gt.pxp); > + > enable_rpm_wakeref_asserts(&dev_priv->runtime_pm); > > return ret; > 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..ebe89262485c > --- /dev/null > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c > @@ -0,0 +1,65 @@ > +// SPDX-License-Identifier: MIT > +/* > + * Copyright(c) 2020 Intel Corporation. > + */ > + > +#include "intel_pxp_context.h" > +#include "intel_pxp_arb.h" > +#include "intel_pxp_pm.h" > + > +void intel_pxp_pm_prepare_suspend(struct intel_pxp *pxp) { > + if (!pxp->ctx.inited) > + return; > + > + mutex_lock(&pxp->ctx.mutex); > + > + /* Disable PXP-IOCTLs */ > + pxp->ctx.global_state_in_suspend = true; > + > + mutex_unlock(&pxp->ctx.mutex); } > + > +void intel_pxp_pm_resume_early(struct intel_pxp *pxp) { > + if (!pxp->ctx.inited) > + return; > + > + mutex_lock(&pxp->ctx.mutex); > + > + if (pxp->ctx.global_state_in_suspend) { > + /* reset the attacked flag even there was a pending > */ > + pxp->ctx.global_state_attacked = false; > + > + pxp->ctx.flag_display_hm_surface_keys = false; > + } > + > + mutex_unlock(&pxp->ctx.mutex); } > + > +int intel_pxp_pm_resume(struct intel_pxp *pxp) { > + int ret = 0; > + struct intel_gt *gt = container_of(pxp, typeof(*gt), pxp); > + > + if (!pxp->ctx.inited) > + return 0; > + > + mutex_lock(&pxp->ctx.mutex); > + > + /* Re-enable PXP-IOCTLs */ > + if (pxp->ctx.global_state_in_suspend) { > + ret = intel_pxp_arb_terminate_session(pxp); I'm confused. I was expecting we terminate the session at suspend and re-stablish at resume. But I'm missing or forgetting something > + if (ret) { > + drm_err(>->i915->drm, "Failed to terminate > the arb session\n"); > + goto end; > + } > + > + pxp->ctx.global_state_in_suspend = false; > + } > + > +end: > + mutex_unlock(&pxp->ctx.mutex); > + > + return ret; > +} > 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..135bfb59aaf7 > --- /dev/null > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h > @@ -0,0 +1,31 @@ > +/* 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_pm_prepare_suspend(struct intel_pxp *pxp); > + > +void intel_pxp_pm_resume_early(struct intel_pxp *pxp); int > +intel_pxp_pm_resume(struct intel_pxp *pxp); #else static inline void > +intel_pxp_pm_prepare_suspend(struct intel_pxp > *pxp) > +{ > +} > + > +static inline void intel_pxp_pm_resume_early(struct intel_pxp *pxp) { > +} > + > +static inline int intel_pxp_pm_resume(struct intel_pxp *pxp) { > + return 0; > +} > +#endif > + > +#endif /* __INTEL_PXP_PM_H__ */ > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h > b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h > index 0b140aaeb6ed..13d7b502b7f9 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h > @@ -44,6 +44,7 @@ struct pxp_context { > > bool flag_display_hm_surface_keys; > bool global_state_attacked; > + bool global_state_in_suspend; > }; > > struct intel_pxp {
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 5599b92bea9b..7592fc8cbd89 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -265,6 +265,7 @@ i915-$(CONFIG_DRM_I915_PXP) += \ pxp/intel_pxp_arb.o \ pxp/intel_pxp_cmd.o \ pxp/intel_pxp_context.o \ + pxp/intel_pxp_pm.o \ pxp/intel_pxp_tee.o # Post-mortem debug and GPU hang state capture diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c index c94e8ac884eb..ae0387e419a2 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c @@ -20,6 +20,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) { @@ -266,6 +267,8 @@ int intel_gt_resume(struct intel_gt *gt) intel_uc_resume(>->uc); + intel_pxp_pm_resume(>->pxp); + user_forcewake(gt, false); out_fw: @@ -300,6 +303,7 @@ void intel_gt_suspend_prepare(struct intel_gt *gt) user_forcewake(gt, true); wait_for_suspend(gt); + intel_pxp_pm_prepare_suspend(>->pxp); intel_uc_suspend(>->uc); } diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 207d50226e64..5923db004d9b 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" @@ -1338,6 +1340,8 @@ static int i915_drm_resume_early(struct drm_device *dev) intel_power_domains_resume(dev_priv); + intel_pxp_pm_resume_early(&dev_priv->gt.pxp); + enable_rpm_wakeref_asserts(&dev_priv->runtime_pm); return ret; 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..ebe89262485c --- /dev/null +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c @@ -0,0 +1,65 @@ +// SPDX-License-Identifier: MIT +/* + * Copyright(c) 2020 Intel Corporation. + */ + +#include "intel_pxp_context.h" +#include "intel_pxp_arb.h" +#include "intel_pxp_pm.h" + +void intel_pxp_pm_prepare_suspend(struct intel_pxp *pxp) +{ + if (!pxp->ctx.inited) + return; + + mutex_lock(&pxp->ctx.mutex); + + /* Disable PXP-IOCTLs */ + pxp->ctx.global_state_in_suspend = true; + + mutex_unlock(&pxp->ctx.mutex); +} + +void intel_pxp_pm_resume_early(struct intel_pxp *pxp) +{ + if (!pxp->ctx.inited) + return; + + mutex_lock(&pxp->ctx.mutex); + + if (pxp->ctx.global_state_in_suspend) { + /* reset the attacked flag even there was a pending */ + pxp->ctx.global_state_attacked = false; + + pxp->ctx.flag_display_hm_surface_keys = false; + } + + mutex_unlock(&pxp->ctx.mutex); +} + +int intel_pxp_pm_resume(struct intel_pxp *pxp) +{ + int ret = 0; + struct intel_gt *gt = container_of(pxp, typeof(*gt), pxp); + + if (!pxp->ctx.inited) + return 0; + + mutex_lock(&pxp->ctx.mutex); + + /* Re-enable PXP-IOCTLs */ + if (pxp->ctx.global_state_in_suspend) { + ret = intel_pxp_arb_terminate_session(pxp); + if (ret) { + drm_err(>->i915->drm, "Failed to terminate the arb session\n"); + goto end; + } + + pxp->ctx.global_state_in_suspend = false; + } + +end: + mutex_unlock(&pxp->ctx.mutex); + + return ret; +} 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..135bfb59aaf7 --- /dev/null +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.h @@ -0,0 +1,31 @@ +/* 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_pm_prepare_suspend(struct intel_pxp *pxp); + +void intel_pxp_pm_resume_early(struct intel_pxp *pxp); +int intel_pxp_pm_resume(struct intel_pxp *pxp); +#else +static inline void intel_pxp_pm_prepare_suspend(struct intel_pxp *pxp) +{ +} + +static inline void intel_pxp_pm_resume_early(struct intel_pxp *pxp) +{ +} + +static inline int intel_pxp_pm_resume(struct intel_pxp *pxp) +{ + return 0; +} +#endif + +#endif /* __INTEL_PXP_PM_H__ */ diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h index 0b140aaeb6ed..13d7b502b7f9 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h @@ -44,6 +44,7 @@ struct pxp_context { bool flag_display_hm_surface_keys; bool global_state_attacked; + bool global_state_in_suspend; }; struct intel_pxp {
During the power event S3+ sleep/resume, hardware will lose all the encryption keys for every hardware session, even though the software session state was marked as alive after resume. So to handle such case, PXP should terminate all the hardware sessions and cleanup all the software states after the power cycle. Signed-off-by: Huang, Sean Z <sean.z.huang@intel.com> --- drivers/gpu/drm/i915/Makefile | 1 + drivers/gpu/drm/i915/gt/intel_gt_pm.c | 4 ++ drivers/gpu/drm/i915/i915_drv.c | 4 ++ drivers/gpu/drm/i915/pxp/intel_pxp_pm.c | 65 ++++++++++++++++++++++ drivers/gpu/drm/i915/pxp/intel_pxp_pm.h | 31 +++++++++++ drivers/gpu/drm/i915/pxp/intel_pxp_types.h | 1 + 6 files changed, 106 insertions(+) create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_pm.c create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_pm.h