On Mon, 22 Nov 2021, Alan Previn <alan.previn.teres.alexis@xxxxxxxxx> 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 ®lists[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
