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 <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 guc_mmio_reg_set reg_state_list[GUC_MAX_ENGINE_CLASSES][GUC_MAX_INSTANCES_PER_CLASS];
>
> --
> Jani Nikula, Intel Open Source Graphics Center
