[Intel-xe] [PATCH v3 2/3] drm/xe: Introduce Intel Accelerated Fabric device

Ruhl, Michael J michael.j.ruhl at intel.com
Thu Jun 29 20:45:45 UTC 2023


>-----Original Message-----
>From: Ofir Bitton <obitton at habana.ai>
>Sent: Thursday, June 29, 2023 1:37 PM
>To: Ruhl, Michael J <michael.j.ruhl at intel.com>; Kershner, David
><david.kershner at intel.com>; intel-xe at lists.freedesktop.org; Fleck, John
><john.fleck at intel.com>
>Subject: Re: [Intel-xe] [PATCH v3 2/3] drm/xe: Introduce Intel Accelerated
>Fabric device
>
>On 28/06/2023 22:19, Ruhl, Michael J wrote:
>>> -----Original Message-----
>>> From: Ofir Bitton <obitton at habana.ai>
>>> Sent: Wednesday, June 28, 2023 11:14 AM
>>> To: Kershner, David <david.kershner at intel.com>; intel-
>>> xe at lists.freedesktop.org; Ruhl, Michael J <michael.j.ruhl at intel.com>;
>Fleck,
>>> John <john.fleck at intel.com>
>>> Subject: Re: [Intel-xe] [PATCH v3 2/3] drm/xe: Introduce Intel Accelerated
>>> Fabric device
>>>
>>> Hey,
>>> First of all I would like to give some basic assumptions regarding
>>> wokring with external driver using aux-bus, afterwards I think my
>>> comments will make sense.
>>>
>>> IAF driver communicats with XE driver using aux-bus which is
>>> configurable and flexible, hence we should leave minimal footprint in XE
>>> driver and move all private implementations to the IAF driver itself.
>>
>> Yup we agree.  However, there is some set up that the parent device needs
>> to do.  This patch is doing that minimal setup on the XE device side.
>>
>>> In order to fulfil this you will need to expose mmio_read/write and
>>> irq_handling functions throught the aux-bus, this will allow us to
>>> remove all IAF mmio/irq dependent code from XE.
>>
>> This is exposed via the sharing of the memory and IRQ resources, and then
>> mapped using ioremap()/irq_request() in the IAF driver.  Do you think that
>> an explicit _read/_write callback should be exposed (similar to the
>.dev_event(),
>>   .register_dev and .unregister_dev callbacks?)
>
>Exactly, we do it also in the habanalabs driver.

Ok.  That might be problematic, we have some internal access to the register space
that is not directly register read/writes...

Being able to map the space from the IAF driver and accessing it directly might not
allow us to do this.

(for instance FW loading is to a 4K section and done directly with a memcpy...)

Having the read/write callback will allow you to control stuff from the parent? 
(power control issues?   which is not an issue for us in this case...)

Is there other reasons why moving this to a callback method is needed?

>>
>> The IRQ path had some race condition issues which is why the IRQ descriptor
>> path was chosen.
>
>I understand, but I still think a significant part of it can move to IAF
>driver.
>
>>
>>> Furthermore, it would be more elegant to use generic names instead of
>>> mentioning IAF wherever possible. This is much needed in order to avoid
>>> painful refactors whenever a different fabric is introduced.
>>
>> We can rename, but note that "IAF" is "Intel Accelerated Fabric".  It was
>thought
>> that was a good "generic" name.  Should we just call it "fabric"?
>
>I don't realy mind as long it does not refer to a specific HW.

Yeah, we named it this to avoid specific HW.  The naming of the driver IAF was for
convenience, expecting that there would be some further direction on naming at
some point, but was never given any direction on it...

So the IAF from the Xe driver perspective is "generic"...

>>
>>
>>> On 23/06/2023 7:12, David Kershner wrote:
>>>> Add accelerator fabric driver support for PVC devices.
>>>>
>>>> Introduce the initial platform data structure that will be shared
>>>> between the Xe driver and the IAF driver.
>>
>> We will update this to "Intel Accelerated Fabric" (IAF)
>>
>>>> If the device is PVC, and there is an accelerator fabric present,
>>>> add and initialize an Auxbus device with the appropriate resources.
>>>>
>>>> Add the accelerator fabric information register.
>>>>
>>>> Add package address register for defining the Device Physical Address
>>>> space (DPA).
>>>>
>>>> Add initial interrupt support.
>>>>
>>>> The package address register defines the device physical
>>>> address (DPA) space for the given device.
>>>>
>>>> Using the "unique" xa_array index, generate a DPA address
>>>> for the device and program the package register appropriately.
>>>>
>>>> Update the LMEM offset to use the DPA address.
>>>>
>>>> Signed-off-by: David Kershner <david.kershner at intel.com>
>>>> ---
>>>>    drivers/gpu/drm/xe/Kconfig           |   1 +
>>>>    drivers/gpu/drm/xe/regs/xe_gt_regs.h |  20 ++
>>>>    drivers/gpu/drm/xe/xe_device.c       |  13 +-
>>>>    drivers/gpu/drm/xe/xe_device_types.h |  25 ++
>>>>    drivers/gpu/drm/xe/xe_gt_types.h     |   2 +
>>>>    drivers/gpu/drm/xe/xe_iaf.c          | 461
>++++++++++++++++++++++++++-
>>>>    drivers/gpu/drm/xe/xe_iaf.h          |  39 +++
>>>>    drivers/gpu/drm/xe/xe_irq.c          |  28 +-
>>>>    drivers/gpu/drm/xe/xe_mmio.c         |   2 +-
>>>>    drivers/gpu/drm/xe/xe_pci.c          |   2 +
>>>>    drivers/gpu/drm/xe/xe_pci_types.h    |   1 +
>>>>    include/drm/intel_iaf_platform.h     | 143 +++++++++
>>>>    12 files changed, 733 insertions(+), 4 deletions(-)
>>>>    create mode 100644 drivers/gpu/drm/xe/xe_iaf.h
>>>>    create mode 100644 include/drm/intel_iaf_platform.h
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/Kconfig b/drivers/gpu/drm/xe/Kconfig
>>>> index d44794f99338..e4be3749778a 100644
>>>> --- a/drivers/gpu/drm/xe/Kconfig
>>>> +++ b/drivers/gpu/drm/xe/Kconfig
>>>> @@ -36,6 +36,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/regs/xe_gt_regs.h
>>> b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
>>>> index 3f664011eaea..d9f68065ca70 100644
>>>> --- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
>>>> +++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
>>>> @@ -76,6 +76,11 @@
>>>>    #define GLOBAL_MOCS(i)
>	XE_REG(0x4000 + (i) *
>>> 4) /* Global MOCS regs */
>>>>    #define GFX_CCS_AUX_NV
>	XE_REG(0x4208)
>>>>
>>>> +#define PKG_ADDR_RANGE
>>> 	XE_REG(0x41B0)
>>>> +#define   PKG_ADDR_RANGE_RANGE_SHIFT		20
>>>> +#define   PKG_ADDR_RANGE_BASE_SHIFT		1
>>>> +#define   PKG_ADDR_RANGE_ENABLE			1
>>>> +
>>>>    #define VD0_AUX_NV				XE_REG(0x4218)
>>>>    #define VE0_AUX_NV				XE_REG(0x4238)
>>>>
>>>> @@ -83,6 +88,11 @@
>>>>    #define   AUX_INV				REG_BIT(0)
>>>>
>>>>    #define XEHP_TILE_ADDR_RANGE(_idx)		XE_REG_MCR(0x4900
>>> + (_idx) * 4)
>>>> +#define   XEHP_TILE_LMEM_RANGE_SHIFT            8
>>>> +#define   XEHP_TILE_LMEM_BASE_SHIFT             1
>>>> +#define   XEHP_TILE_LMEM_BASE_MASK              REG_GENMASK(7, 1)
>>>> +#define   XEHP_TILE_LMEM_RANGE_MASK             REG_GENMASK(14, 8)
>>>> +
>>>>    #define XEHP_FLAT_CCS_BASE_ADDR
>>> 	XE_REG_MCR(0x4910)
>>>>
>>>>    #define CHICKEN_RASTER_1			XE_REG_MCR(0x6204,
>>> XE_REG_OPTION_MASKED)
>>>> @@ -344,6 +354,11 @@
>>>>    #define RCU_MODE				XE_REG(0x14800,
>>> XE_REG_OPTION_MASKED)
>>>>    #define   RCU_MODE_CCS_ENABLE			REG_BIT(0)
>>>>
>>>> +#define PKG_ADDR_BASE
>	XE_REG(0x108390)
>>>> +#define   PKG_ADDR_BASE_RANGE_SHIFT		20
>>>> +#define   PKG_ADDR_BASE_BASE_SHIFT		1
>>>> +#define   PKG_ADDR_BASE_ENABLE			1
>>>> +
>>>>    #define FORCEWAKE_ACK_GT			XE_REG(0x130044)
>>>>    #define   FORCEWAKE_KERNEL			BIT(0)
>>>>    #define   FORCEWAKE_USER			BIT(1)
>>>> @@ -390,4 +405,9 @@
>>>>    #define XEHPC_BCS5_BCS6_INTR_MASK		XE_REG(0x190118)
>>>>    #define XEHPC_BCS7_BCS8_INTR_MASK		XE_REG(0x19011c)
>>>>
>>>> +#define PUNIT_MMIO_CR_POC_STRAPS		XE_REG(0x281078)
>>>> +#define NUM_TILES_MASK
>	REG_GENMASK(1, 0)
>>>> +#define CD_ALIVE				REG_BIT(2)
>>>> +#define SOCKET_ID_MASK
>	REG_GENMASK(7, 3)
>>>> +
>>>>    #endif
>>>> diff --git a/drivers/gpu/drm/xe/xe_device.c
>>> b/drivers/gpu/drm/xe/xe_device.c
>>>> index c7985af85a53..bf2e370a12fc 100644
>>>> --- a/drivers/gpu/drm/xe/xe_device.c
>>>> +++ b/drivers/gpu/drm/xe/xe_device.c
>>>> @@ -1,6 +1,6 @@
>>>>    // SPDX-License-Identifier: MIT
>>>>    /*
>>>> - * Copyright © 2021 Intel Corporation
>>>> + * Copyright © 2021 - 2023 Intel Corporation
>>>>     */
>>>>
>>>>    #include "xe_device.h"
>>>> @@ -22,6 +22,7 @@
>>>>    #include "xe_engine.h"
>>>>    #include "xe_exec.h"
>>>>    #include "xe_gt.h"
>>>> +#include "xe_iaf.h"
>>>>    #include "xe_irq.h"
>>>>    #include "xe_mmio.h"
>>>>    #include "xe_module.h"
>>>> @@ -255,6 +256,8 @@ int xe_device_probe(struct xe_device *xe)
>>>>    	if (err)
>>>>    		return err;
>>>>
>>>> +	xe_iaf_init_early(xe);
>>>> +
>>>>    	for_each_tile(tile, xe, id) {
>>>>    		err = xe_tile_alloc(tile);
>>>>    		if (err)
>>>> @@ -265,6 +268,8 @@ int xe_device_probe(struct xe_device *xe)
>>>>    	if (err)
>>>>    		return err;
>>>>
>>>> +	xe_iaf_init_mmio(xe);
>>>> +
>>>>    	for_each_gt(gt, xe, id) {
>>>>    		err = xe_pcode_probe(gt);
>>>>    		if (err)
>>>> @@ -285,6 +290,8 @@ int xe_device_probe(struct xe_device *xe)
>>>>    			goto err_irq_shutdown;
>>>>    	}
>>>>
>>>> +	xe_iaf_init(xe);
>>>> +
>>>>    	err = xe_mmio_probe_vram(xe);
>>>>    	if (err)
>>>>    		goto err_irq_shutdown;
>>>> @@ -326,6 +333,8 @@ int xe_device_probe(struct xe_device *xe)
>>>>
>>>>    	xe_display_register(xe);
>>>>
>>>> +	xe_iaf_init_aux(xe);
>>>
>>> Like I wrote in the top, it would be nice to have more generic calls
>>>from common code rather than assuming all XE supported platforms will
>>> use IAF driver.
>>>
>>>> +
>>>>    	xe_debugfs_register(xe);
>>>>
>>>>    	err = drmm_add_action_or_reset(&xe->drm, xe_device_sanitize,
>>> xe);
>>>> @@ -358,6 +367,8 @@ void xe_device_remove(struct xe_device *xe)
>>>>
>>>>    	xe_display_unlink(xe);
>>>>
>>>> +	xe_iaf_remove(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 0226d44a6af2..7de55a6712ad 100644
>>>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>>>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>>>> @@ -14,6 +14,7 @@
>>>>
>>>>    #include "xe_devcoredump_types.h"
>>>>    #include "xe_gt_types.h"
>>>> +#include "xe_iaf.h"
>>>>    #include "xe_platform_types.h"
>>>>    #include "xe_step_types.h"
>>>>
>>>> @@ -202,6 +203,8 @@ struct xe_device {
>>>>    		u8 supports_usm:1;
>>>>    		/** @has_asid: Has address space ID */
>>>>    		u8 has_asid:1;
>>>> +		/** @has_iaf: Has Intel Accelerated Fabric */
>>>> +		u8 has_iaf:1;
>>>>    		/** @enable_guc: GuC submission enabled */
>>>>    		u8 enable_guc:1;
>>>>    		/** @has_flat_ccs: Whether flat CCS metadata is used */
>>>> @@ -329,6 +332,28 @@ struct xe_device {
>>>>    		bool hold_rpm;
>>>>    	} mem_access;
>>>>
>>>> +	/** @intel_iaf: fabric information, for those gpus with fabric
>connectivity */
>>>> +	struct {
>>>> +		/** @ops: shared interface operations */
>>>> +		const struct iaf_ops *ops;
>>>> +		/** @handle: fabric device handle */
>>>> +		void *handle;
>>>> +		/** @pd: platform data needed for auxiliary bus */
>>>> +		struct iaf_pdata *pd;
>>>> +		/** @dpa: base device physical address */
>>>> +		u64 dpa;
>>>> +		/** @irq_base: base IRQ for multi tile devices */
>>>> +		int irq_base;
>>>> +		/** @index: internal index for xe devices */
>>>> +		int index;
>>>> +		/** @fabric_id: fabric id generated by the fabric device */
>>>> +		u32 fabric_id;
>>>> +		/** @socket_id: socket from certain platforms */
>>>> +		u8 socket_id;
>>>> +		/* @present: Reflect PUNIT presence information */
>>>> +		bool present;
>>>> +	} intel_ia > +
>>>>    	/** @d3cold_allowed: Indicates if d3cold is a valid device state */
>>>>    	bool d3cold_allowed;
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/xe_gt_types.h
>>> b/drivers/gpu/drm/xe/xe_gt_types.h
>>>> index 99ab7ec99ccd..0a56e982a6cd 100644
>>>> --- a/drivers/gpu/drm/xe/xe_gt_types.h
>>>> +++ b/drivers/gpu/drm/xe/xe_gt_types.h
>>>> @@ -334,6 +334,8 @@ struct xe_gt {
>>>>    		/** @oob: bitmap with active OOB workaroudns */
>>>>    		unsigned long *oob;
>>>>    	} wa_active;
>>>> +	/** @iaf_irq: IRQ value assigned to the ANR HW */
>>>> +	int iaf_irq;
>>>>    };
>>>>
>>>>    #endif
>>>> diff --git a/drivers/gpu/drm/xe/xe_iaf.c b/drivers/gpu/drm/xe/xe_iaf.c
>>>> index 0049e6854f80..094ff8fffa43 100644
>>>> --- a/drivers/gpu/drm/xe/xe_iaf.c
>>>> +++ b/drivers/gpu/drm/xe/xe_iaf.c
>>>> @@ -3,7 +3,20 @@
>>>>     * Copyright © 2023 Intel Corporation
>>>>     */
>>>>
>>>> +#include <linux/auxiliary_bus.h>
>>>> +#include <linux/firmware.h>
>>>> +#include <linux/irq.h>
>>>>    #include <linux/moduleparam.h>
>>>> +#include <linux/xarray.h>
>>>> +
>>>> +#include <drm/intel_iaf_platform.h>
>>>> +
>>>> +#include "xe_device.h"
>>>> +#include "xe_iaf.h"
>>>> +#include "xe_mmio.h"
>>>> +#include "xe_gt_mcr.h"
>>>> +#include "regs/xe_reg_defs.h"
>>>> +#include "regs/xe_gt_regs.h"
>>>>
>>>>    /*
>>>>     * This module parameter is needed because SRIOV PF and IAF are
>>> mutually
>>>> @@ -13,6 +26,452 @@
>>>>     * SRIOV PF path, this parameter is needed to explicitly disable IAF when
>>>>     * SRIOV PF is required.
>>>>     */
>>>> -static bool xe_enable_iaf;
>>>> +static bool xe_enable_iaf = true;
>>>>    module_param_named(enable_iaf, xe_enable_iaf, bool, 0400);
>>>>    MODULE_PARM_DESC(enable_iaf, "Enable IAF feature (default:
>true)");
>>>> +
>>>> +#define HAS_IAF(xe) ((xe)->info.has_iaf)
>>>> +/* Define the BAR and offset for the accelerator fabric CSRs */
>>>> +#define GTTMMADR_BAR 0
>>>> +#define CD_BASE_OFFSET 0x291000
>>>> +#define CD_BAR_SIZE (256 * 1024)
>>>> +
>>>> +/* Xarray of fabric devices */
>>>> +static DEFINE_XARRAY_ALLOC(intel_fdevs);
>>>> +
>>>> +static struct query_info *default_query(void *handle, u32 fabric_id)
>>>> +{
>>>> +	return ERR_PTR(-EOPNOTSUPP);
>>>> +}
>>>> +
>>>> +static int default_handle_event(void *handle, enum iaf_parent_event
>>> event)
>>>> +{
>>>> +	return -EOPNOTSUPP;
>>>> +}
>>>> +
>>>> +static const struct iaf_ops default_ops = {
>>>> +	.connectivity_query = default_query,
>>>> +	.parent_event = default_handle_event,
>>>> +};
>>>> +
>>>> +static int register_dev(void *parent, void *handle, u32 fabric_id,
>>>> +			const struct iaf_ops *ops)
>>>> +{
>>>> +	struct xe_device *xe = parent;
>>>> +
>>>> +	WARN(xe->intel_iaf.ops != &default_ops, "IAF: already registered");
>>>> +
>>>> +	xe->intel_iaf.handle = handle;
>>>> +	xe->intel_iaf.fabric_id = fabric_id;
>>>> +	xe->intel_iaf.ops = ops;
>>>> +
>>>> +	drm_info(&xe->drm, "IAF: registered fabric: 0x%x\n", fabric_id);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static void unregister_dev(void *parent, const void *handle)
>>>> +{
>>>> +	struct xe_device *xe = parent;
>>>> +
>>>> +	WARN(xe->intel_iaf.handle != handle, "IAF: invalid handle");
>>>> +
>>>> +	drm_info(&xe->drm, "IAF: unregistered fabric: 0x%x\n",
>>>> +		 xe->intel_iaf.fabric_id);
>>>> +	xe->intel_iaf.handle = NULL;
>>>> +	xe->intel_iaf.ops = &default_ops;
>>>> +}
>>>> +
>>>> +static int dev_event(void *parent, void *handle, enum iaf_dev_event
>>> event,
>>>> +		     void *event_data)
>>>> +{
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +/**
>>>> + * init_pd - Allocate and initialize platform specific data
>>>> + * @xe: Valid xe instance
>>>> + *
>>>> + * Return:
>>>> + * * pd - initialized iaf_pdata,
>>>> + * * NULL - Allocation failure
>>>> + */
>>>> +static struct iaf_pdata *init_pd(struct xe_device *xe)
>>>> +{
>>>> +	struct iaf_pdata *pd;
>>>> +	u32 reg;
>>>> +
>>>> +	pd = kzalloc(sizeof(*pd), GFP_KERNEL);
>>>> +	if (!pd)
>>>> +		return NULL;
>>>> +
>>>> +	pd->version = IAF_VERSION;
>>>> +	pd->parent = xe;
>>>> +	pd->product = IAF_PONTEVECCHIO;
>>>> +	pd->index = xe->intel_iaf.index & 0xFFFF;
>>>> +	pd->sd_cnt = xe->info.tile_count;
>>>> +	pd->socket_id = xe->intel_iaf.socket_id;
>>>> +	pd->slot = PCI_SLOT(to_pci_dev(xe->drm.dev)->devfn);
>>>> +
>>>> +	pd->resources = NULL;
>>>> +	pd->num_resources = 0;
>>>> +	pd->register_dev = register_dev;
>>>> +	pd->unregister_dev = unregister_dev;
>>>> +	pd->dev_event = dev_event;
>>>> +
>>>> +	/*
>>>> +	 * Calculate the actual DPA offset and size (in GB) for the device.
>>>> +	 * Each tile will have the same amount of memory, so we only need to
>>>> +	 * read the first one.
>>>> +	 */
>>>> +	reg = xe_gt_mcr_unicast_read_any(xe_device_get_root_tile(xe)-
>>>> primary_gt,
>>>> +					 XEHP_TILE_ADDR_RANGE(0)) &
>>> XEHP_TILE_LMEM_RANGE_MASK;
>>>> +
>>>> +	// FIXME: On some systems, TILE0 is < 8Gb, PVC needs 8GB, so fake it.
>>>> +	if (reg >> XEHP_TILE_LMEM_RANGE_SHIFT < 8) {
>>>> +		drm_err(&xe->drm, "XEHP_TILE0_ADDR_RANGE: %x\n", reg);
>>>> +		reg = 8 << XEHP_TILE_LMEM_RANGE_SHIFT;
>>>> +	}
>>>> +	pd->dpa.pkg_offset = (u32)xe->intel_iaf.index * MAX_DPA_SIZE;
>>>> +	pd->dpa.pkg_size = (reg >> XEHP_TILE_LMEM_RANGE_SHIFT) * pd-
>>>> sd_cnt;
>>>> +
>>>> +	return pd;
>>>> +}
>>>
>>> Some of this code can move to IAF driver.
>>
>> This is resource and inter-device communication information owned by the
>parent
>> driver (XE) that will be shared with the auxbus driver IAF.
>>
>> Not clear on which part could be moved to the IAF driver?
>
>My intention is XE driver should supply only the mandatory raw values,
>The IAF driver will use those values for any calculation.
>Having said that, if those values are mandatory for XE opertaion such as
>VRAM settings then ignore.

In this case the Xe device is getting the value, configuring itself, and then sharing
that info with the IAF driver.

>>
>>>> +
>>>> +/**
>>>> + * init_resource - Create the resource array, and apply the appropriate
>data
>>>> + * @xe: valid xe instance
>>>> + * @res_cnt: pointer to return number of allocated resources
>>>> + *
>>>> + * First resource [0] is for the IRQ(s).  Each device gets 1 IRQ. Subsequent
>>>> + * resources describe the IO memory space for the device(s).
>>>> + *
>>>> + * Make sure to set the gt->iaf_irq value.
>>>> + *
>>>> + * Return:
>>>> + * * res - Initialized resource array
>>>> + * * NULL - Allocaction failure
>>>> + */
>>>> +static struct resource *init_resource(struct xe_device *xe,
>>>> +				      u32 *res_cnt)
>>>> +{
>>>> +	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
>>>> +	struct xe_gt *gt;
>>>> +	struct resource *res_base, *res;
>>>> +	u32 cnt = xe->info.tile_count * 2;
>>>> +	unsigned int i;
>>>> +
>>>> +	/* Each sd gets one resource for IRQ and one for MEM */
>>>> +	res_base = kcalloc(cnt, sizeof(*res_base), GFP_KERNEL);
>>>> +	if (!res_base)
>>>> +		return NULL;
>>>> +
>>>> +	res = res_base;
>>>> +	for_each_gt(gt, xe, i) {
>>>> +		res->start = xe->intel_iaf.irq_base + i;
>>>> +		res->end = xe->intel_iaf.irq_base + i;
>>>> +		res->flags = IORESOURCE_IRQ;
>>>> +		res++;
>>>> +
>>>> +		res->start = pci_resource_start(pdev, GTTMMADR_BAR) +
>>> CD_BASE_OFFSET +
>>>> +			     i * gt_to_tile(gt)->mmio.size;
>>>> +		res->end = res->start + CD_BAR_SIZE - 1;
>>>> +		res->flags = IORESOURCE_MEM;
>>>> +		drm_info(&xe->drm, "IAF: mem_resource = %pR\n", res);
>>>> +		res++;
>>>> +		gt->iaf_irq = xe->intel_iaf.irq_base + i;
>>>> +	}
>>>> +
>>>> +	*res_cnt = cnt;
>>>> +	return res_base;
>>>> +}
>>>
>>> Can move to IAF driver as part of irq handling.
>>
>> This is gathering the resource information (register bar and IRQ) owned by
>the XE device
>> that is to be used by the IAF driver.
>>
>> I am not clear on how the IAF driver can get this info if we don't do this.  Can
>you clarify
>> your comment?
>>
>> The memory resource is part of the XE BAR space.
>>
>> The IRQ descriptor is created and then used as the base IRQ resource for the
>IAF.
>>
>> Since the IRQ (in this case) is sourced by the XE device, I am not sure how
>we could move
>> this to the IAF driver.
>>
>> When the IRQ goes off, the Xe device services it and then passed the
>notification to the IAF driver
>> (through the IRQ API), and the IAF driver "handles" the IRQ from its own
>context.
>>
>> Do you have different example of how resource information should be
>shared?
>
>We can extract here per-gt GTTMMADR_BAR resource start address and store
>it, Later on the IFA driver will perform the other specific calculations.
>My whole point is logic separation between the two drivers.

This is what the Xe driver is doing.

Note:  This a "base die" plus a fabric part.

The original architecture allows for the base die to change and to move the register
offset for the fabric part.

Because of this I wanted to supply the offset that the parent had defined rather 
than trying to derive the "right" offset as a child device.

This was an attempt to limit the amount of info that needs to be passed between the
two drivers, but allow for some flexibility based on the HW design.

>>
>>>> +
>>>> +/**
>>>> + * iaf_irq_mask - Null callback.  Masking/unmasking happens in the
>parent
>>>> + * driver
>>>> + * @d: Valid irq_data information
>>>> + */
>>>> +static void iaf_irq_mask(struct irq_data *d)
>>>> +{
>>>> +}
>>>> +
>>>> +static void iaf_irq_unmask(struct irq_data *d)
>>>> +{
>>>> +}
>>>> +
>>>> +static struct irq_chip iaf_irq_chip = {
>>>> +	.name = "iaf_irq_chip",
>>>> +	.irq_mask = iaf_irq_mask,
>>>> +	.irq_unmask = iaf_irq_unmask,
>>>> +};
>>>> +
>>>> +/**
>>>> + * init_irq_desc - Allocate IRQ descriptors to use for the iaf
>>>> + * @xe: Valid xe instance
>>>> + *
>>>> + * Allocate the required IRQ descriptor(s) and initialize the
>>>> + * appropriate state.
>>>> + *
>>>> + * Return:
>>>> + * * 0 - Success
>>>> + * * errno - Error that occurred
>>>> + */
>>>> +static int init_irq_desc(struct xe_device *xe)
>>>> +{
>>>> +	unsigned int num_subdevs = xe->info.tile_count;
>>>> +	int err;
>>>> +	int irq;
>>>> +	int irq_base;
>>>> +
>>>> +	irq_base = irq_alloc_descs(-1, 0, num_subdevs, 0);
>>>> +	if (irq_base < 0) {
>>>> +		err = irq_base;
>>>> +		goto cleanup;
>>>> +	}
>>>> +
>>>> +	err = 0;
>>>> +	for (irq = irq_base; !err && irq < irq_base + num_subdevs; irq++) {
>>>> +		irq_set_chip_and_handler_name(irq, &iaf_irq_chip,
>>>> +					      handle_simple_irq,
>>>> +					      "iaf_irq_handler");
>>>> +		err = irq_set_chip_data(irq, xe);
>>>> +	}
>>>> +
>>>> +	if (err) {
>>>> +		irq_free_descs(irq_base, num_subdevs);
>>>> +		goto cleanup;
>>>> +	}
>>>> +
>>>> +	drm_info(&xe->drm, "IAF: IRQ base: %d  cnt: %d\n", irq_base,
>>>> +		 num_subdevs);
>>>> +
>>>> +	xe->intel_iaf.irq_base = irq_base;
>>>> +
>>>> +	return 0;
>>>> +
>>>> +cleanup:
>>>> +	xe->intel_iaf.index = err;
>>>> +	drm_err(&xe->drm, "IAF: Failed to allocate IRQ data: %d\n", err);
>>>> +	return err;
>>>> +}
>>>> +
>>>> +/**
>>>> + * xe_iaf_init_early - Set the iaf info to the defaults
>>>> + * @xe: valid xe instance
>>>> + *
>>>> + * index is set to ENODEV to show that, by default, there is no device.
>>>> + * If any of the initialization steps fail, it will be set to the appropriate
>>>> + * errno value.
>>>> + */
>>>> +void xe_iaf_init_early(struct xe_device *xe)
>>>> +{
>>>> +	xe->intel_iaf.ops = &default_ops;
>>>> +	xe->intel_iaf.index = -ENODEV;
>>>> +}
>>>
>>> Not sure why we need this so 'early', can't we do it later in order to
>>> remove this call from common code?
>>
>> This is a pattern in the driver for "internal state init"... was just following that
>pattern..
>>
>> "Early init" is generally non device touching inits.
>>
>> We could move this to _init_mmio, but that is another specific init and it
>can't happen
>> after _mmio code...
>
>I agree, we indeed can move it to _init_mmio, actually _init_mmio can be
>renamed to early_init as I don't see any mmio init there anyway, only
>status register read.
>
>>
>>>> +
>>>> +/**
>>>> + * xe_iaf_init_mmio - check if iaf is available via MMIO
>>>> + * @xe: valid xe instance
>>>> + *
>>>> + * Read the relevant regs to check for IAF availability and get the socket
>id
>>>> + */
>>>> +void xe_iaf_init_mmio(struct xe_device *xe)
>>>> +{
>>>> +	u32 iaf_info;
>>>> +
>>>> +	if (!HAS_IAF(xe) || !xe_enable_iaf)
>>>> +		return;
>>>> +
>>>> +	iaf_info = xe_mmio_read32(xe_device_get_root_tile(xe)-
>>primary_gt,
>>>> +				  PUNIT_MMIO_CR_POC_STRAPS);
>>>> +
>>>> +	xe->intel_iaf.socket_id = REG_FIELD_GET(SOCKET_ID_MASK,
>iaf_info);
>>>> +
>>>> +	if (REG_FIELD_GET(CD_ALIVE, iaf_info)) {
>>>> +		drm_info(&xe->drm, "IAF available\n");
>>>> +		xe->intel_iaf.present = true;
>>>> +	}
>>>> +}
>>>
>>> As I wrote in the top of the review, if you will have mmio_read/write
>>> through aux-bus this won't be needed at all.
>>
>> This is information read from the parent device and is needed by the parent
>device to
>> determine the IAF init path.
>>
>> This cannot be done by the IAF driver since the XE IAF setup code is
>predicated on this
>> information.
>
>Yeah, now I better understand the dependencies, your previous suggestion
>though sounds good - to move _early_init code here.
>I commented there that this function might be more suitable to be called
>_early_init as we check here the presence of IAF HW.

Ok, we will consolidate and rename.

>>>> +
>>>> +/**
>>>> + * xe_iaf_init - Allocate device index and complete initial HW setup
>>>> + * @xe: valid device instance
>>>> + *
>>>> + * NOTE: index is zero inited.  If the IAF is not present, or an error occurs
>>>> + * during setup, this must be 0 for the range registers.
>>>> + *
>>>> + */
>>>> +void xe_iaf_init(struct xe_device *xe)
>>>> +{
>>>> +	struct xe_gt *gt;
>>>> +	static u32 last_id;
>>>> +	unsigned int i;
>>>> +	u32 index = 0;
>>>> +	u32 range;
>>>> +	u32 base;
>>>> +	int err;
>>>> +
>>>> +	if (!HAS_IAF(xe))
>>>> +		return;
>>>> +
>>>> +	if (!xe->intel_iaf.present)
>>>> +		goto set_range;
>>>> +
>>>> +	err = init_irq_desc(xe);
>>>> +	if (err) {
>>>> +		xe->intel_iaf.index = err;
>>>> +		goto set_range;
>>>> +	}
>>>> +
>>>> +	/*
>>>> +	 * Try the socket id first.  Systems with this feature, will
>>>> +	 * get a deterministic value.  If not, try with the cyclic.
>>>> +	 */
>>>> +	err = xa_insert(&intel_fdevs, xe->intel_iaf.socket_id, xe,
>>>> +			GFP_KERNEL);
>>>> +	if (!err)
>>>> +		index = xe->intel_iaf.socket_id;
>>>> +
>>>> +	/* socket_id is not available */
>>>> +	if (err == -EBUSY) {
>>>> +		/*
>>>> +		 * NOTE: index is only updated on success i.e. >= 0
>>>> +		 * < 0 err, 0 ok, > 0 wrapped
>>>> +		 */
>>>> +		err = xa_alloc_cyclic(&intel_fdevs, &index, xe,
>>>> +				      XA_LIMIT(0, MAX_DEVICE_COUNT - 1),
>>>> +				      &last_id, GFP_KERNEL);
>>>> +	}
>>>> +	if (err < 0) {
>>>> +		index = 0;
>>>> +		xe->intel_iaf.index = err;
>>>> +		drm_err(&xe->drm,
>>>> +			"IAF: Failed to allocate fabric index: %d\n",
>>>> +			err);
>>>> +		irq_free_descs(xe->intel_iaf.irq_base,
>>>> +			       xe->info.tile_count);
>>>> +		goto set_range;
>>>> +	}
>>>> +	xe->intel_iaf.index = index;
>>>> +	xe->intel_iaf.dpa = (u64)index * MAX_DPA_SIZE * SZ_1G;
>>>> +	drm_info(&xe->drm, "IAF: [dpa 0x%llx-0x%llx\n", xe->intel_iaf.dpa,
>>>> +		 ((u64)index + 1) * MAX_DPA_SIZE * SZ_1G - 1);
>>>> +
>>>> +	/*
>>>> +	 * Set range has to be done for all devices that support device
>>>> +	 * address space, regardless of presence or error.
>>>> +	 */
>>>> +set_range:
>>>> +	/* Set GAM address range registers */
>>>> +	range = index * MAX_DPA_SIZE << PKG_ADDR_RANGE_BASE_SHIFT;
>>>> +	range |= MAX_DPA_SIZE << PKG_ADDR_RANGE_RANGE_SHIFT;
>>>> +	range |= PKG_ADDR_RANGE_ENABLE;
>>>> +
>>>> +	/* set SGunit address range register */
>>>> +	base = index * MAX_DPA_SIZE << PKG_ADDR_BASE_BASE_SHIFT;
>>>> +	base |= MAX_DPA_SIZE << PKG_ADDR_BASE_RANGE_SHIFT;
>>>> +	base |= PKG_ADDR_BASE_ENABLE;
>>>> +
>>>> +	/* Needs to be set for each gt */
>>>> +	for_each_gt(gt, xe, i) {
>>>> +		xe_mmio_write32(gt, PKG_ADDR_RANGE, range);
>>>> +		xe_mmio_write32(gt, PKG_ADDR_BASE, base);
>>>> +	}
>>>> +}
>>>
>>> Initialization code can easily move to IAF driver.
>>
>> This is setting memory offset information for the XE device, that MUST be
>set before
>> the VRAM access is initialized.  So this set up cannot wait for the IAF driver to
>probe.
>>
>> If the IAF driver is not used, this setup is not necessary (0 based offsets are
>ok for
>> individual use), but without this offset the fabric cannot be used.
>>
>> So this cannot be done in the IAF driver.
>
>I wasn't aware, you can ignore my comment here.
>
>>
>>>> +
>>>> +static void xe_iaf_release_dev(struct device *dev)
>>>> +{
>>>> +	struct auxiliary_device *aux = to_auxiliary_dev(dev);
>>>> +	struct iaf_pdata *pd = container_of(aux, struct iaf_pdata, aux_dev);
>>>> +
>>>> +	kfree(pd->resources);
>>>> +	pd->resources = NULL;
>>>> +
>>>> +	kfree(pd);
>>>> +}
>>>> +
>>>> +/**
>>>> + * xe_iaf_init_aux - Initialize resources and add auxiliary bus interface
>>>> + * @xe: valid xe instance
>>>> + *
>>>> + */
>>>> +void xe_iaf_init_aux(struct xe_device *xe)
>>>> +{
>>>> +	struct device *dev = &to_pci_dev(xe->drm.dev)->dev;
>>>> +	struct resource *res = NULL;
>>>> +	struct iaf_pdata *pd;
>>>> +	int err = -ENOMEM;
>>>> +	u32 res_cnt;
>>>> +
>>>> +	if (!xe->intel_iaf.present)
>>>> +		return;
>>>> +
>>>> +	if (xe->intel_iaf.index < 0) {
>>>> +		err = xe->intel_iaf.index;
>>>> +		goto fail;
>>>> +	}
>>>> +
>>>> +	pd = init_pd(xe);
>>>> +	if (!pd)
>>>> +		goto cleanup;
>>>> +
>>>> +	res = init_resource(xe, &res_cnt);
>>>> +	if (!res)
>>>> +		goto cleanup;
>>>> +
>>>> +	pd->resources = res;
>>>> +	pd->num_resources = res_cnt;
>>>> +
>>>> +	pd->aux_dev.name = "iaf";
>>>> +	pd->aux_dev.id = pd->index;
>>>> +	pd->aux_dev.dev.parent = dev;
>>>> +	pd->aux_dev.dev.release = xe_iaf_release_dev;
>>>> +
>>>> +	err = auxiliary_device_init(&pd->aux_dev);
>>>> +	if (err)
>>>> +		goto cleanup;
>>>> +
>>>> +	err = auxiliary_device_add(&pd->aux_dev);
>>>> +	if (err) {
>>>> +		auxiliary_device_uninit(&pd->aux_dev);
>>>> +		goto cleanup;
>>>> +	}
>>>> +
>>>> +	xe->intel_iaf.pd = pd;
>>>> +
>>>> +	return;
>>>> +
>>>> +cleanup:
>>>> +	xa_erase(&intel_fdevs, xe->intel_iaf.index);
>>>> +	irq_free_descs(xe->intel_iaf.irq_base, xe->info.tile_count);
>>>> +	kfree(res);
>>>> +	kfree(pd);
>>>> +	xe->intel_iaf.index = err;
>>>> +fail:
>>>> +	drm_err(&xe->drm, "IAF: Failed to initialize fabric: %d\n", err);
>>>> +}
>>>> +
>>>> +void xe_iaf_remove(struct xe_device *xe)
>>>> +{
>>>> +	if (xe->intel_iaf.index < 0)
>>>> +		return;
>>>> +
>>>> +	auxiliary_device_delete(&xe->intel_iaf.pd->aux_dev);
>>>> +	auxiliary_device_uninit(&xe->intel_iaf.pd->aux_dev);
>>>> +	xa_erase(&intel_fdevs, xe->intel_iaf.index);
>>>> +	irq_free_descs(xe->intel_iaf.irq_base, xe->info.tile_count);
>>>> +
>>>> +	xe->intel_iaf.ops = &default_ops;
>>>> +}
>>>> +
>>>> +int xe_iaf_mapping_get(struct xe_device *xe)
>>>> +{
>>>> +	return xe->intel_iaf.ops->parent_event(xe->intel_iaf.handle,
>>>> +					       IAF_PARENT_MAPPING_GET);
>>>> +}
>>>> +
>>>> +int xe_iaf_mapping_put(struct xe_device *xe)
>>>> +{
>>>> +	return xe->intel_iaf.ops->parent_event(xe->intel_iaf.handle,
>>>> +					       IAF_PARENT_MAPPING_PUT);
>>>> +}
>>>
>>> I don't see any usage of those 2 functions, anyway they can also move to
>>> IAF driver.
>>
>> Yeah, these will be used later.... We can remove them from this patch.
>>
>>>> diff --git a/drivers/gpu/drm/xe/xe_iaf.h b/drivers/gpu/drm/xe/xe_iaf.h
>>>> new file mode 100644
>>>> index 000000000000..7a383406ec4d
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/xe/xe_iaf.h
>>>> @@ -0,0 +1,39 @@
>>>> +/* SPDX-License-Identifier: MIT */
>>>> +/*
>>>> + * Copyright © 2019 - 2023 Intel Corporation
>>>> + */
>>>> +
>>>> +#ifndef _XE_IAF_H_
>>>> +#define _XE_IAF_H_
>>>> +
>>>> +/*
>>>> + * Define the maximum number of devices instances based on the
>amount
>>> of
>>>> + * FID space.
>>>> + *
>>>> + * XARRAY limits are "inclusive", but using this value as a range check
>>>> + * outside of xarray, makes the exclusive upper bound a little easier to
>>>> + * deal with.
>>>> + *
>>>> + * I.e.:
>>>> + * [0 - 256)
>>>> + *
>>>> + * Less than HW supports, but more than will be currently possible.
>>>> + *
>>>> + */
>>>> +#define MAX_DEVICE_COUNT 256
>>>> +
>>>> +/* Fixed Device Physical Address (DPA) size for a device/package (in GB)
>*/
>>>> +#define MAX_DPA_SIZE 128
>>>> +
>>>> +struct xe_device;
>>>> +
>>>> +void xe_iaf_init_early(struct xe_device *xe);
>>>> +void xe_iaf_init_mmio(struct xe_device *xe);
>>>> +void xe_iaf_init(struct xe_device *xe);
>>>> +void xe_iaf_init_aux(struct xe_device *xe);
>>>> +void xe_iaf_remove(struct xe_device *xe);
>>>> +int xe_iaf_pcie_error_notify(struct xe_device *xe);
>>>
>>> Not used.
>>
>> Yeah, more future code.  We can clean up.
>>
>>>> +int xe_iaf_mapping_get(struct xe_device *xe);
>>>> +int xe_iaf_mapping_put(struct xe_device *xe);
>>>> +
>>>> +#endif
>>>> diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
>>>> index b4ed1e4a3388..edcdb1017c9a 100644
>>>> --- a/drivers/gpu/drm/xe/xe_irq.c
>>>> +++ b/drivers/gpu/drm/xe/xe_irq.c
>>>> @@ -1,10 +1,11 @@
>>>>    // SPDX-License-Identifier: MIT
>>>>    /*
>>>> - * Copyright © 2021 Intel Corporation
>>>> + * Copyright © 2021 - 2023 Intel Corporation
>>>>     */
>>>>
>>>>    #include "xe_irq.h"
>>>>
>>>> +#include <linux/irq.h>
>>>>    #include <linux/sched/clock.h>
>>>>
>>>>    #include <drm/drm_managed.h>
>>>> @@ -27,6 +28,15 @@
>>>>    #define IIR(offset)				XE_REG(offset + 0x8)
>>>>    #define IER(offset)				XE_REG(offset + 0xc)
>>>>
>>>> +/* Define the BAR and offset for the accelerator fabric CSRs */
>>>> +#define XE_IAF_IRQ BIT(8)
>>>> +#define CD_BASE_OFFSET 0x291000
>>>> +#define CD_BAR_SIZE (256 * 1024)
>>>> +
>>>> +#define CPORT_MBDB_CSRS
>>> 	XE_REG(CD_BASE_OFFSET + 0x6000)
>>>> +#define CPORT_MBDB_CSRS_END
>>> 	XE_REG(CD_BASE_OFFSET + 0x1000)
>>>> +#define CPORT_MBDB_INT_ENABLE_MASK
>>> 	XE_REG(CD_BASE_OFFSET + 0x8)
>>>> +
>>>>    static void assert_iir_is_zero(struct xe_gt *mmio, struct xe_reg reg)
>>>>    {
>>>>    	u32 val = xe_mmio_read32(mmio, reg);
>>>> @@ -355,6 +365,18 @@ static void dg1_intr_enable(struct xe_device *xe,
>>> bool stall)
>>>>    		xe_mmio_read32(mmio, DG1_MSTR_TILE_INTR);
>>>>    }
>>>>
>>>> +/*
>>>> + * iaf_irq_handler - handle accelerator fabric IRQs
>>>> + *
>>>> + * PVC can have an accelerator fabric attached.  Handle the IRQs that are
>>>> + * sourced by the device supporting the fabric.
>>>> + */
>>>> +static void iaf_irq_handler(struct xe_gt *gt, const u32 master_ctl)
>>>> +{
>>>> +	if (master_ctl & XE_IAF_IRQ)
>>>> +		generic_handle_irq(gt->iaf_irq);
>>>> +}
>>>> +
>>>>    /*
>>>>     * Top-level interrupt handler for Xe_LP+ and beyond.  These platforms
>>> have
>>>>     * a "master tile" interrupt register which must be consulted before the
>>>> @@ -409,6 +431,7 @@ static irqreturn_t dg1_irq_handler(int irq, void
>*arg)
>>>>    			xe_display_irq_handler(xe, master_ctl);
>>>>    			gu_misc_iir = gu_misc_irq_ack(xe, master_ctl);
>>>>    		}
>>>> +		iaf_irq_handler(mmio, master_ctl);
>>>>    	}
>>>>
>>>>    	dg1_intr_enable(xe, false);
>>>> @@ -455,6 +478,9 @@ static void gt_irq_reset(struct xe_tile *tile)
>>>>    	xe_mmio_write32(mmio, GPM_WGBOXPERF_INTR_MASK,  ~0);
>>>>    	xe_mmio_write32(mmio, GUC_SG_INTR_ENABLE,	 0);
>>>>    	xe_mmio_write32(mmio, GUC_SG_INTR_MASK,		~0);
>>>> +
>>>> +	if (tile->xe->intel_iaf.present)
>>>> +		xe_mmio_write64(mmio, CPORT_MBDB_INT_ENABLE_MASK,
>>> 0);
>>>>    }
>>>>
>>>>    static void xelp_irq_reset(struct xe_tile *tile)
>>>> diff --git a/drivers/gpu/drm/xe/xe_mmio.c
>>> b/drivers/gpu/drm/xe/xe_mmio.c
>>>> index f1336803b915..c962a697e86b 100644
>>>> --- a/drivers/gpu/drm/xe/xe_mmio.c
>>>> +++ b/drivers/gpu/drm/xe/xe_mmio.c
>>>> @@ -173,7 +173,7 @@ static int xe_determine_lmem_bar_size(struct
>>> xe_device *xe)
>>>>    	if (!xe->mem.vram.io_size)
>>>>    		return -EIO;
>>>>
>>>> -	xe->mem.vram.base = 0; /* DPA offset */
>>>> +	xe->mem.vram.base = xe->intel_iaf.dpa;
>>>>
>>>>    	/* set up a map to the total memory area. */
>>>>    	xe->mem.vram.mapping = ioremap_wc(xe->mem.vram.io_start, xe-
>>>> mem.vram.io_size);
>>>
>>> irq handling can be done in IAF driver using aux-bus.
>>
>> This is not an IRQ handler, this is for the internal offset of the device
>memory.
>>
>> If you have multiple XE (PVC) devices each one needs a unique memory
>range, and this
>> is where this is being determined.
>>
>> So no IRQ info here.
>
>Sorry, this comment is not in the correct location, I was referring to
>'iaf_irq_handler(mmio, master_ctl);'. In IAF case the handler is rather
>small but in other cases the handler might be more complex and hence we
>should consider moving it to the IAF driver.

Ahhh.  Ok.

The XE device (and its associated parts, read IAF) has one effective IRQ.   When an interrupt
is generated by the IAF device, it is routed to the Xe device IRQ and the Xe device needs to
do work (so the IAF IRQ is not a separately resourced IRQ).

The Xe driver receives the IRQ, "handles" its part of it, and then calls the
genreric_handle_irq() which will deliver the IRQ to the IAF driver at the IRQ level.

So the IAF driver does have an IRQ handler as well (that does more work).

The IAF driver registers for and handles this IRQ from its own context (the irq info being
passed as a resource via the auxbus init).

Originally this was a call back, registered by the IAF driver, but I ran into some load/unload
race conditions, so I went with this implementation...

If there is a better way to handle this, we would be very willing to work with it. 😊

Thanks,

Mike

>--
>Ofir
>
>>
>> The IAF driver requests the IRQ based on the resource created by the XE
>driver.
>>
>> This was modeled after some other Auxbus drivers.
>>
>> Is there different way to share IRQ resource information via auxbus?
>>
>> Patch 3 of this series shows how the IAF driver uses the memory and IRQ
>resources.
>>
>> Thank you for your comments, lets continue to discuss! >
>> Mike
>>
>>> --
>>> Ofir
>>>
>>>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>>>> index 2991cf5365d5..e38c62ed9a6c 100644
>>>> --- a/drivers/gpu/drm/xe/xe_pci.c
>>>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>>>> @@ -135,6 +135,7 @@ static const struct xe_graphics_desc
>graphics_xehpc
>>> = {
>>>>
>>>>    	.has_asid = 1,
>>>>    	.has_flat_ccs = 0,
>>>> +	.has_iaf = 1,
>>>>    	.has_link_copy_engine = 1,
>>>>    	.supports_usm = 1,
>>>>    };
>>>> @@ -529,6 +530,7 @@ static int xe_info_init(struct xe_device *xe,
>>>>    	xe->info.vm_max_level = graphics_desc->vm_max_level;
>>>>    	xe->info.supports_usm = graphics_desc->supports_usm;
>>>>    	xe->info.has_asid = graphics_desc->has_asid;
>>>> +	xe->info.has_iaf = graphics_desc->has_iaf;
>>>>    	xe->info.has_flat_ccs = graphics_desc->has_flat_ccs;
>>>>    	xe->info.has_range_tlb_invalidation = graphics_desc-
>>>> has_range_tlb_invalidation;
>>>>    	xe->info.has_link_copy_engine = graphics_desc-
>>>> has_link_copy_engine;
>>>> diff --git a/drivers/gpu/drm/xe/xe_pci_types.h
>>> b/drivers/gpu/drm/xe/xe_pci_types.h
>>>> index ba31b933eb8e..54d78cbbc70e 100644
>>>> --- a/drivers/gpu/drm/xe/xe_pci_types.h
>>>> +++ b/drivers/gpu/drm/xe/xe_pci_types.h
>>>> @@ -22,6 +22,7 @@ struct xe_graphics_desc {
>>>>    	u8 max_remote_tiles:2;
>>>>
>>>>    	u8 has_asid:1;
>>>> +	u8 has_iaf:1;
>>>>    	u8 has_flat_ccs:1;
>>>>    	u8 has_link_copy_engine:1;
>>>>    	u8 has_range_tlb_invalidation:1;
>>>> diff --git a/include/drm/intel_iaf_platform.h
>>> b/include/drm/intel_iaf_platform.h
>>>> new file mode 100644
>>>> index 000000000000..b2a1e75b1f75
>>>> --- /dev/null
>>>> +++ b/include/drm/intel_iaf_platform.h
>>>> @@ -0,0 +1,143 @@
>>>> +/* SPDX-License-Identifier: MIT */
>>>> +/*
>>>> + * Copyright(c) 2019 - 2023 Intel Corporation.
>>>> + */
>>>> +
>>>> +#ifndef __INTEL_IAF_PLATFORM_H
>>>> +#define __INTEL_IAF_PLATFORM_H
>>>> +
>>>> +#define IAF_VERSION 1
>>>> +
>>>> +#include <linux/auxiliary_bus.h>
>>>> +#include <linux/types.h>
>>>> +
>>>> +/**
>>>> + * enum product_type - Product type identifying the parent
>>>> + * @IAF_UNDEFINED: no product type hints
>>>> + * @IAF_PONTEVECCHIO: parent is a PVC
>>>> + * @IAF_PRODUCTS: end of the list
>>>> + *
>>>> + * i.e. Pontevecchio is the first product.  Other products would
>>>> + * be next generation GPU, NIC, etc.
>>>> + */
>>>> +enum product_type {
>>>> +	IAF_UNDEFINED,
>>>> +	IAF_PONTEVECCHIO,
>>>> +	IAF_PRODUCTS
>>>> +};
>>>> +
>>>> +/**
>>>> + * enum iaf_dev_event - events generated to the parent device
>>>> + * @IAF_DEV_CONNECTIVITY: Connectivity event occurred
>>>> + * @IAF_DEV_ERROR: An error occurred
>>>> + * @IAF_DEV_EVENTS: end of list
>>>> + *
>>>> + * Connectivity events, possible errors, etc.
>>>> + */
>>>> +enum iaf_dev_event {
>>>> +	IAF_DEV_CONNECTIVITY,
>>>> +	IAF_DEV_ERROR,
>>>> +	IAF_DEV_EVENTS
>>>> +};
>>>> +
>>>> +/**
>>>> + * enum iaf_parent_event - Events generated by the parent device
>>>> + * @IAF_PARENT_PCIE_ERR: Parent had a PCI error
>>>> + * @IAF_PARENT_MAPPING_GET: Notify IAF of buffer mapping
>>>> + * @IAF_PARENT_MAPPING_PUT: Notify IAF of buffer unmapping
>>>> + *
>>>> + * These are examples.
>>>> + */
>>>> +enum iaf_parent_event {
>>>> +	IAF_PARENT_PCIE_ERR,
>>>> +	IAF_PARENT_MAPPING_GET,
>>>> +	IAF_PARENT_MAPPING_PUT,
>>>> +};
>>>> +
>>>> +/**
>>>> + * struct sd2sd_info - Subdevice to subdevice connectivity info
>>>> + * @bandwidth: in Gbps units not factoring in width or quality
>degredation
>>>> + * @latency: in 1/10 hops units
>>>> + */
>>>> +struct sd2sd_info {
>>>> +	u16 bandwidth;
>>>> +	u16 latency;
>>>> +};
>>>> +
>>>> +/**
>>>> + * struct query_info - connectivity query response information
>>>> + * @src_cnt: Requester subdevice count
>>>> + * @dst_cnt: Destination path count
>>>> + * @sd2sd: array of src/dst bandwidth/latency information
>>>> + *
>>>> + * Query info will be a variably sized data structure allocated by the
>>>> + * IAF driver.  The access will be indexed by
>>>> + *    (src_index * dst_cnt) + dst_index
>>>> + *
>>>> + * The caller will need to free the buffer when done.
>>>> + */
>>>> +struct query_info {
>>>> +	u8 src_cnt;
>>>> +	u8 dst_cnt;
>>>> +	struct sd2sd_info sd2sd[];
>>>> +};
>>>> +
>>>> +/**
>>>> + * struct iaf_ops - Communication path from parent to IAF instance
>>>> + * @connectivity_query: Query a device for fabric_id connectivity
>>>> + * @parent_event: Any events needed by the IAF device
>>>> + *
>>>> + * connectivity_query() returns:
>>>> + *   a populated query_info on success,
>>>> + *   an ERR_PTR() on failure
>>>> + *
>>>> + */
>>>> +struct iaf_ops {
>>>> +	struct query_info *(*connectivity_query)(void *handle, u32
>>> fabric_id);
>>>> +	int (*parent_event)(void *handle, enum iaf_parent_event event);
>>>> +};
>>>> +
>>>> +struct dpa_space {
>>>> +	u32 pkg_offset;
>>>> +	u16 pkg_size;
>>>> +};
>>>> +
>>>> +/**
>>>> + * struct iaf_pdata - Platform specific data that needs to be shared
>>>> + * @version: PSD version information
>>>> + * @parent: Handle to use when calling the parent device
>>>> + * @product: a product hint for any necessary special case requirements
>>>> + * @index: unique device index. This will be part of the device name
>>>> + * @dpa: Device physical address offset and size
>>>> + * @sd_cnt: parent subdevice count
>>>> + * @socket_id: device socket information
>>>> + * @slot: PCI/CXL slot number
>>>> + * @aux_dev: Auxiliary bus device
>>>> + * @resources: Array of resources (Assigned by Xe, the IRQ/MEM for
>the
>>> device)
>>>> + * @num_resources: number of resources in resources array
>>>> + * @register_dev: Register an IAF instance and ops with the parent
>device
>>>> + * @unregister_dev: Unregister an IAF instance from the parent device
>>>> + * @dev_event: Notify parent that an event has occurred
>>>> + */
>>>> +struct iaf_pdata {
>>>> +	u16 version;
>>>> +	void *parent;
>>>> +	enum product_type product;
>>>> +	u16 index;
>>>> +	struct dpa_space dpa;
>>>> +	u8 sd_cnt;
>>>> +	u8 socket_id;
>>>> +	u8 slot;
>>>> +
>>>> +	struct auxiliary_device aux_dev;
>>>> +	struct resource *resources;
>>>> +	u32 num_resources;
>>>> +
>>>> +	int (*register_dev)(void *parent, void *handle, u32 fabric_id,
>>>> +			    const struct iaf_ops *ops);
>>>> +	void (*unregister_dev)(void *parent, const void *handle);
>>>> +	int (*dev_event)(void *parent, void *handle,
>>>> +			 enum iaf_dev_event event, void *event_data);
>>>> +};
>>>> +
>>>> +#endif /* __INTEL_IAF_PLATFORM_H */
>>>
>>
>



More information about the Intel-xe mailing list