[Intel-xe] [PATCH 4/5] fixup! drm/xe/display: Implement display support

[Lucas De Marchi]

On Tue, Jun 27, 2023 at 01:22:22PM +0300, Jani Nikula wrote:
>Add raw_reg_* accessors and use them.
>
>Signed-off-by: Jani Nikula <jani.nikula at intel.com>
>---
> .../drm/xe/compat-i915-headers/intel_uncore.h | 24 +++++++++++++++++++
> drivers/gpu/drm/xe/display/ext/i915_irq.c     | 15 ++----------
> 2 files changed, 26 insertions(+), 13 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h b/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h
>index 652654b5481d..a46dca558366 100644
>--- a/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h
>+++ b/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h
>@@ -140,4 +140,28 @@ static inline void intel_uncore_write_notrace(struct intel_uncore *uncore,
> 	xe_mmio_write32(__compat_uncore_to_gt(uncore), reg, val);
> }
>
>+static inline void __iomem *intel_uncore_regs(struct intel_uncore *uncore)
>+{
>+	struct xe_device *xe = container_of(uncore, struct xe_device, uncore);
>+
>+	return xe_device_get_root_tile(xe)->mmio.regs;
>+}
>+
>+/*
>+ * The raw_reg_{read,write} macros are intended as a micro-optimization for
>+ * interrupt handlers so that the pointer indirection on uncore->regs can
>+ * be computed once (and presumably cached in a register) instead of generating
>+ * extra load instructions for each MMIO access.
>+ *
>+ * Given that these macros are only intended for non-GSI interrupt registers
>+ * (and the goal is to avoid extra instructions generated by the compiler),
>+ * these macros do not account for uncore->gsi_offset.  Any caller that needs
>+ * to use these macros on a GSI register is responsible for adding the
>+ * appropriate GSI offset to the 'base' parameter.
>+ */
>+#define raw_reg_read(base, reg) \

it's more a "map" rather than a "base". Currently it bypasses any
forcewake, gsi and it's also hidden from tracing.

As a micro-optimization I wonder if there are numbers anywhere to
justify them.  As for the *move* being done here from
xe/display/ext/i915_irq.c to xe/compat-i915-headers/intel_uncore.h, I'm
ok with it. But why are we converting it to a macro rather than the
inline function?  That would mean it also silently ignores any steering
needed, adding to the list above of things ignored, since there is no
type check anymore.

Lucas De Marchi

>+	readl(base + i915_mmio_reg_offset(reg))
>+#define raw_reg_write(base, reg, value) \
>+	writel(value, base + i915_mmio_reg_offset(reg))
>+
> #endif /* __INTEL_UNCORE_H__ */
>diff --git a/drivers/gpu/drm/xe/display/ext/i915_irq.c b/drivers/gpu/drm/xe/display/ext/i915_irq.c
>index 6235ff9dec36..157403d1d8fe 100644
>--- a/drivers/gpu/drm/xe/display/ext/i915_irq.c
>+++ b/drivers/gpu/drm/xe/display/ext/i915_irq.c
>@@ -35,8 +35,10 @@
> #include <drm/drm_drv.h>
>
> #include "i915_drv.h"
>+#include "i915_irq.h"
> #include "i915_reg.h"
> #include "icl_dsi_regs.h"
>+#include "intel_clock_gating.h"
> #include "intel_display_trace.h"
> #include "intel_display_types.h"
> #include "intel_dp_aux.h"
>@@ -48,19 +50,6 @@
> #include "intel_psr_regs.h"
> #include "intel_uncore.h"
>
>-static u32 raw_reg_read(void __iomem *base, i915_reg_t reg)
>-{
>-	return readl(base + reg.reg);
>-}
>-
>-static void raw_reg_write(void __iomem *base, i915_reg_t reg, u32 value)
>-{
>-	writel(value, base + reg.reg);
>-}
>-
>-#include "i915_irq.h"
>-#include "intel_clock_gating.h"
>-
> static void gen3_irq_reset(struct xe_device *dev_priv, i915_reg_t imr,
> 		    i915_reg_t iir, i915_reg_t ier)
> {
>-- 
>2.39.2
>