Re: [RFC] drm/i915: Use DOUBLE_BUFFER_CTL instead of vblank evasion for GEN9+.

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

On Thu, Feb 08, 2018 at 09:55:38AM +0100, Maarten Lankhorst wrote:
> References: https://bugs.freedesktop.org/show_bug.cgi?id=104975
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  2 ++
>  drivers/gpu/drm/i915/i915_reg.h      |  3 +++
>  drivers/gpu/drm/i915/intel_display.c |  1 +
>  drivers/gpu/drm/i915/intel_sprite.c  | 16 ++++++++++++++++
>  4 files changed, 22 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index d5e695da2fc4..11251e0107f4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1967,6 +1967,8 @@ struct drm_i915_private {
>  	/* Display functions */
>  	struct drm_i915_display_funcs display;
>  
> +	spinlock_t display_evasion_lock;
> +
>  	/* PCH chipset type */
>  	enum intel_pch pch_type;
>  	unsigned short pch_id;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 93f6ec2ea8f2..20af56644844 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6594,6 +6594,9 @@ enum {
>  #define  DIGITAL_PORTA_HOTPLUG_SHORT_DETECT	(1 << 0)
>  #define  DIGITAL_PORTA_HOTPLUG_LONG_DETECT	(2 << 0)
>  
> +#define DOUBLE_BUFFER_CTL	_MMIO(0x44500)
> +#define  DOUBLE_BUFFER_CTL_GLOBAL_DISABLE	(1 << 0)
> +
>  /* refresh rate hardware control */
>  #define RR_HW_CTL       _MMIO(0x45300)
>  #define  RR_HW_LOW_POWER_FRAMES_MASK    0xff
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 707406d1bf57..81087722bc49 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14476,6 +14476,7 @@ int intel_modeset_init(struct drm_device *dev)
>  	dev_priv->modeset_wq = alloc_ordered_workqueue("i915_modeset", 0);
>  
>  	drm_mode_config_init(dev);
> +	spin_lock_init(&dev_priv->display_evasion_lock);
>  
>  	dev->mode_config.min_width = 0;
>  	dev->mode_config.min_height = 0;
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index ac0a4f9c1954..4d1e9b166d57 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -98,6 +98,13 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
>  		intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI);
>  	DEFINE_WAIT(wait);
>  
> +	if (INTEL_GEN(dev_priv) >= 9) {
> +		spin_lock_irq(&dev_priv->display_evasion_lock);
> +		I915_WRITE(DOUBLE_BUFFER_CTL, DOUBLE_BUFFER_CTL_GLOBAL_DISABLE);
> +		scanline = intel_get_crtc_scanline(crtc);
> +		goto done;
> +	}

I think we still want to do the vblank evasion. It gives us more accurate
infromation on which frame the operation will complete. If we don't do
it we may end up signalling the completion one frame late. Although I
suppose it doesn't matter too much from the user POV since in each case
we'd end up dropping a frame. Maybe for av sync etc. it might matter
in some cases.

It would become even more important if we allowed flips to be overridden
because then we'd really need to know whether the previous flip happened
at all or not (so that we could signal fences and whatnot correctly to
keep userspace informed on which framebuffer is actually being scanned
out). And we might end up holding the lock across every vblank start,
thus preventing the display from updating at all.

So I think we should use DOUBLE_BUFFER_CTL just make missing the
deadline bit less dangerous in the presence of fragile hardware. I do
think we should still try to optimize plane/pipe updates more to reduce
the chances of that happening in general.

Also IIRC there are some "(dis)allow double buffer disable" bits in various
hardware blocks which have to set correctly for this to work. Did you check
whether we're setting them appropriately?

> +
>  	vblank_start = adjusted_mode->crtc_vblank_start;
>  	if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE)
>  		vblank_start = DIV_ROUND_UP(vblank_start, 2);
> @@ -166,6 +173,7 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
>  	while (need_vlv_dsi_wa && scanline == vblank_start)
>  		scanline = intel_get_crtc_scanline(crtc);
>  
> +done:
>  	crtc->debug.scanline_start = scanline;
>  	crtc->debug.start_vbl_time = ktime_get();
>  	crtc->debug.start_vbl_count = intel_crtc_get_vblank_counter(crtc);
> @@ -192,6 +200,9 @@ void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state)
>  
>  	trace_i915_pipe_update_end(crtc, end_vbl_count, scanline_end);
>  
> +	if (INTEL_GEN(dev_priv) >= 9)
> +		I915_WRITE(DOUBLE_BUFFER_CTL, 0);
> +
>  	/* We're still in the vblank-evade critical section, this can't race.
>  	 * Would be slightly nice to just grab the vblank count and arm the
>  	 * event outside of the critical section - the spinlock might spin for a
> @@ -206,6 +217,11 @@ void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state)
>  		new_crtc_state->base.event = NULL;
>  	}
>  
> +	if (INTEL_GEN(dev_priv) >= 9) {
> +		spin_unlock_irq(&dev_priv->display_evasion_lock);
> +		return;
> +	}
> +
>  	local_irq_enable();
>  
>  	if (intel_vgpu_active(dev_priv))
> -- 
> 2.16.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx