Re: [PATCH v11 1/5] drm/xe/guc: Prepare GuC register list and update ADS size for error capture

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

Hi Zhanjun, i see you addressed everything i commented on in rev8 as
well as everything that Michal commented on in rev9 (except one which
wasnt an issue but was by design).

Although I still found couple of the issues which are not nits, they
are very trivial so i'm giving a conditional RB for the comments i
prefixed with "rb-condition".

Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com>

On Mon, 2024-06-24 at 14:54 -0700, Zhanjun Dong wrote:
> Add referenced registers defines and list of registers.
> Update GuC ADS size allocation to include space for
> the lists of error state capture register descriptors.
> 
> 
alan:snip
> --- a/drivers/gpu/drm/xe/xe_guc_ads.c
> +++ b/drivers/gpu/drm/xe/xe_guc_ads.c
> @@ -18,6 +18,8 @@
>  #include "xe_gt_ccs_mode.h"
>  #include "xe_gt_printk.h"
>  #include "xe_guc.h"
> +#include "xe_guc_capture.h"
> +#include "xe_guc_capture_types.h"
>  #include "xe_guc_ct.h"
>  #include "xe_hw_engine.h"
>  #include "xe_lrc.h"
> @@ -127,6 +129,8 @@ struct __guc_ads_blob {
>  #define info_map_read(xe_, map_, field_) \
>         xe_map_rd_field(xe_, map_, 0, struct guc_gt_system_info,
> field_)
>  
> +static int guc_capture_prep_lists(struct xe_guc_ads *ads);
alan:nit: I think you don't need this prototype definition here now?



alan:snip

> diff --git a/drivers/gpu/drm/xe/xe_guc_capture_types.h
> b/drivers/gpu/drm/xe/xe_guc_capture_types.h
> new file mode 100644
> index 000000000000..b2e78ab53e2e

alan:snip
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_guc_capture_types.h
> @@ -0,0 +1,80 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2024 Intel Corporation
> + */
> +
> +#ifndef _xe_guc_capture_types_H
> +#define _xe_guc_capture_types_H
alan: [rb-condition] this should be all caps, no?
> +
> +#include <linux/types.h>
> +
alan:snip

> +       /**
> +        * @cachelist: Pool of pre-allocated nodes for error capture
> output
> +        *
> +        * We need this pool of pre-allocated nodes because we cannot
> +        * dynamically allocate new nodes when receiving the G2H
> notification
> +        * because the event handlers for all G2H event-processing is
> called
> +        * by the ct processing worker queue and when that queue is
> being
> +        * processed, there is no absoluate guarantee that we are not
> in the
> +        * midst of a GT reset operation (which doesn't allow
> allocations).
> +        */
> +       struct list_head cachelist;
alan: [rb-condition] please move the introduction of cachelist to the
patch where its actually getting initialized and used (extraction
patch, right?)
> +#define PREALLOC_NODES_MAX_COUNT (3 * GUC_MAX_ENGINE_CLASSES *
> GUC_MAX_INSTANCES_PER_CLASS)
> +#define PREALLOC_NODES_DEFAULT_NUMREGS 64
> +
> +       /**
> +        * @max_mmio_per_node: Max MMIO per node.
> +        */
> +       int max_mmio_per_node;
> +
> +       /**
> +        * @outlist: Pool of pre-allocated nodes for error capture
> output
> +        *
> +        * A linked list of parsed GuC error-capture output data
> before
> +        * reporting with formatting via xe_devcoredump. Each node in
> this linked list shall
> +        * contain a single engine-capture including global, engine-
> class and
> +        * engine-instance register dumps as per
> guc_capture_parsed_output_node
> +        */
> +       struct list_head outlist;
> 
alan: [rb-condition] please move the introduction of outlist to the
patch where its actually getting initialized and used (extraction
patch, right?)
> +};
> +