Re: [PATCH v2 1/3] drm/i915/gmbus: Switch GMBUS fully to _fw() register accesses

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 30 Sep 2026, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Switch the gmbus code over from _notrace() to _fw() register
> accesses.
>
> My original goal was just to eliminate the _notrace() variants
> to shrink the intel_de.h API a bit, but while doing this I
> stumbled on a huge performance issue with the current code;
> Bit banging turns out to be extremely expensive on PCH
> platforms while in DC5/6, and cause turns out not to be the
> actual south GMBUS/GPIO register accesses, but rather the
> extra north FPGA_DBG register access done from the non _fw()
> register access functions. Those accesses cause a DC5/6 exit
> which take a huge chunk of time.
>
> $ time echo detect > /sys/class/drm/card0/HDMI-A-1/status
> -real	0m16.281s
> +real	0m1.459s
> ...
>
> There does still seem to be a smallish (maybe ~15%) performance
> penalty for accessing PCH registers while in DC5/6. Not really
> sure what's causing it, but seems to be small enough to not cause
> a huge performance problem for bit banging. When not in DC5/6
> the same HDMI detect cycle takes ~1.25 seconds. If we want to
> further speed that up we could completely disable DC5/6 during
> big banged i2c transfers. Currently we only do that on BXT/GLK
> because there the GMBUS hardware lives on the SoC and thus is
> affected by DC states, even for non-bit banged transfers.
>
> Note that by using the _fw() register accessors we also lose the
> uncore.lock protection, but that's fine because everything is
> already protected by the gmbus mutex, and nothing else should
> live on the same cachelines of registers. So even platforms like
> IVB/HSW which may have some issues with concurrent register
> accesses to the same cacheline are still covered.
>
> v2: Drop the accidental wait_for_fw_ms()->wait_for_ms() change
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I'm basically losing track when we're okay to use the _fw variants and
when not. Because it's not just forcewake anymore, it's also the DMC
wakelock now.

I've been complaining about pretty much the same thing with the display
PREEMPT_RT changes.

I'm sure a lot of people know exactly when it's okay, but then there's
also going to be a lot of cargo culting around _fw. :(

This does seem fine, though.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


> ---
>  drivers/gpu/drm/i915/display/intel_de.h    |  6 ++++
>  drivers/gpu/drm/i915/display/intel_gmbus.c | 32 +++++++++++-----------
>  2 files changed, 22 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_de.h b/drivers/gpu/drm/i915/display/intel_de.h
> index 102979019429..8144588d3cf7 100644
> --- a/drivers/gpu/drm/i915/display/intel_de.h
> +++ b/drivers/gpu/drm/i915/display/intel_de.h
> @@ -158,6 +158,12 @@ intel_de_rmw_fw(struct intel_display *display, intel_reg_t reg, u32 clear, u32 s
>  	return old;
>  }
>  
> +static inline void
> +intel_de_posting_read_fw(struct intel_display *display, intel_reg_t reg)
> +{
> +	intel_de_read_fw(display, reg);
> +}

Side note, I've thought about adding intel_de_write_post() to do the
posting read internally. Probably 99% of the intel_de_posting_read()
calls could be handled with that, reducing quite a bit of duplication,
especially for verbose cases like:

	intel_de_write(display, BXT_BLC_PWM_CTL(panel->backlight.controller), pwm_ctl);
	intel_de_posting_read(display, BXT_BLC_PWM_CTL(panel->backlight.controller));
->
	intel_de_write_post(display, BXT_BLC_PWM_CTL(panel->backlight.controller), pwm_ctl);


BR,
Jani.


> +
>  static inline u32
>  intel_de_read_notrace(struct intel_display *display, intel_reg_t reg)
>  {
> diff --git a/drivers/gpu/drm/i915/display/intel_gmbus.c b/drivers/gpu/drm/i915/display/intel_gmbus.c
> index 60a70dea5d85..13bc6ab6b2f6 100644
> --- a/drivers/gpu/drm/i915/display/intel_gmbus.c
> +++ b/drivers/gpu/drm/i915/display/intel_gmbus.c
> @@ -218,8 +218,8 @@ to_intel_gmbus(struct i2c_adapter *i2c)
>  void
>  intel_gmbus_reset(struct intel_display *display)
>  {
> -	intel_de_write(display, GMBUS0(display), 0);
> -	intel_de_write(display, GMBUS4(display), 0);
> +	intel_de_write_fw(display, GMBUS0(display), 0);
> +	intel_de_write_fw(display, GMBUS4(display), 0);
>  }
>  
>  static void pnv_gmbus_clock_gating(struct intel_display *display,
> @@ -262,7 +262,7 @@ static u32 get_reserved(struct intel_gmbus *bus)
>  		preserve_bits |= GPIO_CLOCK_DIR_MASK | GPIO_CLOCK_VAL_MASK |
>  				 GPIO_DATA_DIR_MASK | GPIO_DATA_VAL_MASK;
>  
> -	return intel_de_read_notrace(display, bus->gpio_reg) & preserve_bits;
> +	return intel_de_read_fw(display, bus->gpio_reg) & preserve_bits;
>  }
>  
>  static int get_clock(void *data)
> @@ -271,10 +271,10 @@ static int get_clock(void *data)
>  	struct intel_display *display = bus->display;
>  	u32 reserved = get_reserved(bus);
>  
> -	intel_de_write_notrace(display, bus->gpio_reg, reserved | GPIO_CLOCK_DIR_MASK);
> -	intel_de_write_notrace(display, bus->gpio_reg, reserved);
> +	intel_de_write_fw(display, bus->gpio_reg, reserved | GPIO_CLOCK_DIR_MASK);
> +	intel_de_write_fw(display, bus->gpio_reg, reserved);
>  
> -	return (intel_de_read_notrace(display, bus->gpio_reg) & GPIO_CLOCK_VAL_IN) != 0;
> +	return (intel_de_read_fw(display, bus->gpio_reg) & GPIO_CLOCK_VAL_IN) != 0;
>  }
>  
>  static int get_data(void *data)
> @@ -283,10 +283,10 @@ static int get_data(void *data)
>  	struct intel_display *display = bus->display;
>  	u32 reserved = get_reserved(bus);
>  
> -	intel_de_write_notrace(display, bus->gpio_reg, reserved | GPIO_DATA_DIR_MASK);
> -	intel_de_write_notrace(display, bus->gpio_reg, reserved);
> +	intel_de_write_fw(display, bus->gpio_reg, reserved | GPIO_DATA_DIR_MASK);
> +	intel_de_write_fw(display, bus->gpio_reg, reserved);
>  
> -	return (intel_de_read_notrace(display, bus->gpio_reg) & GPIO_DATA_VAL_IN) != 0;
> +	return (intel_de_read_fw(display, bus->gpio_reg) & GPIO_DATA_VAL_IN) != 0;
>  }
>  
>  static void set_clock(void *data, int state_high)
> @@ -302,8 +302,8 @@ static void set_clock(void *data, int state_high)
>  		clock_bits = GPIO_CLOCK_DIR_OUT | GPIO_CLOCK_DIR_MASK |
>  			     GPIO_CLOCK_VAL_MASK;
>  
> -	intel_de_write_notrace(display, bus->gpio_reg, reserved | clock_bits);
> -	intel_de_posting_read(display, bus->gpio_reg);
> +	intel_de_write_fw(display, bus->gpio_reg, reserved | clock_bits);
> +	intel_de_posting_read_fw(display, bus->gpio_reg);
>  }
>  
>  static void set_data(void *data, int state_high)
> @@ -319,15 +319,15 @@ static void set_data(void *data, int state_high)
>  		data_bits = GPIO_DATA_DIR_OUT | GPIO_DATA_DIR_MASK |
>  			GPIO_DATA_VAL_MASK;
>  
> -	intel_de_write_notrace(display, bus->gpio_reg, reserved | data_bits);
> -	intel_de_posting_read(display, bus->gpio_reg);
> +	intel_de_write_fw(display, bus->gpio_reg, reserved | data_bits);
> +	intel_de_posting_read_fw(display, bus->gpio_reg);
>  }
>  
>  static void
>  ptl_handle_mask_bits(struct intel_gmbus *bus, bool set)
>  {
>  	struct intel_display *display = bus->display;
> -	u32 reg_val = intel_de_read_notrace(display, bus->gpio_reg);
> +	u32 reg_val = intel_de_read_fw(display, bus->gpio_reg);
>  	u32 mask_bits = GPIO_CLOCK_DIR_MASK | GPIO_CLOCK_VAL_MASK |
>  			GPIO_DATA_DIR_MASK | GPIO_DATA_VAL_MASK;
>  	if (set)
> @@ -335,8 +335,8 @@ ptl_handle_mask_bits(struct intel_gmbus *bus, bool set)
>  	else
>  		reg_val &= ~mask_bits;
>  
> -	intel_de_write_notrace(display, bus->gpio_reg, reg_val);
> -	intel_de_posting_read(display, bus->gpio_reg);
> +	intel_de_write_fw(display, bus->gpio_reg, reg_val);
> +	intel_de_posting_read_fw(display, bus->gpio_reg);
>  }
>  
>  static int

-- 
Jani Nikula, Intel