On Thu, 17 Oct 2013 22:53:17 +0300
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.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_irq.c | 34 ++++++++++++++---
> drivers/gpu/drm/i915/intel_display.c | 2 +
> drivers/gpu/drm/i915/intel_drv.h | 3 ++
> drivers/gpu/drm/i915/intel_sprite.c | 72 ++++++++++++++++++++++++++++++++++++
> 4 files changed, 105 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 49bcf4e..776fe2f 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1271,6 +1271,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;
> + wake_up(&intel_crtc->vbl_wait);
> +}
> +
> static irqreturn_t valleyview_irq_handler(int irq, void *arg)
> {
> struct drm_device *dev = (struct drm_device *) arg;
> @@ -1313,8 +1322,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_VBLANK_INTERRUPT_STATUS)
> + if (pipe_stats[pipe] & PIPE_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);
> @@ -1509,11 +1520,15 @@ static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir)
> if (de_iir & DE_GSE)
> intel_opregion_asle_intr(dev);
>
> - if (de_iir & DE_PIPEA_VBLANK)
> - drm_handle_vblank(dev, 0);
> + if (de_iir & DE_PIPEA_VBLANK) {
> + drm_handle_vblank(dev, PIPE_A);
> + intel_pipe_handle_vblank(dev, PIPE_A);
> + }
>
> - if (de_iir & DE_PIPEB_VBLANK)
> - drm_handle_vblank(dev, 1);
> + if (de_iir & DE_PIPEB_VBLANK) {
> + drm_handle_vblank(dev, PIPE_B);
> + intel_pipe_handle_vblank(dev, PIPE_B);
> + }
>
> if (de_iir & DE_POISON)
> DRM_ERROR("Poison interrupt\n");
> @@ -1568,8 +1583,11 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
> intel_opregion_asle_intr(dev);
>
> for (i = 0; i < 3; i++) {
> - if (de_iir & (DE_PIPEA_VBLANK_IVB << (5 * i)))
> + if (de_iir & (DE_PIPEA_VBLANK_IVB << (5 * i))) {
> drm_handle_vblank(dev, i);
> + intel_pipe_handle_vblank(dev, i);
> + }
> +
> if (de_iir & (DE_PLANEA_FLIP_DONE_IVB << (5 * i))) {
> intel_prepare_page_flip(dev, i);
> intel_finish_page_flip_plane(dev, i);
> @@ -2695,6 +2713,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;
>
> @@ -2869,6 +2889,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 be056fd..34eb952 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9754,6 +9754,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 7f5f74d..83cb2a0 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -361,6 +361,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 c160e4f..ce4e4ec 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -37,6 +37,54 @@
> #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)
> +{
> + 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 = i915_get_crtc_scanline(crtc);
> +
> + while (scanline >= min && scanline <= max && timeout > 0) {
> + local_irq_enable();
> + timeout = wait_event_timeout(intel_crtc->vbl_wait,
> + intel_crtc->vbl_received,
> + timeout);
> + local_irq_disable();
> +
> + intel_crtc->vbl_received = false;
> + scanline = i915_get_crtc_scanline(crtc);
> + }
> +
> + drm_vblank_put(dev, pipe);
> +}
> +
> +static void intel_pipe_update_end(struct drm_crtc *crtc)
> +{
> + local_irq_enable();
> +}
> +
> static void
> vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
> struct drm_framebuffer *fb,
> @@ -125,6 +173,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);
>
> @@ -138,6 +188,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
> @@ -149,12 +201,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);
> }
>
> @@ -301,6 +357,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);
>
> @@ -321,6 +379,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> i915_gem_obj_ggtt_offset(obj) + sprsurf_offset);
> POSTING_READ(SPRSURF(pipe));
>
> + intel_pipe_update_end(crtc);
> +
> /* potentially re-enable LP watermarks */
> if (scaling_was_enabled && !dev_priv->sprite_scaling_enabled)
> intel_update_watermarks(crtc);
> @@ -335,6 +395,8 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
> int pipe = intel_plane->pipe;
> bool scaling_was_enabled = dev_priv->sprite_scaling_enabled;
>
> + 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)
> @@ -343,6 +405,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);
> +
> dev_priv->sprite_scaling_enabled &= ~(1 << pipe);
>
> intel_update_sprite_watermarks(plane, crtc, 0, 0, false, false);
> @@ -479,6 +543,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);
>
> @@ -493,6 +559,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
> @@ -503,6 +571,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);
> @@ -510,6 +580,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);
> +
> intel_update_sprite_watermarks(plane, crtc, 0, 0, false, false);
> }
>
It looks like we may need to check for preemption again on
local_irq_enable() according to preempt-locking.txt? Otherwise it
looks like this does the right thing.
With the above addressed or explained away:
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
