diff mbox series

[v2,06/13] drm/i915: Move LUT programming to happen after vblank waits

Message ID 20190205160848.24662-7-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series Enable/disable gamma/csc dynamically and fix C8 | expand

Commit Message

Ville Syrjälä Feb. 5, 2019, 4:08 p.m. UTC 
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.

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(-)

Comments

Maarten Lankhorst Feb. 7, 2019, 3:46 p.m. UTC | #1 
Hey,

Op 05-02-2019 om 17:08 schreef Ville Syrjala:
> 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.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Hmm I guess there is no good way to do this, either before or after. :/

The idea was that we do it before was to make sure that the frame after the flip would be perfect. I guess

we can't be sure, and after is as good as before..

For patch 1-6:

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Ville Syrjälä Feb. 7, 2019, 7:24 p.m. UTC | #2 
On Thu, Feb 07, 2019 at 04:46:54PM +0100, Maarten Lankhorst wrote:
> Hey,
> 
> Op 05-02-2019 om 17:08 schreef Ville Syrjala:
> > 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.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Hmm I guess there is no good way to do this, either before or after. :/
> 
> The idea was that we do it before was to make sure that the frame after the flip would be perfect. I guess
> 
> we can't be sure, and after is as good as before..

After is much better than before since we have actually have a
chance that we're inside the correct vblank interval. Doing it before
guarantees that we're either in the middle of the previous frame or in
the vblank before the previous frame (which would make the entire
previous frame use the wrong LUT values).

Anyways, it should be mostly OK for single pipe updates since
we'll just wait for the vblank of that one pipe, and thus should still
be in the vblank for the LUT reprogramming. For multi-pipe we'd need to
get significantly more lucky.

> 
> For patch 1-6:
> 
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index c06bf1c687f9..2a371eed8061 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);
@@ -675,7 +652,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 5a8a36a552d4..ad5d39d81d6e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5346,24 +5346,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)
+		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.
@@ -11117,7 +11147,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;
@@ -13210,6 +13240,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
@@ -13725,11 +13765,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);