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

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

 



On Mon, Jan 20, 2014 at 05:23:39PM +0100, Daniel Vetter wrote:
> On Fri, Jan 17, 2014 at 08:09:04PM +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.
> > 
> > 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@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> 
> 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.

I can add a note saying something about wake_up() and wait_event()
implying memory barriers.

> 
> > +	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 ;-)

I can change it.

> 
> 
> > +{
> > +	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;

Now that you got me thinking about barriers again, I wonder if I should
add an explicit compiler barrier here. The intel_get_crtc_scanline() call
should act as a compiler barrier though, so it shouldn't be needed. So
maybe I should add a comment here too?

> > +	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@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
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