From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 6748DC433EF for ; Tue, 23 Nov 2021 21:46:48 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id E691F6E073; Tue, 23 Nov 2021 21:46:47 +0000 (UTC) Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by gabe.freedesktop.org (Postfix) with ESMTPS id B573C6E073 for ; Tue, 23 Nov 2021 21:46:45 +0000 (UTC) X-IronPort-AV: E=McAfee;i="6200,9189,10177"; a="298546976" X-IronPort-AV: E=Sophos;i="5.87,258,1631602800"; d="scan'208";a="298546976" Received: from orsmga007.jf.intel.com ([10.7.209.58]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Nov 2021 13:46:43 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.87,258,1631602800"; d="scan'208";a="497436023" Received: from irvmail001.ir.intel.com ([10.43.11.63]) by orsmga007.jf.intel.com with ESMTP; 23 Nov 2021 13:46:42 -0800 Received: from [10.249.159.246] (mwajdecz-MOBL.ger.corp.intel.com [10.249.159.246]) by irvmail001.ir.intel.com (8.14.3/8.13.6/MailSET/Hub) with ESMTP id 1ANLkfY9019187; Tue, 23 Nov 2021 21:46:41 GMT Message-ID: <3d153da0-8bef-1a6b-d292-34b1f54ac938@intel.com> Date: Tue, 23 Nov 2021 22:46:41 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Firefox/91.0 Thunderbird/91.3.1 Content-Language: en-US To: Alan Previn , intel-gfx@lists.freedesktop.org References: <20211122230402.2023576-1-alan.previn.teres.alexis@intel.com> <20211122230402.2023576-3-alan.previn.teres.alexis@intel.com> From: Michal Wajdeczko In-Reply-To: <20211122230402.2023576-3-alan.previn.teres.alexis@intel.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Subject: Re: [Intel-gfx] [RFC 2/7] drm/i915/guc: Update GuC ADS size for error capture lists X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" 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 > --- > 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 > + > +#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 ®lists[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 > +#include > +#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]; >