[Intel-gfx] [PATCH 02/19] drm/i915: Call intel_update_watermarks() in specific place during modeset

[Paulo Zanoni]

2013/8/30  <ville.syrjala at linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>
> Make the call to intel_update_watermarks() just once or twice during
> modeset. Ideally it should happen independently when each plane gets
> enabled/disabled, but for now it seems better to keep it in central
> place. We can improve things when we get all the planes sorted out
> in a better way.
>
> When enabling set up the watermarks just before the pipe is enabled.
> And when disabling we need to wait until we've marked the crtc as
> inactive.

Why do we need to wait until we've marked the CRTC as inactive?
(Daniel/Ville should put the answer in the commit message)

The patch looks correct, but <bikeshed> I'd separate the "don't update
watermarks at crtc->mode_set" part from the current patch, since if we
start getting regressions we won't know if they're caused by the fact
that now we don't adjust the watermarks before enabling the PCH and
doing link training, or if they're caused by the fact that we don't
update them anymore at mode_set </bikeshed>. But since it looks
correct, we shouldn't really be getting regressions...

With or without changes: Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com>



>
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 13856ec..a5181fe 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3278,8 +3278,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>         intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
>         intel_set_pch_fifo_underrun_reporting(dev, pipe, true);
>
> -       intel_update_watermarks(crtc);
> -
>         for_each_encoder_on_crtc(dev, crtc, encoder)
>                 if (encoder->pre_enable)
>                         encoder->pre_enable(encoder);
> @@ -3302,6 +3300,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>          */
>         intel_crtc_load_lut(crtc);
>
> +       intel_update_watermarks(crtc);
>         intel_enable_pipe(dev_priv, pipe,
>                           intel_crtc->config.has_pch_encoder, false);
>         intel_enable_plane(dev_priv, plane, pipe);
> @@ -3388,8 +3387,6 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>         if (intel_crtc->config.has_pch_encoder)
>                 intel_set_pch_fifo_underrun_reporting(dev, TRANSCODER_A, true);
>
> -       intel_update_watermarks(crtc);
> -
>         if (intel_crtc->config.has_pch_encoder)
>                 dev_priv->display.fdi_link_train(crtc);
>
> @@ -3410,6 +3407,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>         intel_ddi_set_pipe_settings(crtc);
>         intel_ddi_enable_transcoder_func(crtc);
>
> +       intel_update_watermarks(crtc);
>         intel_enable_pipe(dev_priv, pipe,
>                           intel_crtc->config.has_pch_encoder, false);
>         intel_enable_plane(dev_priv, plane, pipe);
> @@ -3677,7 +3675,6 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>                 return;
>
>         intel_crtc->active = true;
> -       intel_update_watermarks(crtc);
>
>         for_each_encoder_on_crtc(dev, crtc, encoder)
>                 if (encoder->pre_pll_enable)
> @@ -3696,6 +3693,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>
>         intel_crtc_load_lut(crtc);
>
> +       intel_update_watermarks(crtc);
>         intel_enable_pipe(dev_priv, pipe, false, is_dsi);
>         intel_enable_plane(dev_priv, plane, pipe);
>         intel_enable_planes(crtc);
> @@ -3722,7 +3720,6 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>                 return;
>
>         intel_crtc->active = true;
> -       intel_update_watermarks(crtc);
>
>         for_each_encoder_on_crtc(dev, crtc, encoder)
>                 if (encoder->pre_enable)
> @@ -3734,6 +3731,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>
>         intel_crtc_load_lut(crtc);
>
> +       intel_update_watermarks(crtc);
>         intel_enable_pipe(dev_priv, pipe, false, false);
>         intel_enable_plane(dev_priv, plane, pipe);
>         intel_enable_planes(crtc);
> @@ -3805,8 +3803,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>                 i9xx_disable_pll(dev_priv, pipe);
>
>         intel_crtc->active = false;
> -       intel_update_fbc(dev);
>         intel_update_watermarks(crtc);
> +
> +       intel_update_fbc(dev);
>  }
>
>  static void i9xx_crtc_off(struct drm_crtc *crtc)
> @@ -4972,8 +4971,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>
>         ret = intel_pipe_set_base(crtc, x, y, fb);
>
> -       intel_update_watermarks(crtc);
> -
>         return ret;
>  }
>
> @@ -5860,8 +5857,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>
>         ret = intel_pipe_set_base(crtc, x, y, fb);
>
> -       intel_update_watermarks(crtc);
> -
>         return ret;
>  }
>
> @@ -6316,8 +6311,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>
>         ret = intel_pipe_set_base(crtc, x, y, fb);
>
> -       intel_update_watermarks(crtc);
> -
>         return ret;
>  }
>
> --
> 1.8.1.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni