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

[Daniel Stone]

Hi,

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

Right, they have to be separate - which applies doubly for async
modesets as well.

> 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?

Yeah, exactly that, but even easier: bool ips_target_state =
!!crtc_state->plane_mask;

So my idea would be that, in prepare_commit, you calculate the target
state based on the updated plane_mask. When actually committing the
state, call intel_ips_disable() in intel_pre_plane_update (if
!ips_target_state && intel_crtc->ips_enabled), and intel_ips_enable()
after the commit has finished (if ips_target_state &&
!intel_crtc->ips_enabled). That split would be a really good fit for
async/atomic, where we need to calculate the target state in advance,
and only apply it some time later. Same goes for PSR.

Basically, any work we need to do for modeset needs to be quite
statically calculated in the prepare stage, so we can apply it from
the commit stage, without needing to pull any further state pointers
(we can't do this with async), and certainly without reference to the
actual hardware configuration (e.g. inspecting registers).

Cheers,
Daniel