[Intel-gfx] [PATCH v4 12/17] drm/i915/pxp: start the arb session on demand
Daniele Ceraolo Spurio
daniele.ceraolospurio at intel.com
Thu Jun 10 22:44:37 UTC 2021
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 at 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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the dri-devel
mailing list