Re: [PATCH v3 09/11] drm/i915: Handle ips_enabled in fastset, v2.

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

On Fri, Nov 10, 2017 at 02:15:04PM +0100, Daniel Vetter wrote:
> On Fri, Nov 10, 2017 at 12:35:01PM +0100, Maarten Lankhorst wrote:
> > pre_plane_disable and post_plane_enable handle set ips correctly,
> > but if there is no modeset and the ips_enabled value changes
> > because of force disabling for crc, or hw state readout, then we
> > don't toggle ips correctly. Handle this special case, which prevents
> > us from having to do a full modeset when collecting pipe crc.
> > 
> > Changes since v1:
> > - Simplify conditions.
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> Hm yeah, more hints that maybe ips should be a plane, or at least a wm
> state thing?

I think we would have to write this as something like:

pre_plane_update()
if (old_crtc_state->ips && old_crtc_state->active_planes != 0 &&
    (!new_crtc_state->ips || new_crtc_state->active_planes == 0)
    disable_ips();
 
post_plane_update()
if ((!old_crtc_state->ips || old_crtc_state->active_planes == 0) &&
    new_crtc_state->ips && new_crtc_state->active_planes != 0)
    enable_ips();

Probably needs a few mode needs_modeset() and crtc_state->active
checks etc. to account for all the disabling pipe, enabling pipe,
toggling pipe, and doing nothing to the pipe cases.

To simplify maybe we should probably just account for active_planes
in crtc_state->ips_enabled already. Ie. call hsw_compute_ips_config()
even if we don't do a modeset.

We'd have to change the bdw cdclk vs. ips workaround to work the
other way around though since currently hsw_compute_ips_config() assumes
that we can do a modeset to increase cdclk (assuming it's not already
maxed out). But I think it would be a worthwile change to make the code
less convoluted.

> 
> This turns a bit into a rabbit hole, so if Ville thinks this is all still
> reasonably clean-ish I think we can go with this here too ... I'm just
> wondering a bit.
> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c  | 6 ++++++
> >  drivers/gpu/drm/i915/intel_pipe_crc.c | 2 +-
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 4aa02e27985d..1b75af773ef7 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5040,6 +5040,9 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
> >  		    (needs_modeset(&pipe_config->base) ||
> >  		     !old_primary_state->base.visible))
> >  			intel_post_enable_primary(&crtc->base, pipe_config);
> > +		else if (pipe_config->update_pipe)
> > +			/* IPS turned on after fastset or CRC collection disable. */
> > +			hsw_enable_ips(pipe_config);
> >  	}
> >  }
> >  
> > @@ -5069,6 +5072,9 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
> >  		if (old_primary_state->base.visible &&
> >  		    (modeset || !primary_state->base.visible))
> >  			intel_pre_disable_primary(&crtc->base, old_crtc_state);
> > +		else if (pipe_config->update_pipe && !pipe_config->ips_enabled)
> > +			/* IPS turned off for CRC, disable it. */
> > +			hsw_disable_ips(old_crtc_state);
> >  	}
> >  
> >  	/*
> > diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > index 899839f2f7c6..cb92befc16d7 100644
> > --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> > +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> > @@ -542,7 +542,7 @@ static void hsw_pipe_A_crc_wa(struct drm_i915_private *dev_priv,
> >  		 */
> >  		pipe_config->ips_force_disable = enable;
> >  		if (pipe_config->ips_enabled == enable)
> > -			pipe_config->base.connectors_changed = true;
> > +			pipe_config->base.mode_changed = true;
> >  	}
> >  
> >  	if (IS_HASWELL(dev_priv)) {
> > -- 
> > 2.15.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx