Re: [PATCH] drm/xe/display: Add intel_plane_initial_vblank_wait

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Hey,

Den 2024-11-07 kl. 13:21, skrev Ville Syrjälä:
> On Thu, Nov 07, 2024 at 12:36:17PM +0100, Maarten Lankhorst wrote:
>> We're changing the driver to have no interrupts during early init for
>> Xe, so we poll the PIPE_FRMSTMSMP counter instead.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Link: https://patchwork.freedesktop.org/patch/msgid/20241105121857.17389-2-maarten.lankhorst@linux.intel.com
>> Signed-off-by: Maarten Lankhorst,,, <dev@lankhorst.se>
>> ---
>>  drivers/gpu/drm/i915/display/intel_display.c  |  6 +++---
>>  .../drm/i915/display/intel_plane_initial.c    |  9 ++++++++-
>>  .../drm/i915/display/intel_plane_initial.h    |  2 ++
>>  drivers/gpu/drm/xe/display/xe_plane_initial.c | 19 ++++++++++++++++++-
>>  4 files changed, 31 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index d7f92dc56d1e9..7bd6da2bb36f7 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -795,7 +795,7 @@ void intel_plane_disable_noatomic(struct intel_crtc *crtc,
>>  	if ((crtc_state->active_planes & ~BIT(PLANE_CURSOR)) == 0 &&
>>  	    hsw_ips_disable(crtc_state)) {
>>  		crtc_state->ips_enabled = false;
>> -		intel_crtc_wait_for_next_vblank(crtc);
>> +		intel_plane_initial_vblank_wait(plane);
>>  	}
>>  
>>  	/*
>> @@ -809,7 +809,7 @@ void intel_plane_disable_noatomic(struct intel_crtc *crtc,
>>  	 */
>>  	if (HAS_GMCH(dev_priv) &&
>>  	    intel_set_memory_cxsr(dev_priv, false))
>> -		intel_crtc_wait_for_next_vblank(crtc);
>> +		intel_plane_initial_vblank_wait(plane);
>>  
>>  	/*
>>  	 * Gen2 reports pipe underruns whenever all planes are disabled.
>> @@ -819,7 +819,7 @@ void intel_plane_disable_noatomic(struct intel_crtc *crtc,
>>  		intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false);
>>  
>>  	intel_plane_disable_arm(NULL, plane, crtc_state);
>> -	intel_crtc_wait_for_next_vblank(crtc);
>> +	intel_plane_initial_vblank_wait(plane);
>>  }
>>  
>>  unsigned int
>> diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c b/drivers/gpu/drm/i915/display/intel_plane_initial.c
>> index 62401f6a04e4f..77eb2b763be5e 100644
>> --- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
>> +++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
>> @@ -14,6 +14,13 @@
>>  #include "intel_frontbuffer.h"
>>  #include "intel_plane_initial.h"
>>  
>> +void intel_plane_initial_vblank_wait(struct intel_plane *plane)
>> +{
>> +	struct intel_crtc *crtc = intel_crtc_for_pipe(to_intel_display(plane->base.dev), plane->pipe);
>> +
>> +	intel_crtc_wait_for_next_vblank(crtc);
>> +}
>> +
>>  static bool
>>  intel_reuse_initial_plane_obj(struct intel_crtc *this,
>>  			      const struct intel_initial_plane_config plane_configs[],
>> @@ -438,7 +445,7 @@ void intel_initial_plane_config(struct drm_i915_private *i915)
>>  		intel_find_initial_plane_obj(crtc, plane_configs);
>>  
>>  		if (i915->display.funcs.display->fixup_initial_plane_config(crtc, plane_config))
>> -			intel_crtc_wait_for_next_vblank(crtc);
>> +			intel_plane_initial_vblank_wait(to_intel_plane(crtc->base.primary));
>>  
>>  		plane_config_fini(plane_config);
>>  	}
>> diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.h b/drivers/gpu/drm/i915/display/intel_plane_initial.h
>> index 64ab95239cd45..149b1bf1bc1f8 100644
>> --- a/drivers/gpu/drm/i915/display/intel_plane_initial.h
>> +++ b/drivers/gpu/drm/i915/display/intel_plane_initial.h
>> @@ -7,7 +7,9 @@
>>  #define __INTEL_PLANE_INITIAL_H__
>>  
>>  struct drm_i915_private;
>> +struct intel_plane;
>>  
>> +void intel_plane_initial_vblank_wait(struct intel_plane *plane);
>>  void intel_initial_plane_config(struct drm_i915_private *i915);
>>  
>>  #endif
>> diff --git a/drivers/gpu/drm/xe/display/xe_plane_initial.c b/drivers/gpu/drm/xe/display/xe_plane_initial.c
>> index 668f544a7ac80..9252cfeef0a1f 100644
>> --- a/drivers/gpu/drm/xe/display/xe_plane_initial.c
>> +++ b/drivers/gpu/drm/xe/display/xe_plane_initial.c
>> @@ -8,7 +8,9 @@
>>  
>>  #include "regs/xe_gtt_defs.h"
>>  #include "xe_ggtt.h"
>> +#include "xe_mmio.h"
>>  
>> +#include "i915_reg.h"
>>  #include "intel_atomic_plane.h"
>>  #include "intel_crtc.h"
>>  #include "intel_display.h"
>> @@ -22,6 +24,21 @@
>>  
>>  #include <generated/xe_wa_oob.h>
>>  
>> +void intel_plane_initial_vblank_wait(struct intel_plane *plane)
>> +{
>> +	/* Early xe has no irq */
>> +	struct xe_device *xe = to_xe_device(plane->base.dev);
>> +	struct xe_reg pipe_frmtmstmp = XE_REG(i915_mmio_reg_offset(PIPE_FRMTMSTMP(plane->pipe)));
>> +	u32 timestamp;
>> +	int ret;
>> +
>> +	timestamp = xe_mmio_read32(xe_root_tile_mmio(xe), pipe_frmtmstmp);
>> +
>> +	ret = xe_mmio_wait32_not(xe_root_tile_mmio(xe), pipe_frmtmstmp, ~0U, timestamp, 40000U, &timestamp, false);
>> +	if (ret < 0)
>> +		drm_warn(&xe->drm, "waiting for early vblank failed with %i\n", ret);
> 
> Why did you change to this from the earlier SURFLIVE approach?This works correctly when calling disable_plane too. When I tested on lunarlake, this gave me an issue.
When failing to construct initial fb, intel_plane_disable_noatomic() is called,
and then calls drm_crtc_wait_one_vblank if not fixed like above.

SURFLIVE works great for the specific case of flipping, not that great for disabling. :)

Cheers,
~Maarten