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

[Teres Alexis, Alan Previn]

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