From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 12/16] drm/i915: Disable LP1+ watermarks while changing the number of active pipes Date: Mon, 9 Jun 2014 19:51:24 +0300 Message-ID: <20140609165124.GS27580@intel.com> References: <1400770101-14277-1-git-send-email-ville.syrjala@linux.intel.com> <1400770101-14277-13-git-send-email-ville.syrjala@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga03.intel.com (mga03.intel.com [143.182.124.21]) by gabe.freedesktop.org (Postfix) with ESMTP id 2D0726E183 for ; Mon, 9 Jun 2014 09:52:48 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Paulo Zanoni Cc: Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Wed, Jun 04, 2014 at 03:24:13PM -0300, Paulo Zanoni wrote: > 2014-05-22 11:48 GMT-03:00 : > > From: Ville Syrj=E4l=E4 > > > > When we switch between one active pipe and multiple active pipes, the > > display FIFO gets repartitioned. Disable the LP1+ waterwarks while that > > is happening to make sure we don't get any glitches on other active > > pipes while doing a modeset on another other pipe. > > > > Signed-off-by: Ville Syrj=E4l=E4 > > --- > > drivers/gpu/drm/i915/intel_display.c | 45 ++++++++++++++++++++++++++++= ++++++ > > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > > drivers/gpu/drm/i915/intel_pm.c | 47 ++++++++++++++++++++++++++++= ++++---- > > 3 files changed, 89 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i91= 5/intel_display.c > > index bccf414..311c0f0 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -3985,6 +3985,27 @@ static struct intel_crtc *get_other_active_crtc(= struct intel_crtc *crtc) > > return other_active_crtc; > > } > > > > +static void ilk_prepare_for_num_pipes_change(struct intel_crtc *crtc) > > +{ > > + struct drm_device *dev =3D crtc->base.dev; > > + struct intel_crtc *other_active_crtc =3D get_other_active_crtc(= crtc); > > + > > + /* > > + * If we change between one pipe and multiple pipes, > > + * make sure the other active pipe is prepared > > + * for having its FIFO resized. We do that by making > > + * sure the pipe isn't using LP1+ watermarks when > > + * the second pipe gets enabled or disabled. > > + */ > > + if (!other_active_crtc) > > + return; > = > But get_other_active_crtc() returns NULL in case 2 other CRTCs are > already active. If we're the third pipe being enabled, we may still > get an underrun. The FIFO repartitioning happens only when going 1<->2 pipes, so the 2<->3 cases don't matter here. > = > > + > > + ilk_wm_synchronize(other_active_crtc); > > + > > + if (ilk_disable_lp_wm(dev)) > > + intel_wait_for_vblank(dev, other_active_crtc->pipe); > > +} > > + > > static void ironlake_crtc_enable(struct drm_crtc *crtc) > > { > > struct drm_device *dev =3D crtc->dev; > > @@ -4023,6 +4044,9 @@ static void ironlake_crtc_enable(struct drm_crtc = *crtc) > > > > intel_crtc->active =3D true; > > > > + /* Make sure other pipes are prepared for FIFO resizing */ > > + ilk_prepare_for_num_pipes_change(intel_crtc); > > + > > intel_set_cpu_fifo_underrun_reporting(dev, pipe, true); > > intel_set_pch_fifo_underrun_reporting(dev, pipe, true); > > > > @@ -4123,6 +4147,9 @@ static void haswell_crtc_enable(struct drm_crtc *= crtc) > > > > intel_crtc->active =3D true; > > > > + /* Make sure other pipes are prepared for FIFO resizing */ > > + ilk_prepare_for_num_pipes_change(intel_crtc); > > + > > intel_set_cpu_fifo_underrun_reporting(dev, pipe, true); > > if (intel_crtc->config.has_pch_encoder) > > intel_set_pch_fifo_underrun_reporting(dev, TRANSCODER_A= , true); > > @@ -4192,6 +4219,9 @@ static void ironlake_crtc_disable(struct drm_crtc= *crtc) > > > > intel_crtc_disable_planes(crtc); > > > > + /* Make sure other pipes are prepared for FIFO resizing */ > > + ilk_prepare_for_num_pipes_change(intel_crtc); > > + > > for_each_encoder_on_crtc(dev, crtc, encoder) > > encoder->disable(encoder); > > > > @@ -4235,6 +4265,12 @@ static void ironlake_crtc_disable(struct drm_crt= c *crtc) > > > > intel_crtc->active =3D false; > > > > + /* > > + * Potentially let some other pipe use the > > + * full FIFO, and re-enable LP1+ watermarks. > > + */ > > + ilk_wm_pipe_post_disable(intel_crtc); > > + > > mutex_lock(&dev->struct_mutex); > > intel_update_fbc(dev); > > intel_edp_psr_update(dev); > > @@ -4255,6 +4291,9 @@ static void haswell_crtc_disable(struct drm_crtc = *crtc) > > > > intel_crtc_disable_planes(crtc); > > > > + /* Make sure other pipes are prepared for FIFO resizing */ > > + ilk_prepare_for_num_pipes_change(intel_crtc); > > + > > for_each_encoder_on_crtc(dev, crtc, encoder) { > > intel_opregion_notify_encoder(encoder, false); > > encoder->disable(encoder); > > @@ -4282,6 +4321,12 @@ static void haswell_crtc_disable(struct drm_crtc= *crtc) > > > > intel_crtc->active =3D false; > > > > + /* > > + * Potentially let some other pipe use the > > + * full FIFO, and re-enable LP1+ watermarks. > > + */ > > + ilk_wm_pipe_post_disable(intel_crtc); > > + > > mutex_lock(&dev->struct_mutex); > > intel_update_fbc(dev); > > intel_edp_psr_update(dev); > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/in= tel_drv.h > > index 5ad7ad5..98f878f 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -1022,6 +1022,8 @@ void intel_program_watermarks_pre(struct intel_cr= tc *crtc, > > void intel_program_watermarks_post(struct intel_crtc *crtc, > > const struct intel_crtc_wm_config *c= onfig); > > void ilk_wm_synchronize(struct intel_crtc *crtc); > > +void ilk_wm_pipe_post_disable(struct intel_crtc *crtc); > > +bool ilk_disable_lp_wm(struct drm_device *dev); > > > > > > /* intel_sdvo.c */ > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/int= el_pm.c > > index 1c072cd..18ea8b1 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -2765,7 +2765,7 @@ static void ilk_write_wm_values(struct drm_i915_p= rivate *dev_priv, > > } > > } > > > > -static bool ilk_disable_lp_wm(struct drm_device *dev) > > +bool ilk_disable_lp_wm(struct drm_device *dev) > > { > > struct drm_i915_private *dev_priv =3D dev->dev_private; > > bool changed; > > @@ -2873,16 +2873,17 @@ static void ilk_program_watermarks(struct drm_d= evice *dev) > > ilk_write_wm_values(dev_priv, &results); > > } > > > > -static void ilk_update_watermarks(struct drm_device *dev) > > +static void ilk_update_watermarks(struct drm_device *dev, bool force) > > { > > bool changed; > > > > changed =3D ilk_refresh_pending_watermarks(dev); > > > > - if (changed) > > + if (changed || force) > > ilk_program_watermarks(dev); > > } > > > > +/* Prepare the pipe to update the its watermarks on the next vblank */ > > static void ilk_setup_pending_watermarks(struct intel_crtc *crtc, > > const struct intel_pipe_wm *pi= pe_wm, > > u32 vbl_count) > > @@ -2905,7 +2906,7 @@ static void ilk_setup_pending_watermarks(struct i= ntel_crtc *crtc, > > spin_unlock_irq(&crtc->wm.lock); > > > > /* try to update immediately */ > > - ilk_update_watermarks(dev); > > + ilk_update_watermarks(dev, false); > > > > spin_lock_irq(&crtc->wm.lock); > > > > @@ -2999,7 +3000,7 @@ static void ilk_watermark_work(struct work_struct= *work) > > > > mutex_lock(&dev_priv->wm.mutex); > > > > - ilk_update_watermarks(dev_priv->dev); > > + ilk_update_watermarks(dev_priv->dev, false); > > > > mutex_unlock(&dev_priv->wm.mutex); > > } > > @@ -3060,6 +3061,42 @@ void ilk_wm_synchronize(struct intel_crtc *crtc) > > mutex_unlock(&dev_priv->wm.mutex); > > } > > > > +void ilk_wm_pipe_post_disable(struct intel_crtc *crtc) > > +{ > > + struct drm_device *dev =3D crtc->base.dev; > > + struct drm_i915_private *dev_priv =3D dev->dev_private; > > + struct ilk_pipe_wm_parameters params =3D {}; > > + struct intel_pipe_wm pipe_wm =3D {}; > > + > > + WARN(crtc->active, "pipe %c should be disabled\n", > > + pipe_name(crtc->pipe)); > > + > > + ilk_compute_wm_parameters(&crtc->base, ¶ms); > > + > > + intel_compute_pipe_wm(&crtc->base, ¶ms, &pipe_wm); > > + > > + mutex_lock(&dev_priv->wm.mutex); > > + > > + spin_lock_irq(&crtc->wm.lock); > > + > > + WARN(crtc->wm.dirty, "pipe %c disabled with dirty watermarks\n", > > + pipe_name(crtc->pipe)); > > + > > + /* clean up if something is left behind */ > > + ilk_wm_cancel(crtc); > > + > > + spin_unlock_irq(&crtc->wm.lock); > > + > > + crtc->wm.active =3D pipe_wm; > > + > > + /* pending update (if any) got cancelled */ > > + crtc->wm.pending =3D crtc->wm.active; > > + > > + ilk_update_watermarks(dev, true); > > + > > + mutex_unlock(&dev_priv->wm.mutex); > > +} > > + > > static int ilk_update_primary_wm(struct intel_crtc *crtc, > > struct intel_crtc_wm_config *config) > > { > > -- > > 1.8.5.5 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > = > = > = > -- = > Paulo Zanoni -- = Ville Syrj=E4l=E4 Intel OTC