From: Rodrigo Vivi <rodrigo.vivi@intel.com>
To: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH v4 12/17] drm/i915/pxp: start the arb session on demand
Date: Wed, 2 Jun 2021 14:14:00 -0400 [thread overview]
Message-ID: <YLfKaAkZaOP2I1ch@intel.com> (raw)
In-Reply-To: <20210525054803.7387-13-daniele.ceraolospurio@intel.com>
On Mon, May 24, 2021 at 10:47:58PM -0700, Daniele Ceraolo Spurio wrote:
> Now that we can handle destruction and re-creation of the arb session,
> we can postpone the start of the session to the first submission that
> requires it, to avoid keeping it running with no user.
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
> .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 8 ++--
> drivers/gpu/drm/i915/pxp/intel_pxp.c | 37 ++++++++++++-------
> drivers/gpu/drm/i915/pxp/intel_pxp.h | 4 +-
> drivers/gpu/drm/i915/pxp/intel_pxp_irq.c | 2 +-
> drivers/gpu/drm/i915/pxp/intel_pxp_session.c | 6 +--
> drivers/gpu/drm/i915/pxp/intel_pxp_tee.c | 10 +----
> drivers/gpu/drm/i915/pxp/intel_pxp_types.h | 3 ++
> 7 files changed, 39 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index a11e9d5767bf..c08e28847064 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -2948,9 +2948,11 @@ eb_select_engine(struct i915_execbuffer *eb)
> intel_gt_pm_get(ce->engine->gt);
>
> if (i915_gem_context_uses_protected_content(eb->gem_context)) {
> - err = intel_pxp_wait_for_arb_start(&ce->engine->gt->pxp);
> - if (err)
> - goto err;
> + if (!intel_pxp_is_active(&ce->engine->gt->pxp)) {
> + err = intel_pxp_start(&ce->engine->gt->pxp);
> + if (err)
> + goto err;
> + }
>
> if (i915_gem_context_invalidated(eb->gem_context)) {
> err = -EACCES;
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> index f713d3423cea..2291c68fd3a0 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> @@ -77,6 +77,7 @@ void intel_pxp_init(struct intel_pxp *pxp)
> init_completion(&pxp->termination);
> complete_all(&pxp->termination);
>
> + mutex_init(&pxp->arb_mutex);
> INIT_WORK(&pxp->session_work, intel_pxp_session_work);
>
> ret = create_vcs_context(pxp);
> @@ -113,7 +114,7 @@ void intel_pxp_mark_termination_in_progress(struct intel_pxp *pxp)
> reinit_completion(&pxp->termination);
> }
>
> -static void intel_pxp_queue_termination(struct intel_pxp *pxp)
> +static void pxp_queue_termination(struct intel_pxp *pxp)
> {
> struct intel_gt *gt = pxp_to_gt(pxp);
>
> @@ -132,31 +133,41 @@ static void intel_pxp_queue_termination(struct intel_pxp *pxp)
> * the arb session is restarted from the irq work when we receive the
> * termination completion interrupt
> */
> -int intel_pxp_wait_for_arb_start(struct intel_pxp *pxp)
> +int intel_pxp_start(struct intel_pxp *pxp)
> {
> + int ret = 0;
> +
> if (!intel_pxp_is_enabled(pxp))
> - return 0;
> + return -ENODEV;
> +
> + mutex_lock(&pxp->arb_mutex);
> +
> + if (pxp->arb_is_valid)
> + goto unlock;
> +
> + pxp_queue_termination(pxp);
>
> if (!wait_for_completion_timeout(&pxp->termination,
> - msecs_to_jiffies(100)))
> - return -ETIMEDOUT;
> + msecs_to_jiffies(100))) {
> + ret = -ETIMEDOUT;
> + goto unlock;
> + }
> +
> + /* make sure the compiler doesn't optimize the double access */
> + barrier();
>
> if (!pxp->arb_is_valid)
> - return -EIO;
> + ret = -EIO;
>
> - return 0;
> +unlock:
> + mutex_unlock(&pxp->arb_mutex);
> + return ret;
> }
>
> void intel_pxp_init_hw(struct intel_pxp *pxp)
> {
> kcr_pxp_enable(pxp_to_gt(pxp));
> intel_pxp_irq_enable(pxp);
> -
> - /*
> - * the session could've been attacked while we weren't loaded, so
> - * handle it as if it was and re-create it.
> - */
> - intel_pxp_queue_termination(pxp);
> }
>
> void intel_pxp_fini_hw(struct intel_pxp *pxp)
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> index 91c1a2056309..1f9871e64096 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> @@ -32,7 +32,7 @@ void intel_pxp_init_hw(struct intel_pxp *pxp);
> void intel_pxp_fini_hw(struct intel_pxp *pxp);
>
> void intel_pxp_mark_termination_in_progress(struct intel_pxp *pxp);
> -int intel_pxp_wait_for_arb_start(struct intel_pxp *pxp);
> +int intel_pxp_start(struct intel_pxp *pxp);
> void intel_pxp_invalidate(struct intel_pxp *pxp);
> #else
> static inline void intel_pxp_init(struct intel_pxp *pxp)
> @@ -43,7 +43,7 @@ static inline void intel_pxp_fini(struct intel_pxp *pxp)
> {
> }
>
> -static inline int intel_pxp_wait_for_arb_start(struct intel_pxp *pxp)
> +static inline int intel_pxp_start(struct intel_pxp *pxp)
> {
> return 0;
> }
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
> index 196449243515..a230d0034e50 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
> @@ -31,7 +31,7 @@ void intel_pxp_irq_handler(struct intel_pxp *pxp, u16 iir)
> GEN12_DISPLAY_APP_TERMINATED_PER_FW_REQ_INTERRUPT)) {
> /* immediately mark PXP as inactive on termination */
> intel_pxp_mark_termination_in_progress(pxp);
> - pxp->session_events |= PXP_TERMINATION_REQUEST;
> + pxp->session_events |= PXP_TERMINATION_REQUEST | PXP_INVAL_REQUIRED;
> }
>
> if (iir & GEN12_DISPLAY_STATE_RESET_COMPLETE_INTERRUPT)
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> index e9fe757e368a..c21620916710 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> @@ -85,9 +85,6 @@ static int pxp_terminate_arb_session_and_global(struct intel_pxp *pxp)
> /* must mark termination in progress calling this function */
> GEM_WARN_ON(pxp->arb_is_valid);
>
> - /* invalidate protected objects */
> - intel_pxp_invalidate(pxp);
> -
> /* terminate the hw sessions */
> ret = intel_pxp_terminate_session(pxp, ARB_SESSION);
> if (ret) {
> @@ -144,6 +141,9 @@ void intel_pxp_session_work(struct work_struct *work)
> if (!events)
> return;
>
> + if (events & PXP_INVAL_REQUIRED)
> + intel_pxp_invalidate(pxp);
> +
doesn't this invalidation change deserves a separated patch?
I'm not sure if I understood why we need this change...
> if (events & PXP_TERMINATION_REQUEST) {
> events &= ~PXP_TERMINATION_COMPLETE;
> pxp_terminate(pxp);
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> index e3d178c44e51..35b3fed4ca2f 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> @@ -63,23 +63,15 @@ 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);
> - int ret;
>
> pxp->pxp_component = data;
> pxp->pxp_component->tee_dev = tee_kdev;
>
> /* the component is required to fully start the PXP HW */
> intel_pxp_init_hw(pxp);
> - ret = intel_pxp_wait_for_arb_start(pxp);
> - if (ret) {
> - drm_err(&i915->drm, "Failed to create arb session during bind\n");
> - intel_pxp_fini_hw(pxp);
> - pxp->pxp_component = NULL;
> - }
>
> - return ret;
> + return 0;
> }
>
> static void i915_pxp_tee_component_unbind(struct device *i915_kdev,
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> index c059a17cbcfe..b3ae49dd73a8 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> @@ -7,6 +7,7 @@
> #define __INTEL_PXP_TYPES_H__
>
> #include <linux/completion.h>
> +#include <linux/mutex.h>
> #include <linux/types.h>
> #include <linux/workqueue.h>
>
> @@ -23,6 +24,7 @@ struct intel_pxp {
> * even if the keys are gone, so we can't rely on the HW state of the
> * session to know if it's valid and need to track the status in SW.
> */
> + struct mutex arb_mutex; /* protects arb session start */
> bool arb_is_valid;
>
> /*
> @@ -40,6 +42,7 @@ struct intel_pxp {
> u32 session_events; /* protected with gt->irq_lock */
> #define PXP_TERMINATION_REQUEST BIT(0)
> #define PXP_TERMINATION_COMPLETE BIT(1)
> +#define PXP_INVAL_REQUIRED BIT(2)
> };
>
> #endif /* __INTEL_PXP_TYPES_H__ */
> --
> 2.29.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2021-06-02 18:14 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-25 5:47 [Intel-gfx] [PATCH v4 00/17] drm/i915: Introduce Intel PXP Daniele Ceraolo Spurio
2021-05-25 5:47 ` [Intel-gfx] [PATCH v4 01/17] drm/i915/pxp: Define PXP component interface Daniele Ceraolo Spurio
2021-05-25 5:47 ` [Intel-gfx] [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 ` [Intel-gfx] [PATCH v4 03/17] drm/i915/pxp: define PXP device flag and kconfig Daniele Ceraolo Spurio
2021-05-25 5:47 ` [Intel-gfx] [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 ` [Intel-gfx] [PATCH v4 05/17] drm/i915/pxp: allocate a vcs context for pxp usage Daniele Ceraolo Spurio
2021-06-01 20:24 ` Rodrigo Vivi
2021-05-25 5:47 ` [Intel-gfx] [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 ` Teres Alexis, Alan Previn
2021-05-25 5:47 ` [Intel-gfx] [PATCH v4 07/17] drm/i915/pxp: set KCR reg init Daniele Ceraolo Spurio
2021-05-25 5:47 ` [Intel-gfx] [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 ` [Intel-gfx] [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 ` [Intel-gfx] [PATCH v4 10/17] drm/i915/pxp: Implement PXP irq handler Daniele Ceraolo Spurio
2021-06-02 16:06 ` Rodrigo Vivi
2021-06-02 16:08 ` Rodrigo Vivi
2021-05-25 5:47 ` [Intel-gfx] [PATCH v4 11/17] drm/i915/pxp: interface for marking contexts as using protected content Daniele Ceraolo Spurio
2021-05-27 10:10 ` Daniel Vetter
2021-05-25 5:47 ` [Intel-gfx] [PATCH v4 12/17] drm/i915/pxp: start the arb session on demand Daniele Ceraolo Spurio
2021-06-02 18:14 ` Rodrigo Vivi [this message]
2021-06-10 22:44 ` Daniele Ceraolo Spurio
2021-06-11 8:38 ` Rodrigo Vivi
2021-05-25 5:47 ` [Intel-gfx] [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
2021-06-11 8:44 ` Rodrigo Vivi
2021-05-25 5:48 ` [Intel-gfx] [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 ` Tang, CQ
2021-05-27 2:13 ` Daniele Ceraolo Spurio
2021-05-25 5:48 ` [Intel-gfx] [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 ` [Intel-gfx] [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 ` [Intel-gfx] [PATCH v4 17/17] drm/i915/pxp: enable PXP for integrated Gen12 Daniele Ceraolo Spurio
2021-05-25 6:18 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Introduce Intel PXP Patchwork
2021-05-25 6:20 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2021-05-25 6:23 ` [Intel-gfx] ✗ Fi.CI.DOCS: " Patchwork
2021-05-25 6:42 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2021-05-25 8:34 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
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=YLfKaAkZaOP2I1ch@intel.com \
--to=rodrigo.vivi@intel.com \
--cc=daniele.ceraolospurio@intel.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
/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