Re: [PATCH 10/10] drm/i915/display: Do not take uncore lock in i915_get_vblank_counter

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

On Thu, Jul 02, 2026 at 09:21:53AM +0200, Maarten Lankhorst wrote:
> This fixes a lockdep splat that occurs in the code that should be run
> with interrupts disabled. The uncore and DMC locks should not be taken
> and released repeatedly in a timing sensitive path.
> 
> Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
> ---
>  drivers/gpu/drm/i915/display/intel_de.h       |  8 ++++++
>  drivers/gpu/drm/i915/display/intel_vblank.c   |  4 +--
>  drivers/gpu/drm/i915/intel_uncore.h           | 26 +++++++++++++------
>  .../drm/xe/compat-i915-headers/intel_uncore.h |  7 +++++
>  4 files changed, 35 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_de.h b/drivers/gpu/drm/i915/display/intel_de.h
> index 1029790194296..497a308322847 100644
> --- a/drivers/gpu/drm/i915/display/intel_de.h
> +++ b/drivers/gpu/drm/i915/display/intel_de.h
> @@ -65,6 +65,14 @@ intel_de_read64_2x32(struct intel_display *display, intel_reg_t reg)
>  	return (u64)upper << 32 | lower;
>  }
>  
> +static inline u64
> +intel_de_read64_2x32_fw(struct intel_display *display,
> +			i915_reg_t lower_reg, i915_reg_t upper_reg)
> +{
> +	return intel_uncore_read64_2x32_fw(__to_uncore(display),
> +					   lower_reg, upper_reg);
> +}
> +
>  static inline void
>  intel_de_posting_read(struct intel_display *display, intel_reg_t reg)
>  {
> diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
> index c0cc0a4c25dbe..5ca22899055d7 100644
> --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> @@ -109,8 +109,8 @@ u32 i915_get_vblank_counter(struct drm_crtc *crtc)
>  	 * we get a low value that's stable across two reads of the high
>  	 * register.
>  	 */
> -	frame = intel_de_read64_2x32_volatile(display, PIPEFRAMEPIXEL(display, pipe),
> -					      PIPEFRAME(display, pipe));
> +	frame = intel_de_read64_2x32_fw(display, PIPEFRAMEPIXEL(display, pipe),
> +				        PIPEFRAME(display, pipe));

This is the only user of intel_de_read64_2x32_volatile() and it
doesn't need any locking/wakelocks/etc so we should just convert
intel_de_read64_2x32_volatile() into intel_de_read64_2x32_volatile_fw().
And we should just skip the uncore dependency (so that we'll have one 
less thing to worry about later) so I'd just implement it directly in
intel_de.[ch] in terms of intel_de_read_fw().

>  
>  	pixel = frame & PIPE_PIXEL_MASK;
>  	frame = (frame >> PIPE_FRAME_LOW_SHIFT) & 0xffffff;
> diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
> index fafc2ca9a2376..507398a562649 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.h
> +++ b/drivers/gpu/drm/i915/intel_uncore.h
> @@ -449,13 +449,28 @@ static inline void intel_uncore_rmw_fw(struct intel_uncore *uncore,
>  		intel_uncore_write_fw(uncore, reg, val);
>  }
>  
> +static inline u64
> +intel_uncore_read64_2x32_fw(struct intel_uncore *uncore,
> +			    i915_reg_t lower_reg, i915_reg_t upper_reg)
> +{
> +	u32 upper, lower, old_upper, loop = 0;
> +	upper = intel_uncore_read_fw(uncore, upper_reg);
> +	do {
> +		old_upper = upper;
> +		lower = intel_uncore_read_fw(uncore, lower_reg);
> +		upper = intel_uncore_read_fw(uncore, upper_reg);
> +	} while (upper != old_upper && loop++ < 2);
> +
> +	return (u64)upper << 32 | lower;
> +}
> +
>  static inline u64
>  intel_uncore_read64_2x32(struct intel_uncore *uncore,
>  			 i915_reg_t lower_reg, i915_reg_t upper_reg)
>  {
> -	u32 upper, lower, old_upper, loop = 0;
>  	enum forcewake_domains fw_domains;
>  	unsigned long flags;
> +	u64 ret;
>  
>  	fw_domains = intel_uncore_forcewake_for_reg(uncore, lower_reg,
>  						    FW_REG_READ);
> @@ -466,17 +481,12 @@ intel_uncore_read64_2x32(struct intel_uncore *uncore,
>  	spin_lock_irqsave(&uncore->lock, flags);
>  	intel_uncore_forcewake_get__locked(uncore, fw_domains);
>  
> -	upper = intel_uncore_read_fw(uncore, upper_reg);
> -	do {
> -		old_upper = upper;
> -		lower = intel_uncore_read_fw(uncore, lower_reg);
> -		upper = intel_uncore_read_fw(uncore, upper_reg);
> -	} while (upper != old_upper && loop++ < 2);
> +	ret = intel_uncore_read64_2x32_fw(uncore, lower_reg, upper_reg);
>  
>  	intel_uncore_forcewake_put__locked(uncore, fw_domains);
>  	spin_unlock_irqrestore(&uncore->lock, flags);
>  
> -	return (u64)upper << 32 | lower;
> +	return ret;
>  }
>  
>  static inline int intel_uncore_write_and_verify(struct intel_uncore *uncore,
> 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 08d7ab9336725..764bc94044537 100644
> --- a/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h
> +++ b/drivers/gpu/drm/xe/compat-i915-headers/intel_uncore.h
> @@ -74,6 +74,13 @@ intel_uncore_read64_2x32(struct intel_uncore *uncore,
>  	return (u64)upper << 32 | lower;
>  }
>  
> +static inline u64
> +intel_uncore_read64_2x32_fw(struct intel_uncore *uncore,
> +			    i915_reg_t i915_lower_reg, i915_reg_t i915_upper_reg)
> +{
> +	return intel_uncore_read64_2x32(uncore, i915_lower_reg, i915_upper_reg);
> +}
> +
>  static inline void intel_uncore_posting_read(struct intel_uncore *uncore,
>  					     i915_reg_t i915_reg)
>  {
> -- 
> 2.53.0

-- 
Ville Syrjälä
Intel