[v7,1/5] drm/i915/gsc: add gsc as a mei auxiliary device

Commit Message

Alexander Usyskin Feb. 13, 2022, 10:32 a.m. UTC

From: Tomas Winkler <[e-mail]>

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.

CC: Rodrigo Vivi <[e-mail]>
Reviewed-by: Greg Kroah-Hartman <[e-mail]>
Signed-off-by: Tomas Winkler <[e-mail]>
Signed-off-by: Vitaly Lubart <[e-mail]>
Signed-off-by: Alexander Usyskin <[e-mail]>
---
V4: add header to the MAINTAINERS file
    drop empty line
V5: rebase
V6: rebase, drop redundant assignments
V7: add Greg KH Reviewed-by
---
 MAINTAINERS                              |   1 +
 drivers/gpu/drm/i915/Kconfig             |   1 +
 drivers/gpu/drm/i915/Makefile            |   3 +
 drivers/gpu/drm/i915/gt/intel_gsc.c      | 196 +++++++++++++++++++++++
 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, 295 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

Comments

Tvrtko Ursulin Feb. 15, 2022, 12:30 p.m. UTC | #1

On 13/02/2022 10:32, Alexander Usyskin wrote:
> From: Tomas Winkler <[e-mail]>
> 
> 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.
> 
> CC: Rodrigo Vivi <[e-mail]>
> Reviewed-by: Greg Kroah-Hartman <[e-mail]>
> Signed-off-by: Tomas Winkler <[e-mail]>
> Signed-off-by: Vitaly Lubart <[e-mail]>
> Signed-off-by: Alexander Usyskin <[e-mail]>
> ---
> V4: add header to the MAINTAINERS file
>      drop empty line
> V5: rebase
> V6: rebase, drop redundant assignments
> V7: add Greg KH Reviewed-by
> ---
>   MAINTAINERS                              |   1 +
>   drivers/gpu/drm/i915/Kconfig             |   1 +
>   drivers/gpu/drm/i915/Makefile            |   3 +
>   drivers/gpu/drm/i915/gt/intel_gsc.c      | 196 +++++++++++++++++++++++
>   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, 295 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 a0c2ae3d71d8..c92d34f8522a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9797,6 +9797,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 2ac220bfd0ed..3b0508e7a805 100644
> --- a/drivers/gpu/drm/i915/Kconfig
> +++ b/drivers/gpu/drm/i915/Kconfig
> @@ -29,6 +29,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 226dbef5f64a..e40ba71b3cdd 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -195,6 +195,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..d07f182f64e4
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/intel_gsc.c
> @@ -0,0 +1,196 @@
> +// 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"
> +
> +#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(struct drm_i915_private *dev_priv, int irq)

I am not insisting but we tend to use i915 for the variable name instead 
of dev_priv since some time back.

> +{
> +	irq_set_chip_and_handler_name(irq, &gsc_irq_chip,
> +				      handle_simple_irq, "gsc_irq_handler");
> +
> +	return irq_set_chip_data(irq, dev_priv);

I am not familiar with this interrupt scheme - does dev_priv get used at 
all by handle_simple_irq, or anyone, after being set here?

> +}
> +
> +struct intel_gsc_def {
> +	const char *name;
> +	const unsigned long bar;

Unusual, why const out of curiosity? And is it "bar" or "base" would be 
more accurate?

> +	size_t bar_size;
> +};
> +
> +/* gscfi (graphics system controller firmware interface) resources */
> +static const struct intel_gsc_def intel_gsc_def_dg1[] = {
> +	{
> +	},

Maybe put a comment in this empty element like "/* 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)
> +{
> +	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 *dev_priv,
> +			       struct intel_gsc_intf *intf,
> +			       unsigned int intf_id)

Could pass in intel_gsc instead of dev_priv to better match with 
intel_gsc prefix, but no big deal.

> +{
> +	struct pci_dev *pdev = to_pci_dev(dev_priv->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(dev_priv))
> +		return;

Isn't inf_id == 0 always a bug with this patch, regardless of 
HAS_HECI_PXP, since the support is incomplete in this patch? If so I'd 
be more comfortable with a plain drm_WARN_ON_ONCE(intf_id == 0).

> +
> +	def = &intel_gsc_def_dg1[intf_id];
> +
> +	dev_dbg(&pdev->dev, "init gsc one with id %d\n", intf_id);

Meh, too much looks like a during development aid.

> +	intf->irq = irq_alloc_desc(0);
> +	if (intf->irq < 0) {
> +		dev_err(&pdev->dev, "gsc irq error %d\n", intf->irq);
> +		return;
> +	}
> +
> +	ret = gsc_irq_init(dev_priv, intf->irq);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "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) {
> +		dev_err(&pdev->dev, "gsc aux init failed %d\n", ret);
> +		kfree(adev);
> +		goto fail;
> +	}
> +
> +	ret = auxiliary_device_add(aux_dev);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "gsc aux add failed %d\n", ret);
> +		/* adev will be freed with the put_device() and .release sequence */

For things which fail but do not prevent driver loading and operating 
otherwise correctly I'd suggest drm_warn throughout but maybe it is just me.

> +		auxiliary_device_uninit(aux_dev);
> +		goto fail;
> +	}
> +	intf->adev = adev;
> +
> +	dev_dbg(&pdev->dev, "gsc init one done\n");
> +	return;
> +fail:
> +	intel_gsc_destroy_one(intf);

Error handling is a bit meh but looks correct. Like you could avoid 
separate free's on the error paths by doing onion unwind, or even full 
onion unwind instead of conditional teardown in intel_gsc_destroy_one. 
Not asking for it, just commenting.

> +}
> +
> +static void intel_gsc_irq_handler(struct intel_gt *gt, unsigned int intf_id)
> +{
> +	int ret;
> +
> +	if (intf_id >= INTEL_GSC_NUM_INTERFACES)
> +		return;

Is this worthy of a WARN_ON or drm_warn? (ratelimited I suppose) Because 
it would be either bad hardware or a bad driver error to see the 
interrupt with wrong interface, no?

> +
> +	if (!HAS_HECI_GSC(gt->i915))
> +		return;

Likewise?

> +
> +	if (gt->gsc.intf[intf_id].irq <= 0) {
> +		DRM_ERROR_RATELIMITED("error handling GSC irq: irq not set");

Like this, but use logging functions which say which device please.

> +		return;
> +	}
> +
> +	ret = generic_handle_irq(gt->gsc.intf[intf_id].irq);
> +	if (ret)
> +		DRM_ERROR_RATELIMITED("error handling GSC irq: %d\n", ret);
> +}
> +
> +void gsc_irq_handler(struct intel_gt *gt, u32 iir)
> +{
> +	if (iir & GSC_IRQ_INTF(0))
> +		intel_gsc_irq_handler(gt, 0);
> +	if (iir & GSC_IRQ_INTF(1))
> +		intel_gsc_irq_handler(gt, 1);
> +}
> +
> +void intel_gsc_init(struct intel_gsc *gsc, struct drm_i915_private *dev_priv)

Here you don't need to pass in dev_priv since you can get it with 
container_of and gt->i915 but up to you, no big deal.

> +{
> +	unsigned int i;
> +
> +	if (!HAS_HECI_GSC(dev_priv))
> +		return;
> +
> +	for (i = 0; i < INTEL_GSC_NUM_INTERFACES; i++)
> +		intel_gsc_init_one(dev_priv, &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..67cb2a75f839
> --- /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 gsc_irq_handler(struct intel_gt *gt, u32 iir);

Why not have consistent prefix on all exported functions?

> +
> +#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 e8403fa53909..18961d154956 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -446,6 +446,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 2dad46c3eff2..6ba817c02baa 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 983264e10e0a..d36f919bbdf5 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 gsc_irq_handler(gt, iir);
> +
>   	WARN_ONCE(1, "unhandled other interrupt instance=0x%x, iir=0x%x\n",
>   		  instance, iir);

Digression - it would be nice to be able to report unhandled iir bits 
here but all handlers return void so refactoring would be needed.

>   }
> @@ -182,6 +185,8 @@ void gen11_gt_irq_reset(struct intel_gt *gt)
>   	/* Disable RCS, BCS, VCS and VECS class engines. */
>   	intel_uncore_write(uncore, GEN11_RENDER_COPY_INTR_ENABLE, 0);
>   	intel_uncore_write(uncore, GEN11_VCS_VECS_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);
> @@ -195,6 +200,8 @@ void gen11_gt_irq_reset(struct intel_gt *gt)
>   	intel_uncore_write(uncore, GEN11_VECS0_VECS1_INTR_MASK,	~0);
>   	if (HAS_ENGINE(gt, VECS2) || HAS_ENGINE(gt, VECS3))
>   		intel_uncore_write(uncore, GEN12_VECS2_VECS3_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);
> @@ -209,6 +216,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);

Why enable the one which is not supported by the patch? No harm doing it?

>   	u32 dmask;
>   	u32 smask;
>   
> @@ -225,6 +233,9 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt)
>   	/* Enable RCS, BCS, VCS and VECS class interrupts. */
>   	intel_uncore_write(uncore, GEN11_RENDER_COPY_INTR_ENABLE, dmask);
>   	intel_uncore_write(uncore, GEN11_VCS_VECS_INTR_ENABLE, dmask);
> +	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);
> @@ -238,6 +249,8 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt)
>   	intel_uncore_write(uncore, GEN11_VECS0_VECS1_INTR_MASK, ~dmask);
>   	if (HAS_ENGINE(gt, VECS2) || HAS_ENGINE(gt, VECS3))
>   		intel_uncore_write(uncore, GEN12_VECS2_VECS3_INTR_MASK, ~dmask);
> +	if (HAS_HECI_GSC(gt->i915))
> +		intel_uncore_write(uncore, GEN11_GUNIT_CSME_INTR_MASK, 0);
>   	/*
>   	 * RPS interrupts will get enabled/disabled on demand when RPS itself
>   	 * is enabled/disabled.
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> index a6f0220c2e9f..427f91900afc 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> @@ -944,6 +944,7 @@ enum {
>   #define OTHER_GUC_INSTANCE	0
>   #define OTHER_GTPM_INSTANCE	1
>   #define OTHER_KCR_INSTANCE	4
> +#define OTHER_GSC_INSTANCE	6
>   
>   #define GEN11_INTR_IDENTITY_REG(x)	_MMIO(0x190060 + ((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 4f057a45654a..b41871abe776 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1444,6 +1444,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 467252f885c2..8f608b03d30b 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -900,7 +900,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 ca6adfc234c0..e74934255198 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -974,6 +974,8 @@
>   #define GEN11_VEBOX2_RING_BASE		0x1d8000
>   #define XEHP_VEBOX3_RING_BASE		0x1e8000
>   #define XEHP_VEBOX4_RING_BASE		0x1f8000
> +#define GSC_DG1_HECI1_BASE	0x00258000
> +#define GSC_DG1_HECI2_BASE	0x00259000
>   #define BLT_RING_BASE		0x22000
>   
>   
> diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
> index 27dcfe6f2429..fa75f464cb01 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.h
> +++ b/drivers/gpu/drm/i915/intel_device_info.h
> @@ -135,6 +135,8 @@ enum intel_ppgtt_type {
>   	func(has_reset_engine); \
>   	func(has_global_mocs); \
>   	func(has_gt_uc); \
> +	func(has_heci_pxp); \

Presumably this will be used soon? (And the variety of HAS_ macros.)

> +	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 */

In summary I think prefix on the exported function and 
drm_err_ratelimited were the only comments which need action. The rest 
were either questions of suggestions on how to polish it a bit more so 
it aligns better with established i915 patterns.

Regards,

Tvrtko

Alexander Usyskin Feb. 15, 2022, 3:22 p.m. UTC | #2

> -----Original Message-----
> From: Tvrtko Ursulin <[e-mail]>
> Sent: Tuesday, February 15, 2022 14:30
> To: Usyskin, Alexander <[e-mail]>; Greg Kroah-
> Hartman <[e-mail]>; Jani Nikula
> <[e-mail]>; Joonas Lahtinen
> <[e-mail]>; Vivi, Rodrigo <[e-mail]>;
> David Airlie <[e-mail]>; Daniel Vetter <[e-mail]>
> Cc: [e-mail]; Winkler, Tomas
> <[e-mail]>; Lubart, Vitaly <[e-mail]>; intel-
> [e-mail]
> Subject: Re: [Intel-gfx] [PATCH v7 1/5] drm/i915/gsc: add gsc as a mei
> auxiliary device
> 
> 
> On 13/02/2022 10:32, Alexander Usyskin wrote:
> > From: Tomas Winkler <[e-mail]>
> >
> > 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.
> >
> > CC: Rodrigo Vivi <[e-mail]>
> > Reviewed-by: Greg Kroah-Hartman <[e-mail]>
> > Signed-off-by: Tomas Winkler <[e-mail]>
> > Signed-off-by: Vitaly Lubart <[e-mail]>
> > Signed-off-by: Alexander Usyskin <[e-mail]>
> > ---
> > V4: add header to the MAINTAINERS file
> >      drop empty line
> > V5: rebase
> > V6: rebase, drop redundant assignments
> > V7: add Greg KH Reviewed-by
> > ---
> >   MAINTAINERS                              |   1 +
> >   drivers/gpu/drm/i915/Kconfig             |   1 +
> >   drivers/gpu/drm/i915/Makefile            |   3 +
> >   drivers/gpu/drm/i915/gt/intel_gsc.c      | 196
> +++++++++++++++++++++++
> >   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, 295 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 a0c2ae3d71d8..c92d34f8522a 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -9797,6 +9797,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 2ac220bfd0ed..3b0508e7a805 100644
> > --- a/drivers/gpu/drm/i915/Kconfig
> > +++ b/drivers/gpu/drm/i915/Kconfig
> > @@ -29,6 +29,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 226dbef5f64a..e40ba71b3cdd 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -195,6 +195,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..d07f182f64e4
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/gt/intel_gsc.c
> > @@ -0,0 +1,196 @@
> > +// 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"
> > +
> > +#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(struct drm_i915_private *dev_priv, int irq)
> 
> I am not insisting but we tend to use i915 for the variable name instead
> of dev_priv since some time back.
> 
Ok, will change to i915

> > +{
> > +	irq_set_chip_and_handler_name(irq, &gsc_irq_chip,
> > +				      handle_simple_irq, "gsc_irq_handler");
> > +
> > +	return irq_set_chip_data(irq, dev_priv);
> 
> I am not familiar with this interrupt scheme - does dev_priv get used at
> all by handle_simple_irq, or anyone, after being set here?
> 
> > +}
> > +
> > +struct intel_gsc_def {
> > +	const char *name;
> > +	const unsigned long bar;
> 
> Unusual, why const out of curiosity? And is it "bar" or "base" would be
> more accurate?
> 
Some leftover, thanks for spotting this!
It is a base of bar. I prefer bar name here. But not really matter.

> > +	size_t bar_size;
> > +};
> > +
> > +/* gscfi (graphics system controller firmware interface) resources */
> > +static const struct intel_gsc_def intel_gsc_def_dg1[] = {
> > +	{
> > +	},
> 
> Maybe put a comment in this empty element like "/* HECI1 not yet
> implemented. */"?
Will add

> 
> > +	{
> > +		.name = "mei-gscfi",
> > +		.bar = GSC_DG1_HECI2_BASE,
> > +		.bar_size = GSC_BAR_LENGTH,
> > +	}
> > +};
> > +
> > +static void intel_gsc_release_dev(struct device *dev)
> > +{
> > +	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 *dev_priv,
> > +			       struct intel_gsc_intf *intf,
> > +			       unsigned int intf_id)
> 
> Could pass in intel_gsc instead of dev_priv to better match with
> intel_gsc prefix, but no big deal.
> 
> > +{
> > +	struct pci_dev *pdev = to_pci_dev(dev_priv->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(dev_priv))
> > +		return;
> 
> Isn't inf_id == 0 always a bug with this patch, regardless of
> HAS_HECI_PXP, since the support is incomplete in this patch? If so I'd
> be more comfortable with a plain drm_WARN_ON_ONCE(intf_id == 0).
> 
There will be patches for other cards that have pxp as soon as this is reviewed.
It is better to have infra prepared for two heads.

> > +
> > +	def = &intel_gsc_def_dg1[intf_id];
> > +
> > +	dev_dbg(&pdev->dev, "init gsc one with id %d\n", intf_id);
> 
> Meh, too much looks like a during development aid.
> 
Will drop.

> > +	intf->irq = irq_alloc_desc(0);
> > +	if (intf->irq < 0) {
> > +		dev_err(&pdev->dev, "gsc irq error %d\n", intf->irq);
> > +		return;
> > +	}
> > +
> > +	ret = gsc_irq_init(dev_priv, intf->irq);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "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) {
> > +		dev_err(&pdev->dev, "gsc aux init failed %d\n", ret);
> > +		kfree(adev);
> > +		goto fail;
> > +	}
> > +
> > +	ret = auxiliary_device_add(aux_dev);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "gsc aux add failed %d\n", ret);
> > +		/* adev will be freed with the put_device() and .release
> sequence */
> 
> For things which fail but do not prevent driver loading and operating
> otherwise correctly I'd suggest drm_warn throughout but maybe it is just
> me.
> 
> > +		auxiliary_device_uninit(aux_dev);
> > +		goto fail;
> > +	}
> > +	intf->adev = adev;
> > +
> > +	dev_dbg(&pdev->dev, "gsc init one done\n");
> > +	return;
> > +fail:
> > +	intel_gsc_destroy_one(intf);
> 
> Error handling is a bit meh but looks correct. Like you could avoid
> separate free's on the error paths by doing onion unwind, or even full
> onion unwind instead of conditional teardown in intel_gsc_destroy_one.
> Not asking for it, just commenting.
> 
> > +}
> > +
> > +static void intel_gsc_irq_handler(struct intel_gt *gt, unsigned int intf_id)
> > +{
> > +	int ret;
> > +
> > +	if (intf_id >= INTEL_GSC_NUM_INTERFACES)
> > +		return;
> 
> Is this worthy of a WARN_ON or drm_warn? (ratelimited I suppose) Because
> it would be either bad hardware or a bad driver error to see the
> interrupt with wrong interface, no?
> 
Will add

> > +
> > +	if (!HAS_HECI_GSC(gt->i915))
> > +		return;
> 
> Likewise?
> 
> > +
> > +	if (gt->gsc.intf[intf_id].irq <= 0) {
> > +		DRM_ERROR_RATELIMITED("error handling GSC irq: irq not
> set");
> 
> Like this, but use logging functions which say which device please.
> 
drm_err_ratelimited fits here?

> > +		return;
> > +	}
> > +
> > +	ret = generic_handle_irq(gt->gsc.intf[intf_id].irq);
> > +	if (ret)
> > +		DRM_ERROR_RATELIMITED("error handling GSC irq: %d\n",
> ret);
> > +}
> > +
> > +void gsc_irq_handler(struct intel_gt *gt, u32 iir)
> > +{
> > +	if (iir & GSC_IRQ_INTF(0))
> > +		intel_gsc_irq_handler(gt, 0);
> > +	if (iir & GSC_IRQ_INTF(1))
> > +		intel_gsc_irq_handler(gt, 1);
> > +}
> > +
> > +void intel_gsc_init(struct intel_gsc *gsc, struct drm_i915_private
> *dev_priv)
> 
> Here you don't need to pass in dev_priv since you can get it with
> container_of and gt->i915 but up to you, no big deal.
> 
> > +{
> > +	unsigned int i;
> > +
> > +	if (!HAS_HECI_GSC(dev_priv))
> > +		return;
> > +
> > +	for (i = 0; i < INTEL_GSC_NUM_INTERFACES; i++)
> > +		intel_gsc_init_one(dev_priv, &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..67cb2a75f839
> > --- /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 gsc_irq_handler(struct intel_gt *gt, u32 iir);
> 
> Why not have consistent prefix on all exported functions?
> 
Will fix

> > +
> > +#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 e8403fa53909..18961d154956 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> > @@ -446,6 +446,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 2dad46c3eff2..6ba817c02baa 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 983264e10e0a..d36f919bbdf5 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 gsc_irq_handler(gt, iir);
> > +
> >   	WARN_ONCE(1, "unhandled other interrupt instance=0x%x,
> iir=0x%x\n",
> >   		  instance, iir);
> 
> Digression - it would be nice to be able to report unhandled iir bits
> here but all handlers return void so refactoring would be needed.
> 
> >   }
> > @@ -182,6 +185,8 @@ void gen11_gt_irq_reset(struct intel_gt *gt)
> >   	/* Disable RCS, BCS, VCS and VECS class engines. */
> >   	intel_uncore_write(uncore, GEN11_RENDER_COPY_INTR_ENABLE,
> 0);
> >   	intel_uncore_write(uncore, GEN11_VCS_VECS_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);
> > @@ -195,6 +200,8 @@ void gen11_gt_irq_reset(struct intel_gt *gt)
> >   	intel_uncore_write(uncore, GEN11_VECS0_VECS1_INTR_MASK,
> 	~0);
> >   	if (HAS_ENGINE(gt, VECS2) || HAS_ENGINE(gt, VECS3))
> >   		intel_uncore_write(uncore,
> GEN12_VECS2_VECS3_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);
> > @@ -209,6 +216,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);
> 
> Why enable the one which is not supported by the patch? No harm doing it?
> 
No harm and the next patch will be soon, this patch unfortunately is long overdue.

> >   	u32 dmask;
> >   	u32 smask;
> >
> > @@ -225,6 +233,9 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt)
> >   	/* Enable RCS, BCS, VCS and VECS class interrupts. */
> >   	intel_uncore_write(uncore, GEN11_RENDER_COPY_INTR_ENABLE,
> dmask);
> >   	intel_uncore_write(uncore, GEN11_VCS_VECS_INTR_ENABLE,
> dmask);
> > +	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);
> > @@ -238,6 +249,8 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt)
> >   	intel_uncore_write(uncore, GEN11_VECS0_VECS1_INTR_MASK,
> ~dmask);
> >   	if (HAS_ENGINE(gt, VECS2) || HAS_ENGINE(gt, VECS3))
> >   		intel_uncore_write(uncore,
> GEN12_VECS2_VECS3_INTR_MASK, ~dmask);
> > +	if (HAS_HECI_GSC(gt->i915))
> > +		intel_uncore_write(uncore,
> GEN11_GUNIT_CSME_INTR_MASK, 0);
> >   	/*
> >   	 * RPS interrupts will get enabled/disabled on demand when RPS
> itself
> >   	 * is enabled/disabled.
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > index a6f0220c2e9f..427f91900afc 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > @@ -944,6 +944,7 @@ enum {
> >   #define OTHER_GUC_INSTANCE	0
> >   #define OTHER_GTPM_INSTANCE	1
> >   #define OTHER_KCR_INSTANCE	4
> > +#define OTHER_GSC_INSTANCE	6
> >
> >   #define GEN11_INTR_IDENTITY_REG(x)	_MMIO(0x190060 + ((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 4f057a45654a..b41871abe776 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1444,6 +1444,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 467252f885c2..8f608b03d30b 100644
> > --- a/drivers/gpu/drm/i915/i915_pci.c
> > +++ b/drivers/gpu/drm/i915/i915_pci.c
> > @@ -900,7 +900,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 ca6adfc234c0..e74934255198 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -974,6 +974,8 @@
> >   #define GEN11_VEBOX2_RING_BASE		0x1d8000
> >   #define XEHP_VEBOX3_RING_BASE		0x1e8000
> >   #define XEHP_VEBOX4_RING_BASE		0x1f8000
> > +#define GSC_DG1_HECI1_BASE	0x00258000
> > +#define GSC_DG1_HECI2_BASE	0x00259000
> >   #define BLT_RING_BASE		0x22000
> >
> >
> > diff --git a/drivers/gpu/drm/i915/intel_device_info.h
> b/drivers/gpu/drm/i915/intel_device_info.h
> > index 27dcfe6f2429..fa75f464cb01 100644
> > --- a/drivers/gpu/drm/i915/intel_device_info.h
> > +++ b/drivers/gpu/drm/i915/intel_device_info.h
> > @@ -135,6 +135,8 @@ enum intel_ppgtt_type {
> >   	func(has_reset_engine); \
> >   	func(has_global_mocs); \
> >   	func(has_gt_uc); \
> > +	func(has_heci_pxp); \
> 
> Presumably this will be used soon? (And the variety of HAS_ macros.)
> 
Yes, I have it staged, but want to have reviews on this one to fix obvious problems

> > +	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 */
> 
> In summary I think prefix on the exported function and
> drm_err_ratelimited were the only comments which need action. The rest
> were either questions of suggestions on how to polish it a bit more so
> it aligns better with established i915 patterns.
> 
> Regards,
> 
> Tvrtko

Thanks for review! Will send a new one with fixes soon.

Tvrtko Ursulin Feb. 16, 2022, 12:03 p.m. UTC | #3

On 15/02/2022 15:22, Usyskin, Alexander wrote:

>>> +{
>>> +	irq_set_chip_and_handler_name(irq, &gsc_irq_chip,
>>> +				      handle_simple_irq, "gsc_irq_handler");
>>> +
>>> +	return irq_set_chip_data(irq, dev_priv);
>>
>> I am not familiar with this interrupt scheme - does dev_priv get used at
>> all by handle_simple_irq, or anyone, after being set here?

What about this? Is dev_priv required or you could pass in NULL just as 
well?

>>
>>> +}
>>> +
>>> +struct intel_gsc_def {
>>> +	const char *name;
>>> +	const unsigned long bar;
>>
>> Unusual, why const out of curiosity? And is it "bar" or "base" would be
>> more accurate?
>>
> Some leftover, thanks for spotting this!
> It is a base of bar. I prefer bar name here. But not really matter.

Is it?

+	adev->bar.start = def->bar + pdev->resource[0].start;

Looks like offset on top of BAR, no?

>>> +{
>>> +	struct pci_dev *pdev = to_pci_dev(dev_priv->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(dev_priv))
>>> +		return;
>>
>> Isn't inf_id == 0 always a bug with this patch, regardless of
>> HAS_HECI_PXP, since the support is incomplete in this patch? If so I'd
>> be more comfortable with a plain drm_WARN_ON_ONCE(intf_id == 0).
>>
> There will be patches for other cards that have pxp as soon as this is reviewed.
> It is better to have infra prepared for two heads.

My point is things are half-prepared since you don't have the id 0 in 
the array, regardless of the HAS_HECI_PXP. Yes it can't be true now, but 
if you add a patch which enables it to be true, you have to modify the 
array at the same time or risk a broken patch in the middle.

I don't see the point of the condition making it sound like there are 
two criteria to enter below, while in fact there is only one in current 
code, and that it that it must not be entered because array is incomplete!

>>> +
>>> +	if (!HAS_HECI_GSC(gt->i915))
>>> +		return;
>>
>> Likewise?
>>
>>> +
>>> +	if (gt->gsc.intf[intf_id].irq <= 0) {
>>> +		DRM_ERROR_RATELIMITED("error handling GSC irq: irq not
>> set");
>>
>> Like this, but use logging functions which say which device please.
>>
> drm_err_ratelimited fits here?

AFAICT it would be a programming bug and not something that can happen 
at runtime hence drm_warn_on_once sounds correct for both.

>>>    }
>>> @@ -182,6 +185,8 @@ void gen11_gt_irq_reset(struct intel_gt *gt)
>>>    	/* Disable RCS, BCS, VCS and VECS class engines. */
>>>    	intel_uncore_write(uncore, GEN11_RENDER_COPY_INTR_ENABLE,
>> 0);
>>>    	intel_uncore_write(uncore, GEN11_VCS_VECS_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);
>>> @@ -195,6 +200,8 @@ void gen11_gt_irq_reset(struct intel_gt *gt)
>>>    	intel_uncore_write(uncore, GEN11_VECS0_VECS1_INTR_MASK,
>> 	~0);
>>>    	if (HAS_ENGINE(gt, VECS2) || HAS_ENGINE(gt, VECS3))
>>>    		intel_uncore_write(uncore,
>> GEN12_VECS2_VECS3_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);
>>> @@ -209,6 +216,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);
>>
>> Why enable the one which is not supported by the patch? No harm doing it?
>>
> No harm and the next patch will be soon, this patch unfortunately is long overdue.

Just feels a bit lazy. You are adding two feature test macros to 
prepare, so why not use them.

Regards,

Tvrtko

Alexander Usyskin Feb. 16, 2022, 5:14 p.m. UTC | #4

> -----Original Message-----
> From: Tvrtko Ursulin <[e-mail]>
> Sent: Wednesday, February 16, 2022 14:04
> To: Usyskin, Alexander <[e-mail]>; Greg Kroah-
> Hartman <[e-mail]>; Jani Nikula
> <[e-mail]>; Joonas Lahtinen
> <[e-mail]>; Vivi, Rodrigo <[e-mail]>;
> David Airlie <[e-mail]>; Daniel Vetter <[e-mail]>
> Cc: [e-mail]; Winkler, Tomas
> <[e-mail]>; Lubart, Vitaly <[e-mail]>; intel-
> [e-mail]
> Subject: Re: [Intel-gfx] [PATCH v7 1/5] drm/i915/gsc: add gsc as a mei
> auxiliary device
> 
> 
> 
> On 15/02/2022 15:22, Usyskin, Alexander wrote:
> 
> >>> +{
> >>> +	irq_set_chip_and_handler_name(irq, &gsc_irq_chip,
> >>> +				      handle_simple_irq, "gsc_irq_handler");
> >>> +
> >>> +	return irq_set_chip_data(irq, dev_priv);
> >>
> >> I am not familiar with this interrupt scheme - does dev_priv get used at
> >> all by handle_simple_irq, or anyone, after being set here?
> 
> What about this? Is dev_priv required or you could pass in NULL just as
> well?
> 

It is not used, will remove

> >>
> >>> +}
> >>> +
> >>> +struct intel_gsc_def {
> >>> +	const char *name;
> >>> +	const unsigned long bar;
> >>
> >> Unusual, why const out of curiosity? And is it "bar" or "base" would be
> >> more accurate?
> >>
> > Some leftover, thanks for spotting this!
> > It is a base of bar. I prefer bar name here. But not really matter.
> 
> Is it?
> 
> +	adev->bar.start = def->bar + pdev->resource[0].start;
> 
> Looks like offset on top of BAR, no?
> 

Offset on top of DG bar; but start of HECI1/2 bar too.

> >>> +{
> >>> +	struct pci_dev *pdev = to_pci_dev(dev_priv->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(dev_priv))
> >>> +		return;
> >>
> >> Isn't inf_id == 0 always a bug with this patch, regardless of
> >> HAS_HECI_PXP, since the support is incomplete in this patch? If so I'd
> >> be more comfortable with a plain drm_WARN_ON_ONCE(intf_id == 0).
> >>
> > There will be patches for other cards that have pxp as soon as this is
> reviewed.
> > It is better to have infra prepared for two heads.
> 
> My point is things are half-prepared since you don't have the id 0 in
> the array, regardless of the HAS_HECI_PXP. Yes it can't be true now, but
> if you add a patch which enables it to be true, you have to modify the
> array at the same time or risk a broken patch in the middle.
> 
> I don't see the point of the condition making it sound like there are
> two criteria to enter below, while in fact there is only one in current
> code, and that it that it must not be entered because array is incomplete!
> 

We initialize both cells in gsc->intf array, the first one with defaults (two lines before this line)
for systems without working PXP, like DG1.
The code on GSC level does not know that we don't have PXP and don't want to know.

> >>> +
> >>> +	if (!HAS_HECI_GSC(gt->i915))
> >>> +		return;
> >>
> >> Likewise?
> >>
> >>> +
> >>> +	if (gt->gsc.intf[intf_id].irq <= 0) {
> >>> +		DRM_ERROR_RATELIMITED("error handling GSC irq: irq not
> >> set");
> >>
> >> Like this, but use logging functions which say which device please.
> >>
> > drm_err_ratelimited fits here?
> 
> AFAICT it would be a programming bug and not something that can happen
> at runtime hence drm_warn_on_once sounds correct for both.
> 

Sure, will do

> >>>    }
> >>> @@ -182,6 +185,8 @@ void gen11_gt_irq_reset(struct intel_gt *gt)
> >>>    	/* Disable RCS, BCS, VCS and VECS class engines. */
> >>>    	intel_uncore_write(uncore, GEN11_RENDER_COPY_INTR_ENABLE,
> >> 0);
> >>>    	intel_uncore_write(uncore, GEN11_VCS_VECS_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);
> >>> @@ -195,6 +200,8 @@ void gen11_gt_irq_reset(struct intel_gt *gt)
> >>>    	intel_uncore_write(uncore, GEN11_VECS0_VECS1_INTR_MASK,
> >> 	~0);
> >>>    	if (HAS_ENGINE(gt, VECS2) || HAS_ENGINE(gt, VECS3))
> >>>    		intel_uncore_write(uncore,
> >> GEN12_VECS2_VECS3_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);
> >>> @@ -209,6 +216,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);
> >>
> >> Why enable the one which is not supported by the patch? No harm doing
> it?
> >>
> > No harm and the next patch will be soon, this patch unfortunately is long
> overdue.
> 
> Just feels a bit lazy. You are adding two feature test macros to
> prepare, so why not use them.
> 

I've been told that better to enable them both from the HW perspective,
the real interrupt enable magic happens in GSC FW, not here.
 
> Regards,
> 
> Tvrtko

Tvrtko Ursulin Feb. 17, 2022, 9:26 a.m. UTC | #5

On 16/02/2022 17:14, Usyskin, Alexander wrote:
> 
> 
>> -----Original Message-----
>> From: Tvrtko Ursulin <[e-mail]>
>> Sent: Wednesday, February 16, 2022 14:04
>> To: Usyskin, Alexander <[e-mail]>; Greg Kroah-
>> Hartman <[e-mail]>; Jani Nikula
>> <[e-mail]>; Joonas Lahtinen
>> <[e-mail]>; Vivi, Rodrigo <[e-mail]>;
>> David Airlie <[e-mail]>; Daniel Vetter <[e-mail]>
>> Cc: [e-mail]; Winkler, Tomas
>> <[e-mail]>; Lubart, Vitaly <[e-mail]>; intel-
>> [e-mail]
>> Subject: Re: [Intel-gfx] [PATCH v7 1/5] drm/i915/gsc: add gsc as a mei
>> auxiliary device
>>
>>
>>
>> On 15/02/2022 15:22, Usyskin, Alexander wrote:
>>
>>>>> +{
>>>>> +	irq_set_chip_and_handler_name(irq, &gsc_irq_chip,
>>>>> +				      handle_simple_irq, "gsc_irq_handler");
>>>>> +
>>>>> +	return irq_set_chip_data(irq, dev_priv);
>>>>
>>>> I am not familiar with this interrupt scheme - does dev_priv get used at
>>>> all by handle_simple_irq, or anyone, after being set here?
>>
>> What about this? Is dev_priv required or you could pass in NULL just as
>> well?
>>
> 
> It is not used, will remove
> 
>>>>
>>>>> +}
>>>>> +
>>>>> +struct intel_gsc_def {
>>>>> +	const char *name;
>>>>> +	const unsigned long bar;
>>>>
>>>> Unusual, why const out of curiosity? And is it "bar" or "base" would be
>>>> more accurate?
>>>>
>>> Some leftover, thanks for spotting this!
>>> It is a base of bar. I prefer bar name here. But not really matter.
>>
>> Is it?
>>
>> +	adev->bar.start = def->bar + pdev->resource[0].start;
>>
>> Looks like offset on top of BAR, no?
>>
> 
> Offset on top of DG bar; but start of HECI1/2 bar too.

Ok. :)

>>>>> +{
>>>>> +	struct pci_dev *pdev = to_pci_dev(dev_priv->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(dev_priv))
>>>>> +		return;
>>>>
>>>> Isn't inf_id == 0 always a bug with this patch, regardless of
>>>> HAS_HECI_PXP, since the support is incomplete in this patch? If so I'd
>>>> be more comfortable with a plain drm_WARN_ON_ONCE(intf_id == 0).
>>>>
>>> There will be patches for other cards that have pxp as soon as this is
>> reviewed.
>>> It is better to have infra prepared for two heads.
>>
>> My point is things are half-prepared since you don't have the id 0 in
>> the array, regardless of the HAS_HECI_PXP. Yes it can't be true now, but
>> if you add a patch which enables it to be true, you have to modify the
>> array at the same time or risk a broken patch in the middle.
>>
>> I don't see the point of the condition making it sound like there are
>> two criteria to enter below, while in fact there is only one in current
>> code, and that it that it must not be entered because array is incomplete!
>>
> 
> We initialize both cells in gsc->intf array, the first one with defaults (two lines before this line)
> for systems without working PXP, like DG1.
> The code on GSC level does not know that we don't have PXP and don't want to know.

By defaults you mean "-1" ?

My point is intel_gsc_def_dg1[] does not contain anything valid for 
interface zero. If you change HAS_HECI_PXP to return true, the code 
below does:

   def = &intel_gsc_def_dg1[intf_id];

And points to template data not populated.

So you have to change two in conjuction. Hence safest code for this 
patch would simply be:

   if (intf_id == 0) {
	drm_WARN_ON_ONCE(, "Code not implemented yet!\n");
	return;
    }

When you add entries to intel_gsc_def_dg1[] in a later series/patch, 
then you simply remove the above lines altogether.

> 
>>>>> +
>>>>> +	if (!HAS_HECI_GSC(gt->i915))
>>>>> +		return;
>>>>
>>>> Likewise?
>>>>
>>>>> +
>>>>> +	if (gt->gsc.intf[intf_id].irq <= 0) {
>>>>> +		DRM_ERROR_RATELIMITED("error handling GSC irq: irq not
>>>> set");
>>>>
>>>> Like this, but use logging functions which say which device please.
>>>>
>>> drm_err_ratelimited fits here?
>>
>> AFAICT it would be a programming bug and not something that can happen
>> at runtime hence drm_warn_on_once sounds correct for both.
>>
> 
> Sure, will do
> 
>>>>>     }
>>>>> @@ -182,6 +185,8 @@ void gen11_gt_irq_reset(struct intel_gt *gt)
>>>>>     	/* Disable RCS, BCS, VCS and VECS class engines. */
>>>>>     	intel_uncore_write(uncore, GEN11_RENDER_COPY_INTR_ENABLE,
>>>> 0);
>>>>>     	intel_uncore_write(uncore, GEN11_VCS_VECS_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);
>>>>> @@ -195,6 +200,8 @@ void gen11_gt_irq_reset(struct intel_gt *gt)
>>>>>     	intel_uncore_write(uncore, GEN11_VECS0_VECS1_INTR_MASK,
>>>> 	~0);
>>>>>     	if (HAS_ENGINE(gt, VECS2) || HAS_ENGINE(gt, VECS3))
>>>>>     		intel_uncore_write(uncore,
>>>> GEN12_VECS2_VECS3_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);
>>>>> @@ -209,6 +216,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);
>>>>
>>>> Why enable the one which is not supported by the patch? No harm doing
>> it?
>>>>
>>> No harm and the next patch will be soon, this patch unfortunately is long
>> overdue.
>>
>> Just feels a bit lazy. You are adding two feature test macros to
>> prepare, so why not use them.
>>
> 
> I've been told that better to enable them both from the HW perspective,
> the real interrupt enable magic happens in GSC FW, not here.

Well whatever.. As long as logging of spurious/unexpected interrupts is 
in place I can live with that.

Regards,

Tvrtko

Alexander Usyskin Feb. 17, 2022, 10:12 a.m. UTC | #6

> -----Original Message-----
> From: Tvrtko Ursulin <[e-mail]>
> Sent: Thursday, February 17, 2022 11:26
> To: Usyskin, Alexander <[e-mail]>; Greg Kroah-
> Hartman <[e-mail]>; Jani Nikula
> <[e-mail]>; Joonas Lahtinen
> <[e-mail]>; Vivi, Rodrigo <[e-mail]>;
> David Airlie <[e-mail]>; Daniel Vetter <[e-mail]>
> Cc: [e-mail]; Winkler, Tomas
> <[e-mail]>; Lubart, Vitaly <[e-mail]>; intel-
> [e-mail]
> Subject: Re: [Intel-gfx] [PATCH v7 1/5] drm/i915/gsc: add gsc as a mei
> auxiliary device
> 
> 
> 
> On 16/02/2022 17:14, Usyskin, Alexander wrote:
> >
> >
> >> -----Original Message-----
> >> From: Tvrtko Ursulin <[e-mail]>
> >> Sent: Wednesday, February 16, 2022 14:04
> >> To: Usyskin, Alexander <[e-mail]>; Greg Kroah-
> >> Hartman <[e-mail]>; Jani Nikula
> >> <[e-mail]>; Joonas Lahtinen
> >> <[e-mail]>; Vivi, Rodrigo
> <[e-mail]>;
> >> David Airlie <[e-mail]>; Daniel Vetter <[e-mail]>
> >> Cc: [e-mail]; Winkler, Tomas
> >> <[e-mail]>; Lubart, Vitaly <[e-mail]>;
> intel-
> >> [e-mail]
> >> Subject: Re: [Intel-gfx] [PATCH v7 1/5] drm/i915/gsc: add gsc as a mei
> >> auxiliary device
> >>
> >>
> >>
> >> On 15/02/2022 15:22, Usyskin, Alexander wrote:
> >>
> >>>>> +{
> >>>>> +	irq_set_chip_and_handler_name(irq, &gsc_irq_chip,
> >>>>> +				      handle_simple_irq,
> "gsc_irq_handler");
> >>>>> +
> >>>>> +	return irq_set_chip_data(irq, dev_priv);
> >>>>
> >>>> I am not familiar with this interrupt scheme - does dev_priv get used at
> >>>> all by handle_simple_irq, or anyone, after being set here?
> >>
> >> What about this? Is dev_priv required or you could pass in NULL just as
> >> well?
> >>
> >
> > It is not used, will remove
> >
> >>>>
> >>>>> +}
> >>>>> +
> >>>>> +struct intel_gsc_def {
> >>>>> +	const char *name;
> >>>>> +	const unsigned long bar;
> >>>>
> >>>> Unusual, why const out of curiosity? And is it "bar" or "base" would be
> >>>> more accurate?
> >>>>
> >>> Some leftover, thanks for spotting this!
> >>> It is a base of bar. I prefer bar name here. But not really matter.
> >>
> >> Is it?
> >>
> >> +	adev->bar.start = def->bar + pdev->resource[0].start;
> >>
> >> Looks like offset on top of BAR, no?
> >>
> >
> > Offset on top of DG bar; but start of HECI1/2 bar too.
> 
> Ok. :)
> 
> >>>>> +{
> >>>>> +	struct pci_dev *pdev = to_pci_dev(dev_priv->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(dev_priv))
> >>>>> +		return;
> >>>>
> >>>> Isn't inf_id == 0 always a bug with this patch, regardless of
> >>>> HAS_HECI_PXP, since the support is incomplete in this patch? If so I'd
> >>>> be more comfortable with a plain drm_WARN_ON_ONCE(intf_id == 0).
> >>>>
> >>> There will be patches for other cards that have pxp as soon as this is
> >> reviewed.
> >>> It is better to have infra prepared for two heads.
> >>
> >> My point is things are half-prepared since you don't have the id 0 in
> >> the array, regardless of the HAS_HECI_PXP. Yes it can't be true now, but
> >> if you add a patch which enables it to be true, you have to modify the
> >> array at the same time or risk a broken patch in the middle.
> >>
> >> I don't see the point of the condition making it sound like there are
> >> two criteria to enter below, while in fact there is only one in current
> >> code, and that it that it must not be entered because array is incomplete!
> >>
> >
> > We initialize both cells in gsc->intf array, the first one with defaults (two
> lines before this line)
> > for systems without working PXP, like DG1.
> > The code on GSC level does not know that we don't have PXP and don't
> want to know.
> 
> By defaults you mean "-1" ?
> 
> My point is intel_gsc_def_dg1[] does not contain anything valid for
> interface zero. If you change HAS_HECI_PXP to return true, the code
> below does:
> 
>    def = &intel_gsc_def_dg1[intf_id];
> 
> And points to template data not populated.
> 
> So you have to change two in conjuction. Hence safest code for this
> patch would simply be:
> 
>    if (intf_id == 0) {
> 	drm_WARN_ON_ONCE(, "Code not implemented yet!\n");
> 	return;
>     }
> 
> When you add entries to intel_gsc_def_dg1[] in a later series/patch,
> then you simply remove the above lines altogether.
> 

Maybe better to add check after def = &intel_gsc_def_dg1[intf_id]; for name pointer
to catch mismatch between supported bits and filled structures in definition array, like:

    if (def->name == NULL) {
 	drm_WARN_ON_ONCE(, "HECI%d is not implemented!\n", intf_id + 1);
 	return;
     }

This way we can leave this line as we'll have more platforms without HECI1

> >>>>> +
> >>>>> +	if (!HAS_HECI_GSC(gt->i915))
> >>>>> +		return;
> >>>>
> >>>> Likewise?
> >>>>
> >>>>> +
> >>>>> +	if (gt->gsc.intf[intf_id].irq <= 0) {
> >>>>> +		DRM_ERROR_RATELIMITED("error handling GSC irq:
> irq not
> >>>> set");
> >>>>
> >>>> Like this, but use logging functions which say which device please.
> >>>>
> >>> drm_err_ratelimited fits here?
> >>
> >> AFAICT it would be a programming bug and not something that can
> happen
> >> at runtime hence drm_warn_on_once sounds correct for both.
> >>
> >
> > Sure, will do
> >
> >>>>>     }
> >>>>> @@ -182,6 +185,8 @@ void gen11_gt_irq_reset(struct intel_gt *gt)
> >>>>>     	/* Disable RCS, BCS, VCS and VECS class engines. */
> >>>>>     	intel_uncore_write(uncore,
> GEN11_RENDER_COPY_INTR_ENABLE,
> >>>> 0);
> >>>>>     	intel_uncore_write(uncore,
> GEN11_VCS_VECS_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);
> >>>>> @@ -195,6 +200,8 @@ void gen11_gt_irq_reset(struct intel_gt *gt)
> >>>>>     	intel_uncore_write(uncore,
> GEN11_VECS0_VECS1_INTR_MASK,
> >>>> 	~0);
> >>>>>     	if (HAS_ENGINE(gt, VECS2) || HAS_ENGINE(gt, VECS3))
> >>>>>     		intel_uncore_write(uncore,
> >>>> GEN12_VECS2_VECS3_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);
> >>>>> @@ -209,6 +216,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);
> >>>>
> >>>> Why enable the one which is not supported by the patch? No harm
> doing
> >> it?
> >>>>
> >>> No harm and the next patch will be soon, this patch unfortunately is long
> >> overdue.
> >>
> >> Just feels a bit lazy. You are adding two feature test macros to
> >> prepare, so why not use them.
> >>
> >
> > I've been told that better to enable them both from the HW perspective,
> > the real interrupt enable magic happens in GSC FW, not here.
> 
> Well whatever.. As long as logging of spurious/unexpected interrupts is
> in place I can live with that.
> 
> Regards,
> 
> Tvrtko

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index a0c2ae3d71d8..c92d34f8522a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9797,6 +9797,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 2ac220bfd0ed..3b0508e7a805 100644
--- a/drivers/gpu/drm/i915/Kconfig
+++ b/drivers/gpu/drm/i915/Kconfig
@@ -29,6 +29,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 226dbef5f64a..e40ba71b3cdd 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -195,6 +195,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..d07f182f64e4
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/intel_gsc.c
@@ -0,0 +1,196 @@ 
+// 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"
+
+#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(struct drm_i915_private *dev_priv, int irq)
+{
+	irq_set_chip_and_handler_name(irq, &gsc_irq_chip,
+				      handle_simple_irq, "gsc_irq_handler");
+
+	return irq_set_chip_data(irq, dev_priv);
+}
+
+struct intel_gsc_def {
+	const char *name;
+	const unsigned long bar;
+	size_t bar_size;
+};
+
+/* gscfi (graphics system controller firmware interface) resources */
+static const struct intel_gsc_def intel_gsc_def_dg1[] = {
+	{
+	},
+	{
+		.name = "mei-gscfi",
+		.bar = GSC_DG1_HECI2_BASE,
+		.bar_size = GSC_BAR_LENGTH,
+	}
+};
+
+static void intel_gsc_release_dev(struct device *dev)
+{
+	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 *dev_priv,
+			       struct intel_gsc_intf *intf,
+			       unsigned int intf_id)
+{
+	struct pci_dev *pdev = to_pci_dev(dev_priv->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(dev_priv))
+		return;
+
+	def = &intel_gsc_def_dg1[intf_id];
+
+	dev_dbg(&pdev->dev, "init gsc one with id %d\n", intf_id);
+	intf->irq = irq_alloc_desc(0);
+	if (intf->irq < 0) {
+		dev_err(&pdev->dev, "gsc irq error %d\n", intf->irq);
+		return;
+	}
+
+	ret = gsc_irq_init(dev_priv, intf->irq);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "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) {
+		dev_err(&pdev->dev, "gsc aux init failed %d\n", ret);
+		kfree(adev);
+		goto fail;
+	}
+
+	ret = auxiliary_device_add(aux_dev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "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;
+
+	dev_dbg(&pdev->dev, "gsc init one done\n");
+	return;
+fail:
+	intel_gsc_destroy_one(intf);
+}
+
+static void intel_gsc_irq_handler(struct intel_gt *gt, unsigned int intf_id)
+{
+	int ret;
+
+	if (intf_id >= INTEL_GSC_NUM_INTERFACES)
+		return;
+
+	if (!HAS_HECI_GSC(gt->i915))
+		return;
+
+	if (gt->gsc.intf[intf_id].irq <= 0) {
+		DRM_ERROR_RATELIMITED("error handling GSC irq: irq not set");
+		return;
+	}
+
+	ret = generic_handle_irq(gt->gsc.intf[intf_id].irq);
+	if (ret)
+		DRM_ERROR_RATELIMITED("error handling GSC irq: %d\n", ret);
+}
+
+void gsc_irq_handler(struct intel_gt *gt, u32 iir)
+{
+	if (iir & GSC_IRQ_INTF(0))
+		intel_gsc_irq_handler(gt, 0);
+	if (iir & GSC_IRQ_INTF(1))
+		intel_gsc_irq_handler(gt, 1);
+}
+
+void intel_gsc_init(struct intel_gsc *gsc, struct drm_i915_private *dev_priv)
+{
+	unsigned int i;
+
+	if (!HAS_HECI_GSC(dev_priv))
+		return;
+
+	for (i = 0; i < INTEL_GSC_NUM_INTERFACES; i++)
+		intel_gsc_init_one(dev_priv, &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..67cb2a75f839
--- /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 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 e8403fa53909..18961d154956 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -446,6 +446,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 2dad46c3eff2..6ba817c02baa 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 983264e10e0a..d36f919bbdf5 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 gsc_irq_handler(gt, iir);
+
 	WARN_ONCE(1, "unhandled other interrupt instance=0x%x, iir=0x%x\n",
 		  instance, iir);
 }
@@ -182,6 +185,8 @@  void gen11_gt_irq_reset(struct intel_gt *gt)
 	/* Disable RCS, BCS, VCS and VECS class engines. */
 	intel_uncore_write(uncore, GEN11_RENDER_COPY_INTR_ENABLE, 0);
 	intel_uncore_write(uncore, GEN11_VCS_VECS_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);
@@ -195,6 +200,8 @@  void gen11_gt_irq_reset(struct intel_gt *gt)
 	intel_uncore_write(uncore, GEN11_VECS0_VECS1_INTR_MASK,	~0);
 	if (HAS_ENGINE(gt, VECS2) || HAS_ENGINE(gt, VECS3))
 		intel_uncore_write(uncore, GEN12_VECS2_VECS3_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);
@@ -209,6 +216,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;
 
@@ -225,6 +233,9 @@  void gen11_gt_irq_postinstall(struct intel_gt *gt)
 	/* Enable RCS, BCS, VCS and VECS class interrupts. */
 	intel_uncore_write(uncore, GEN11_RENDER_COPY_INTR_ENABLE, dmask);
 	intel_uncore_write(uncore, GEN11_VCS_VECS_INTR_ENABLE, dmask);
+	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);
@@ -238,6 +249,8 @@  void gen11_gt_irq_postinstall(struct intel_gt *gt)
 	intel_uncore_write(uncore, GEN11_VECS0_VECS1_INTR_MASK, ~dmask);
 	if (HAS_ENGINE(gt, VECS2) || HAS_ENGINE(gt, VECS3))
 		intel_uncore_write(uncore, GEN12_VECS2_VECS3_INTR_MASK, ~dmask);
+	if (HAS_HECI_GSC(gt->i915))
+		intel_uncore_write(uncore, GEN11_GUNIT_CSME_INTR_MASK, 0);
 	/*
 	 * RPS interrupts will get enabled/disabled on demand when RPS itself
 	 * is enabled/disabled.
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
index a6f0220c2e9f..427f91900afc 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
@@ -944,6 +944,7 @@  enum {
 #define OTHER_GUC_INSTANCE	0
 #define OTHER_GTPM_INSTANCE	1
 #define OTHER_KCR_INSTANCE	4
+#define OTHER_GSC_INSTANCE	6
 
 #define GEN11_INTR_IDENTITY_REG(x)	_MMIO(0x190060 + ((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 4f057a45654a..b41871abe776 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1444,6 +1444,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 467252f885c2..8f608b03d30b 100644
--- a/drivers/gpu/drm/i915/i915_pci.c
+++ b/drivers/gpu/drm/i915/i915_pci.c
@@ -900,7 +900,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 ca6adfc234c0..e74934255198 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -974,6 +974,8 @@ 
 #define GEN11_VEBOX2_RING_BASE		0x1d8000
 #define XEHP_VEBOX3_RING_BASE		0x1e8000
 #define XEHP_VEBOX4_RING_BASE		0x1f8000
+#define GSC_DG1_HECI1_BASE	0x00258000
+#define GSC_DG1_HECI2_BASE	0x00259000
 #define BLT_RING_BASE		0x22000
 
 
diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
index 27dcfe6f2429..fa75f464cb01 100644
--- a/drivers/gpu/drm/i915/intel_device_info.h
+++ b/drivers/gpu/drm/i915/intel_device_info.h
@@ -135,6 +135,8 @@  enum intel_ppgtt_type {
 	func(has_reset_engine); \
 	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 */