-----Original Message-----
From: Tvrtko Ursulin <tvrtko.ursu...@igalia.com>
Sent: Tuesday, February 18, 2025 10:39 AM
To: Simona Vetter <simona.vet...@ffwll.ch>; Cavitt, Jonathan
<jonathan.cav...@intel.com>
Cc: intel...@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Gupta,
saurabhg <saurabhg.gu...@intel.com>; Zuo, Alex <alex....@intel.com>; Lahtinen,
Joonas <joonas.lahti...@intel.com>; De Marchi, Lucas
<lucas.demar...@intel.com>; Brost, Matthew <matthew.br...@intel.com>
Subject: Re: [PATCH 3/4] FIXME: drm/xe/xe_drm_client: Add per drm client
pagefault info
>
> On 17/02/2025 17:01, Simona Vetter wrote:
> > On Fri, Feb 14, 2025 at 08:37:56PM +0000, Jonathan Cavitt wrote:
> >> Add additional information to drm client so it can report the last 50
> >> exec queues to have been banned on it, as well as the last pagefault
> >> seen when said exec queues were banned. Since we cannot reasonably
> >> associate a pagefault to a specific exec queue, we currently report the
> >> last seen pagefault on the associated hw engine instead.
> >>
> >> The last pagefault seen per exec queue is saved to the hw engine, and the
> >> pagefault is updated during the pagefault handling process in
> >> xe_gt_pagefault. The last seen pagefault is reset when the engine is
> >> reset because any future exec queue bans likely were not caused by said
> >> pagefault after the reset.
> >>
> >> v2: Remove exec queue from blame list on destroy and recreate (Joonas)
> >>
> >> Signed-off-by: Jonathan Cavitt <jonathan.cav...@intel.com>
> >> ---
> >> drivers/gpu/drm/xe/xe_drm_client.c | 128 ++++++++++++++++++++++++
> >> drivers/gpu/drm/xe/xe_drm_client.h | 36 +++++++
> >> drivers/gpu/drm/xe/xe_exec_queue.c | 7 ++
> >> drivers/gpu/drm/xe/xe_gt_pagefault.c | 19 ++++
> >> drivers/gpu/drm/xe/xe_guc_submit.c | 17 ++++
> >> drivers/gpu/drm/xe/xe_hw_engine.c | 4 +
> >> drivers/gpu/drm/xe/xe_hw_engine_types.h | 8 ++
> >> 7 files changed, 219 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/xe/xe_drm_client.c
> >> b/drivers/gpu/drm/xe/xe_drm_client.c
> >> index 2d4874d2b922..f15560d0b6ff 100644
> >> --- a/drivers/gpu/drm/xe/xe_drm_client.c
> >> +++ b/drivers/gpu/drm/xe/xe_drm_client.c
> >> @@ -17,6 +17,7 @@
> >> #include "xe_exec_queue.h"
> >> #include "xe_force_wake.h"
> >> #include "xe_gt.h"
> >> +#include "xe_gt_pagefault.h"
> >> #include "xe_hw_engine.h"
> >> #include "xe_pm.h"
> >> #include "xe_trace.h"
> >> @@ -70,6 +71,21 @@
> >> * drm-total-cycles-ccs: 7655183225
> >> * drm-engine-capacity-ccs: 4
> >> *
> >> + * - Exec queue ban list -
> >
> > This looks like you're just totally tossing the drm fdinfo format and
> > going with something no one else can even parse.
> >
> > I think it's time for some proper helper functions/macros and not
> > open-coded drm_printf for this stuff.
> >
> > Also for sure needs an ack from Tvrtko.
> >
> > Cheers, Sima
> >
> >> + *
> >> + * Exec queue 1 banned:
> >> + * Associated pagefault:
> >> + * ASID: 9
> >> + * VFID: 0
> >> + * PDATA: 0x0450
> >> + * Faulted Address: 0x000001fff86a9000
> >> + * FaultType: NOT_PRESENT
> >> + * AccessType: ACCESS_TYPE_WRITE
> >> + * FaultLevel: 0
> >> + * EngineClass: 1 vcs
> >> + * EngineInstance: 0
> >> + *
> >> + *
> >> * Possible `drm-cycles-` key names are: `rcs`, `ccs`, `bcs`, `vcs`,
> >> `vecs` and
> >> * "other".
>
> From one angle one could say the new data is not drm fdinfo compliant
> and so existing parsers will just skip over it and do nothing. Which is
> not a problem per se.
>
> So without going into any analysis of what data is exposed here and its
> usefulness maybe drm fdinfo does not care.
>
> Then from another angle, going slightly deeper into what is proposed
> here, it _seems_ to be adding some sort of logging to fdinfo which is
> way different than current single counters paradigm. I wonder if there
> is precedent anywhere in the kernel for stuffing logs into fdinfo?
>
> My first reaction would be that it feels a questionable approach (how
> will userspace correlate "Exec queue *N*" to something?). I guess more
The user is given an exec queue ID number when they call the exec queue
create ioctl, and that ID is reported back as "Exec queue *N*" here.
However, it was requested that this be converted into an xe_query query
(by Joonas), so this will no longer be reported through xe_drm_client_fdinfo.
Apologies for the confusion.
-Jonathan Cavitt
> info on how is this useful for userspace should be added to the patch
> description.
>
> Regards,
>
> Tvrtko
>
> >> */
> >> @@ -97,6 +113,8 @@ struct xe_drm_client *xe_drm_client_alloc(void)
> >> #ifdef CONFIG_PROC_FS
> >> spin_lock_init(&client->bos_lock);
> >> INIT_LIST_HEAD(&client->bos_list);
> >> + spin_lock_init(&client->blame_lock);
> >> + INIT_LIST_HEAD(&client->blame_list);
> >> #endif
> >> return client;
> >> }
> >> @@ -164,6 +182,72 @@ void xe_drm_client_remove_bo(struct xe_bo *bo)
> >> xe_drm_client_put(client);
> >> }
> >>
> >> +static void free_blame(struct blame *b)
> >> +{
> >> + list_del(&b->list);
> >> + kfree(b->pf);
> >> + kfree(b);
> >> +}
> >> +
> >> +void xe_drm_client_add_blame(struct xe_drm_client *client,
> >> + struct xe_exec_queue *q)
> >> +{
> >> + struct blame *b = NULL;
> >> + struct list_head *h;
> >> + struct pagefault *pf = NULL;
> >> + struct xe_file *xef = q->xef;
> >> + struct xe_hw_engine *hwe = q->hwe;
> >> + unsigned long count;
> >> +
> >> + b = kzalloc(sizeof(struct blame), GFP_KERNEL);
> >> + xe_assert(xef->xe, b);
> >> +
> >> + spin_lock(&client->blame_lock);
> >> + list_add_tail(&b->list, &client->blame_list);
> >> + /**
> >> + * Limit the number of blames in the blame list to prevent memory
> >> overuse.
> >> + *
> >> + * TODO: Parameterize max blame list size.
> >> + */
> >> + count = 0;
> >> + list_for_each(h, &client->blame_list)
> >> + count++;
> >> + if (count >= 50) {
> >> + struct blame *rem = list_first_entry(&client->blame_list,
> >> struct blame, list);
> >> + free_blame(rem);
> >> + }
> >> + spin_unlock(&client->blame_lock);
> >> +
> >> + /**
> >> + * Duplicate pagefault on engine to blame, if one may have caused the
> >> + * exec queue to be banned.
> >> + */
> >> + b->pf = NULL;
> >> + spin_lock(&hwe->pf.lock);
> >> + if (hwe->pf.info) {
> >> + pf = kzalloc(sizeof(struct pagefault), GFP_KERNEL);
> >> + memcpy(pf, hwe->pf.info, sizeof(struct pagefault));
> >> + }
> >> + spin_unlock(&hwe->pf.lock);
> >> +
> >> + /** Save blame data to list element */
> >> + b->exec_queue_id = q->id;
> >> + b->pf = pf;
> >> +}
> >> +
> >> +void xe_drm_client_remove_blame(struct xe_drm_client *client,
> >> + struct xe_exec_queue *q)
> >> +{
> >> + struct blame *b, *tmp;
> >> +
> >> + spin_lock(&client->blame_lock);
> >> + list_for_each_entry_safe(b, tmp, &client->blame_list, list)
> >> + if (b->exec_queue_id == q->id)
> >> + free_blame(b);
> >> + spin_unlock(&client->blame_lock);
> >> +
> >> +}
> >> +
> >> static void bo_meminfo(struct xe_bo *bo,
> >> struct drm_memory_stats stats[TTM_NUM_MEM_TYPES])
> >> {
> >> @@ -380,6 +464,49 @@ static void show_run_ticks(struct drm_printer *p,
> >> struct drm_file *file)
> >> }
> >> }
> >>
> >> +static void print_pagefault(struct drm_printer *p, struct pagefault *pf)
> >> +{
> >> + drm_printf(p, "\n\t\tASID: %d\n"
> >> + "\t\tVFID: %d\n"
> >> + "\t\tPDATA: 0x%04x\n"
> >> + "\t\tFaulted Address: 0x%08x%08x\n"
> >> + "\t\tFaultType: %s\n"
> >> + "\t\tAccessType: %s\n"
> >> + "\t\tFaultLevel: %d\n"
> >> + "\t\tEngineClass: %d %s\n"
> >> + "\t\tEngineInstance: %d\n",
> >> + pf->asid, pf->vfid, pf->pdata, upper_32_bits(pf->page_addr),
> >> + lower_32_bits(pf->page_addr),
> >> + fault_type_to_str(pf->fault_type),
> >> + access_type_to_str(pf->access_type),
> >> + pf->fault_level, pf->engine_class,
> >> + xe_hw_engine_class_to_str(pf->engine_class),
> >> + pf->engine_instance);
> >> +}
> >> +
> >> +static void show_blames(struct drm_printer *p, struct drm_file *file)
> >> +{
> >> + struct xe_file *xef = file->driver_priv;
> >> + struct xe_drm_client *client;
> >> + struct blame *b;
> >> +
> >> + client = xef->client;
> >> +
> >> + drm_printf(p, "\n");
> >> + drm_printf(p, "- Exec queue ban list -\n");
> >> + spin_lock(&client->blame_lock);
> >> + list_for_each_entry(b, &client->blame_list, list) {
> >> + struct pagefault *pf = b->pf;
> >> + drm_printf(p, "\n\tExec queue %u banned:\n", b->exec_queue_id);
> >> + drm_printf(p, "\t\tAssociated pagefault:\n");
> >> + if (pf)
> >> + print_pagefault(p, pf);
> >> + else
> >> + drm_printf(p, "\t\t- No associated pagefault -\n");
> >> + }
> >> + spin_unlock(&client->blame_lock);
> >> +}
> >> +
> >> /**
> >> * xe_drm_client_fdinfo() - Callback for fdinfo interface
> >> * @p: The drm_printer ptr
> >> @@ -394,5 +521,6 @@ void xe_drm_client_fdinfo(struct drm_printer *p,
> >> struct drm_file *file)
> >> {
> >> show_meminfo(p, file);
> >> show_run_ticks(p, file);
> >> + show_blames(p, file);
> >> }
> >> #endif
> >> diff --git a/drivers/gpu/drm/xe/xe_drm_client.h
> >> b/drivers/gpu/drm/xe/xe_drm_client.h
> >> index a9649aa36011..d21fd0b90742 100644
> >> --- a/drivers/gpu/drm/xe/xe_drm_client.h
> >> +++ b/drivers/gpu/drm/xe/xe_drm_client.h
> >> @@ -15,7 +15,18 @@
> >>
> >> struct drm_file;
> >> struct drm_printer;
> >> +struct pagefault;
> >> struct xe_bo;
> >> +struct xe_exec_queue;
> >> +
> >> +struct blame {
> >> + /** @exec_queue_id: ID number of banned exec queue */
> >> + u32 exec_queue_id;
> >> + /** @pf: pagefault on engine of banned exec queue, if any at time */
> >> + struct pagefault *pf;
> >> + /** @list: link into @xe_drm_client.blame_list */
> >> + struct list_head list;
> >> +};
> >>
> >> struct xe_drm_client {
> >> struct kref kref;
> >> @@ -31,6 +42,17 @@ struct xe_drm_client {
> >> * Protected by @bos_lock.
> >> */
> >> struct list_head bos_list;
> >> + /**
> >> + * @blame_lock: lock protecting @blame_list
> >> + */
> >> + spinlock_t blame_lock;
> >> + /**
> >> + * @blame_list: list of banned exec queues associated with this drm
> >> + * client, as well as any pagefaults at time of ban.
> >> + *
> >> + * Protected by @blame_lock;
> >> + */
> >> + struct list_head blame_list;
> >> #endif
> >> };
> >>
> >> @@ -57,6 +79,10 @@ void xe_drm_client_fdinfo(struct drm_printer *p, struct
> >> drm_file *file);
> >> void xe_drm_client_add_bo(struct xe_drm_client *client,
> >> struct xe_bo *bo);
> >> void xe_drm_client_remove_bo(struct xe_bo *bo);
> >> +void xe_drm_client_add_blame(struct xe_drm_client *client,
> >> + struct xe_exec_queue *q);
> >> +void xe_drm_client_remove_blame(struct xe_drm_client *client,
> >> + struct xe_exec_queue *q);
> >> #else
> >> static inline void xe_drm_client_add_bo(struct xe_drm_client *client,
> >> struct xe_bo *bo)
> >> @@ -66,5 +92,15 @@ static inline void xe_drm_client_add_bo(struct
> >> xe_drm_client *client,
> >> static inline void xe_drm_client_remove_bo(struct xe_bo *bo)
> >> {
> >> }
> >> +
> >> +static inline void xe_drm_client_add_blame(struct xe_drm_client *client,
> >> + struct xe_exec_queue *q)
> >> +{
> >> +}
> >> +
> >> +static inline void xe_drm_client_remove_blame(struct xe_drm_client
> >> *client,
> >> + struct xe_exec_queue *q)
> >> +{
> >> +}
> >> #endif
> >> #endif
> >> diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c
> >> b/drivers/gpu/drm/xe/xe_exec_queue.c
> >> index a02e62465e01..9c9bc617020c 100644
> >> --- a/drivers/gpu/drm/xe/xe_exec_queue.c
> >> +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
> >> @@ -13,6 +13,7 @@
> >> #include <uapi/drm/xe_drm.h>
> >>
> >> #include "xe_device.h"
> >> +#include "xe_drm_client.h"
> >> #include "xe_gt.h"
> >> #include "xe_hw_engine_class_sysfs.h"
> >> #include "xe_hw_engine_group.h"
> >> @@ -714,6 +715,12 @@ int xe_exec_queue_create_ioctl(struct drm_device
> >> *dev, void *data,
> >> q->id = id;
> >> args->exec_queue_id = id;
> >>
> >> + /**
> >> + * If an exec queue in the blame list shares the same exec queue
> >> + * ID, remove it from the blame list to avoid confusion.
> >> + */
> >> + xe_drm_client_remove_blame(q->xef->client, q);
> >> +
> >> return 0;
> >>
> >> kill_exec_queue:
> >> diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> >> b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> >> index fe18e3ec488a..a0e6f2281e37 100644
> >> --- a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> >> +++ b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> >> @@ -330,6 +330,23 @@ int xe_guc_pagefault_handler(struct xe_guc *guc, u32
> >> *msg, u32 len)
> >> return full ? -ENOSPC : 0;
> >> }
> >>
> >> +static void save_pagefault_to_engine(struct xe_gt *gt, struct pagefault
> >> *pf)
> >> +{
> >> + struct xe_hw_engine *hwe;
> >> +
> >> + hwe = xe_gt_hw_engine(gt, pf->engine_class, pf->engine_instance, false);
> >> + if (hwe) {
> >> + spin_lock(&hwe->pf.lock);
> >> + /** The latest pagefault is pf, so remove old pf info from
> >> engine */
> >> + if (hwe->pf.info)
> >> + kfree(hwe->pf.info);
> >> + hwe->pf.info = kzalloc(sizeof(struct pagefault), GFP_KERNEL);
> >> + if (hwe->pf.info)
> >> + memcpy(hwe->pf.info, pf, sizeof(struct pagefault));
> >> + spin_unlock(&hwe->pf.lock);
> >> + }
> >> +}
> >> +
> >> #define USM_QUEUE_MAX_RUNTIME_MS 20
> >>
> >> static void pf_queue_work_func(struct work_struct *w)
> >> @@ -352,6 +369,8 @@ static void pf_queue_work_func(struct work_struct *w)
> >> drm_dbg(&xe->drm, "Fault response: Unsuccessful %d\n",
> >> ret);
> >> }
> >>
> >> + save_pagefault_to_engine(gt, &pf);
> >> +
> >> reply.dw0 = FIELD_PREP(PFR_VALID, 1) |
> >> FIELD_PREP(PFR_SUCCESS, pf.fault_unsuccessful) |
> >> FIELD_PREP(PFR_REPLY, PFR_ACCESS) |
> >> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c
> >> b/drivers/gpu/drm/xe/xe_guc_submit.c
> >> index 913c74d6e2ae..d9da5c89429e 100644
> >> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> >> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> >> @@ -20,11 +20,13 @@
> >> #include "xe_assert.h"
> >> #include "xe_devcoredump.h"
> >> #include "xe_device.h"
> >> +#include "xe_drm_client.h"
> >> #include "xe_exec_queue.h"
> >> #include "xe_force_wake.h"
> >> #include "xe_gpu_scheduler.h"
> >> #include "xe_gt.h"
> >> #include "xe_gt_clock.h"
> >> +#include "xe_gt_pagefault.h"
> >> #include "xe_gt_printk.h"
> >> #include "xe_guc.h"
> >> #include "xe_guc_capture.h"
> >> @@ -146,6 +148,7 @@ static bool exec_queue_banned(struct xe_exec_queue *q)
> >> static void set_exec_queue_banned(struct xe_exec_queue *q)
> >> {
> >> atomic_or(EXEC_QUEUE_STATE_BANNED, &q->guc->state);
> >> + xe_drm_client_add_blame(q->xef->client, q);
> >> }
> >>
> >> static bool exec_queue_suspended(struct xe_exec_queue *q)
> >> @@ -1971,6 +1974,7 @@ int xe_guc_deregister_done_handler(struct xe_guc
> >> *guc, u32 *msg, u32 len)
> >> int xe_guc_exec_queue_reset_handler(struct xe_guc *guc, u32 *msg, u32
> >> len)
> >> {
> >> struct xe_gt *gt = guc_to_gt(guc);
> >> + struct xe_hw_engine *hwe;
> >> struct xe_exec_queue *q;
> >> u32 guc_id;
> >>
> >> @@ -1983,11 +1987,24 @@ int xe_guc_exec_queue_reset_handler(struct xe_guc
> >> *guc, u32 *msg, u32 len)
> >> if (unlikely(!q))
> >> return -EPROTO;
> >>
> >> + hwe = q->hwe;
> >> +
> >> xe_gt_info(gt, "Engine reset: engine_class=%s, logical_mask: 0x%x,
> >> guc_id=%d",
> >> xe_hw_engine_class_to_str(q->class), q->logical_mask,
> >> guc_id);
> >>
> >> trace_xe_exec_queue_reset(q);
> >>
> >> + /**
> >> + * Clear last pagefault from engine. Any future exec queue bans likely
> >> were
> >> + * not caused by said pagefault now that the engine has reset.
> >> + */
> >> + spin_lock(&hwe->pf.lock);
> >> + if (hwe->pf.info) {
> >> + kfree(hwe->pf.info);
> >> + hwe->pf.info = kzalloc(sizeof(struct pagefault), GFP_KERNEL);
> >> + }
> >> + spin_unlock(&hwe->pf.lock);
> >> +
> >> /*
> >> * A banned engine is a NOP at this point (came from
> >> * guc_exec_queue_timedout_job). Otherwise, kick drm scheduler to cancel
> >> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c
> >> b/drivers/gpu/drm/xe/xe_hw_engine.c
> >> index fc447751fe78..69f61e4905e2 100644
> >> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> >> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> >> @@ -21,6 +21,7 @@
> >> #include "xe_gsc.h"
> >> #include "xe_gt.h"
> >> #include "xe_gt_ccs_mode.h"
> >> +#include "xe_gt_pagefault.h"
> >> #include "xe_gt_printk.h"
> >> #include "xe_gt_mcr.h"
> >> #include "xe_gt_topology.h"
> >> @@ -557,6 +558,9 @@ static void hw_engine_init_early(struct xe_gt *gt,
> >> struct xe_hw_engine *hwe,
> >> hwe->eclass->defaults = hwe->eclass->sched_props;
> >> }
> >>
> >> + hwe->pf.info = NULL;
> >> + spin_lock_init(&hwe->pf.lock);
> >> +
> >> xe_reg_sr_init(&hwe->reg_sr, hwe->name, gt_to_xe(gt));
> >> xe_tuning_process_engine(hwe);
> >> xe_wa_process_engine(hwe);
> >> diff --git a/drivers/gpu/drm/xe/xe_hw_engine_types.h
> >> b/drivers/gpu/drm/xe/xe_hw_engine_types.h
> >> index e4191a7a2c31..2e1be9481d9b 100644
> >> --- a/drivers/gpu/drm/xe/xe_hw_engine_types.h
> >> +++ b/drivers/gpu/drm/xe/xe_hw_engine_types.h
> >> @@ -64,6 +64,7 @@ enum xe_hw_engine_id {
> >> struct xe_bo;
> >> struct xe_execlist_port;
> >> struct xe_gt;
> >> +struct pagefault;
> >>
> >> /**
> >> * struct xe_hw_engine_class_intf - per hw engine class struct interface
> >> @@ -150,6 +151,13 @@ struct xe_hw_engine {
> >> struct xe_oa_unit *oa_unit;
> >> /** @hw_engine_group: the group of hw engines this one belongs to */
> >> struct xe_hw_engine_group *hw_engine_group;
> >> + /** @pf: the last pagefault seen on this engine */
> >> + struct {
> >> + /** @pf.info: info containing last seen pagefault details */
> >> + struct pagefault *info;
> >> + /** @pf.lock: lock protecting @pf.info */
> >> + spinlock_t lock;
> >> + } pf;
> >> };
> >>
> >> enum xe_hw_engine_snapshot_source_id {
> >> --
> >> 2.43.0
> >>
> >
>
>
