Re: [PATCH v10 1/5] drm/i915/gsc: add gsc as a mei auxiliary device

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----Original Message-----
> From: Ceraolo Spurio, Daniele <daniele.ceraolospurio@xxxxxxxxx>
> Sent: Thursday, March 10, 2022 00:50
> To: Usyskin, Alexander <alexander.usyskin@xxxxxxxxx>; Greg Kroah-
> Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Jani Nikula
> <jani.nikula@xxxxxxxxxxxxxxx>; Joonas Lahtinen
> <joonas.lahtinen@xxxxxxxxxxxxxxx>; Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>;
> David Airlie <airlied@xxxxxxxx>; Daniel Vetter <daniel@xxxxxxxx>; Tvrtko
> Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Winkler,
> Tomas <tomas.winkler@xxxxxxxxx>; Lubart, Vitaly <vitaly.lubart@xxxxxxxxx>
> Subject: Re:  [PATCH v10 1/5] drm/i915/gsc: add gsc as a mei
> auxiliary device
> 
> 
> 
> On 3/8/2022 8:36 AM, Alexander Usyskin wrote:
> > From: Tomas Winkler <tomas.winkler@xxxxxxxxx>
> >
> > GSC is a graphics system controller, it provides
> > a chassis controller for graphics discrete cards.
> >
> > There are two MEI interfaces in GSC: HECI1 and HECI2.
> >
> > Both interfaces are on the BAR0 at offsets 0x00258000 and 0x00259000.
> > GSC is a GT Engine (class 4: instance 6). HECI1 interrupt is signaled
> > via bit 15 and HECI2 via bit 14 in the interrupt register.
> >
> > This patch exports GSC as auxiliary device for mei driver to bind to
> > for HECI2 interface.
> 
> Do we need a test for this? E.g. to catch the unlikely case where we
> stop exposing the GSC device. We are going to get some indirect coverage
> once we start making use of the PXP interface from within the kernel,
> would that be enough?
> 
The IGT tests checks that there is no dmesg errors in many error flows.
Do not think that we need special functional tests.

> Also, IMO we need a line here to explain that we're adding the code for
> HECI1 as well because we plan to follow up with it soon.
> 
Will add such line

> >
> > CC: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > Signed-off-by: Tomas Winkler <tomas.winkler@xxxxxxxxx>
> > Signed-off-by: Vitaly Lubart <vitaly.lubart@xxxxxxxxx>
> > Signed-off-by: Alexander Usyskin <alexander.usyskin@xxxxxxxxx>
> > Acked-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > ---
> >   MAINTAINERS                              |   1 +
> >   drivers/gpu/drm/i915/Kconfig             |   1 +
> >   drivers/gpu/drm/i915/Makefile            |   3 +
> >   drivers/gpu/drm/i915/gt/intel_gsc.c      | 204
> +++++++++++++++++++++++
> >   drivers/gpu/drm/i915/gt/intel_gsc.h      |  37 ++++
> >   drivers/gpu/drm/i915/gt/intel_gt.c       |   3 +
> >   drivers/gpu/drm/i915/gt/intel_gt.h       |   5 +
> >   drivers/gpu/drm/i915/gt/intel_gt_irq.c   |  13 ++
> >   drivers/gpu/drm/i915/gt/intel_gt_regs.h  |   1 +
> >   drivers/gpu/drm/i915/gt/intel_gt_types.h |   2 +
> >   drivers/gpu/drm/i915/i915_drv.h          |   8 +
> >   drivers/gpu/drm/i915/i915_pci.c          |   3 +-
> >   drivers/gpu/drm/i915/i915_reg.h          |   2 +
> >   drivers/gpu/drm/i915/intel_device_info.h |   2 +
> >   include/linux/mei_aux.h                  |  19 +++
> >   15 files changed, 303 insertions(+), 1 deletion(-)
> >   create mode 100644 drivers/gpu/drm/i915/gt/intel_gsc.c
> >   create mode 100644 drivers/gpu/drm/i915/gt/intel_gsc.h
> >   create mode 100644 include/linux/mei_aux.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 2b1d296f92e9..d322e630d1d1 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -9822,6 +9822,7 @@ S:	Supported
> >   F:	Documentation/driver-api/mei/*
> >   F:	drivers/misc/mei/
> >   F:	drivers/watchdog/mei_wdt.c
> > +F:	include/linux/mei_aux.h
> >   F:	include/linux/mei_cl_bus.h
> >   F:	include/uapi/linux/mei.h
> >   F:	samples/mei/*
> > diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
> > index 98c5450b8eac..2660a85175d9 100644
> > --- a/drivers/gpu/drm/i915/Kconfig
> > +++ b/drivers/gpu/drm/i915/Kconfig
> > @@ -30,6 +30,7 @@ config DRM_I915
> >   	select VMAP_PFN
> >   	select DRM_TTM
> >   	select DRM_BUDDY
> > +	select AUXILIARY_BUS
> >   	help
> >   	  Choose this option if you have a system that has "Intel Graphics
> >   	  Media Accelerator" or "HD Graphics" integrated graphics,
> > diff --git a/drivers/gpu/drm/i915/Makefile
> b/drivers/gpu/drm/i915/Makefile
> > index 1a771ee5b1d0..9be7b13d8822 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -196,6 +196,9 @@ i915-y += gt/uc/intel_uc.o \
> >   	  gt/uc/intel_huc_debugfs.o \
> >   	  gt/uc/intel_huc_fw.o
> >
> > +# graphics system controller (GSC) support
> > +i915-y += gt/intel_gsc.o
> > +
> >   # modesetting core code
> >   i915-y += \
> >   	display/hsw_ips.o \
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gsc.c
> b/drivers/gpu/drm/i915/gt/intel_gsc.c
> > new file mode 100644
> > index 000000000000..152804e7c41a
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/gt/intel_gsc.c
> > @@ -0,0 +1,204 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright(c) 2019-2022, Intel Corporation. All rights reserved.
> > + */
> > +
> > +#include <linux/irq.h>
> > +#include <linux/mei_aux.h>
> > +#include "i915_reg.h"
> > +#include "i915_drv.h"
> > +#include "gt/intel_gt.h"
> > +#include "intel_gsc.h"
> 
> A bit of inconsistency here because intel_gsc.h and intel_gt.h are both
> in the gt/ folder but you're only prefixing one with gt/. Also, we
> usually try to keep includes in alphabetical order, but overall not a
> blocker for me.
> 
Well set both to gt/ and reshuffle.

> > +
> > +#define GSC_BAR_LENGTH  0x00000FFC
> > +
> > +static void gsc_irq_mask(struct irq_data *d)
> > +{
> > +	/* generic irq handling */
> > +}
> > +
> > +static void gsc_irq_unmask(struct irq_data *d)
> > +{
> > +	/* generic irq handling */
> > +}
> > +
> > +static struct irq_chip gsc_irq_chip = {
> > +	.name = "gsc_irq_chip",
> > +	.irq_mask = gsc_irq_mask,
> > +	.irq_unmask = gsc_irq_unmask,
> > +};
> > +
> > +static int gsc_irq_init(int irq)
> > +{
> > +	irq_set_chip_and_handler_name(irq, &gsc_irq_chip,
> > +				      handle_simple_irq, "gsc_irq_handler");
> > +
> > +	return irq_set_chip_data(irq, NULL);
> > +}
> > +
> > +struct intel_gsc_def {
> > +	const char *name;
> > +	unsigned long bar;
> > +	size_t bar_size;
> > +};
> > +
> > +/* gscfi (graphics system controller firmware interface) resources */
> 
> Should this comment be moved down in the array? the array has sections
> for both pxp and gscfi resources, even if the former are not set. Or
> maybe expand to something like:
> 
> /* for DG1 we only need gscfi (....) resources */
> 
I'll change it to describe the array in whole

> > +static const struct intel_gsc_def intel_gsc_def_dg1[] = {
> > +	{
> > +		/* HECI1 not yet implemented. */
> > +	},
> > +	{
> > +		.name = "mei-gscfi",
> > +		.bar = GSC_DG1_HECI2_BASE,
> > +		.bar_size = GSC_BAR_LENGTH,
> > +	}
> > +};
> > +
> > +static void intel_gsc_release_dev(struct device *dev)
> 
> We usually avoid the intel_* prefix for static functions. Same for other
> functions below.
> 
Ok, will drop intel_ prefix from static

> > +{
> > +	struct auxiliary_device *aux_dev = to_auxiliary_dev(dev);
> > +	struct mei_aux_device *adev =
> auxiliary_dev_to_mei_aux_dev(aux_dev);
> > +
> > +	kfree(adev);
> > +}
> > +
> > +static void intel_gsc_destroy_one(struct intel_gsc_intf *intf)
> > +{
> > +	if (intf->adev) {
> > +		auxiliary_device_delete(&intf->adev->aux_dev);
> > +		auxiliary_device_uninit(&intf->adev->aux_dev);
> > +		intf->adev = NULL;
> > +	}
> > +	if (intf->irq >= 0)
> > +		irq_free_desc(intf->irq);
> > +	intf->irq = -1;
> > +}
> > +
> > +static void intel_gsc_init_one(struct drm_i915_private *i915,
> > +			       struct intel_gsc_intf *intf,
> > +			       unsigned int intf_id)
> > +{
> > +	struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
> > +	struct mei_aux_device *adev;
> > +	struct auxiliary_device *aux_dev;
> > +	const struct intel_gsc_def *def;
> > +	int ret;
> > +
> > +	intf->irq = -1;
> > +	intf->id = intf_id;
> > +
> > +	if (intf_id == 0 && !HAS_HECI_PXP(i915))
> > +		return;
> > +
> > +	def = &intel_gsc_def_dg1[intf_id];
> > +
> > +	if (!def->name) {
> > +		drm_warn_once(&i915->drm, "HECI%d is not
> implemented!\n", intf_id + 1);
> > +		return;
> > +	}
> > +
> > +	intf->irq = irq_alloc_desc(0);
> > +	if (intf->irq < 0) {
> > +		drm_err(&i915->drm, "gsc irq error %d\n", intf->irq);
> > +		return;
> > +	}
> > +
> > +	ret = gsc_irq_init(intf->irq);
> > +	if (ret < 0) {
> > +		drm_err(&i915->drm, "gsc irq init failed %d\n", ret);
> > +		goto fail;
> > +	}
> > +
> > +	adev = kzalloc(sizeof(*adev), GFP_KERNEL);
> > +	if (!adev)
> > +		goto fail;
> > +
> > +	adev->irq = intf->irq;
> > +	adev->bar.parent = &pdev->resource[0];
> > +	adev->bar.start = def->bar + pdev->resource[0].start;
> > +	adev->bar.end = adev->bar.start + def->bar_size - 1;
> > +	adev->bar.flags = IORESOURCE_MEM;
> > +	adev->bar.desc = IORES_DESC_NONE;
> > +
> > +	aux_dev = &adev->aux_dev;
> > +	aux_dev->name = def->name;
> > +	aux_dev->id = (pci_domain_nr(pdev->bus) << 16) |
> > +		      PCI_DEVID(pdev->bus->number, pdev->devfn);
> > +	aux_dev->dev.parent = &pdev->dev;
> > +	aux_dev->dev.release = intel_gsc_release_dev;
> > +
> > +	ret = auxiliary_device_init(aux_dev);
> > +	if (ret < 0) {
> > +		drm_err(&i915->drm, "gsc aux init failed %d\n", ret);
> > +		kfree(adev);
> > +		goto fail;
> > +	}
> > +
> > +	ret = auxiliary_device_add(aux_dev);
> > +	if (ret < 0) {
> > +		drm_err(&i915->drm, "gsc aux add failed %d\n", ret);
> > +		/* adev will be freed with the put_device() and .release
> sequence */
> > +		auxiliary_device_uninit(aux_dev);
> > +		goto fail;
> > +	}
> > +	intf->adev = adev;
> > +
> > +	return;
> > +fail:
> > +	intel_gsc_destroy_one(intf);
> > +}
> > +
> > +static void gsc_irq_handler(struct intel_gt *gt, unsigned int intf_id)
> > +{
> > +	int ret;
> > +
> > +	if (intf_id >= INTEL_GSC_NUM_INTERFACES) {
> > +		drm_warn_once(&gt->i915->drm, "GSC irq: intf_id %d is out
> of range", intf_id);
> > +		return;
> > +	}
> > +
> > +	if (!HAS_HECI_GSC(gt->i915)) {
> > +		drm_warn_once(&gt->i915->drm, "GSC irq: not supported");
> > +		return;
> > +	}
> > +
> > +	if (gt->gsc.intf[intf_id].irq < 0) {
> > +		drm_err_ratelimited(&gt->i915->drm, "GSC irq: irq not set");
> > +		return;
> > +	}
> > +
> > +	ret = generic_handle_irq(gt->gsc.intf[intf_id].irq);
> > +	if (ret)
> > +		drm_err_ratelimited(&gt->i915->drm, "error handling GSC
> irq: %d\n", ret);
> > +}
> > +
> > +void intel_gsc_irq_handler(struct intel_gt *gt, u32 iir)
> > +{
> > +	if (iir & GSC_IRQ_INTF(0))
> > +		gsc_irq_handler(gt, 0);
> > +	if (iir & GSC_IRQ_INTF(1))
> > +		gsc_irq_handler(gt, 1);
> > +}
> > +
> > +void intel_gsc_init(struct intel_gsc *gsc, struct drm_i915_private *i915)
> > +{
> > +	unsigned int i;
> > +
> > +	if (!HAS_HECI_GSC(i915))
> > +		return;
> > +
> > +	for (i = 0; i < INTEL_GSC_NUM_INTERFACES; i++)
> > +		intel_gsc_init_one(i915, &gsc->intf[i], i);
> > +}
> > +
> > +void intel_gsc_fini(struct intel_gsc *gsc)
> > +{
> > +	struct intel_gt *gt = gsc_to_gt(gsc);
> > +	unsigned int i;
> > +
> > +	if (!HAS_HECI_GSC(gt->i915))
> > +		return;
> > +
> > +	for (i = 0; i < INTEL_GSC_NUM_INTERFACES; i++)
> > +		intel_gsc_destroy_one(&gsc->intf[i]);
> > +}
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gsc.h
> b/drivers/gpu/drm/i915/gt/intel_gsc.h
> > new file mode 100644
> > index 000000000000..68582f912b21
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/gt/intel_gsc.h
> > @@ -0,0 +1,37 @@
> > +/* SPDX-License-Identifier: MIT */
> > +/*
> > + * Copyright(c) 2019-2022, Intel Corporation. All rights reserved.
> > + */
> > +#ifndef __INTEL_GSC_DEV_H__
> > +#define __INTEL_GSC_DEV_H__
> > +
> > +#include <linux/types.h>
> > +
> > +struct drm_i915_private;
> > +struct intel_gt;
> > +struct mei_aux_device;
> > +
> > +#define INTEL_GSC_NUM_INTERFACES 2
> > +/*
> > + * The HECI1 bit corresponds to bit15 and HECI2 to bit14.
> > + * The reason for this is to allow growth for more interfaces in the future.
> > + */
> > +#define GSC_IRQ_INTF(_x)  BIT(15 - (_x))
> > +
> > +/**
> > + * struct intel_gsc - graphics security controller
> > + * @intf : gsc interface
> > + */
> > +struct intel_gsc {
> > +	struct intel_gsc_intf {
> > +		struct mei_aux_device *adev;
> > +		int irq;
> > +		unsigned int id;
> > +	} intf[INTEL_GSC_NUM_INTERFACES];
> > +};
> > +
> > +void intel_gsc_init(struct intel_gsc *gsc, struct drm_i915_private
> *dev_priv);
> > +void intel_gsc_fini(struct intel_gsc *gsc);
> > +void intel_gsc_irq_handler(struct intel_gt *gt, u32 iir);
> > +
> > +#endif /* __INTEL_GSC_DEV_H__ */
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c
> b/drivers/gpu/drm/i915/gt/intel_gt.c
> > index 8a2483ccbfb9..fd83ab4b8849 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> > @@ -444,6 +444,8 @@ void intel_gt_chipset_flush(struct intel_gt *gt)
> >
> >   void intel_gt_driver_register(struct intel_gt *gt)
> >   {
> > +	intel_gsc_init(&gt->gsc, gt->i915);
> > +
> >   	intel_rps_driver_register(&gt->rps);
> >
> >   	intel_gt_debugfs_register(gt);
> > @@ -766,6 +768,7 @@ void intel_gt_driver_unregister(struct intel_gt *gt)
> >   	intel_wakeref_t wakeref;
> >
> >   	intel_rps_driver_unregister(&gt->rps);
> > +	intel_gsc_fini(&gt->gsc);
> >
> >   	intel_pxp_fini(&gt->pxp);
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h
> b/drivers/gpu/drm/i915/gt/intel_gt.h
> > index 0f571c8ee22b..de779a505c21 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt.h
> > @@ -34,6 +34,11 @@ static inline struct intel_gt *huc_to_gt(struct
> intel_huc *huc)
> >   	return container_of(huc, struct intel_gt, uc.huc);
> >   }
> >
> > +static inline struct intel_gt *gsc_to_gt(struct intel_gsc *gsc)
> > +{
> > +	return container_of(gsc, struct intel_gt, gsc);
> > +}
> > +
> >   void intel_gt_init_early(struct intel_gt *gt, struct drm_i915_private *i915);
> >   void __intel_gt_init_early(struct intel_gt *gt, struct drm_i915_private
> *i915);
> >   int intel_gt_assign_ggtt(struct intel_gt *gt);
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> > index e443ac4c8059..917b85d0c189 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> > @@ -68,6 +68,9 @@ gen11_other_irq_handler(struct intel_gt *gt, const u8
> instance,
> >   	if (instance == OTHER_KCR_INSTANCE)
> >   		return intel_pxp_irq_handler(&gt->pxp, iir);
> >
> > +	if (instance == OTHER_GSC_INSTANCE)
> > +		return intel_gsc_irq_handler(gt, iir);
> > +
> >   	WARN_ONCE(1, "unhandled other interrupt instance=0x%x,
> iir=0x%x\n",
> >   		  instance, iir);
> >   }
> > @@ -184,6 +187,8 @@ void gen11_gt_irq_reset(struct intel_gt *gt)
> >   	intel_uncore_write(uncore, GEN11_VCS_VECS_INTR_ENABLE,	  0);
> >   	if (CCS_MASK(gt))
> >   		intel_uncore_write(uncore,
> GEN12_CCS_RSVD_INTR_ENABLE, 0);
> > +	if (HAS_HECI_GSC(gt->i915))
> > +		intel_uncore_write(uncore,
> GEN11_GUNIT_CSME_INTR_ENABLE, 0);
> >
> >   	/* Restore masks irqs on RCS, BCS, VCS and VECS engines. */
> >   	intel_uncore_write(uncore, GEN11_RCS0_RSVD_INTR_MASK,	~0);
> > @@ -201,6 +206,8 @@ void gen11_gt_irq_reset(struct intel_gt *gt)
> >   		intel_uncore_write(uncore,
> GEN12_CCS0_CCS1_INTR_MASK, ~0);
> >   	if (HAS_ENGINE(gt, CCS2) || HAS_ENGINE(gt, CCS3))
> >   		intel_uncore_write(uncore,
> GEN12_CCS2_CCS3_INTR_MASK, ~0);
> > +	if (HAS_HECI_GSC(gt->i915))
> > +		intel_uncore_write(uncore,
> GEN11_GUNIT_CSME_INTR_MASK, ~0);
> >
> >   	intel_uncore_write(uncore,
> GEN11_GPM_WGBOXPERF_INTR_ENABLE, 0);
> >   	intel_uncore_write(uncore,
> GEN11_GPM_WGBOXPERF_INTR_MASK,  ~0);
> > @@ -215,6 +222,7 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt)
> >   {
> >   	struct intel_uncore *uncore = gt->uncore;
> >   	u32 irqs = GT_RENDER_USER_INTERRUPT;
> > +	const u32 gsc_mask = GSC_IRQ_INTF(0) | GSC_IRQ_INTF(1);
> >   	u32 dmask;
> >   	u32 smask;
> >
> > @@ -233,6 +241,9 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt)
> >   	intel_uncore_write(uncore, GEN11_VCS_VECS_INTR_ENABLE,
> dmask);
> >   	if (CCS_MASK(gt))
> >   		intel_uncore_write(uncore,
> GEN12_CCS_RSVD_INTR_ENABLE, smask);
> > +	if (HAS_HECI_GSC(gt->i915))
> > +		intel_uncore_write(uncore,
> GEN11_GUNIT_CSME_INTR_ENABLE,
> > +				   gsc_mask);
> >
> >   	/* Unmask irqs on RCS, BCS, VCS and VECS engines. */
> >   	intel_uncore_write(uncore, GEN11_RCS0_RSVD_INTR_MASK,
> ~smask);
> > @@ -250,6 +261,8 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt)
> >   		intel_uncore_write(uncore,
> GEN12_CCS0_CCS1_INTR_MASK, ~dmask);
> >   	if (HAS_ENGINE(gt, CCS2) || HAS_ENGINE(gt, CCS3))
> >   		intel_uncore_write(uncore,
> GEN12_CCS2_CCS3_INTR_MASK, ~dmask);
> > +	if (HAS_HECI_GSC(gt->i915))
> > +		intel_uncore_write(uncore,
> GEN11_GUNIT_CSME_INTR_MASK, 0);
> 
> This should be ~gsc_mask, right?
> 
Yes, sure, rebase fallout. Thanks for catching this!

> >
> >   	/*
> >   	 * RPS interrupts will get enabled/disabled on demand when RPS
> itself
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > index 19cd34f24263..a277fb480cc8 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > @@ -1483,6 +1483,7 @@
> >   #define   OTHER_GUC_INSTANCE			0
> >   #define   OTHER_GTPM_INSTANCE			1
> >   #define   OTHER_KCR_INSTANCE			4
> > +#define   OTHER_GSC_INSTANCE			6
> >
> >   #define GEN11_IIR_REG_SELECTOR(x)		_MMIO(0x190070 +
> ((x) * 4))
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > index f20687796490..5556d55f76ea 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > @@ -16,6 +16,7 @@
> >   #include <linux/workqueue.h>
> >
> >   #include "uc/intel_uc.h"
> > +#include "intel_gsc.h"
> >
> >   #include "i915_vma.h"
> >   #include "intel_engine_types.h"
> > @@ -72,6 +73,7 @@ struct intel_gt {
> >   	struct i915_ggtt *ggtt;
> >
> >   	struct intel_uc uc;
> > +	struct intel_gsc gsc;
> >
> >   	struct mutex tlb_invalidate_lock;
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> > index 943267393ecb..1c000c15493d 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1327,6 +1327,14 @@ IS_SUBPLATFORM(const struct
> drm_i915_private *i915,
> >
> >   #define HAS_DMC(dev_priv)	(INTEL_INFO(dev_priv)-
> >display.has_dmc)
> >
> > +#define HAS_HECI_PXP(dev_priv) \
> > +	(INTEL_INFO(dev_priv)->has_heci_pxp)
> > +
> > +#define HAS_HECI_GSCFI(dev_priv) \
> > +	(INTEL_INFO(dev_priv)->has_heci_gscfi)
> > +
> > +#define HAS_HECI_GSC(dev_priv) (HAS_HECI_PXP(dev_priv) ||
> HAS_HECI_GSCFI(dev_priv))
> > +
> >   #define HAS_MSO(i915)		(DISPLAY_VER(i915) >= 12)
> >
> >   #define HAS_RUNTIME_PM(dev_priv) (INTEL_INFO(dev_priv)-
> >has_runtime_pm)
> > diff --git a/drivers/gpu/drm/i915/i915_pci.c
> b/drivers/gpu/drm/i915/i915_pci.c
> > index 67b89769f577..a948f566bd3d 100644
> > --- a/drivers/gpu/drm/i915/i915_pci.c
> > +++ b/drivers/gpu/drm/i915/i915_pci.c
> > @@ -901,7 +901,8 @@ static const struct intel_device_info rkl_info = {
> >   	.has_llc = 0, \
> >   	.has_pxp = 0, \
> >   	.has_snoop = 1, \
> > -	.is_dgfx = 1
> > +	.is_dgfx = 1, \
> > +	.has_heci_gscfi = 1
> >
> >   static const struct intel_device_info dg1_info = {
> >   	GEN12_FEATURES,
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> > index 70484f6f2b8b..0ed305ff07a9 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -975,6 +975,8 @@
> >   #define GEN12_COMPUTE1_RING_BASE	0x1c000
> >   #define GEN12_COMPUTE2_RING_BASE	0x1e000
> >   #define GEN12_COMPUTE3_RING_BASE	0x26000
> > +#define GSC_DG1_HECI1_BASE	0x00258000
> 
> Do we need the DG1 HECI1 base definition, considering we're not adding
> HECI1 support for DG1?

There are other usages of this offset in follow up patches also when
MEI is not going to support this interface.

> Also, we usually put the platform name (DG1) before the unit (GSC) in
> our defines, so DG1_GSC_HECI* would probably be more inline with the
> rest of the defs.

Will rename as suggested

> 
> > +#define GSC_DG1_HECI2_BASE	0x00259000
> >   #define BLT_RING_BASE		0x22000
> 
> IMO better to move the GSC defs to after the BLT, that way we don't
> separate it from the other RING_BASE definitions, given that the GSC is
> not a RING and its base is something different. Not a blocker.
> 

Sure, will move as rebasing anyway

> Daniele
> 
> >
> >
> > diff --git a/drivers/gpu/drm/i915/intel_device_info.h
> b/drivers/gpu/drm/i915/intel_device_info.h
> > index f9b955810593..576d15a04c9e 100644
> > --- a/drivers/gpu/drm/i915/intel_device_info.h
> > +++ b/drivers/gpu/drm/i915/intel_device_info.h
> > @@ -141,6 +141,8 @@ enum intel_ppgtt_type {
> >   	func(has_flat_ccs); \
> >   	func(has_global_mocs); \
> >   	func(has_gt_uc); \
> > +	func(has_heci_pxp); \
> > +	func(has_heci_gscfi); \
> >   	func(has_guc_deprivilege); \
> >   	func(has_l3_dpf); \
> >   	func(has_llc); \
> > diff --git a/include/linux/mei_aux.h b/include/linux/mei_aux.h
> > new file mode 100644
> > index 000000000000..587f25128848
> > --- /dev/null
> > +++ b/include/linux/mei_aux.h
> > @@ -0,0 +1,19 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2022, Intel Corporation. All rights reserved.
> > + */
> > +#ifndef _LINUX_MEI_AUX_H
> > +#define _LINUX_MEI_AUX_H
> > +
> > +#include <linux/auxiliary_bus.h>
> > +
> > +struct mei_aux_device {
> > +	struct auxiliary_device aux_dev;
> > +	int irq;
> > +	struct resource bar;
> > +};
> > +
> > +#define auxiliary_dev_to_mei_aux_dev(auxiliary_dev) \
> > +	container_of(auxiliary_dev, struct mei_aux_device, aux_dev)
> > +
> > +#endif /* _LINUX_MEI_AUX_H */

-- 
Thanks,
Sasha






[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux