Re: [PATCH 2/3] drm/i915: Enable IPS for sprite plane

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@linux.intel.com>
> >> ---
> >>  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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx