Quoting Daniele Ceraolo Spurio (2021-03-01 19:31:53)
> From: "Huang, Sean Z" <sean.z.hu...@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.hu...@intel.com>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospu...@intel.com>
> Cc: Chris Wilson <ch...@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?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
