Re: [PATCH 8/9] drm/i915/de: Stop using intel_uncore_{read,write}()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Nov 06, 2026 at 05:37:59PM +0200, Jani Nikula wrote:
> On Thu, 05 Nov 2026, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Stop using the heavier intel_uncore_{read,write}() and
> > just use intel_uncore_{read,write}_fw() for all display
> > register accesses.
> >
> > The main functional change here is that we now stop
> > grabbing uncore.lock for display registers. Its job
> > is now fully taken over by the display mmio.lock.
> >
> > The one little thing we want to keep from the uncore
> > implementation is the runtime pm asserts (sadly xe
> > doesn't implement them, but i915 does). We have the
> > parent interface for it, so just use it. That is one
> > extra vfunc call, but not sure there's any way around
> > it.
> >
> > One somewhat unexpected side effect (or bonus) is that
> > on ILK we also lose ilk_dummy_write(), which is currently
> > done ahead of all (non-fw) mmio accesses. The only
> > purpose of the dummy write is to wake the GT from
> > RC6 ahead of the actual mmio access. Display has
> > nothing to do with RC6, and I don't think we actually
> > ever enabled RC6 on ILK, so the whole thing is pointless.
> >
> > As a result of eliminating ilk_dummy_write() display
> > register accesses become faster on ILK (numbers in
> > nanoseconds):
> >       | fw  |   non-fw   |
> > read  | 140 | 430 -> 140 |
> > write | 170 | 470 -> 170 |
> >
> > Register accesses on TGL also get a bit faster,
> > compared against the original code before the
> > display mmio vfuncs were introduced:
> >       | fw  |   non-fw   | theoretical non-fw(*)
> > read  | 230 | 970 -> 840 | 2*230+230 = 690
> > write | 100 | 820 -> 740 | 2*230+100 = 560
> >
> > (*) two FPGA_DBG reads + the actual register access
> >
> > I presume the improvement was due to some combination of
> > eliminating the extra debug lock and just reducing the
> > runtime overhead in general. The remaining delta between
> > theory and practice seems to be mostly due to mmio.lock.
> > We'll probably want to think of ways to eliminate the lock
> > entirely, on the platforms that don't need it for register
> > access serialization...
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I guess would like to see mmio tracing pulled into display side too.

That'll happen eventually. The current problem is that
(unline i915 core stuff) xe traces even the _fw()
accesses, so if we switch to a display specific
tracepoint right now we'll get double traces on xe
if the xe tracpoint is also enabled. But perhaps
that's fine as a temporary state of affairs?

> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> > ---
> >  drivers/gpu/drm/i915/display/intel_de.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_de.c b/drivers/gpu/drm/i915/display/intel_de.c
> > index 0a51105e0fba..397145bc7e5b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_de.c
> > +++ b/drivers/gpu/drm/i915/display/intel_de.c
> > @@ -9,6 +9,7 @@
> >  
> >  #include "intel_de.h"
> >  #include "intel_display_regs.h"
> > +#include "intel_display_rpm.h"
> >  
> >  static int intel_de_wait_for_register(struct intel_display *display,
> >  				      intel_reg_t reg, u32 mask, u32 value,
> > @@ -341,11 +342,13 @@ __intel_de_read(struct intel_display *display, intel_reg_t reg,
> >  	unsigned long irqflags;
> >  	u32 val;
> >  
> > +	assert_display_rpm_held(display);
> > +
> >  	__mmio_wakelock_pre(display, reg, mmio_flags);
> >  	__mmio_lock_pre(display, &irqflags, mmio_flags);
> >  	__mmio_unclaimed_pre(display, reg, false, mmio_flags);
> >  
> > -	val = intel_uncore_read(__to_uncore(display), reg);
> > +	val = intel_de_read_fw(display, reg);
> >  
> >  	__mmio_unclaimed_post(display, reg, false, mmio_flags);
> >  	__mmio_lock_post(display, irqflags, mmio_flags);
> > @@ -360,11 +363,13 @@ __intel_de_write(struct intel_display *display, intel_reg_t reg, u32 val,
> >  {
> >  	unsigned long irqflags;
> >  
> > +	assert_display_rpm_held(display);
> > +
> >  	__mmio_wakelock_pre(display, reg, mmio_flags);
> >  	__mmio_lock_pre(display, &irqflags, mmio_flags);
> >  	__mmio_unclaimed_pre(display, reg, true, mmio_flags);
> >  
> > -	intel_uncore_write(__to_uncore(display), reg, val);
> > +	intel_de_write_fw(display, reg, val);
> >  
> >  	__mmio_unclaimed_post(display, reg, true, mmio_flags);
> >  	__mmio_lock_post(display, irqflags, mmio_flags);
> 
> -- 
> Jani Nikula, Intel

-- 
Ville Syrjälä
Intel