From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH v2 3/5] drm/i915: Make sprite updates atomic Date: Mon, 20 Jan 2014 18:56:44 +0200 Message-ID: <20140120165644.GW9454@intel.com> References: <1389982146-1460-1-git-send-email-ville.syrjala@linux.intel.com> <1389982146-1460-4-git-send-email-ville.syrjala@linux.intel.com> <20140120162339.GJ15089@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id 15666FAD63 for ; Mon, 20 Jan 2014 08:56:48 -0800 (PST) Content-Disposition: inline In-Reply-To: <20140120162339.GJ15089@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Daniel Vetter Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org 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@linux.intel.com w= rote: > > From: Ville Syrj=E4l=E4 > > = > > 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 > > Signed-off-by: Ville Syrj=E4l=E4 > = > 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/i91= 5_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 =3D > > + to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe)); > > + > > + intel_crtc->vbl_received =3D 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 =3D (struct drm_device *) arg; > > @@ -1476,8 +1485,10 @@ static irqreturn_t valleyview_irq_handler(int ir= q, 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_d= evice *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_d= evice *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, voi= d *arg) > > continue; > > = > > pipe_iir =3D 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) =3D=3D 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) =3D=3D 0) > > return false; > > = > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i91= 5/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 =3D !pipe; > > } > > = > > + init_waitqueue_head(&intel_crtc->vbl_wait); > > + > > BUG_ON(pipe >=3D ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) || > > dev_priv->plane_to_crtc_mapping[intel_crtc->plane] !=3D NULL); > > dev_priv->plane_to_crtc_mapping[intel_crtc->plane] =3D &intel_crtc->b= ase; > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/in= tel_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 > > #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_htota= l); > > +} > > + > > +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 =3D crtc->dev; > > + struct intel_crtc *intel_crtc =3D to_intel_crtc(crtc); > > + const struct drm_display_mode *mode =3D &intel_crtc->config.adjusted_= mode; > > + enum pipe pipe =3D intel_crtc->pipe; > > + /* FIXME needs to be calibrated sensibly */ > > + unsigned int min =3D mode->crtc_vblank_start - usecs_to_scanlines(mod= e, 100); > > + unsigned int max =3D mode->crtc_vblank_start - 1; > > + long timeout =3D msecs_to_jiffies_timeout(1); > > + unsigned int scanline; > > + > > + if (WARN_ON(drm_vblank_get(dev, pipe))) > > + return; > > + > > + local_irq_disable(); > > + > > + intel_crtc->vbl_received =3D 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 =3D intel_get_crtc_scanline(crtc); > > + > > + while (scanline >=3D min && scanline <=3D max && timeout > 0) { > > + local_irq_enable(); > > + preempt_check_resched(); > > + > > + timeout =3D wait_event_timeout(intel_crtc->vbl_wait, > > + intel_crtc->vbl_received, > > + timeout); > > + > > + local_irq_disable(); > > + > > + intel_crtc->vbl_received =3D false; > > + scanline =3D 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 d= rm_crtc *crtc, > > fb->pitches[0]); > > linear_offset -=3D 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 d= rm_crtc *crtc, > > I915_MODIFY_DISPBASE(SPSURF(pipe, plane), i915_gem_obj_ggtt_offset(ob= j) + > > 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, struc= t drm_crtc *crtc) > > int pipe =3D intel_plane->pipe; > > int plane =3D 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 dr= m_crtc *crtc, > > pixel_size, fb->pitches[0]); > > linear_offset -=3D 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 dr= m_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 d= rm_crtc *crtc) > > struct intel_plane *intel_plane =3D to_intel_plane(plane); > > int pipe =3D 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 d= rm_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 dr= m_crtc *crtc, > > pixel_size, fb->pitches[0]); > > linear_offset -=3D 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 dr= m_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 d= rm_crtc *crtc) > > struct intel_plane *intel_plane =3D to_intel_plane(plane); > > int pipe =3D 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 d= rm_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@lists.freedesktop.org > > 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=E4l=E4 Intel OTC