[RFC,2/7] drm/i915/guc: Update GuC ADS size for error capture lists

Commit Message

Alan Previn Nov. 22, 2021, 11:03 p.m. UTC

Update GuC ADS size allocation to include space for
the lists of error state capture register descriptors.

Also, populate the lists of registers we want GuC to report back to
Host on engine reset events. This list should include global,
engine-class and engine-instance registers for every engine-class
type on the current hardware.

NOTE: Start with a fake table of register lists to layout the
framework before adding real registers in subsequent patch.

Signed-off-by: Alan Previn <[e-mail]>
---
 drivers/gpu/drm/i915/Makefile                 |   1 +
 drivers/gpu/drm/i915/gt/uc/intel_guc.c        |  10 +-
 drivers/gpu/drm/i915/gt/uc/intel_guc.h        |   5 +
 drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c    | 176 ++++++++++++-
 .../gpu/drm/i915/gt/uc/intel_guc_capture.c    | 232 ++++++++++++++++++
 .../gpu/drm/i915/gt/uc/intel_guc_capture.h    |  47 ++++
 drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h   |  19 +-
 7 files changed, 476 insertions(+), 14 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_capture.h

Comments

Michal Wajdeczko Nov. 23, 2021, 9:46 p.m. UTC | #1

Hi,

just few random nits below

-Michal


On 23.11.2021 00:03, Alan Previn wrote:
> Update GuC ADS size allocation to include space for
> the lists of error state capture register descriptors.
> 
> Also, populate the lists of registers we want GuC to report back to
> Host on engine reset events. This list should include global,
> engine-class and engine-instance registers for every engine-class
> type on the current hardware.
> 
> NOTE: Start with a fake table of register lists to layout the
> framework before adding real registers in subsequent patch.
> 
> Signed-off-by: Alan Previn <[e-mail]>
> ---
>  drivers/gpu/drm/i915/Makefile                 |   1 +
>  drivers/gpu/drm/i915/gt/uc/intel_guc.c        |  10 +-
>  drivers/gpu/drm/i915/gt/uc/intel_guc.h        |   5 +
>  drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c    | 176 ++++++++++++-
>  .../gpu/drm/i915/gt/uc/intel_guc_capture.c    | 232 ++++++++++++++++++
>  .../gpu/drm/i915/gt/uc/intel_guc_capture.h    |  47 ++++
>  drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h   |  19 +-
>  7 files changed, 476 insertions(+), 14 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
>  create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_capture.h
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 074d6b8edd23..e3c4d5cea4c3 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -190,6 +190,7 @@ i915-y += gt/uc/intel_uc.o \
>  	  gt/uc/intel_guc_rc.o \
>  	  gt/uc/intel_guc_slpc.o \
>  	  gt/uc/intel_guc_submission.o \
> +	  gt/uc/intel_guc_capture.o \

use alphabetical order

>  	  gt/uc/intel_huc.o \
>  	  gt/uc/intel_huc_debugfs.o \
>  	  gt/uc/intel_huc_fw.o
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> index 5cf9ebd2ee55..458f0d248a5a 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> @@ -335,9 +335,14 @@ int intel_guc_init(struct intel_guc *guc)
>  	if (ret)
>  		goto err_fw;
>  
> -	ret = intel_guc_ads_create(guc);
> +	ret = intel_guc_capture_init(guc);
>  	if (ret)
>  		goto err_log;
> +
> +	ret = intel_guc_ads_create(guc);
> +	if (ret)
> +		goto err_capture;
> +
>  	GEM_BUG_ON(!guc->ads_vma);
>  
>  	ret = intel_guc_ct_init(&guc->ct);
> @@ -376,6 +381,8 @@ int intel_guc_init(struct intel_guc *guc)
>  	intel_guc_ct_fini(&guc->ct);
>  err_ads:
>  	intel_guc_ads_destroy(guc);
> +err_capture:
> +	intel_guc_capture_destroy(guc);
>  err_log:
>  	intel_guc_log_destroy(&guc->log);
>  err_fw:
> @@ -403,6 +410,7 @@ void intel_guc_fini(struct intel_guc *guc)
>  	intel_guc_ct_fini(&guc->ct);
>  
>  	intel_guc_ads_destroy(guc);
> +	intel_guc_capture_destroy(guc);
>  	intel_guc_log_destroy(&guc->log);
>  	intel_uc_fw_fini(&guc->fw);
>  }
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> index 9de99772f916..d136c69abe12 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> @@ -16,6 +16,7 @@
>  #include "intel_guc_log.h"
>  #include "intel_guc_reg.h"
>  #include "intel_guc_slpc_types.h"
> +#include "intel_guc_capture.h"

use alphabetical order

>  #include "intel_uc_fw.h"
>  #include "i915_utils.h"
>  #include "i915_vma.h"
> @@ -37,6 +38,8 @@ struct intel_guc {
>  	struct intel_guc_ct ct;
>  	/** @slpc: sub-structure containing SLPC related data and objects */
>  	struct intel_guc_slpc slpc;
> +	/** @capture: the error-state-capture module's data and objects */
> +	struct intel_guc_state_capture capture;
>  
>  	/** @sched_engine: Global engine used to submit requests to GuC */
>  	struct i915_sched_engine *sched_engine;
> @@ -138,6 +141,8 @@ struct intel_guc {
>  	u32 ads_regset_size;
>  	/** @ads_golden_ctxt_size: size of the golden contexts in the ADS */
>  	u32 ads_golden_ctxt_size;
> +	/** @ads_capture_size: size of register lists in the ADS used for error capture */
> +	u32 ads_capture_size;
>  	/** @ads_engine_usage_size: size of engine usage in the ADS */
>  	u32 ads_engine_usage_size;
>  
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> index 6c81ddd303d3..2780c0fadd01 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> @@ -10,6 +10,7 @@
>  #include "gt/shmem_utils.h"
>  #include "intel_guc_ads.h"
>  #include "intel_guc_fwif.h"
> +#include "intel_guc_capture.h"

wrong order

>  #include "intel_uc.h"
>  #include "i915_drv.h"
>  
> @@ -71,8 +72,7 @@ static u32 guc_ads_golden_ctxt_size(struct intel_guc *guc)
>  
>  static u32 guc_ads_capture_size(struct intel_guc *guc)
>  {
> -	/* Basic support to init ADS without a proper GuC error capture list */
> -	return PAGE_ALIGN(PAGE_SIZE);
> +	return PAGE_ALIGN(guc->ads_capture_size);
>  }
>  
>  static u32 guc_ads_private_data_size(struct intel_guc *guc)
> @@ -519,24 +519,170 @@ static void guc_init_golden_context(struct intel_guc *guc)
>  	GEM_BUG_ON(guc->ads_golden_ctxt_size != total_size);
>  }
>  
> -static void guc_capture_prep_lists(struct intel_guc *guc, struct __guc_ads_blob *blob)
> +static int
> +guc_fill_reglist(struct intel_guc *guc, struct __guc_ads_blob *blob, int vf, bool enabled,
> +		 int classid, int type, char *typename, u16 *p_numregs, int newnum, u8 **p_virt_ptr,
> +		 u32 *p_blobptr_to_ggtt, u32 *p_ggtt, u32 null_ggtt)

hmm, this does not look good - do we really need all these params ?

>  {
> -	int i, j;
> -	u32 addr_ggtt, offset;
> +	struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
> +	struct guc_debug_capture_list *listnode;
> +	int size = 0;
>  
> -	offset = guc_ads_capture_offset(guc);
> -	addr_ggtt = intel_guc_ggtt_offset(guc, guc->ads_vma) + offset;
> +	if (blob && *p_numregs != newnum) {
> +		if (type == GUC_CAPTURE_LIST_TYPE_GLOBAL)
> +			drm_warn(&i915->drm, "Guc-Cap VF%d-%s num-reg mismatch was=%d now=%d!\n",
> +				 vf, typename, *p_numregs, newnum);
> +		else
> +			drm_warn(&i915->drm, "Guc-Cap VF%d-Class-%d-%s num-reg mismatch was=%d now=%d!\n",
> +				 vf, classid, typename, *p_numregs, newnum);
> +	}
> +	/*
> +	 * For enabled capture lists, we not only need to call capture module to help
> +	 * populate the list-descriptor into the correct ads capture structures, but
> +	 * we also need to increment the virtual pointers and ggtt offsets so that
> +	 * caller has the subsequent gfx memory location.
> +	 */
> +	*p_numregs = newnum;
> +	size = PAGE_ALIGN((sizeof(struct guc_debug_capture_list)) +
> +			  (newnum * sizeof(struct guc_mmio_reg)));
> +	/* if caller hasn't allocated ADS blob, return size and counts, we're done */
> +	if (!blob)
> +		return size;
> +	if (blob) {

redundant

> +		/* if caller allocated ADS blob, populate the capture register descriptors */
> +		if (!newnum) {
> +			*p_blobptr_to_ggtt = null_ggtt;
> +		} else {
> +			/* get ptr and populate header info: */
> +			*p_blobptr_to_ggtt = *p_ggtt;
> +			listnode = (struct guc_debug_capture_list *)*p_virt_ptr;
> +			*p_ggtt += sizeof(struct guc_debug_capture_list);
> +			*p_virt_ptr += sizeof(struct guc_debug_capture_list);
> +			listnode->header.info = FIELD_PREP(GUC_CAPTURELISTHDR_NUMDESCR, *p_numregs);
> +
> +			/* get ptr and populate register descriptor list: */
> +			intel_guc_capture_list_init(guc, vf, type, classid,
> +						    (struct guc_mmio_reg *)*p_virt_ptr,
> +						    *p_numregs);
> +
> +			/* increment ptrs for that header: */
> +			*p_ggtt += size - sizeof(struct guc_debug_capture_list);
> +			*p_virt_ptr += size - sizeof(struct guc_debug_capture_list);
> +		}
> +	}
> +
> +	return size;
> +}
> +
> +static int guc_capture_prep_lists(struct intel_guc *guc, struct __guc_ads_blob *blob)
> +{
> +	struct intel_gt *gt = guc_to_gt(guc);
> +	int i, j, size;
> +	u32 ggtt, null_ggtt, offset, alloc_size = 0;
> +	struct guc_gt_system_info *info, local_info;
> +	struct guc_debug_capture_list *listnode;
> +	struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
> +	struct intel_guc_state_capture *gc = &guc->capture;
> +	u16 tmp = 0;
> +	u8 *ptr = NULL;
> +
> +	if (blob) {
> +		offset = guc_ads_capture_offset(guc);
> +		ggtt = intel_guc_ggtt_offset(guc, guc->ads_vma) + offset;
> +		ptr = ((u8 *)blob) + offset;
> +		info = &blob->system_info;
> +	} else {
> +		memset(&local_info, 0, sizeof(local_info));
> +		info = &local_info;
> +		fill_engine_enable_masks(gt, info);
> +	}
> +
> +	/* first, set aside the first page for a capture_list with zero descriptors */
> +	alloc_size = PAGE_SIZE;
> +	if (blob) {
> +		listnode = (struct guc_debug_capture_list *)ptr;
> +		listnode->header.info = FIELD_PREP(GUC_CAPTURELISTHDR_NUMDESCR, 0);
> +		null_ggtt = ggtt;
> +		ggtt += PAGE_SIZE;
> +		ptr +=  PAGE_SIZE;
> +	}
>  
> -	/* FIXME: Populate a proper capture list */
> +#define COUNT_REGS intel_guc_capture_list_count
> +#define FILL_REGS guc_fill_reglist
> +#define TYPE_CLASS GUC_CAPTURE_LIST_TYPE_ENGINE_CLASS
> +#define TYPE_INSTANCE GUC_CAPTURE_LIST_TYPE_ENGINE_INSTANCE
>  
>  	for (i = 0; i < GUC_CAPTURE_LIST_INDEX_MAX; i++) {
>  		for (j = 0; j < GUC_MAX_ENGINE_CLASSES; j++) {
> -			blob->ads.capture_instance[i][j] = addr_ggtt;
> -			blob->ads.capture_class[i][j] = addr_ggtt;
> +			if (!info->engine_enabled_masks[j]) {
> +				if (gc->num_class_regs[i][j])
> +					drm_warn(&i915->drm, "GuC-Cap VF%d-class-%d "
> +						 "class regs valid mismatch was=%d now=%d!\n",
> +						 i, j, gc->num_class_regs[i][j], tmp);
> +				if (gc->num_instance_regs[i][j])
> +					drm_warn(&i915->drm, "GuC-Cap VF%d-class-%d "
> +						 "inst regs valid mismatch was=%d now=%d!\n",
> +						 i, j, gc->num_instance_regs[i][j], tmp);
> +				gc->num_class_regs[i][j] = 0;
> +				gc->num_instance_regs[i][j] = 0;
> +				if (blob) {
> +					blob->ads.capture_class[i][j] = null_ggtt;
> +					blob->ads.capture_instance[i][j] = null_ggtt;
> +				}
> +			} else {
> +				if (!COUNT_REGS(guc, i, TYPE_CLASS,
> +						guc_class_to_engine_class(j), &tmp)) {
> +					size = FILL_REGS(guc, blob, i, true, j, TYPE_CLASS,
> +							 "class", &gc->num_class_regs[i][j],
> +							 tmp, &ptr,
> +							 &blob->ads.capture_class[i][j],
> +							 &ggtt, null_ggtt);
> +					gc->class_list_size += size;
> +					alloc_size += size;
> +				} else {
> +					gc->num_class_regs[i][j] = 0;
> +					if (blob)
> +						blob->ads.capture_class[i][j] = null_ggtt;
> +				}
> +				if (!COUNT_REGS(guc, i, TYPE_INSTANCE,
> +						guc_class_to_engine_class(j), &tmp)) {
> +					size = FILL_REGS(guc, blob, i, true, j, TYPE_INSTANCE,
> +							 "instance", &gc->num_instance_regs[i][j],
> +							 tmp, &ptr,
> +							 &blob->ads.capture_instance[i][j],
> +							 &ggtt, null_ggtt);
> +					gc->instance_list_size += size;
> +					alloc_size += size;
> +				} else {
> +					gc->num_instance_regs[i][j] = 0;
> +					if (blob)
> +						blob->ads.capture_instance[i][j] = null_ggtt;
> +				}
> +			}
> +		}
> +		if (!COUNT_REGS(guc, i, GUC_CAPTURE_LIST_TYPE_GLOBAL, 0, &tmp)) {
> +			size = FILL_REGS(guc, blob, i, true, 0, GUC_CAPTURE_LIST_TYPE_GLOBAL,
> +					 "global", &gc->num_global_regs[i], tmp, &ptr,
> +					 &blob->ads.capture_global[i], &ggtt, null_ggtt);
> +			gc->global_list_size += size;
> +			alloc_size += size;
> +		} else {
> +			gc->num_global_regs[i] = 0;
> +			if (blob)
> +				blob->ads.capture_global[i] = null_ggtt;
>  		}
> -
> -		blob->ads.capture_global[i] = addr_ggtt;
>  	}
> +
> +#undef COUNT_REGS
> +#undef FILL_REGS
> +#undef TYPE_CLASS
> +#undef TYPE_INSTANCE
> +
> +	if (guc->ads_capture_size && guc->ads_capture_size != PAGE_ALIGN(alloc_size))
> +		drm_warn(&i915->drm, "GuC->ADS->Capture alloc size changed from %d to %d\n",
> +			 guc->ads_capture_size, PAGE_ALIGN(alloc_size));
> +
> +	return PAGE_ALIGN(alloc_size);
>  }
>  
>  static void __guc_ads_init(struct intel_guc *guc)
> @@ -614,6 +760,12 @@ int intel_guc_ads_create(struct intel_guc *guc)
>  		return ret;
>  	guc->ads_golden_ctxt_size = ret;
>  
> +	/* Likewise the capture lists: */
> +	ret = guc_capture_prep_lists(guc, NULL);
> +	if (ret < 0)
> +		return ret;
> +	guc->ads_capture_size = ret;
> +
>  	/* Now the total size can be determined: */
>  	size = guc_ads_blob_size(guc);
>  
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> new file mode 100644
> index 000000000000..c741c77b7fc8
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> @@ -0,0 +1,232 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2021-2021 Intel Corporation
> + */
> +
> +#include <drm/drm_print.h>
> +
> +#include "i915_drv.h"
> +#include "i915_drv.h"

duplicated include

> +#include "i915_memcpy.h"
> +#include "gt/intel_gt.h"
> +
> +#include "intel_guc_fwif.h"
> +#include "intel_guc_capture.h"
> +
> +/* Define all device tables of GuC error capture register lists */
> +
> +/********************************* Gen12 LP  *********************************/

didn't we move away from "GEN" naming ?

> +/************** GLOBAL *************/

do we really need all these decorations ?

> +struct __guc_mmio_reg_descr gen12lp_global_regs[] = {
> +	{SWF_ILK(0),               0,      0, "SWF_ILK0"},
> +	/* Add additional register list */

do we need this reminder ?

> +};
> +
> +/********** RENDER/COMPUTE *********/
> +/* Per-Class */
> +struct __guc_mmio_reg_descr gen12lp_rc_class_regs[] = {
> +	{SWF_ILK(0),               0,      0, "SWF_ILK0"},
> +	/* Add additional register list */
> +};
> +
> +/* Per-Engine-Instance */
> +struct __guc_mmio_reg_descr gen12lp_rc_inst_regs[] = {
> +	{SWF_ILK(0),               0,      0, "SWF_ILK0"},
> +	/* Add additional register list */
> +};
> +
> +/************* MEDIA-VD ************/
> +/* Per-Class */
> +struct __guc_mmio_reg_descr gen12lp_vd_class_regs[] = {
> +	{SWF_ILK(0),               0,      0, "SWF_ILK0"},
> +	/* Add additional register list */
> +};
> +
> +/* Per-Engine-Instance */
> +struct __guc_mmio_reg_descr gen12lp_vd_inst_regs[] = {
> +	{SWF_ILK(0),               0,      0, "SWF_ILK0"},
> +	/* Add additional register list */
> +};
> +
> +/************* MEDIA-VEC ***********/
> +/* Per-Class */
> +struct __guc_mmio_reg_descr gen12lp_vec_class_regs[] = {
> +	{SWF_ILK(0),               0,      0, "SWF_ILK0"},
> +	/* Add additional register list */
> +};
> +
> +/* Per-Engine-Instance */
> +struct __guc_mmio_reg_descr gen12lp_vec_inst_regs[] = {
> +	{SWF_ILK(0),               0,      0, "SWF_ILK0"},
> +	/* Add additional register list */
> +};
> +
> +/********** List of lists **********/
> +struct __guc_mmio_reg_descr_group gen12lp_lists[] = {
> +	{
> +		.list = gen12lp_global_regs,
> +		.num_regs = (sizeof(gen12lp_global_regs) / sizeof(struct __guc_mmio_reg_descr)),

ARRAY_SIZE ?

> +		.owner = GUC_CAPTURE_LIST_INDEX_PF,
> +		.type = GUC_CAPTURE_LIST_TYPE_GLOBAL,
> +		.engine = 0
> +	},
> +	{
> +		.list = gen12lp_rc_class_regs,
> +		.num_regs = (sizeof(gen12lp_rc_class_regs) / sizeof(struct __guc_mmio_reg_descr)),
> +		.owner = GUC_CAPTURE_LIST_INDEX_PF,
> +		.type = GUC_CAPTURE_LIST_TYPE_ENGINE_CLASS,
> +		.engine = RENDER_CLASS
> +	},
> +	{
> +		.list = gen12lp_rc_inst_regs,
> +		.num_regs = (sizeof(gen12lp_rc_inst_regs) / sizeof(struct __guc_mmio_reg_descr)),
> +		.owner = GUC_CAPTURE_LIST_INDEX_PF,
> +		.type = GUC_CAPTURE_LIST_TYPE_ENGINE_INSTANCE,
> +		.engine = RENDER_CLASS
> +	},
> +	{
> +		.list = gen12lp_vd_class_regs,
> +		.num_regs = (sizeof(gen12lp_vd_class_regs) / sizeof(struct __guc_mmio_reg_descr)),
> +		.owner = GUC_CAPTURE_LIST_INDEX_PF,
> +		.type = GUC_CAPTURE_LIST_TYPE_ENGINE_CLASS,
> +		.engine = VIDEO_DECODE_CLASS
> +	},
> +	{
> +		.list = gen12lp_vd_inst_regs,
> +		.num_regs = (sizeof(gen12lp_vd_inst_regs) / sizeof(struct __guc_mmio_reg_descr)),
> +		.owner = GUC_CAPTURE_LIST_INDEX_PF,
> +		.type = GUC_CAPTURE_LIST_TYPE_ENGINE_INSTANCE,
> +		.engine = VIDEO_DECODE_CLASS
> +	},
> +	{
> +		.list = gen12lp_vec_class_regs,
> +		.num_regs = (sizeof(gen12lp_vec_class_regs) / sizeof(struct __guc_mmio_reg_descr)),
> +		.owner = GUC_CAPTURE_LIST_INDEX_PF,
> +		.type = GUC_CAPTURE_LIST_TYPE_ENGINE_CLASS,
> +		.engine = VIDEO_ENHANCEMENT_CLASS
> +	},
> +	{
> +		.list = gen12lp_vec_inst_regs,
> +		.num_regs = (sizeof(gen12lp_vec_inst_regs) / sizeof(struct __guc_mmio_reg_descr)),
> +		.owner = GUC_CAPTURE_LIST_INDEX_PF,
> +		.type = GUC_CAPTURE_LIST_TYPE_ENGINE_INSTANCE,
> +		.engine = VIDEO_ENHANCEMENT_CLASS
> +	},
> +	{NULL, 0, 0, 0, 0}
> +};
> +
> +/************ FIXME: Populate tables for other devices in subsequent patch ************/
> +
> +static struct __guc_mmio_reg_descr_group *
> +guc_capture_get_device_reglist(struct drm_i915_private *dev_priv)

in new code we are using "i915" instead of "dev_priv" and since this
function has "guc" prefix it shall rather take "guc" as param:

guc_capture_get_device_reglist(struct intel_guc *guc)
{
	struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
	...


> +{
> +	if (IS_TIGERLAKE(dev_priv) || IS_ROCKETLAKE(dev_priv) ||
> +	    IS_ALDERLAKE_S(dev_priv) || IS_ALDERLAKE_P(dev_priv)) {
> +		return gen12lp_lists;
> +	}
> +
> +	return NULL;
> +}
> +
> +static inline struct __guc_mmio_reg_descr_group *
> +guc_capture_get_one_list(struct __guc_mmio_reg_descr_group *reglists, u32 owner, u32 type, u32 id)
> +{
> +	int i = 0;
> +
> +	if (!reglists)
> +		return NULL;
> +	while (reglists[i].list) {
> +		if (reglists[i].owner == owner &&
> +		    reglists[i].type == type) {
> +			if (reglists[i].type == GUC_CAPTURE_LIST_TYPE_GLOBAL ||
> +			    reglists[i].engine == id) {
> +				return &reglists[i];
> +			}
> +		}
> +		++i;
> +	}
> +	return NULL;
> +}
> +
> +static inline void
> +warn_with_capture_list_identifier(struct drm_i915_private *i915, char *msg,
> +				  u32 owner, u32 type, u32 classid)
> +{
> +	const char *ownerstr[GUC_CAPTURE_LIST_INDEX_MAX] = {"PF", "VF"};
> +	const char *typestr[GUC_CAPTURE_LIST_TYPE_MAX - 1] = {"Class", "Instance"};
> +	const char *classstr[GUC_LAST_ENGINE_CLASS + 1] = {"Render", "Video", "VideoEnhance",
> +							   "Blitter", "Reserved"};

better to wrap that into simple small helpers like

	const char *stringify_guc_capture_owner(u32 owner) { .. }
	const char *stringify_guc_capture_type(u32 type) { .. }
	const char *stringify_guc_capture_class(u32 class) { .. }

> +	static const char unknownstr[] = "unknown";
> +
> +	if (type == GUC_CAPTURE_LIST_TYPE_GLOBAL)
> +		drm_warn(&i915->drm, "GuC-capture: %s for %s Global-Registers.\n", msg,
> +			 (owner < GUC_CAPTURE_LIST_INDEX_MAX) ? ownerstr[owner] : unknownstr);
> +	else
> +		drm_warn(&i915->drm, "GuC-capture: %s for %s %s-Registers on %s-Engine\n", msg,
> +			 (owner < GUC_CAPTURE_LIST_INDEX_MAX) ? ownerstr[owner] : unknownstr,
> +			 (type < GUC_CAPTURE_LIST_TYPE_MAX) ? typestr[type - 1] :  unknownstr,
> +			 (classid < GUC_LAST_ENGINE_CLASS + 1) ? classstr[classid] : unknownstr);
> +}
> +
> +int intel_guc_capture_list_count(struct intel_guc *guc, u32 owner, u32 type, u32 classid,
> +				 u16 *num_entries)
> +{
> +	struct drm_i915_private *dev_priv = (guc_to_gt(guc))->i915;

s/dev_priv/i915
redundant ()

> +	struct __guc_mmio_reg_descr_group *reglists = guc->capture.reglists;
> +	struct __guc_mmio_reg_descr_group *match;
> +
> +	if (!reglists)
> +		return -ENODEV;
> +
> +	match = guc_capture_get_one_list(reglists, owner, type, classid);
> +	if (match) {
> +		*num_entries = match->num_regs;
> +		return 0;

IIRC early returns are preferred for error cases, not success

> +	}
> +
> +	warn_with_capture_list_identifier(dev_priv, "Missing register list size", owner, type,
> +					  classid);
> +
> +	return -ENODATA;
> +}
> +
> +int intel_guc_capture_list_init(struct intel_guc *guc, u32 owner, u32 type, u32 classid,
> +				struct guc_mmio_reg *ptr, u16 num_entries)
> +{
> +	u32 j = 0;
> +	struct drm_i915_private *dev_priv = (guc_to_gt(guc))->i915;

s/dev_priv/i915
redundant ()

> +	struct __guc_mmio_reg_descr_group *reglists = guc->capture.reglists;
> +	struct __guc_mmio_reg_descr_group *match;
> +
> +	if (!reglists)
> +		return -ENODEV;
> +
> +	match = guc_capture_get_one_list(reglists, owner, type, classid);
> +	if (match) {
> +		while (j < num_entries && j < match->num_regs) {
> +			ptr[j].offset = match->list[j].reg.reg;
> +			ptr[j].value = 0xDEADF00D;
> +			ptr[j].flags = match->list[j].flags;
> +			ptr[j].mask = match->list[j].mask;
> +			++j;
> +		}
> +		return 0;
> +	}
> +
> +	warn_with_capture_list_identifier(dev_priv, "Missing register list init", owner, type,
> +					  classid);
> +
> +	return -ENODATA;
> +}
> +
> +void intel_guc_capture_destroy(struct intel_guc *guc)
> +{
> +}
> +
> +int intel_guc_capture_init(struct intel_guc *guc)
> +{
> +	struct drm_i915_private *dev_priv = (guc_to_gt(guc))->i915;
> +
> +	guc->capture.reglists = guc_capture_get_device_reglist(dev_priv);
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.h
> new file mode 100644
> index 000000000000..352940b8bc87
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.h
> @@ -0,0 +1,47 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2021-2021 Intel Corporation
> + */
> +
> +#ifndef _INTEL_GUC_CAPTURE_H
> +#define _INTEL_GUC_CAPTURE_H
> +
> +#include <linux/mutex.h>
> +#include <linux/workqueue.h>
> +#include "intel_guc_fwif.h"
> +
> +struct intel_guc;
> +
> +struct __guc_mmio_reg_descr {
> +	i915_reg_t reg;
> +	u32 flags;
> +	u32 mask;
> +	char *regname;

const char* ?

but maybe instead of adding reg name to the GuC specific struct we
should add generic purpose function that will return pretty name of the
register:

i915_reg.c:

const char *i915_reg_to_string(i915_reg_r reg)
{
	...
}

> +};
> +
> +struct __guc_mmio_reg_descr_group {
> +	struct __guc_mmio_reg_descr *list;
> +	u32 num_regs;
> +	u32 owner; /* see enum guc_capture_owner */
> +	u32 type; /* see enum guc_capture_type */
> +	u32 engine; /* as per MAX_ENGINE_CLASS */
> +};
> +
> +struct intel_guc_state_capture {
> +	struct __guc_mmio_reg_descr_group *reglists;
> +	u16 num_instance_regs[GUC_CAPTURE_LIST_INDEX_MAX][GUC_MAX_ENGINE_CLASSES];
> +	u16 num_class_regs[GUC_CAPTURE_LIST_INDEX_MAX][GUC_MAX_ENGINE_CLASSES];
> +	u16 num_global_regs[GUC_CAPTURE_LIST_INDEX_MAX];
> +	int instance_list_size;
> +	int class_list_size;
> +	int global_list_size;
> +};
> +
> +int intel_guc_capture_list_count(struct intel_guc *guc, u32 owner, u32 type, u32 class,
> +				 u16 *num_entries);
> +int intel_guc_capture_list_init(struct intel_guc *guc, u32 owner, u32 type, u32 class,
> +				struct guc_mmio_reg *ptr, u16 num_entries);
> +void intel_guc_capture_destroy(struct intel_guc *guc);
> +int intel_guc_capture_init(struct intel_guc *guc);
> +
> +#endif /* _INTEL_GUC_CAPTURE_H */
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> index 767684b6af67..1a1d2271c7e9 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> @@ -285,13 +285,30 @@ struct guc_gt_system_info {
>  } __packed;
>  
>  /* Capture-types of GuC capture register lists */
> -enum
> +enum guc_capture_owner
>  {
>  	GUC_CAPTURE_LIST_INDEX_PF = 0,
>  	GUC_CAPTURE_LIST_INDEX_VF = 1,
>  	GUC_CAPTURE_LIST_INDEX_MAX = 2,

s/INDEX/OWNER ?

>  };
>  
> +/*Register-types of GuC capture register lists */
> +enum guc_capture_type {
> +	GUC_CAPTURE_LIST_TYPE_GLOBAL = 0,
> +	GUC_CAPTURE_LIST_TYPE_ENGINE_CLASS,
> +	GUC_CAPTURE_LIST_TYPE_ENGINE_INSTANCE,
> +	GUC_CAPTURE_LIST_TYPE_MAX,
> +};
> +
> +struct guc_debug_capture_list_header {
> +	u32 info;
> +		#define GUC_CAPTURELISTHDR_NUMDESCR GENMASK(15, 0)
> +};
> +
> +struct guc_debug_capture_list {
> +	struct guc_debug_capture_list_header header;
> +};
> +
>  /* GuC Additional Data Struct */
>  struct guc_ads {
>  	struct guc_mmio_reg_set reg_state_list[GUC_MAX_ENGINE_CLASSES][GUC_MAX_INSTANCES_PER_CLASS];
>

Jani Nikula Nov. 24, 2021, 9:52 a.m. UTC | #2

On Tue, 23 Nov 2021, Michal Wajdeczko <[e-mail]> wrote:
> Hi,
>
> just few random nits below
>
> -Michal
>
>
> On 23.11.2021 00:03, Alan Previn wrote:
>> +/* Define all device tables of GuC error capture register lists */
>> +
>> +/********************************* Gen12 LP  *********************************/
>
> didn't we move away from "GEN" naming ?

Yes.

>
>> +/************** GLOBAL *************/
>
> do we really need all these decorations ?

No, please remove them.

>
>> +struct __guc_mmio_reg_descr gen12lp_global_regs[] = {
>> +	{SWF_ILK(0),               0,      0, "SWF_ILK0"},
>> +	/* Add additional register list */
>
> do we need this reminder ?

No, please remove them.

Also, all of these need to be static.


BR,
Jani.

Jani Nikula Nov. 24, 2021, 10:06 a.m. UTC | #3

On Mon, 22 Nov 2021, Alan Previn <[e-mail]> wrote:
> +	{
> +		.list = gen12lp_vec_class_regs,
> +		.num_regs = (sizeof(gen12lp_vec_class_regs) / sizeof(struct __guc_mmio_reg_descr)),
> +		.owner = GUC_CAPTURE_LIST_INDEX_PF,
> +		.type = GUC_CAPTURE_LIST_TYPE_ENGINE_CLASS,
> +		.engine = VIDEO_ENHANCEMENT_CLASS
> +	},
> +	{

Usually }, { on the same line

> +		.list = gen12lp_vec_inst_regs,
> +		.num_regs = (sizeof(gen12lp_vec_inst_regs) / sizeof(struct __guc_mmio_reg_descr)),
> +		.owner = GUC_CAPTURE_LIST_INDEX_PF,
> +		.type = GUC_CAPTURE_LIST_TYPE_ENGINE_INSTANCE,
> +		.engine = VIDEO_ENHANCEMENT_CLASS
> +	},
> +	{NULL, 0, 0, 0, 0}

Just {}  should work as a sentinel.

> +};
> +
> +/************ FIXME: Populate tables for other devices in subsequent patch ************/

Please don't add any of this ******* nonsense.

> +
> +static struct __guc_mmio_reg_descr_group *
> +guc_capture_get_device_reglist(struct drm_i915_private *dev_priv)
> +{
> +	if (IS_TIGERLAKE(dev_priv) || IS_ROCKETLAKE(dev_priv) ||
> +	    IS_ALDERLAKE_S(dev_priv) || IS_ALDERLAKE_P(dev_priv)) {
> +		return gen12lp_lists;
> +	}
> +
> +	return NULL;
> +}
> +
> +static inline struct __guc_mmio_reg_descr_group *
> +guc_capture_get_one_list(struct __guc_mmio_reg_descr_group *reglists, u32 owner, u32 type, u32 id)

Please don't use inlines in .c files. Let the compiler decide.

> +{
> +	int i = 0;
> +
> +	if (!reglists)
> +		return NULL;
> +	while (reglists[i].list) {
> +		if (reglists[i].owner == owner &&
> +		    reglists[i].type == type) {
> +			if (reglists[i].type == GUC_CAPTURE_LIST_TYPE_GLOBAL ||
> +			    reglists[i].engine == id) {
> +				return &reglists[i];
> +			}
> +		}
> +		++i;
> +	}

That's a for loop right there.

> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.h
> new file mode 100644
> index 000000000000..352940b8bc87
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.h
> @@ -0,0 +1,47 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2021-2021 Intel Corporation
> + */
> +
> +#ifndef _INTEL_GUC_CAPTURE_H
> +#define _INTEL_GUC_CAPTURE_H
> +
> +#include <linux/mutex.h>
> +#include <linux/workqueue.h>

Both of these seem random and completely unnecessary. linux/types.h is
required but it's not here.

> +#include "intel_guc_fwif.h"

I've been trying hard to reduce includes from headers throughout the
driver, to clean up and clarify the interfaces and dependencies. I don't
know how the guc headers have grown the kind of interdependencies that
they all pull in almost everything.

This one line pulls in another 19 headers. Just to get
GUC_CAPTURE_LIST_INDEX_MAX and GUC_MAX_ENGINE_CLASSES. Everything else
could be solved through forward declarations.

BR,
Jani.


> +
> +struct intel_guc;
> +
> +struct __guc_mmio_reg_descr {
> +	i915_reg_t reg;
> +	u32 flags;
> +	u32 mask;
> +	char *regname;
> +};
> +
> +struct __guc_mmio_reg_descr_group {
> +	struct __guc_mmio_reg_descr *list;
> +	u32 num_regs;
> +	u32 owner; /* see enum guc_capture_owner */
> +	u32 type; /* see enum guc_capture_type */
> +	u32 engine; /* as per MAX_ENGINE_CLASS */
> +};
> +
> +struct intel_guc_state_capture {
> +	struct __guc_mmio_reg_descr_group *reglists;
> +	u16 num_instance_regs[GUC_CAPTURE_LIST_INDEX_MAX][GUC_MAX_ENGINE_CLASSES];
> +	u16 num_class_regs[GUC_CAPTURE_LIST_INDEX_MAX][GUC_MAX_ENGINE_CLASSES];
> +	u16 num_global_regs[GUC_CAPTURE_LIST_INDEX_MAX];
> +	int instance_list_size;
> +	int class_list_size;
> +	int global_list_size;
> +};
> +
> +int intel_guc_capture_list_count(struct intel_guc *guc, u32 owner, u32 type, u32 class,
> +				 u16 *num_entries);
> +int intel_guc_capture_list_init(struct intel_guc *guc, u32 owner, u32 type, u32 class,
> +				struct guc_mmio_reg *ptr, u16 num_entries);
> +void intel_guc_capture_destroy(struct intel_guc *guc);
> +int intel_guc_capture_init(struct intel_guc *guc);
> +
> +#endif /* _INTEL_GUC_CAPTURE_H */
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> index 767684b6af67..1a1d2271c7e9 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> @@ -285,13 +285,30 @@ struct guc_gt_system_info {
>  } __packed;
>  
>  /* Capture-types of GuC capture register lists */
> -enum
> +enum guc_capture_owner
>  {
>  	GUC_CAPTURE_LIST_INDEX_PF = 0,
>  	GUC_CAPTURE_LIST_INDEX_VF = 1,
>  	GUC_CAPTURE_LIST_INDEX_MAX = 2,
>  };
>  
> +/*Register-types of GuC capture register lists */
> +enum guc_capture_type {
> +	GUC_CAPTURE_LIST_TYPE_GLOBAL = 0,
> +	GUC_CAPTURE_LIST_TYPE_ENGINE_CLASS,
> +	GUC_CAPTURE_LIST_TYPE_ENGINE_INSTANCE,
> +	GUC_CAPTURE_LIST_TYPE_MAX,
> +};
> +
> +struct guc_debug_capture_list_header {
> +	u32 info;
> +		#define GUC_CAPTURELISTHDR_NUMDESCR GENMASK(15, 0)
> +};
> +
> +struct guc_debug_capture_list {
> +	struct guc_debug_capture_list_header header;
> +};
> +
>  /* GuC Additional Data Struct */
>  struct guc_ads {
>  	struct guc_mmio_reg_set reg_state_list[GUC_MAX_ENGINE_CLASSES][GUC_MAX_INSTANCES_PER_CLASS];

Alan Previn Nov. 24, 2021, 5:34 p.m. UTC | #4

Thanks Michal for the thorough review of the code (and the other patches). I will fix them all.

On the register-to-string helper function,
i'll have to think it through because i do want to keep future development
maintenance work when adding new registers simple (in the sense that
adding a single line into the table will be all thats needed).

Unless you are suggesting keeping a global i915-wide list somewhere?
which might be a bit of an overhead when searching through an offset list
to find the mmio being requested for string return - unless i keep a sorted tree
initialized with registers ordered by address, but would not work well for
different registers that share addresses on diff gen's).


...alan


On Tue, 2021-11-23 at 22:46 +0100, Michal Wajdeczko wrote:
> Hi,
> 
> just few random nits below
> 
> -Michal
> 
> 
> On 23.11.2021 00:03, Alan Previn wrote:
> > Update GuC ADS size allocation to include space for
> > the lists of error state capture register descriptors.
> > 
> > Also, populate the lists of registers we want GuC to report back to
> > Host on engine reset events. This list should include global,
> > engine-class and engine-instance registers for every engine-class
> > type on the current hardware.
> > 
> > NOTE: Start with a fake table of register lists to layout the
> > framework before adding real registers in subsequent patch.
> > 
> > Signed-off-by: Alan Previn <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/Makefile                 |   1 +
> >  drivers/gpu/drm/i915/gt/uc/intel_guc.c        |  10 +-
> >  drivers/gpu/drm/i915/gt/uc/intel_guc.h        |   5 +
> >  drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c    | 176 ++++++++++++-
> >  .../gpu/drm/i915/gt/uc/intel_guc_capture.c    | 232 ++++++++++++++++++
> >  .../gpu/drm/i915/gt/uc/intel_guc_capture.h    |  47 ++++
> >  drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h   |  19 +-
> >  7 files changed, 476 insertions(+), 14 deletions(-)
> >  create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> >  create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_capture.h
> > 
> > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> > index 074d6b8edd23..e3c4d5cea4c3 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -190,6 +190,7 @@ i915-y += gt/uc/intel_uc.o \
> >  	  gt/uc/intel_guc_rc.o \
> >  	  gt/uc/intel_guc_slpc.o \
> >  	  gt/uc/intel_guc_submission.o \
> > +	  gt/uc/intel_guc_capture.o \
> 
> use alphabetical order
> 
> >  	  gt/uc/intel_huc.o \
> >  	  gt/uc/intel_huc_debugfs.o \
> >  	  gt/uc/intel_huc_fw.o
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> > index 5cf9ebd2ee55..458f0d248a5a 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> > @@ -335,9 +335,14 @@ int intel_guc_init(struct intel_guc *guc)
> >  	if (ret)
> >  		goto err_fw;
> >  
> > -	ret = intel_guc_ads_create(guc);
> > +	ret = intel_guc_capture_init(guc);
> >  	if (ret)
> >  		goto err_log;
> > +
> > +	ret = intel_guc_ads_create(guc);
> > +	if (ret)
> > +		goto err_capture;
> > +
> >  	GEM_BUG_ON(!guc->ads_vma);
> >  
> >  	ret = intel_guc_ct_init(&guc->ct);
> > @@ -376,6 +381,8 @@ int intel_guc_init(struct intel_guc *guc)
> >  	intel_guc_ct_fini(&guc->ct);
> >  err_ads:
> >  	intel_guc_ads_destroy(guc);
> > +err_capture:
> > +	intel_guc_capture_destroy(guc);
> >  err_log:
> >  	intel_guc_log_destroy(&guc->log);
> >  err_fw:
> > @@ -403,6 +410,7 @@ void intel_guc_fini(struct intel_guc *guc)
> >  	intel_guc_ct_fini(&guc->ct);
> >  
> >  	intel_guc_ads_destroy(guc);
> > +	intel_guc_capture_destroy(guc);
> >  	intel_guc_log_destroy(&guc->log);
> >  	intel_uc_fw_fini(&guc->fw);
> >  }
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > index 9de99772f916..d136c69abe12 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > @@ -16,6 +16,7 @@
> >  #include "intel_guc_log.h"
> >  #include "intel_guc_reg.h"
> >  #include "intel_guc_slpc_types.h"
> > +#include "intel_guc_capture.h"
> 
> use alphabetical order
> 
> >  #include "intel_uc_fw.h"
> >  #include "i915_utils.h"
> >  #include "i915_vma.h"
> > @@ -37,6 +38,8 @@ struct intel_guc {
> >  	struct intel_guc_ct ct;
> >  	/** @slpc: sub-structure containing SLPC related data and objects */
> >  	struct intel_guc_slpc slpc;
> > +	/** @capture: the error-state-capture module's data and objects */
> > +	struct intel_guc_state_capture capture;
> >  
> >  	/** @sched_engine: Global engine used to submit requests to GuC */
> >  	struct i915_sched_engine *sched_engine;
> > @@ -138,6 +141,8 @@ struct intel_guc {
> >  	u32 ads_regset_size;
> >  	/** @ads_golden_ctxt_size: size of the golden contexts in the ADS */
> >  	u32 ads_golden_ctxt_size;
> > +	/** @ads_capture_size: size of register lists in the ADS used for error capture */
> > +	u32 ads_capture_size;
> >  	/** @ads_engine_usage_size: size of engine usage in the ADS */
> >  	u32 ads_engine_usage_size;
> >  
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> > index 6c81ddd303d3..2780c0fadd01 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> > @@ -10,6 +10,7 @@
> >  #include "gt/shmem_utils.h"
> >  #include "intel_guc_ads.h"
> >  #include "intel_guc_fwif.h"
> > +#include "intel_guc_capture.h"
> 
> wrong order
> 
> >  #include "intel_uc.h"
> >  #include "i915_drv.h"
> >  
> > @@ -71,8 +72,7 @@ static u32 guc_ads_golden_ctxt_size(struct intel_guc *guc)
> >  
> >  static u32 guc_ads_capture_size(struct intel_guc *guc)
> >  {
> > -	/* Basic support to init ADS without a proper GuC error capture list */
> > -	return PAGE_ALIGN(PAGE_SIZE);
> > +	return PAGE_ALIGN(guc->ads_capture_size);
> >  }
> >  
> >  static u32 guc_ads_private_data_size(struct intel_guc *guc)
> > @@ -519,24 +519,170 @@ static void guc_init_golden_context(struct intel_guc *guc)
> >  	GEM_BUG_ON(guc->ads_golden_ctxt_size != total_size);
> >  }
> >  
> > -static void guc_capture_prep_lists(struct intel_guc *guc, struct __guc_ads_blob *blob)
> > +static int
> > +guc_fill_reglist(struct intel_guc *guc, struct __guc_ads_blob *blob, int vf, bool enabled,
> > +		 int classid, int type, char *typename, u16 *p_numregs, int newnum, u8 **p_virt_ptr,
> > +		 u32 *p_blobptr_to_ggtt, u32 *p_ggtt, u32 null_ggtt)
> 
> hmm, this does not look good - do we really need all these params ?
> 
> >  {
> > -	int i, j;
> > -	u32 addr_ggtt, offset;
> > +	struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
> > +	struct guc_debug_capture_list *listnode;
> > +	int size = 0;
> >  
> > -	offset = guc_ads_capture_offset(guc);
> > -	addr_ggtt = intel_guc_ggtt_offset(guc, guc->ads_vma) + offset;
> > +	if (blob && *p_numregs != newnum) {
> > +		if (type == GUC_CAPTURE_LIST_TYPE_GLOBAL)
> > +			drm_warn(&i915->drm, "Guc-Cap VF%d-%s num-reg mismatch was=%d now=%d!\n",
> > +				 vf, typename, *p_numregs, newnum);
> > +		else
> > +			drm_warn(&i915->drm, "Guc-Cap VF%d-Class-%d-%s num-reg mismatch was=%d now=%d!\n",
> > +				 vf, classid, typename, *p_numregs, newnum);
> > +	}
> > +	/*
> > +	 * For enabled capture lists, we not only need to call capture module to help
> > +	 * populate the list-descriptor into the correct ads capture structures, but
> > +	 * we also need to increment the virtual pointers and ggtt offsets so that
> > +	 * caller has the subsequent gfx memory location.
> > +	 */
> > +	*p_numregs = newnum;
> > +	size = PAGE_ALIGN((sizeof(struct guc_debug_capture_list)) +
> > +			  (newnum * sizeof(struct guc_mmio_reg)));
> > +	/* if caller hasn't allocated ADS blob, return size and counts, we're done */
> > +	if (!blob)
> > +		return size;
> > +	if (blob) {
> 
> redundant
> 
> > +		/* if caller allocated ADS blob, populate the capture register descriptors */
> > +		if (!newnum) {
> > +			*p_blobptr_to_ggtt = null_ggtt;
> > +		} else {
> > +			/* get ptr and populate header info: */
> > +			*p_blobptr_to_ggtt = *p_ggtt;
> > +			listnode = (struct guc_debug_capture_list *)*p_virt_ptr;
> > +			*p_ggtt += sizeof(struct guc_debug_capture_list);
> > +			*p_virt_ptr += sizeof(struct guc_debug_capture_list);
> > +			listnode->header.info = FIELD_PREP(GUC_CAPTURELISTHDR_NUMDESCR, *p_numregs);
> > +
> > +			/* get ptr and populate register descriptor list: */
> > +			intel_guc_capture_list_init(guc, vf, type, classid,
> > +						    (struct guc_mmio_reg *)*p_virt_ptr,
> > +						    *p_numregs);
> > +
> > +			/* increment ptrs for that header: */
> > +			*p_ggtt += size - sizeof(struct guc_debug_capture_list);
> > +			*p_virt_ptr += size - sizeof(struct guc_debug_capture_list);
> > +		}
> > +	}
> > +
> > +	return size;
> > +}
> > +
> > +static int guc_capture_prep_lists(struct intel_guc *guc, struct __guc_ads_blob *blob)
> > +{
> > +	struct intel_gt *gt = guc_to_gt(guc);
> > +	int i, j, size;
> > +	u32 ggtt, null_ggtt, offset, alloc_size = 0;
> > +	struct guc_gt_system_info *info, local_info;
> > +	struct guc_debug_capture_list *listnode;
> > +	struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
> > +	struct intel_guc_state_capture *gc = &guc->capture;
> > +	u16 tmp = 0;
> > +	u8 *ptr = NULL;
> > +
> > +	if (blob) {
> > +		offset = guc_ads_capture_offset(guc);
> > +		ggtt = intel_guc_ggtt_offset(guc, guc->ads_vma) + offset;
> > +		ptr = ((u8 *)blob) + offset;
> > +		info = &blob->system_info;
> > +	} else {
> > +		memset(&local_info, 0, sizeof(local_info));
> > +		info = &local_info;
> > +		fill_engine_enable_masks(gt, info);
> > +	}
> > +
> > +	/* first, set aside the first page for a capture_list with zero descriptors */
> > +	alloc_size = PAGE_SIZE;
> > +	if (blob) {
> > +		listnode = (struct guc_debug_capture_list *)ptr;
> > +		listnode->header.info = FIELD_PREP(GUC_CAPTURELISTHDR_NUMDESCR, 0);
> > +		null_ggtt = ggtt;
> > +		ggtt += PAGE_SIZE;
> > +		ptr +=  PAGE_SIZE;
> > +	}
> >  
> > -	/* FIXME: Populate a proper capture list */
> > +#define COUNT_REGS intel_guc_capture_list_count
> > +#define FILL_REGS guc_fill_reglist
> > +#define TYPE_CLASS GUC_CAPTURE_LIST_TYPE_ENGINE_CLASS
> > +#define TYPE_INSTANCE GUC_CAPTURE_LIST_TYPE_ENGINE_INSTANCE
> >  
> >  	for (i = 0; i < GUC_CAPTURE_LIST_INDEX_MAX; i++) {
> >  		for (j = 0; j < GUC_MAX_ENGINE_CLASSES; j++) {
> > -			blob->ads.capture_instance[i][j] = addr_ggtt;
> > -			blob->ads.capture_class[i][j] = addr_ggtt;
> > +			if (!info->engine_enabled_masks[j]) {
> > +				if (gc->num_class_regs[i][j])
> > +					drm_warn(&i915->drm, "GuC-Cap VF%d-class-%d "
> > +						 "class regs valid mismatch was=%d now=%d!\n",
> > +						 i, j, gc->num_class_regs[i][j], tmp);
> > +				if (gc->num_instance_regs[i][j])
> > +					drm_warn(&i915->drm, "GuC-Cap VF%d-class-%d "
> > +						 "inst regs valid mismatch was=%d now=%d!\n",
> > +						 i, j, gc->num_instance_regs[i][j], tmp);
> > +				gc->num_class_regs[i][j] = 0;
> > +				gc->num_instance_regs[i][j] = 0;
> > +				if (blob) {
> > +					blob->ads.capture_class[i][j] = null_ggtt;
> > +					blob->ads.capture_instance[i][j] = null_ggtt;
> > +				}
> > +			} else {
> > +				if (!COUNT_REGS(guc, i, TYPE_CLASS,
> > +						guc_class_to_engine_class(j), &tmp)) {
> > +					size = FILL_REGS(guc, blob, i, true, j, TYPE_CLASS,
> > +							 "class", &gc->num_class_regs[i][j],
> > +							 tmp, &ptr,
> > +							 &blob->ads.capture_class[i][j],
> > +							 &ggtt, null_ggtt);
> > +					gc->class_list_size += size;
> > +					alloc_size += size;
> > +				} else {
> > +					gc->num_class_regs[i][j] = 0;
> > +					if (blob)
> > +						blob->ads.capture_class[i][j] = null_ggtt;
> > +				}
> > +				if (!COUNT_REGS(guc, i, TYPE_INSTANCE,
> > +						guc_class_to_engine_class(j), &tmp)) {
> > +					size = FILL_REGS(guc, blob, i, true, j, TYPE_INSTANCE,
> > +							 "instance", &gc->num_instance_regs[i][j],
> > +							 tmp, &ptr,
> > +							 &blob->ads.capture_instance[i][j],
> > +							 &ggtt, null_ggtt);
> > +					gc->instance_list_size += size;
> > +					alloc_size += size;
> > +				} else {
> > +					gc->num_instance_regs[i][j] = 0;
> > +					if (blob)
> > +						blob->ads.capture_instance[i][j] = null_ggtt;
> > +				}
> > +			}
> > +		}
> > +		if (!COUNT_REGS(guc, i, GUC_CAPTURE_LIST_TYPE_GLOBAL, 0, &tmp)) {
> > +			size = FILL_REGS(guc, blob, i, true, 0, GUC_CAPTURE_LIST_TYPE_GLOBAL,
> > +					 "global", &gc->num_global_regs[i], tmp, &ptr,
> > +					 &blob->ads.capture_global[i], &ggtt, null_ggtt);
> > +			gc->global_list_size += size;
> > +			alloc_size += size;
> > +		} else {
> > +			gc->num_global_regs[i] = 0;
> > +			if (blob)
> > +				blob->ads.capture_global[i] = null_ggtt;
> >  		}
> > -
> > -		blob->ads.capture_global[i] = addr_ggtt;
> >  	}
> > +
> > +#undef COUNT_REGS
> > +#undef FILL_REGS
> > +#undef TYPE_CLASS
> > +#undef TYPE_INSTANCE
> > +
> > +	if (guc->ads_capture_size && guc->ads_capture_size != PAGE_ALIGN(alloc_size))
> > +		drm_warn(&i915->drm, "GuC->ADS->Capture alloc size changed from %d to %d\n",
> > +			 guc->ads_capture_size, PAGE_ALIGN(alloc_size));
> > +
> > +	return PAGE_ALIGN(alloc_size);
> >  }
> >  
> >  static void __guc_ads_init(struct intel_guc *guc)
> > @@ -614,6 +760,12 @@ int intel_guc_ads_create(struct intel_guc *guc)
> >  		return ret;
> >  	guc->ads_golden_ctxt_size = ret;
> >  
> > +	/* Likewise the capture lists: */
> > +	ret = guc_capture_prep_lists(guc, NULL);
> > +	if (ret < 0)
> > +		return ret;
> > +	guc->ads_capture_size = ret;
> > +
> >  	/* Now the total size can be determined: */
> >  	size = guc_ads_blob_size(guc);
> >  
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> > new file mode 100644
> > index 000000000000..c741c77b7fc8
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> > @@ -0,0 +1,232 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright © 2021-2021 Intel Corporation
> > + */
> > +
> > +#include <drm/drm_print.h>
> > +
> > +#include "i915_drv.h"
> > +#include "i915_drv.h"
> 
> duplicated include
> 
> > +#include "i915_memcpy.h"
> > +#include "gt/intel_gt.h"
> > +
> > +#include "intel_guc_fwif.h"
> > +#include "intel_guc_capture.h"
> > +
> > +/* Define all device tables of GuC error capture register lists */
> > +
> > +/********************************* Gen12 LP  *********************************/
> 
> didn't we move away from "GEN" naming ?
> 
> > +/************** GLOBAL *************/
> 
> do we really need all these decorations ?
> 
> > +struct __guc_mmio_reg_descr gen12lp_global_regs[] = {
> > +	{SWF_ILK(0),               0,      0, "SWF_ILK0"},
> > +	/* Add additional register list */
> 
> do we need this reminder ?
> 
> > +/********** List of lists **********/
> > +struct __guc_mmio_reg_descr_group gen12lp_lists[] = {
> > +	{
> > +		.list = gen12lp_global_regs,
> > +		.num_regs = (sizeof(gen12lp_global_regs) / sizeof(struct __guc_mmio_reg_descr)),
> 
> ARRAY_SIZE ?
> 
> > +/************ FIXME: Populate tables for other devices in subsequent patch ************/
> > +
> > +static struct __guc_mmio_reg_descr_group *
> > +guc_capture_get_device_reglist(struct drm_i915_private *dev_priv)
> 
> in new code we are using "i915" instead of "dev_priv" and since this
> function has "guc" prefix it shall rather take "guc" as param:
> 
> guc_capture_get_device_reglist(struct intel_guc *guc)
> {
> 	struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
> 	...
> 
> 
> > +static inline void
> > +warn_with_capture_list_identifier(struct drm_i915_private *i915, char *msg,
> > +				  u32 owner, u32 type, u32 classid)
> > +{
> > +	const char *ownerstr[GUC_CAPTURE_LIST_INDEX_MAX] = {"PF", "VF"};
> > +	const char *typestr[GUC_CAPTURE_LIST_TYPE_MAX - 1] = {"Class", "Instance"};
> > +	const char *classstr[GUC_LAST_ENGINE_CLASS + 1] = {"Render", "Video", "VideoEnhance",
> > +							   "Blitter", "Reserved"};
> 
> better to wrap that into simple small helpers like
> 
> 	const char *stringify_guc_capture_owner(u32 owner) { .. }
> 	const char *stringify_guc_capture_type(u32 type) { .. }
> 	const char *stringify_guc_capture_class(u32 class) { .. }
> 
> > +int intel_guc_capture_list_count(struct intel_guc *guc, u32 owner, u32 type, u32 classid,
> > +				 u16 *num_entries)
> > +{
> > +	struct drm_i915_private *dev_priv = (guc_to_gt(guc))->i915;
> 
> s/dev_priv/i915
> redundant ()
> 
> > +	struct __guc_mmio_reg_descr_group *reglists = guc->capture.reglists;
> > +	struct __guc_mmio_reg_descr_group *match;
> > +
> > +	if (!reglists)
> > +		return -ENODEV;
> > +
> > +	match = guc_capture_get_one_list(reglists, owner, type, classid);
> > +	if (match) {
> > +		*num_entries = match->num_regs;
> > +		return 0;
> 
> IIRC early returns are preferred for error cases, not success
> 
> > +int intel_guc_capture_list_init(struct intel_guc *guc, u32 owner, u32 type, u32 classid,
> > +				struct guc_mmio_reg *ptr, u16 num_entries)
> > +{
> > +	u32 j = 0;
> > +	struct drm_i915_private *dev_priv = (guc_to_gt(guc))->i915;
> 
> s/dev_priv/i915
> redundant ()
> 
> > +struct intel_guc;
> > +
> > +struct __guc_mmio_reg_descr {
> > +	i915_reg_t reg;
> > +	u32 flags;
> > +	u32 mask;
> > +	char *regname;
> 
> const char* ?
> 
> but maybe instead of adding reg name to the GuC specific struct we
> should add generic purpose function that will return pretty name of the
> register:
> 
> i915_reg.c:
> 
> const char *i915_reg_to_string(i915_reg_r reg)
> {
> 	...
> }
> 
> >  
> >  /* Capture-types of GuC capture register lists */
> > -enum
> > +enum guc_capture_owner
> >  {
> >  	GUC_CAPTURE_LIST_INDEX_PF = 0,
> >  	GUC_CAPTURE_LIST_INDEX_VF = 1,
> >  	GUC_CAPTURE_LIST_INDEX_MAX = 2,
> 
> s/INDEX/OWNER ?
> 
> >  };
> >

Alan Previn Nov. 24, 2021, 5:37 p.m. UTC | #5

Thanks very much Jani for the detail review of the code... apologies on some of the styling mishaps.
I will fix them all. I agree completely with the header file comments - my bad on that - had already
learnt that lesson on pxp side. Will fix accordingly.

...alan


On Wed, 2021-11-24 at 12:06 +0200, Jani Nikula wrote:
> On Mon, 22 Nov 2021, Alan Previn <[e-mail]> wrote:
> > +	{
> > +		.list = gen12lp_vec_class_regs,
> > +		.num_regs = (sizeof(gen12lp_vec_class_regs) / sizeof(struct __guc_mmio_reg_descr)),
> > +		.owner = GUC_CAPTURE_LIST_INDEX_PF,
> > +		.type = GUC_CAPTURE_LIST_TYPE_ENGINE_CLASS,
> > +		.engine = VIDEO_ENHANCEMENT_CLASS
> > +	},
> > +	{
> 
> Usually }, { on the same line
> 
> > +		.list = gen12lp_vec_inst_regs,
> > +		.num_regs = (sizeof(gen12lp_vec_inst_regs) / sizeof(struct __guc_mmio_reg_descr)),
> > +		.owner = GUC_CAPTURE_LIST_INDEX_PF,
> > +		.type = GUC_CAPTURE_LIST_TYPE_ENGINE_INSTANCE,
> > +		.engine = VIDEO_ENHANCEMENT_CLASS
> > +	},
> > +	{NULL, 0, 0, 0, 0}
> 
> Just {}  should work as a sentinel.
> 
> > +};
> > +
> > +/************ FIXME: Populate tables for other devices in subsequent patch ************/
> 
> Please don't add any of this ******* nonsense.
> 
> > +
> > +static struct __guc_mmio_reg_descr_group *
> > +guc_capture_get_device_reglist(struct drm_i915_private *dev_priv)
> > +{
> > +	if (IS_TIGERLAKE(dev_priv) || IS_ROCKETLAKE(dev_priv) ||
> > +	    IS_ALDERLAKE_S(dev_priv) || IS_ALDERLAKE_P(dev_priv)) {
> > +		return gen12lp_lists;
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> > +static inline struct __guc_mmio_reg_descr_group *
> > +guc_capture_get_one_list(struct __guc_mmio_reg_descr_group *reglists, u32 owner, u32 type, u32 id)
> 
> Please don't use inlines in .c files. Let the compiler decide.
> 
> > +{
> > +	int i = 0;
> > +
> > +	if (!reglists)
> > +		return NULL;
> > +	while (reglists[i].list) {
> > +		if (reglists[i].owner == owner &&
> > +		    reglists[i].type == type) {
> > +			if (reglists[i].type == GUC_CAPTURE_LIST_TYPE_GLOBAL ||
> > +			    reglists[i].engine == id) {
> > +				return &reglists[i];
> > +			}
> > +		}
> > +		++i;
> > +	}
> 
> That's a for loop right there.
> 
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.h
> > new file mode 100644
> > index 000000000000..352940b8bc87
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.h
> > @@ -0,0 +1,47 @@
> > +/* SPDX-License-Identifier: MIT */
> > +/*
> > + * Copyright © 2021-2021 Intel Corporation
> > + */
> > +
> > +#ifndef _INTEL_GUC_CAPTURE_H
> > +#define _INTEL_GUC_CAPTURE_H
> > +
> > +#include <linux/mutex.h>
> > +#include <linux/workqueue.h>
> 
> Both of these seem random and completely unnecessary. linux/types.h is
> required but it's not here.
> 
> > +#include "intel_guc_fwif.h"
> 
> I've been trying hard to reduce includes from headers throughout the
> driver, to clean up and clarify the interfaces and dependencies. I don't
> know how the guc headers have grown the kind of interdependencies that
> they all pull in almost everything.
> 
> This one line pulls in another 19 headers. Just to get
> GUC_CAPTURE_LIST_INDEX_MAX and GUC_MAX_ENGINE_CLASSES. Everything else
> could be solved through forward declarations.
> 
> BR,
> Jani.
> 
> 
> >  	struct guc_mmio_reg_set reg_state_list[GUC_MAX_ENGINE_CLASSES][GUC_MAX_INSTANCES_PER_CLASS];
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

Alan Previn Dec. 21, 2021, 11:15 p.m. UTC | #6

Michal, wrt this one:

+/************ FIXME: Populate tables for other devices in subsequent patch ************/
> > > +
> > > +static struct __guc_mmio_reg_descr_group *
> > > +guc_capture_get_device_reglist(struct drm_i915_private *dev_priv)
> > 
> > in new code we are using "i915" instead of "dev_priv" and since this
> > function has "guc" prefix it shall rather take "guc" as param:
> > 
> > guc_capture_get_device_reglist(struct intel_guc *guc)
> > {
> > 	struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
> > 	...
> > 

if its a static function that is only used internally, does the rule still apply?
I thought rules on primary handle inputs are for cross-i915-component interfaces that should start with an "intel_" in
front no? I will fix the others, but will keep static internal only functions the way they are (to avoid unnecessary de-
refencing).


...alan



On Wed, 2021-11-24 at 09:35 -0800, Alan Previn Teres Alexis wrote:
> Thanks Michal for the thorough review of the code (and the other patches). I will fix them all.
> 
> On the register-to-string helper function,
> i'll have to think it through because i do want to keep future development
> maintenance work when adding new registers simple (in the sense that
> adding a single line into the table will be all thats needed).
> 
> Unless you are suggesting keeping a global i915-wide list somewhere?
> which might be a bit of an overhead when searching through an offset list
> to find the mmio being requested for string return - unless i keep a sorted tree
> initialized with registers ordered by address, but would not work well for
> different registers that share addresses on diff gen's).
> 
> 
> ...alan
> 
> 
> On Tue, 2021-11-23 at 22:46 +0100, Michal Wajdeczko wrote:
> > Hi,
> > 
> > just few random nits below
> > 
> > -Michal
> > 
> > 
> > On 23.11.2021 00:03, Alan Previn wrote:
> > > Update GuC ADS size allocation to include space for
> > > the lists of error state capture register descriptors.
> > > 
> > > Also, populate the lists of registers we want GuC to report back to
> > > Host on engine reset events. This list should include global,
> > > engine-class and engine-instance registers for every engine-class
> > > type on the current hardware.
> > > 
> > > NOTE: Start with a fake table of register lists to layout the
> > > framework before adding real registers in subsequent patch.
> > > 
> > > Signed-off-by: Alan Previn <[e-mail]>
> > > ---
> > >  drivers/gpu/drm/i915/Makefile                 |   1 +
> > >  drivers/gpu/drm/i915/gt/uc/intel_guc.c        |  10 +-
> > >  drivers/gpu/drm/i915/gt/uc/intel_guc.h        |   5 +
> > >  drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c    | 176 ++++++++++++-
> > >  .../gpu/drm/i915/gt/uc/intel_guc_capture.c    | 232 ++++++++++++++++++
> > >  .../gpu/drm/i915/gt/uc/intel_guc_capture.h    |  47 ++++
> > >  drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h   |  19 +-
> > >  7 files changed, 476 insertions(+), 14 deletions(-)
> > >  create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> > >  create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_capture.h
> > >

Alan Previn Dec. 22, 2021, 1:49 a.m. UTC | #7

Hi Michal - wrt to this comment:

+struct intel_guc;
> > > +
> > > +struct __guc_mmio_reg_descr {
> > > +	i915_reg_t reg;
> > > +	u32 flags;
> > > +	u32 mask;
> > > +	char *regname;
> > 
> > const char* ?
> > 
> > but maybe instead of adding reg name to the GuC specific struct we
> > should add generic purpose function that will return pretty name of the
> > register:
> > 
> > i915_reg.c:
> > 
> > const char *i915_reg_to_string(i915_reg_r reg)
> > {
> > 	...
> > }
> > 

I dont think this will scale if we have different generation hardware with different register names but same offset.
I checked that this has happenned in the past. Additionally, it would mean anyone adding a new register in what could
some day be a pretty long list, would have to update 2 locations.

If its okay with you, i think i would stick with current version for this specific hunk. (am fixing all the rest ofc).



...alan


On Wed, 2021-11-24 at 09:35 -0800, Alan Previn Teres Alexis wrote:
> Thanks Michal for the thorough review of the code (and the other patches). I will fix them all.
> 
> On the register-to-string helper function,
> i'll have to think it through because i do want to keep future development
> maintenance work when adding new registers simple (in the sense that
> adding a single line into the table will be all thats needed).
> 
> Unless you are suggesting keeping a global i915-wide list somewhere?
> which might be a bit of an overhead when searching through an offset list
> to find the mmio being requested for string return - unless i keep a sorted tree
> initialized with registers ordered by address, but would not work well for
> different registers that share addresses on diff gen's).
> 
> 
> ...alan
> 
> 
> On Tue, 2021-11-23 at 22:46 +0100, Michal Wajdeczko wrote:
> > Hi,
> > 
> > just few random nits below
> > 
> > -Michal
> > 
> > 
> > On 23.11.2021 00:03, Alan Previn wrote:
> > > Update GuC ADS size allocation to include space for
> > > the lists of error state capture register descriptors.
> > > 
> > > Also, populate the lists of registers we want GuC to report back to
> > > Host on engine reset events. This list should include global,
> > > engine-class and engine-instance registers for every engine-class
> > > type on the current hardware.
> > > 
> > > NOTE: Start with a fake table of register lists to layout the
> > > framework before adding real registers in subsequent patch.
> > > 
> > > Signed-off-by: Alan Previn <[e-mail]>
> > > ---
> > >  drivers/gpu/drm/i915/Makefile                 |   1 +
> > >  drivers/gpu/drm/i915/gt/uc/intel_guc.c        |  10 +-
> > >  drivers/gpu/drm/i915/gt/uc/intel_guc.h        |   5 +
> > >  drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c    | 176 ++++++++++++-
> > >  .../gpu/drm/i915/gt/uc/intel_guc_capture.c    | 232 ++++++++++++++++++
> > >  .../gpu/drm/i915/gt/uc/intel_guc_capture.h    |  47 ++++
> > >  drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h   |  19 +-
> > >  7 files changed, 476 insertions(+), 14 deletions(-)
> > >  create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> > >  create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_capture.h
> > > 
> > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> > > index 074d6b8edd23..e3c4d5cea4c3 100644
> > > --- a/drivers/gpu/drm/i915/Makefile
> > > +++ b/drivers/gpu/drm/i915/Makefile
> > > @@ -190,6 +190,7 @@ i915-y += gt/uc/intel_uc.o \
> > >  	  gt/uc/intel_guc_rc.o \
> > >  	  gt/uc/intel_guc_slpc.o \
> > >  	  gt/uc/intel_guc_submission.o \
> > > +	  gt/uc/intel_guc_capture.o \
> > 
> > use alphabetical order
> > 
> > >  	  gt/uc/intel_huc.o \
> > >  	  gt/uc/intel_huc_debugfs.o \
> > >  	  gt/uc/intel_huc_fw.o
> > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> > > index 5cf9ebd2ee55..458f0d248a5a 100644
> > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> > > @@ -335,9 +335,14 @@ int intel_guc_init(struct intel_guc *guc)
> > >  	if (ret)
> > >  		goto err_fw;
> > >  
> > > -	ret = intel_guc_ads_create(guc);
> > > +	ret = intel_guc_capture_init(guc);
> > >  	if (ret)
> > >  		goto err_log;
> > > +
> > > +	ret = intel_guc_ads_create(guc);
> > > +	if (ret)
> > > +		goto err_capture;
> > > +
> > >  	GEM_BUG_ON(!guc->ads_vma);
> > >  
> > >  	ret = intel_guc_ct_init(&guc->ct);
> > > @@ -376,6 +381,8 @@ int intel_guc_init(struct intel_guc *guc)
> > >  	intel_guc_ct_fini(&guc->ct);
> > >  err_ads:
> > >  	intel_guc_ads_destroy(guc);
> > > +err_capture:
> > > +	intel_guc_capture_destroy(guc);
> > >  err_log:
> > >  	intel_guc_log_destroy(&guc->log);
> > >  err_fw:
> > > @@ -403,6 +410,7 @@ void intel_guc_fini(struct intel_guc *guc)
> > >  	intel_guc_ct_fini(&guc->ct);
> > >  
> > >  	intel_guc_ads_destroy(guc);
> > > +	intel_guc_capture_destroy(guc);
> > >  	intel_guc_log_destroy(&guc->log);
> > >  	intel_uc_fw_fini(&guc->fw);
> > >  }
> > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > > index 9de99772f916..d136c69abe12 100644
> > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > > @@ -16,6 +16,7 @@
> > >  #include "intel_guc_log.h"
> > >  #include "intel_guc_reg.h"
> > >  #include "intel_guc_slpc_types.h"
> > > +#include "intel_guc_capture.h"
> > 
> > use alphabetical order
> > 
> > >  #include "intel_uc_fw.h"
> > >  #include "i915_utils.h"
> > >  #include "i915_vma.h"
> > > @@ -37,6 +38,8 @@ struct intel_guc {
> > >  	struct intel_guc_ct ct;
> > >  	/** @slpc: sub-structure containing SLPC related data and objects */
> > >  	struct intel_guc_slpc slpc;
> > > +	/** @capture: the error-state-capture module's data and objects */
> > > +	struct intel_guc_state_capture capture;
> > >  
> > >  	/** @sched_engine: Global engine used to submit requests to GuC */
> > >  	struct i915_sched_engine *sched_engine;
> > > @@ -138,6 +141,8 @@ struct intel_guc {
> > >  	u32 ads_regset_size;
> > >  	/** @ads_golden_ctxt_size: size of the golden contexts in the ADS */
> > >  	u32 ads_golden_ctxt_size;
> > > +	/** @ads_capture_size: size of register lists in the ADS used for error capture */
> > > +	u32 ads_capture_size;
> > >  	/** @ads_engine_usage_size: size of engine usage in the ADS */
> > >  	u32 ads_engine_usage_size;
> > >  
> > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> > > index 6c81ddd303d3..2780c0fadd01 100644
> > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> > > @@ -10,6 +10,7 @@
> > >  #include "gt/shmem_utils.h"
> > >  #include "intel_guc_ads.h"
> > >  #include "intel_guc_fwif.h"
> > > +#include "intel_guc_capture.h"
> > 
> > wrong order
> > 
> > >  #include "intel_uc.h"
> > >  #include "i915_drv.h"
> > >  
> > > @@ -71,8 +72,7 @@ static u32 guc_ads_golden_ctxt_size(struct intel_guc *guc)
> > >  
> > >  static u32 guc_ads_capture_size(struct intel_guc *guc)
> > >  {
> > > -	/* Basic support to init ADS without a proper GuC error capture list */
> > > -	return PAGE_ALIGN(PAGE_SIZE);
> > > +	return PAGE_ALIGN(guc->ads_capture_size);
> > >  }
> > >  
> > >  static u32 guc_ads_private_data_size(struct intel_guc *guc)
> > > @@ -519,24 +519,170 @@ static void guc_init_golden_context(struct intel_guc *guc)
> > >  	GEM_BUG_ON(guc->ads_golden_ctxt_size != total_size);
> > >  }
> > >  
> > > -static void guc_capture_prep_lists(struct intel_guc *guc, struct __guc_ads_blob *blob)
> > > +static int
> > > +guc_fill_reglist(struct intel_guc *guc, struct __guc_ads_blob *blob, int vf, bool enabled,
> > > +		 int classid, int type, char *typename, u16 *p_numregs, int newnum, u8 **p_virt_ptr,
> > > +		 u32 *p_blobptr_to_ggtt, u32 *p_ggtt, u32 null_ggtt)
> > 
> > hmm, this does not look good - do we really need all these params ?
> > 
> > >  {
> > > -	int i, j;
> > > -	u32 addr_ggtt, offset;
> > > +	struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
> > > +	struct guc_debug_capture_list *listnode;
> > > +	int size = 0;
> > >  
> > > -	offset = guc_ads_capture_offset(guc);
> > > -	addr_ggtt = intel_guc_ggtt_offset(guc, guc->ads_vma) + offset;
> > > +	if (blob && *p_numregs != newnum) {
> > > +		if (type == GUC_CAPTURE_LIST_TYPE_GLOBAL)
> > > +			drm_warn(&i915->drm, "Guc-Cap VF%d-%s num-reg mismatch was=%d now=%d!\n",
> > > +				 vf, typename, *p_numregs, newnum);
> > > +		else
> > > +			drm_warn(&i915->drm, "Guc-Cap VF%d-Class-%d-%s num-reg mismatch was=%d now=%d!\n",
> > > +				 vf, classid, typename, *p_numregs, newnum);
> > > +	}
> > > +	/*
> > > +	 * For enabled capture lists, we not only need to call capture module to help
> > > +	 * populate the list-descriptor into the correct ads capture structures, but
> > > +	 * we also need to increment the virtual pointers and ggtt offsets so that
> > > +	 * caller has the subsequent gfx memory location.
> > > +	 */
> > > +	*p_numregs = newnum;
> > > +	size = PAGE_ALIGN((sizeof(struct guc_debug_capture_list)) +
> > > +			  (newnum * sizeof(struct guc_mmio_reg)));
> > > +	/* if caller hasn't allocated ADS blob, return size and counts, we're done */
> > > +	if (!blob)
> > > +		return size;
> > > +	if (blob) {
> > 
> > redundant
> > 
> > > +		/* if caller allocated ADS blob, populate the capture register descriptors */
> > > +		if (!newnum) {
> > > +			*p_blobptr_to_ggtt = null_ggtt;
> > > +		} else {
> > > +			/* get ptr and populate header info: */
> > > +			*p_blobptr_to_ggtt = *p_ggtt;
> > > +			listnode = (struct guc_debug_capture_list *)*p_virt_ptr;
> > > +			*p_ggtt += sizeof(struct guc_debug_capture_list);
> > > +			*p_virt_ptr += sizeof(struct guc_debug_capture_list);
> > > +			listnode->header.info = FIELD_PREP(GUC_CAPTURELISTHDR_NUMDESCR, *p_numregs);
> > > +
> > > +			/* get ptr and populate register descriptor list: */
> > > +			intel_guc_capture_list_init(guc, vf, type, classid,
> > > +						    (struct guc_mmio_reg *)*p_virt_ptr,
> > > +						    *p_numregs);
> > > +
> > > +			/* increment ptrs for that header: */
> > > +			*p_ggtt += size - sizeof(struct guc_debug_capture_list);
> > > +			*p_virt_ptr += size - sizeof(struct guc_debug_capture_list);
> > > +		}
> > > +	}
> > > +
> > > +	return size;
> > > +}
> > > +
> > > +static int guc_capture_prep_lists(struct intel_guc *guc, struct __guc_ads_blob *blob)
> > > +{
> > > +	struct intel_gt *gt = guc_to_gt(guc);
> > > +	int i, j, size;
> > > +	u32 ggtt, null_ggtt, offset, alloc_size = 0;
> > > +	struct guc_gt_system_info *info, local_info;
> > > +	struct guc_debug_capture_list *listnode;
> > > +	struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
> > > +	struct intel_guc_state_capture *gc = &guc->capture;
> > > +	u16 tmp = 0;
> > > +	u8 *ptr = NULL;
> > > +
> > > +	if (blob) {
> > > +		offset = guc_ads_capture_offset(guc);
> > > +		ggtt = intel_guc_ggtt_offset(guc, guc->ads_vma) + offset;
> > > +		ptr = ((u8 *)blob) + offset;
> > > +		info = &blob->system_info;
> > > +	} else {
> > > +		memset(&local_info, 0, sizeof(local_info));
> > > +		info = &local_info;
> > > +		fill_engine_enable_masks(gt, info);
> > > +	}
> > > +
> > > +	/* first, set aside the first page for a capture_list with zero descriptors */
> > > +	alloc_size = PAGE_SIZE;
> > > +	if (blob) {
> > > +		listnode = (struct guc_debug_capture_list *)ptr;
> > > +		listnode->header.info = FIELD_PREP(GUC_CAPTURELISTHDR_NUMDESCR, 0);
> > > +		null_ggtt = ggtt;
> > > +		ggtt += PAGE_SIZE;
> > > +		ptr +=  PAGE_SIZE;
> > > +	}
> > >  
> > > -	/* FIXME: Populate a proper capture list */
> > > +#define COUNT_REGS intel_guc_capture_list_count
> > > +#define FILL_REGS guc_fill_reglist
> > > +#define TYPE_CLASS GUC_CAPTURE_LIST_TYPE_ENGINE_CLASS
> > > +#define TYPE_INSTANCE GUC_CAPTURE_LIST_TYPE_ENGINE_INSTANCE
> > >  
> > >  	for (i = 0; i < GUC_CAPTURE_LIST_INDEX_MAX; i++) {
> > >  		for (j = 0; j < GUC_MAX_ENGINE_CLASSES; j++) {
> > > -			blob->ads.capture_instance[i][j] = addr_ggtt;
> > > -			blob->ads.capture_class[i][j] = addr_ggtt;
> > > +			if (!info->engine_enabled_masks[j]) {
> > > +				if (gc->num_class_regs[i][j])
> > > +					drm_warn(&i915->drm, "GuC-Cap VF%d-class-%d "
> > > +						 "class regs valid mismatch was=%d now=%d!\n",
> > > +						 i, j, gc->num_class_regs[i][j], tmp);
> > > +				if (gc->num_instance_regs[i][j])
> > > +					drm_warn(&i915->drm, "GuC-Cap VF%d-class-%d "
> > > +						 "inst regs valid mismatch was=%d now=%d!\n",
> > > +						 i, j, gc->num_instance_regs[i][j], tmp);
> > > +				gc->num_class_regs[i][j] = 0;
> > > +				gc->num_instance_regs[i][j] = 0;
> > > +				if (blob) {
> > > +					blob->ads.capture_class[i][j] = null_ggtt;
> > > +					blob->ads.capture_instance[i][j] = null_ggtt;
> > > +				}
> > > +			} else {
> > > +				if (!COUNT_REGS(guc, i, TYPE_CLASS,
> > > +						guc_class_to_engine_class(j), &tmp)) {
> > > +					size = FILL_REGS(guc, blob, i, true, j, TYPE_CLASS,
> > > +							 "class", &gc->num_class_regs[i][j],
> > > +							 tmp, &ptr,
> > > +							 &blob->ads.capture_class[i][j],
> > > +							 &ggtt, null_ggtt);
> > > +					gc->class_list_size += size;
> > > +					alloc_size += size;
> > > +				} else {
> > > +					gc->num_class_regs[i][j] = 0;
> > > +					if (blob)
> > > +						blob->ads.capture_class[i][j] = null_ggtt;
> > > +				}
> > > +				if (!COUNT_REGS(guc, i, TYPE_INSTANCE,
> > > +						guc_class_to_engine_class(j), &tmp)) {
> > > +					size = FILL_REGS(guc, blob, i, true, j, TYPE_INSTANCE,
> > > +							 "instance", &gc->num_instance_regs[i][j],
> > > +							 tmp, &ptr,
> > > +							 &blob->ads.capture_instance[i][j],
> > > +							 &ggtt, null_ggtt);
> > > +					gc->instance_list_size += size;
> > > +					alloc_size += size;
> > > +				} else {
> > > +					gc->num_instance_regs[i][j] = 0;
> > > +					if (blob)
> > > +						blob->ads.capture_instance[i][j] = null_ggtt;
> > > +				}
> > > +			}
> > > +		}
> > > +		if (!COUNT_REGS(guc, i, GUC_CAPTURE_LIST_TYPE_GLOBAL, 0, &tmp)) {
> > > +			size = FILL_REGS(guc, blob, i, true, 0, GUC_CAPTURE_LIST_TYPE_GLOBAL,
> > > +					 "global", &gc->num_global_regs[i], tmp, &ptr,
> > > +					 &blob->ads.capture_global[i], &ggtt, null_ggtt);
> > > +			gc->global_list_size += size;
> > > +			alloc_size += size;
> > > +		} else {
> > > +			gc->num_global_regs[i] = 0;
> > > +			if (blob)
> > > +				blob->ads.capture_global[i] = null_ggtt;
> > >  		}
> > > -
> > > -		blob->ads.capture_global[i] = addr_ggtt;
> > >  	}
> > > +
> > > +#undef COUNT_REGS
> > > +#undef FILL_REGS
> > > +#undef TYPE_CLASS
> > > +#undef TYPE_INSTANCE
> > > +
> > > +	if (guc->ads_capture_size && guc->ads_capture_size != PAGE_ALIGN(alloc_size))
> > > +		drm_warn(&i915->drm, "GuC->ADS->Capture alloc size changed from %d to %d\n",
> > > +			 guc->ads_capture_size, PAGE_ALIGN(alloc_size));
> > > +
> > > +	return PAGE_ALIGN(alloc_size);
> > >  }
> > >  
> > >  static void __guc_ads_init(struct intel_guc *guc)
> > > @@ -614,6 +760,12 @@ int intel_guc_ads_create(struct intel_guc *guc)
> > >  		return ret;
> > >  	guc->ads_golden_ctxt_size = ret;
> > >  
> > > +	/* Likewise the capture lists: */
> > > +	ret = guc_capture_prep_lists(guc, NULL);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +	guc->ads_capture_size = ret;
> > > +
> > >  	/* Now the total size can be determined: */
> > >  	size = guc_ads_blob_size(guc);
> > >  
> > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> > > new file mode 100644
> > > index 000000000000..c741c77b7fc8
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> > > @@ -0,0 +1,232 @@
> > > +// SPDX-License-Identifier: MIT
> > > +/*
> > > + * Copyright © 2021-2021 Intel Corporation
> > > + */
> > > +
> > > +#include <drm/drm_print.h>
> > > +
> > > +#include "i915_drv.h"
> > > +#include "i915_drv.h"
> > 
> > duplicated include
> > 
> > > +#include "i915_memcpy.h"
> > > +#include "gt/intel_gt.h"
> > > +
> > > +#include "intel_guc_fwif.h"
> > > +#include "intel_guc_capture.h"
> > > +
> > > +/* Define all device tables of GuC error capture register lists */
> > > +
> > > +/********************************* Gen12 LP  *********************************/
> > 
> > didn't we move away from "GEN" naming ?
> > 
> > > +/************** GLOBAL *************/
> > 
> > do we really need all these decorations ?
> > 
> > > +struct __guc_mmio_reg_descr gen12lp_global_regs[] = {
> > > +	{SWF_ILK(0),               0,      0, "SWF_ILK0"},
> > > +	/* Add additional register list */
> > 
> > do we need this reminder ?
> > 
> > > +/********** List of lists **********/
> > > +struct __guc_mmio_reg_descr_group gen12lp_lists[] = {
> > > +	{
> > > +		.list = gen12lp_global_regs,
> > > +		.num_regs = (sizeof(gen12lp_global_regs) / sizeof(struct __guc_mmio_reg_descr)),
> > 
> > ARRAY_SIZE ?
> > 
> > > +/************ FIXME: Populate tables for other devices in subsequent patch ************/
> > > +
> > > +static struct __guc_mmio_reg_descr_group *
> > > +guc_capture_get_device_reglist(struct drm_i915_private *dev_priv)
> > 
> > in new code we are using "i915" instead of "dev_priv" and since this
> > function has "guc" prefix it shall rather take "guc" as param:
> > 
> > guc_capture_get_device_reglist(struct intel_guc *guc)
> > {
> > 	struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
> > 	...
> > 
> > 
> > > +static inline void
> > > +warn_with_capture_list_identifier(struct drm_i915_private *i915, char *msg,
> > > +				  u32 owner, u32 type, u32 classid)
> > > +{
> > > +	const char *ownerstr[GUC_CAPTURE_LIST_INDEX_MAX] = {"PF", "VF"};
> > > +	const char *typestr[GUC_CAPTURE_LIST_TYPE_MAX - 1] = {"Class", "Instance"};
> > > +	const char *classstr[GUC_LAST_ENGINE_CLASS + 1] = {"Render", "Video", "VideoEnhance",
> > > +							   "Blitter", "Reserved"};
> > 
> > better to wrap that into simple small helpers like
> > 
> > 	const char *stringify_guc_capture_owner(u32 owner) { .. }
> > 	const char *stringify_guc_capture_type(u32 type) { .. }
> > 	const char *stringify_guc_capture_class(u32 class) { .. }
> > 
> > > +int intel_guc_capture_list_count(struct intel_guc *guc, u32 owner, u32 type, u32 classid,
> > > +				 u16 *num_entries)
> > > +{
> > > +	struct drm_i915_private *dev_priv = (guc_to_gt(guc))->i915;
> > 
> > s/dev_priv/i915
> > redundant ()
> > 
> > > +	struct __guc_mmio_reg_descr_group *reglists = guc->capture.reglists;
> > > +	struct __guc_mmio_reg_descr_group *match;
> > > +
> > > +	if (!reglists)
> > > +		return -ENODEV;
> > > +
> > > +	match = guc_capture_get_one_list(reglists, owner, type, classid);
> > > +	if (match) {
> > > +		*num_entries = match->num_regs;
> > > +		return 0;
> > 
> > IIRC early returns are preferred for error cases, not success
> > 
> > > +int intel_guc_capture_list_init(struct intel_guc *guc, u32 owner, u32 type, u32 classid,
> > > +				struct guc_mmio_reg *ptr, u16 num_entries)
> > > +{
> > > +	u32 j = 0;
> > > +	struct drm_i915_private *dev_priv = (guc_to_gt(guc))->i915;
> > 
> > s/dev_priv/i915
> > redundant ()
> > 
> > > +struct intel_guc;
> > > +
> > > +struct __guc_mmio_reg_descr {
> > > +	i915_reg_t reg;
> > > +	u32 flags;
> > > +	u32 mask;
> > > +	char *regname;
> > 
> > const char* ?
> > 
> > but maybe instead of adding reg name to the GuC specific struct we
> > should add generic purpose function that will return pretty name of the
> > register:
> > 
> > i915_reg.c:
> > 
> > const char *i915_reg_to_string(i915_reg_r reg)
> > {
> > 	...
> > }
> > 
> > >  
> > >  /* Capture-types of GuC capture register lists */
> > > -enum
> > > +enum guc_capture_owner
> > >  {
> > >  	GUC_CAPTURE_LIST_INDEX_PF = 0,
> > >  	GUC_CAPTURE_LIST_INDEX_VF = 1,
> > >  	GUC_CAPTURE_LIST_INDEX_MAX = 2,
> > 
> > s/INDEX/OWNER ?
> > 
> > >  };
> > >

Alan Previn Dec. 22, 2021, 8:13 p.m. UTC | #8

Michal, WRT below, feedback from other developers is match spec names.
(i.e. means other struct names introduced in the series needs minor touch-ups).

>  
>  /* Capture-types of GuC capture register lists */ -enum
> +enum guc_capture_owner
>  {
>  	GUC_CAPTURE_LIST_INDEX_PF = 0,
>  	GUC_CAPTURE_LIST_INDEX_VF = 1,
>  	GUC_CAPTURE_LIST_INDEX_MAX = 2,

s/INDEX/OWNER ?

-----Original Message-----
From: Wajdeczko, Michal <[e-mail]> 
Sent: Tuesday, November 23, 2021 1:47 PM
To: Teres Alexis, Alan Previn <[e-mail]>; [e-mail]
Subject: Re: [Intel-gfx] [RFC 2/7] drm/i915/guc: Update GuC ADS size for error capture lists

Hi,

just few random nits below

-Michal


On 23.11.2021 00:03, Alan Previn wrote:
> Update GuC ADS size allocation to include space for the lists of error 
> state capture register descriptors.
> 
> Also, populate the lists of registers we want GuC to report back to 
> Host on engine reset events. This list should include global, 
> engine-class and engine-instance registers for every engine-class type 
> on the current hardware.
> 
> NOTE: Start with a fake table of register lists to layout the 
> framework before adding real registers in subsequent patch.
> 
> Signed-off-by: Alan Previn <[e-mail]>
> ---
>  drivers/gpu/drm/i915/Makefile                 |   1 +
>  drivers/gpu/drm/i915/gt/uc/intel_guc.c        |  10 +-
>  drivers/gpu/drm/i915/gt/uc/intel_guc.h        |   5 +
>  drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c    | 176 ++++++++++++-
>  .../gpu/drm/i915/gt/uc/intel_guc_capture.c    | 232 ++++++++++++++++++
>  .../gpu/drm/i915/gt/uc/intel_guc_capture.h    |  47 ++++
>  drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h   |  19 +-
>  7 files changed, 476 insertions(+), 14 deletions(-)  create mode 
> 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
>  create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_capture.h
> 
> diff --git a/drivers/gpu/drm/i915/Makefile 
> b/drivers/gpu/drm/i915/Makefile index 074d6b8edd23..e3c4d5cea4c3 
> 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -190,6 +190,7 @@ i915-y += gt/uc/intel_uc.o \
>  	  gt/uc/intel_guc_rc.o \
>  	  gt/uc/intel_guc_slpc.o \
>  	  gt/uc/intel_guc_submission.o \
> +	  gt/uc/intel_guc_capture.o \

use alphabetical order

>  	  gt/uc/intel_huc.o \
>  	  gt/uc/intel_huc_debugfs.o \
>  	  gt/uc/intel_huc_fw.o
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c 
> b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> index 5cf9ebd2ee55..458f0d248a5a 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> @@ -335,9 +335,14 @@ int intel_guc_init(struct intel_guc *guc)
>  	if (ret)
>  		goto err_fw;
>  
> -	ret = intel_guc_ads_create(guc);
> +	ret = intel_guc_capture_init(guc);
>  	if (ret)
>  		goto err_log;
> +
> +	ret = intel_guc_ads_create(guc);
> +	if (ret)
> +		goto err_capture;
> +
>  	GEM_BUG_ON(!guc->ads_vma);
>  
>  	ret = intel_guc_ct_init(&guc->ct);
> @@ -376,6 +381,8 @@ int intel_guc_init(struct intel_guc *guc)
>  	intel_guc_ct_fini(&guc->ct);
>  err_ads:
>  	intel_guc_ads_destroy(guc);
> +err_capture:
> +	intel_guc_capture_destroy(guc);
>  err_log:
>  	intel_guc_log_destroy(&guc->log);
>  err_fw:
> @@ -403,6 +410,7 @@ void intel_guc_fini(struct intel_guc *guc)
>  	intel_guc_ct_fini(&guc->ct);
>  
>  	intel_guc_ads_destroy(guc);
> +	intel_guc_capture_destroy(guc);
>  	intel_guc_log_destroy(&guc->log);
>  	intel_uc_fw_fini(&guc->fw);
>  }
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h 
> b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> index 9de99772f916..d136c69abe12 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> @@ -16,6 +16,7 @@
>  #include "intel_guc_log.h"
>  #include "intel_guc_reg.h"
>  #include "intel_guc_slpc_types.h"
> +#include "intel_guc_capture.h"

use alphabetical order

>  #include "intel_uc_fw.h"
>  #include "i915_utils.h"
>  #include "i915_vma.h"
> @@ -37,6 +38,8 @@ struct intel_guc {
>  	struct intel_guc_ct ct;
>  	/** @slpc: sub-structure containing SLPC related data and objects */
>  	struct intel_guc_slpc slpc;
> +	/** @capture: the error-state-capture module's data and objects */
> +	struct intel_guc_state_capture capture;
>  
>  	/** @sched_engine: Global engine used to submit requests to GuC */
>  	struct i915_sched_engine *sched_engine; @@ -138,6 +141,8 @@ struct 
> intel_guc {
>  	u32 ads_regset_size;
>  	/** @ads_golden_ctxt_size: size of the golden contexts in the ADS */
>  	u32 ads_golden_ctxt_size;
> +	/** @ads_capture_size: size of register lists in the ADS used for error capture */
> +	u32 ads_capture_size;
>  	/** @ads_engine_usage_size: size of engine usage in the ADS */
>  	u32 ads_engine_usage_size;
>  
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c 
> b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> index 6c81ddd303d3..2780c0fadd01 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> @@ -10,6 +10,7 @@
>  #include "gt/shmem_utils.h"
>  #include "intel_guc_ads.h"
>  #include "intel_guc_fwif.h"
> +#include "intel_guc_capture.h"

wrong order

>  #include "intel_uc.h"
>  #include "i915_drv.h"
>  
> @@ -71,8 +72,7 @@ static u32 guc_ads_golden_ctxt_size(struct intel_guc 
> *guc)
>  
>  static u32 guc_ads_capture_size(struct intel_guc *guc)  {
> -	/* Basic support to init ADS without a proper GuC error capture list */
> -	return PAGE_ALIGN(PAGE_SIZE);
> +	return PAGE_ALIGN(guc->ads_capture_size);
>  }
>  
>  static u32 guc_ads_private_data_size(struct intel_guc *guc) @@ 
> -519,24 +519,170 @@ static void guc_init_golden_context(struct intel_guc *guc)
>  	GEM_BUG_ON(guc->ads_golden_ctxt_size != total_size);  }
>  
> -static void guc_capture_prep_lists(struct intel_guc *guc, struct 
> __guc_ads_blob *blob)
> +static int
> +guc_fill_reglist(struct intel_guc *guc, struct __guc_ads_blob *blob, int vf, bool enabled,
> +		 int classid, int type, char *typename, u16 *p_numregs, int newnum, u8 **p_virt_ptr,
> +		 u32 *p_blobptr_to_ggtt, u32 *p_ggtt, u32 null_ggtt)

hmm, this does not look good - do we really need all these params ?

>  {
> -	int i, j;
> -	u32 addr_ggtt, offset;
> +	struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
> +	struct guc_debug_capture_list *listnode;
> +	int size = 0;
>  
> -	offset = guc_ads_capture_offset(guc);
> -	addr_ggtt = intel_guc_ggtt_offset(guc, guc->ads_vma) + offset;
> +	if (blob && *p_numregs != newnum) {
> +		if (type == GUC_CAPTURE_LIST_TYPE_GLOBAL)
> +			drm_warn(&i915->drm, "Guc-Cap VF%d-%s num-reg mismatch was=%d now=%d!\n",
> +				 vf, typename, *p_numregs, newnum);
> +		else
> +			drm_warn(&i915->drm, "Guc-Cap VF%d-Class-%d-%s num-reg mismatch was=%d now=%d!\n",
> +				 vf, classid, typename, *p_numregs, newnum);
> +	}
> +	/*
> +	 * For enabled capture lists, we not only need to call capture module to help
> +	 * populate the list-descriptor into the correct ads capture structures, but
> +	 * we also need to increment the virtual pointers and ggtt offsets so that
> +	 * caller has the subsequent gfx memory location.
> +	 */
> +	*p_numregs = newnum;
> +	size = PAGE_ALIGN((sizeof(struct guc_debug_capture_list)) +
> +			  (newnum * sizeof(struct guc_mmio_reg)));
> +	/* if caller hasn't allocated ADS blob, return size and counts, we're done */
> +	if (!blob)
> +		return size;
> +	if (blob) {

redundant

> +		/* if caller allocated ADS blob, populate the capture register descriptors */
> +		if (!newnum) {
> +			*p_blobptr_to_ggtt = null_ggtt;
> +		} else {
> +			/* get ptr and populate header info: */
> +			*p_blobptr_to_ggtt = *p_ggtt;
> +			listnode = (struct guc_debug_capture_list *)*p_virt_ptr;
> +			*p_ggtt += sizeof(struct guc_debug_capture_list);
> +			*p_virt_ptr += sizeof(struct guc_debug_capture_list);
> +			listnode->header.info = FIELD_PREP(GUC_CAPTURELISTHDR_NUMDESCR, 
> +*p_numregs);
> +
> +			/* get ptr and populate register descriptor list: */
> +			intel_guc_capture_list_init(guc, vf, type, classid,
> +						    (struct guc_mmio_reg *)*p_virt_ptr,
> +						    *p_numregs);
> +
> +			/* increment ptrs for that header: */
> +			*p_ggtt += size - sizeof(struct guc_debug_capture_list);
> +			*p_virt_ptr += size - sizeof(struct guc_debug_capture_list);
> +		}
> +	}
> +
> +	return size;
> +}
> +
> +static int guc_capture_prep_lists(struct intel_guc *guc, struct 
> +__guc_ads_blob *blob) {
> +	struct intel_gt *gt = guc_to_gt(guc);
> +	int i, j, size;
> +	u32 ggtt, null_ggtt, offset, alloc_size = 0;
> +	struct guc_gt_system_info *info, local_info;
> +	struct guc_debug_capture_list *listnode;
> +	struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
> +	struct intel_guc_state_capture *gc = &guc->capture;
> +	u16 tmp = 0;
> +	u8 *ptr = NULL;
> +
> +	if (blob) {
> +		offset = guc_ads_capture_offset(guc);
> +		ggtt = intel_guc_ggtt_offset(guc, guc->ads_vma) + offset;
> +		ptr = ((u8 *)blob) + offset;
> +		info = &blob->system_info;
> +	} else {
> +		memset(&local_info, 0, sizeof(local_info));
> +		info = &local_info;
> +		fill_engine_enable_masks(gt, info);
> +	}
> +
> +	/* first, set aside the first page for a capture_list with zero descriptors */
> +	alloc_size = PAGE_SIZE;
> +	if (blob) {
> +		listnode = (struct guc_debug_capture_list *)ptr;
> +		listnode->header.info = FIELD_PREP(GUC_CAPTURELISTHDR_NUMDESCR, 0);
> +		null_ggtt = ggtt;
> +		ggtt += PAGE_SIZE;
> +		ptr +=  PAGE_SIZE;
> +	}
>  
> -	/* FIXME: Populate a proper capture list */
> +#define COUNT_REGS intel_guc_capture_list_count #define FILL_REGS 
> +guc_fill_reglist #define TYPE_CLASS 
> +GUC_CAPTURE_LIST_TYPE_ENGINE_CLASS
> +#define TYPE_INSTANCE GUC_CAPTURE_LIST_TYPE_ENGINE_INSTANCE
>  
>  	for (i = 0; i < GUC_CAPTURE_LIST_INDEX_MAX; i++) {
>  		for (j = 0; j < GUC_MAX_ENGINE_CLASSES; j++) {
> -			blob->ads.capture_instance[i][j] = addr_ggtt;
> -			blob->ads.capture_class[i][j] = addr_ggtt;
> +			if (!info->engine_enabled_masks[j]) {
> +				if (gc->num_class_regs[i][j])
> +					drm_warn(&i915->drm, "GuC-Cap VF%d-class-%d "
> +						 "class regs valid mismatch was=%d now=%d!\n",
> +						 i, j, gc->num_class_regs[i][j], tmp);
> +				if (gc->num_instance_regs[i][j])
> +					drm_warn(&i915->drm, "GuC-Cap VF%d-class-%d "
> +						 "inst regs valid mismatch was=%d now=%d!\n",
> +						 i, j, gc->num_instance_regs[i][j], tmp);
> +				gc->num_class_regs[i][j] = 0;
> +				gc->num_instance_regs[i][j] = 0;
> +				if (blob) {
> +					blob->ads.capture_class[i][j] = null_ggtt;
> +					blob->ads.capture_instance[i][j] = null_ggtt;
> +				}
> +			} else {
> +				if (!COUNT_REGS(guc, i, TYPE_CLASS,
> +						guc_class_to_engine_class(j), &tmp)) {
> +					size = FILL_REGS(guc, blob, i, true, j, TYPE_CLASS,
> +							 "class", &gc->num_class_regs[i][j],
> +							 tmp, &ptr,
> +							 &blob->ads.capture_class[i][j],
> +							 &ggtt, null_ggtt);
> +					gc->class_list_size += size;
> +					alloc_size += size;
> +				} else {
> +					gc->num_class_regs[i][j] = 0;
> +					if (blob)
> +						blob->ads.capture_class[i][j] = null_ggtt;
> +				}
> +				if (!COUNT_REGS(guc, i, TYPE_INSTANCE,
> +						guc_class_to_engine_class(j), &tmp)) {
> +					size = FILL_REGS(guc, blob, i, true, j, TYPE_INSTANCE,
> +							 "instance", &gc->num_instance_regs[i][j],
> +							 tmp, &ptr,
> +							 &blob->ads.capture_instance[i][j],
> +							 &ggtt, null_ggtt);
> +					gc->instance_list_size += size;
> +					alloc_size += size;
> +				} else {
> +					gc->num_instance_regs[i][j] = 0;
> +					if (blob)
> +						blob->ads.capture_instance[i][j] = null_ggtt;
> +				}
> +			}
> +		}
> +		if (!COUNT_REGS(guc, i, GUC_CAPTURE_LIST_TYPE_GLOBAL, 0, &tmp)) {
> +			size = FILL_REGS(guc, blob, i, true, 0, GUC_CAPTURE_LIST_TYPE_GLOBAL,
> +					 "global", &gc->num_global_regs[i], tmp, &ptr,
> +					 &blob->ads.capture_global[i], &ggtt, null_ggtt);
> +			gc->global_list_size += size;
> +			alloc_size += size;
> +		} else {
> +			gc->num_global_regs[i] = 0;
> +			if (blob)
> +				blob->ads.capture_global[i] = null_ggtt;
>  		}
> -
> -		blob->ads.capture_global[i] = addr_ggtt;
>  	}
> +
> +#undef COUNT_REGS
> +#undef FILL_REGS
> +#undef TYPE_CLASS
> +#undef TYPE_INSTANCE
> +
> +	if (guc->ads_capture_size && guc->ads_capture_size != PAGE_ALIGN(alloc_size))
> +		drm_warn(&i915->drm, "GuC->ADS->Capture alloc size changed from %d to %d\n",
> +			 guc->ads_capture_size, PAGE_ALIGN(alloc_size));
> +
> +	return PAGE_ALIGN(alloc_size);
>  }
>  
>  static void __guc_ads_init(struct intel_guc *guc) @@ -614,6 +760,12 
> @@ int intel_guc_ads_create(struct intel_guc *guc)
>  		return ret;
>  	guc->ads_golden_ctxt_size = ret;
>  
> +	/* Likewise the capture lists: */
> +	ret = guc_capture_prep_lists(guc, NULL);
> +	if (ret < 0)
> +		return ret;
> +	guc->ads_capture_size = ret;
> +
>  	/* Now the total size can be determined: */
>  	size = guc_ads_blob_size(guc);
>  
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c 
> b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> new file mode 100644
> index 000000000000..c741c77b7fc8
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> @@ -0,0 +1,232 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2021-2021 Intel Corporation  */
> +
> +#include <drm/drm_print.h>
> +
> +#include "i915_drv.h"
> +#include "i915_drv.h"

duplicated include

> +#include "i915_memcpy.h"
> +#include "gt/intel_gt.h"
> +
> +#include "intel_guc_fwif.h"
> +#include "intel_guc_capture.h"
> +
> +/* Define all device tables of GuC error capture register lists */
> +
> +/********************************* Gen12 LP  
> +*********************************/

didn't we move away from "GEN" naming ?

> +/************** GLOBAL *************/

do we really need all these decorations ?

> +struct __guc_mmio_reg_descr gen12lp_global_regs[] = {
> +	{SWF_ILK(0),               0,      0, "SWF_ILK0"},
> +	/* Add additional register list */

do we need this reminder ?

> +};
> +
> +/********** RENDER/COMPUTE *********/
> +/* Per-Class */
> +struct __guc_mmio_reg_descr gen12lp_rc_class_regs[] = {
> +	{SWF_ILK(0),               0,      0, "SWF_ILK0"},
> +	/* Add additional register list */
> +};
> +
> +/* Per-Engine-Instance */
> +struct __guc_mmio_reg_descr gen12lp_rc_inst_regs[] = {
> +	{SWF_ILK(0),               0,      0, "SWF_ILK0"},
> +	/* Add additional register list */
> +};
> +
> +/************* MEDIA-VD ************/
> +/* Per-Class */
> +struct __guc_mmio_reg_descr gen12lp_vd_class_regs[] = {
> +	{SWF_ILK(0),               0,      0, "SWF_ILK0"},
> +	/* Add additional register list */
> +};
> +
> +/* Per-Engine-Instance */
> +struct __guc_mmio_reg_descr gen12lp_vd_inst_regs[] = {
> +	{SWF_ILK(0),               0,      0, "SWF_ILK0"},
> +	/* Add additional register list */
> +};
> +
> +/************* MEDIA-VEC ***********/
> +/* Per-Class */
> +struct __guc_mmio_reg_descr gen12lp_vec_class_regs[] = {
> +	{SWF_ILK(0),               0,      0, "SWF_ILK0"},
> +	/* Add additional register list */
> +};
> +
> +/* Per-Engine-Instance */
> +struct __guc_mmio_reg_descr gen12lp_vec_inst_regs[] = {
> +	{SWF_ILK(0),               0,      0, "SWF_ILK0"},
> +	/* Add additional register list */
> +};
> +
> +/********** List of lists **********/ struct 
> +__guc_mmio_reg_descr_group gen12lp_lists[] = {
> +	{
> +		.list = gen12lp_global_regs,
> +		.num_regs = (sizeof(gen12lp_global_regs) / sizeof(struct 
> +__guc_mmio_reg_descr)),

ARRAY_SIZE ?

> +		.owner = GUC_CAPTURE_LIST_INDEX_PF,
> +		.type = GUC_CAPTURE_LIST_TYPE_GLOBAL,
> +		.engine = 0
> +	},
> +	{
> +		.list = gen12lp_rc_class_regs,
> +		.num_regs = (sizeof(gen12lp_rc_class_regs) / sizeof(struct __guc_mmio_reg_descr)),
> +		.owner = GUC_CAPTURE_LIST_INDEX_PF,
> +		.type = GUC_CAPTURE_LIST_TYPE_ENGINE_CLASS,
> +		.engine = RENDER_CLASS
> +	},
> +	{
> +		.list = gen12lp_rc_inst_regs,
> +		.num_regs = (sizeof(gen12lp_rc_inst_regs) / sizeof(struct __guc_mmio_reg_descr)),
> +		.owner = GUC_CAPTURE_LIST_INDEX_PF,
> +		.type = GUC_CAPTURE_LIST_TYPE_ENGINE_INSTANCE,
> +		.engine = RENDER_CLASS
> +	},
> +	{
> +		.list = gen12lp_vd_class_regs,
> +		.num_regs = (sizeof(gen12lp_vd_class_regs) / sizeof(struct __guc_mmio_reg_descr)),
> +		.owner = GUC_CAPTURE_LIST_INDEX_PF,
> +		.type = GUC_CAPTURE_LIST_TYPE_ENGINE_CLASS,
> +		.engine = VIDEO_DECODE_CLASS
> +	},
> +	{
> +		.list = gen12lp_vd_inst_regs,
> +		.num_regs = (sizeof(gen12lp_vd_inst_regs) / sizeof(struct __guc_mmio_reg_descr)),
> +		.owner = GUC_CAPTURE_LIST_INDEX_PF,
> +		.type = GUC_CAPTURE_LIST_TYPE_ENGINE_INSTANCE,
> +		.engine = VIDEO_DECODE_CLASS
> +	},
> +	{
> +		.list = gen12lp_vec_class_regs,
> +		.num_regs = (sizeof(gen12lp_vec_class_regs) / sizeof(struct __guc_mmio_reg_descr)),
> +		.owner = GUC_CAPTURE_LIST_INDEX_PF,
> +		.type = GUC_CAPTURE_LIST_TYPE_ENGINE_CLASS,
> +		.engine = VIDEO_ENHANCEMENT_CLASS
> +	},
> +	{
> +		.list = gen12lp_vec_inst_regs,
> +		.num_regs = (sizeof(gen12lp_vec_inst_regs) / sizeof(struct __guc_mmio_reg_descr)),
> +		.owner = GUC_CAPTURE_LIST_INDEX_PF,
> +		.type = GUC_CAPTURE_LIST_TYPE_ENGINE_INSTANCE,
> +		.engine = VIDEO_ENHANCEMENT_CLASS
> +	},
> +	{NULL, 0, 0, 0, 0}
> +};
> +
> +/************ FIXME: Populate tables for other devices in subsequent 
> +patch ************/
> +
> +static struct __guc_mmio_reg_descr_group * 
> +guc_capture_get_device_reglist(struct drm_i915_private *dev_priv)

in new code we are using "i915" instead of "dev_priv" and since this function has "guc" prefix it shall rather take "guc" as param:

guc_capture_get_device_reglist(struct intel_guc *guc) {
	struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
	...


> +{
> +	if (IS_TIGERLAKE(dev_priv) || IS_ROCKETLAKE(dev_priv) ||
> +	    IS_ALDERLAKE_S(dev_priv) || IS_ALDERLAKE_P(dev_priv)) {
> +		return gen12lp_lists;
> +	}
> +
> +	return NULL;
> +}
> +
> +static inline struct __guc_mmio_reg_descr_group * 
> +guc_capture_get_one_list(struct __guc_mmio_reg_descr_group *reglists, 
> +u32 owner, u32 type, u32 id) {
> +	int i = 0;
> +
> +	if (!reglists)
> +		return NULL;
> +	while (reglists[i].list) {
> +		if (reglists[i].owner == owner &&
> +		    reglists[i].type == type) {
> +			if (reglists[i].type == GUC_CAPTURE_LIST_TYPE_GLOBAL ||
> +			    reglists[i].engine == id) {
> +				return &reglists[i];
> +			}
> +		}
> +		++i;
> +	}
> +	return NULL;
> +}
> +
> +static inline void
> +warn_with_capture_list_identifier(struct drm_i915_private *i915, char *msg,
> +				  u32 owner, u32 type, u32 classid) {
> +	const char *ownerstr[GUC_CAPTURE_LIST_INDEX_MAX] = {"PF", "VF"};
> +	const char *typestr[GUC_CAPTURE_LIST_TYPE_MAX - 1] = {"Class", "Instance"};
> +	const char *classstr[GUC_LAST_ENGINE_CLASS + 1] = {"Render", "Video", "VideoEnhance",
> +							   "Blitter", "Reserved"};

better to wrap that into simple small helpers like

	const char *stringify_guc_capture_owner(u32 owner) { .. }
	const char *stringify_guc_capture_type(u32 type) { .. }
	const char *stringify_guc_capture_class(u32 class) { .. }

> +	static const char unknownstr[] = "unknown";
> +
> +	if (type == GUC_CAPTURE_LIST_TYPE_GLOBAL)
> +		drm_warn(&i915->drm, "GuC-capture: %s for %s Global-Registers.\n", msg,
> +			 (owner < GUC_CAPTURE_LIST_INDEX_MAX) ? ownerstr[owner] : unknownstr);
> +	else
> +		drm_warn(&i915->drm, "GuC-capture: %s for %s %s-Registers on %s-Engine\n", msg,
> +			 (owner < GUC_CAPTURE_LIST_INDEX_MAX) ? ownerstr[owner] : unknownstr,
> +			 (type < GUC_CAPTURE_LIST_TYPE_MAX) ? typestr[type - 1] :  unknownstr,
> +			 (classid < GUC_LAST_ENGINE_CLASS + 1) ? classstr[classid] : 
> +unknownstr); }
> +
> +int intel_guc_capture_list_count(struct intel_guc *guc, u32 owner, u32 type, u32 classid,
> +				 u16 *num_entries)
> +{
> +	struct drm_i915_private *dev_priv = (guc_to_gt(guc))->i915;

s/dev_priv/i915
redundant ()

> +	struct __guc_mmio_reg_descr_group *reglists = guc->capture.reglists;
> +	struct __guc_mmio_reg_descr_group *match;
> +
> +	if (!reglists)
> +		return -ENODEV;
> +
> +	match = guc_capture_get_one_list(reglists, owner, type, classid);
> +	if (match) {
> +		*num_entries = match->num_regs;
> +		return 0;

IIRC early returns are preferred for error cases, not success

> +	}
> +
> +	warn_with_capture_list_identifier(dev_priv, "Missing register list size", owner, type,
> +					  classid);
> +
> +	return -ENODATA;
> +}
> +
> +int intel_guc_capture_list_init(struct intel_guc *guc, u32 owner, u32 type, u32 classid,
> +				struct guc_mmio_reg *ptr, u16 num_entries) {
> +	u32 j = 0;
> +	struct drm_i915_private *dev_priv = (guc_to_gt(guc))->i915;

s/dev_priv/i915
redundant ()

> +	struct __guc_mmio_reg_descr_group *reglists = guc->capture.reglists;
> +	struct __guc_mmio_reg_descr_group *match;
> +
> +	if (!reglists)
> +		return -ENODEV;
> +
> +	match = guc_capture_get_one_list(reglists, owner, type, classid);
> +	if (match) {
> +		while (j < num_entries && j < match->num_regs) {
> +			ptr[j].offset = match->list[j].reg.reg;
> +			ptr[j].value = 0xDEADF00D;
> +			ptr[j].flags = match->list[j].flags;
> +			ptr[j].mask = match->list[j].mask;
> +			++j;
> +		}
> +		return 0;
> +	}
> +
> +	warn_with_capture_list_identifier(dev_priv, "Missing register list init", owner, type,
> +					  classid);
> +
> +	return -ENODATA;
> +}
> +
> +void intel_guc_capture_destroy(struct intel_guc *guc) { }
> +
> +int intel_guc_capture_init(struct intel_guc *guc) {
> +	struct drm_i915_private *dev_priv = (guc_to_gt(guc))->i915;
> +
> +	guc->capture.reglists = guc_capture_get_device_reglist(dev_priv);
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.h 
> b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.h
> new file mode 100644
> index 000000000000..352940b8bc87
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.h
> @@ -0,0 +1,47 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2021-2021 Intel Corporation  */
> +
> +#ifndef _INTEL_GUC_CAPTURE_H
> +#define _INTEL_GUC_CAPTURE_H
> +
> +#include <linux/mutex.h>
> +#include <linux/workqueue.h>
> +#include "intel_guc_fwif.h"
> +
> +struct intel_guc;
> +
> +struct __guc_mmio_reg_descr {
> +	i915_reg_t reg;
> +	u32 flags;
> +	u32 mask;
> +	char *regname;

const char* ?

but maybe instead of adding reg name to the GuC specific struct we should add generic purpose function that will return pretty name of the
register:

i915_reg.c:

const char *i915_reg_to_string(i915_reg_r reg) {
	...
}

> +};
> +
> +struct __guc_mmio_reg_descr_group {
> +	struct __guc_mmio_reg_descr *list;
> +	u32 num_regs;
> +	u32 owner; /* see enum guc_capture_owner */
> +	u32 type; /* see enum guc_capture_type */
> +	u32 engine; /* as per MAX_ENGINE_CLASS */ };
> +
> +struct intel_guc_state_capture {
> +	struct __guc_mmio_reg_descr_group *reglists;
> +	u16 num_instance_regs[GUC_CAPTURE_LIST_INDEX_MAX][GUC_MAX_ENGINE_CLASSES];
> +	u16 num_class_regs[GUC_CAPTURE_LIST_INDEX_MAX][GUC_MAX_ENGINE_CLASSES];
> +	u16 num_global_regs[GUC_CAPTURE_LIST_INDEX_MAX];
> +	int instance_list_size;
> +	int class_list_size;
> +	int global_list_size;
> +};
> +
> +int intel_guc_capture_list_count(struct intel_guc *guc, u32 owner, u32 type, u32 class,
> +				 u16 *num_entries);
> +int intel_guc_capture_list_init(struct intel_guc *guc, u32 owner, u32 type, u32 class,
> +				struct guc_mmio_reg *ptr, u16 num_entries); void 
> +intel_guc_capture_destroy(struct intel_guc *guc); int 
> +intel_guc_capture_init(struct intel_guc *guc);
> +
> +#endif /* _INTEL_GUC_CAPTURE_H */
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h 
> b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> index 767684b6af67..1a1d2271c7e9 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> @@ -285,13 +285,30 @@ struct guc_gt_system_info {  } __packed;
>  
>  /* Capture-types of GuC capture register lists */ -enum
> +enum guc_capture_owner
>  {
>  	GUC_CAPTURE_LIST_INDEX_PF = 0,
>  	GUC_CAPTURE_LIST_INDEX_VF = 1,
>  	GUC_CAPTURE_LIST_INDEX_MAX = 2,

s/INDEX/OWNER ?

>  };
>  
> +/*Register-types of GuC capture register lists */ enum 
> +guc_capture_type {
> +	GUC_CAPTURE_LIST_TYPE_GLOBAL = 0,
> +	GUC_CAPTURE_LIST_TYPE_ENGINE_CLASS,
> +	GUC_CAPTURE_LIST_TYPE_ENGINE_INSTANCE,
> +	GUC_CAPTURE_LIST_TYPE_MAX,
> +};
> +
> +struct guc_debug_capture_list_header {
> +	u32 info;
> +		#define GUC_CAPTURELISTHDR_NUMDESCR GENMASK(15, 0) };
> +
> +struct guc_debug_capture_list {
> +	struct guc_debug_capture_list_header header; };
> +
>  /* GuC Additional Data Struct */
>  struct guc_ads {
>  	struct guc_mmio_reg_set 
> reg_state_list[GUC_MAX_ENGINE_CLASSES][GUC_MAX_INSTANCES_PER_CLASS];
>

Patch

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 074d6b8edd23..e3c4d5cea4c3 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -190,6 +190,7 @@  i915-y += gt/uc/intel_uc.o \
 	  gt/uc/intel_guc_rc.o \
 	  gt/uc/intel_guc_slpc.o \
 	  gt/uc/intel_guc_submission.o \
+	  gt/uc/intel_guc_capture.o \
 	  gt/uc/intel_huc.o \
 	  gt/uc/intel_huc_debugfs.o \
 	  gt/uc/intel_huc_fw.o
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
index 5cf9ebd2ee55..458f0d248a5a 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
@@ -335,9 +335,14 @@  int intel_guc_init(struct intel_guc *guc)
 	if (ret)
 		goto err_fw;
 
-	ret = intel_guc_ads_create(guc);
+	ret = intel_guc_capture_init(guc);
 	if (ret)
 		goto err_log;
+
+	ret = intel_guc_ads_create(guc);
+	if (ret)
+		goto err_capture;
+
 	GEM_BUG_ON(!guc->ads_vma);
 
 	ret = intel_guc_ct_init(&guc->ct);
@@ -376,6 +381,8 @@  int intel_guc_init(struct intel_guc *guc)
 	intel_guc_ct_fini(&guc->ct);
 err_ads:
 	intel_guc_ads_destroy(guc);
+err_capture:
+	intel_guc_capture_destroy(guc);
 err_log:
 	intel_guc_log_destroy(&guc->log);
 err_fw:
@@ -403,6 +410,7 @@  void intel_guc_fini(struct intel_guc *guc)
 	intel_guc_ct_fini(&guc->ct);
 
 	intel_guc_ads_destroy(guc);
+	intel_guc_capture_destroy(guc);
 	intel_guc_log_destroy(&guc->log);
 	intel_uc_fw_fini(&guc->fw);
 }
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
index 9de99772f916..d136c69abe12 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
@@ -16,6 +16,7 @@ 
 #include "intel_guc_log.h"
 #include "intel_guc_reg.h"
 #include "intel_guc_slpc_types.h"
+#include "intel_guc_capture.h"
 #include "intel_uc_fw.h"
 #include "i915_utils.h"
 #include "i915_vma.h"
@@ -37,6 +38,8 @@  struct intel_guc {
 	struct intel_guc_ct ct;
 	/** @slpc: sub-structure containing SLPC related data and objects */
 	struct intel_guc_slpc slpc;
+	/** @capture: the error-state-capture module's data and objects */
+	struct intel_guc_state_capture capture;
 
 	/** @sched_engine: Global engine used to submit requests to GuC */
 	struct i915_sched_engine *sched_engine;
@@ -138,6 +141,8 @@  struct intel_guc {
 	u32 ads_regset_size;
 	/** @ads_golden_ctxt_size: size of the golden contexts in the ADS */
 	u32 ads_golden_ctxt_size;
+	/** @ads_capture_size: size of register lists in the ADS used for error capture */
+	u32 ads_capture_size;
 	/** @ads_engine_usage_size: size of engine usage in the ADS */
 	u32 ads_engine_usage_size;
 
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
index 6c81ddd303d3..2780c0fadd01 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
@@ -10,6 +10,7 @@ 
 #include "gt/shmem_utils.h"
 #include "intel_guc_ads.h"
 #include "intel_guc_fwif.h"
+#include "intel_guc_capture.h"
 #include "intel_uc.h"
 #include "i915_drv.h"
 
@@ -71,8 +72,7 @@  static u32 guc_ads_golden_ctxt_size(struct intel_guc *guc)
 
 static u32 guc_ads_capture_size(struct intel_guc *guc)
 {
-	/* Basic support to init ADS without a proper GuC error capture list */
-	return PAGE_ALIGN(PAGE_SIZE);
+	return PAGE_ALIGN(guc->ads_capture_size);
 }
 
 static u32 guc_ads_private_data_size(struct intel_guc *guc)
@@ -519,24 +519,170 @@  static void guc_init_golden_context(struct intel_guc *guc)
 	GEM_BUG_ON(guc->ads_golden_ctxt_size != total_size);
 }
 
-static void guc_capture_prep_lists(struct intel_guc *guc, struct __guc_ads_blob *blob)
+static int
+guc_fill_reglist(struct intel_guc *guc, struct __guc_ads_blob *blob, int vf, bool enabled,
+		 int classid, int type, char *typename, u16 *p_numregs, int newnum, u8 **p_virt_ptr,
+		 u32 *p_blobptr_to_ggtt, u32 *p_ggtt, u32 null_ggtt)
 {
-	int i, j;
-	u32 addr_ggtt, offset;
+	struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
+	struct guc_debug_capture_list *listnode;
+	int size = 0;
 
-	offset = guc_ads_capture_offset(guc);
-	addr_ggtt = intel_guc_ggtt_offset(guc, guc->ads_vma) + offset;
+	if (blob && *p_numregs != newnum) {
+		if (type == GUC_CAPTURE_LIST_TYPE_GLOBAL)
+			drm_warn(&i915->drm, "Guc-Cap VF%d-%s num-reg mismatch was=%d now=%d!\n",
+				 vf, typename, *p_numregs, newnum);
+		else
+			drm_warn(&i915->drm, "Guc-Cap VF%d-Class-%d-%s num-reg mismatch was=%d now=%d!\n",
+				 vf, classid, typename, *p_numregs, newnum);
+	}
+	/*
+	 * For enabled capture lists, we not only need to call capture module to help
+	 * populate the list-descriptor into the correct ads capture structures, but
+	 * we also need to increment the virtual pointers and ggtt offsets so that
+	 * caller has the subsequent gfx memory location.
+	 */
+	*p_numregs = newnum;
+	size = PAGE_ALIGN((sizeof(struct guc_debug_capture_list)) +
+			  (newnum * sizeof(struct guc_mmio_reg)));
+	/* if caller hasn't allocated ADS blob, return size and counts, we're done */
+	if (!blob)
+		return size;
+	if (blob) {
+		/* if caller allocated ADS blob, populate the capture register descriptors */
+		if (!newnum) {
+			*p_blobptr_to_ggtt = null_ggtt;
+		} else {
+			/* get ptr and populate header info: */
+			*p_blobptr_to_ggtt = *p_ggtt;
+			listnode = (struct guc_debug_capture_list *)*p_virt_ptr;
+			*p_ggtt += sizeof(struct guc_debug_capture_list);
+			*p_virt_ptr += sizeof(struct guc_debug_capture_list);
+			listnode->header.info = FIELD_PREP(GUC_CAPTURELISTHDR_NUMDESCR, *p_numregs);
+
+			/* get ptr and populate register descriptor list: */
+			intel_guc_capture_list_init(guc, vf, type, classid,
+						    (struct guc_mmio_reg *)*p_virt_ptr,
+						    *p_numregs);
+
+			/* increment ptrs for that header: */
+			*p_ggtt += size - sizeof(struct guc_debug_capture_list);
+			*p_virt_ptr += size - sizeof(struct guc_debug_capture_list);
+		}
+	}
+
+	return size;
+}
+
+static int guc_capture_prep_lists(struct intel_guc *guc, struct __guc_ads_blob *blob)
+{
+	struct intel_gt *gt = guc_to_gt(guc);
+	int i, j, size;
+	u32 ggtt, null_ggtt, offset, alloc_size = 0;
+	struct guc_gt_system_info *info, local_info;
+	struct guc_debug_capture_list *listnode;
+	struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
+	struct intel_guc_state_capture *gc = &guc->capture;
+	u16 tmp = 0;
+	u8 *ptr = NULL;
+
+	if (blob) {
+		offset = guc_ads_capture_offset(guc);
+		ggtt = intel_guc_ggtt_offset(guc, guc->ads_vma) + offset;
+		ptr = ((u8 *)blob) + offset;
+		info = &blob->system_info;
+	} else {
+		memset(&local_info, 0, sizeof(local_info));
+		info = &local_info;
+		fill_engine_enable_masks(gt, info);
+	}
+
+	/* first, set aside the first page for a capture_list with zero descriptors */
+	alloc_size = PAGE_SIZE;
+	if (blob) {
+		listnode = (struct guc_debug_capture_list *)ptr;
+		listnode->header.info = FIELD_PREP(GUC_CAPTURELISTHDR_NUMDESCR, 0);
+		null_ggtt = ggtt;
+		ggtt += PAGE_SIZE;
+		ptr +=  PAGE_SIZE;
+	}
 
-	/* FIXME: Populate a proper capture list */
+#define COUNT_REGS intel_guc_capture_list_count
+#define FILL_REGS guc_fill_reglist
+#define TYPE_CLASS GUC_CAPTURE_LIST_TYPE_ENGINE_CLASS
+#define TYPE_INSTANCE GUC_CAPTURE_LIST_TYPE_ENGINE_INSTANCE
 
 	for (i = 0; i < GUC_CAPTURE_LIST_INDEX_MAX; i++) {
 		for (j = 0; j < GUC_MAX_ENGINE_CLASSES; j++) {
-			blob->ads.capture_instance[i][j] = addr_ggtt;
-			blob->ads.capture_class[i][j] = addr_ggtt;
+			if (!info->engine_enabled_masks[j]) {
+				if (gc->num_class_regs[i][j])
+					drm_warn(&i915->drm, "GuC-Cap VF%d-class-%d "
+						 "class regs valid mismatch was=%d now=%d!\n",
+						 i, j, gc->num_class_regs[i][j], tmp);
+				if (gc->num_instance_regs[i][j])
+					drm_warn(&i915->drm, "GuC-Cap VF%d-class-%d "
+						 "inst regs valid mismatch was=%d now=%d!\n",
+						 i, j, gc->num_instance_regs[i][j], tmp);
+				gc->num_class_regs[i][j] = 0;
+				gc->num_instance_regs[i][j] = 0;
+				if (blob) {
+					blob->ads.capture_class[i][j] = null_ggtt;
+					blob->ads.capture_instance[i][j] = null_ggtt;
+				}
+			} else {
+				if (!COUNT_REGS(guc, i, TYPE_CLASS,
+						guc_class_to_engine_class(j), &tmp)) {
+					size = FILL_REGS(guc, blob, i, true, j, TYPE_CLASS,
+							 "class", &gc->num_class_regs[i][j],
+							 tmp, &ptr,
+							 &blob->ads.capture_class[i][j],
+							 &ggtt, null_ggtt);
+					gc->class_list_size += size;
+					alloc_size += size;
+				} else {
+					gc->num_class_regs[i][j] = 0;
+					if (blob)
+						blob->ads.capture_class[i][j] = null_ggtt;
+				}
+				if (!COUNT_REGS(guc, i, TYPE_INSTANCE,
+						guc_class_to_engine_class(j), &tmp)) {
+					size = FILL_REGS(guc, blob, i, true, j, TYPE_INSTANCE,
+							 "instance", &gc->num_instance_regs[i][j],
+							 tmp, &ptr,
+							 &blob->ads.capture_instance[i][j],
+							 &ggtt, null_ggtt);
+					gc->instance_list_size += size;
+					alloc_size += size;
+				} else {
+					gc->num_instance_regs[i][j] = 0;
+					if (blob)
+						blob->ads.capture_instance[i][j] = null_ggtt;
+				}
+			}
+		}
+		if (!COUNT_REGS(guc, i, GUC_CAPTURE_LIST_TYPE_GLOBAL, 0, &tmp)) {
+			size = FILL_REGS(guc, blob, i, true, 0, GUC_CAPTURE_LIST_TYPE_GLOBAL,
+					 "global", &gc->num_global_regs[i], tmp, &ptr,
+					 &blob->ads.capture_global[i], &ggtt, null_ggtt);
+			gc->global_list_size += size;
+			alloc_size += size;
+		} else {
+			gc->num_global_regs[i] = 0;
+			if (blob)
+				blob->ads.capture_global[i] = null_ggtt;
 		}
-
-		blob->ads.capture_global[i] = addr_ggtt;
 	}
+
+#undef COUNT_REGS
+#undef FILL_REGS
+#undef TYPE_CLASS
+#undef TYPE_INSTANCE
+
+	if (guc->ads_capture_size && guc->ads_capture_size != PAGE_ALIGN(alloc_size))
+		drm_warn(&i915->drm, "GuC->ADS->Capture alloc size changed from %d to %d\n",
+			 guc->ads_capture_size, PAGE_ALIGN(alloc_size));
+
+	return PAGE_ALIGN(alloc_size);
 }
 
 static void __guc_ads_init(struct intel_guc *guc)
@@ -614,6 +760,12 @@  int intel_guc_ads_create(struct intel_guc *guc)
 		return ret;
 	guc->ads_golden_ctxt_size = ret;
 
+	/* Likewise the capture lists: */
+	ret = guc_capture_prep_lists(guc, NULL);
+	if (ret < 0)
+		return ret;
+	guc->ads_capture_size = ret;
+
 	/* Now the total size can be determined: */
 	size = guc_ads_blob_size(guc);
 
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
new file mode 100644
index 000000000000..c741c77b7fc8
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
@@ -0,0 +1,232 @@ 
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2021-2021 Intel Corporation
+ */
+
+#include <drm/drm_print.h>
+
+#include "i915_drv.h"
+#include "i915_drv.h"
+#include "i915_memcpy.h"
+#include "gt/intel_gt.h"
+
+#include "intel_guc_fwif.h"
+#include "intel_guc_capture.h"
+
+/* Define all device tables of GuC error capture register lists */
+
+/********************************* Gen12 LP  *********************************/
+/************** GLOBAL *************/
+struct __guc_mmio_reg_descr gen12lp_global_regs[] = {
+	{SWF_ILK(0),               0,      0, "SWF_ILK0"},
+	/* Add additional register list */
+};
+
+/********** RENDER/COMPUTE *********/
+/* Per-Class */
+struct __guc_mmio_reg_descr gen12lp_rc_class_regs[] = {
+	{SWF_ILK(0),               0,      0, "SWF_ILK0"},
+	/* Add additional register list */
+};
+
+/* Per-Engine-Instance */
+struct __guc_mmio_reg_descr gen12lp_rc_inst_regs[] = {
+	{SWF_ILK(0),               0,      0, "SWF_ILK0"},
+	/* Add additional register list */
+};
+
+/************* MEDIA-VD ************/
+/* Per-Class */
+struct __guc_mmio_reg_descr gen12lp_vd_class_regs[] = {
+	{SWF_ILK(0),               0,      0, "SWF_ILK0"},
+	/* Add additional register list */
+};
+
+/* Per-Engine-Instance */
+struct __guc_mmio_reg_descr gen12lp_vd_inst_regs[] = {
+	{SWF_ILK(0),               0,      0, "SWF_ILK0"},
+	/* Add additional register list */
+};
+
+/************* MEDIA-VEC ***********/
+/* Per-Class */
+struct __guc_mmio_reg_descr gen12lp_vec_class_regs[] = {
+	{SWF_ILK(0),               0,      0, "SWF_ILK0"},
+	/* Add additional register list */
+};
+
+/* Per-Engine-Instance */
+struct __guc_mmio_reg_descr gen12lp_vec_inst_regs[] = {
+	{SWF_ILK(0),               0,      0, "SWF_ILK0"},
+	/* Add additional register list */
+};
+
+/********** List of lists **********/
+struct __guc_mmio_reg_descr_group gen12lp_lists[] = {
+	{
+		.list = gen12lp_global_regs,
+		.num_regs = (sizeof(gen12lp_global_regs) / sizeof(struct __guc_mmio_reg_descr)),
+		.owner = GUC_CAPTURE_LIST_INDEX_PF,
+		.type = GUC_CAPTURE_LIST_TYPE_GLOBAL,
+		.engine = 0
+	},
+	{
+		.list = gen12lp_rc_class_regs,
+		.num_regs = (sizeof(gen12lp_rc_class_regs) / sizeof(struct __guc_mmio_reg_descr)),
+		.owner = GUC_CAPTURE_LIST_INDEX_PF,
+		.type = GUC_CAPTURE_LIST_TYPE_ENGINE_CLASS,
+		.engine = RENDER_CLASS
+	},
+	{
+		.list = gen12lp_rc_inst_regs,
+		.num_regs = (sizeof(gen12lp_rc_inst_regs) / sizeof(struct __guc_mmio_reg_descr)),
+		.owner = GUC_CAPTURE_LIST_INDEX_PF,
+		.type = GUC_CAPTURE_LIST_TYPE_ENGINE_INSTANCE,
+		.engine = RENDER_CLASS
+	},
+	{
+		.list = gen12lp_vd_class_regs,
+		.num_regs = (sizeof(gen12lp_vd_class_regs) / sizeof(struct __guc_mmio_reg_descr)),
+		.owner = GUC_CAPTURE_LIST_INDEX_PF,
+		.type = GUC_CAPTURE_LIST_TYPE_ENGINE_CLASS,
+		.engine = VIDEO_DECODE_CLASS
+	},
+	{
+		.list = gen12lp_vd_inst_regs,
+		.num_regs = (sizeof(gen12lp_vd_inst_regs) / sizeof(struct __guc_mmio_reg_descr)),
+		.owner = GUC_CAPTURE_LIST_INDEX_PF,
+		.type = GUC_CAPTURE_LIST_TYPE_ENGINE_INSTANCE,
+		.engine = VIDEO_DECODE_CLASS
+	},
+	{
+		.list = gen12lp_vec_class_regs,
+		.num_regs = (sizeof(gen12lp_vec_class_regs) / sizeof(struct __guc_mmio_reg_descr)),
+		.owner = GUC_CAPTURE_LIST_INDEX_PF,
+		.type = GUC_CAPTURE_LIST_TYPE_ENGINE_CLASS,
+		.engine = VIDEO_ENHANCEMENT_CLASS
+	},
+	{
+		.list = gen12lp_vec_inst_regs,
+		.num_regs = (sizeof(gen12lp_vec_inst_regs) / sizeof(struct __guc_mmio_reg_descr)),
+		.owner = GUC_CAPTURE_LIST_INDEX_PF,
+		.type = GUC_CAPTURE_LIST_TYPE_ENGINE_INSTANCE,
+		.engine = VIDEO_ENHANCEMENT_CLASS
+	},
+	{NULL, 0, 0, 0, 0}
+};
+
+/************ FIXME: Populate tables for other devices in subsequent patch ************/
+
+static struct __guc_mmio_reg_descr_group *
+guc_capture_get_device_reglist(struct drm_i915_private *dev_priv)
+{
+	if (IS_TIGERLAKE(dev_priv) || IS_ROCKETLAKE(dev_priv) ||
+	    IS_ALDERLAKE_S(dev_priv) || IS_ALDERLAKE_P(dev_priv)) {
+		return gen12lp_lists;
+	}
+
+	return NULL;
+}
+
+static inline struct __guc_mmio_reg_descr_group *
+guc_capture_get_one_list(struct __guc_mmio_reg_descr_group *reglists, u32 owner, u32 type, u32 id)
+{
+	int i = 0;
+
+	if (!reglists)
+		return NULL;
+	while (reglists[i].list) {
+		if (reglists[i].owner == owner &&
+		    reglists[i].type == type) {
+			if (reglists[i].type == GUC_CAPTURE_LIST_TYPE_GLOBAL ||
+			    reglists[i].engine == id) {
+				return &reglists[i];
+			}
+		}
+		++i;
+	}
+	return NULL;
+}
+
+static inline void
+warn_with_capture_list_identifier(struct drm_i915_private *i915, char *msg,
+				  u32 owner, u32 type, u32 classid)
+{
+	const char *ownerstr[GUC_CAPTURE_LIST_INDEX_MAX] = {"PF", "VF"};
+	const char *typestr[GUC_CAPTURE_LIST_TYPE_MAX - 1] = {"Class", "Instance"};
+	const char *classstr[GUC_LAST_ENGINE_CLASS + 1] = {"Render", "Video", "VideoEnhance",
+							   "Blitter", "Reserved"};
+	static const char unknownstr[] = "unknown";
+
+	if (type == GUC_CAPTURE_LIST_TYPE_GLOBAL)
+		drm_warn(&i915->drm, "GuC-capture: %s for %s Global-Registers.\n", msg,
+			 (owner < GUC_CAPTURE_LIST_INDEX_MAX) ? ownerstr[owner] : unknownstr);
+	else
+		drm_warn(&i915->drm, "GuC-capture: %s for %s %s-Registers on %s-Engine\n", msg,
+			 (owner < GUC_CAPTURE_LIST_INDEX_MAX) ? ownerstr[owner] : unknownstr,
+			 (type < GUC_CAPTURE_LIST_TYPE_MAX) ? typestr[type - 1] :  unknownstr,
+			 (classid < GUC_LAST_ENGINE_CLASS + 1) ? classstr[classid] : unknownstr);
+}
+
+int intel_guc_capture_list_count(struct intel_guc *guc, u32 owner, u32 type, u32 classid,
+				 u16 *num_entries)
+{
+	struct drm_i915_private *dev_priv = (guc_to_gt(guc))->i915;
+	struct __guc_mmio_reg_descr_group *reglists = guc->capture.reglists;
+	struct __guc_mmio_reg_descr_group *match;
+
+	if (!reglists)
+		return -ENODEV;
+
+	match = guc_capture_get_one_list(reglists, owner, type, classid);
+	if (match) {
+		*num_entries = match->num_regs;
+		return 0;
+	}
+
+	warn_with_capture_list_identifier(dev_priv, "Missing register list size", owner, type,
+					  classid);
+
+	return -ENODATA;
+}
+
+int intel_guc_capture_list_init(struct intel_guc *guc, u32 owner, u32 type, u32 classid,
+				struct guc_mmio_reg *ptr, u16 num_entries)
+{
+	u32 j = 0;
+	struct drm_i915_private *dev_priv = (guc_to_gt(guc))->i915;
+	struct __guc_mmio_reg_descr_group *reglists = guc->capture.reglists;
+	struct __guc_mmio_reg_descr_group *match;
+
+	if (!reglists)
+		return -ENODEV;
+
+	match = guc_capture_get_one_list(reglists, owner, type, classid);
+	if (match) {
+		while (j < num_entries && j < match->num_regs) {
+			ptr[j].offset = match->list[j].reg.reg;
+			ptr[j].value = 0xDEADF00D;
+			ptr[j].flags = match->list[j].flags;
+			ptr[j].mask = match->list[j].mask;
+			++j;
+		}
+		return 0;
+	}
+
+	warn_with_capture_list_identifier(dev_priv, "Missing register list init", owner, type,
+					  classid);
+
+	return -ENODATA;
+}
+
+void intel_guc_capture_destroy(struct intel_guc *guc)
+{
+}
+
+int intel_guc_capture_init(struct intel_guc *guc)
+{
+	struct drm_i915_private *dev_priv = (guc_to_gt(guc))->i915;
+
+	guc->capture.reglists = guc_capture_get_device_reglist(dev_priv);
+	return 0;
+}
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.h
new file mode 100644
index 000000000000..352940b8bc87
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.h
@@ -0,0 +1,47 @@ 
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2021-2021 Intel Corporation
+ */
+
+#ifndef _INTEL_GUC_CAPTURE_H
+#define _INTEL_GUC_CAPTURE_H
+
+#include <linux/mutex.h>
+#include <linux/workqueue.h>
+#include "intel_guc_fwif.h"
+
+struct intel_guc;
+
+struct __guc_mmio_reg_descr {
+	i915_reg_t reg;
+	u32 flags;
+	u32 mask;
+	char *regname;
+};
+
+struct __guc_mmio_reg_descr_group {
+	struct __guc_mmio_reg_descr *list;
+	u32 num_regs;
+	u32 owner; /* see enum guc_capture_owner */
+	u32 type; /* see enum guc_capture_type */
+	u32 engine; /* as per MAX_ENGINE_CLASS */
+};
+
+struct intel_guc_state_capture {
+	struct __guc_mmio_reg_descr_group *reglists;
+	u16 num_instance_regs[GUC_CAPTURE_LIST_INDEX_MAX][GUC_MAX_ENGINE_CLASSES];
+	u16 num_class_regs[GUC_CAPTURE_LIST_INDEX_MAX][GUC_MAX_ENGINE_CLASSES];
+	u16 num_global_regs[GUC_CAPTURE_LIST_INDEX_MAX];
+	int instance_list_size;
+	int class_list_size;
+	int global_list_size;
+};
+
+int intel_guc_capture_list_count(struct intel_guc *guc, u32 owner, u32 type, u32 class,
+				 u16 *num_entries);
+int intel_guc_capture_list_init(struct intel_guc *guc, u32 owner, u32 type, u32 class,
+				struct guc_mmio_reg *ptr, u16 num_entries);
+void intel_guc_capture_destroy(struct intel_guc *guc);
+int intel_guc_capture_init(struct intel_guc *guc);
+
+#endif /* _INTEL_GUC_CAPTURE_H */
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
index 767684b6af67..1a1d2271c7e9 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
@@ -285,13 +285,30 @@  struct guc_gt_system_info {
 } __packed;
 
 /* Capture-types of GuC capture register lists */
-enum
+enum guc_capture_owner
 {
 	GUC_CAPTURE_LIST_INDEX_PF = 0,
 	GUC_CAPTURE_LIST_INDEX_VF = 1,
 	GUC_CAPTURE_LIST_INDEX_MAX = 2,
 };
 
+/*Register-types of GuC capture register lists */
+enum guc_capture_type {
+	GUC_CAPTURE_LIST_TYPE_GLOBAL = 0,
+	GUC_CAPTURE_LIST_TYPE_ENGINE_CLASS,
+	GUC_CAPTURE_LIST_TYPE_ENGINE_INSTANCE,
+	GUC_CAPTURE_LIST_TYPE_MAX,
+};
+
+struct guc_debug_capture_list_header {
+	u32 info;
+		#define GUC_CAPTURELISTHDR_NUMDESCR GENMASK(15, 0)
+};
+
+struct guc_debug_capture_list {
+	struct guc_debug_capture_list_header header;
+};
+
 /* GuC Additional Data Struct */
 struct guc_ads {
 	struct guc_mmio_reg_set reg_state_list[GUC_MAX_ENGINE_CLASSES][GUC_MAX_INSTANCES_PER_CLASS];