Re: [PATCH 12/16] drm/i915: Disable LP1+ watermarks while changing the number of active pipes

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Jun 04, 2014 at 03:24:13PM -0300, Paulo Zanoni wrote:
> 2014-05-22 11:48 GMT-03:00  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > 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älä <ville.syrjala@linux.intel.com>
> > ---
> >  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/i915/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 = crtc->base.dev;
> > +       struct intel_crtc *other_active_crtc = 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 = crtc->dev;
> > @@ -4023,6 +4044,9 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
> >
> >         intel_crtc->active = 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 = 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_crtc *crtc)
> >
> >         intel_crtc->active = 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 = 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/intel_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_crtc *crtc,
> >  void intel_program_watermarks_post(struct intel_crtc *crtc,
> >                                    const struct intel_crtc_wm_config *config);
> >  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/intel_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_private *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 = dev->dev_private;
> >         bool changed;
> > @@ -2873,16 +2873,17 @@ static void ilk_program_watermarks(struct drm_device *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 = 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 *pipe_wm,
> >                                          u32 vbl_count)
> > @@ -2905,7 +2906,7 @@ static void ilk_setup_pending_watermarks(struct intel_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 = crtc->base.dev;
> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> > +       struct ilk_pipe_wm_parameters params = {};
> > +       struct intel_pipe_wm pipe_wm = {};
> > +
> > +       WARN(crtc->active, "pipe %c should be disabled\n",
> > +            pipe_name(crtc->pipe));
> > +
> > +       ilk_compute_wm_parameters(&crtc->base, &params);
> > +
> > +       intel_compute_pipe_wm(&crtc->base, &params, &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 = pipe_wm;
> > +
> > +       /* pending update (if any) got cancelled */
> > +       crtc->wm.pending = 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älä
Intel OTC