Re: [Intel-gfx] [PATCH v4 12/17] drm/i915/pxp: start the arb session on demand

On Thu, Jun 10, 2021 at 03:44:37PM -0700, Daniele Ceraolo Spurio wrote:

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.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...

Before this patch, we always did a full invalidation and an arb restart

oh, indeed, I had missed that part, sorry

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.

Thanks for the explanation, it makes sense

Reviewed-by: Rodrigo Vivi rodrigo.vivi@intel.com

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