Re: [Intel-gfx] [PATCH 06/31] drm/i915: Fix IPS disable sequence.
Hi,
On 11 November 2015 at 23:31, Vivi, Rodrigo wrote:
> On Tue, 2015-11-10 at 16:34 +, Daniel Stone wrote:
>> On 5 November 2015 at 18:49, Rodrigo Vivi
>> 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/31] drm/i915: Fix IPS disable sequence.
On Tue, 2015-11-10 at 16:34 +, Daniel Stone wrote:
> Hi,
>
> On 5 November 2015 at 18:49, Rodrigo Vivi
> 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
Re: [Intel-gfx] [PATCH 06/31] drm/i915: Fix IPS disable sequence.
Hi,
On 5 November 2015 at 18:49, Rodrigo Vivi 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
