On Tue, 2015-11-10 at 16:34 +0000, Daniel Stone wrote:
> Hi,
>
> On 5 November 2015 at 18:49, Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
