Re: [Intel-gfx] [RFC 2/7] drm/i915/guc: Update GuC ADS size for error capture lists

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 22 Nov 2021, Alan Previn <alan.previn.teres.alexis@intel.com> 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];

-- 
Jani Nikula, Intel Open Source Graphics Center