[Intel-gfx] [PATCH v2 09/16] drm/i915/pxp: Implement PXP irq handler
Daniele Ceraolo Spurio
daniele.ceraolospurio at intel.com
Mon Mar 8 18:49:42 UTC 2021
On 3/3/2021 1:18 PM, Chris Wilson wrote:
> Quoting Daniele Ceraolo Spurio (2021-03-01 19:31:53)
>> From: "Huang, Sean Z" <sean.z.huang at intel.com>
>>
>> The HW will generate a teardown interrupt when session termination is
>> required, which requires i915 to submit a terminating batch. Once the HW
>> is done with the termination it will generate another interrupt, at
>> which point it is safe to re-create the session.
>>
>> v2: use struct completion instead of bool (Chris)
>>
>> Signed-off-by: Huang, Sean Z <sean.z.huang at intel.com>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> ---
>> drivers/gpu/drm/i915/Makefile | 1 +
>> drivers/gpu/drm/i915/gt/intel_gt_irq.c | 7 +
>> drivers/gpu/drm/i915/i915_reg.h | 1 +
>> drivers/gpu/drm/i915/pxp/intel_pxp.c | 34 +++++
>> drivers/gpu/drm/i915/pxp/intel_pxp.h | 16 ++
>> drivers/gpu/drm/i915/pxp/intel_pxp_irq.c | 151 +++++++++++++++++++
>> drivers/gpu/drm/i915/pxp/intel_pxp_irq.h | 33 ++++
>> drivers/gpu/drm/i915/pxp/intel_pxp_session.c | 9 +-
>> drivers/gpu/drm/i915/pxp/intel_pxp_session.h | 1 +
>> drivers/gpu/drm/i915/pxp/intel_pxp_tee.c | 10 +-
>> drivers/gpu/drm/i915/pxp/intel_pxp_types.h | 8 +
>> 11 files changed, 268 insertions(+), 3 deletions(-)
>> create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
>> create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_irq.h
>>
>> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
>> index 8b605f326039..5e9bd34dec38 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -274,6 +274,7 @@ i915-y += i915_perf.o
>> i915-$(CONFIG_DRM_I915_PXP) += \
>> pxp/intel_pxp.o \
>> pxp/intel_pxp_cmd.o \
>> + pxp/intel_pxp_irq.o \
>> pxp/intel_pxp_session.o \
>> pxp/intel_pxp_tee.o
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
>> index d29126c458ba..0d3585efe2b8 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
>> @@ -13,6 +13,7 @@
>> #include "intel_lrc_reg.h"
>> #include "intel_uncore.h"
>> #include "intel_rps.h"
>> +#include "pxp/intel_pxp_irq.h"
>>
>> static void guc_irq_handler(struct intel_guc *guc, u16 iir)
>> {
>> @@ -64,6 +65,9 @@ gen11_other_irq_handler(struct intel_gt *gt, const u8 instance,
>> if (instance == OTHER_GTPM_INSTANCE)
>> return gen11_rps_irq_handler(>->rps, iir);
>>
>> + if (instance == OTHER_KCR_INSTANCE)
>> + return intel_pxp_irq_handler(>->pxp, iir);
>> +
>> WARN_ONCE(1, "unhandled other interrupt instance=0x%x, iir=0x%x\n",
>> instance, iir);
>> }
>> @@ -190,6 +194,9 @@ void gen11_gt_irq_reset(struct intel_gt *gt)
>> intel_uncore_write(uncore, GEN11_GPM_WGBOXPERF_INTR_MASK, ~0);
>> intel_uncore_write(uncore, GEN11_GUC_SG_INTR_ENABLE, 0);
>> intel_uncore_write(uncore, GEN11_GUC_SG_INTR_MASK, ~0);
>> +
>> + intel_uncore_write(uncore, GEN11_CRYPTO_RSVD_INTR_ENABLE, 0);
>> + intel_uncore_write(uncore, GEN11_CRYPTO_RSVD_INTR_MASK, ~0);
>> }
>>
>> void gen11_gt_irq_postinstall(struct intel_gt *gt)
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index e5dd0203991b..97a6d0c638ec 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -7958,6 +7958,7 @@ enum {
>> /* irq instances for OTHER_CLASS */
>> #define OTHER_GUC_INSTANCE 0
>> #define OTHER_GTPM_INSTANCE 1
>> +#define OTHER_KCR_INSTANCE 4
>>
>> #define GEN11_INTR_IDENTITY_REG(x) _MMIO(0x190060 + ((x) * 4))
>>
>> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
>> index cbec9395bde9..0ca1c2c16972 100644
>> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
>> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
>> @@ -2,7 +2,9 @@
>> /*
>> * Copyright(c) 2020 Intel Corporation.
>> */
>> +#include <linux/workqueue.h>
>> #include "intel_pxp.h"
>> +#include "intel_pxp_irq.h"
>> #include "intel_pxp_tee.h"
>> #include "gt/intel_context.h"
>> #include "i915_drv.h"
>> @@ -67,12 +69,23 @@ void intel_pxp_init(struct intel_pxp *pxp)
>>
>> mutex_init(&pxp->mutex);
>>
>> + /*
>> + * we'll use the completion to check if there is a termination pending,
>> + * so we start it as completed and we reinit it when a termination
>> + * is triggered.
>> + */
>> + init_completion(&pxp->termination);
>> + complete_all(&pxp->termination);
>> +
>> kcr_pxp_enable(gt);
>>
>> ret = create_vcs_context(pxp);
>> if (ret)
>> goto out_kcr;
>>
>> + intel_pxp_irq_init(pxp);
>> + intel_pxp_irq_enable(pxp);
>> +
>> ret = intel_pxp_tee_component_init(pxp);
>> if (ret)
>> goto out_context;
>> @@ -94,10 +107,31 @@ void intel_pxp_fini(struct intel_pxp *pxp)
>> if (!intel_pxp_is_enabled(pxp))
>> return;
>>
>> + intel_pxp_irq_disable(pxp);
>> +
>> intel_pxp_tee_component_fini(pxp);
>>
>> destroy_vcs_context(pxp);
>>
>> kcr_pxp_disable(gt);
>> +}
>>
>> +int intel_pxp_wait_for_termination_completion(struct intel_pxp *pxp)
>> +{
>> + int ret;
>> +
>> + if (!intel_pxp_is_enabled(pxp))
>> + return 0;
>> +
>> + ret = wait_for_completion_timeout(&pxp->termination,
>> + msecs_to_jiffies(100));
>> +
>> + /* the wait returns 0 on failure */
>> + if (ret)
>> + ret = 0;
>> + else
>> + ret = -ETIMEDOUT;
>> +
>> + return ret;
>> }
>> +
>> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
>> index 3bede9306481..89cf66c9bef3 100644
>> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
>> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
>> @@ -9,6 +9,15 @@
>> #include "gt/intel_gt_types.h"
>> #include "intel_pxp_types.h"
>>
>> +#define GEN12_DISPLAY_PXP_STATE_TERMINATED_INTERRUPT BIT(1)
>> +#define GEN12_DISPLAY_APP_TERMINATED_PER_FW_REQ_INTERRUPT BIT(2)
>> +#define GEN12_DISPLAY_STATE_RESET_COMPLETE_INTERRUPT BIT(3)
>> +
>> +#define GEN12_PXP_INTERRUPTS \
>> + (GEN12_DISPLAY_PXP_STATE_TERMINATED_INTERRUPT | \
>> + GEN12_DISPLAY_APP_TERMINATED_PER_FW_REQ_INTERRUPT | \
>> + GEN12_DISPLAY_STATE_RESET_COMPLETE_INTERRUPT)
>> +
>> static inline struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp)
>> {
>> return container_of(pxp, struct intel_gt, pxp);
>> @@ -27,6 +36,8 @@ static inline bool intel_pxp_is_active(const struct intel_pxp *pxp)
>> #ifdef CONFIG_DRM_I915_PXP
>> void intel_pxp_init(struct intel_pxp *pxp);
>> void intel_pxp_fini(struct intel_pxp *pxp);
>> +
>> +int intel_pxp_wait_for_termination_completion(struct intel_pxp *pxp);
>> #else
>> static inline void intel_pxp_init(struct intel_pxp *pxp)
>> {
>> @@ -35,6 +46,11 @@ static inline void intel_pxp_init(struct intel_pxp *pxp)
>> static inline void intel_pxp_fini(struct intel_pxp *pxp)
>> {
>> }
>> +
>> +static inline int intel_pxp_wait_for_termination_completion(struct intel_pxp *pxp)
>> +{
>> + return 0;
>> +}
>> #endif
>>
>> #endif /* __INTEL_PXP_H__ */
>> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
>> new file mode 100644
>> index 000000000000..40115bf0b6bb
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
>> @@ -0,0 +1,151 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright(c) 2020 Intel Corporation.
>> + */
>> +#include <linux/workqueue.h>
>> +#include "intel_pxp.h"
>> +#include "intel_pxp_irq.h"
>> +#include "intel_pxp_session.h"
>> +#include "gt/intel_gt_irq.h"
>> +#include "i915_irq.h"
>> +#include "i915_reg.h"
>> +
>> +static int pxp_terminate(struct intel_pxp *pxp)
>> +{
>> + int ret = 0;
>> +
>> + mutex_lock(&pxp->mutex);
>> +
>> + pxp->global_state_attacked = true;
>> +
>> + ret = intel_pxp_arb_terminate_session_with_global_terminate(pxp);
>> +
>> + mutex_unlock(&pxp->mutex);
>> +
>> + return ret;
>> +}
>> +
>> +static int pxp_terminate_complete(struct intel_pxp *pxp)
>> +{
>> + int ret = 0;
>> +
>> + mutex_lock(&pxp->mutex);
>> +
>> + if (pxp->global_state_attacked) {
>> + pxp->global_state_attacked = false;
>> +
>> + /* Re-create the arb session after teardown handle complete */
>> + ret = intel_pxp_create_arb_session(pxp);
>> + }
> /* Re-create the arb session after teardown handle complete */
> if (fetch_and_zero(&pxp->global_state_attacked))
> ret = intel_pxp_create_arb_session(pxp);
>
>> +
>> + mutex_unlock(&pxp->mutex);
>> +
>> + complete_all(&pxp->termination);
>> +
>> + return ret;
>> +}
>> +
>> +static void intel_pxp_irq_work(struct work_struct *work)
>> +{
>> + struct intel_pxp *pxp = container_of(work, typeof(*pxp), irq_work);
>> + struct intel_gt *gt = pxp_to_gt(pxp);
>> + u32 events = 0;
>> +
>> + spin_lock_irq(>->irq_lock);
>> + events = fetch_and_zero(&pxp->current_events);
>> + spin_unlock_irq(>->irq_lock);
>> +
>> + if (!events)
>> + return;
>> +
>> + if (events & (GEN12_DISPLAY_PXP_STATE_TERMINATED_INTERRUPT |
>> + GEN12_DISPLAY_APP_TERMINATED_PER_FW_REQ_INTERRUPT))
>> + pxp_terminate(pxp);
>> +
>> + if (events & GEN12_DISPLAY_STATE_RESET_COMPLETE_INTERRUPT)
>> + pxp_terminate_complete(pxp);
>> +
>> + /*
>> + * we expect the terminate complete to arrive quickly after emitting
>> + * the terminate, so check back on it
>> + */
>> + if (pxp->irq_enabled)
>> + queue_work(system_unbound_wq, &pxp->irq_work);
> pxp->current_events is only updated in the interrupt handler, so running
> the work before the irq gains nothing.
>
>> +}
>> +
>> +/**
>> + * intel_pxp_irq_handler - Handles PXP interrupts.
>> + * @pxp: pointer to pxp struct
>> + * @iir: interrupt vector
>> + */
>> +void intel_pxp_irq_handler(struct intel_pxp *pxp, u16 iir)
>> +{
>> + struct intel_gt *gt = pxp_to_gt(pxp);
>> +
>> + if (GEM_WARN_ON(!intel_pxp_is_enabled(pxp)))
>> + return;
>> +
>> + lockdep_assert_held(>->irq_lock);
>> +
>> + if (unlikely(!iir))
>> + return;
>> +
>> + /* immediately mark PXP as inactive on termination */
>> + if (iir & (GEN12_DISPLAY_PXP_STATE_TERMINATED_INTERRUPT |
>> + GEN12_DISPLAY_APP_TERMINATED_PER_FW_REQ_INTERRUPT))
>> + intel_pxp_mark_termination_in_progress(pxp);
>> +
>> + pxp->current_events |= iir;
>> + queue_work(system_unbound_wq, &pxp->irq_work);
>> +}
>> +
>> +static inline void __pxp_set_interrupts(struct intel_gt *gt, u32 interrupts)
>> +{
>> + struct intel_uncore *uncore = gt->uncore;
>> + const u32 mask = interrupts << 16;
>> +
>> + intel_uncore_write(uncore, GEN11_CRYPTO_RSVD_INTR_ENABLE, mask);
>> + intel_uncore_write(uncore, GEN11_CRYPTO_RSVD_INTR_MASK, ~mask);
>> +}
>> +
>> +static inline void pxp_irq_reset(struct intel_gt *gt)
>> +{
>> + spin_lock_irq(>->irq_lock);
>> + gen11_gt_reset_one_iir(gt, 0, GEN11_KCR);
>> + spin_unlock_irq(>->irq_lock);
>> +}
>> +
>> +void intel_pxp_irq_init(struct intel_pxp *pxp)
>> +{
>> + INIT_WORK(&pxp->irq_work, intel_pxp_irq_work);
>> +}
>> +
>> +void intel_pxp_irq_enable(struct intel_pxp *pxp)
>> +{
>> + struct intel_gt *gt = pxp_to_gt(pxp);
>> +
>> + spin_lock_irq(>->irq_lock);
>> + if (!pxp->irq_enabled) {
>> + WARN_ON_ONCE(gen11_gt_reset_one_iir(gt, 0, GEN11_KCR));
>> + __pxp_set_interrupts(gt, GEN12_PXP_INTERRUPTS);
>> + pxp->irq_enabled = true;
>> + }
>> + spin_unlock_irq(>->irq_lock);
>> +}
>> +
>> +void intel_pxp_irq_disable(struct intel_pxp *pxp)
>> +{
>> + struct intel_gt *gt = pxp_to_gt(pxp);
>> +
>> + spin_lock_irq(>->irq_lock);
>> +
>> + pxp->irq_enabled = false;
>> + __pxp_set_interrupts(gt, 0);
>> +
>> + spin_unlock_irq(>->irq_lock);
>> + intel_synchronize_irq(gt->i915);
>> +
>> + pxp_irq_reset(gt);
>> +
>> + flush_work(&pxp->irq_work);
> As I read it, if the session was in play at the time of irq_disable and
> there were inflight interrupts then the state of the session at the end
> of this function is undefined.
>
> Should the session be terminated prior to disabling irq (that would seem
> appropriate for the driver flow)? Certainly at the point of
> unregistering the driver from userspace, all user sessions should cease.
>
> Is an assert like GEM_BUG_ON(!completion_done(&pxp->termination)); valid
> here?
I wanted to avoid doing a termination here because we have to do it
anyway when we re-enable. I've checked with the archs and basically the
HW state is undefined on resume (even the in_play register might be out
of sync with actual HW state), so that termination is mandatory. I can
add a check to make sure PXP is not considered active by the driver when
we disable the interrupts, to make sure we're in a flow that will submit
a termination to re-activate later.
Daniele
> -Chris
More information about the Intel-gfx
mailing list