From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
To: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Huang@freedesktop.org, intel-gfx@lists.freedesktop.org, "Huang,
Sean Z" <sean.z.huang@intel.com>,
dri-devel@lists.freedesktop.org,
Chris Wilson <chris@chris-wilson.co.uk>
Subject: Re: [PATCH v4 13/17] drm/i915/pxp: Enable PXP power management
Date: Thu, 10 Jun 2021 15:58:13 -0700 [thread overview]
Message-ID: <6e2a064a-a218-2915-00f2-d453fad3f0c0@intel.com> (raw)
In-Reply-To: <YLev6vAt4gEHAeFj@intel.com>
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
>>
next prev parent reply other threads:[~2021-06-10 22:58 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-25 5:47 [PATCH v4 00/17] drm/i915: Introduce Intel PXP Daniele Ceraolo Spurio
2021-05-25 5:47 ` [PATCH v4 01/17] drm/i915/pxp: Define PXP component interface Daniele Ceraolo Spurio
2021-05-25 5:47 ` [PATCH v4 02/17] mei: pxp: export pavp client to me client bus Daniele Ceraolo Spurio
2021-06-02 19:10 ` Rodrigo Vivi
2021-05-25 5:47 ` [PATCH v4 03/17] drm/i915/pxp: define PXP device flag and kconfig Daniele Ceraolo Spurio
2021-05-25 5:47 ` [PATCH v4 04/17] drm/i915/gt: Export the pinned context constructor and destructor Daniele Ceraolo Spurio
2021-06-01 20:20 ` Rodrigo Vivi
2021-06-01 21:23 ` Daniele Ceraolo Spurio
2021-06-02 18:18 ` Rodrigo Vivi
2021-05-25 5:47 ` [PATCH v4 05/17] drm/i915/pxp: allocate a vcs context for pxp usage Daniele Ceraolo Spurio
2021-06-01 20:24 ` [Intel-gfx] " Rodrigo Vivi
2021-05-25 5:47 ` [PATCH v4 06/17] drm/i915/pxp: Implement funcs to create the TEE channel Daniele Ceraolo Spurio
2021-06-01 20:26 ` Rodrigo Vivi
2021-06-03 0:07 ` [Intel-gfx] " Teres Alexis, Alan Previn
2021-05-25 5:47 ` [PATCH v4 07/17] drm/i915/pxp: set KCR reg init Daniele Ceraolo Spurio
2021-05-25 5:47 ` [PATCH v4 08/17] drm/i915/pxp: Create the arbitrary session after boot Daniele Ceraolo Spurio
2021-06-01 20:32 ` Rodrigo Vivi
2021-05-25 5:47 ` [PATCH v4 09/17] drm/i915/pxp: Implement arb session teardown Daniele Ceraolo Spurio
2021-05-25 20:24 ` kernel test robot
2021-05-25 5:47 ` [PATCH v4 10/17] drm/i915/pxp: Implement PXP irq handler Daniele Ceraolo Spurio
2021-06-02 16:06 ` [Intel-gfx] " Rodrigo Vivi
2021-06-02 16:08 ` Rodrigo Vivi
2021-05-25 5:47 ` [PATCH v4 11/17] drm/i915/pxp: interface for marking contexts as using protected content Daniele Ceraolo Spurio
2021-05-27 10:10 ` [Intel-gfx] " Daniel Vetter
2021-05-25 5:47 ` [PATCH v4 12/17] drm/i915/pxp: start the arb session on demand Daniele Ceraolo Spurio
2021-06-02 18:14 ` [Intel-gfx] " Rodrigo Vivi
2021-06-10 22:44 ` Daniele Ceraolo Spurio
2021-06-11 8:38 ` Rodrigo Vivi
2021-05-25 5:47 ` [PATCH v4 13/17] drm/i915/pxp: Enable PXP power management Daniele Ceraolo Spurio
2021-06-02 16:20 ` Rodrigo Vivi
2021-06-10 22:58 ` Daniele Ceraolo Spurio [this message]
2021-06-11 8:44 ` Rodrigo Vivi
2021-05-25 5:48 ` [PATCH v4 14/17] drm/i915/pxp: User interface for Protected buffer Daniele Ceraolo Spurio
2021-05-25 13:32 ` Daniel Vetter
2021-05-27 2:03 ` Daniele Ceraolo Spurio
2021-05-25 18:36 ` [Intel-gfx] " Tang, CQ
2021-05-27 2:13 ` Daniele Ceraolo Spurio
2021-05-25 5:48 ` [PATCH v4 15/17] drm/i915/pxp: Add plane decryption support Daniele Ceraolo Spurio
2021-06-02 18:23 ` Rodrigo Vivi
2021-05-25 5:48 ` [PATCH v4 16/17] drm/i915/pxp: black pixels on pxp disabled Daniele Ceraolo Spurio
2021-06-02 19:00 ` Rodrigo Vivi
2021-05-25 5:48 ` [PATCH v4 17/17] drm/i915/pxp: enable PXP for integrated Gen12 Daniele Ceraolo Spurio
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6e2a064a-a218-2915-00f2-d453fad3f0c0@intel.com \
--to=daniele.ceraolospurio@intel.com \
--cc=Huang@freedesktop.org \
--cc=chris@chris-wilson.co.uk \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=rodrigo.vivi@intel.com \
--cc=sean.z.huang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).