Re: [PATCH 2/2] drm/i915: clean up panel fitter handling in lvds

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Feb 8, 2013 at 3:35 PM, Mika Kuoppala
<mika.kuoppala@linux.intel.com> wrote:
> commit c1d1f5aeda2033d96e872f416388653f05d4c16d
> Author: Mika Kuoppala <mika.kuoppala@intel.com>
> Date:   Tue Feb 5 17:26:52 2013 +0200
>
>     drm/i915: disable shared panel fitter for pipe

You can't cite the sha1 of a patch which isn't merged yet, since the
sha1 you have in your git tree will be different than the one this
patch will get when I pull it in. If you want to cite the commit just
say "... previous patch titled 'drm/i915: blabla' in this series ...".

Note that even when a patch is merged into dinq the sha1 might still
change when I do a rebase. Commits are only truly frozen once they've
landed in Dave's tree.

> moved panel fit disabling to be crtc property. Thus
> the need to explicitly turn pfit off in encoder side
> became obsolete. Take advantage of that and get rid of
> pfit_dirty state handling in lvds.

I think you should add that this simplification is only possible due
to the modeset rework - before the crtc helpers could call the pfit
setup code a few times too often.

But imo the important thing is that we now enable/disable the pfit at
the right time in the modeset sequence (when planes are already
disabled). The old code you're removing here didn't do that correctly.

Can you please update the commit message?

Thanks, Daniel
>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    5 ++++
>  drivers/gpu/drm/i915/intel_lvds.c    |   47 +++++++++++++++++-----------------
>  2 files changed, 29 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9b5f0fb..175538d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3615,6 +3615,11 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>         intel_update_watermarks(dev);
>
>         intel_enable_pll(dev_priv, pipe);
> +
> +       for_each_encoder_on_crtc(dev, crtc, encoder)
> +               if (encoder->pre_enable)
> +                       encoder->pre_enable(encoder);
> +
>         intel_enable_pipe(dev_priv, pipe, false);
>         intel_enable_plane(dev_priv, plane, pipe);
>
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 7e4ec63..f93058a 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -51,7 +51,6 @@ struct intel_lvds_encoder {
>
>         u32 pfit_control;
>         u32 pfit_pgm_ratios;
> -       bool pfit_dirty;
>         bool is_dual_link;
>         u32 reg;
>
> @@ -151,6 +150,29 @@ static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder)
>         I915_WRITE(lvds_encoder->reg, temp);
>  }
>
> +static void intel_pre_enable_lvds(struct intel_encoder *encoder)
> +{
> +       struct drm_device *dev = encoder->base.dev;
> +       struct intel_lvds_encoder *enc = to_lvds_encoder(&encoder->base);
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +       if (HAS_PCH_SPLIT(dev) || !enc->pfit_control)
> +               return;
> +
> +       /*
> +        * Enable automatic panel scaling so that non-native modes
> +        * fill the screen.  The panel fitter should only be
> +        * adjusted whilst the pipe is disabled, according to
> +        * register description and PRM.
> +        */
> +       DRM_DEBUG_KMS("applying panel-fitter: %x, %x\n",
> +                     enc->pfit_control,
> +                     enc->pfit_pgm_ratios);
> +
> +       I915_WRITE(PFIT_PGM_RATIOS, enc->pfit_pgm_ratios);
> +       I915_WRITE(PFIT_CONTROL, enc->pfit_control);
> +}
> +
>  /**
>   * Sets the power state for the panel.
>   */
> @@ -172,22 +194,6 @@ static void intel_enable_lvds(struct intel_encoder *encoder)
>
>         I915_WRITE(lvds_encoder->reg, I915_READ(lvds_encoder->reg) | LVDS_PORT_EN);
>
> -       if (lvds_encoder->pfit_dirty) {
> -               /*
> -                * Enable automatic panel scaling so that non-native modes
> -                * fill the screen.  The panel fitter should only be
> -                * adjusted whilst the pipe is disabled, according to
> -                * register description and PRM.
> -                */
> -               DRM_DEBUG_KMS("applying panel-fitter: %x, %x\n",
> -                             lvds_encoder->pfit_control,
> -                             lvds_encoder->pfit_pgm_ratios);
> -
> -               I915_WRITE(PFIT_PGM_RATIOS, lvds_encoder->pfit_pgm_ratios);
> -               I915_WRITE(PFIT_CONTROL, lvds_encoder->pfit_control);
> -               lvds_encoder->pfit_dirty = false;
> -       }
> -
>         I915_WRITE(ctl_reg, I915_READ(ctl_reg) | POWER_TARGET_ON);
>         POSTING_READ(lvds_encoder->reg);
>         if (wait_for((I915_READ(stat_reg) & PP_ON) != 0, 1000))
> @@ -217,11 +223,6 @@ static void intel_disable_lvds(struct intel_encoder *encoder)
>         if (wait_for((I915_READ(stat_reg) & PP_ON) == 0, 1000))
>                 DRM_ERROR("timed out waiting for panel to power off\n");
>
> -       if (lvds_encoder->pfit_control) {
> -               I915_WRITE(PFIT_CONTROL, 0);
> -               lvds_encoder->pfit_dirty = true;
> -       }
> -
>         I915_WRITE(lvds_encoder->reg, I915_READ(lvds_encoder->reg) & ~LVDS_PORT_EN);
>         POSTING_READ(lvds_encoder->reg);
>  }
> @@ -461,7 +462,6 @@ out:
>             pfit_pgm_ratios != lvds_encoder->pfit_pgm_ratios) {
>                 lvds_encoder->pfit_control = pfit_control;
>                 lvds_encoder->pfit_pgm_ratios = pfit_pgm_ratios;
> -               lvds_encoder->pfit_dirty = true;
>         }
>         dev_priv->lvds_border_bits = border;
>
> @@ -1109,6 +1109,7 @@ bool intel_lvds_init(struct drm_device *dev)
>                          DRM_MODE_ENCODER_LVDS);
>
>         intel_encoder->enable = intel_enable_lvds;
> +       intel_encoder->pre_enable = intel_pre_enable_lvds;
>         intel_encoder->pre_pll_enable = intel_pre_pll_enable_lvds;
>         intel_encoder->disable = intel_disable_lvds;
>         intel_encoder->get_hw_state = intel_lvds_get_hw_state;
> --
> 1.7.9.5
>
> _______________________________________________
> 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