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
