On Mon, Nov 20, 2017 at 12:12:40PM +0100, Maarten Lankhorst wrote:
> Op 17-11-17 om 16:55 schreef Ville Syrjälä:
> > On Fri, Nov 17, 2017 at 04:37:55PM +0100, Maarten Lankhorst wrote:
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> >> ---
> >> drivers/gpu/drm/i915/intel_display.c | 13 ++++++-------
> >> 1 file changed, 6 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 8283e80597bd..38a1cdb3dbb2 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -5044,7 +5044,7 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
> >> intel_fbc_post_update(crtc);
> >>
> >> if (primary_state->base.visible &&
> >> - (needs_modeset(&pipe_config->base) ||
> >> + (pipe_config->disable_cxsr ||
> >> !old_primary_state->base.visible))
> >> intel_post_enable_primary(&crtc->base, pipe_config);
> >> }
> >> @@ -5064,7 +5064,7 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
> >> struct intel_atomic_state *old_intel_state =
> >> to_intel_atomic_state(old_state);
> >>
> >> - if (needs_modeset(&pipe_config->base) || !pipe_config->ips_enabled)
> >> + if (pipe_config->disable_cxsr || !pipe_config->ips_enabled)
> > What does IPS have to do with cxsr?
> Nothing, laziness. disable cxsr is set when planes get disabled.
> >> hsw_disable_ips(old_crtc_state);
> >>
> >> if (old_pri_state) {
> >> @@ -6224,12 +6224,11 @@ static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
> >> visible_planes = pipe_config->active_planes & ~BIT(PLANE_CURSOR);
> >>
> >> /*
> >> - * FIXME IPS should be fine as long as one plane is
> >> - * enabled, but in practice it seems to have problems
> >> - * when going from primary only to sprite only and vice
> >> - * versa.
> >> + * IPS should be fine as long as one plane is enabled, but
> >> + * temporarily disable it when when going from primary only
> >> + * to sprite only and vice versa.
> >> */
> >> - if (visible_planes != BIT(PLANE_PRIMARY))
> >> + if (hweight32(visible_planes) != 1)
> >> return false;
> > That should just be
> > if (active_planes == 0)
> > return false;
> >
> > assuming we have no problems with the toggling between
> > primary only and sprite only.
> >
> > I can't recall how the cursor affecrs IPS. But I think IPS should
> > work as long as any plane (including the cursor) is enabled.
> But cursor visibility can change without the full atomic commit, so it's too unreliable to take into
> account with the calculations.
Hmm. I guess we can ignore the cursor then. It's generally pretty
small, so probably no huge benefit from using IPS with cursor only
anyway.
>
> What happens when it doesn't work well? Would it be caught by any tests?
I think it should be caught by CRCs, if we have any that capture
CRCs across plane enable/disable.
Except, there is a problem on ilk-ivb at least, maybe it went all
the way to bdw. IIRC when I last tried a test like that the CRC for
a black primary plane didn't match the CRC for no planes. And I think I
tried to eliminate gamma/csc as the source of the discrepancy without
success. Also IIRC there was always a single frame with an even stranger
CRC just after turning off the last plane.
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
