Re: [PATCH v2 09/16] drm/i915/pxp: Implement PXP irq handler

[Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>]

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@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > ---
> >   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(&gt->rps, iir);
> >   
> > +       if (instance == OTHER_KCR_INSTANCE)
> > +               return intel_pxp_irq_handler(&gt->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(&gt->irq_lock);
> > +       events = fetch_and_zero(&pxp->current_events);
> > +       spin_unlock_irq(&gt->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(&gt->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(&gt->irq_lock);
> > +       gen11_gt_reset_one_iir(gt, 0, GEN11_KCR);
> > +       spin_unlock_irq(&gt->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(&gt->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(&gt->irq_lock);
> > +}
> > +
> > +void intel_pxp_irq_disable(struct intel_pxp *pxp)
> > +{
> > +       struct intel_gt *gt = pxp_to_gt(pxp);
> > +
> > +       spin_lock_irq(&gt->irq_lock);
> > +
> > +       pxp->irq_enabled = false;
> > +       __pxp_set_interrupts(gt, 0);
> > +
> > +       spin_unlock_irq(&gt->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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx