Messages in this thread | | | From | "Usyskin, Alexander" <> | Subject | RE: [Intel-xe] [PATCH v2 3/4] drm/xe/gsc: add gsc device support | Date | Tue, 19 Sep 2023 14:51:27 +0000 |
| |
> -----Original Message----- > From: Vivi, Rodrigo <rodrigo.vivi@intel.com> > Sent: Monday, September 18, 2023 19:25 > To: Usyskin, Alexander <alexander.usyskin@intel.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; De Marchi, Lucas > <lucas.demarchi@intel.com>; Ceraolo Spurio, Daniele > <daniele.ceraolospurio@intel.com>; linux-kernel@vger.kernel.org; Winkler, > Tomas <tomas.winkler@intel.com>; intel-xe@lists.freedesktop.org > Subject: Re: [Intel-xe] [PATCH v2 3/4] drm/xe/gsc: add gsc device support > > On Thu, Sep 14, 2023 at 11:01:37AM +0300, Alexander Usyskin wrote: > > From: Vitaly Lubart <vitaly.lubart@intel.com> > > > > Create mei-gscfi auxiliary device and configure interrupts > > to be consumed by mei-gsc device driver. > > > > Signed-off-by: Vitaly Lubart <vitaly.lubart@intel.com> > > Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com> > > --- > > drivers/gpu/drm/xe/Kconfig | 1 + > > drivers/gpu/drm/xe/Makefile | 1 + > > drivers/gpu/drm/xe/xe_device.c | 4 + > > drivers/gpu/drm/xe/xe_device_types.h | 4 + > > drivers/gpu/drm/xe/xe_heci_gsc.c | 205 > +++++++++++++++++++++++++++ > > drivers/gpu/drm/xe/xe_heci_gsc.h | 35 +++++ > > drivers/gpu/drm/xe/xe_irq.c | 14 +- > > 7 files changed, 262 insertions(+), 2 deletions(-) > > create mode 100644 drivers/gpu/drm/xe/xe_heci_gsc.c > > create mode 100644 drivers/gpu/drm/xe/xe_heci_gsc.h > > > > diff --git a/drivers/gpu/drm/xe/Kconfig b/drivers/gpu/drm/xe/Kconfig > > index 096bd066afa8..da82084fe236 100644 > > --- a/drivers/gpu/drm/xe/Kconfig > > +++ b/drivers/gpu/drm/xe/Kconfig > > @@ -37,6 +37,7 @@ config DRM_XE > > select DRM_SCHED > > select MMU_NOTIFIER > > select WANT_DEV_COREDUMP > > + select AUXILIARY_BUS > > help > > Experimental driver for Intel Xe series GPUs > > > > diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile > > index 9d2311f8141f..fbdb28fa5ace 100644 > > --- a/drivers/gpu/drm/xe/Makefile > > +++ b/drivers/gpu/drm/xe/Makefile > > @@ -73,6 +73,7 @@ xe-y += xe_bb.o \ > > xe_guc_log.o \ > > xe_guc_pc.o \ > > xe_guc_submit.o \ > > + xe_heci_gsc.o \ > > xe_hw_engine.o \ > > xe_hw_engine_class_sysfs.o \ > > xe_hw_fence.o \ > > diff --git a/drivers/gpu/drm/xe/xe_device.c > b/drivers/gpu/drm/xe/xe_device.c > > index d6fc06d4c9dc..4d6e2f2b281f 100644 > > --- a/drivers/gpu/drm/xe/xe_device.c > > +++ b/drivers/gpu/drm/xe/xe_device.c > > @@ -323,6 +323,8 @@ int xe_device_probe(struct xe_device *xe) > > goto err_irq_shutdown; > > } > > > > + xe_heci_gsc_init(xe); > > + > > could we place this call earlier in the flow? maybe right after setting up mmio? > or maybe after pcode init where we confirmed the boot can proceed on > discrete?
Right after xe_irq_install will be ok (as we need working irq to setup)? Will try, if this works.
> > > err = xe_display_init(xe); > > if (err) > > goto err_irq_shutdown; > > @@ -365,6 +367,8 @@ void xe_device_remove(struct xe_device *xe) > > > > xe_display_fini(xe); > > > > + xe_heci_gsc_fini(xe); > > + > > xe_irq_shutdown(xe); > > } > > > > diff --git a/drivers/gpu/drm/xe/xe_device_types.h > b/drivers/gpu/drm/xe/xe_device_types.h > > index 1d1fe53fc30d..80233c2f0d81 100644 > > --- a/drivers/gpu/drm/xe/xe_device_types.h > > +++ b/drivers/gpu/drm/xe/xe_device_types.h > > @@ -13,6 +13,7 @@ > > #include <drm/ttm/ttm_device.h> > > > > #include "xe_devcoredump_types.h" > > +#include "xe_heci_gsc.h" > > #include "xe_gt_types.h" > > #include "xe_platform_types.h" > > #include "xe_step_types.h" > > @@ -364,6 +365,9 @@ struct xe_device { > > */ > > struct task_struct *pm_callback_task; > > > > + /** @gsc: graphics security controller */ > > + struct xe_heci_gsc heci_gsc; > > documentation doesn't match variable name.
Will fix in V2
> > > + > > /* private: */ > > > > #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY) > > diff --git a/drivers/gpu/drm/xe/xe_heci_gsc.c > b/drivers/gpu/drm/xe/xe_heci_gsc.c > > new file mode 100644 > > index 000000000000..1eca1c46f257 > > --- /dev/null > > +++ b/drivers/gpu/drm/xe/xe_heci_gsc.c > > @@ -0,0 +1,205 @@ > > +// SPDX-License-Identifier: MIT > > +/* > > + * Copyright(c) 2023, Intel Corporation. All rights reserved. > > + */ > > + > > +#include <linux/irq.h> > > +#include <linux/mei_aux.h> > > +#include <linux/pci.h> > > +#include <linux/sizes.h> > > + > > +#include "regs/xe_regs.h" > > +#include "xe_device_types.h" > > +#include "xe_drv.h" > > +#include "xe_heci_gsc.h" > > +#include "xe_platform_types.h" > > + > > +#define GSC_BAR_LENGTH 0x00000FFC > > + > > +static void heci_gsc_irq_mask(struct irq_data *d) > > +{ > > + /* generic irq handling */ > > +} > > + > > +static void heci_gsc_irq_unmask(struct irq_data *d) > > +{ > > + /* generic irq handling */ > > +} > > + > > +static struct irq_chip heci_gsc_irq_chip = { > > + .name = "gsc_irq_chip", > > + .irq_mask = heci_gsc_irq_mask, > > + .irq_unmask = heci_gsc_irq_unmask, > > +}; > > + > > +static int heci_gsc_irq_init(int irq) > > +{ > > + irq_set_chip_and_handler_name(irq, &heci_gsc_irq_chip, > > + handle_simple_irq, > "heci_gsc_irq_handler"); > > + > > + return irq_set_chip_data(irq, NULL); > > +} > > + > > +/** > > + * struct heci_gsc_def - graphics security controller heci interface definitions > > + * > > + * @name: name of the heci device > > + * @bar: address of the mmio bar > > + * @bar_size: size of the mmio bar > > + * @use_polling: indication of using polling mode for the device > > + * @slow_firmware: indication of whether the device is slow (needs longer > timeouts) > > + */ > > +struct heci_gsc_def { > > + const char *name; > > + unsigned long bar; > > + size_t bar_size; > > + bool use_polling; > > + bool slow_firmware; > > will we need the lmem_size here? > PXP is not planned now, so will not need.
> or what's the difference on the mei-gsc and mei-gscfi exactly? >
GSC sports two HECI heads - one for PXP (we call created device mei-gsc), another for chassis (mei-gscfi) Here only second head is supported, as PXP is not planned now.
> > +}; > > + > > +/* gsc resources and definitions */ > > +static const struct heci_gsc_def heci_gsc_def_dg1 = { > > + .name = "mei-gscfi", > > + .bar = DG1_GSC_HECI2_BASE, > > + .bar_size = GSC_BAR_LENGTH, > > +}; > > + > > +static const struct heci_gsc_def heci_gsc_def_dg2 = { > > + .name = "mei-gscfi", > > + .bar = DG2_GSC_HECI2_BASE, > > + .bar_size = GSC_BAR_LENGTH, > > +}; > > + > > +static void heci_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); > > +} > > + > > +void xe_heci_gsc_fini(struct xe_device *xe) > > +{ > > + struct xe_heci_gsc *heci_gsc = &xe->heci_gsc; > > + > > + if (!HAS_HECI_GSCFI(xe)) > > + return; > > + > > + if (heci_gsc->adev) { > > + struct auxiliary_device *aux_dev = &heci_gsc->adev->aux_dev; > > + > > + auxiliary_device_delete(aux_dev); > > + auxiliary_device_uninit(aux_dev); > > + heci_gsc->adev = NULL; > > + } > > + > > + if (heci_gsc->irq >= 0) > > + irq_free_desc(heci_gsc->irq); > > + heci_gsc->irq = -1; > > +} > > + > > +void xe_heci_gsc_init(struct xe_device *xe) > > +{ > > + struct xe_heci_gsc *heci_gsc = &xe->heci_gsc; > > + struct pci_dev *pdev = to_pci_dev(xe->drm.dev); > > + struct mei_aux_device *adev; > > + struct auxiliary_device *aux_dev; > > + const struct heci_gsc_def *def; > > + int ret; > > + > > + if (!HAS_HECI_GSCFI(xe)) > > + return; > > + > > + heci_gsc->irq = -1; > > + > > + if (xe->info.platform == XE_DG1) { > > + def = &heci_gsc_def_dg1; > > + } else if (xe->info.platform == XE_DG2) { > > + def = &heci_gsc_def_dg2; > > in general it is better to add the most recent on top of the oldest.
Ok, but this is exact copy from i915
> > > + } else { > > + drm_warn_once(&xe->drm, "Unknown platform\n"); > > + return; > > + } > > + > > + if (!def->name) { > > + drm_warn_once(&xe->drm, "HECI is not implemented!\n"); > > + return; > > + } > > + > > + /* skip irq initialization */ > > + if (def->use_polling) > > + goto add_device; > > what about creating a function > heci_gsc_irq_setup(...) > > and then > if (!def->use_polling) { > ret = heci_gsc_irq_setup(...) > if (ret) > goto fail; > Will do, thx.
> > + > > + heci_gsc->irq = irq_alloc_desc(0); > > + if (heci_gsc->irq < 0) { > > + drm_err(&xe->drm, "gsc irq error %d\n", heci_gsc->irq); > > + goto fail; > > + } > > + > > + ret = heci_gsc_irq_init(heci_gsc->irq); > > + if (ret < 0) { > > + drm_err(&xe->drm, "gsc irq init failed %d\n", ret); > > + goto fail; > > + } > > + > > +add_device: > > it looks like this add_device always deserve a dedicated function. > Sure, will create one
> > + adev = kzalloc(sizeof(*adev), GFP_KERNEL); > > + if (!adev) > > + goto fail; > > + adev->irq = heci_gsc->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; > > + adev->slow_firmware = def->slow_firmware; > > + > > + 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 = heci_gsc_release_dev; > > + > > + ret = auxiliary_device_init(aux_dev); > > + if (ret < 0) { > > + drm_err(&xe->drm, "gsc aux init failed %d\n", ret); > > + kfree(adev); > > + goto fail; > > + } > > + > > + heci_gsc->adev = adev; /* needed by the notifier */ > > + ret = auxiliary_device_add(aux_dev); > > + if (ret < 0) { > > + drm_err(&xe->drm, "gsc aux add failed %d\n", ret); > > + heci_gsc->adev = NULL; > > + > > + /* adev will be freed with the put_device() and .release > sequence */ > > + auxiliary_device_uninit(aux_dev); > > + goto fail; > > + } > > + > > + return; > > +fail: > > + xe_heci_gsc_fini(xe); > > +} > > + > > +void xe_heci_gsc_irq_handler(struct xe_device *xe, u32 iir) > > +{ > > + int ret; > > + > > + if ((iir & GSC_IRQ_INTF(1)) == 0) > > + return; > > + > > + if (!HAS_HECI_GSCFI(xe)) { > > + drm_warn_once(&xe->drm, "GSC irq: not supported"); > > + return; > > + } > > + > > + if (xe->heci_gsc.irq < 0) > > + return; > > + > > + ret = generic_handle_irq(xe->heci_gsc.irq); > > + if (ret) > > + drm_err_ratelimited(&xe->drm, "error handling GSC irq: > %d\n", ret); > > +} > > diff --git a/drivers/gpu/drm/xe/xe_heci_gsc.h > b/drivers/gpu/drm/xe/xe_heci_gsc.h > > new file mode 100644 > > index 000000000000..9db454478fae > > --- /dev/null > > +++ b/drivers/gpu/drm/xe/xe_heci_gsc.h > > @@ -0,0 +1,35 @@ > > +/* SPDX-License-Identifier: MIT */ > > +/* > > + * Copyright(c) 2023, Intel Corporation. All rights reserved. > > + */ > > +#ifndef __XE_HECI_GSC_DEV_H__ > > +#define __XE_HECI_GSC_DEV_H__ > > + > > +#include <linux/types.h> > > + > > +struct xe_device; > > +struct mei_aux_device; > > + > > +/* > > + * 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 xe_heci_gsc - graphics security controller for xe, HECI interface > > + * > > + * @adev : pointer to mei auxiliary device structure > > + * @irq : irq number > > + * > > + */ > > +struct xe_heci_gsc { > > + struct mei_aux_device *adev; > > + int irq; > > +}; > > + > > +void xe_heci_gsc_init(struct xe_device *xe); > > +void xe_heci_gsc_fini(struct xe_device *xe); > > +void xe_heci_gsc_irq_handler(struct xe_device *xe, u32 iir); > > + > > +#endif /* __XE_HECI_GSC_DEV_H__ */ > > diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c > > index 1dee3e832eb5..d297e9b8a3be 100644 > > --- a/drivers/gpu/drm/xe/xe_irq.c > > +++ b/drivers/gpu/drm/xe/xe_irq.c > > @@ -128,6 +128,7 @@ void xe_irq_enable_hwe(struct xe_gt *gt) > > struct xe_device *xe = gt_to_xe(gt); > > u32 ccs_mask, bcs_mask; > > u32 irqs, dmask, smask; > > + u32 gsc_mask = GSC_IRQ_INTF(1); > > > > if (xe_device_guc_submission_enabled(xe)) { > > irqs = GT_RENDER_USER_INTERRUPT | > > @@ -180,6 +181,9 @@ void xe_irq_enable_hwe(struct xe_gt *gt) > > if (xe_hw_engine_mask_per_class(gt, > XE_ENGINE_CLASS_OTHER)) { > > xe_mmio_write32(gt, GUNIT_GSC_INTR_ENABLE, > irqs); > > xe_mmio_write32(gt, GUNIT_GSC_INTR_MASK, > ~irqs); > > + } else if (HAS_HECI_GSCFI(xe)) { > > + xe_mmio_write32(gt, GUNIT_GSC_INTR_ENABLE, > gsc_mask); > > + xe_mmio_write32(gt, GUNIT_GSC_INTR_MASK, > ~gsc_mask); > > is there any way we could combine this with the upper calls to the same > register? > I believe i915 is combining them for instance... >
Ok, will do, small gain, but it will be more readable.
> > } > > } > > } > > @@ -284,6 +288,11 @@ static void gt_irq_handler(struct xe_tile *tile, > > instance = INTR_ENGINE_INSTANCE(identity[bit]); > > intr_vec = INTR_ENGINE_INTR(identity[bit]); > > > > + if (class == XE_ENGINE_CLASS_OTHER && instance == > OTHER_GSC_INSTANCE) { > > + xe_heci_gsc_irq_handler(xe, intr_vec); > > + continue; > > + } > > + > > engine_gt = pick_engine_gt(tile, class, instance); > > > > hwe = xe_gt_hw_engine(engine_gt, class, instance, > false); > > @@ -470,8 +479,9 @@ static void gt_irq_reset(struct xe_tile *tile) > > if (ccs_mask & (BIT(2)|BIT(3))) > > xe_mmio_write32(mmio, CCS2_CCS3_INTR_MASK, ~0); > > > > - if (tile->media_gt && > > - xe_hw_engine_mask_per_class(tile->media_gt, > XE_ENGINE_CLASS_OTHER)) { > > + if ((tile->media_gt && > > + xe_hw_engine_mask_per_class(tile->media_gt, > XE_ENGINE_CLASS_OTHER)) || > > + HAS_HECI_GSCFI(tile_to_xe(tile))) { > > xe_mmio_write32(mmio, GUNIT_GSC_INTR_ENABLE, 0); > > xe_mmio_write32(mmio, GUNIT_GSC_INTR_MASK, ~0); > > } > > -- > > 2.34.1 > >
-- Thanks, Sasha
| |