Re: [PATCH 07/13] drm/i915: Move LUT programming to happen after vblank waits

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

On Wed, Jan 16, 2019 at 09:38:59AM -0800, Matt Roper wrote:
> On Fri, Jan 11, 2019 at 07:08:17PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > The LUTs are single buffered so we should program them after
> > the double buffered pipe updates have been latched by the
> > hardware.
> > 
> > We'll also fix up the IPS vs. split gamma w/a to do the IPS
> > disable like everyone else. Note that this is currently dead
> > code as we don't use the split gamma mode on HSW, but that
> > will be fixed up shortly.
> 
> I don't think this is quite dead code...we don't use split gamma
> ourselves, but we could potentially inherit that setup from the BIOS
> (which will stick around until it eventually gets clobbered by the first
> modeset/fastset).
> 
> Uma's series added some logic to sanitize the LUT's immediately on boot,
> but that hasn't landed yet.

With the gamma_enable/csc_enable flags properly tracked we shouldn't
need anything like that. Or am I missing something?

> 
> 
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_color.c   | 25 +--------------
> >  drivers/gpu/drm/i915/intel_display.c | 47 ++++++++++++++++++++++++----
> >  2 files changed, 42 insertions(+), 30 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> > index f9e0855162f3..0c0da7ed0fd7 100644
> > --- a/drivers/gpu/drm/i915/intel_color.c
> > +++ b/drivers/gpu/drm/i915/intel_color.c
> > @@ -361,29 +361,6 @@ static void hsw_color_commit(const struct intel_crtc_state *crtc_state)
> >  	ilk_load_csc_matrix(crtc_state);
> >  }
> >  
> > -/* Loads the legacy palette/gamma unit for the CRTC on Haswell. */
> > -static void haswell_load_luts(const struct intel_crtc_state *crtc_state)
> > -{
> > -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> > -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > -	bool reenable_ips = false;
> > -
> > -	/*
> > -	 * Workaround : Do not read or write the pipe palette/gamma data while
> > -	 * GAMMA_MODE is configured for split gamma and IPS_CTL has IPS enabled.
> > -	 */
> > -	if (IS_HASWELL(dev_priv) && crtc_state->ips_enabled &&
> > -	    (crtc_state->gamma_mode == GAMMA_MODE_MODE_SPLIT)) {
> > -		hsw_disable_ips(crtc_state);
> > -		reenable_ips = true;
> > -	}
> > -
> > -	i9xx_load_luts(crtc_state);
> > -
> > -	if (reenable_ips)
> > -		hsw_enable_ips(crtc_state);
> > -}
> > -
> >  static void bdw_load_degamma_lut(const struct intel_crtc_state *crtc_state)
> >  {
> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> > @@ -660,7 +637,7 @@ void intel_color_init(struct intel_crtc *crtc)
> >  	if (IS_CHERRYVIEW(dev_priv)) {
> >  		dev_priv->display.load_luts = cherryview_load_luts;
> >  	} else if (IS_HASWELL(dev_priv)) {
> > -		dev_priv->display.load_luts = haswell_load_luts;
> > +		dev_priv->display.load_luts = i9xx_load_luts;
> >  		dev_priv->display.color_commit = hsw_color_commit;
> >  	} else if (IS_BROADWELL(dev_priv) || IS_GEN9_BC(dev_priv) ||
> >  		   IS_BROXTON(dev_priv)) {
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 96c78566b8e6..1caee4128974 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5299,24 +5299,54 @@ intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
> >  static bool hsw_pre_update_disable_ips(const struct intel_crtc_state *old_crtc_state,
> >  				       const struct intel_crtc_state *new_crtc_state)
> >  {
> > +	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->base.crtc);
> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > +
> >  	if (!old_crtc_state->ips_enabled)
> >  		return false;
> >  
> >  	if (needs_modeset(&new_crtc_state->base))
> >  		return true;
> >  
> > +	/*
> > +	 * Workaround : Do not read or write the pipe palette/gamma data while
> > +	 * GAMMA_MODE is configured for split gamma and IPS_CTL has IPS enabled.
> > +	 *
> > +	 * Disable IPS before we program the LUT.
> > +	 */
> > +	if (IS_HASWELL(dev_priv) &&
> > +	    (new_crtc_state->base.color_mgmt_changed ||
> > +	     new_crtc_state->update_pipe) &&
> > +	    new_crtc_state->gamma_mode == GAMMA_MODE_MODE_SPLIT)
> 
> Wouldn't we want old_crtc_state for the gamma_mode test?  We need to
> disable IPS if we're already in split gamma mode (inherited from BIOS),
> regardless of whether we're moving to non-split gamma.
> 
> 
> Matt
> 
> > +		return true;
> > +
> >  	return !new_crtc_state->ips_enabled;
> >  }
> >  
> >  static bool hsw_post_update_enable_ips(const struct intel_crtc_state *old_crtc_state,
> >  				       const struct intel_crtc_state *new_crtc_state)
> >  {
> > +	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->base.crtc);
> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > +
> >  	if (!new_crtc_state->ips_enabled)
> >  		return false;
> >  
> >  	if (needs_modeset(&new_crtc_state->base))
> >  		return true;
> >  
> > +	/*
> > +	 * Workaround : Do not read or write the pipe palette/gamma data while
> > +	 * GAMMA_MODE is configured for split gamma and IPS_CTL has IPS enabled.
> > +	 *
> > +	 * Re-enable IPS after the LUT has been programmed.
> > +	 */
> > +	if (IS_HASWELL(dev_priv) &&
> > +	    (new_crtc_state->base.color_mgmt_changed ||
> > +	     new_crtc_state->update_pipe) &&
> > +	    new_crtc_state->gamma_mode == GAMMA_MODE_MODE_SPLIT)
> > +		return true;
> > +
> >  	/*
> >  	 * We can't read out IPS on broadwell, assume the worst and
> >  	 * forcibly enable IPS on the first fastset.
> > @@ -11050,7 +11080,7 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> >  			return ret;
> >  	}
> >  
> > -	if (crtc_state->color_mgmt_changed) {
> > +	if (mode_changed || crtc_state->color_mgmt_changed) {
> >  		ret = intel_color_check(pipe_config);
> >  		if (ret)
> >  			return ret;
> > @@ -13117,6 +13147,16 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> >  	 */
> >  	drm_atomic_helper_wait_for_flip_done(dev, state);
> >  
> > +	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> > +		new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
> > +
> > +		if (new_crtc_state->active &&
> > +		    !needs_modeset(new_crtc_state) &&
> > +		    (new_intel_crtc_state->base.color_mgmt_changed ||
> > +		     new_intel_crtc_state->update_pipe))
> > +			intel_color_load_luts(new_intel_crtc_state);
> > +	}
> > +
> >  	/*
> >  	 * Now that the vblank has passed, we can go ahead and program the
> >  	 * optimal watermarks on platforms that need two-step watermark
> > @@ -13632,11 +13672,6 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
> >  		intel_atomic_get_new_crtc_state(old_intel_state, intel_crtc);
> >  	bool modeset = needs_modeset(&intel_cstate->base);
> >  
> > -	if (!modeset &&
> > -	    (intel_cstate->base.color_mgmt_changed ||
> > -	     intel_cstate->update_pipe))
> > -		intel_color_load_luts(intel_cstate);
> > -
> >  	/* Perform vblank evasion around commit operation */
> >  	intel_pipe_update_start(intel_cstate);
> >  
> > -- 
> > 2.19.2
> > 
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

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