Re: [PATCH 4/5] drm/i915/rps: make fence priority setting part of the rps interface

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

On Fri, 07 Nov 2025, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Nov 06, 2025 at 04:43:12PM +0200, Jani Nikula wrote:
>> This is perhaps not ideal, but simplifies the interfaces, and allows us
>> to get rid of the compat header in xe.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_display_rps.c    |  2 ++
>>  drivers/gpu/drm/i915/display/intel_plane.c          |  6 +-----
>>  drivers/gpu/drm/i915/gt/intel_rps.c                 |  9 +++++++++
>>  .../xe/compat-i915-headers/gem/i915_gem_object.h    | 13 -------------
>>  include/drm/intel/display_parent_interface.h        |  1 +
>>  5 files changed, 13 insertions(+), 18 deletions(-)
>>  delete mode 100644 drivers/gpu/drm/xe/compat-i915-headers/gem/i915_gem_object.h
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_rps.c b/drivers/gpu/drm/i915/display/intel_display_rps.c
>> index 44cb9dba0c19..a2d57671c419 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_rps.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display_rps.c
>> @@ -48,6 +48,8 @@ void intel_display_rps_boost_after_vblank(struct drm_crtc *crtc,
>>  	if (!display->parent->rps)
>>  		return;
>>  
>> +	display->parent->rps->priority_display(fence);
>
> This is quite confusing now. This thing is about the scheduler
> and nothing to do with RPS boosting.
>
> If we were to mix these into one interface somehow then I don't think
> it should be called rps_something.

Yeah, I was on the, uh, fence about this.

A separate function at a higher level, because we don't seem to have
anything else to group this with? display->parent->priority_display()?

BR,
Jani.



>
> As a side note, there's also some kind of performance problem with this
> thing when using GUC submission. I first noticed the problem on DG2
> where GPU utilization was very low on simple workloads, and skipping
> this priority shuffling increases the performance considerably. Though
> even with this removed, GUC submissions is still very slow compared to
> execlists (when compared on TGL which still supports both submission
> paths). I haven't really had time to look into it in any great detail,
> but we might need to get rid of this thing for the GUC submission path.
> Though I suppose the interface changes don't really matter for that
> because the submission path checks would be on the i915 side anyway.
>
>> +
>>  	if (DISPLAY_VER(display) < 6)
>>  		return;
>>  
>> diff --git a/drivers/gpu/drm/i915/display/intel_plane.c b/drivers/gpu/drm/i915/display/intel_plane.c
>> index 505c776c0585..28ee9502b596 100644
>> --- a/drivers/gpu/drm/i915/display/intel_plane.c
>> +++ b/drivers/gpu/drm/i915/display/intel_plane.c
>> @@ -45,7 +45,6 @@
>>  #include <drm/drm_panic.h>
>>  #include <drm/drm_print.h>
>>  
>> -#include "gem/i915_gem_object.h"
>>  #include "i9xx_plane_regs.h"
>>  #include "intel_cdclk.h"
>>  #include "intel_cursor.h"
>> @@ -1176,12 +1175,9 @@ intel_prepare_plane_fb(struct drm_plane *_plane,
>>  	if (ret < 0)
>>  		goto unpin_fb;
>>  
>> -	if (new_plane_state->uapi.fence) {
>> -		i915_gem_fence_wait_priority_display(new_plane_state->uapi.fence);
>> -
>> +	if (new_plane_state->uapi.fence)
>>  		intel_display_rps_boost_after_vblank(new_plane_state->hw.crtc,
>>  						     new_plane_state->uapi.fence);
>> -	}
>>  
>>  	/*
>>  	 * We declare pageflips to be interactive and so merit a small bias
>> diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c
>> index 05b21de6c24b..1af39198e0c5 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_rps.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_rps.c
>> @@ -2915,6 +2915,14 @@ bool i915_gpu_turbo_disable(void)
>>  }
>>  EXPORT_SYMBOL_GPL(i915_gpu_turbo_disable);
>>  
>> +static void priority_display(struct dma_fence *fence)
>> +{
>> +	if (!dma_fence_is_i915(fence))
>> +		return;
>> +
>> +	i915_gem_fence_wait_priority_display(fence);
>> +}
>> +
>>  static void boost(struct dma_fence *fence)
>>  {
>>  	struct i915_request *rq;
>> @@ -2948,6 +2956,7 @@ static void ilk_irq_handler(struct drm_device *drm)
>>  }
>>  
>>  const struct intel_display_rps_interface i915_display_rps_interface = {
>> +	.priority_display = priority_display,
>>  	.boost = boost,
>>  	.mark_interactive = mark_interactive,
>>  	.ilk_irq_handler = ilk_irq_handler,
>> diff --git a/drivers/gpu/drm/xe/compat-i915-headers/gem/i915_gem_object.h b/drivers/gpu/drm/xe/compat-i915-headers/gem/i915_gem_object.h
>> deleted file mode 100644
>> index 0548b2e0316f..000000000000
>> --- a/drivers/gpu/drm/xe/compat-i915-headers/gem/i915_gem_object.h
>> +++ /dev/null
>> @@ -1,13 +0,0 @@
>> -/* SPDX-License-Identifier: MIT */
>> -/* Copyright © 2025 Intel Corporation */
>> -
>> -#ifndef __I915_GEM_OBJECT_H__
>> -#define __I915_GEM_OBJECT_H__
>> -
>> -struct dma_fence;
>> -
>> -static inline void i915_gem_fence_wait_priority_display(struct dma_fence *fence)
>> -{
>> -}
>> -
>> -#endif
>> diff --git a/include/drm/intel/display_parent_interface.h b/include/drm/intel/display_parent_interface.h
>> index 8920404545be..7614b35660c9 100644
>> --- a/include/drm/intel/display_parent_interface.h
>> +++ b/include/drm/intel/display_parent_interface.h
>> @@ -27,6 +27,7 @@ struct intel_display_rpm_interface {
>>  };
>>  
>>  struct intel_display_rps_interface {
>> +	void (*priority_display)(struct dma_fence *fence);
>>  	void (*boost)(struct dma_fence *fence);
>>  	void (*mark_interactive)(struct drm_device *drm, bool interactive);
>>  	void (*ilk_irq_handler)(struct drm_device *drm);
>> -- 
>> 2.47.3

-- 
Jani Nikula, Intel