[Intel-gfx] [PATCH 06/31] drm/i915: Fix IPS disable sequence.

[Daniel Stone]

Hi,

On 5 November 2015 at 18:49, Rodrigo Vivi <rodrigo.vivi at intel.com> wrote:
>  /**
> + * intel_ips_disable_if_alone - Disable IPS if alone in the pipe.
> + * @crtc: intel crtc
> + *
> + * This function should be called when primary plane is being disabled.
> + * It checks if there is any other plane enabled on the pipe when primary is
> + * going to be disabled. In this case IPS can continue enabled, but it needs
> + * to be disabled otherwise.
> + */

As an example of what I meant before, I would reword this to reflect
its actual functionality, which doesn't necessarily have anything to
do specifically with disabling the primary plane:
'This function examines the CRTC state to determine if IPS should
be disabled. Currently, IPS is disabled if no planes are active on the
CRTC.'

Discussing its use in the context of disabling the primary plane I
think obscures its intent, and also introduces a bug. :)

> +void intel_ips_disable_if_alone(struct intel_crtc *crtc)
> +{
> +       struct drm_device *dev = crtc->base.dev;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       bool ips_enabled;
> +       struct intel_plane *intel_plane;
> +
> +       mutex_lock(&dev_priv->display_ips.lock);
> +       ips_enabled = dev_priv->display_ips.enabled;
> +       mutex_unlock(&dev_priv->display_ips.lock);
> +
> +       if (!ips_enabled)
> +               return;
> +
> +       for_each_intel_plane_on_crtc(dev, crtc, intel_plane) {
> +               enum plane plane = intel_plane->plane;
> +
> +               if (plane != PLANE_A &&
> +                   !!(I915_READ(DSPCNTR(plane)) & DISPLAY_PLANE_ENABLE))
> +                       return;
> +               intel_ips_disable(crtc);
> +       }
> +}

Rather than reading the registers, this should just inspect
plane_state->visible. Reading the registers introduces the same bug as
I mentioned the last mail, but in a different way:
  - IPS is enabled
  - primary and overlay planes are both enabled
  - user commits an atomic state which disables both primary and
overlay planes, so IPS must be disabled
  - disabling the primary plane calls this function, which sees that
the overlay plane is still active, so IPS can remain enabled
  - the overlay plane gets disabled, with IPS still active
  - :(

Making this work on states would eliminate this entire class of bugs,
and also make it much easier to handle async modesets.

Cheers,
Daniel