Re: [PATCH 07/10] drm/i915/rps: call RPS functions via the parent interface

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

On Fri, Nov 14, 2025 at 12:26:46PM +0200, Jani Nikula wrote:
> Add struct intel_display_rps_interface to the display parent interface,
> and call the RPS functions through it. The RPS interface is optional.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  .../gpu/drm/i915/display/intel_display_rps.c  | 32 ++++++++---------
>  drivers/gpu/drm/i915/display/intel_parent.c   | 23 +++++++++++++
>  drivers/gpu/drm/i915/display/intel_parent.h   |  6 ++++
>  drivers/gpu/drm/i915/gt/intel_rps.c           | 34 +++++++++++++++++++
>  drivers/gpu/drm/i915/gt/intel_rps.h           |  2 ++
>  drivers/gpu/drm/i915/i915_driver.c            |  2 ++
>  include/drm/intel/display_parent_interface.h  | 10 ++++++
>  7 files changed, 93 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_rps.c b/drivers/gpu/drm/i915/display/intel_display_rps.c
> index b6720f7c09d9..e70c4f0eab80 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_rps.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_rps.c
> @@ -3,16 +3,18 @@
>   * Copyright © 2023 Intel Corporation
>   */
>  
> +#include <linux/dma-fence.h>
> +
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_vblank.h>
>  
> -#include "gt/intel_rps.h"
> -#include "i915_drv.h"
>  #include "i915_reg.h"
> +#include "i915_request.h"
>  #include "intel_display_core.h"
>  #include "intel_display_irq.h"
>  #include "intel_display_rps.h"
>  #include "intel_display_types.h"
> +#include "intel_parent.h"
>  
>  struct wait_rps_boost {
>  	struct wait_queue_entry wait;
> @@ -25,15 +27,10 @@ static int do_rps_boost(struct wait_queue_entry *_wait,
>  			unsigned mode, int sync, void *key)
>  {
>  	struct wait_rps_boost *wait = container_of(_wait, typeof(*wait), wait);
> -	struct i915_request *rq = to_request(wait->fence);
> -
> -	/*
> -	 * If we missed the vblank, but the request is already running it
> -	 * is reasonable to assume that it will complete before the next
> -	 * vblank without our intervention, so leave RPS alone.
> -	 */
> -	if (!i915_request_started(rq))
> -		intel_rps_boost(rq);
> +	struct intel_display *display = to_intel_display(wait->crtc->dev);
> +
> +	intel_parent_rps_boost(display, wait->fence);
> +
>  	dma_fence_put(wait->fence);
>  
>  	drm_crtc_vblank_put(wait->crtc);
> @@ -49,6 +46,9 @@ void intel_display_rps_boost_after_vblank(struct drm_crtc *crtc,
>  	struct intel_display *display = to_intel_display(crtc->dev);
>  	struct wait_rps_boost *wait;
>  
> +	if (!intel_parent_rps_available(display))
> +		return;
> +
>  	if (!dma_fence_is_i915(fence))
>  		return;
>  
> @@ -77,12 +77,14 @@ void intel_display_rps_mark_interactive(struct intel_display *display,
>  					struct intel_atomic_state *state,
>  					bool interactive)
>  {
> -	struct drm_i915_private *i915 = to_i915(display->drm);
> +	if (!intel_parent_rps_available(display))
> +		return;
>  
>  	if (state->rps_interactive == interactive)
>  		return;
>  
> -	intel_rps_mark_interactive(&to_gt(i915)->rps, interactive);
> +	intel_parent_rps_mark_interactive(display, interactive);
> +
>  	state->rps_interactive = interactive;
>  }
>  
> @@ -102,7 +104,5 @@ void ilk_display_rps_disable(struct intel_display *display)
>  
>  void ilk_display_rps_irq_handler(struct intel_display *display)
>  {
> -	struct drm_i915_private *i915 = to_i915(display->drm);
> -
> -	gen5_rps_irq_handler(&to_gt(i915)->rps);
> +	intel_parent_rps_ilk_irq_handler(display);
>  }
> diff --git a/drivers/gpu/drm/i915/display/intel_parent.c b/drivers/gpu/drm/i915/display/intel_parent.c
> index 3dd31852e2e1..9370da9d215c 100644
> --- a/drivers/gpu/drm/i915/display/intel_parent.c
> +++ b/drivers/gpu/drm/i915/display/intel_parent.c
> @@ -32,6 +32,29 @@ void intel_parent_irq_synchronize(struct intel_display *display)
>  	display->parent->irq->synchronize(display->drm);
>  }
>  
> +bool intel_parent_rps_available(struct intel_display *display)
> +{
> +	return display->parent->rps;
> +}
> +
> +void intel_parent_rps_boost(struct intel_display *display, struct dma_fence *fence)
> +{
> +	if (display->parent->rps)
> +		display->parent->rps->boost(fence);
> +}
> +
> +void intel_parent_rps_mark_interactive(struct intel_display *display, bool interactive)
> +{
> +	if (display->parent->rps)
> +		display->parent->rps->mark_interactive(display->drm, interactive);
> +}
> +
> +void intel_parent_rps_ilk_irq_handler(struct intel_display *display)
> +{
> +	if (display->parent->rps)
> +		display->parent->rps->ilk_irq_handler(display->drm);
> +}
> +
>  bool intel_parent_vgpu_active(struct intel_display *display)
>  {
>  	return display->parent->vgpu_active && display->parent->vgpu_active(display->drm);
> diff --git a/drivers/gpu/drm/i915/display/intel_parent.h b/drivers/gpu/drm/i915/display/intel_parent.h
> index fc6799db0361..41d6943786fb 100644
> --- a/drivers/gpu/drm/i915/display/intel_parent.h
> +++ b/drivers/gpu/drm/i915/display/intel_parent.h
> @@ -6,11 +6,17 @@
>  
>  #include <linux/types.h>
>  
> +struct dma_fence;
>  struct intel_display;
>  
>  bool intel_parent_irq_enabled(struct intel_display *display);
>  void intel_parent_irq_synchronize(struct intel_display *display);
>  
> +bool intel_parent_rps_available(struct intel_display *display);
> +void intel_parent_rps_boost(struct intel_display *display, struct dma_fence *fence);
> +void intel_parent_rps_mark_interactive(struct intel_display *display, bool interactive);
> +void intel_parent_rps_ilk_irq_handler(struct intel_display *display);
> +
>  bool intel_parent_vgpu_active(struct intel_display *display);
>  
>  bool intel_parent_fence_support_legacy(struct intel_display *display);
> diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c
> index b01c837ab646..61d746bda462 100644
> --- a/drivers/gpu/drm/i915/gt/intel_rps.c
> +++ b/drivers/gpu/drm/i915/gt/intel_rps.c
> @@ -6,6 +6,7 @@
>  #include <linux/string_helpers.h>
>  
>  #include <drm/intel/i915_drm.h>
> +#include <drm/intel/display_parent_interface.h>
>  
>  #include "display/intel_display_rps.h"
>  #include "display/vlv_clock.h"
> @@ -2914,6 +2915,39 @@ bool i915_gpu_turbo_disable(void)
>  }
>  EXPORT_SYMBOL_GPL(i915_gpu_turbo_disable);
>  
> +static void boost(struct dma_fence *fence)
> +{
> +	struct i915_request *rq = to_request(fence);
> +
> +	/*
> +	 * If we missed the vblank, but the request is already running it
> +	 * is reasonable to assume that it will complete before the next
> +	 * vblank without our intervention, so leave RPS alone.
> +	 */

Hmm. The comment is now rather detached from the actual vblank
related details. So I'd kinda prefer to keep the comment in the
caller, but then the request started vs. not started stuff gets
confusing. Argh.

Maybe
a) comment in caller and make this .boost_if_not_started()
b) comment here and make this .boost_after_missed_vblank()
?

Dunno.

> +	if (!i915_request_started(rq))
> +		intel_rps_boost(rq);
> +}
> +
> +static void mark_interactive(struct drm_device *drm, bool interactive)
> +{
> +	struct drm_i915_private *i915 = to_i915(drm);
> +
> +	intel_rps_mark_interactive(&to_gt(i915)->rps, interactive);
> +}
> +
> +static void ilk_irq_handler(struct drm_device *drm)
> +{
> +	struct drm_i915_private *i915 = to_i915(drm);
> +
> +	gen5_rps_irq_handler(&to_gt(i915)->rps);
> +}
> +
> +const struct intel_display_rps_interface i915_display_rps_interface = {
> +	.boost = boost,
> +	.mark_interactive = mark_interactive,
> +	.ilk_irq_handler = ilk_irq_handler,
> +};
> +
>  #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
>  #include "selftest_rps.c"
>  #include "selftest_slpc.c"
> diff --git a/drivers/gpu/drm/i915/gt/intel_rps.h b/drivers/gpu/drm/i915/gt/intel_rps.h
> index 92fb01f5a452..5dbcebd7d4a5 100644
> --- a/drivers/gpu/drm/i915/gt/intel_rps.h
> +++ b/drivers/gpu/drm/i915/gt/intel_rps.h
> @@ -128,4 +128,6 @@ static inline void intel_rps_clear_timer(struct intel_rps *rps)
>  	clear_bit(INTEL_RPS_TIMER, &rps->flags);
>  }
>  
> +extern const struct intel_display_rps_interface i915_display_rps_interface;
> +
>  #endif /* INTEL_RPS_H */
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index 814b430de960..ac189b90f985 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -81,6 +81,7 @@
>  #include "gt/intel_gt_pm.h"
>  #include "gt/intel_gt_print.h"
>  #include "gt/intel_rc6.h"
> +#include "gt/intel_rps.h"
>  
>  #include "pxp/intel_pxp.h"
>  #include "pxp/intel_pxp_debugfs.h"
> @@ -752,6 +753,7 @@ static bool fence_support_legacy(struct drm_device *drm)
>  static const struct intel_display_parent_interface parent = {
>  	.rpm = &i915_display_rpm_interface,
>  	.irq = &i915_display_irq_interface,
> +	.rps = &i915_display_rps_interface,
>  	.vgpu_active = vgpu_active,
>  	.fence_support_legacy = fence_support_legacy,
>  };
> diff --git a/include/drm/intel/display_parent_interface.h b/include/drm/intel/display_parent_interface.h
> index 11767adb0083..2ea68a31224d 100644
> --- a/include/drm/intel/display_parent_interface.h
> +++ b/include/drm/intel/display_parent_interface.h
> @@ -6,6 +6,7 @@
>  
>  #include <linux/types.h>
>  
> +struct dma_fence;
>  struct drm_device;
>  struct ref_tracker;
>  
> @@ -30,6 +31,12 @@ struct intel_display_irq_interface {
>  	void (*synchronize)(struct drm_device *drm);
>  };
>  
> +struct intel_display_rps_interface {
> +	void (*boost)(struct dma_fence *fence);
> +	void (*mark_interactive)(struct drm_device *drm, bool interactive);
> +	void (*ilk_irq_handler)(struct drm_device *drm);
> +};
> +
>  /**
>   * struct intel_display_parent_interface - services parent driver provides to display
>   *
> @@ -49,6 +56,9 @@ struct intel_display_parent_interface {
>  	/** @irq: IRQ interface */
>  	const struct intel_display_irq_interface *irq;
>  
> +	/** @rpm: RPS interface. Optional. */
> +	const struct intel_display_rps_interface *rps;
> +
>  	/** @vgpu_active: Is vGPU active? Optional. */
>  	bool (*vgpu_active)(struct drm_device *drm);
>  
> -- 
> 2.47.3

-- 
Ville Syrjälä
Intel