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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri,  7 Mar 2014 15:42:43 +0200
ville.syrjala@xxxxxxxxxxxxxxx wrote:

> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> 
> 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.
> 
> Note that we now go digging through pipe_to_crtc_mapping[] in the
> vblank interrupt handler, which is a bit dangerous since we set up
> interrupts before the crtcs. However in this case since it's the vblank
> interrupt, we don't actually unmask it until some piece of code
> requests it.
> 
> v2: preempt_check_resched() calls after local_irq_enable() (Jesse)
>     Hook up the vblank irq stuff on BDW as well
> v3: Pass intel_crtc instead of drm_crtc (Daniel)
>     Warn if crtc.mutex isn't locked (Daniel)
>     Add an explicit compiler barrier and document the barriers (Daniel)
>     Note the irq vs. modeset setup madness in the commit message (Daniel)
> v4: Use prepare_to_wait() & co. directly and eliminate vbl_received
> v5: Refactor intel_pipe_handle_vblank() vs. drm_handle_vblank() (Chris)
>     Check for min/max scanline <= 0 (Chris)
>     Don't call intel_pipe_update_end() if start failed totally (Chris)
>     Check that the vblank counters match on both sides of the critical
>     section (Chris)
> v6: Fix atomic update for interlaced modes
> v7: Reorder code for better readability (Chris)
> v8: Drop preempt_check_resched(). It's not available to modules
>     anymore and isn't even needed unless we ourselves cause
>     a wakeup needing reschedule while interrupts are off
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/i915_irq.c      |  25 +++++--
>  drivers/gpu/drm/i915/intel_display.c |   2 +
>  drivers/gpu/drm/i915/intel_drv.h     |   2 +
>  drivers/gpu/drm/i915/intel_sprite.c  | 131 +++++++++++++++++++++++++++++++++++
>  4 files changed, 154 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 6ec3f8a..d35120e 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1536,6 +1536,19 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
>  	}
>  }
>  
> +static bool intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe)
> +{
> +	struct intel_crtc *crtc;
> +
> +	if (!drm_handle_vblank(dev, pipe))
> +		return false;
> +
> +	crtc = to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
> +	wake_up(&crtc->vbl_wait);
> +
> +	return true;
> +}
> +
>  static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -1587,7 +1600,7 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
>  
>  	for_each_pipe(pipe) {
>  		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_FLIP_DONE_INT_STATUS_VLV) {
>  			intel_prepare_page_flip(dev, pipe);
> @@ -1814,7 +1827,7 @@ static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir)
>  
>  	for_each_pipe(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))
> @@ -1864,7 +1877,7 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
>  
>  	for_each_pipe(pipe) {
>  		if (de_iir & (DE_PIPE_VBLANK_IVB(pipe)))
> -			drm_handle_vblank(dev, pipe);
> +			intel_pipe_handle_vblank(dev, pipe);
>  
>  		/* plane/pipes map 1:1 on ilk+ */
>  		if (de_iir & DE_PLANE_FLIP_DONE_IVB(pipe)) {
> @@ -2007,7 +2020,7 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
>  
>  		pipe_iir = I915_READ(GEN8_DE_PIPE_IIR(pipe));
>  		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);
> @@ -3284,7 +3297,7 @@ static bool i8xx_handle_vblank(struct drm_device *dev,
>  	drm_i915_private_t *dev_priv = dev->dev_private;
>  	u16 flip_pending = DISPLAY_PLANE_FLIP_PENDING(plane);
>  
> -	if (!drm_handle_vblank(dev, pipe))
> +	if (!intel_pipe_handle_vblank(dev, pipe))
>  		return false;
>  
>  	if ((iir & flip_pending) == 0)
> @@ -3469,7 +3482,7 @@ static bool i915_handle_vblank(struct drm_device *dev,
>  	drm_i915_private_t *dev_priv = dev->dev_private;
>  	u32 flip_pending = DISPLAY_PLANE_FLIP_PENDING(plane);
>  
> -	if (!drm_handle_vblank(dev, pipe))
> +	if (!intel_pipe_handle_vblank(dev, pipe))
>  		return false;
>  
>  	if ((iir & flip_pending) == 0)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7e8bfd8..3fc739c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10299,6 +10299,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 9790477..8c9892d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -384,6 +384,8 @@ struct intel_crtc {
>  		/* watermarks currently being used  */
>  		struct intel_pipe_wm active;
>  	} wm;
> +
> +	wait_queue_head_t vbl_wait;
>  };
>  
>  struct intel_plane_wm_parameters {
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 336ae6c..a3ed840 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -37,6 +37,89 @@
>  #include <drm/i915_drm.h>
>  #include "i915_drv.h"
>  
> +static int usecs_to_scanlines(const struct drm_display_mode *mode, int usecs)
> +{
> +	/* paranoia */
> +	if (!mode->crtc_htotal)
> +		return 1;
> +
> +	return DIV_ROUND_UP(usecs * mode->crtc_clock, 1000 * mode->crtc_htotal);
> +}
> +
> +static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	const struct drm_display_mode *mode = &crtc->config.adjusted_mode;
> +	enum pipe pipe = crtc->pipe;
> +	long timeout = msecs_to_jiffies_timeout(1);
> +	int scanline, min, max, vblank_start;
> +	DEFINE_WAIT(wait);
> +
> +	WARN_ON(!mutex_is_locked(&crtc->base.mutex));
> +
> +	vblank_start = mode->crtc_vblank_start;
> +	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> +		vblank_start = DIV_ROUND_UP(vblank_start, 2);
> +
> +	/* FIXME needs to be calibrated sensibly */
> +	min = vblank_start - usecs_to_scanlines(mode, 100);
> +	max = vblank_start - 1;
> +
> +	if (min <= 0 || max <= 0)
> +		return false;
> +
> +	if (WARN_ON(drm_vblank_get(dev, pipe)))
> +		return false;
> +
> +	local_irq_disable();
> +
> +	for (;;) {
> +		/*
> +		 * prepare_to_wait() has a memory barrier, which guarantees
> +		 * other CPUs can see the task state update by the time we
> +		 * read the scanline.
> +		 */
> +		prepare_to_wait(&crtc->vbl_wait, &wait, TASK_UNINTERRUPTIBLE);
> +
> +		scanline = intel_get_crtc_scanline(crtc);
> +		if (scanline < min || scanline > max)
> +			break;
> +
> +		if (timeout <= 0) {
> +			DRM_ERROR("Potential atomic update failure on pipe %c\n",
> +				  pipe_name(crtc->pipe));
> +			break;
> +		}
> +
> +		local_irq_enable();
> +
> +		timeout = schedule_timeout(timeout);
> +
> +		local_irq_disable();
> +	}
> +
> +	finish_wait(&crtc->vbl_wait, &wait);
> +
> +	drm_vblank_put(dev, pipe);
> +
> +	*start_vbl_count = dev->driver->get_vblank_counter(dev, pipe);
> +
> +	return true;
> +}
> +
> +static void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	enum pipe pipe = crtc->pipe;
> +	u32 end_vbl_count = dev->driver->get_vblank_counter(dev, pipe);
> +
> +	local_irq_enable();
> +
> +	if (start_vbl_count != end_vbl_count)
> +		DRM_ERROR("Atomic update failure on pipe %c (start=%u end=%u)\n",
> +			  pipe_name(pipe), start_vbl_count, end_vbl_count);
> +}
> +
>  static void
>  vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>  		 struct drm_framebuffer *fb,
> @@ -48,11 +131,14 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>  	struct drm_device *dev = dplane->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_plane *intel_plane = to_intel_plane(dplane);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	int pipe = intel_plane->pipe;
>  	int plane = intel_plane->plane;
>  	u32 sprctl;
>  	unsigned long sprsurf_offset, linear_offset;
>  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> +	u32 start_vbl_count;
> +	bool atomic_update;
>  
>  	sprctl = I915_READ(SPCNTR(pipe, plane));
>  
> @@ -131,6 +217,8 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>  							fb->pitches[0]);
>  	linear_offset -= sprsurf_offset;
>  
> +	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
> +
>  	I915_WRITE(SPSTRIDE(pipe, plane), fb->pitches[0]);
>  	I915_WRITE(SPPOS(pipe, plane), (crtc_y << 16) | crtc_x);
>  
> @@ -144,6 +232,9 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
>  	I915_WRITE(SPSURF(pipe, plane), i915_gem_obj_ggtt_offset(obj) +
>  		   sprsurf_offset);
>  	POSTING_READ(SPSURF(pipe, plane));
> +
> +	if (atomic_update)
> +		intel_pipe_update_end(intel_crtc, start_vbl_count);
>  }
>  
>  static void
> @@ -152,8 +243,13 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
>  	struct drm_device *dev = dplane->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_plane *intel_plane = to_intel_plane(dplane);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	int pipe = intel_plane->pipe;
>  	int plane = intel_plane->plane;
> +	u32 start_vbl_count;
> +	bool atomic_update;
> +
> +	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
>  
>  	I915_WRITE(SPCNTR(pipe, plane), I915_READ(SPCNTR(pipe, plane)) &
>  		   ~SP_ENABLE);
> @@ -161,6 +257,9 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
>  	I915_WRITE(SPSURF(pipe, plane), 0);
>  	POSTING_READ(SPSURF(pipe, plane));
>  
> +	if (atomic_update)
> +		intel_pipe_update_end(intel_crtc, start_vbl_count);
> +
>  	intel_update_sprite_watermarks(dplane, crtc, 0, 0, false, false);
>  }
>  
> @@ -226,10 +325,13 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	struct drm_device *dev = plane->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	int pipe = intel_plane->pipe;
>  	u32 sprctl, sprscale = 0;
>  	unsigned long sprsurf_offset, linear_offset;
>  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> +	u32 start_vbl_count;
> +	bool atomic_update;
>  
>  	sprctl = I915_READ(SPRCTL(pipe));
>  
> @@ -299,6 +401,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  					       pixel_size, fb->pitches[0]);
>  	linear_offset -= sprsurf_offset;
>  
> +	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
> +
>  	I915_WRITE(SPRSTRIDE(pipe), fb->pitches[0]);
>  	I915_WRITE(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
>  
> @@ -318,6 +422,9 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	I915_WRITE(SPRSURF(pipe),
>  		   i915_gem_obj_ggtt_offset(obj) + sprsurf_offset);
>  	POSTING_READ(SPRSURF(pipe));
> +
> +	if (atomic_update)
> +		intel_pipe_update_end(intel_crtc, start_vbl_count);
>  }
>  
>  static void
> @@ -326,7 +433,12 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>  	struct drm_device *dev = plane->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	int pipe = intel_plane->pipe;
> +	u32 start_vbl_count;
> +	bool atomic_update;
> +
> +	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
>  
>  	I915_WRITE(SPRCTL(pipe), I915_READ(SPRCTL(pipe)) & ~SPRITE_ENABLE);
>  	/* Can't leave the scaler enabled... */
> @@ -336,6 +448,9 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>  	I915_WRITE(SPRSURF(pipe), 0);
>  	POSTING_READ(SPRSURF(pipe));
>  
> +	if (atomic_update)
> +		intel_pipe_update_end(intel_crtc, start_vbl_count);
> +
>  	/*
>  	 * Avoid underruns when disabling the sprite.
>  	 * FIXME remove once watermark updates are done properly.
> @@ -410,10 +525,13 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	struct drm_device *dev = plane->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	int pipe = intel_plane->pipe;
>  	unsigned long dvssurf_offset, linear_offset;
>  	u32 dvscntr, dvsscale;
>  	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> +	u32 start_vbl_count;
> +	bool atomic_update;
>  
>  	dvscntr = I915_READ(DVSCNTR(pipe));
>  
> @@ -478,6 +596,8 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  					       pixel_size, fb->pitches[0]);
>  	linear_offset -= dvssurf_offset;
>  
> +	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
> +
>  	I915_WRITE(DVSSTRIDE(pipe), fb->pitches[0]);
>  	I915_WRITE(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
>  
> @@ -492,6 +612,9 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	I915_WRITE(DVSSURF(pipe),
>  		   i915_gem_obj_ggtt_offset(obj) + dvssurf_offset);
>  	POSTING_READ(DVSSURF(pipe));
> +
> +	if (atomic_update)
> +		intel_pipe_update_end(intel_crtc, start_vbl_count);
>  }
>  
>  static void
> @@ -500,7 +623,12 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>  	struct drm_device *dev = plane->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	int pipe = intel_plane->pipe;
> +	u32 start_vbl_count;
> +	bool atomic_update;
> +
> +	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
>  
>  	I915_WRITE(DVSCNTR(pipe), I915_READ(DVSCNTR(pipe)) & ~DVS_ENABLE);
>  	/* Disable the scaler */
> @@ -509,6 +637,9 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>  	I915_WRITE(DVSSURF(pipe), 0);
>  	POSTING_READ(DVSSURF(pipe));
>  
> +	if (atomic_update)
> +		intel_pipe_update_end(intel_crtc, start_vbl_count);
> +
>  	/*
>  	 * Avoid underruns when disabling the sprite.
>  	 * FIXME remove once watermark updates are done properly.

Yeah looks like this will work ok.

I don't understand the prepare_to_wait() comment, since we're both
holding the crtc mutex and prepare_to_wait() will take the crtc
vbl_wait queue lock, but since things look safe as-is I guess it's not
a big deal.

Reviewed-by: Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx>

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx





[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux