[Intel-xe] [RFC PATCH 1/1] drm/xe: Introduce function pointers for MMIO functions

Thu Jun 8 17:35:29 UTC 2023

On Fri, Jun 02, 2023 at 02:25:01PM +0000, Francois Dugast wrote:
>A local structure of function pointers is used as a minimal hardware
>abstraction layer to prepare for platform independent MMIO calls.
>
>Cc: Oded Gabbay <ogabbay at kernel.org>
>Cc: Ofir Bitton <ofir1.bitton at intel.com>
>Cc: Ohad Sharabi <ohadshar at intel.com>
>Signed-off-by: Francois Dugast <francois.dugast at intel.com>
>---
> drivers/gpu/drm/xe/xe_device_types.h |  3 ++
> drivers/gpu/drm/xe/xe_mmio.c         | 81 ++++++++++++++++++++++++++++
> drivers/gpu/drm/xe/xe_mmio.h         | 35 ++++++------
> 3 files changed, 99 insertions(+), 20 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
>index 17b6b1cc5adb..3f8fd0d8129b 100644
>--- a/drivers/gpu/drm/xe/xe_device_types.h
>+++ b/drivers/gpu/drm/xe/xe_device_types.h
>@@ -378,6 +378,9 @@ struct xe_device {
> 	/** @d3cold_allowed: Indicates if d3cold is a valid device state */
> 	bool d3cold_allowed;
>
>+	/** @mmio_funcs: function pointers for MMIO related functions */
>+	const struct xe_mmio_funcs *mmio_funcs;
>+
> 	/* private: */
>
> #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
>index 475b14fe4356..f3d08676a77a 100644
>--- a/drivers/gpu/drm/xe/xe_mmio.c
>+++ b/drivers/gpu/drm/xe/xe_mmio.c
>@@ -25,6 +25,62 @@
>
> #define BAR_SIZE_SHIFT 20
>
>+static void xe_mmio_write32_device(struct xe_gt *gt,
>+				   struct xe_reg reg, u32 val);
>+static u32 xe_mmio_read32_device(struct xe_gt *gt, struct xe_reg reg);
>+static void xe_mmio_write64_device(struct xe_gt *gt,
>+				   struct xe_reg reg, u64 val);
>+static u64 xe_mmio_read64_device(struct xe_gt *gt, struct xe_reg reg);
>+
>+static const struct xe_mmio_funcs xe_mmio_funcs_device = {
>+	.write32 = xe_mmio_write32_device,
>+	.read32 = xe_mmio_read32_device,
>+	.write64 = xe_mmio_write64_device,
>+	.read64 = xe_mmio_read64_device,
>+};
>+
>+static inline void xe_mmio_write32_device(struct xe_gt *gt,
>+				   struct xe_reg reg, u32 val)
>+{
>+	struct xe_tile *tile = gt_to_tile(gt);
>+
>+	if (reg.addr < gt->mmio.adj_limit)
>+		reg.addr += gt->mmio.adj_offset;
>+
>+	writel(val, tile->mmio.regs + reg.addr);
>+}
>+
>+static inline u32 xe_mmio_read32_device(struct xe_gt *gt, struct xe_reg reg)
>+{
>+	struct xe_tile *tile = gt_to_tile(gt);
>+
>+	if (reg.addr < gt->mmio.adj_limit)
>+		reg.addr += gt->mmio.adj_offset;
>+
>+	return readl(tile->mmio.regs + reg.addr);
>+}
>+
>+static inline void xe_mmio_write64_device(struct xe_gt *gt,
>+				   struct xe_reg reg, u64 val)
>+{
>+	struct xe_tile *tile = gt_to_tile(gt);
>+
>+	if (reg.addr < gt->mmio.adj_limit)
>+		reg.addr += gt->mmio.adj_offset;
>+
>+	writeq(val, tile->mmio.regs + reg.addr);
>+}
>+
>+static inline u64 xe_mmio_read64_device(struct xe_gt *gt, struct xe_reg reg)
>+{
>+	struct xe_tile *tile = gt_to_tile(gt);
>+
>+	if (reg.addr < gt->mmio.adj_limit)
>+		reg.addr += gt->mmio.adj_offset;
>+
>+	return readq(tile->mmio.regs + reg.addr);
>+}
>+
> static int xe_set_dma_info(struct xe_device *xe)
> {
> 	unsigned int mask_size = xe->info.dma_mask_size;
>@@ -377,6 +433,29 @@ static void mmio_fini(struct drm_device *drm, void *arg)
> 		iounmap(xe->mem.vram.mapping);
> }
>
>+static void xe_mmio_set_funcs(struct xe_device *xe)
>+{
>+	/* For now all platforms use the set of MMIO functions for a
>+	 * physical device.
>+	 */

what is "device" in this context? that seems confusing as we always ever
just support reading/writing to a real device (physical here may also
add to the confusion when thinking about SR-IOV and VFs).
We shouldn't add abstractions that are then never used and all platforms
end up using the same. Unless it's a preparation for a follow up series
adding the different handling.

+Matt as there is still (at least) one refactor planned in this area,
since gt is not always the proper target for the MMIOs. He was saying in
his earlier series about having a mmio_view or such to abstract the
offset and other differences between each IO range. Not sure if this
series would go the rigth direction, maybe we need to think in both
cases together.

Lucas De Marchi

>+	switch (xe->info.platform) {
>+	case XE_TIGERLAKE:
>+	case XE_ROCKETLAKE:
>+	case XE_ALDERLAKE_S:
>+	case XE_ALDERLAKE_P:
>+	case XE_ALDERLAKE_N:
>+	case XE_DG1:
>+	case XE_DG2:
>+	case XE_PVC:
>+	case XE_METEORLAKE:
>+		xe->mmio_funcs = &xe_mmio_funcs_device;
>+		break;
>+	default:
>+		DRM_ERROR("Unsupported platform\n");
>+		break;
>+	}
>+}
>+
> int xe_mmio_init(struct xe_device *xe)
> {
> 	struct xe_tile *root_tile = xe_device_get_root_tile(xe);
>@@ -384,6 +463,8 @@ int xe_mmio_init(struct xe_device *xe)
> 	const int mmio_bar = 0;
> 	int err;
>
>+	xe_mmio_set_funcs(xe);
>+
> 	/*
> 	 * Map the first 16MB of th BAR, which includes the registers (0-4MB),
> 	 * reserved space (4MB-8MB), and GGTT (8MB-16MB) for a single tile.
>diff --git a/drivers/gpu/drm/xe/xe_mmio.h b/drivers/gpu/drm/xe/xe_mmio.h
>index 3c547d78afba..80ce9de7aac4 100644
>--- a/drivers/gpu/drm/xe/xe_mmio.h
>+++ b/drivers/gpu/drm/xe/xe_mmio.h
>@@ -19,6 +19,13 @@ struct xe_device;
>
> #define GEN12_LMEM_BAR		2
>
>+struct xe_mmio_funcs {
>+	u32 (*read32)(struct xe_gt *gt, struct xe_reg reg);
>+	u64 (*read64)(struct xe_gt *gt, struct xe_reg reg);
>+	void (*write32)(struct xe_gt *gt, struct xe_reg reg, u32 val);
>+	void (*write64)(struct xe_gt *gt, struct xe_reg reg, u64 val);
>+};
>+
> int xe_mmio_init(struct xe_device *xe);
>
> static inline u8 xe_mmio_read8(struct xe_gt *gt, struct xe_reg reg)
>@@ -34,22 +41,16 @@ static inline u8 xe_mmio_read8(struct xe_gt *gt, struct xe_reg reg)
> static inline void xe_mmio_write32(struct xe_gt *gt,
> 				   struct xe_reg reg, u32 val)
> {
>-	struct xe_tile *tile = gt_to_tile(gt);
>-
>-	if (reg.addr < gt->mmio.adj_limit)
>-		reg.addr += gt->mmio.adj_offset;
>+	struct xe_device *xe = gt_to_xe(gt);
>
>-	writel(val, tile->mmio.regs + reg.addr);
>+	xe->mmio_funcs->write32(gt, reg, val);
> }
>
> static inline u32 xe_mmio_read32(struct xe_gt *gt, struct xe_reg reg)
> {
>-	struct xe_tile *tile = gt_to_tile(gt);
>-
>-	if (reg.addr < gt->mmio.adj_limit)
>-		reg.addr += gt->mmio.adj_offset;
>+	struct xe_device *xe = gt_to_xe(gt);
>
>-	return readl(tile->mmio.regs + reg.addr);
>+	return xe->mmio_funcs->read32(gt, reg);
> }
>
> static inline u32 xe_mmio_rmw32(struct xe_gt *gt, struct xe_reg reg, u32 clr,
>@@ -67,22 +68,16 @@ static inline u32 xe_mmio_rmw32(struct xe_gt *gt, struct xe_reg reg, u32 clr,
> static inline void xe_mmio_write64(struct xe_gt *gt,
> 				   struct xe_reg reg, u64 val)
> {
>-	struct xe_tile *tile = gt_to_tile(gt);
>-
>-	if (reg.addr < gt->mmio.adj_limit)
>-		reg.addr += gt->mmio.adj_offset;
>+	struct xe_device *xe = gt_to_xe(gt);
>
>-	writeq(val, tile->mmio.regs + reg.addr);
>+	xe->mmio_funcs->write64(gt, reg, val);
> }
>
> static inline u64 xe_mmio_read64(struct xe_gt *gt, struct xe_reg reg)
> {
>-	struct xe_tile *tile = gt_to_tile(gt);
>-
>-	if (reg.addr < gt->mmio.adj_limit)
>-		reg.addr += gt->mmio.adj_offset;
>+	struct xe_device *xe = gt_to_xe(gt);
>
>-	return readq(tile->mmio.regs + reg.addr);
>+	return xe->mmio_funcs->read64(gt, reg);
> }
>
> static inline int xe_mmio_write32_and_verify(struct xe_gt *gt,
>-- 
>2.34.1
>