[PATCH 1/2] drm/i915/display: remove small micro-optimizations in irq handling

[Lucas De Marchi]

On Mon, Apr 08, 2024 at 03:54:44PM GMT, Jani Nikula wrote:
>The raw register reads/writes are there as micro-optimizations to avoid
>multiple pointer indirections on uncore->regs. Presumably this is useful
>when there are plenty of register reads/writes in the same
>function. However, the display irq handling only has a few raw
>reads/writes. Remove them for simplification.

I think that comment didn't age well. Not to say there's something wrong
with this commit, but just to make sure we are aware of the additional
stuff going on and we if we are ok with that.

using intel_de_read() in place of raw_reg_read() will do (for newer
platforms):

	1) Read FPGA_DBG to detect unclaimed access before the actual read
	2) Find the relevant forcewake for that register, acquire and wait for ack
	3) readl(reg)
	4) Read FPGA_DBG to detect unclaimed access after the actual read
	5) Trace reg rw

That's much more than a pointer indirection. Are we ok with that in the
irq?  Also, I don't know why but we have variants to skip tracing (step
5 above), but on my books a disabled tracepoint is order of magnitudes
less overhead than 1, 2 and 4.

btw, if we drop the raw accesses, then we can probably drop (1) above.

Lucas De Marchi

>
>Signed-off-by: Jani Nikula <jani.nikula at intel.com>
>---
> drivers/gpu/drm/i915/display/intel_display_irq.c | 15 +++++++--------
> 1 file changed, 7 insertions(+), 8 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c b/drivers/gpu/drm/i915/display/intel_display_irq.c
>index f846c5b108b5..d4ae9139be39 100644
>--- a/drivers/gpu/drm/i915/display/intel_display_irq.c
>+++ b/drivers/gpu/drm/i915/display/intel_display_irq.c
>@@ -1148,15 +1148,14 @@ void gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
>
> u32 gen11_gu_misc_irq_ack(struct drm_i915_private *i915, const u32 master_ctl)
> {
>-	void __iomem * const regs = intel_uncore_regs(&i915->uncore);
> 	u32 iir;
>
> 	if (!(master_ctl & GEN11_GU_MISC_IRQ))
> 		return 0;
>
>-	iir = raw_reg_read(regs, GEN11_GU_MISC_IIR);
>+	iir = intel_de_read(i915, GEN11_GU_MISC_IIR);
> 	if (likely(iir))
>-		raw_reg_write(regs, GEN11_GU_MISC_IIR, iir);
>+		intel_de_write(i915, GEN11_GU_MISC_IIR, iir);
>
> 	return iir;
> }
>@@ -1169,18 +1168,18 @@ void gen11_gu_misc_irq_handler(struct drm_i915_private *i915, const u32 iir)
>
> void gen11_display_irq_handler(struct drm_i915_private *i915)
> {
>-	void __iomem * const regs = intel_uncore_regs(&i915->uncore);
>-	const u32 disp_ctl = raw_reg_read(regs, GEN11_DISPLAY_INT_CTL);
>+	u32 disp_ctl;
>
> 	disable_rpm_wakeref_asserts(&i915->runtime_pm);
> 	/*
> 	 * GEN11_DISPLAY_INT_CTL has same format as GEN8_MASTER_IRQ
> 	 * for the display related bits.
> 	 */
>-	raw_reg_write(regs, GEN11_DISPLAY_INT_CTL, 0x0);
>+	disp_ctl = intel_de_read(i915, GEN11_DISPLAY_INT_CTL);
>+
>+	intel_de_write(i915, GEN11_DISPLAY_INT_CTL, 0);
> 	gen8_de_irq_handler(i915, disp_ctl);
>-	raw_reg_write(regs, GEN11_DISPLAY_INT_CTL,
>-		      GEN11_DISPLAY_IRQ_ENABLE);
>+	intel_de_write(i915, GEN11_DISPLAY_INT_CTL, GEN11_DISPLAY_IRQ_ENABLE);
>
> 	enable_rpm_wakeref_asserts(&i915->runtime_pm);
> }
>-- 
>2.39.2
>