From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id EEDDEC433DB for ; Mon, 8 Mar 2021 18:49:51 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 84A9D6526C for ; Mon, 8 Mar 2021 18:49:51 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 84A9D6526C Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=intel-gfx-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 1B6D689BA5; Mon, 8 Mar 2021 18:49:51 +0000 (UTC) Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by gabe.freedesktop.org (Postfix) with ESMTPS id 5175C89BA5 for ; Mon, 8 Mar 2021 18:49:50 +0000 (UTC) IronPort-SDR: 8CrEL6aqqk0lsEFdsnjZFZ4tlGrDQWBhIQPqgcNs5c98EzfGyVfC50nOnYhjqIga1Hv57CVjGe BOSHRLqOYCEQ== X-IronPort-AV: E=McAfee;i="6000,8403,9917"; a="175699151" X-IronPort-AV: E=Sophos;i="5.81,233,1610438400"; d="scan'208";a="175699151" Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Mar 2021 10:49:48 -0800 IronPort-SDR: LU9eExS7kLi9DlTjhU8s/MqhDt9DljHP5bgbH3H2wCM1M1HSA93VIDWoKv+kZsiiCVVf3fI1Q3 iQu7qyHOzfhA== X-IronPort-AV: E=Sophos;i="5.81,233,1610438400"; d="scan'208";a="447223792" Received: from dceraolo-mobl.amr.corp.intel.com (HELO [10.212.237.60]) ([10.212.237.60]) by orsmga001-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Mar 2021 10:49:45 -0800 To: Chris Wilson , intel-gfx@lists.freedesktop.org References: <20210301193200.1369-1-daniele.ceraolospurio@intel.com> <20210301193200.1369-10-daniele.ceraolospurio@intel.com> <161480629555.25897.8434912809854085938@build.alporthouse.com> From: Daniele Ceraolo Spurio Message-ID: Date: Mon, 8 Mar 2021 10:49:42 -0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 MIME-Version: 1.0 In-Reply-To: <161480629555.25897.8434912809854085938@build.alporthouse.com> Content-Language: en-US Subject: Re: [Intel-gfx] [PATCH v2 09/16] drm/i915/pxp: Implement PXP irq handler X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "Huang, Sean Z" Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On 3/3/2021 1:18 PM, Chris Wilson wrote: > Quoting Daniele Ceraolo Spurio (2021-03-01 19:31:53) >> From: "Huang, Sean Z" >> >> 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 >> Signed-off-by: Daniele Ceraolo Spurio >> Cc: Chris Wilson >> --- >> 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 >> #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 >> +#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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx