Re: [PATCH v2 3/5] drm/i915: Make sprite updates atomic

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Jan 17, 2014 at 08:09:04PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Add a mechanism by which we can evade the leading edge of vblank. This
> guarantees that no two sprite register writes will straddle on either
> side of the vblank start, and that means all the writes will be latched
> together in one atomic operation.
> 
> We do the vblank evade by checking the scanline counter, and if it's too
> close to the start of vblank (too close has been hardcoded to 100usec
> for now), we will wait for the vblank start to pass. In order to
> eliminate random delayes from the rest of the system, we operate with
> interrupts disabled, except when waiting for the vblank obviously.
> 
> v2: preempt_check_resched() calls after local_irq_enable() (Jesse)
>     Hook up the vblank irq stuff on BDW as well
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Two nitpicks below. Otherwise I'm ok (and I actually started to pull in
the entire series already).
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_irq.c      | 29 ++++++++++++--
>  drivers/gpu/drm/i915/intel_display.c |  2 +
>  drivers/gpu/drm/i915/intel_drv.h     |  3 ++
>  drivers/gpu/drm/i915/intel_sprite.c  | 76 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 106 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index da761a6a..888fb45 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1434,6 +1434,15 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
>  	}
>  }
>  
> +static void intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe)
> +{
> +	struct intel_crtc *intel_crtc =
> +		to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
> +
> +	intel_crtc->vbl_received = true;

vbl_received needs a comment in the header about how the access rules work
and probably some comments about barriers and stuff in the code. You
access this both from process context and irq context without locks, so
the code must come with comments explaining that all the right barriers
and access restrictions (like ACCESS_ONCE) are enforced.

> +	wake_up(&intel_crtc->vbl_wait);
> +}
> +
>  static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>  {
>  	struct drm_device *dev = (struct drm_device *) arg;
> @@ -1476,8 +1485,10 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>  		spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  
>  		for_each_pipe(pipe) {
> -			if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
> +			if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS) {
>  				drm_handle_vblank(dev, pipe);
> +				intel_pipe_handle_vblank(dev, pipe);
> +			}
>  
>  			if (pipe_stats[pipe] & PLANE_FLIPDONE_INT_STATUS_VLV) {
>  				intel_prepare_page_flip(dev, pipe);
> @@ -1676,8 +1687,10 @@ static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir)
>  		DRM_ERROR("Poison interrupt\n");
>  
>  	for_each_pipe(pipe) {
> -		if (de_iir & DE_PIPE_VBLANK(pipe))
> +		if (de_iir & DE_PIPE_VBLANK(pipe)) {
>  			drm_handle_vblank(dev, pipe);
> +			intel_pipe_handle_vblank(dev, pipe);
> +		}
>  
>  		if (de_iir & DE_PIPE_FIFO_UNDERRUN(pipe))
>  			if (intel_set_cpu_fifo_underrun_reporting(dev, pipe, false))
> @@ -1726,8 +1739,10 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
>  		intel_opregion_asle_intr(dev);
>  
>  	for_each_pipe(i) {
> -		if (de_iir & (DE_PIPE_VBLANK_IVB(i)))
> +		if (de_iir & (DE_PIPE_VBLANK_IVB(i))) {
>  			drm_handle_vblank(dev, i);
> +			intel_pipe_handle_vblank(dev, i);
> +		}
>  
>  		/* plane/pipes map 1:1 on ilk+ */
>  		if (de_iir & DE_PLANE_FLIP_DONE_IVB(i)) {
> @@ -1873,8 +1888,10 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
>  			continue;
>  
>  		pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
> -		if (pipe_iir & GEN8_PIPE_VBLANK)
> +		if (pipe_iir & GEN8_PIPE_VBLANK) {
>  			drm_handle_vblank(dev, pipe);
> +			intel_pipe_handle_vblank(dev, pipe);
> +		}
>  
>  		if (pipe_iir & GEN8_PIPE_FLIP_DONE) {
>  			intel_prepare_page_flip(dev, pipe);
> @@ -3172,6 +3189,8 @@ static bool i8xx_handle_vblank(struct drm_device *dev,
>  	if (!drm_handle_vblank(dev, pipe))
>  		return false;
>  
> +	intel_pipe_handle_vblank(dev, pipe);
> +
>  	if ((iir & flip_pending) == 0)
>  		return false;
>  
> @@ -3359,6 +3378,8 @@ static bool i915_handle_vblank(struct drm_device *dev,
>  	if (!drm_handle_vblank(dev, pipe))
>  		return false;
>  
> +	intel_pipe_handle_vblank(dev, pipe);
> +
>  	if ((iir & flip_pending) == 0)
>  		return false;
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 15f55e8..31d1d4d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10297,6 +10297,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  		intel_crtc->plane = !pipe;
>  	}
>  
> +	init_waitqueue_head(&intel_crtc->vbl_wait);
> +
>  	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
>  	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
>  	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 6df4b69..ff97ea4 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -376,6 +376,9 @@ struct intel_crtc {
>  		/* watermarks currently being used  */
>  		struct intel_pipe_wm active;
>  	} wm;
> +
> +	wait_queue_head_t vbl_wait;
> +	bool vbl_received;
>  };
>  
>  struct intel_plane_wm_parameters {
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index ed9fa7c..198a056 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -37,6 +37,58 @@
>  #include <drm/i915_drm.h>
>  #include "i915_drv.h"
>  
> +static unsigned int usecs_to_scanlines(const struct drm_display_mode *mode, unsigned int usecs)
> +{
> +	/* paranoia */
> +	if (!mode->crtc_htotal)
> +		return 1;
> +
> +	return DIV_ROUND_UP(usecs * mode->crtc_clock, 1000 * mode->crtc_htotal);
> +}
> +
> +static void intel_pipe_update_start(struct drm_crtc *crtc)

Since this is an internal helper in our driver I prefer struct intel_crtc
*crtc as the parameter. Long term (and we're slowly getting there) this
should lead to tigther code and less downcasting all over the place - the
usual foo and intel_foo duo is a bit annoying ;-)


> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	const struct drm_display_mode *mode = &intel_crtc->config.adjusted_mode;
> +	enum pipe pipe = intel_crtc->pipe;
> +	/* FIXME needs to be calibrated sensibly */
> +	unsigned int min = mode->crtc_vblank_start - usecs_to_scanlines(mode, 100);
> +	unsigned int max = mode->crtc_vblank_start - 1;
> +	long timeout = msecs_to_jiffies_timeout(1);
> +	unsigned int scanline;
> +
> +	if (WARN_ON(drm_vblank_get(dev, pipe)))
> +		return;
> +
> +	local_irq_disable();
> +
> +	intel_crtc->vbl_received = false;
> +	scanline = intel_get_crtc_scanline(crtc);
> +
> +	while (scanline >= min && scanline <= max && timeout > 0) {
> +		local_irq_enable();
> +		preempt_check_resched();
> +
> +		timeout = wait_event_timeout(intel_crtc->vbl_wait,
> +					     intel_crtc->vbl_received,
> +					     timeout);
> +
> +		local_irq_disable();
> +
> +		intel_crtc->vbl_received = false;
> +		scanline = intel_get_crtc_scanline(crtc);
> +	}
> +
> +	drm_vblank_put(dev, pipe);
> +}
> +
> +static void intel_pipe_update_end(struct drm_crtc *crtc)
> +{
> +	local_irq_enable();
> +	preempt_check_resched();
> +}
> +
>  static void
>  vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>  		 struct drm_framebuffer *fb,
> @@ -131,6 +183,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>  							fb->pitches[0]);
>  	linear_offset -= sprsurf_offset;
>  
> +	intel_pipe_update_start(crtc);
> +
>  	I915_WRITE(SPSTRIDE(pipe, plane), fb->pitches[0]);
>  	I915_WRITE(SPPOS(pipe, plane), (crtc_y << 16) | crtc_x);
>  
> @@ -144,6 +198,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>  	I915_MODIFY_DISPBASE(SPSURF(pipe, plane), i915_gem_obj_ggtt_offset(obj) +
>  			     sprsurf_offset);
>  	POSTING_READ(SPSURF(pipe, plane));
> +
> +	intel_pipe_update_end(crtc);
>  }
>  
>  static void
> @@ -155,12 +211,16 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
>  	int pipe = intel_plane->pipe;
>  	int plane = intel_plane->plane;
>  
> +	intel_pipe_update_start(crtc);
> +
>  	I915_WRITE(SPCNTR(pipe, plane), I915_READ(SPCNTR(pipe, plane)) &
>  		   ~SP_ENABLE);
>  	/* Activate double buffered register update */
>  	I915_MODIFY_DISPBASE(SPSURF(pipe, plane), 0);
>  	POSTING_READ(SPSURF(pipe, plane));
>  
> +	intel_pipe_update_end(crtc);
> +
>  	intel_update_sprite_watermarks(dplane, crtc, 0, 0, false, false);
>  }
>  
> @@ -299,6 +359,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  					       pixel_size, fb->pitches[0]);
>  	linear_offset -= sprsurf_offset;
>  
> +	intel_pipe_update_start(crtc);
> +
>  	I915_WRITE(SPRSTRIDE(pipe), fb->pitches[0]);
>  	I915_WRITE(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
>  
> @@ -318,6 +380,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	I915_MODIFY_DISPBASE(SPRSURF(pipe),
>  			     i915_gem_obj_ggtt_offset(obj) + sprsurf_offset);
>  	POSTING_READ(SPRSURF(pipe));
> +
> +	intel_pipe_update_end(crtc);
>  }
>  
>  static void
> @@ -328,6 +392,8 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	int pipe = intel_plane->pipe;
>  
> +	intel_pipe_update_start(crtc);
> +
>  	I915_WRITE(SPRCTL(pipe), I915_READ(SPRCTL(pipe)) & ~SPRITE_ENABLE);
>  	/* Can't leave the scaler enabled... */
>  	if (intel_plane->can_scale)
> @@ -336,6 +402,8 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>  	I915_MODIFY_DISPBASE(SPRSURF(pipe), 0);
>  	POSTING_READ(SPRSURF(pipe));
>  
> +	intel_pipe_update_end(crtc);
> +
>  	/*
>  	 * Avoid underruns when disabling the sprite.
>  	 * FIXME remove once watermark updates are done properly.
> @@ -478,6 +546,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  					       pixel_size, fb->pitches[0]);
>  	linear_offset -= dvssurf_offset;
>  
> +	intel_pipe_update_start(crtc);
> +
>  	I915_WRITE(DVSSTRIDE(pipe), fb->pitches[0]);
>  	I915_WRITE(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
>  
> @@ -492,6 +562,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	I915_MODIFY_DISPBASE(DVSSURF(pipe),
>  			     i915_gem_obj_ggtt_offset(obj) + dvssurf_offset);
>  	POSTING_READ(DVSSURF(pipe));
> +
> +	intel_pipe_update_end(crtc);
>  }
>  
>  static void
> @@ -502,6 +574,8 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
>  	int pipe = intel_plane->pipe;
>  
> +	intel_pipe_update_start(crtc);
> +
>  	I915_WRITE(DVSCNTR(pipe), I915_READ(DVSCNTR(pipe)) & ~DVS_ENABLE);
>  	/* Disable the scaler */
>  	I915_WRITE(DVSSCALE(pipe), 0);
> @@ -509,6 +583,8 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>  	I915_MODIFY_DISPBASE(DVSSURF(pipe), 0);
>  	POSTING_READ(DVSSURF(pipe));
>  
> +	intel_pipe_update_end(crtc);
> +
>  	/*
>  	 * Avoid underruns when disabling the sprite.
>  	 * FIXME remove once watermark updates are done properly.
> -- 
> 1.8.3.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch