On 6/2/2021 11:14 AM, Rodrigo Vivi wrote:
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.ceraolospu...@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...

Before this patch, we always did a full invalidation and an arb restart back-to-back and therefore the invalidation was implicit in the PXP_TERMINATION_REQUEST flag. This patch changes this into different cases:

termination irq: invalidate objects, submit termination and restart the arb session
suspend: invalidate objects
first pxp submission: submit termination and start the arb session

Therefore we need to be able to select the invalidation as an independent case from the termination.

Daniele



        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

Reply via email to