Re: [PATCH v8 4/6] drm/xe/guc: Extract GuC error capture lists

["Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>]

On Mon, 2024-05-06 at 18:47 -0700, Zhanjun Dong wrote:
> Upon the G2H Notify-Err-Capture event, parse through the
> GuC Log Buffer (error-capture-subregion) and generate one or
> more capture-nodes. A single node represents a single "engine-
> instance-capture-dump" and contains at least 3 register lists:
> global, engine-class and engine-instance. An internal link
> list is maintained to store one or more nodes.
> 
> Because the link-list node generation happen before the call
> to devcoredump, duplicate global and engine-class register
> lists for each engine-instance register dump if we find
> dependent-engine resets in a engine-capture-group.
> 
alan:snip
> diff --git a/drivers/gpu/drm/xe/xe_guc_capture.c b/drivers/gpu/drm/xe/xe_guc_capture.c
> index d2df027081b5..71d7c4a58925 100644
> --- a/drivers/gpu/drm/xe/xe_guc_capture.c
> +++ b/drivers/gpu/drm/xe/xe_guc_capture.c
> @@ -520,6 +520,560 @@ static void check_guc_capture_size(struct xe_guc *guc)
>                           buffer_size, spare_size, capture_size);
>  }
> 
alan:snip
> +static struct __guc_capture_parsed_output *
> +guc_capture_get_prealloc_node(struct xe_guc *guc)
> +{
> +       struct __guc_capture_parsed_output *found = NULL;
> +
> +       if (!list_empty(&guc->capture->cachelist)) {
> +               struct __guc_capture_parsed_output *n, *ntmp;
> +
> +               /* get first avail node from the cache list */
> +               list_for_each_entry_safe(n, ntmp, &guc->capture->cachelist, link) {
> +                       found = n;
> +                       list_del(&n->link);
> +                       break;
> +               }
> +       } else {
> +               struct __guc_capture_parsed_output *n, *ntmp;
> +
> +               /* traverse down and steal back the oldest node already allocated */
> +               list_for_each_entry_safe(n, ntmp, &guc->capture->outlist, link) {
> +                       found = n;
> +               }
> +               if (found)
> +                       list_del(&found->link);
> +       }
> +       if (found)
> +               guc_capture_init_node(guc, found);
> +
> +       return found;
> +}
alan: I mentioned this in rev6, you cannot start pre-allocated nodelist anywhere
in this patch when you are only allocating it in patch 6. Look back at my rev 6
comments on this. Also, take a look at the original i915 patch on how to implement
guc_capture_alloc/delete_one_node without preallocated nodelist:
https://patchwork.freedesktop.org/patch/479022/?series=101604&rev=1
(note: watch especially for the use of new->reginfo[i].regs which needed
additional allocation step. Alternatively we could squash patch 4 and patch 6
together and change patch 4's comment but not sure it might be too large a
patch (can discuss offline).

> +static int
> +guc_capture_extract_reglists(struct xe_guc *guc, struct __guc_capture_bufstate *buf)
> +{
> +       struct xe_gt *gt = guc_to_gt(guc);
> +       struct guc_state_capture_group_header_t ghdr = {0};
> +       struct guc_state_capture_header_t hdr = {0};
> +       struct __guc_capture_parsed_output *node = NULL;
> +       struct guc_mmio_reg *regs = NULL;
> +       int i, numlists, numregs, ret = 0;
> +       enum guc_capture_type datatype;
> +       struct guc_mmio_reg tmp;
> +       bool is_partial = false;
alan:snip
> +               if (!node) {
> +                       node = guc_capture_get_prealloc_node(guc);
alan: see above comment on the use of prealloc_node (as per rev 6's comments)
alan:snip

> +static void __guc_capture_process_output(struct xe_guc *guc)
> +{
> +       unsigned int buffer_size, read_offset, write_offset, full_count;
> +       struct xe_uc *uc = container_of(guc, typeof(*uc), guc);
> +       struct guc_log_buffer_state log_buf_state_local;
> +       struct guc_log_buffer_state *log_buf_state;
> +       struct __guc_capture_bufstate buf;
> +       bool new_overflow;
> +       int ret;
> +       u32 log_buf_state_offset;
> +       u32 src_data_offset;
> +
> +       log_buf_state = (struct guc_log_buffer_state *)((ulong)guc->log.bo->vmap.vaddr +
> +                       (sizeof(struct guc_log_buffer_state) * GUC_CAPTURE_LOG_BUFFER));
alan: once again, i dont think we can use vmap.vaddr directly this this anymore right?
i dont think we use "log_buf_state" until the end of this function to set the new read_ptr
and flush flag. We ought to use xe_map_wr below?
> +
> +       log_buf_state_offset = sizeof(struct guc_log_buffer_state) * GUC_CAPTURE_LOG_BUFFER;
> +       src_data_offset = xe_guc_get_log_buffer_offset(&guc->log, GUC_CAPTURE_LOG_BUFFER);
> +
> +       /*
> +        * Make a copy of the state structure, inside GuC log buffer
> +        * (which is uncached mapped), on the stack to avoid reading
> +        * from it multiple times.
> +        */
> +       xe_map_memcpy_from(guc_to_xe(guc), &log_buf_state_local, &guc->log.bo->vmap,
> +                          log_buf_state_offset, sizeof(struct guc_log_buffer_state));
alan:snip