[Intel-gfx] [PATCH v8 3/5] drm/i915: Make sprite updates atomic
Jesse Barnes
jbarnes at virtuousgeek.org
Fri Mar 7 16:57:17 CET 2014
On Fri, 7 Mar 2014 15:42:43 +0200
ville.syrjala at linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala at 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.
>
> 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 at linux.intel.com>
> ---
> 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 at virtuousgeek.org>
--
Jesse Barnes, Intel Open Source Technology Center
More information about the Intel-gfx
mailing list