| Message ID | 20210525054803.7387-13-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Introduce Intel PXP | expand |
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
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 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
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
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); + 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__ */
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(-)