From: Matthew Brost <matthew.brost@intel.com>
To: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: intel-xe@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Subject: Re: [Intel-xe] [PATCH 05/14] drm/xe: Convert GuC CT print to snapshot capture and print.
Date: Tue, 2 May 2023 05:27:08 +0000 [thread overview]
Message-ID: <ZFCfLB9bPSLACRsO@DUT025-TGLU.fm.intel.com> (raw)
In-Reply-To: <20230426205713.512695-6-rodrigo.vivi@intel.com>
On Wed, Apr 26, 2023 at 04:57:04PM -0400, Rodrigo Vivi wrote:
> The goal is to allow for a snapshot capture to be taken at the time
> of the crash, while the print out can happen at a later time through
> the exposed devcoredump virtual device.
>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
> drivers/gpu/drm/xe/xe_guc_ct.c | 132 +++++++++++++++++++++++----
> drivers/gpu/drm/xe/xe_guc_ct.h | 7 +-
> drivers/gpu/drm/xe/xe_guc_ct_types.h | 26 ++++++
> 3 files changed, 145 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
> index e16e5fe37ed4..0b7b95dbd9be 100644
> --- a/drivers/gpu/drm/xe/xe_guc_ct.c
> +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
> @@ -1095,31 +1095,26 @@ static void g2h_worker_func(struct work_struct *w)
> xe_device_mem_access_put(ct_to_xe(ct));
> }
>
> -static void guc_ct_ctb_print(struct xe_device *xe, struct guc_ctb *ctb,
> - struct drm_printer *p)
> +static void guc_ctb_snapshot_capture(struct xe_device *xe, struct guc_ctb *ctb,
> + struct guc_ctb_snapshot *snapshot)
> {
> u32 head, tail;
>
> - drm_printf(p, "\tsize: %d\n", ctb->info.size);
> - drm_printf(p, "\tresv_space: %d\n", ctb->info.resv_space);
> - drm_printf(p, "\thead: %d\n", ctb->info.head);
> - drm_printf(p, "\ttail: %d\n", ctb->info.tail);
> - drm_printf(p, "\tspace: %d\n", ctb->info.space);
> - drm_printf(p, "\tbroken: %d\n", ctb->info.broken);
> + snapshot->cmds = kmalloc_array(ctb->info.size, sizeof(u32), GFP_ATOMIC);
So since this GFP_ATOMIC I assume this so we can call this code from the
TDR or a CTB handler (dma fence signaling paths)? Also I don't see
where you check for this allocation failing.
>
> - head = desc_read(xe, ctb, head);
> - tail = desc_read(xe, ctb, tail);
> - drm_printf(p, "\thead (memory): %d\n", head);
> - drm_printf(p, "\ttail (memory): %d\n", tail);
> - drm_printf(p, "\tstatus (memory): 0x%x\n", desc_read(xe, ctb, status));
> + xe_map_memcpy_from(xe, &snapshot->desc, &ctb->desc, 0,
> + sizeof(struct guc_ct_buffer_desc));
> + memcpy(&snapshot->info, &ctb->info, sizeof(struct guc_ctb_info));
> +
> + head = snapshot->desc.head;
> + tail = snapshot->desc.tail;
>
> if (head != tail) {
> struct iosys_map map =
> IOSYS_MAP_INIT_OFFSET(&ctb->cmds, head * sizeof(u32));
>
> while (head != tail) {
> - drm_printf(p, "\tcmd[%d]: 0x%08x\n", head,
> - xe_map_rd(xe, &map, 0, u32));
> + snapshot->cmds[head] = xe_map_rd(xe, &map, 0, u32);
> ++head;
> if (head == ctb->info.size) {
> head = 0;
> @@ -1131,20 +1126,119 @@ static void guc_ct_ctb_print(struct xe_device *xe, struct guc_ctb *ctb,
> }
> }
>
> -void xe_guc_ct_print(struct xe_guc_ct *ct, struct drm_printer *p)
> +static void guc_ctb_snapshot_print(struct guc_ctb_snapshot *snapshot,
> + struct drm_printer *p)
> +{
> + u32 head, tail;
> +
> + drm_printf(p, "\tsize: %d\n", snapshot->info.size);
> + drm_printf(p, "\tresv_space: %d\n", snapshot->info.space);
> + drm_printf(p, "\thead: %d\n", snapshot->info.head);
> + drm_printf(p, "\ttail: %d\n", snapshot->info.tail);
> + drm_printf(p, "\tspace: %d\n", snapshot->info.space);
> + drm_printf(p, "\tbroken: %d\n", snapshot->info.broken);
> + drm_printf(p, "\thead (memory): %d\n", snapshot->desc.head);
> + drm_printf(p, "\ttail (memory): %d\n", snapshot->desc.tail);
> + drm_printf(p, "\tstatus (memory): 0x%x\n", snapshot->desc.status);
> +
> + head = snapshot->desc.head;
> + tail = snapshot->desc.tail;
> +
> + while (head != tail) {
> + drm_printf(p, "\tcmd[%d]: 0x%08x\n", head,
> + snapshot->cmds[head]);
> + ++head;
> + if (head == snapshot->info.size)
> + head = 0;
> + }
> +}
> +
> +static void guc_ctb_snapshot_free(struct guc_ctb_snapshot *snapshot)
> +{
> + kfree(snapshot->cmds);
> +}
> +
> +/**
> + * xe_guc_ct_snapshot_capture - Take a quick snapshot of the CT state.
> + * @ct: GuC CT object.
> + *
> + * This can be printed out in a later stage like during dev_coredump
> + * analysis.
> + *
> + * Returns: a GuC CT snapshot object that must be freed by the caller
> + * by using `xe_guc_ct_snapshot_free`.
> + */
> +struct xe_guc_ct_snapshot *xe_guc_ct_snapshot_capture(struct xe_guc_ct *ct)
> {
> + struct xe_guc_ct_snapshot *snapshot;
> +
> + snapshot = kzalloc(sizeof(struct xe_guc_ct_snapshot), GFP_ATOMIC);
> +
Same here, need to check for an alloc failure.
Also maybe we should a flag to switch between GFP_ATOMIC (signaling
path) and GFP_KERNEL (debugfs). In the case above, CMDs might be huge as
deadlock workaround (like 16 MBs or something) so atomic seems risky.
Aside from these comments, I do rather like what you have done here.
Matt
> if (ct->enabled) {
> + snapshot->ct_enabled = true;
> + guc_ctb_snapshot_capture(ct_to_xe(ct), &ct->ctbs.h2g,
> + &snapshot->h2g);
> + guc_ctb_snapshot_capture(ct_to_xe(ct), &ct->ctbs.g2h,
> + &snapshot->g2h);
> + }
> +
> + return snapshot;
> +}
> +
> +/**
> + * xe_guc_ct_snapshot_print - Print out a given GuC CT snapshot.
> + * @snapshot: GuC CT snapshot object.
> + * @p: drm_printer where it will be printed out.
> + *
> + * This function prints out a given GuC CT snapshot object.
> + */
> +void xe_guc_ct_snapshot_print(struct xe_guc_ct_snapshot *snapshot,
> + struct drm_printer *p)
> +{
> + if (snapshot->ct_enabled) {
> drm_puts(p, "\nH2G CTB (all sizes in DW):\n");
> - guc_ct_ctb_print(ct_to_xe(ct), &ct->ctbs.h2g, p);
> + guc_ctb_snapshot_print(&snapshot->h2g, p);
>
> drm_puts(p, "\nG2H CTB (all sizes in DW):\n");
> - guc_ct_ctb_print(ct_to_xe(ct), &ct->ctbs.g2h, p);
> - drm_printf(p, "\tg2h outstanding: %d\n", ct->g2h_outstanding);
> + guc_ctb_snapshot_print(&snapshot->g2h, p);
> +
> + drm_printf(p, "\tg2h outstanding: %d\n",
> + snapshot->g2h_outstanding);
> } else {
> drm_puts(p, "\nCT disabled\n");
> }
> }
>
> +/**
> + * xe_guc_ct_snapshot_free - Free all allocated objects for a given snapshot.
> + * @snapshot: GuC CT snapshot object.
> + *
> + * This function free all the memory that needed to be allocated at capture
> + * time.
> + */
> +void xe_guc_ct_snapshot_free(struct xe_guc_ct_snapshot *snapshot)
> +{
> + guc_ctb_snapshot_free(&snapshot->h2g);
> + guc_ctb_snapshot_free(&snapshot->g2h);
> + kfree(snapshot);
> +}
> +
> +/**
> + * xe_guc_ct_print - GuC CT Print.
> + * @ct: GuC CT.
> + * @p: drm_printer where it will be printed out.
> + *
> + * This function quickly capture a snapshot and immediately print it out.
> + */
> +void xe_guc_ct_print(struct xe_guc_ct *ct, struct drm_printer *p)
> +{
> + struct xe_guc_ct_snapshot *snapshot;
> +
> + snapshot = xe_guc_ct_snapshot_capture(ct);
> + xe_guc_ct_snapshot_print(snapshot, p);
> + xe_guc_ct_snapshot_free(snapshot);
> +}
> +
> #ifdef XE_GUC_CT_SELFTEST
> /*
> * Disable G2H processing in IRQ handler to force xe_guc_ct_send to enter flow
> diff --git a/drivers/gpu/drm/xe/xe_guc_ct.h b/drivers/gpu/drm/xe/xe_guc_ct.h
> index 49fb74f91e4d..29e0dff7ad9b 100644
> --- a/drivers/gpu/drm/xe/xe_guc_ct.h
> +++ b/drivers/gpu/drm/xe/xe_guc_ct.h
> @@ -13,9 +13,14 @@ struct drm_printer;
> int xe_guc_ct_init(struct xe_guc_ct *ct);
> int xe_guc_ct_enable(struct xe_guc_ct *ct);
> void xe_guc_ct_disable(struct xe_guc_ct *ct);
> -void xe_guc_ct_print(struct xe_guc_ct *ct, struct drm_printer *p);
> void xe_guc_ct_fast_path(struct xe_guc_ct *ct);
>
> +struct xe_guc_ct_snapshot *xe_guc_ct_snapshot_capture(struct xe_guc_ct *ct);
> +void xe_guc_ct_snapshot_print(struct xe_guc_ct_snapshot *snapshot,
> + struct drm_printer *p);
> +void xe_guc_ct_snapshot_free(struct xe_guc_ct_snapshot *snapshot);
> +void xe_guc_ct_print(struct xe_guc_ct *ct, struct drm_printer *p);
> +
> static inline void xe_guc_ct_irq_handler(struct xe_guc_ct *ct)
> {
> wake_up_all(&ct->wq);
> diff --git a/drivers/gpu/drm/xe/xe_guc_ct_types.h b/drivers/gpu/drm/xe/xe_guc_ct_types.h
> index 64e3dd14d4b2..93046d95b009 100644
> --- a/drivers/gpu/drm/xe/xe_guc_ct_types.h
> +++ b/drivers/gpu/drm/xe/xe_guc_ct_types.h
> @@ -48,6 +48,32 @@ struct guc_ctb {
> struct guc_ctb_info info;
> };
>
> +/**
> + * struct guc_ctb_snapshot - GuC command transport buffer (CTB) snapshot
> + */
> +struct guc_ctb_snapshot {
> + /** @desc: snapshot of the CTB descriptor */
> + struct guc_ct_buffer_desc desc;
> + /** @cmds: snapshot of the CTB commands */
> + u32 *cmds;
> + /** @info: snapshot of the CTB info */
> + struct guc_ctb_info info;
> +};
> +
> +/**
> + * struct xe_guc_ct_snapshot - GuC command transport (CT) snapshot
> + */
> +struct xe_guc_ct_snapshot {
> + /** @ct_enabled: CT enabled info at capture time. */
> + bool ct_enabled;
> + /** @g2h_outstanding: G2H outstanding info at the capture time */
> + u32 g2h_outstanding;
> + /** @g2h: G2H CTB snapshot */
> + struct guc_ctb_snapshot g2h;
> + /** @h2g: H2G CTB snapshot */
> + struct guc_ctb_snapshot h2g;
> +};
> +
> /**
> * struct xe_guc_ct - GuC command transport (CT) layer
> *
> --
> 2.39.2
>
next prev parent reply other threads:[~2023-05-02 5:27 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-26 20:56 [Intel-xe] [PATCH 00/14] Introduce xe_devcoredump Rodrigo Vivi
2023-04-26 20:57 ` [Intel-xe] [PATCH 01/14] drm/xe: Fix print of RING_EXECLIST_SQ_CONTENTS_HI Rodrigo Vivi
2023-04-26 21:40 ` Lucas De Marchi
2023-04-26 21:59 ` Rodrigo Vivi
2023-04-26 20:57 ` [Intel-xe] [PATCH 02/14] drm/xe: Introduce the dev_coredump infrastructure Rodrigo Vivi
2023-04-27 8:28 ` Thomas Hellström
2023-05-02 7:57 ` Matthew Brost
2023-05-02 18:06 ` Rodrigo Vivi
2023-05-02 20:29 ` Matthew Brost
2023-05-02 7:55 ` Jani Nikula
2023-05-02 17:25 ` Rodrigo Vivi
2023-04-26 20:57 ` [Intel-xe] [PATCH 03/14] drm/xe: Do not take any action if our device was removed Rodrigo Vivi
2023-05-02 15:40 ` Matthew Brost
2023-05-02 17:21 ` Rodrigo Vivi
2023-05-02 23:06 ` Matthew Brost
2023-04-26 20:57 ` [Intel-xe] [PATCH 04/14] drm/xe: Extract non mapped regions out of GuC CTB into its own struct Rodrigo Vivi
2023-05-02 5:12 ` Matthew Brost
2023-04-26 20:57 ` [Intel-xe] [PATCH 05/14] drm/xe: Convert GuC CT print to snapshot capture and print Rodrigo Vivi
2023-05-02 5:27 ` Matthew Brost [this message]
2023-04-26 20:57 ` [Intel-xe] [PATCH 06/14] drm/xe: Add GuC CT snapshot to xe_devcoredump Rodrigo Vivi
2023-05-02 14:55 ` Matthew Brost
2023-04-26 20:57 ` [Intel-xe] [PATCH 07/14] drm/xe: Introduce guc_submit_types.h with relevant structs Rodrigo Vivi
2023-05-02 7:44 ` Matthew Brost
2023-04-26 20:57 ` [Intel-xe] [PATCH 08/14] drm/xe: Convert GuC Engine print to snapshot capture and print Rodrigo Vivi
2023-05-02 15:01 ` Matthew Brost
2023-04-26 20:57 ` [Intel-xe] [PATCH 09/14] drm/xe: Add GuC Submit Engine snapshot to xe_devcoredump Rodrigo Vivi
2023-05-02 15:03 ` Matthew Brost
2023-04-26 20:57 ` [Intel-xe] [PATCH 10/14] drm/xe: Convert Xe HW Engine print to snapshot capture and print Rodrigo Vivi
2023-05-02 15:20 ` Matthew Brost
2023-04-26 20:57 ` [Intel-xe] [PATCH 11/14] drm/xe: Add HW Engine snapshot to xe_devcoredump Rodrigo Vivi
2023-05-02 15:30 ` Matthew Brost
2023-04-26 20:57 ` [Intel-xe] [PATCH 12/14] drm/xe: Limit CONFIG_DRM_XE_SIMPLE_ERROR_CAPTURE to itself Rodrigo Vivi
2023-05-02 15:35 ` Matthew Brost
2023-04-26 20:57 ` [Intel-xe] [PATCH 13/14] drm/xe: Convert VM print to snapshot capture and print Rodrigo Vivi
2023-05-02 7:50 ` Matthew Brost
2023-05-02 8:07 ` Matthew Brost
2023-04-26 20:57 ` [Intel-xe] [PATCH 14/14] drm/xe: Add VM snapshot to xe_devcoredump Rodrigo Vivi
2023-05-02 15:38 ` Matthew Brost
2023-04-26 21:01 ` [Intel-xe] ✓ CI.Patch_applied: success for Introduce xe_devcoredump Patchwork
2023-04-26 21:02 ` [Intel-xe] ✓ CI.KUnit: " Patchwork
2023-04-26 21:06 ` [Intel-xe] ✓ CI.Build: " Patchwork
2023-04-26 21:29 ` [Intel-xe] ○ CI.BAT: info " Patchwork
2023-05-02 8:11 ` [Intel-xe] [PATCH 00/14] " Matthew Brost
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZFCfLB9bPSLACRsO@DUT025-TGLU.fm.intel.com \
--to=matthew.brost@intel.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-xe@lists.freedesktop.org \
--cc=rodrigo.vivi@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox