Re: [PATCH v2 05/12] drm/xe/pxp: Handle the PXP termination interrupt

[John Harrison <john.c.harrison@intel.com>]

On 11/6/2024 16:33, Daniele Ceraolo Spurio wrote:
> On 10/7/24 17:34, John Harrison wrote:
>> On 8/16/2024 12:00, Daniele Ceraolo Spurio wrote:
>>> When something happen to the session, the HW generates a termination
>>> interrupt. In reply to this, the driver is required to submit an inline
>>> session termination via the VCS, trigger the global termination and
>>> notify the GSC FW that the session is now invalid.
>>>
>>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>> ---
>>>   drivers/gpu/drm/xe/regs/xe_gt_regs.h  |   8 ++
>>>   drivers/gpu/drm/xe/regs/xe_pxp_regs.h |   6 ++
>>>   drivers/gpu/drm/xe/xe_irq.c           |  20 +++-
>>>   drivers/gpu/drm/xe/xe_pxp.c           | 138 
>>> +++++++++++++++++++++++++-
>>>   drivers/gpu/drm/xe/xe_pxp.h           |   3 +
>>>   drivers/gpu/drm/xe/xe_pxp_types.h     |  13 +++
>>>   6 files changed, 184 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/regs/xe_gt_regs.h 
>>> b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
>>> index 0d1a4a9f4e11..9e9c20f1f1f4 100644
>>> --- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
>>> +++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
>>> @@ -570,6 +570,7 @@
>>>   #define   ENGINE1_MASK                REG_GENMASK(31, 16)
>>>   #define   ENGINE0_MASK                REG_GENMASK(15, 0)
>>>   #define GPM_WGBOXPERF_INTR_ENABLE        XE_REG(0x19003c, 
>>> XE_REG_OPTION_VF)
>>> +#define CRYPTO_RSVD_INTR_ENABLE            XE_REG(0x190040)
>>>   #define GUNIT_GSC_INTR_ENABLE            XE_REG(0x190044, 
>>> XE_REG_OPTION_VF)
>>>   #define CCS_RSVD_INTR_ENABLE            XE_REG(0x190048, 
>>> XE_REG_OPTION_VF)
>>>   @@ -580,6 +581,7 @@
>>>   #define   INTR_ENGINE_INTR(x) REG_FIELD_GET(GENMASK(15, 0), x)
>>>   #define   OTHER_GUC_INSTANCE            0
>>>   #define   OTHER_GSC_HECI2_INSTANCE        3
>>> +#define   OTHER_KCR_INSTANCE            4
>>>   #define   OTHER_GSC_INSTANCE            6
>>>     #define IIR_REG_SELECTOR(x)            XE_REG(0x190070 + ((x) * 
>>> 4), XE_REG_OPTION_VF)
>>> @@ -591,6 +593,7 @@
>>>   #define HECI2_RSVD_INTR_MASK            XE_REG(0x1900e4)
>>>   #define GUC_SG_INTR_MASK            XE_REG(0x1900e8, 
>>> XE_REG_OPTION_VF)
>>>   #define GPM_WGBOXPERF_INTR_MASK            XE_REG(0x1900ec, 
>>> XE_REG_OPTION_VF)
>>> +#define CRYPTO_RSVD_INTR_MASK            XE_REG(0x1900f0)
>>>   #define GUNIT_GSC_INTR_MASK            XE_REG(0x1900f4, 
>>> XE_REG_OPTION_VF)
>>>   #define CCS0_CCS1_INTR_MASK            XE_REG(0x190100)
>>>   #define CCS2_CCS3_INTR_MASK            XE_REG(0x190104)
>>> @@ -605,4 +608,9 @@
>>>   #define   GT_CS_MASTER_ERROR_INTERRUPT        REG_BIT(3)
>>>   #define   GT_RENDER_USER_INTERRUPT        REG_BIT(0)
>>>   +/* irqs for OTHER_KCR_INSTANCE */
>>> +#define   KCR_PXP_STATE_TERMINATED_INTERRUPT REG_BIT(1)
>>> +#define   KCR_APP_TERMINATED_PER_FW_REQ_INTERRUPT REG_BIT(2)
>>> +#define   KCR_PXP_STATE_RESET_COMPLETE_INTERRUPT REG_BIT(3)
>>> +
>>>   #endif
>>> diff --git a/drivers/gpu/drm/xe/regs/xe_pxp_regs.h 
>>> b/drivers/gpu/drm/xe/regs/xe_pxp_regs.h
>>> index d67cf210d23d..aa158938b42e 100644
>>> --- a/drivers/gpu/drm/xe/regs/xe_pxp_regs.h
>>> +++ b/drivers/gpu/drm/xe/regs/xe_pxp_regs.h
>>> @@ -14,4 +14,10 @@
>>>   #define KCR_INIT                XE_REG(0x3860f0)
>>>   #define   KCR_INIT_ALLOW_DISPLAY_ME_WRITES    REG_BIT(14)
>>>   +/* KCR hwdrm session in play status 0-31 */
>>> +#define KCR_SIP                    XE_REG(0x386260)
>>> +
>>> +/* PXP global terminate register for session termination */
>>> +#define KCR_GLOBAL_TERMINATE            XE_REG(0x3860f8)
>>> +
>>>   #endif /* __XE_PXP_REGS_H__ */
>>> diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
>>> index 5f2c368c35ad..f11d9a740627 100644
>>> --- a/drivers/gpu/drm/xe/xe_irq.c
>>> +++ b/drivers/gpu/drm/xe/xe_irq.c
>>> @@ -20,6 +20,7 @@
>>>   #include "xe_hw_engine.h"
>>>   #include "xe_memirq.h"
>>>   #include "xe_mmio.h"
>>> +#include "xe_pxp.h"
>>>   #include "xe_sriov.h"
>>>     /*
>>> @@ -202,6 +203,15 @@ void xe_irq_enable_hwe(struct xe_gt *gt)
>>>           }
>>>           if (heci_mask)
>>>               xe_mmio_write32(gt, HECI2_RSVD_INTR_MASK, ~(heci_mask 
>>> << 16));
>>> +
>>> +        if (xe_pxp_is_supported(xe)) {
>>> +            u32 kcr_mask = KCR_PXP_STATE_TERMINATED_INTERRUPT |
>>> + KCR_APP_TERMINATED_PER_FW_REQ_INTERRUPT |
>>> + KCR_PXP_STATE_RESET_COMPLETE_INTERRUPT;
>>> +
>>> +            xe_mmio_write32(gt, CRYPTO_RSVD_INTR_ENABLE, kcr_mask 
>>> << 16);
>>> +            xe_mmio_write32(gt, CRYPTO_RSVD_INTR_MASK, ~(kcr_mask 
>>> << 16));
>>> +        }
>>>       }
>>>   }
>>>   @@ -324,9 +334,15 @@ static void gt_irq_handler(struct xe_tile *tile,
>>>               }
>>>                 if (class == XE_ENGINE_CLASS_OTHER) {
>>> -                /* HECI GSCFI interrupts come from outside of GT */
>>> +                /*
>>> +                 * HECI GSCFI interrupts come from outside of GT.
>>> +                 * KCR irqs come from inside GT but are handled
>>> +                 * by the global PXP subsystem.
>>> +                 */
>>>                   if (HAS_HECI_GSCFI(xe) && instance == 
>>> OTHER_GSC_INSTANCE)
>>>                       xe_heci_gsc_irq_handler(xe, intr_vec);
>>> +                else if (instance == OTHER_KCR_INSTANCE)
>>> +                    xe_pxp_irq_handler(xe, intr_vec);
>>>                   else
>>>                       gt_other_irq_handler(engine_gt, instance, 
>>> intr_vec);
>>>               }
>>> @@ -512,6 +528,8 @@ static void gt_irq_reset(struct xe_tile *tile)
>>>           xe_mmio_write32(mmio, GUNIT_GSC_INTR_ENABLE, 0);
>>>           xe_mmio_write32(mmio, GUNIT_GSC_INTR_MASK, ~0);
>>>           xe_mmio_write32(mmio, HECI2_RSVD_INTR_MASK, ~0);
>>> +        xe_mmio_write32(mmio, CRYPTO_RSVD_INTR_ENABLE, 0);
>>> +        xe_mmio_write32(mmio, CRYPTO_RSVD_INTR_MASK, ~0);
>>>       }
>>>         xe_mmio_write32(mmio, GPM_WGBOXPERF_INTR_ENABLE, 0);
>>> diff --git a/drivers/gpu/drm/xe/xe_pxp.c b/drivers/gpu/drm/xe/xe_pxp.c
>>> index 56bb7d927c07..382eb0cb0018 100644
>>> --- a/drivers/gpu/drm/xe/xe_pxp.c
>>> +++ b/drivers/gpu/drm/xe/xe_pxp.c
>>> @@ -12,9 +12,11 @@
>>>   #include "xe_gt.h"
>>>   #include "xe_gt_types.h"
>>>   #include "xe_mmio.h"
>>> +#include "xe_pm.h"
>>>   #include "xe_pxp_submit.h"
>>>   #include "xe_pxp_types.h"
>>>   #include "xe_uc_fw.h"
>>> +#include "regs/xe_gt_regs.h"
>>>   #include "regs/xe_pxp_regs.h"
>>>     /**
>>> @@ -25,11 +27,133 @@
>>>    * integrated parts.
>>>    */
>>>   -static bool pxp_is_supported(const struct xe_device *xe)
>>> +#define ARB_SESSION 0xF /* TODO: move to UAPI */
>>> +
>>> +bool xe_pxp_is_supported(const struct xe_device *xe)
>>>   {
>>>       return xe->info.has_pxp && 
>>> IS_ENABLED(CONFIG_INTEL_MEI_GSC_PROXY);
>>>   }
>>>   +static bool pxp_is_enabled(const struct xe_pxp *pxp)
>>> +{
>>> +    return pxp;
>>> +}
>>> +
>>> +static int pxp_wait_for_session_state(struct xe_pxp *pxp, u32 id, 
>>> bool in_play)
>>> +{
>>> +    struct xe_gt *gt = pxp->gt;
>>> +    u32 mask = BIT(id);
>>> +    int ret;
>>> +
>>> +    ret = xe_force_wake_get(gt_to_fw(gt), XE_FW_GT);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    ret = xe_mmio_wait32(gt, KCR_SIP, mask, in_play ? mask : 0,
>>> +                 250, NULL, false);
>>> +    xe_force_wake_put(gt_to_fw(gt), XE_FW_GT);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static void pxp_terminate(struct xe_pxp *pxp)
>>> +{
>>> +    int ret = 0;
>>> +    struct xe_device *xe = pxp->xe;
>>> +    struct xe_gt *gt = pxp->gt;
>>> +
>>> +    drm_dbg(&xe->drm, "Terminating PXP\n");
>>> +
>>> +    /* terminate the hw session */
>>> +    ret = xe_pxp_submit_session_termination(pxp, ARB_SESSION);
>>> +    if (ret)
>>> +        goto out;
>>> +
>>> +    ret = pxp_wait_for_session_state(pxp, ARB_SESSION, false);
>>> +    if (ret)
>>> +        goto out;
>>> +
>>> +    /* Trigger full HW cleanup */
>>> +    XE_WARN_ON(xe_force_wake_get(gt_to_fw(gt), XE_FW_GT));
>> Why WARN here but no explicit message at all if the earlier force 
>> wake fails? And is it safe to keep going if the fw did fail?
>
> The idea was that if we know that the state is good enough to 
> terminate we could try to do it even with a forcewake error, worst 
> case it doesn't work. If we don't know the state we can't attempt a 
> termination at all.
>
That needs a comment to describe the reasoning for the different behaviour.

Also, note that xe_force_wake_get() doesn't work the same any more. See 
"drm/xe/gt: Update handling of xe_force_wake_get return". It now returns 
a domain reference that must be passed back in to the put call.

John.

>
>>
>> Also, given two identical, back-to-back fw get/put sets, would it not 
>> be more efficient to have pxp_terminate do the get and share that 
>> across the two register access? It would also remove the issue with 
>> failed fw half way through causing problems due to not wanting to abort.
>
> will do.
>
>
>>
>>> +    xe_mmio_write32(gt, KCR_GLOBAL_TERMINATE, 1);
>> BSpec description for KCR_GLOBAL_TERMINATE says need to check 
>> KCR_SIP_GCD rather than KCR_SIP_MEDIA for bits 0-15 being 0. Whereas 
>> the KCR_SIP being checked above is KCR_SIP_MEDIA only.
>
> That's just the spec not being super clear. The description is common 
> between the render and the media copies of the registers, but you need 
> to check the version of the registers on the actual GT you're doing 
> the termination on.
>
>
>>
>>> +    xe_force_wake_put(gt_to_fw(gt), XE_FW_GT);
>>> +
>>> +    /* now we can tell the GSC to clean up its own state */
>>> +    ret = xe_pxp_submit_session_invalidation(&pxp->gsc_res, 
>>> ARB_SESSION);
>>> +
>>> +out:
>>> +    if (ret)
>>> +        drm_err(&xe->drm, "PXP termination failed: %pe\n", 
>>> ERR_PTR(ret));
>>> +}
>>> +
>>> +static void pxp_terminate_complete(struct xe_pxp *pxp)
>>> +{
>>> +    /* TODO mark the session as ready to start */
>>> +}
>>> +
>>> +static void pxp_irq_work(struct work_struct *work)
>>> +{
>>> +    struct xe_pxp *pxp = container_of(work, typeof(*pxp), irq.work);
>>> +    struct xe_device *xe = pxp->xe;
>>> +    u32 events = 0;
>>> +
>>> +    spin_lock_irq(&xe->irq.lock);
>>> +    events = pxp->irq.events;
>>> +    pxp->irq.events = 0;
>>> +    spin_unlock_irq(&xe->irq.lock);
>>> +
>>> +    if (!events)
>>> +        return;
>>> +
>>> +    /*
>>> +     * If we're processing a termination irq while suspending then 
>>> don't
>>> +     * bother, we're going to re-init everything on resume anyway.
>>> +     */
>>> +    if ((events & PXP_TERMINATION_REQUEST) && 
>>> !xe_pm_runtime_get_if_active(xe))
>>> +        return;
>> I assume it is not possible to have both REQUEST and COMPLETE set at 
>> the same time? I.e. is it possible for this early exit to cause a 
>> lost termination complete call?
>
> A complete is only received in response to the submission of a 
> termination request, so they should never be set at the same time. 
> Doesn't really matter anyway since we submit a termination on resume 
> anyway.
>
> Daniele
>
>
>>
>> John.
>>
>>> +
>>> +    if (events & PXP_TERMINATION_REQUEST) {
>>> +        events &= ~PXP_TERMINATION_COMPLETE;
>>> +        pxp_terminate(pxp);
>>> +    }
>>> +
>>> +    if (events & PXP_TERMINATION_COMPLETE)
>>> +        pxp_terminate_complete(pxp);
>>> +
>>> +    if (events & PXP_TERMINATION_REQUEST)
>>> +        xe_pm_runtime_put(xe);
>>> +}
>>> +
>>> +/**
>>> + * xe_pxp_irq_handler - Handles PXP interrupts.
>>> + * @pxp: pointer to pxp struct
>>> + * @iir: interrupt vector
>>> + */
>>> +void xe_pxp_irq_handler(struct xe_device *xe, u16 iir)
>>> +{
>>> +    struct xe_pxp *pxp = xe->pxp;
>>> +
>>> +    if (!pxp_is_enabled(pxp)) {
>>> +        drm_err(&xe->drm, "PXP irq 0x%x received with PXP 
>>> disabled!\n", iir);
>>> +        return;
>>> +    }
>>> +
>>> +    lockdep_assert_held(&xe->irq.lock);
>>> +
>>> +    if (unlikely(!iir))
>>> +        return;
>>> +
>>> +    if (iir & (KCR_PXP_STATE_TERMINATED_INTERRUPT |
>>> +           KCR_APP_TERMINATED_PER_FW_REQ_INTERRUPT))
>>> +        pxp->irq.events |= PXP_TERMINATION_REQUEST;
>>> +
>>> +    if (iir & KCR_PXP_STATE_RESET_COMPLETE_INTERRUPT)
>>> +        pxp->irq.events |= PXP_TERMINATION_COMPLETE;
>>> +
>>> +    if (pxp->irq.events)
>>> +        queue_work(pxp->irq.wq, &pxp->irq.work);
>>> +}
>>> +
>>>   static int kcr_pxp_set_status(const struct xe_pxp *pxp, bool enable)
>>>   {
>>>       u32 val = enable ? 
>>> _MASKED_BIT_ENABLE(KCR_INIT_ALLOW_DISPLAY_ME_WRITES) :
>>> @@ -60,6 +184,7 @@ static void pxp_fini(void *arg)
>>>   {
>>>       struct xe_pxp *pxp = arg;
>>>   +    destroy_workqueue(pxp->irq.wq);
>>>       xe_pxp_destroy_execution_resources(pxp);
>>>         /* no need to explicitly disable KCR since we're going to do 
>>> an FLR */
>>> @@ -83,7 +208,7 @@ int xe_pxp_init(struct xe_device *xe)
>>>       struct xe_pxp *pxp;
>>>       int err;
>>>   -    if (!pxp_is_supported(xe))
>>> +    if (!xe_pxp_is_supported(xe))
>>>           return -EOPNOTSUPP;
>>>         /* we only support PXP on single tile devices with a media 
>>> GT */
>>> @@ -105,12 +230,17 @@ int xe_pxp_init(struct xe_device *xe)
>>>       if (!pxp)
>>>           return -ENOMEM;
>>>   +    INIT_WORK(&pxp->irq.work, pxp_irq_work);
>>>       pxp->xe = xe;
>>>       pxp->gt = gt;
>>>   +    pxp->irq.wq = alloc_ordered_workqueue("pxp-wq", 0);
>>> +    if (!pxp->irq.wq)
>>> +        return -ENOMEM;
>>> +
>>>       err = kcr_pxp_enable(pxp);
>>>       if (err)
>>> -        return err;
>>> +        goto out_wq;
>>>         err = xe_pxp_allocate_execution_resources(pxp);
>>>       if (err)
>>> @@ -122,5 +252,7 @@ int xe_pxp_init(struct xe_device *xe)
>>>     kcr_disable:
>>>       kcr_pxp_disable(pxp);
>>> +out_wq:
>>> +    destroy_workqueue(pxp->irq.wq);
>>>       return err;
>>>   }
>>> diff --git a/drivers/gpu/drm/xe/xe_pxp.h b/drivers/gpu/drm/xe/xe_pxp.h
>>> index 79c951667f13..81bafe2714ff 100644
>>> --- a/drivers/gpu/drm/xe/xe_pxp.h
>>> +++ b/drivers/gpu/drm/xe/xe_pxp.h
>>> @@ -10,6 +10,9 @@
>>>     struct xe_device;
>>>   +bool xe_pxp_is_supported(const struct xe_device *xe);
>>> +
>>>   int xe_pxp_init(struct xe_device *xe);
>>> +void xe_pxp_irq_handler(struct xe_device *xe, u16 iir);
>>>     #endif /* __XE_PXP_H__ */
>>> diff --git a/drivers/gpu/drm/xe/xe_pxp_types.h 
>>> b/drivers/gpu/drm/xe/xe_pxp_types.h
>>> index 3463caaad101..d5cf8faed7be 100644
>>> --- a/drivers/gpu/drm/xe/xe_pxp_types.h
>>> +++ b/drivers/gpu/drm/xe/xe_pxp_types.h
>>> @@ -8,6 +8,7 @@
>>>     #include <linux/iosys-map.h>
>>>   #include <linux/types.h>
>>> +#include <linux/workqueue.h>
>>>     struct xe_bo;
>>>   struct xe_exec_queue;
>>> @@ -69,6 +70,18 @@ struct xe_pxp {
>>>         /** @gsc_exec: kernel-owned objects for PXP submissions to 
>>> the GSCCS */
>>>       struct xe_pxp_gsc_client_resources gsc_res;
>>> +
>>> +    /** @irq: wrapper for the worker and queue used for PXP irq 
>>> support */
>>> +    struct {
>>> +        /** @irq.work: worker that manages irq events. */
>>> +        struct work_struct work;
>>> +        /** @irq.wq: workqueue on which to queue the irq work. */
>>> +        struct workqueue_struct *wq;
>>> +        /** @irq.events: pending events, protected with 
>>> xe->irq.lock. */
>>> +        u32 events;
>>> +#define PXP_TERMINATION_REQUEST  BIT(0)
>>> +#define PXP_TERMINATION_COMPLETE BIT(1)
>>> +    } irq;
>>>   };
>>>     #endif /* __XE_PXP_TYPES_H__ */
>>