Re: [PATCH 06/31] drm/i915: Fix IPS disable sequence.

["Vivi, Rodrigo" <rodrigo.vivi@intel.com>]

On Tue, 2015-11-10 at 16:34 +0000, Daniel Stone wrote:
> Hi,
> 
> On 5 November 2015 at 18:49, Rodrigo Vivi <rodrigo.vivi@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
>   - :(

You are absolutely right on this case... :/ Thanks for spotting this
case.

So I was considering your idea for the unified place but I ended up in
some concerns questions here.

First is the disable must occur on pre-update and enable on post
-update, so I would prefer to still let them spread and reactive.

But now I believe that we need to detach the atomic
->ips_{enable,disable} from primary and do for every plane on/off. So
if we are enabling any plane we just call ips_enable(). 
And if plane is being disabled and there is no other plane->visible in
this crtc we call intel_disable().

But I wonder how to skip the plane itself on for_each_plane_in_state...
Or should I just counter the number of state->visible and disable if <=
1 and let enable if we count more than 1 visible plane. Any better
idea?


> 
> Making this work on states would eliminate this entire class of bugs,
> and also make it much easier to handle async modesets.
> 
> Cheers,
> Daniel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx