Re: [Intel-gfx] [RFC-v19 08/13] drm/i915/pxp: Enable PXP power management

["Huang, Sean Z" <sean.z.huang@intel.com>]

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(&gt->uc);
>  
> +       intel_pxp_pm_resume(&gt->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(&gt->pxp);
>         intel_uc_suspend(&gt->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(&gt->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 {


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx