[PATCH] TEST ONLY: drm/intel: add mmio to display parent interface

[PATCH] TEST ONLY: drm/intel: add mmio to display parent interface

[Jani Nikula]

Just testing, no need to review. We probably don't want this.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/i9xx_wm.c       |  5 +-
 drivers/gpu/drm/i915/display/intel_de.c      |  6 +-
 drivers/gpu/drm/i915/display/intel_de.h      | 23 ++----
 drivers/gpu/drm/i915/display/intel_display.c |  3 +-
 drivers/gpu/drm/i915/display/intel_vblank.c  |  6 +-
 drivers/gpu/drm/i915/i915_driver.c           |  1 +
 drivers/gpu/drm/i915/intel_uncore.c          | 80 ++++++++++++++++++++
 drivers/gpu/drm/i915/intel_uncore.h          |  2 +
 drivers/gpu/drm/xe/display/xe_display.c      | 78 +++++++++++++++++++
 include/drm/intel/display_parent_interface.h | 17 +++++
 10 files changed, 192 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/i9xx_wm.c b/drivers/gpu/drm/i915/display/i9xx_wm.c
index 0233f5053d95..785f1a7e38a1 100644
--- a/drivers/gpu/drm/i915/display/i9xx_wm.c
+++ b/drivers/gpu/drm/i915/display/i9xx_wm.c
@@ -1863,7 +1863,6 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
 				   struct intel_crtc *crtc)
 {
 	struct intel_display *display = to_intel_display(crtc);
-	struct intel_uncore *uncore = to_intel_uncore(display->drm);
 	const struct intel_crtc_state *crtc_state =
 		intel_atomic_get_new_crtc_state(state, crtc);
 	const struct vlv_fifo_state *fifo_state =
@@ -1892,7 +1891,7 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
 	 * intel_pipe_update_start() has already disabled interrupts
 	 * for us, so a plain spin_lock() is sufficient here.
 	 */
-	spin_lock(&uncore->lock);
+	display->parent->mmio->lock(display->drm);
 
 	switch (crtc->pipe) {
 	case PIPE_A:
@@ -1952,7 +1951,7 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
 
 	intel_de_read_fw(display, DSPARB(display));
 
-	spin_unlock(&uncore->lock);
+	display->parent->mmio->unlock(display->drm);
 }
 
 #undef VLV_FIFO
diff --git a/drivers/gpu/drm/i915/display/intel_de.c b/drivers/gpu/drm/i915/display/intel_de.c
index a7417905192d..3f042bbcc80f 100644
--- a/drivers/gpu/drm/i915/display/intel_de.c
+++ b/drivers/gpu/drm/i915/display/intel_de.c
@@ -145,14 +145,14 @@ u8 intel_de_read8(struct intel_display *display, intel_reg_t reg)
 	/* this is only used on VGA registers (possible on pre-g4x) */
 	drm_WARN_ON(display->drm, DISPLAY_VER(display) >= 5 || display->platform.g4x);
 
-	return intel_uncore_read8(__to_uncore(display), reg);
+	return display->parent->mmio->read8(display->drm, intel_reg_offset(reg));
 }
 
 void intel_de_write8(struct intel_display *display, intel_reg_t reg, u8 val)
 {
 	drm_WARN_ON(display->drm, DISPLAY_VER(display) >= 5 || display->platform.g4x);
 
-	intel_uncore_write8(__to_uncore(display), reg, val);
+	display->parent->mmio->write8(display->drm, intel_reg_offset(reg), val);
 }
 
 u16 intel_de_read16(struct intel_display *display, intel_reg_t reg)
@@ -160,5 +160,5 @@ u16 intel_de_read16(struct intel_display *display, intel_reg_t reg)
 	/* this is only used on MCHBAR registers on pre-SNB */
 	drm_WARN_ON(display->drm, DISPLAY_VER(display) >= 6);
 
-	return intel_uncore_read16(__to_uncore(display), reg);
+	return display->parent->mmio->read16(display->drm, intel_reg_offset(reg));
 }
diff --git a/drivers/gpu/drm/i915/display/intel_de.h b/drivers/gpu/drm/i915/display/intel_de.h
index d0a239a36980..1fddb85d5640 100644
--- a/drivers/gpu/drm/i915/display/intel_de.h
+++ b/drivers/gpu/drm/i915/display/intel_de.h
@@ -6,16 +6,11 @@
 #ifndef __INTEL_DE_H__
 #define __INTEL_DE_H__
 
+#include <drm/intel/display_parent_interface.h>
+
 #include "intel_display_core.h"
 #include "intel_dmc_wl.h"
 #include "intel_dsb.h"
-#include "intel_uncore.h"
-#include "intel_uncore_trace.h"
-
-static inline struct intel_uncore *__to_uncore(struct intel_display *display)
-{
-	return to_intel_uncore(display->drm);
-}
 
 u8 intel_de_read8(struct intel_display *display, intel_reg_t reg);
 void intel_de_write8(struct intel_display *display, intel_reg_t reg, u8 val);
@@ -28,7 +23,7 @@ intel_de_read(struct intel_display *display, intel_reg_t reg)
 
 	intel_dmc_wl_get(display, reg);
 
-	val = intel_uncore_read(__to_uncore(display), reg);
+	val = display->parent->mmio->read32(display->drm, intel_reg_offset(reg));
 
 	intel_dmc_wl_put(display, reg);
 
@@ -58,7 +53,7 @@ intel_de_write(struct intel_display *display, intel_reg_t reg, u32 val)
 {
 	intel_dmc_wl_get(display, reg);
 
-	intel_uncore_write(__to_uncore(display), reg, val);
+	display->parent->mmio->write32(display->drm, intel_reg_offset(reg), val);
 
 	intel_dmc_wl_put(display, reg);
 }
@@ -107,19 +102,13 @@ int intel_de_wait_for_clear_ms(struct intel_display *display, intel_reg_t reg,
 static inline u32
 intel_de_read_fw(struct intel_display *display, intel_reg_t reg)
 {
-	u32 val;
-
-	val = intel_uncore_read_fw(__to_uncore(display), reg);
-	trace_i915_reg_rw(false, reg, val, sizeof(val), true);
-
-	return val;
+	return display->parent->mmio->read32_fw(display->drm, intel_reg_offset(reg));
 }
 
 static inline void
 intel_de_write_fw(struct intel_display *display, intel_reg_t reg, u32 val)
 {
-	trace_i915_reg_rw(true, reg, val, sizeof(val), true);
-	intel_uncore_write_fw(__to_uncore(display), reg, val);
+	display->parent->mmio->write32_fw(display->drm, intel_reg_offset(reg), val);
 }
 
 static inline u32
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index c1c2b99cfb53..99864877b74d 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -7731,7 +7731,6 @@ static void intel_atomic_dsb_finish(struct intel_atomic_state *state,
 static void intel_atomic_commit_tail(struct intel_atomic_state *state)
 {
 	struct intel_display *display = to_intel_display(state);
-	struct intel_uncore *uncore = to_intel_uncore(display->drm);
 	struct intel_crtc_state *new_crtc_state, *old_crtc_state;
 	struct intel_crtc *crtc;
 	struct intel_power_domain_mask put_domains[I915_MAX_PIPES] = {};
@@ -7945,7 +7944,7 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
 		 * so enable debugging for the next modeset - and hope we catch
 		 * the culprit.
 		 */
-		intel_uncore_arm_unclaimed_mmio_detection(uncore);
+		display->parent->mmio->arm_unclaimed_mmio_detection(display->drm);
 	}
 
 	power_async_delay = intel_display_power_select_target_dc_state(state);
diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
index 1d4753605675..5a0af95a7442 100644
--- a/drivers/gpu/drm/i915/display/intel_vblank.c
+++ b/drivers/gpu/drm/i915/display/intel_vblank.c
@@ -294,15 +294,13 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
 static void intel_vblank_section_enter(struct intel_display *display)
 	__acquires(uncore->lock)
 {
-	struct intel_uncore *uncore = to_intel_uncore(display->drm);
-	spin_lock(&uncore->lock);
+	display->parent->mmio->lock(display->drm);
 }
 
 static void intel_vblank_section_exit(struct intel_display *display)
 	__releases(uncore->lock)
 {
-	struct intel_uncore *uncore = to_intel_uncore(display->drm);
-	spin_unlock(&uncore->lock);
+	display->parent->mmio->unlock(display->drm);
 }
 #else
 static void intel_vblank_section_enter(struct intel_display *display)
diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index 5787f40ac62e..451a27fed0e7 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -753,6 +753,7 @@ static const struct intel_display_parent_interface parent = {
 	.hdcp = &i915_display_hdcp_interface,
 	.initial_plane = &i915_display_initial_plane_interface,
 	.irq = &i915_display_irq_interface,
+	.mmio = &i915_display_mmio_interface,
 	.overlay = &i915_display_overlay_interface,
 	.panic = &i915_display_panic_interface,
 	.pc8 = &i915_display_pc8_interface,
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index e360e4f9d9a2..b11be8acf5b3 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -26,6 +26,7 @@
 
 #include <drm/drm_managed.h>
 #include <drm/drm_print.h>
+#include <drm/intel/display_parent_interface.h>
 
 #include "display/intel_display_core.h"
 #include "gt/intel_engine_regs.h"
@@ -2923,6 +2924,85 @@ intel_uncore_forcewake_for_reg(struct intel_uncore *uncore,
 	return fw_domains;
 }
 
+static u8 i915_uncore_read8(struct drm_device *drm, u32 reg)
+{
+	return intel_uncore_read8(to_intel_uncore(drm), _MMIO(reg));
+}
+
+static void i915_uncore_write8(struct drm_device *drm, u32 reg, u8 val)
+{
+	intel_uncore_write8(to_intel_uncore(drm), _MMIO(reg), val);
+}
+
+static u16 i915_uncore_read16(struct drm_device *drm, u32 reg)
+{
+	return intel_uncore_read16(to_intel_uncore(drm), _MMIO(reg));
+}
+
+static void i915_uncore_write16(struct drm_device *drm, u32 reg, u16 val)
+{
+	intel_uncore_write16(to_intel_uncore(drm), _MMIO(reg), val);
+}
+
+static u32 i915_uncore_read32(struct drm_device *drm, u32 reg)
+{
+	return intel_uncore_read(to_intel_uncore(drm), _MMIO(reg));
+}
+
+static u32 i915_uncore_read32_fw(struct drm_device *drm, u32 reg)
+{
+	u32 val;
+
+	val = intel_uncore_read_fw(to_intel_uncore(drm), _MMIO(reg));
+	trace_i915_reg_rw(false, _MMIO(reg), val, sizeof(val), true);
+
+	return val;
+}
+
+static void i915_uncore_write32(struct drm_device *drm, u32 reg, u32 val)
+{
+	intel_uncore_write(to_intel_uncore(drm), _MMIO(reg), val);
+}
+
+static void i915_uncore_write32_fw(struct drm_device *drm, u32 reg, u32 val)
+{
+	trace_i915_reg_rw(true, _MMIO(reg), val, sizeof(val), true);
+	intel_uncore_write_fw(to_intel_uncore(drm), _MMIO(reg), val);
+}
+
+static void i915_uncore_lock(struct drm_device *drm)
+{
+	struct intel_uncore *uncore = to_intel_uncore(drm);
+
+	spin_lock(&uncore->lock);
+}
+
+static void i915_uncore_unlock(struct drm_device *drm)
+{
+	struct intel_uncore *uncore = to_intel_uncore(drm);
+
+	spin_unlock(&uncore->lock);
+}
+
+static void i915_uncore_arm_unclaimed_mmio_detection(struct drm_device *drm)
+{
+	intel_uncore_arm_unclaimed_mmio_detection(to_intel_uncore(drm));
+}
+
+const struct intel_display_mmio_interface i915_display_mmio_interface = {
+	.read8 = i915_uncore_read8,
+	.write8 = i915_uncore_write8,
+	.read16 = i915_uncore_read16,
+	.write16 = i915_uncore_write16,
+	.read32 = i915_uncore_read32,
+	.read32_fw = i915_uncore_read32_fw,
+	.write32 = i915_uncore_write32,
+	.write32_fw = i915_uncore_write32_fw,
+	.lock = i915_uncore_lock,
+	.unlock = i915_uncore_unlock,
+	.arm_unclaimed_mmio_detection = i915_uncore_arm_unclaimed_mmio_detection,
+};
+
 #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
 #include "selftests/mock_uncore.c"
 #include "selftests/intel_uncore.c"
diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
index fafc2ca9a237..0f29fd0a6309 100644
--- a/drivers/gpu/drm/i915/intel_uncore.h
+++ b/drivers/gpu/drm/i915/intel_uncore.h
@@ -515,4 +515,6 @@ struct intel_uncore *to_intel_uncore(struct drm_device *drm);
 #define raw_reg_write(base, reg, value) \
 	writel(value, base + i915_mmio_reg_offset(reg))
 
+extern const struct intel_display_mmio_interface i915_display_mmio_interface;
+
 #endif /* !__INTEL_UNCORE_H__ */
diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
index aab416485e3e..42cc19009bab 100644
--- a/drivers/gpu/drm/xe/display/xe_display.c
+++ b/drivers/gpu/drm/xe/display/xe_display.c
@@ -33,6 +33,7 @@
 #include "xe_frontbuffer.h"
 #include "xe_hdcp_gsc.h"
 #include "xe_initial_plane.h"
+#include "xe_mmio.h"
 #include "xe_module.h"
 #include "xe_panic.h"
 #include "xe_stolen.h"
@@ -444,6 +445,82 @@ static void transient_data_flush(struct drm_device *drm)
 	xe_device_td_flush(xe);
 }
 
+static struct xe_mmio *to_xe_mmio(struct drm_device *drm)
+{
+	return xe_root_tile_mmio(to_xe_device(drm));
+}
+
+static u8 xe_uncore_read8(struct drm_device *drm, u32 reg)
+{
+	return xe_mmio_read8(to_xe_mmio(drm), XE_REG(reg));
+}
+
+static void xe_uncore_write8(struct drm_device *drm, u32 reg, u8 val)
+{
+	xe_mmio_write8(to_xe_mmio(drm), XE_REG(reg), val);
+}
+
+static u16 xe_uncore_read16(struct drm_device *drm, u32 reg)
+{
+	return xe_mmio_read16(to_xe_mmio(drm), XE_REG(reg));
+}
+
+static void xe_uncore_write16(struct drm_device *drm, u32 reg, u16 val)
+{
+}
+
+static u32 xe_uncore_read32(struct drm_device *drm, u32 reg)
+{
+	return xe_mmio_read32(to_xe_mmio(drm), XE_REG(reg));
+}
+
+static u32 xe_uncore_read32_fw(struct drm_device *drm, u32 reg)
+{
+	return xe_mmio_read32(to_xe_mmio(drm), XE_REG(reg));
+}
+
+static void xe_uncore_write32(struct drm_device *drm, u32 reg, u32 val)
+{
+	xe_mmio_write32(to_xe_mmio(drm), XE_REG(reg), val);
+}
+
+static void xe_uncore_write32_fw(struct drm_device *drm, u32 reg, u32 val)
+{
+	xe_mmio_write32(to_xe_mmio(drm), XE_REG(reg), val);
+}
+
+static void xe_uncore_lock(struct drm_device *drm)
+{
+	struct xe_device *xe = to_xe_device(drm);
+
+	spin_lock(&xe->uncore.lock);
+}
+
+static void xe_uncore_unlock(struct drm_device *drm)
+{
+	struct xe_device *xe = to_xe_device(drm);
+
+	spin_unlock(&xe->uncore.lock);
+}
+
+static void xe_uncore_arm_unclaimed_mmio_detection(struct drm_device *drm)
+{
+}
+
+const struct intel_display_mmio_interface xe_display_mmio_interface = {
+	.read8 = xe_uncore_read8,
+	.write8 = xe_uncore_write8,
+	.read16 = xe_uncore_read16,
+	.write16 = xe_uncore_write16,
+	.read32 = xe_uncore_read32,
+	.read32_fw = xe_uncore_read32_fw,
+	.write32 = xe_uncore_write32,
+	.write32_fw = xe_uncore_write32_fw,
+	.lock = xe_uncore_lock,
+	.unlock = xe_uncore_unlock,
+	.arm_unclaimed_mmio_detection = xe_uncore_arm_unclaimed_mmio_detection,
+};
+
 static const struct intel_display_parent_interface parent = {
 	.bo = &xe_display_bo_interface,
 	.dsb = &xe_display_dsb_interface,
@@ -452,6 +529,7 @@ static const struct intel_display_parent_interface parent = {
 	.hdcp = &xe_display_hdcp_interface,
 	.initial_plane = &xe_display_initial_plane_interface,
 	.irq = &xe_display_irq_interface,
+	.mmio = &xe_display_mmio_interface,
 	.panic = &xe_display_panic_interface,
 	.pcode = &xe_display_pcode_interface,
 	.rpm = &xe_display_rpm_interface,
diff --git a/include/drm/intel/display_parent_interface.h b/include/drm/intel/display_parent_interface.h
index 464fb588cb60..22a90a7411cc 100644
--- a/include/drm/intel/display_parent_interface.h
+++ b/include/drm/intel/display_parent_interface.h
@@ -136,6 +136,20 @@ struct intel_display_irq_interface {
 	void (*synchronize)(struct drm_device *drm);
 };
 
+struct intel_display_mmio_interface {
+	u8 (*read8)(struct drm_device *drm, u32 reg);
+	void (*write8)(struct drm_device *drm, u32 reg, u8 val);
+	u16 (*read16)(struct drm_device *drm, u32 reg);
+	void (*write16)(struct drm_device *drm, u32 reg, u16 val);
+	u32 (*read32)(struct drm_device *drm, u32 reg);
+	u32 (*read32_fw)(struct drm_device *drm, u32 reg);
+	void (*write32)(struct drm_device *drm, u32 reg, u32 val);
+	void (*write32_fw)(struct drm_device *drm, u32 reg, u32 val);
+	void (*lock)(struct drm_device *drm);
+	void (*unlock)(struct drm_device *drm);
+	void (*arm_unclaimed_mmio_detection)(struct drm_device *drm);
+};
+
 struct intel_display_overlay_interface {
 	bool (*is_active)(struct drm_device *drm);
 
@@ -272,6 +286,9 @@ struct intel_display_parent_interface {
 	/** @irq: IRQ interface */
 	const struct intel_display_irq_interface *irq;
 
+	/** @mmio: MMIO interface */
+	const struct intel_display_mmio_interface *mmio;
+
 	/** @panic: Panic interface */
 	const struct intel_display_panic_interface *panic;
 
-- 
2.47.3

Re: [PATCH] TEST ONLY: drm/intel: add mmio to display parent interface

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] drm/i915/display: indirect call overhead in intel_de_read_fw() and write_fw()
- [Low] drm/i915/display: broken context lock annotations in intel_vblank.c
- [Low] drm/xe/display: missing static qualifier on xe_display_mmio_interface
- [Low] drm/intel: unused write16 hook and silent no-op in xe_uncore_write16()
- [Low] drm/xe: orphaned uncore compat headers and TEST ONLY commit message
--

commit 0609c4221fda9fb179c2667810e576a6d92944ab
Author: Jani Nikula <jani.nikula@intel.com>

TEST ONLY: drm/intel: add mmio to display parent interface

This commit introduces an intel_display_mmio_interface to the display parent
interface, converting display MMIO register accesses, uncore spinlock calls,
and unclaimed MMIO detection in i915 and Xe to go through parent interface
function pointers.

>     TEST ONLY: drm/intel: add mmio to display parent interface
>     
>     Just testing, no need to review. We probably don't want this.
>     
>     Signed-off-by: Jani Nikula <jani.nikula@intel.com>

[ ... ]

> diff --git a/drivers/gpu/drm/i915/display/intel_de.h b/drivers/gpu/drm/i915/display/intel_de.h
> index d0a239a369807..1fddb85d5640a 100644
> --- a/drivers/gpu/drm/i915/display/intel_de.h
> +++ b/drivers/gpu/drm/i915/display/intel_de.h
> @@ -6,16 +6,11 @@
>  #ifndef __INTEL_DE_H__
>  #define __INTEL_DE_H__
>  
> +#include <drm/intel/display_parent_interface.h>
> +
>  #include "intel_display_core.h"
>  #include "intel_dmc_wl.h"
>  #include "intel_dsb.h"
> -#include "intel_uncore.h"
> -#include "intel_uncore_trace.h"
> -
> -static inline struct intel_uncore *__to_uncore(struct intel_display *display)
> -{
> -	return to_intel_uncore(display->drm);
> -}

[Severity: Low]
This isn't a runtime bug, but removing the intel_uncore.h and
intel_uncore_trace.h includes from drivers/gpu/drm/i915/display/intel_de.h
removes the last include sites for
drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h and
drivers/gpu/drm/xe/compat-i915-headers/intel_uncore_trace.h.

Should those two unused compatibility headers in Xe be removed as part of
this change, and should the placeholder commit subject and message be
updated to describe the change and its rationale?

>  u8 intel_de_read8(struct intel_display *display, intel_reg_t reg);
>  void intel_de_write8(struct intel_display *display, intel_reg_t reg, u8 val);
> @@ -28,7 +23,7 @@ intel_de_read(struct intel_display *display, intel_reg_t reg)
>  
>  	intel_dmc_wl_get(display, reg);
>  
> -	val = intel_uncore_read(__to_uncore(display), reg);
> +	val = display->parent->mmio->read32(display->drm, intel_reg_offset(reg));
>  
>  	intel_dmc_wl_put(display, reg);
>  
> @@ -58,7 +53,7 @@ intel_de_write(struct intel_display *display, intel_reg_t reg, u32 val)
>  {
>  	intel_dmc_wl_get(display, reg);
>  
> -	intel_uncore_write(__to_uncore(display), reg, val);
> +	display->parent->mmio->write32(display->drm, intel_reg_offset(reg), val);
>  
>  	intel_dmc_wl_put(display, reg);
>  }
> @@ -107,19 +102,13 @@ int intel_de_wait_for_clear_ms(struct intel_display *display, intel_reg_t reg,
>  static inline u32
>  intel_de_read_fw(struct intel_display *display, intel_reg_t reg)
>  {
> -	u32 val;
> -
> -	val = intel_uncore_read_fw(__to_uncore(display), reg);
> -	trace_i915_reg_rw(false, reg, val, sizeof(val), true);
> -
> -	return val;
> +	return display->parent->mmio->read32_fw(display->drm, intel_reg_offset(reg));
>  }
>  
>  static inline void
>  intel_de_write_fw(struct intel_display *display, intel_reg_t reg, u32 val)
>  {
> -	trace_i915_reg_rw(true, reg, val, sizeof(val), true);
> -	intel_uncore_write_fw(__to_uncore(display), reg, val);
> +	display->parent->mmio->write32_fw(display->drm, intel_reg_offset(reg), val);
>  }

[Severity: Medium]
Won't routing intel_de_read_fw() and intel_de_write_fw() (as well as
intel_de_read() and intel_de_write()) in
drivers/gpu/drm/i915/display/intel_de.h through indirect function pointers
in display->parent->mmio introduce a performance regression on hot MMIO
paths?

In particular, intel_de_read_fw() and intel_de_write_fw() are used inside
vblank evasion critical sections with local interrupts disabled to minimize
MMIO latency. Replacing the inlined register accesses with indirect calls
through display->parent->mmio->read32_fw() and write32_fw() adds pointer
chasing, indirect branch overhead, and repeated drm-to-uncore/mmio lookups
on every register access.

Also, drivers/gpu/drm/i915/display/intel_de.h, i9xx_wm.c, intel_display.c,
and intel_vblank.c dereference display->parent->mmio directly instead of
going through helpers in drivers/gpu/drm/i915/display/intel_parent.c like
the other parent interfaces. Is that intentional?

[ ... ]

> diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
> index 1d4753605675e..5a0af95a7442b 100644
> --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> @@ -294,15 +294,13 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
>  static void intel_vblank_section_enter(struct intel_display *display)
>  	__acquires(uncore->lock)
>  {
> -	struct intel_uncore *uncore = to_intel_uncore(display->drm);
> -	spin_lock(&uncore->lock);
> +	display->parent->mmio->lock(display->drm);
>  }
>  
>  static void intel_vblank_section_exit(struct intel_display *display)
>  	__releases(uncore->lock)
>  {
> -	struct intel_uncore *uncore = to_intel_uncore(display->drm);
> -	spin_unlock(&uncore->lock);
> +	display->parent->mmio->unlock(display->drm);
>  }
>  #else
>  static void intel_vblank_section_enter(struct intel_display *display)

[Severity: Low]
Does drivers/gpu/drm/i915/display/intel_vblank.c fail to build when compiler
context analysis (CONFIG_WARN_CONTEXT_ANALYSIS_ALL) is enabled?

Both intel_vblank_section_enter() and intel_vblank_section_exit() remove the
local uncore variable (and intel_de.h no longer includes intel_uncore.h),
but they still retain:

    __acquires(uncore->lock)
    __releases(uncore->lock)

which reference the undeclared identifier uncore when context analysis
expands those attributes.

In addition, the new lock/unlock callbacks i915_uncore_lock() and
i915_uncore_unlock() in drivers/gpu/drm/i915/intel_uncore.c, as well as
xe_uncore_lock() and xe_uncore_unlock() in
drivers/gpu/drm/xe/display/xe_display.c, acquire and release spinlocks
across function boundaries without context analysis annotations, and
intel_vblank_section_enter() / intel_vblank_section_exit() remain guarded by
#ifdef I915 even though mmio->lock and mmio->unlock are now part of the
parent interface.

[ ... ]

> diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
> index aab416485e3ea..42cc19009bab2 100644
> --- a/drivers/gpu/drm/xe/display/xe_display.c
> +++ b/drivers/gpu/drm/xe/display/xe_display.c
> @@ -444,6 +445,82 @@ static void transient_data_flush(struct drm_device *drm)
>  	xe_device_td_flush(xe);
>  }
>  
> +static struct xe_mmio *to_xe_mmio(struct drm_device *drm)
> +{
> +	return xe_root_tile_mmio(to_xe_device(drm));
> +}
> +
> +static u8 xe_uncore_read8(struct drm_device *drm, u32 reg)
> +{
> +	return xe_mmio_read8(to_xe_mmio(drm), XE_REG(reg));
> +}
> +
> +static void xe_uncore_write8(struct drm_device *drm, u32 reg, u8 val)
> +{
> +	xe_mmio_write8(to_xe_mmio(drm), XE_REG(reg), val);
> +}
> +
> +static u16 xe_uncore_read16(struct drm_device *drm, u32 reg)
> +{
> +	return xe_mmio_read16(to_xe_mmio(drm), XE_REG(reg));
> +}
> +
> +static void xe_uncore_write16(struct drm_device *drm, u32 reg, u16 val)
> +{
> +}

[Severity: Low]
Why is write16 added to struct intel_display_mmio_interface in
include/drm/intel/display_parent_interface.h with an empty no-op
implementation in xe_uncore_write16()?

The display driver has no intel_de_write16() helper and never calls
display->parent->mmio->write16, and having xe_uncore_write16() silently
discard writes on Xe without a warning could hide bugs if a caller is ever
added. Also, since xe_uncore_read32_fw() and xe_uncore_write32_fw() are
identical to xe_uncore_read32() and xe_uncore_write32() on Xe, could they
share the same function pointers in xe_display_mmio_interface?

> +
> +static u32 xe_uncore_read32(struct drm_device *drm, u32 reg)
> +{
> +	return xe_mmio_read32(to_xe_mmio(drm), XE_REG(reg));
> +}
> +
> +static u32 xe_uncore_read32_fw(struct drm_device *drm, u32 reg)
> +{
> +	return xe_mmio_read32(to_xe_mmio(drm), XE_REG(reg));
> +}
> +
> +static void xe_uncore_write32(struct drm_device *drm, u32 reg, u32 val)
> +{
> +	xe_mmio_write32(to_xe_mmio(drm), XE_REG(reg), val);
> +}
> +
> +static void xe_uncore_write32_fw(struct drm_device *drm, u32 reg, u32 val)
> +{
> +	xe_mmio_write32(to_xe_mmio(drm), XE_REG(reg), val);
> +}
> +
> +static void xe_uncore_lock(struct drm_device *drm)
> +{
> +	struct xe_device *xe = to_xe_device(drm);
> +
> +	spin_lock(&xe->uncore.lock);
> +}
> +
> +static void xe_uncore_unlock(struct drm_device *drm)
> +{
> +	struct xe_device *xe = to_xe_device(drm);
> +
> +	spin_unlock(&xe->uncore.lock);
> +}
> +
> +static void xe_uncore_arm_unclaimed_mmio_detection(struct drm_device *drm)
> +{
> +}
> +
> +const struct intel_display_mmio_interface xe_display_mmio_interface = {

[Severity: Low]
Should xe_display_mmio_interface in drivers/gpu/drm/xe/display/xe_display.c
be declared static const?

It has no external declaration in any header and is only used within
xe_display.c to initialize parent.mmio, so omitting static triggers
-Wmissing-variable-declarations and Sparse -Wdecl warnings (unlike the
sibling xe_display_irq_interface in the same file, which is marked static).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104130831.152563-1-jani.nikula@intel.com?part=1

✓ i915.CI.BAT: success for TEST ONLY: drm/intel: add mmio to display parent interface

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 6765 bytes --]

== Series Details ==

Series: TEST ONLY: drm/intel: add mmio to display parent interface
URL   : https://patchwork.freedesktop.org/series/177723/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_19444 -> Patchwork_177723v1
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_177723v1/index.html

Participating hosts (38 -> 38)
------------------------------

  Additional (2): fi-skl-6600u bat-adls-6 
  Missing    (2): bat-dg2-13 fi-glk-j4005 

Known issues
------------

  Here are the changes found in Patchwork_177723v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_huc_copy@huc-copy:
    - fi-skl-6600u:       NOTRUN -> [SKIP][1] ([i915#2190])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_177723v1/fi-skl-6600u/igt@gem_huc_copy@huc-copy.html

  * igt@gem_lmem_swapping@parallel-random-engines:
    - bat-adls-6:         NOTRUN -> [SKIP][2] ([i915#4613]) +3 other tests skip
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_177723v1/bat-adls-6/igt@gem_lmem_swapping@parallel-random-engines.html

  * igt@gem_lmem_swapping@random-engines:
    - fi-skl-6600u:       NOTRUN -> [SKIP][3] ([i915#4613]) +3 other tests skip
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_177723v1/fi-skl-6600u/igt@gem_lmem_swapping@random-engines.html

  * igt@gem_tiled_pread_basic@basic:
    - bat-adls-6:         NOTRUN -> [SKIP][4] ([i915#15656])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_177723v1/bat-adls-6/igt@gem_tiled_pread_basic@basic.html

  * igt@intel_hwmon@hwmon-read:
    - bat-adls-6:         NOTRUN -> [SKIP][5] ([i915#7707]) +1 other test skip
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_177723v1/bat-adls-6/igt@intel_hwmon@hwmon-read.html
    - fi-skl-6600u:       NOTRUN -> [SKIP][6] ([i915#11680]) +1 other test skip
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_177723v1/fi-skl-6600u/igt@intel_hwmon@hwmon-read.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
    - bat-adls-6:         NOTRUN -> [SKIP][7] ([i915#4103]) +1 other test skip
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_177723v1/bat-adls-6/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
    - fi-skl-6600u:       NOTRUN -> [SKIP][8] ([i915#17457]) +1 other test skip
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_177723v1/fi-skl-6600u/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html

  * igt@kms_dsc@dsc-basic:
    - fi-skl-6600u:       NOTRUN -> [SKIP][9] ([i915#17458])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_177723v1/fi-skl-6600u/igt@kms_dsc@dsc-basic.html
    - bat-adls-6:         NOTRUN -> [SKIP][10] ([i915#16361])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_177723v1/bat-adls-6/igt@kms_dsc@dsc-basic.html

  * igt@kms_force_connector_basic@force-load-detect:
    - bat-adls-6:         NOTRUN -> [SKIP][11] ([i915#17452])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_177723v1/bat-adls-6/igt@kms_force_connector_basic@force-load-detect.html
    - fi-skl-6600u:       NOTRUN -> [SKIP][12] ([i915#17465])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_177723v1/fi-skl-6600u/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_pipe_crc_basic@compare-crc-sanitycheck-nv12@pipe-b-edp-1:
    - fi-skl-6600u:       NOTRUN -> [SKIP][13] ([i915#17461]) +3 other tests skip
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_177723v1/fi-skl-6600u/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-nv12@pipe-b-edp-1.html

  * igt@kms_pm_backlight@basic-brightness:
    - bat-adls-6:         NOTRUN -> [SKIP][14] ([i915#5354])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_177723v1/bat-adls-6/igt@kms_pm_backlight@basic-brightness.html

  * igt@kms_psr@psr-primary-mmap-gtt:
    - bat-adls-6:         NOTRUN -> [SKIP][15] ([i915#1072] / [i915#9732]) +3 other tests skip
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_177723v1/bat-adls-6/igt@kms_psr@psr-primary-mmap-gtt.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - fi-skl-6600u:       NOTRUN -> [SKIP][16] ([i915#17463])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_177723v1/fi-skl-6600u/igt@kms_setmode@basic-clone-single-crtc.html
    - bat-adls-6:         NOTRUN -> [SKIP][17] ([i915#3555])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_177723v1/bat-adls-6/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-fence-read:
    - bat-adls-6:         NOTRUN -> [SKIP][18] ([i915#17448] / [i915#3291]) +2 other tests skip
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_177723v1/bat-adls-6/igt@prime_vgem@basic-fence-read.html

  
  [i915#1072]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/1072
  [i915#11680]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11680
  [i915#15656]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/15656
  [i915#16361]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/16361
  [i915#17448]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/17448
  [i915#17452]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/17452
  [i915#17457]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/17457
  [i915#17458]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/17458
  [i915#17461]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/17461
  [i915#17463]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/17463
  [i915#17465]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/17465
  [i915#2190]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/2190
  [i915#3291]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/3291
  [i915#3555]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/3555
  [i915#4103]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/4103
  [i915#4613]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/4613
  [i915#5354]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/5354
  [i915#7707]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/7707
  [i915#9732]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9732


Build changes
-------------

  * Linux: CI_DRM_19444 -> Patchwork_177723v1

  CI-20190529: 20190529
  CI_DRM_19444: c0cb7e7c00c9563d72c1e1b08fac139a57fd0ebf @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_9177: 9177
  Patchwork_177723v1: c0cb7e7c00c9563d72c1e1b08fac139a57fd0ebf @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_177723v1/index.html

[-- Attachment #2: Type: text/html, Size: 7763 bytes --]

✗ i915.CI.Full: failure for TEST ONLY: drm/intel: add mmio to display parent interface

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 1452 bytes --]

== Series Details ==

Series: TEST ONLY: drm/intel: add mmio to display parent interface
URL   : https://patchwork.freedesktop.org/series/177723/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_19444_full -> Patchwork_177723v1_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_177723v1_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_177723v1_full, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

Participating hosts (1 -> 0)
------------------------------

  ERROR: It appears as if the changes made in Patchwork_177723v1_full prevented too many machines from booting.

  Missing    (1): pig-kbl-iris 


Changes
-------

  No changes found


Build changes
-------------

  * Linux: CI_DRM_19444 -> Patchwork_177723v1

  CI_DRM_19444: c0cb7e7c00c9563d72c1e1b08fac139a57fd0ebf @ git://anongit.freedesktop.org/gfx-ci/linux
  Patchwork_177723v1: c0cb7e7c00c9563d72c1e1b08fac139a57fd0ebf @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_177723v1/index.html

[-- Attachment #2: Type: text/html, Size: 2027 bytes --]