[PATCH 0/5] Fix recent watermark breakage

[PATCH 0/5] Fix recent watermark breakage

[Matt Roper]

A couple recent changes to the display watermark code by Tvrtko and me haven't
played well together and resulted in a situation where we can miscalculate
watermarks or even panic the kernel by disabling/enabling the primary display
plane.  This series should clean up the problem areas and prevent these issues
until the proper atomic watermark rework is ready.

Matt Roper (5):
  drm/i915: Ensure crtc_state backpointer is initialized
  drm/i915: Kill intel_crtc->active
  drm/i915: Update intel_crtc_active() to use state values
  drm/i915: Use crtc->state->active in ilk/skl watermark calculations
  drm/i915: Don't assume primary & cursor are always on for wm
    calculation (v3)

 drivers/gpu/drm/i915/i915_debugfs.c       |   9 +-
 drivers/gpu/drm/i915/i915_irq.c           |   2 +-
 drivers/gpu/drm/i915/intel_atomic_plane.c |   4 +-
 drivers/gpu/drm/i915/intel_display.c      | 134 ++++++++++++++--------------
 drivers/gpu/drm/i915/intel_dp.c           |   4 +-
 drivers/gpu/drm/i915/intel_drv.h          |   6 --
 drivers/gpu/drm/i915/intel_fbdev.c        |   6 +-
 drivers/gpu/drm/i915/intel_overlay.c      |   2 +-
 drivers/gpu/drm/i915/intel_pm.c           | 139 ++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_sprite.c       |   6 +-
 10 files changed, 178 insertions(+), 134 deletions(-)

-- 
1.8.5.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/5] drm/i915: Ensure crtc_state backpointer is initialized

[Matt Roper]

From: Matt Roper <matt@mattrope.com>

Since our legacy modeset paths directly kzalloc CRTC state objects at
the moment rather than calling intel_crtc_duplicate_state(), we need to
manually ensure the ->crtc backpointer is always initialized.

Signed-off-by: Matt Roper <matt@mattrope.com>
---
 drivers/gpu/drm/i915/intel_display.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 43d3575..188f87f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11228,6 +11228,7 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 		/* mode_set/enable/disable functions rely on a correct pipe
 		 * config. */
 		intel_crtc_set_state(to_intel_crtc(crtc), pipe_config);
+                pipe_config->base.crtc = &intel_crtc->base;
 
 		/*
 		 * Calculate and store various constants which
-- 
1.8.5.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915: Ensure crtc_state backpointer is initialized

[Daniel Vetter]

On Sun, Mar 08, 2015 at 02:00:41PM -0700, Matt Roper wrote:
> From: Matt Roper <matt@mattrope.com>
> 
> Since our legacy modeset paths directly kzalloc CRTC state objects at
> the moment rather than calling intel_crtc_duplicate_state(), we need to
> manually ensure the ->crtc backpointer is always initialized.
> 
> Signed-off-by: Matt Roper <matt@mattrope.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 43d3575..188f87f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11228,6 +11228,7 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>  		/* mode_set/enable/disable functions rely on a correct pipe
>  		 * config. */
>  		intel_crtc_set_state(to_intel_crtc(crtc), pipe_config);
> +                pipe_config->base.crtc = &intel_crtc->base;

Spaces instead of tabs. For coordination could we just pick up the patch
from Ander's series instead? Or is that in the part that needs slight
reworking?
-Daniel

>  
>  		/*
>  		 * Calculate and store various constants which
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/5] drm/i915: Kill intel_crtc->active

[Matt Roper]

Replace all uses of intel_crtc->active with crtc->state->active.  At the
moment we don't have atomic state handling wired up for CRTC's so this
means we just directly set the state active field at various points in
our (legacy) modeset pipeline.  Once we have CRTC atomic states properly
hooked up, crtc_state->active will be set during the check phase.

Patch generated with Coccinelle via:

        // If we already have the base pointer, don't access it through
        // intel_crtc->base needlessly.
        @@
        expression E;
        struct intel_crtc *IC;
        @@
        IC = to_intel_crtc(E);
        <+...
        - IC->active
        + E->state->active
        ...+>

        // Not sure why the rule above doesn't catch these, but this seems
        // to mop up the remainders.
        @@
        identifier I;
        expression E;
        @@
        {
        struct intel_crtc *I = to_intel_crtc(E);
        <+...
        - I->active
        + E->state->active
        ...+>
        }

        // We might have left an unused intel_crtc in some functions; clean that up.
        @@
        identifier IC;
        constant C;
        @@
        {
        ...
        - struct intel_crtc *IC;
        ... when != IC
        }

        // Change all of the other uses of intel_crtc->active
        @@
        struct intel_crtc *IC;
        @@
        - IC->active
        + IC->base.state->active

        // Ditto
        @@
        expression E;
        @@
        - to_intel_crtc(E)->active
        + E->state->active

        // Remove the active field from intel_crtc
        @@ @@
        struct intel_crtc {
        ...
        - bool active;
        ...
        };

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c       |   9 ++-
 drivers/gpu/drm/i915/i915_irq.c           |   2 +-
 drivers/gpu/drm/i915/intel_atomic_plane.c |   4 +-
 drivers/gpu/drm/i915/intel_display.c      | 129 +++++++++++++++---------------
 drivers/gpu/drm/i915/intel_dp.c           |   4 +-
 drivers/gpu/drm/i915/intel_drv.h          |   6 --
 drivers/gpu/drm/i915/intel_fbdev.c        |   6 +-
 drivers/gpu/drm/i915/intel_overlay.c      |   2 +-
 drivers/gpu/drm/i915/intel_pm.c           |  10 +--
 drivers/gpu/drm/i915/intel_sprite.c       |   6 +-
 10 files changed, 86 insertions(+), 92 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 3f64786..28176fd 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2656,9 +2656,10 @@ static int i915_display_info(struct seq_file *m, void *unused)
 
 		seq_printf(m, "CRTC %d: pipe: %c, active=%s (size=%dx%d)\n",
 			   crtc->base.base.id, pipe_name(crtc->pipe),
-			   yesno(crtc->active), crtc->config->pipe_src_w,
+			   yesno(crtc->base.state->active),
+			   crtc->config->pipe_src_w,
 			   crtc->config->pipe_src_h);
-		if (crtc->active) {
+		if (crtc->base.state->active) {
 			intel_crtc_info(m, crtc);
 
 			active = cursor_position(dev, crtc->pipe, &x, &y);
@@ -2951,7 +2952,7 @@ static int i915_drrs_status(struct seq_file *m, void *unused)
 	for_each_intel_crtc(dev, intel_crtc) {
 		drm_modeset_lock(&intel_crtc->base.mutex, NULL);
 
-		if (intel_crtc->active) {
+		if (intel_crtc->base.state->active) {
 			active_crtc_cnt++;
 			seq_printf(m, "\nCRTC %d:  ", active_crtc_cnt);
 
@@ -3650,7 +3651,7 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 				 pipe_name(pipe));
 
 		drm_modeset_lock(&crtc->base.mutex, NULL);
-		if (crtc->active)
+		if (crtc->base.state->active)
 			intel_wait_for_vblank(dev, pipe);
 		drm_modeset_unlock(&crtc->base.mutex);
 
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 9baecb7..ac834de 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -649,7 +649,7 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 	int ret = 0;
 	unsigned long irqflags;
 
-	if (!intel_crtc->active) {
+	if (!intel_crtc->base.state->active) {
 		DRM_DEBUG_DRIVER("trying to get scanoutpos for disabled "
 				 "pipe %c\n", pipe_name(pipe));
 		return 0;
diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index 976b891..43cb21f 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -144,9 +144,9 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
 	intel_state->clip.x1 = 0;
 	intel_state->clip.y1 = 0;
 	intel_state->clip.x2 =
-		intel_crtc->active ? intel_crtc->config->pipe_src_w : 0;
+		crtc->state->active ? intel_crtc->config->pipe_src_w : 0;
 	intel_state->clip.y2 =
-		intel_crtc->active ? intel_crtc->config->pipe_src_h : 0;
+		crtc->state->active ? intel_crtc->config->pipe_src_h : 0;
 
 	/*
 	 * Disabling a plane is always okay; we just need to update
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 188f87f..b11528f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -897,7 +897,7 @@ bool intel_crtc_active(struct drm_crtc *crtc)
 	 * We can ditch the crtc->primary->fb check as soon as we can
 	 * properly reconstruct framebuffers.
 	 */
-	return intel_crtc->active && crtc->primary->fb &&
+	return crtc->state->active && crtc->primary->fb &&
 		intel_crtc->config->base.adjusted_mode.crtc_clock;
 }
 
@@ -1591,7 +1591,7 @@ static int intel_num_dvo_pipes(struct drm_device *dev)
 	int count = 0;
 
 	for_each_intel_crtc(dev, crtc)
-		count += crtc->active &&
+		count += crtc->base.state->active &&
 			intel_pipe_has_type(crtc, INTEL_OUTPUT_DVO);
 
 	return count;
@@ -2168,7 +2168,7 @@ static void intel_disable_primary_hw_plane(struct drm_plane *plane,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
-	if (WARN_ON(!intel_crtc->active))
+	if (WARN_ON(!intel_crtc->base.state->active))
 		return;
 
 	if (!intel_crtc->primary_enabled)
@@ -2512,7 +2512,7 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
 		if (c == &intel_crtc->base)
 			continue;
 
-		if (!i->active)
+		if (!i->base.state->active)
 			continue;
 
 		obj = intel_fb_obj(c->primary->fb);
@@ -2937,15 +2937,13 @@ static void intel_update_primary_planes(struct drm_device *dev)
 	struct drm_crtc *crtc;
 
 	for_each_crtc(dev, crtc) {
-		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-
 		drm_modeset_lock(&crtc->mutex, NULL);
 		/*
 		 * FIXME: Once we have proper support for primary planes (and
 		 * disabling them without disabling the entire crtc) allow again
 		 * a NULL crtc->primary->fb.
 		 */
-		if (intel_crtc->active && crtc->primary->fb)
+		if (crtc->state->active && crtc->primary->fb)
 			dev_priv->display.update_primary_plane(crtc,
 							       crtc->primary->fb,
 							       crtc->x,
@@ -2974,7 +2972,7 @@ void intel_prepare_reset(struct drm_device *dev)
 	 * g33 docs say we should at least disable all the planes.
 	 */
 	for_each_intel_crtc(dev, crtc) {
-		if (crtc->active)
+		if (crtc->base.state->active)
 			dev_priv->display.crtc_disable(&crtc->base);
 	}
 }
@@ -3150,7 +3148,7 @@ static void intel_fdi_normal_train(struct drm_crtc *crtc)
 
 static bool pipe_has_enabled_pch(struct intel_crtc *crtc)
 {
-	return crtc->base.state->enable && crtc->active &&
+	return crtc->base.state->enable && crtc->base.state->active &&
 		crtc->config->has_pch_encoder;
 }
 
@@ -4306,7 +4304,7 @@ static void intel_crtc_load_lut(struct drm_crtc *crtc)
 	bool reenable_ips = false;
 
 	/* The clocks have to be on to load the palette. */
-	if (!crtc->state->enable || !intel_crtc->active)
+	if (!crtc->state->enable || !intel_crtc->base.state->active)
 		return;
 
 	if (!HAS_PCH_SPLIT(dev_priv->dev)) {
@@ -4421,7 +4419,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 
 	WARN_ON(!crtc->state->enable);
 
-	if (intel_crtc->active)
+	if (intel_crtc->base.state->active)
 		return;
 
 	if (intel_crtc->config->has_pch_encoder)
@@ -4439,7 +4437,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 
 	ironlake_set_pipeconf(crtc);
 
-	intel_crtc->active = true;
+	intel_crtc->base.state->active = true;
 
 	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
 	intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
@@ -4504,7 +4502,7 @@ static void haswell_mode_set_planes_workaround(struct intel_crtc *crtc)
 	/* We want to get the other_active_crtc only if there's only 1 other
 	 * active crtc. */
 	for_each_intel_crtc(dev, crtc_it) {
-		if (!crtc_it->active || crtc_it == crtc)
+		if (!crtc_it->base.state->active || crtc_it == crtc)
 			continue;
 
 		if (other_active_crtc)
@@ -4529,7 +4527,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 
 	WARN_ON(!crtc->state->enable);
 
-	if (intel_crtc->active)
+	if (intel_crtc->base.state->active)
 		return;
 
 	if (intel_crtc_to_shared_dpll(intel_crtc))
@@ -4554,7 +4552,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 
 	intel_set_pipe_csc(crtc);
 
-	intel_crtc->active = true;
+	intel_crtc->base.state->active = true;
 
 	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
 	for_each_encoder_on_crtc(dev, crtc, encoder)
@@ -4645,7 +4643,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 	int pipe = intel_crtc->pipe;
 	u32 reg, temp;
 
-	if (!intel_crtc->active)
+	if (!intel_crtc->base.state->active)
 		return;
 
 	intel_crtc_disable_planes(crtc);
@@ -4693,7 +4691,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 		ironlake_fdi_pll_disable(intel_crtc);
 	}
 
-	intel_crtc->active = false;
+	intel_crtc->base.state->active = false;
 	intel_update_watermarks(crtc);
 
 	mutex_lock(&dev->struct_mutex);
@@ -4709,7 +4707,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 	struct intel_encoder *encoder;
 	enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
 
-	if (!intel_crtc->active)
+	if (!intel_crtc->base.state->active)
 		return;
 
 	intel_crtc_disable_planes(crtc);
@@ -4748,7 +4746,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 		if (encoder->post_disable)
 			encoder->post_disable(encoder);
 
-	intel_crtc->active = false;
+	intel_crtc->base.state->active = false;
 	intel_update_watermarks(crtc);
 
 	mutex_lock(&dev->struct_mutex);
@@ -5137,7 +5135,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 
 	WARN_ON(!crtc->state->enable);
 
-	if (intel_crtc->active)
+	if (intel_crtc->base.state->active)
 		return;
 
 	is_dsi = intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DSI);
@@ -5163,7 +5161,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 
 	i9xx_set_pipeconf(intel_crtc);
 
-	intel_crtc->active = true;
+	intel_crtc->base.state->active = true;
 
 	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
 
@@ -5220,7 +5218,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 
 	WARN_ON(!crtc->state->enable);
 
-	if (intel_crtc->active)
+	if (intel_crtc->base.state->active)
 		return;
 
 	i9xx_set_pll_dividers(intel_crtc);
@@ -5232,7 +5230,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 
 	i9xx_set_pipeconf(intel_crtc);
 
-	intel_crtc->active = true;
+	intel_crtc->base.state->active = true;
 
 	if (!IS_GEN2(dev))
 		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
@@ -5295,7 +5293,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	struct intel_encoder *encoder;
 	int pipe = intel_crtc->pipe;
 
-	if (!intel_crtc->active)
+	if (!intel_crtc->base.state->active)
 		return;
 
 	/*
@@ -5353,7 +5351,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	if (!IS_GEN2(dev))
 		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
 
-	intel_crtc->active = false;
+	intel_crtc->base.state->active = false;
 	intel_update_watermarks(crtc);
 
 	mutex_lock(&dev->struct_mutex);
@@ -5375,7 +5373,7 @@ void intel_crtc_control(struct drm_crtc *crtc, bool enable)
 	unsigned long domains;
 
 	if (enable) {
-		if (!intel_crtc->active) {
+		if (!intel_crtc->base.state->active) {
 			domains = get_crtc_power_domains(crtc);
 			for_each_power_domain(domain, domains)
 				intel_display_power_get(dev_priv, domain);
@@ -5384,7 +5382,7 @@ void intel_crtc_control(struct drm_crtc *crtc, bool enable)
 			dev_priv->display.crtc_enable(crtc);
 		}
 	} else {
-		if (intel_crtc->active) {
+		if (intel_crtc->base.state->active) {
 			dev_priv->display.crtc_disable(crtc);
 
 			domains = intel_crtc->enabled_power_domains;
@@ -5497,7 +5495,8 @@ static void intel_connector_check_state(struct intel_connector *connector)
 
 			I915_STATE_WARN(!crtc->state->enable,
 					"crtc not enabled\n");
-			I915_STATE_WARN(!to_intel_crtc(crtc)->active, "crtc not active\n");
+			I915_STATE_WARN(!crtc->state->active,
+					"crtc not active\n");
 			I915_STATE_WARN(pipe != to_intel_crtc(crtc)->pipe,
 			     "encoder active on the wrong pipe\n");
 		}
@@ -8032,7 +8031,8 @@ static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv)
 	struct intel_crtc *crtc;
 
 	for_each_intel_crtc(dev, crtc)
-		I915_STATE_WARN(crtc->active, "CRTC for pipe %c enabled\n",
+		I915_STATE_WARN(crtc->base.state->active,
+				"CRTC for pipe %c enabled\n",
 		     pipe_name(crtc->pipe));
 
 	I915_STATE_WARN(I915_READ(HSW_PWR_WELL_DRIVER), "Power well on\n");
@@ -10816,7 +10816,7 @@ static void check_wm_state(struct drm_device *dev)
 		struct skl_ddb_entry *hw_entry, *sw_entry;
 		const enum pipe pipe = intel_crtc->pipe;
 
-		if (!intel_crtc->active)
+		if (!intel_crtc->base.state->active)
 			continue;
 
 		/* planes */
@@ -10945,7 +10945,7 @@ check_crtc_state(struct drm_device *dev)
 		DRM_DEBUG_KMS("[CRTC:%d]\n",
 			      crtc->base.base.id);
 
-		I915_STATE_WARN(crtc->active && !crtc->base.state->enable,
+		I915_STATE_WARN(crtc->base.state->active && !crtc->base.state->enable,
 		     "active crtc, but not enabled in sw tracking\n");
 
 		for_each_intel_encoder(dev, encoder) {
@@ -10956,9 +10956,10 @@ check_crtc_state(struct drm_device *dev)
 				active = true;
 		}
 
-		I915_STATE_WARN(active != crtc->active,
+		I915_STATE_WARN(active != crtc->base.state->active,
 		     "crtc's computed active state doesn't match tracked active state "
-		     "(expected %i, found %i)\n", active, crtc->active);
+		     "(expected %i, found %i)\n", active,
+				crtc->base.state->active);
 		I915_STATE_WARN(enabled != crtc->base.state->enable,
 		     "crtc's computed enabled state doesn't match tracked enabled state "
 		     "(expected %i, found %i)\n", enabled,
@@ -10970,7 +10971,7 @@ check_crtc_state(struct drm_device *dev)
 		/* hw state is inconsistent with the pipe quirk */
 		if ((crtc->pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) ||
 		    (crtc->pipe == PIPE_B && dev_priv->quirks & QUIRK_PIPEB_FORCE))
-			active = crtc->active;
+			active = crtc->base.state->active;
 
 		for_each_intel_encoder(dev, encoder) {
 			enum pipe pipe;
@@ -10980,9 +10981,10 @@ check_crtc_state(struct drm_device *dev)
 				encoder->get_config(encoder, &pipe_config);
 		}
 
-		I915_STATE_WARN(crtc->active != active,
+		I915_STATE_WARN(crtc->base.state->active != active,
 		     "crtc active state doesn't match with hw state "
-		     "(expected %i, found %i)\n", crtc->active, active);
+		     "(expected %i, found %i)\n", crtc->base.state->active,
+				active);
 
 		if (active &&
 		    !intel_pipe_config_compare(dev, crtc->config, &pipe_config)) {
@@ -11028,7 +11030,7 @@ check_shared_dpll_state(struct drm_device *dev)
 		for_each_intel_crtc(dev, crtc) {
 			if (crtc->base.state->enable && intel_crtc_to_shared_dpll(crtc) == pll)
 				enabled_crtcs++;
-			if (crtc->active && intel_crtc_to_shared_dpll(crtc) == pll)
+			if (crtc->base.state->active && intel_crtc_to_shared_dpll(crtc) == pll)
 				active_crtcs++;
 		}
 		I915_STATE_WARN(pll->active != active_crtcs,
@@ -11449,10 +11451,7 @@ intel_set_config_compute_mode_changes(struct drm_mode_set *set,
 		 * in fastboot situations.
 		 */
 		if (set->crtc->primary->fb == NULL) {
-			struct intel_crtc *intel_crtc =
-				to_intel_crtc(set->crtc);
-
-			if (intel_crtc->active) {
+			if (set->crtc->state->active) {
 				DRM_DEBUG_KMS("crtc has no fb, will flip\n");
 				config->fb_changed = true;
 			} else {
@@ -11738,7 +11737,7 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
 		 * has previously been turned off.
 		 */
 		if (!intel_crtc->primary_enabled && ret == 0) {
-			WARN_ON(!intel_crtc->active);
+			WARN_ON(!set->crtc->state->active);
 			intel_enable_primary_hw_plane(set->crtc->primary, set->crtc);
 		}
 
@@ -12001,7 +12000,7 @@ intel_check_primary_plane(struct drm_plane *plane,
 	if (ret)
 		return ret;
 
-	if (intel_crtc->active) {
+	if (crtc->state->active) {
 		intel_crtc->atomic.wait_for_flips = true;
 
 		/*
@@ -12068,7 +12067,7 @@ intel_commit_primary_plane(struct drm_plane *plane,
 
 	intel_plane->obj = obj;
 
-	if (intel_crtc->active) {
+	if (crtc->state->active) {
 		if (state->visible) {
 			/* FIXME: kill this fastboot hack */
 			intel_update_pipe_size(intel_crtc);
@@ -12138,7 +12137,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
 	intel_runtime_pm_get(dev_priv);
 
 	/* Perform vblank evasion around commit operation */
-	if (intel_crtc->active)
+	if (intel_crtc->base.state->active)
 		intel_crtc->atomic.evade =
 			intel_pipe_update_start(intel_crtc,
 						&intel_crtc->atomic.start_vbl_count);
@@ -12313,7 +12312,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
 	}
 
 finish:
-	if (intel_crtc->active) {
+	if (crtc->state->active) {
 		if (intel_crtc->base.cursor->state->crtc_w != state->base.crtc_w)
 			intel_crtc->atomic.update_wm = true;
 
@@ -12358,7 +12357,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 	intel_crtc->cursor_bo = obj;
 update:
 
-	if (intel_crtc->active)
+	if (crtc->state->active)
 		intel_crtc_update_cursor(crtc, state->visible);
 }
 
@@ -13399,7 +13398,7 @@ void intel_modeset_init(struct drm_device *dev)
 	drm_modeset_unlock_all(dev);
 
 	for_each_intel_crtc(dev, crtc) {
-		if (!crtc->active)
+		if (!crtc->base.state->active)
 			continue;
 
 		/*
@@ -13477,7 +13476,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 
 	/* restore vblank interrupts to correct state */
 	drm_crtc_vblank_reset(&crtc->base);
-	if (crtc->active) {
+	if (crtc->base.state->active) {
 		update_scanline_offset(crtc);
 		drm_crtc_vblank_on(&crtc->base);
 	}
@@ -13517,13 +13516,13 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 				connector->encoder->connectors_active = false;
 			}
 
-		WARN_ON(crtc->active);
+		WARN_ON(crtc->base.state->active);
 		crtc->base.state->enable = false;
 		crtc->base.enabled = false;
 	}
 
 	if (dev_priv->quirks & QUIRK_PIPEA_FORCE &&
-	    crtc->pipe == PIPE_A && !crtc->active) {
+	    crtc->pipe == PIPE_A && !crtc->base.state->active) {
 		/* BIOS forgot to enable pipe A, this mostly happens after
 		 * resume. Force-enable the pipe to fix this, the update_dpms
 		 * call below we restore the pipe to the right state, but leave
@@ -13535,7 +13534,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 	 * have active connectors/encoders. */
 	intel_crtc_update_dpms(&crtc->base);
 
-	if (crtc->active != crtc->base.state->enable) {
+	if (crtc->base.state->active != crtc->base.state->enable) {
 		struct intel_encoder *encoder;
 
 		/* This can happen either due to bugs in the get_hw_state
@@ -13544,17 +13543,17 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 		DRM_DEBUG_KMS("[CRTC:%d] hw state adjusted, was %s, now %s\n",
 			      crtc->base.base.id,
 			      crtc->base.state->enable ? "enabled" : "disabled",
-			      crtc->active ? "enabled" : "disabled");
+			      crtc->base.state->active ? "enabled" : "disabled");
 
-		crtc->base.state->enable = crtc->active;
-		crtc->base.enabled = crtc->active;
+		crtc->base.state->enable = crtc->base.state->active;
+		crtc->base.enabled = crtc->base.state->active;
 
 		/* Because we only establish the connector -> encoder ->
 		 * crtc links if something is active, this means the
 		 * crtc is now deactivated. Break the links. connector
 		 * -> encoder links are only establish when things are
 		 *  actually up, hence no need to break them. */
-		WARN_ON(crtc->active);
+		WARN_ON(crtc->base.state->active);
 
 		for_each_encoder_on_crtc(dev, &crtc->base, encoder) {
 			WARN_ON(encoder->connectors_active);
@@ -13562,7 +13561,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 		}
 	}
 
-	if (crtc->active || HAS_GMCH_DISPLAY(dev)) {
+	if (crtc->base.state->active || HAS_GMCH_DISPLAY(dev)) {
 		/*
 		 * We start out with underrun reporting disabled to avoid races.
 		 * For correct bookkeeping mark this on active crtcs.
@@ -13590,7 +13589,7 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
 	 * encoder is active and trying to read from a pipe) and the
 	 * pipe itself being active. */
 	bool has_active_crtc = encoder->base.crtc &&
-		to_intel_crtc(encoder->base.crtc)->active;
+		encoder->base.crtc->state->active;
 
 	if (encoder->connectors_active && !has_active_crtc) {
 		DRM_DEBUG_KMS("[ENCODER:%d:%s] has active connectors but no active pipe!\n",
@@ -13658,7 +13657,7 @@ static bool primary_get_hw_state(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
 
-	if (!crtc->active)
+	if (!crtc->base.state->active)
 		return false;
 
 	return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE;
@@ -13678,16 +13677,16 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 
 		crtc->config->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE;
 
-		crtc->active = dev_priv->display.get_pipe_config(crtc,
+		crtc->base.state->active = dev_priv->display.get_pipe_config(crtc,
 								 crtc->config);
 
-		crtc->base.state->enable = crtc->active;
-		crtc->base.enabled = crtc->active;
+		crtc->base.state->enable = crtc->base.state->active;
+		crtc->base.enabled = crtc->base.state->active;
 		crtc->primary_enabled = primary_get_hw_state(crtc);
 
 		DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
 			      crtc->base.base.id,
-			      crtc->active ? "enabled" : "disabled");
+			      crtc->base.state->active ? "enabled" : "disabled");
 	}
 
 	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
@@ -13698,7 +13697,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 		pll->active = 0;
 		pll->config.crtc_mask = 0;
 		for_each_intel_crtc(dev, crtc) {
-			if (crtc->active && intel_crtc_to_shared_dpll(crtc) == pll) {
+			if (crtc->base.state->active && intel_crtc_to_shared_dpll(crtc) == pll) {
 				pll->active++;
 				pll->config.crtc_mask |= 1 << crtc->pipe;
 			}
@@ -13765,7 +13764,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 	 * checking everywhere.
 	 */
 	for_each_intel_crtc(dev, crtc) {
-		if (crtc->active && i915.fastboot) {
+		if (crtc->base.state->active && i915.fastboot) {
 			intel_mode_from_pipe_config(&crtc->base.mode,
 						    crtc->config);
 			DRM_DEBUG_KMS("[CRTC:%d] found active mode: ",
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 33d5877..e4f447f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3974,7 +3974,7 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 	if (WARN_ON(!intel_encoder->base.crtc))
 		return;
 
-	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
+	if (!intel_encoder->base.crtc->state->active)
 		return;
 
 	/* Try to read receiver status if the link appears to be up */
@@ -4955,7 +4955,7 @@ static void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
 		return;
 	}
 
-	if (!intel_crtc->active) {
+	if (!intel_crtc->base.state->active) {
 		DRM_DEBUG_KMS("eDP encoder disabled. CRTC not Active\n");
 		return;
 	}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ff79dca..d194948 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -443,12 +443,6 @@ struct intel_crtc {
 	enum pipe pipe;
 	enum plane plane;
 	u8 lut_r[256], lut_g[256], lut_b[256];
-	/*
-	 * Whether the crtc and the connected output pipeline is active. Implies
-	 * that crtc->enabled is set, i.e. the current mode configuration has
-	 * some outputs connected to this crtc.
-	 */
-	bool active;
 	unsigned long enabled_power_domains;
 	bool primary_enabled; /* is the primary plane (partially) visible? */
 	bool lowfreq_avail;
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 234a699..192d229 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -541,7 +541,7 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
 	for_each_crtc(dev, crtc) {
 		intel_crtc = to_intel_crtc(crtc);
 
-		if (!intel_crtc->active || !crtc->primary->fb) {
+		if (!intel_crtc->base.state->active || !crtc->primary->fb) {
 			DRM_DEBUG_KMS("pipe %c not active or no fb, skipping\n",
 				      pipe_name(intel_crtc->pipe));
 			continue;
@@ -567,7 +567,7 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
 
 		intel_crtc = to_intel_crtc(crtc);
 
-		if (!intel_crtc->active) {
+		if (!intel_crtc->base.state->active) {
 			DRM_DEBUG_KMS("pipe %c not active, skipping\n",
 				      pipe_name(intel_crtc->pipe));
 			continue;
@@ -632,7 +632,7 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
 	for_each_crtc(dev, crtc) {
 		intel_crtc = to_intel_crtc(crtc);
 
-		if (!intel_crtc->active)
+		if (!intel_crtc->base.state->active)
 			continue;
 
 		WARN(!crtc->primary->fb,
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index 823d1d9..050a68e 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -852,7 +852,7 @@ int intel_overlay_switch_off(struct intel_overlay *overlay)
 static int check_overlay_possible_on_crtc(struct intel_overlay *overlay,
 					  struct intel_crtc *crtc)
 {
-	if (!crtc->active)
+	if (!crtc->base.state->active)
 		return -EINVAL;
 
 	/* can't use the overlay with double wide pipe */
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 4f04fab..d9b115e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -822,7 +822,7 @@ static uint8_t vlv_compute_drain_latency(struct drm_crtc *crtc,
 	 * FIXME the plane might have an fb
 	 * but be invisible (eg. due to clipping)
 	 */
-	if (!intel_crtc->active || !plane->state->fb)
+	if (!intel_crtc->base.state->active || !plane->state->fb)
 		return 0;
 
 	if (WARN(clock == 0, "Pixel clock is zero!\n"))
@@ -3080,7 +3080,7 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
 	 * re-allocate the freed space without this pipe fetching from it.
 	 */
 	for_each_intel_crtc(dev, crtc) {
-		if (!crtc->active)
+		if (!crtc->base.state->active)
 			continue;
 
 		pipe = crtc->pipe;
@@ -3103,7 +3103,7 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
 	 * space is not used anymore.
 	 */
 	for_each_intel_crtc(dev, crtc) {
-		if (!crtc->active)
+		if (!crtc->base.state->active)
 			continue;
 
 		pipe = crtc->pipe;
@@ -3126,7 +3126,7 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
 	 * will just get more DDB space with the correct WM values.
 	 */
 	for_each_intel_crtc(dev, crtc) {
-		if (!crtc->active)
+		if (!crtc->base.state->active)
 			continue;
 
 		pipe = crtc->pipe;
@@ -3191,7 +3191,7 @@ static void skl_update_other_pipe_wm(struct drm_device *dev,
 		if (this_crtc->pipe == intel_crtc->pipe)
 			continue;
 
-		if (!intel_crtc->active)
+		if (!intel_crtc->base.state->active)
 			continue;
 
 		wm_changed = skl_update_pipe_wm(&intel_crtc->base,
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 7051da7..7ad9b7d 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1242,9 +1242,9 @@ finish:
 	 */
 	state->hides_primary = fb != NULL && drm_rect_equals(dst, clip) &&
 		!colorkey_enabled(intel_plane);
-	WARN_ON(state->hides_primary && !state->visible && intel_crtc->active);
+	WARN_ON(state->hides_primary && !state->visible && state->base.crtc->state->active);
 
-	if (intel_crtc->active) {
+	if (intel_crtc->base.state->active) {
 		if (intel_crtc->primary_enabled == state->hides_primary)
 			intel_crtc->atomic.wait_for_flips = true;
 
@@ -1286,7 +1286,7 @@ intel_commit_sprite_plane(struct drm_plane *plane,
 	plane->fb = state->base.fb;
 	intel_plane->obj = obj;
 
-	if (intel_crtc->active) {
+	if (crtc->state->active) {
 		intel_crtc->primary_enabled = !state->hides_primary;
 
 		if (state->visible) {
-- 
1.8.5.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915: Kill intel_crtc->active

[Ville Syrjälä]

On Sun, Mar 08, 2015 at 02:00:42PM -0700, Matt Roper wrote:
> Replace all uses of intel_crtc->active with crtc->state->active.  At the
> moment we don't have atomic state handling wired up for CRTC's so this
> means we just directly set the state active field at various points in
> our (legacy) modeset pipeline.  Once we have CRTC atomic states properly
> hooked up, crtc_state->active will be set during the check phase.

I'm not at all sure we want to do this change, or at the very least I'd
not do it through a mass rename like this. I think we need to evaluate
each case separately.

The most important question is what are the planned semantics of
crtc->state->active, and do they match the semantics of intel_crtc->active.
intel_crtc->active is that actual pipe state, and that means it'll change
in the middle of the modeset sequence, and not before or after it. And since
crtc->state is going to be swapped in via the atomic code as a whole
(before the commit assuming the pattern from the plane code holds) it would
already change before the modeset sequence started.

> 
> Patch generated with Coccinelle via:
> 
>         // If we already have the base pointer, don't access it through
>         // intel_crtc->base needlessly.
>         @@
>         expression E;
>         struct intel_crtc *IC;
>         @@
>         IC = to_intel_crtc(E);
>         <+...
>         - IC->active
>         + E->state->active
>         ...+>
> 
>         // Not sure why the rule above doesn't catch these, but this seems
>         // to mop up the remainders.
>         @@
>         identifier I;
>         expression E;
>         @@
>         {
>         struct intel_crtc *I = to_intel_crtc(E);
>         <+...
>         - I->active
>         + E->state->active
>         ...+>
>         }
> 
>         // We might have left an unused intel_crtc in some functions; clean that up.
>         @@
>         identifier IC;
>         constant C;
>         @@
>         {
>         ...
>         - struct intel_crtc *IC;
>         ... when != IC
>         }
> 
>         // Change all of the other uses of intel_crtc->active
>         @@
>         struct intel_crtc *IC;
>         @@
>         - IC->active
>         + IC->base.state->active
> 
>         // Ditto
>         @@
>         expression E;
>         @@
>         - to_intel_crtc(E)->active
>         + E->state->active
> 
>         // Remove the active field from intel_crtc
>         @@ @@
>         struct intel_crtc {
>         ...
>         - bool active;
>         ...
>         };
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c       |   9 ++-
>  drivers/gpu/drm/i915/i915_irq.c           |   2 +-
>  drivers/gpu/drm/i915/intel_atomic_plane.c |   4 +-
>  drivers/gpu/drm/i915/intel_display.c      | 129 +++++++++++++++---------------
>  drivers/gpu/drm/i915/intel_dp.c           |   4 +-
>  drivers/gpu/drm/i915/intel_drv.h          |   6 --
>  drivers/gpu/drm/i915/intel_fbdev.c        |   6 +-
>  drivers/gpu/drm/i915/intel_overlay.c      |   2 +-
>  drivers/gpu/drm/i915/intel_pm.c           |  10 +--
>  drivers/gpu/drm/i915/intel_sprite.c       |   6 +-
>  10 files changed, 86 insertions(+), 92 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 3f64786..28176fd 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2656,9 +2656,10 @@ static int i915_display_info(struct seq_file *m, void *unused)
>  
>  		seq_printf(m, "CRTC %d: pipe: %c, active=%s (size=%dx%d)\n",
>  			   crtc->base.base.id, pipe_name(crtc->pipe),
> -			   yesno(crtc->active), crtc->config->pipe_src_w,
> +			   yesno(crtc->base.state->active),
> +			   crtc->config->pipe_src_w,
>  			   crtc->config->pipe_src_h);
> -		if (crtc->active) {
> +		if (crtc->base.state->active) {
>  			intel_crtc_info(m, crtc);
>  
>  			active = cursor_position(dev, crtc->pipe, &x, &y);
> @@ -2951,7 +2952,7 @@ static int i915_drrs_status(struct seq_file *m, void *unused)
>  	for_each_intel_crtc(dev, intel_crtc) {
>  		drm_modeset_lock(&intel_crtc->base.mutex, NULL);
>  
> -		if (intel_crtc->active) {
> +		if (intel_crtc->base.state->active) {
>  			active_crtc_cnt++;
>  			seq_printf(m, "\nCRTC %d:  ", active_crtc_cnt);
>  
> @@ -3650,7 +3651,7 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
>  				 pipe_name(pipe));
>  
>  		drm_modeset_lock(&crtc->base.mutex, NULL);
> -		if (crtc->active)
> +		if (crtc->base.state->active)
>  			intel_wait_for_vblank(dev, pipe);
>  		drm_modeset_unlock(&crtc->base.mutex);
>  
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 9baecb7..ac834de 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -649,7 +649,7 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
>  	int ret = 0;
>  	unsigned long irqflags;
>  
> -	if (!intel_crtc->active) {
> +	if (!intel_crtc->base.state->active) {
>  		DRM_DEBUG_DRIVER("trying to get scanoutpos for disabled "
>  				 "pipe %c\n", pipe_name(pipe));
>  		return 0;
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index 976b891..43cb21f 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -144,9 +144,9 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>  	intel_state->clip.x1 = 0;
>  	intel_state->clip.y1 = 0;
>  	intel_state->clip.x2 =
> -		intel_crtc->active ? intel_crtc->config->pipe_src_w : 0;
> +		crtc->state->active ? intel_crtc->config->pipe_src_w : 0;
>  	intel_state->clip.y2 =
> -		intel_crtc->active ? intel_crtc->config->pipe_src_h : 0;
> +		crtc->state->active ? intel_crtc->config->pipe_src_h : 0;
>  
>  	/*
>  	 * Disabling a plane is always okay; we just need to update
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 188f87f..b11528f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -897,7 +897,7 @@ bool intel_crtc_active(struct drm_crtc *crtc)
>  	 * We can ditch the crtc->primary->fb check as soon as we can
>  	 * properly reconstruct framebuffers.
>  	 */
> -	return intel_crtc->active && crtc->primary->fb &&
> +	return crtc->state->active && crtc->primary->fb &&
>  		intel_crtc->config->base.adjusted_mode.crtc_clock;
>  }
>  
> @@ -1591,7 +1591,7 @@ static int intel_num_dvo_pipes(struct drm_device *dev)
>  	int count = 0;
>  
>  	for_each_intel_crtc(dev, crtc)
> -		count += crtc->active &&
> +		count += crtc->base.state->active &&
>  			intel_pipe_has_type(crtc, INTEL_OUTPUT_DVO);
>  
>  	return count;
> @@ -2168,7 +2168,7 @@ static void intel_disable_primary_hw_plane(struct drm_plane *plane,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  
> -	if (WARN_ON(!intel_crtc->active))
> +	if (WARN_ON(!intel_crtc->base.state->active))
>  		return;
>  
>  	if (!intel_crtc->primary_enabled)
> @@ -2512,7 +2512,7 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
>  		if (c == &intel_crtc->base)
>  			continue;
>  
> -		if (!i->active)
> +		if (!i->base.state->active)
>  			continue;
>  
>  		obj = intel_fb_obj(c->primary->fb);
> @@ -2937,15 +2937,13 @@ static void intel_update_primary_planes(struct drm_device *dev)
>  	struct drm_crtc *crtc;
>  
>  	for_each_crtc(dev, crtc) {
> -		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -
>  		drm_modeset_lock(&crtc->mutex, NULL);
>  		/*
>  		 * FIXME: Once we have proper support for primary planes (and
>  		 * disabling them without disabling the entire crtc) allow again
>  		 * a NULL crtc->primary->fb.
>  		 */
> -		if (intel_crtc->active && crtc->primary->fb)
> +		if (crtc->state->active && crtc->primary->fb)
>  			dev_priv->display.update_primary_plane(crtc,
>  							       crtc->primary->fb,
>  							       crtc->x,
> @@ -2974,7 +2972,7 @@ void intel_prepare_reset(struct drm_device *dev)
>  	 * g33 docs say we should at least disable all the planes.
>  	 */
>  	for_each_intel_crtc(dev, crtc) {
> -		if (crtc->active)
> +		if (crtc->base.state->active)
>  			dev_priv->display.crtc_disable(&crtc->base);
>  	}
>  }
> @@ -3150,7 +3148,7 @@ static void intel_fdi_normal_train(struct drm_crtc *crtc)
>  
>  static bool pipe_has_enabled_pch(struct intel_crtc *crtc)
>  {
> -	return crtc->base.state->enable && crtc->active &&
> +	return crtc->base.state->enable && crtc->base.state->active &&
>  		crtc->config->has_pch_encoder;
>  }
>  
> @@ -4306,7 +4304,7 @@ static void intel_crtc_load_lut(struct drm_crtc *crtc)
>  	bool reenable_ips = false;
>  
>  	/* The clocks have to be on to load the palette. */
> -	if (!crtc->state->enable || !intel_crtc->active)
> +	if (!crtc->state->enable || !intel_crtc->base.state->active)
>  		return;
>  
>  	if (!HAS_PCH_SPLIT(dev_priv->dev)) {
> @@ -4421,7 +4419,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>  
>  	WARN_ON(!crtc->state->enable);
>  
> -	if (intel_crtc->active)
> +	if (intel_crtc->base.state->active)
>  		return;
>  
>  	if (intel_crtc->config->has_pch_encoder)
> @@ -4439,7 +4437,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>  
>  	ironlake_set_pipeconf(crtc);
>  
> -	intel_crtc->active = true;
> +	intel_crtc->base.state->active = true;
>  
>  	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
>  	intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
> @@ -4504,7 +4502,7 @@ static void haswell_mode_set_planes_workaround(struct intel_crtc *crtc)
>  	/* We want to get the other_active_crtc only if there's only 1 other
>  	 * active crtc. */
>  	for_each_intel_crtc(dev, crtc_it) {
> -		if (!crtc_it->active || crtc_it == crtc)
> +		if (!crtc_it->base.state->active || crtc_it == crtc)
>  			continue;
>  
>  		if (other_active_crtc)
> @@ -4529,7 +4527,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  
>  	WARN_ON(!crtc->state->enable);
>  
> -	if (intel_crtc->active)
> +	if (intel_crtc->base.state->active)
>  		return;
>  
>  	if (intel_crtc_to_shared_dpll(intel_crtc))
> @@ -4554,7 +4552,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  
>  	intel_set_pipe_csc(crtc);
>  
> -	intel_crtc->active = true;
> +	intel_crtc->base.state->active = true;
>  
>  	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
> @@ -4645,7 +4643,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>  	int pipe = intel_crtc->pipe;
>  	u32 reg, temp;
>  
> -	if (!intel_crtc->active)
> +	if (!intel_crtc->base.state->active)
>  		return;
>  
>  	intel_crtc_disable_planes(crtc);
> @@ -4693,7 +4691,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>  		ironlake_fdi_pll_disable(intel_crtc);
>  	}
>  
> -	intel_crtc->active = false;
> +	intel_crtc->base.state->active = false;
>  	intel_update_watermarks(crtc);
>  
>  	mutex_lock(&dev->struct_mutex);
> @@ -4709,7 +4707,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>  	struct intel_encoder *encoder;
>  	enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
>  
> -	if (!intel_crtc->active)
> +	if (!intel_crtc->base.state->active)
>  		return;
>  
>  	intel_crtc_disable_planes(crtc);
> @@ -4748,7 +4746,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>  		if (encoder->post_disable)
>  			encoder->post_disable(encoder);
>  
> -	intel_crtc->active = false;
> +	intel_crtc->base.state->active = false;
>  	intel_update_watermarks(crtc);
>  
>  	mutex_lock(&dev->struct_mutex);
> @@ -5137,7 +5135,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>  
>  	WARN_ON(!crtc->state->enable);
>  
> -	if (intel_crtc->active)
> +	if (intel_crtc->base.state->active)
>  		return;
>  
>  	is_dsi = intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DSI);
> @@ -5163,7 +5161,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>  
>  	i9xx_set_pipeconf(intel_crtc);
>  
> -	intel_crtc->active = true;
> +	intel_crtc->base.state->active = true;
>  
>  	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
>  
> @@ -5220,7 +5218,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>  
>  	WARN_ON(!crtc->state->enable);
>  
> -	if (intel_crtc->active)
> +	if (intel_crtc->base.state->active)
>  		return;
>  
>  	i9xx_set_pll_dividers(intel_crtc);
> @@ -5232,7 +5230,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>  
>  	i9xx_set_pipeconf(intel_crtc);
>  
> -	intel_crtc->active = true;
> +	intel_crtc->base.state->active = true;
>  
>  	if (!IS_GEN2(dev))
>  		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
> @@ -5295,7 +5293,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>  	struct intel_encoder *encoder;
>  	int pipe = intel_crtc->pipe;
>  
> -	if (!intel_crtc->active)
> +	if (!intel_crtc->base.state->active)
>  		return;
>  
>  	/*
> @@ -5353,7 +5351,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>  	if (!IS_GEN2(dev))
>  		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
>  
> -	intel_crtc->active = false;
> +	intel_crtc->base.state->active = false;
>  	intel_update_watermarks(crtc);
>  
>  	mutex_lock(&dev->struct_mutex);
> @@ -5375,7 +5373,7 @@ void intel_crtc_control(struct drm_crtc *crtc, bool enable)
>  	unsigned long domains;
>  
>  	if (enable) {
> -		if (!intel_crtc->active) {
> +		if (!intel_crtc->base.state->active) {
>  			domains = get_crtc_power_domains(crtc);
>  			for_each_power_domain(domain, domains)
>  				intel_display_power_get(dev_priv, domain);
> @@ -5384,7 +5382,7 @@ void intel_crtc_control(struct drm_crtc *crtc, bool enable)
>  			dev_priv->display.crtc_enable(crtc);
>  		}
>  	} else {
> -		if (intel_crtc->active) {
> +		if (intel_crtc->base.state->active) {
>  			dev_priv->display.crtc_disable(crtc);
>  
>  			domains = intel_crtc->enabled_power_domains;
> @@ -5497,7 +5495,8 @@ static void intel_connector_check_state(struct intel_connector *connector)
>  
>  			I915_STATE_WARN(!crtc->state->enable,
>  					"crtc not enabled\n");
> -			I915_STATE_WARN(!to_intel_crtc(crtc)->active, "crtc not active\n");
> +			I915_STATE_WARN(!crtc->state->active,
> +					"crtc not active\n");
>  			I915_STATE_WARN(pipe != to_intel_crtc(crtc)->pipe,
>  			     "encoder active on the wrong pipe\n");
>  		}
> @@ -8032,7 +8031,8 @@ static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv)
>  	struct intel_crtc *crtc;
>  
>  	for_each_intel_crtc(dev, crtc)
> -		I915_STATE_WARN(crtc->active, "CRTC for pipe %c enabled\n",
> +		I915_STATE_WARN(crtc->base.state->active,
> +				"CRTC for pipe %c enabled\n",
>  		     pipe_name(crtc->pipe));
>  
>  	I915_STATE_WARN(I915_READ(HSW_PWR_WELL_DRIVER), "Power well on\n");
> @@ -10816,7 +10816,7 @@ static void check_wm_state(struct drm_device *dev)
>  		struct skl_ddb_entry *hw_entry, *sw_entry;
>  		const enum pipe pipe = intel_crtc->pipe;
>  
> -		if (!intel_crtc->active)
> +		if (!intel_crtc->base.state->active)
>  			continue;
>  
>  		/* planes */
> @@ -10945,7 +10945,7 @@ check_crtc_state(struct drm_device *dev)
>  		DRM_DEBUG_KMS("[CRTC:%d]\n",
>  			      crtc->base.base.id);
>  
> -		I915_STATE_WARN(crtc->active && !crtc->base.state->enable,
> +		I915_STATE_WARN(crtc->base.state->active && !crtc->base.state->enable,
>  		     "active crtc, but not enabled in sw tracking\n");
>  
>  		for_each_intel_encoder(dev, encoder) {
> @@ -10956,9 +10956,10 @@ check_crtc_state(struct drm_device *dev)
>  				active = true;
>  		}
>  
> -		I915_STATE_WARN(active != crtc->active,
> +		I915_STATE_WARN(active != crtc->base.state->active,
>  		     "crtc's computed active state doesn't match tracked active state "
> -		     "(expected %i, found %i)\n", active, crtc->active);
> +		     "(expected %i, found %i)\n", active,
> +				crtc->base.state->active);
>  		I915_STATE_WARN(enabled != crtc->base.state->enable,
>  		     "crtc's computed enabled state doesn't match tracked enabled state "
>  		     "(expected %i, found %i)\n", enabled,
> @@ -10970,7 +10971,7 @@ check_crtc_state(struct drm_device *dev)
>  		/* hw state is inconsistent with the pipe quirk */
>  		if ((crtc->pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) ||
>  		    (crtc->pipe == PIPE_B && dev_priv->quirks & QUIRK_PIPEB_FORCE))
> -			active = crtc->active;
> +			active = crtc->base.state->active;
>  
>  		for_each_intel_encoder(dev, encoder) {
>  			enum pipe pipe;
> @@ -10980,9 +10981,10 @@ check_crtc_state(struct drm_device *dev)
>  				encoder->get_config(encoder, &pipe_config);
>  		}
>  
> -		I915_STATE_WARN(crtc->active != active,
> +		I915_STATE_WARN(crtc->base.state->active != active,
>  		     "crtc active state doesn't match with hw state "
> -		     "(expected %i, found %i)\n", crtc->active, active);
> +		     "(expected %i, found %i)\n", crtc->base.state->active,
> +				active);
>  
>  		if (active &&
>  		    !intel_pipe_config_compare(dev, crtc->config, &pipe_config)) {
> @@ -11028,7 +11030,7 @@ check_shared_dpll_state(struct drm_device *dev)
>  		for_each_intel_crtc(dev, crtc) {
>  			if (crtc->base.state->enable && intel_crtc_to_shared_dpll(crtc) == pll)
>  				enabled_crtcs++;
> -			if (crtc->active && intel_crtc_to_shared_dpll(crtc) == pll)
> +			if (crtc->base.state->active && intel_crtc_to_shared_dpll(crtc) == pll)
>  				active_crtcs++;
>  		}
>  		I915_STATE_WARN(pll->active != active_crtcs,
> @@ -11449,10 +11451,7 @@ intel_set_config_compute_mode_changes(struct drm_mode_set *set,
>  		 * in fastboot situations.
>  		 */
>  		if (set->crtc->primary->fb == NULL) {
> -			struct intel_crtc *intel_crtc =
> -				to_intel_crtc(set->crtc);
> -
> -			if (intel_crtc->active) {
> +			if (set->crtc->state->active) {
>  				DRM_DEBUG_KMS("crtc has no fb, will flip\n");
>  				config->fb_changed = true;
>  			} else {
> @@ -11738,7 +11737,7 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  		 * has previously been turned off.
>  		 */
>  		if (!intel_crtc->primary_enabled && ret == 0) {
> -			WARN_ON(!intel_crtc->active);
> +			WARN_ON(!set->crtc->state->active);
>  			intel_enable_primary_hw_plane(set->crtc->primary, set->crtc);
>  		}
>  
> @@ -12001,7 +12000,7 @@ intel_check_primary_plane(struct drm_plane *plane,
>  	if (ret)
>  		return ret;
>  
> -	if (intel_crtc->active) {
> +	if (crtc->state->active) {
>  		intel_crtc->atomic.wait_for_flips = true;
>  
>  		/*
> @@ -12068,7 +12067,7 @@ intel_commit_primary_plane(struct drm_plane *plane,
>  
>  	intel_plane->obj = obj;
>  
> -	if (intel_crtc->active) {
> +	if (crtc->state->active) {
>  		if (state->visible) {
>  			/* FIXME: kill this fastboot hack */
>  			intel_update_pipe_size(intel_crtc);
> @@ -12138,7 +12137,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
>  	intel_runtime_pm_get(dev_priv);
>  
>  	/* Perform vblank evasion around commit operation */
> -	if (intel_crtc->active)
> +	if (intel_crtc->base.state->active)
>  		intel_crtc->atomic.evade =
>  			intel_pipe_update_start(intel_crtc,
>  						&intel_crtc->atomic.start_vbl_count);
> @@ -12313,7 +12312,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
>  	}
>  
>  finish:
> -	if (intel_crtc->active) {
> +	if (crtc->state->active) {
>  		if (intel_crtc->base.cursor->state->crtc_w != state->base.crtc_w)
>  			intel_crtc->atomic.update_wm = true;
>  
> @@ -12358,7 +12357,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>  	intel_crtc->cursor_bo = obj;
>  update:
>  
> -	if (intel_crtc->active)
> +	if (crtc->state->active)
>  		intel_crtc_update_cursor(crtc, state->visible);
>  }
>  
> @@ -13399,7 +13398,7 @@ void intel_modeset_init(struct drm_device *dev)
>  	drm_modeset_unlock_all(dev);
>  
>  	for_each_intel_crtc(dev, crtc) {
> -		if (!crtc->active)
> +		if (!crtc->base.state->active)
>  			continue;
>  
>  		/*
> @@ -13477,7 +13476,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  
>  	/* restore vblank interrupts to correct state */
>  	drm_crtc_vblank_reset(&crtc->base);
> -	if (crtc->active) {
> +	if (crtc->base.state->active) {
>  		update_scanline_offset(crtc);
>  		drm_crtc_vblank_on(&crtc->base);
>  	}
> @@ -13517,13 +13516,13 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  				connector->encoder->connectors_active = false;
>  			}
>  
> -		WARN_ON(crtc->active);
> +		WARN_ON(crtc->base.state->active);
>  		crtc->base.state->enable = false;
>  		crtc->base.enabled = false;
>  	}
>  
>  	if (dev_priv->quirks & QUIRK_PIPEA_FORCE &&
> -	    crtc->pipe == PIPE_A && !crtc->active) {
> +	    crtc->pipe == PIPE_A && !crtc->base.state->active) {
>  		/* BIOS forgot to enable pipe A, this mostly happens after
>  		 * resume. Force-enable the pipe to fix this, the update_dpms
>  		 * call below we restore the pipe to the right state, but leave
> @@ -13535,7 +13534,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  	 * have active connectors/encoders. */
>  	intel_crtc_update_dpms(&crtc->base);
>  
> -	if (crtc->active != crtc->base.state->enable) {
> +	if (crtc->base.state->active != crtc->base.state->enable) {
>  		struct intel_encoder *encoder;
>  
>  		/* This can happen either due to bugs in the get_hw_state
> @@ -13544,17 +13543,17 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  		DRM_DEBUG_KMS("[CRTC:%d] hw state adjusted, was %s, now %s\n",
>  			      crtc->base.base.id,
>  			      crtc->base.state->enable ? "enabled" : "disabled",
> -			      crtc->active ? "enabled" : "disabled");
> +			      crtc->base.state->active ? "enabled" : "disabled");
>  
> -		crtc->base.state->enable = crtc->active;
> -		crtc->base.enabled = crtc->active;
> +		crtc->base.state->enable = crtc->base.state->active;
> +		crtc->base.enabled = crtc->base.state->active;
>  
>  		/* Because we only establish the connector -> encoder ->
>  		 * crtc links if something is active, this means the
>  		 * crtc is now deactivated. Break the links. connector
>  		 * -> encoder links are only establish when things are
>  		 *  actually up, hence no need to break them. */
> -		WARN_ON(crtc->active);
> +		WARN_ON(crtc->base.state->active);
>  
>  		for_each_encoder_on_crtc(dev, &crtc->base, encoder) {
>  			WARN_ON(encoder->connectors_active);
> @@ -13562,7 +13561,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  		}
>  	}
>  
> -	if (crtc->active || HAS_GMCH_DISPLAY(dev)) {
> +	if (crtc->base.state->active || HAS_GMCH_DISPLAY(dev)) {
>  		/*
>  		 * We start out with underrun reporting disabled to avoid races.
>  		 * For correct bookkeeping mark this on active crtcs.
> @@ -13590,7 +13589,7 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
>  	 * encoder is active and trying to read from a pipe) and the
>  	 * pipe itself being active. */
>  	bool has_active_crtc = encoder->base.crtc &&
> -		to_intel_crtc(encoder->base.crtc)->active;
> +		encoder->base.crtc->state->active;
>  
>  	if (encoder->connectors_active && !has_active_crtc) {
>  		DRM_DEBUG_KMS("[ENCODER:%d:%s] has active connectors but no active pipe!\n",
> @@ -13658,7 +13657,7 @@ static bool primary_get_hw_state(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
>  
> -	if (!crtc->active)
> +	if (!crtc->base.state->active)
>  		return false;
>  
>  	return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE;
> @@ -13678,16 +13677,16 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  
>  		crtc->config->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE;
>  
> -		crtc->active = dev_priv->display.get_pipe_config(crtc,
> +		crtc->base.state->active = dev_priv->display.get_pipe_config(crtc,
>  								 crtc->config);
>  
> -		crtc->base.state->enable = crtc->active;
> -		crtc->base.enabled = crtc->active;
> +		crtc->base.state->enable = crtc->base.state->active;
> +		crtc->base.enabled = crtc->base.state->active;
>  		crtc->primary_enabled = primary_get_hw_state(crtc);
>  
>  		DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
>  			      crtc->base.base.id,
> -			      crtc->active ? "enabled" : "disabled");
> +			      crtc->base.state->active ? "enabled" : "disabled");
>  	}
>  
>  	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
> @@ -13698,7 +13697,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  		pll->active = 0;
>  		pll->config.crtc_mask = 0;
>  		for_each_intel_crtc(dev, crtc) {
> -			if (crtc->active && intel_crtc_to_shared_dpll(crtc) == pll) {
> +			if (crtc->base.state->active && intel_crtc_to_shared_dpll(crtc) == pll) {
>  				pll->active++;
>  				pll->config.crtc_mask |= 1 << crtc->pipe;
>  			}
> @@ -13765,7 +13764,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  	 * checking everywhere.
>  	 */
>  	for_each_intel_crtc(dev, crtc) {
> -		if (crtc->active && i915.fastboot) {
> +		if (crtc->base.state->active && i915.fastboot) {
>  			intel_mode_from_pipe_config(&crtc->base.mode,
>  						    crtc->config);
>  			DRM_DEBUG_KMS("[CRTC:%d] found active mode: ",
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 33d5877..e4f447f 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3974,7 +3974,7 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>  	if (WARN_ON(!intel_encoder->base.crtc))
>  		return;
>  
> -	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> +	if (!intel_encoder->base.crtc->state->active)
>  		return;
>  
>  	/* Try to read receiver status if the link appears to be up */
> @@ -4955,7 +4955,7 @@ static void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
>  		return;
>  	}
>  
> -	if (!intel_crtc->active) {
> +	if (!intel_crtc->base.state->active) {
>  		DRM_DEBUG_KMS("eDP encoder disabled. CRTC not Active\n");
>  		return;
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index ff79dca..d194948 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -443,12 +443,6 @@ struct intel_crtc {
>  	enum pipe pipe;
>  	enum plane plane;
>  	u8 lut_r[256], lut_g[256], lut_b[256];
> -	/*
> -	 * Whether the crtc and the connected output pipeline is active. Implies
> -	 * that crtc->enabled is set, i.e. the current mode configuration has
> -	 * some outputs connected to this crtc.
> -	 */
> -	bool active;
>  	unsigned long enabled_power_domains;
>  	bool primary_enabled; /* is the primary plane (partially) visible? */
>  	bool lowfreq_avail;
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 234a699..192d229 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -541,7 +541,7 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
>  	for_each_crtc(dev, crtc) {
>  		intel_crtc = to_intel_crtc(crtc);
>  
> -		if (!intel_crtc->active || !crtc->primary->fb) {
> +		if (!intel_crtc->base.state->active || !crtc->primary->fb) {
>  			DRM_DEBUG_KMS("pipe %c not active or no fb, skipping\n",
>  				      pipe_name(intel_crtc->pipe));
>  			continue;
> @@ -567,7 +567,7 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
>  
>  		intel_crtc = to_intel_crtc(crtc);
>  
> -		if (!intel_crtc->active) {
> +		if (!intel_crtc->base.state->active) {
>  			DRM_DEBUG_KMS("pipe %c not active, skipping\n",
>  				      pipe_name(intel_crtc->pipe));
>  			continue;
> @@ -632,7 +632,7 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
>  	for_each_crtc(dev, crtc) {
>  		intel_crtc = to_intel_crtc(crtc);
>  
> -		if (!intel_crtc->active)
> +		if (!intel_crtc->base.state->active)
>  			continue;
>  
>  		WARN(!crtc->primary->fb,
> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> index 823d1d9..050a68e 100644
> --- a/drivers/gpu/drm/i915/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/intel_overlay.c
> @@ -852,7 +852,7 @@ int intel_overlay_switch_off(struct intel_overlay *overlay)
>  static int check_overlay_possible_on_crtc(struct intel_overlay *overlay,
>  					  struct intel_crtc *crtc)
>  {
> -	if (!crtc->active)
> +	if (!crtc->base.state->active)
>  		return -EINVAL;
>  
>  	/* can't use the overlay with double wide pipe */
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 4f04fab..d9b115e 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -822,7 +822,7 @@ static uint8_t vlv_compute_drain_latency(struct drm_crtc *crtc,
>  	 * FIXME the plane might have an fb
>  	 * but be invisible (eg. due to clipping)
>  	 */
> -	if (!intel_crtc->active || !plane->state->fb)
> +	if (!intel_crtc->base.state->active || !plane->state->fb)
>  		return 0;
>  
>  	if (WARN(clock == 0, "Pixel clock is zero!\n"))
> @@ -3080,7 +3080,7 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
>  	 * re-allocate the freed space without this pipe fetching from it.
>  	 */
>  	for_each_intel_crtc(dev, crtc) {
> -		if (!crtc->active)
> +		if (!crtc->base.state->active)
>  			continue;
>  
>  		pipe = crtc->pipe;
> @@ -3103,7 +3103,7 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
>  	 * space is not used anymore.
>  	 */
>  	for_each_intel_crtc(dev, crtc) {
> -		if (!crtc->active)
> +		if (!crtc->base.state->active)
>  			continue;
>  
>  		pipe = crtc->pipe;
> @@ -3126,7 +3126,7 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
>  	 * will just get more DDB space with the correct WM values.
>  	 */
>  	for_each_intel_crtc(dev, crtc) {
> -		if (!crtc->active)
> +		if (!crtc->base.state->active)
>  			continue;
>  
>  		pipe = crtc->pipe;
> @@ -3191,7 +3191,7 @@ static void skl_update_other_pipe_wm(struct drm_device *dev,
>  		if (this_crtc->pipe == intel_crtc->pipe)
>  			continue;
>  
> -		if (!intel_crtc->active)
> +		if (!intel_crtc->base.state->active)
>  			continue;
>  
>  		wm_changed = skl_update_pipe_wm(&intel_crtc->base,
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 7051da7..7ad9b7d 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1242,9 +1242,9 @@ finish:
>  	 */
>  	state->hides_primary = fb != NULL && drm_rect_equals(dst, clip) &&
>  		!colorkey_enabled(intel_plane);
> -	WARN_ON(state->hides_primary && !state->visible && intel_crtc->active);
> +	WARN_ON(state->hides_primary && !state->visible && state->base.crtc->state->active);
>  
> -	if (intel_crtc->active) {
> +	if (intel_crtc->base.state->active) {
>  		if (intel_crtc->primary_enabled == state->hides_primary)
>  			intel_crtc->atomic.wait_for_flips = true;
>  
> @@ -1286,7 +1286,7 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>  	plane->fb = state->base.fb;
>  	intel_plane->obj = obj;
>  
> -	if (intel_crtc->active) {
> +	if (crtc->state->active) {
>  		intel_crtc->primary_enabled = !state->hides_primary;
>  
>  		if (state->visible) {
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 2/5] drm/i915: Kill intel_crtc->active

[Daniel Vetter]

On Sun, Mar 08, 2015 at 02:00:42PM -0700, Matt Roper wrote:
> Replace all uses of intel_crtc->active with crtc->state->active.  At the
> moment we don't have atomic state handling wired up for CRTC's so this
> means we just directly set the state active field at various points in
> our (legacy) modeset pipeline.  Once we have CRTC atomic states properly
> hooked up, crtc_state->active will be set during the check phase.
> 
> Patch generated with Coccinelle via:
> 
>         // If we already have the base pointer, don't access it through
>         // intel_crtc->base needlessly.
>         @@
>         expression E;
>         struct intel_crtc *IC;
>         @@
>         IC = to_intel_crtc(E);
>         <+...
>         - IC->active
>         + E->state->active
>         ...+>
> 
>         // Not sure why the rule above doesn't catch these, but this seems
>         // to mop up the remainders.
>         @@
>         identifier I;
>         expression E;
>         @@
>         {
>         struct intel_crtc *I = to_intel_crtc(E);
>         <+...
>         - I->active
>         + E->state->active
>         ...+>
>         }
> 
>         // We might have left an unused intel_crtc in some functions; clean that up.
>         @@
>         identifier IC;
>         constant C;
>         @@
>         {
>         ...
>         - struct intel_crtc *IC;
>         ... when != IC
>         }
> 
>         // Change all of the other uses of intel_crtc->active
>         @@
>         struct intel_crtc *IC;
>         @@
>         - IC->active
>         + IC->base.state->active
> 
>         // Ditto
>         @@
>         expression E;
>         @@
>         - to_intel_crtc(E)->active
>         + E->state->active
> 
>         // Remove the active field from intel_crtc
>         @@ @@
>         struct intel_crtc {
>         ...
>         - bool active;
>         ...
>         };
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Just realized that blindly changing our handling of intel_crtc->active to
crtc_state->active will break the fdi bifurcate logic. But Ander is
working on that. Will at most have a small conflict with this one here.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c       |   9 ++-
>  drivers/gpu/drm/i915/i915_irq.c           |   2 +-
>  drivers/gpu/drm/i915/intel_atomic_plane.c |   4 +-
>  drivers/gpu/drm/i915/intel_display.c      | 129 +++++++++++++++---------------
>  drivers/gpu/drm/i915/intel_dp.c           |   4 +-
>  drivers/gpu/drm/i915/intel_drv.h          |   6 --
>  drivers/gpu/drm/i915/intel_fbdev.c        |   6 +-
>  drivers/gpu/drm/i915/intel_overlay.c      |   2 +-
>  drivers/gpu/drm/i915/intel_pm.c           |  10 +--
>  drivers/gpu/drm/i915/intel_sprite.c       |   6 +-
>  10 files changed, 86 insertions(+), 92 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 3f64786..28176fd 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2656,9 +2656,10 @@ static int i915_display_info(struct seq_file *m, void *unused)
>  
>  		seq_printf(m, "CRTC %d: pipe: %c, active=%s (size=%dx%d)\n",
>  			   crtc->base.base.id, pipe_name(crtc->pipe),
> -			   yesno(crtc->active), crtc->config->pipe_src_w,
> +			   yesno(crtc->base.state->active),
> +			   crtc->config->pipe_src_w,
>  			   crtc->config->pipe_src_h);
> -		if (crtc->active) {
> +		if (crtc->base.state->active) {
>  			intel_crtc_info(m, crtc);
>  
>  			active = cursor_position(dev, crtc->pipe, &x, &y);
> @@ -2951,7 +2952,7 @@ static int i915_drrs_status(struct seq_file *m, void *unused)
>  	for_each_intel_crtc(dev, intel_crtc) {
>  		drm_modeset_lock(&intel_crtc->base.mutex, NULL);
>  
> -		if (intel_crtc->active) {
> +		if (intel_crtc->base.state->active) {
>  			active_crtc_cnt++;
>  			seq_printf(m, "\nCRTC %d:  ", active_crtc_cnt);
>  
> @@ -3650,7 +3651,7 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
>  				 pipe_name(pipe));
>  
>  		drm_modeset_lock(&crtc->base.mutex, NULL);
> -		if (crtc->active)
> +		if (crtc->base.state->active)
>  			intel_wait_for_vblank(dev, pipe);
>  		drm_modeset_unlock(&crtc->base.mutex);
>  
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 9baecb7..ac834de 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -649,7 +649,7 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
>  	int ret = 0;
>  	unsigned long irqflags;
>  
> -	if (!intel_crtc->active) {
> +	if (!intel_crtc->base.state->active) {
>  		DRM_DEBUG_DRIVER("trying to get scanoutpos for disabled "
>  				 "pipe %c\n", pipe_name(pipe));
>  		return 0;
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index 976b891..43cb21f 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -144,9 +144,9 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
>  	intel_state->clip.x1 = 0;
>  	intel_state->clip.y1 = 0;
>  	intel_state->clip.x2 =
> -		intel_crtc->active ? intel_crtc->config->pipe_src_w : 0;
> +		crtc->state->active ? intel_crtc->config->pipe_src_w : 0;
>  	intel_state->clip.y2 =
> -		intel_crtc->active ? intel_crtc->config->pipe_src_h : 0;
> +		crtc->state->active ? intel_crtc->config->pipe_src_h : 0;
>  
>  	/*
>  	 * Disabling a plane is always okay; we just need to update
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 188f87f..b11528f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -897,7 +897,7 @@ bool intel_crtc_active(struct drm_crtc *crtc)
>  	 * We can ditch the crtc->primary->fb check as soon as we can
>  	 * properly reconstruct framebuffers.
>  	 */
> -	return intel_crtc->active && crtc->primary->fb &&
> +	return crtc->state->active && crtc->primary->fb &&
>  		intel_crtc->config->base.adjusted_mode.crtc_clock;
>  }
>  
> @@ -1591,7 +1591,7 @@ static int intel_num_dvo_pipes(struct drm_device *dev)
>  	int count = 0;
>  
>  	for_each_intel_crtc(dev, crtc)
> -		count += crtc->active &&
> +		count += crtc->base.state->active &&
>  			intel_pipe_has_type(crtc, INTEL_OUTPUT_DVO);
>  
>  	return count;
> @@ -2168,7 +2168,7 @@ static void intel_disable_primary_hw_plane(struct drm_plane *plane,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  
> -	if (WARN_ON(!intel_crtc->active))
> +	if (WARN_ON(!intel_crtc->base.state->active))
>  		return;
>  
>  	if (!intel_crtc->primary_enabled)
> @@ -2512,7 +2512,7 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
>  		if (c == &intel_crtc->base)
>  			continue;
>  
> -		if (!i->active)
> +		if (!i->base.state->active)
>  			continue;
>  
>  		obj = intel_fb_obj(c->primary->fb);
> @@ -2937,15 +2937,13 @@ static void intel_update_primary_planes(struct drm_device *dev)
>  	struct drm_crtc *crtc;
>  
>  	for_each_crtc(dev, crtc) {
> -		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -
>  		drm_modeset_lock(&crtc->mutex, NULL);
>  		/*
>  		 * FIXME: Once we have proper support for primary planes (and
>  		 * disabling them without disabling the entire crtc) allow again
>  		 * a NULL crtc->primary->fb.
>  		 */
> -		if (intel_crtc->active && crtc->primary->fb)
> +		if (crtc->state->active && crtc->primary->fb)
>  			dev_priv->display.update_primary_plane(crtc,
>  							       crtc->primary->fb,
>  							       crtc->x,
> @@ -2974,7 +2972,7 @@ void intel_prepare_reset(struct drm_device *dev)
>  	 * g33 docs say we should at least disable all the planes.
>  	 */
>  	for_each_intel_crtc(dev, crtc) {
> -		if (crtc->active)
> +		if (crtc->base.state->active)
>  			dev_priv->display.crtc_disable(&crtc->base);
>  	}
>  }
> @@ -3150,7 +3148,7 @@ static void intel_fdi_normal_train(struct drm_crtc *crtc)
>  
>  static bool pipe_has_enabled_pch(struct intel_crtc *crtc)
>  {
> -	return crtc->base.state->enable && crtc->active &&
> +	return crtc->base.state->enable && crtc->base.state->active &&
>  		crtc->config->has_pch_encoder;
>  }
>  
> @@ -4306,7 +4304,7 @@ static void intel_crtc_load_lut(struct drm_crtc *crtc)
>  	bool reenable_ips = false;
>  
>  	/* The clocks have to be on to load the palette. */
> -	if (!crtc->state->enable || !intel_crtc->active)
> +	if (!crtc->state->enable || !intel_crtc->base.state->active)
>  		return;
>  
>  	if (!HAS_PCH_SPLIT(dev_priv->dev)) {
> @@ -4421,7 +4419,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>  
>  	WARN_ON(!crtc->state->enable);
>  
> -	if (intel_crtc->active)
> +	if (intel_crtc->base.state->active)
>  		return;
>  
>  	if (intel_crtc->config->has_pch_encoder)
> @@ -4439,7 +4437,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>  
>  	ironlake_set_pipeconf(crtc);
>  
> -	intel_crtc->active = true;
> +	intel_crtc->base.state->active = true;
>  
>  	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
>  	intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
> @@ -4504,7 +4502,7 @@ static void haswell_mode_set_planes_workaround(struct intel_crtc *crtc)
>  	/* We want to get the other_active_crtc only if there's only 1 other
>  	 * active crtc. */
>  	for_each_intel_crtc(dev, crtc_it) {
> -		if (!crtc_it->active || crtc_it == crtc)
> +		if (!crtc_it->base.state->active || crtc_it == crtc)
>  			continue;
>  
>  		if (other_active_crtc)
> @@ -4529,7 +4527,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  
>  	WARN_ON(!crtc->state->enable);
>  
> -	if (intel_crtc->active)
> +	if (intel_crtc->base.state->active)
>  		return;
>  
>  	if (intel_crtc_to_shared_dpll(intel_crtc))
> @@ -4554,7 +4552,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  
>  	intel_set_pipe_csc(crtc);
>  
> -	intel_crtc->active = true;
> +	intel_crtc->base.state->active = true;
>  
>  	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
> @@ -4645,7 +4643,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>  	int pipe = intel_crtc->pipe;
>  	u32 reg, temp;
>  
> -	if (!intel_crtc->active)
> +	if (!intel_crtc->base.state->active)
>  		return;
>  
>  	intel_crtc_disable_planes(crtc);
> @@ -4693,7 +4691,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>  		ironlake_fdi_pll_disable(intel_crtc);
>  	}
>  
> -	intel_crtc->active = false;
> +	intel_crtc->base.state->active = false;
>  	intel_update_watermarks(crtc);
>  
>  	mutex_lock(&dev->struct_mutex);
> @@ -4709,7 +4707,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>  	struct intel_encoder *encoder;
>  	enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
>  
> -	if (!intel_crtc->active)
> +	if (!intel_crtc->base.state->active)
>  		return;
>  
>  	intel_crtc_disable_planes(crtc);
> @@ -4748,7 +4746,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>  		if (encoder->post_disable)
>  			encoder->post_disable(encoder);
>  
> -	intel_crtc->active = false;
> +	intel_crtc->base.state->active = false;
>  	intel_update_watermarks(crtc);
>  
>  	mutex_lock(&dev->struct_mutex);
> @@ -5137,7 +5135,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>  
>  	WARN_ON(!crtc->state->enable);
>  
> -	if (intel_crtc->active)
> +	if (intel_crtc->base.state->active)
>  		return;
>  
>  	is_dsi = intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DSI);
> @@ -5163,7 +5161,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>  
>  	i9xx_set_pipeconf(intel_crtc);
>  
> -	intel_crtc->active = true;
> +	intel_crtc->base.state->active = true;
>  
>  	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
>  
> @@ -5220,7 +5218,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>  
>  	WARN_ON(!crtc->state->enable);
>  
> -	if (intel_crtc->active)
> +	if (intel_crtc->base.state->active)
>  		return;
>  
>  	i9xx_set_pll_dividers(intel_crtc);
> @@ -5232,7 +5230,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>  
>  	i9xx_set_pipeconf(intel_crtc);
>  
> -	intel_crtc->active = true;
> +	intel_crtc->base.state->active = true;
>  
>  	if (!IS_GEN2(dev))
>  		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
> @@ -5295,7 +5293,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>  	struct intel_encoder *encoder;
>  	int pipe = intel_crtc->pipe;
>  
> -	if (!intel_crtc->active)
> +	if (!intel_crtc->base.state->active)
>  		return;
>  
>  	/*
> @@ -5353,7 +5351,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>  	if (!IS_GEN2(dev))
>  		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
>  
> -	intel_crtc->active = false;
> +	intel_crtc->base.state->active = false;
>  	intel_update_watermarks(crtc);
>  
>  	mutex_lock(&dev->struct_mutex);
> @@ -5375,7 +5373,7 @@ void intel_crtc_control(struct drm_crtc *crtc, bool enable)
>  	unsigned long domains;
>  
>  	if (enable) {
> -		if (!intel_crtc->active) {
> +		if (!intel_crtc->base.state->active) {
>  			domains = get_crtc_power_domains(crtc);
>  			for_each_power_domain(domain, domains)
>  				intel_display_power_get(dev_priv, domain);
> @@ -5384,7 +5382,7 @@ void intel_crtc_control(struct drm_crtc *crtc, bool enable)
>  			dev_priv->display.crtc_enable(crtc);
>  		}
>  	} else {
> -		if (intel_crtc->active) {
> +		if (intel_crtc->base.state->active) {
>  			dev_priv->display.crtc_disable(crtc);
>  
>  			domains = intel_crtc->enabled_power_domains;
> @@ -5497,7 +5495,8 @@ static void intel_connector_check_state(struct intel_connector *connector)
>  
>  			I915_STATE_WARN(!crtc->state->enable,
>  					"crtc not enabled\n");
> -			I915_STATE_WARN(!to_intel_crtc(crtc)->active, "crtc not active\n");
> +			I915_STATE_WARN(!crtc->state->active,
> +					"crtc not active\n");
>  			I915_STATE_WARN(pipe != to_intel_crtc(crtc)->pipe,
>  			     "encoder active on the wrong pipe\n");
>  		}
> @@ -8032,7 +8031,8 @@ static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv)
>  	struct intel_crtc *crtc;
>  
>  	for_each_intel_crtc(dev, crtc)
> -		I915_STATE_WARN(crtc->active, "CRTC for pipe %c enabled\n",
> +		I915_STATE_WARN(crtc->base.state->active,
> +				"CRTC for pipe %c enabled\n",
>  		     pipe_name(crtc->pipe));
>  
>  	I915_STATE_WARN(I915_READ(HSW_PWR_WELL_DRIVER), "Power well on\n");
> @@ -10816,7 +10816,7 @@ static void check_wm_state(struct drm_device *dev)
>  		struct skl_ddb_entry *hw_entry, *sw_entry;
>  		const enum pipe pipe = intel_crtc->pipe;
>  
> -		if (!intel_crtc->active)
> +		if (!intel_crtc->base.state->active)
>  			continue;
>  
>  		/* planes */
> @@ -10945,7 +10945,7 @@ check_crtc_state(struct drm_device *dev)
>  		DRM_DEBUG_KMS("[CRTC:%d]\n",
>  			      crtc->base.base.id);
>  
> -		I915_STATE_WARN(crtc->active && !crtc->base.state->enable,
> +		I915_STATE_WARN(crtc->base.state->active && !crtc->base.state->enable,
>  		     "active crtc, but not enabled in sw tracking\n");
>  
>  		for_each_intel_encoder(dev, encoder) {
> @@ -10956,9 +10956,10 @@ check_crtc_state(struct drm_device *dev)
>  				active = true;
>  		}
>  
> -		I915_STATE_WARN(active != crtc->active,
> +		I915_STATE_WARN(active != crtc->base.state->active,
>  		     "crtc's computed active state doesn't match tracked active state "
> -		     "(expected %i, found %i)\n", active, crtc->active);
> +		     "(expected %i, found %i)\n", active,
> +				crtc->base.state->active);
>  		I915_STATE_WARN(enabled != crtc->base.state->enable,
>  		     "crtc's computed enabled state doesn't match tracked enabled state "
>  		     "(expected %i, found %i)\n", enabled,
> @@ -10970,7 +10971,7 @@ check_crtc_state(struct drm_device *dev)
>  		/* hw state is inconsistent with the pipe quirk */
>  		if ((crtc->pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) ||
>  		    (crtc->pipe == PIPE_B && dev_priv->quirks & QUIRK_PIPEB_FORCE))
> -			active = crtc->active;
> +			active = crtc->base.state->active;
>  
>  		for_each_intel_encoder(dev, encoder) {
>  			enum pipe pipe;
> @@ -10980,9 +10981,10 @@ check_crtc_state(struct drm_device *dev)
>  				encoder->get_config(encoder, &pipe_config);
>  		}
>  
> -		I915_STATE_WARN(crtc->active != active,
> +		I915_STATE_WARN(crtc->base.state->active != active,
>  		     "crtc active state doesn't match with hw state "
> -		     "(expected %i, found %i)\n", crtc->active, active);
> +		     "(expected %i, found %i)\n", crtc->base.state->active,
> +				active);
>  
>  		if (active &&
>  		    !intel_pipe_config_compare(dev, crtc->config, &pipe_config)) {
> @@ -11028,7 +11030,7 @@ check_shared_dpll_state(struct drm_device *dev)
>  		for_each_intel_crtc(dev, crtc) {
>  			if (crtc->base.state->enable && intel_crtc_to_shared_dpll(crtc) == pll)
>  				enabled_crtcs++;
> -			if (crtc->active && intel_crtc_to_shared_dpll(crtc) == pll)
> +			if (crtc->base.state->active && intel_crtc_to_shared_dpll(crtc) == pll)
>  				active_crtcs++;
>  		}
>  		I915_STATE_WARN(pll->active != active_crtcs,
> @@ -11449,10 +11451,7 @@ intel_set_config_compute_mode_changes(struct drm_mode_set *set,
>  		 * in fastboot situations.
>  		 */
>  		if (set->crtc->primary->fb == NULL) {
> -			struct intel_crtc *intel_crtc =
> -				to_intel_crtc(set->crtc);
> -
> -			if (intel_crtc->active) {
> +			if (set->crtc->state->active) {
>  				DRM_DEBUG_KMS("crtc has no fb, will flip\n");
>  				config->fb_changed = true;
>  			} else {
> @@ -11738,7 +11737,7 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  		 * has previously been turned off.
>  		 */
>  		if (!intel_crtc->primary_enabled && ret == 0) {
> -			WARN_ON(!intel_crtc->active);
> +			WARN_ON(!set->crtc->state->active);
>  			intel_enable_primary_hw_plane(set->crtc->primary, set->crtc);
>  		}
>  
> @@ -12001,7 +12000,7 @@ intel_check_primary_plane(struct drm_plane *plane,
>  	if (ret)
>  		return ret;
>  
> -	if (intel_crtc->active) {
> +	if (crtc->state->active) {
>  		intel_crtc->atomic.wait_for_flips = true;
>  
>  		/*
> @@ -12068,7 +12067,7 @@ intel_commit_primary_plane(struct drm_plane *plane,
>  
>  	intel_plane->obj = obj;
>  
> -	if (intel_crtc->active) {
> +	if (crtc->state->active) {
>  		if (state->visible) {
>  			/* FIXME: kill this fastboot hack */
>  			intel_update_pipe_size(intel_crtc);
> @@ -12138,7 +12137,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
>  	intel_runtime_pm_get(dev_priv);
>  
>  	/* Perform vblank evasion around commit operation */
> -	if (intel_crtc->active)
> +	if (intel_crtc->base.state->active)
>  		intel_crtc->atomic.evade =
>  			intel_pipe_update_start(intel_crtc,
>  						&intel_crtc->atomic.start_vbl_count);
> @@ -12313,7 +12312,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
>  	}
>  
>  finish:
> -	if (intel_crtc->active) {
> +	if (crtc->state->active) {
>  		if (intel_crtc->base.cursor->state->crtc_w != state->base.crtc_w)
>  			intel_crtc->atomic.update_wm = true;
>  
> @@ -12358,7 +12357,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
>  	intel_crtc->cursor_bo = obj;
>  update:
>  
> -	if (intel_crtc->active)
> +	if (crtc->state->active)
>  		intel_crtc_update_cursor(crtc, state->visible);
>  }
>  
> @@ -13399,7 +13398,7 @@ void intel_modeset_init(struct drm_device *dev)
>  	drm_modeset_unlock_all(dev);
>  
>  	for_each_intel_crtc(dev, crtc) {
> -		if (!crtc->active)
> +		if (!crtc->base.state->active)
>  			continue;
>  
>  		/*
> @@ -13477,7 +13476,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  
>  	/* restore vblank interrupts to correct state */
>  	drm_crtc_vblank_reset(&crtc->base);
> -	if (crtc->active) {
> +	if (crtc->base.state->active) {
>  		update_scanline_offset(crtc);
>  		drm_crtc_vblank_on(&crtc->base);
>  	}
> @@ -13517,13 +13516,13 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  				connector->encoder->connectors_active = false;
>  			}
>  
> -		WARN_ON(crtc->active);
> +		WARN_ON(crtc->base.state->active);
>  		crtc->base.state->enable = false;
>  		crtc->base.enabled = false;
>  	}
>  
>  	if (dev_priv->quirks & QUIRK_PIPEA_FORCE &&
> -	    crtc->pipe == PIPE_A && !crtc->active) {
> +	    crtc->pipe == PIPE_A && !crtc->base.state->active) {
>  		/* BIOS forgot to enable pipe A, this mostly happens after
>  		 * resume. Force-enable the pipe to fix this, the update_dpms
>  		 * call below we restore the pipe to the right state, but leave
> @@ -13535,7 +13534,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  	 * have active connectors/encoders. */
>  	intel_crtc_update_dpms(&crtc->base);
>  
> -	if (crtc->active != crtc->base.state->enable) {
> +	if (crtc->base.state->active != crtc->base.state->enable) {
>  		struct intel_encoder *encoder;
>  
>  		/* This can happen either due to bugs in the get_hw_state
> @@ -13544,17 +13543,17 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  		DRM_DEBUG_KMS("[CRTC:%d] hw state adjusted, was %s, now %s\n",
>  			      crtc->base.base.id,
>  			      crtc->base.state->enable ? "enabled" : "disabled",
> -			      crtc->active ? "enabled" : "disabled");
> +			      crtc->base.state->active ? "enabled" : "disabled");
>  
> -		crtc->base.state->enable = crtc->active;
> -		crtc->base.enabled = crtc->active;
> +		crtc->base.state->enable = crtc->base.state->active;
> +		crtc->base.enabled = crtc->base.state->active;
>  
>  		/* Because we only establish the connector -> encoder ->
>  		 * crtc links if something is active, this means the
>  		 * crtc is now deactivated. Break the links. connector
>  		 * -> encoder links are only establish when things are
>  		 *  actually up, hence no need to break them. */
> -		WARN_ON(crtc->active);
> +		WARN_ON(crtc->base.state->active);
>  
>  		for_each_encoder_on_crtc(dev, &crtc->base, encoder) {
>  			WARN_ON(encoder->connectors_active);
> @@ -13562,7 +13561,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  		}
>  	}
>  
> -	if (crtc->active || HAS_GMCH_DISPLAY(dev)) {
> +	if (crtc->base.state->active || HAS_GMCH_DISPLAY(dev)) {
>  		/*
>  		 * We start out with underrun reporting disabled to avoid races.
>  		 * For correct bookkeeping mark this on active crtcs.
> @@ -13590,7 +13589,7 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
>  	 * encoder is active and trying to read from a pipe) and the
>  	 * pipe itself being active. */
>  	bool has_active_crtc = encoder->base.crtc &&
> -		to_intel_crtc(encoder->base.crtc)->active;
> +		encoder->base.crtc->state->active;
>  
>  	if (encoder->connectors_active && !has_active_crtc) {
>  		DRM_DEBUG_KMS("[ENCODER:%d:%s] has active connectors but no active pipe!\n",
> @@ -13658,7 +13657,7 @@ static bool primary_get_hw_state(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
>  
> -	if (!crtc->active)
> +	if (!crtc->base.state->active)
>  		return false;
>  
>  	return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE;
> @@ -13678,16 +13677,16 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  
>  		crtc->config->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE;
>  
> -		crtc->active = dev_priv->display.get_pipe_config(crtc,
> +		crtc->base.state->active = dev_priv->display.get_pipe_config(crtc,
>  								 crtc->config);
>  
> -		crtc->base.state->enable = crtc->active;
> -		crtc->base.enabled = crtc->active;
> +		crtc->base.state->enable = crtc->base.state->active;
> +		crtc->base.enabled = crtc->base.state->active;
>  		crtc->primary_enabled = primary_get_hw_state(crtc);
>  
>  		DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
>  			      crtc->base.base.id,
> -			      crtc->active ? "enabled" : "disabled");
> +			      crtc->base.state->active ? "enabled" : "disabled");
>  	}
>  
>  	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
> @@ -13698,7 +13697,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  		pll->active = 0;
>  		pll->config.crtc_mask = 0;
>  		for_each_intel_crtc(dev, crtc) {
> -			if (crtc->active && intel_crtc_to_shared_dpll(crtc) == pll) {
> +			if (crtc->base.state->active && intel_crtc_to_shared_dpll(crtc) == pll) {
>  				pll->active++;
>  				pll->config.crtc_mask |= 1 << crtc->pipe;
>  			}
> @@ -13765,7 +13764,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  	 * checking everywhere.
>  	 */
>  	for_each_intel_crtc(dev, crtc) {
> -		if (crtc->active && i915.fastboot) {
> +		if (crtc->base.state->active && i915.fastboot) {
>  			intel_mode_from_pipe_config(&crtc->base.mode,
>  						    crtc->config);
>  			DRM_DEBUG_KMS("[CRTC:%d] found active mode: ",
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 33d5877..e4f447f 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3974,7 +3974,7 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>  	if (WARN_ON(!intel_encoder->base.crtc))
>  		return;
>  
> -	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> +	if (!intel_encoder->base.crtc->state->active)
>  		return;
>  
>  	/* Try to read receiver status if the link appears to be up */
> @@ -4955,7 +4955,7 @@ static void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
>  		return;
>  	}
>  
> -	if (!intel_crtc->active) {
> +	if (!intel_crtc->base.state->active) {
>  		DRM_DEBUG_KMS("eDP encoder disabled. CRTC not Active\n");
>  		return;
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index ff79dca..d194948 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -443,12 +443,6 @@ struct intel_crtc {
>  	enum pipe pipe;
>  	enum plane plane;
>  	u8 lut_r[256], lut_g[256], lut_b[256];
> -	/*
> -	 * Whether the crtc and the connected output pipeline is active. Implies
> -	 * that crtc->enabled is set, i.e. the current mode configuration has
> -	 * some outputs connected to this crtc.
> -	 */
> -	bool active;
>  	unsigned long enabled_power_domains;
>  	bool primary_enabled; /* is the primary plane (partially) visible? */
>  	bool lowfreq_avail;
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 234a699..192d229 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -541,7 +541,7 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
>  	for_each_crtc(dev, crtc) {
>  		intel_crtc = to_intel_crtc(crtc);
>  
> -		if (!intel_crtc->active || !crtc->primary->fb) {
> +		if (!intel_crtc->base.state->active || !crtc->primary->fb) {
>  			DRM_DEBUG_KMS("pipe %c not active or no fb, skipping\n",
>  				      pipe_name(intel_crtc->pipe));
>  			continue;
> @@ -567,7 +567,7 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
>  
>  		intel_crtc = to_intel_crtc(crtc);
>  
> -		if (!intel_crtc->active) {
> +		if (!intel_crtc->base.state->active) {
>  			DRM_DEBUG_KMS("pipe %c not active, skipping\n",
>  				      pipe_name(intel_crtc->pipe));
>  			continue;
> @@ -632,7 +632,7 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
>  	for_each_crtc(dev, crtc) {
>  		intel_crtc = to_intel_crtc(crtc);
>  
> -		if (!intel_crtc->active)
> +		if (!intel_crtc->base.state->active)
>  			continue;
>  
>  		WARN(!crtc->primary->fb,
> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> index 823d1d9..050a68e 100644
> --- a/drivers/gpu/drm/i915/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/intel_overlay.c
> @@ -852,7 +852,7 @@ int intel_overlay_switch_off(struct intel_overlay *overlay)
>  static int check_overlay_possible_on_crtc(struct intel_overlay *overlay,
>  					  struct intel_crtc *crtc)
>  {
> -	if (!crtc->active)
> +	if (!crtc->base.state->active)
>  		return -EINVAL;
>  
>  	/* can't use the overlay with double wide pipe */
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 4f04fab..d9b115e 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -822,7 +822,7 @@ static uint8_t vlv_compute_drain_latency(struct drm_crtc *crtc,
>  	 * FIXME the plane might have an fb
>  	 * but be invisible (eg. due to clipping)
>  	 */
> -	if (!intel_crtc->active || !plane->state->fb)
> +	if (!intel_crtc->base.state->active || !plane->state->fb)
>  		return 0;
>  
>  	if (WARN(clock == 0, "Pixel clock is zero!\n"))
> @@ -3080,7 +3080,7 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
>  	 * re-allocate the freed space without this pipe fetching from it.
>  	 */
>  	for_each_intel_crtc(dev, crtc) {
> -		if (!crtc->active)
> +		if (!crtc->base.state->active)
>  			continue;
>  
>  		pipe = crtc->pipe;
> @@ -3103,7 +3103,7 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
>  	 * space is not used anymore.
>  	 */
>  	for_each_intel_crtc(dev, crtc) {
> -		if (!crtc->active)
> +		if (!crtc->base.state->active)
>  			continue;
>  
>  		pipe = crtc->pipe;
> @@ -3126,7 +3126,7 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
>  	 * will just get more DDB space with the correct WM values.
>  	 */
>  	for_each_intel_crtc(dev, crtc) {
> -		if (!crtc->active)
> +		if (!crtc->base.state->active)
>  			continue;
>  
>  		pipe = crtc->pipe;
> @@ -3191,7 +3191,7 @@ static void skl_update_other_pipe_wm(struct drm_device *dev,
>  		if (this_crtc->pipe == intel_crtc->pipe)
>  			continue;
>  
> -		if (!intel_crtc->active)
> +		if (!intel_crtc->base.state->active)
>  			continue;
>  
>  		wm_changed = skl_update_pipe_wm(&intel_crtc->base,
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 7051da7..7ad9b7d 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1242,9 +1242,9 @@ finish:
>  	 */
>  	state->hides_primary = fb != NULL && drm_rect_equals(dst, clip) &&
>  		!colorkey_enabled(intel_plane);
> -	WARN_ON(state->hides_primary && !state->visible && intel_crtc->active);
> +	WARN_ON(state->hides_primary && !state->visible && state->base.crtc->state->active);
>  
> -	if (intel_crtc->active) {
> +	if (intel_crtc->base.state->active) {
>  		if (intel_crtc->primary_enabled == state->hides_primary)
>  			intel_crtc->atomic.wait_for_flips = true;
>  
> @@ -1286,7 +1286,7 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>  	plane->fb = state->base.fb;
>  	intel_plane->obj = obj;
>  
> -	if (intel_crtc->active) {
> +	if (crtc->state->active) {
>  		intel_crtc->primary_enabled = !state->hides_primary;
>  
>  		if (state->visible) {
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/5] drm/i915: Update intel_crtc_active() to use state values

[Matt Roper]

With the switch to atomic plumbing for planes, some of our commit-time
work (e.g., watermarks) is done after the new atomic state is swapped
into the relevant DRM object, but before the DRM core has a chance to
update its legacy state values.  Switch intel_crtc_active() to look at
the state objects rather than legacy fields to ensure we operate on the
proper values.

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b11528f..4f8c622d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -886,8 +886,6 @@ chv_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
 
 bool intel_crtc_active(struct drm_crtc *crtc)
 {
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-
 	/* Be paranoid as we can arrive here with only partial
 	 * state retrieved from the hardware during setup.
 	 *
@@ -897,8 +895,8 @@ bool intel_crtc_active(struct drm_crtc *crtc)
 	 * We can ditch the crtc->primary->fb check as soon as we can
 	 * properly reconstruct framebuffers.
 	 */
-	return crtc->state->active && crtc->primary->fb &&
-		intel_crtc->config->base.adjusted_mode.crtc_clock;
+	return crtc->state->active && crtc->primary->state->fb &&
+		crtc->state->adjusted_mode.crtc_clock;
 }
 
 enum transcoder intel_pipe_to_cpu_transcoder(struct drm_i915_private *dev_priv,
-- 
1.8.5.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915: Update intel_crtc_active() to use state values

[Ville Syrjälä]

On Sun, Mar 08, 2015 at 02:00:43PM -0700, Matt Roper wrote:
> With the switch to atomic plumbing for planes, some of our commit-time
> work (e.g., watermarks) is done after the new atomic state is swapped
> into the relevant DRM object, but before the DRM core has a chance to
> update its legacy state values.  Switch intel_crtc_active() to look at
> the state objects rather than legacy fields to ensure we operate on the
> proper values.
> 
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

This gets used from the wm code and for that I think it's correct in the
sense that it'll preventt oopses, and from the FBC code which is.. well
yeah.

So this is
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

And due to this I think you can for now drop all the plane->state->fb checks
you add in patch 5/5 to the pre-ilk platforms' wm code. We'll need to
eventually change all that code to consider disabled planes correctly,
but for now just not oopsing is good enough I think.

> ---
>  drivers/gpu/drm/i915/intel_display.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b11528f..4f8c622d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -886,8 +886,6 @@ chv_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
>  
>  bool intel_crtc_active(struct drm_crtc *crtc)
>  {
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -
>  	/* Be paranoid as we can arrive here with only partial
>  	 * state retrieved from the hardware during setup.
>  	 *
> @@ -897,8 +895,8 @@ bool intel_crtc_active(struct drm_crtc *crtc)
>  	 * We can ditch the crtc->primary->fb check as soon as we can
>  	 * properly reconstruct framebuffers.
>  	 */
> -	return crtc->state->active && crtc->primary->fb &&
> -		intel_crtc->config->base.adjusted_mode.crtc_clock;
> +	return crtc->state->active && crtc->primary->state->fb &&
> +		crtc->state->adjusted_mode.crtc_clock;
>  }
>  
>  enum transcoder intel_pipe_to_cpu_transcoder(struct drm_i915_private *dev_priv,
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 3/5] drm/i915: Update intel_crtc_active() to use state values

[Daniel Vetter]

On Sun, Mar 08, 2015 at 02:00:43PM -0700, Matt Roper wrote:
> With the switch to atomic plumbing for planes, some of our commit-time
> work (e.g., watermarks) is done after the new atomic state is swapped
> into the relevant DRM object, but before the DRM core has a chance to
> update its legacy state values.  Switch intel_crtc_active() to look at
> the state objects rather than legacy fields to ensure we operate on the
> proper values.

Maybe clarify that we also set plane->fb in our own plane update code (and
yeah the drm core follows suit), but even that is too late. Thinking about
this ... follow-up patch to stop us from updating plane->fb in various
places and rely upon the drm core once this series has landed?
-Daniel

> 
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b11528f..4f8c622d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -886,8 +886,6 @@ chv_find_best_dpll(const intel_limit_t *limit, struct intel_crtc *crtc,
>  
>  bool intel_crtc_active(struct drm_crtc *crtc)
>  {
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -
>  	/* Be paranoid as we can arrive here with only partial
>  	 * state retrieved from the hardware during setup.
>  	 *
> @@ -897,8 +895,8 @@ bool intel_crtc_active(struct drm_crtc *crtc)
>  	 * We can ditch the crtc->primary->fb check as soon as we can
>  	 * properly reconstruct framebuffers.
>  	 */
> -	return crtc->state->active && crtc->primary->fb &&
> -		intel_crtc->config->base.adjusted_mode.crtc_clock;
> +	return crtc->state->active && crtc->primary->state->fb &&
> +		crtc->state->adjusted_mode.crtc_clock;
>  }
>  
>  enum transcoder intel_pipe_to_cpu_transcoder(struct drm_i915_private *dev_priv,
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/5] drm/i915: Use crtc->state->active in ilk/skl watermark calculations

[Matt Roper]

Existing watermark code calls intel_crtc_active() to determine whether a CRTC
is active for the purpose of watermark calculations (and bails out early if it
determines the CRTC is not active).  However intel_crtc_active() only returns
true if crtc->primary->fb is non-NULL, which isn't appropriate in the modern
age of universal planes and atomic modeset since userspace can now disable the
primary plane, but leave the CRTC (and other planes) running.

Note that commit

        commit 0fda65680e92545caea5be7805a7f0a617fb6c20
        Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
        Date:   Fri Feb 27 15:12:35 2015 +0000

            drm/i915/skl: Update watermarks for Y tiling

adds a test for primary plane enable/disable to trigger a watermark update
(previously we ignored updates to primary planes, which wasn't really correct,
but we got lucky since we always pretended the primary plane was on).  Tvrtko's
patch tries to update watermarks when we re-enable the primary plane, but that
watermark computation gets aborted early because intel_crtc_active() returns
false due to the disabled primary plane.

Switch the ILK and SKL watermark code over to use crtc->state->active rather
than calling intel_crtc_active() so that we'll properly compute watermarks when
re-enabling the primary plane.

Note that this commit doesn't touch callsites in the watermark code for
older platforms since there were concerns that doing so would lead to
other types of breakage.

Also note that all of the watermark calculation at the moment takes place after
new crtc/plane states are swapped into the DRM objects.  This will change in
the future, so we'll be working with in-flight state objects, but for the time
being, crtc->state is what we want to operate on.

v2: Don't drop primary->fb check from intel_crtc_active(), but rather replace
    ILK/SKL callsites with direct tests of crtc->state->active.  There is
    concern that messing with intel_crtc_active() will lead to other breakage for
    old hardware platforms.  (Ville)

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d9b115e..dafd7de 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -822,7 +822,7 @@ static uint8_t vlv_compute_drain_latency(struct drm_crtc *crtc,
 	 * FIXME the plane might have an fb
 	 * but be invisible (eg. due to clipping)
 	 */
-	if (!intel_crtc->base.state->active || !plane->state->fb)
+	if (!crtc->state->active || !plane->state->fb)
 		return 0;
 
 	if (WARN(clock == 0, "Pixel clock is zero!\n"))
@@ -1701,7 +1701,7 @@ hsw_compute_linetime_wm(struct drm_device *dev, struct drm_crtc *crtc)
 	struct drm_display_mode *mode = &intel_crtc->config->base.adjusted_mode;
 	u32 linetime, ips_linetime;
 
-	if (!intel_crtc_active(crtc))
+	if (!crtc->state->active)
 		return 0;
 
 	/* The WM are computed with base on how long it takes to fill a single
@@ -1956,7 +1956,7 @@ static void ilk_compute_wm_parameters(struct drm_crtc *crtc,
 	enum pipe pipe = intel_crtc->pipe;
 	struct drm_plane *plane;
 
-	if (!intel_crtc_active(crtc))
+	if (!crtc->state->active)
 		return;
 
 	p->active = true;
@@ -2468,7 +2468,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 
 	nth_active_pipe = 0;
 	for_each_crtc(dev, crtc) {
-		if (!intel_crtc_active(crtc))
+		if (!crtc->state->active)
 			continue;
 
 		if (crtc == for_crtc)
@@ -2708,7 +2708,7 @@ static void skl_compute_wm_global_parameters(struct drm_device *dev,
 	struct drm_plane *plane;
 
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
-		config->num_pipes_active += intel_crtc_active(crtc);
+		config->num_pipes_active += crtc->state->active;
 
 	/* FIXME: I don't think we need those two global parameters on SKL */
 	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
@@ -2729,7 +2729,7 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
 	struct drm_framebuffer *fb;
 	int i = 1; /* Index for sprite planes start */
 
-	p->active = intel_crtc_active(crtc);
+	p->active = crtc->state->active;
 	if (p->active) {
 		p->pipe_htotal = intel_crtc->config->base.adjusted_mode.crtc_htotal;
 		p->pixel_rate = skl_pipe_pixel_rate(intel_crtc->config);
@@ -2860,7 +2860,7 @@ static void skl_compute_wm_level(const struct drm_i915_private *dev_priv,
 static uint32_t
 skl_compute_linetime_wm(struct drm_crtc *crtc, struct skl_pipe_wm_parameters *p)
 {
-	if (!intel_crtc_active(crtc))
+	if (!crtc->state->active)
 		return 0;
 
 	return DIV_ROUND_UP(8 * p->pipe_htotal * 1000, p->pixel_rate);
@@ -3191,7 +3191,7 @@ static void skl_update_other_pipe_wm(struct drm_device *dev,
 		if (this_crtc->pipe == intel_crtc->pipe)
 			continue;
 
-		if (!intel_crtc->base.state->active)
+		if (!crtc->state->active)
 			continue;
 
 		wm_changed = skl_update_pipe_wm(&intel_crtc->base,
@@ -3407,7 +3407,7 @@ static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc)
 		hw->plane_trans[pipe][i] = I915_READ(PLANE_WM_TRANS(pipe, i));
 	hw->cursor_trans[pipe] = I915_READ(CUR_WM_TRANS(pipe));
 
-	if (!intel_crtc_active(crtc))
+	if (!crtc->state->active)
 		return;
 
 	hw->dirty[pipe] = true;
@@ -3462,7 +3462,7 @@ static void ilk_pipe_wm_get_hw_state(struct drm_crtc *crtc)
 	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
 		hw->wm_linetime[pipe] = I915_READ(PIPE_WM_LINETIME(pipe));
 
-	active->pipe_enabled = intel_crtc_active(crtc);
+	active->pipe_enabled = crtc->state->active;
 
 	if (active->pipe_enabled) {
 		u32 tmp = hw->wm_pipe[pipe];
-- 
1.8.5.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915: Use crtc->state->active in ilk/skl watermark calculations

[Ville Syrjälä]

On Sun, Mar 08, 2015 at 02:00:44PM -0700, Matt Roper wrote:
> Existing watermark code calls intel_crtc_active() to determine whether a CRTC
> is active for the purpose of watermark calculations (and bails out early if it
> determines the CRTC is not active).  However intel_crtc_active() only returns
> true if crtc->primary->fb is non-NULL, which isn't appropriate in the modern
> age of universal planes and atomic modeset since userspace can now disable the
> primary plane, but leave the CRTC (and other planes) running.
> 
> Note that commit
> 
>         commit 0fda65680e92545caea5be7805a7f0a617fb6c20
>         Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>         Date:   Fri Feb 27 15:12:35 2015 +0000
> 
>             drm/i915/skl: Update watermarks for Y tiling
> 
> adds a test for primary plane enable/disable to trigger a watermark update
> (previously we ignored updates to primary planes, which wasn't really correct,
> but we got lucky since we always pretended the primary plane was on).  Tvrtko's
> patch tries to update watermarks when we re-enable the primary plane, but that
> watermark computation gets aborted early because intel_crtc_active() returns
> false due to the disabled primary plane.
> 
> Switch the ILK and SKL watermark code over to use crtc->state->active rather
> than calling intel_crtc_active() so that we'll properly compute watermarks when
> re-enabling the primary plane.
> 
> Note that this commit doesn't touch callsites in the watermark code for
> older platforms since there were concerns that doing so would lead to
> other types of breakage.
> 
> Also note that all of the watermark calculation at the moment takes place after
> new crtc/plane states are swapped into the DRM objects.  This will change in
> the future, so we'll be working with in-flight state objects, but for the time
> being, crtc->state is what we want to operate on.
> 
> v2: Don't drop primary->fb check from intel_crtc_active(), but rather replace
>     ILK/SKL callsites with direct tests of crtc->state->active.  There is
>     concern that messing with intel_crtc_active() will lead to other breakage for
>     old hardware platforms.  (Ville)
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>


If you make this use intel_crtc->active you can slap on my 
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

The intel_crtc->active to crtc->state->active change is more
controversial so I think we need to look at the code in more detail
before we go make such a big change.

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d9b115e..dafd7de 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -822,7 +822,7 @@ static uint8_t vlv_compute_drain_latency(struct drm_crtc *crtc,
>  	 * FIXME the plane might have an fb
>  	 * but be invisible (eg. due to clipping)
>  	 */
> -	if (!intel_crtc->base.state->active || !plane->state->fb)
> +	if (!crtc->state->active || !plane->state->fb)
>  		return 0;
>  
>  	if (WARN(clock == 0, "Pixel clock is zero!\n"))
> @@ -1701,7 +1701,7 @@ hsw_compute_linetime_wm(struct drm_device *dev, struct drm_crtc *crtc)
>  	struct drm_display_mode *mode = &intel_crtc->config->base.adjusted_mode;
>  	u32 linetime, ips_linetime;
>  
> -	if (!intel_crtc_active(crtc))
> +	if (!crtc->state->active)
>  		return 0;
>  
>  	/* The WM are computed with base on how long it takes to fill a single
> @@ -1956,7 +1956,7 @@ static void ilk_compute_wm_parameters(struct drm_crtc *crtc,
>  	enum pipe pipe = intel_crtc->pipe;
>  	struct drm_plane *plane;
>  
> -	if (!intel_crtc_active(crtc))
> +	if (!crtc->state->active)
>  		return;
>  
>  	p->active = true;
> @@ -2468,7 +2468,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>  
>  	nth_active_pipe = 0;
>  	for_each_crtc(dev, crtc) {
> -		if (!intel_crtc_active(crtc))
> +		if (!crtc->state->active)
>  			continue;
>  
>  		if (crtc == for_crtc)
> @@ -2708,7 +2708,7 @@ static void skl_compute_wm_global_parameters(struct drm_device *dev,
>  	struct drm_plane *plane;
>  
>  	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
> -		config->num_pipes_active += intel_crtc_active(crtc);
> +		config->num_pipes_active += crtc->state->active;
>  
>  	/* FIXME: I don't think we need those two global parameters on SKL */
>  	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> @@ -2729,7 +2729,7 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
>  	struct drm_framebuffer *fb;
>  	int i = 1; /* Index for sprite planes start */
>  
> -	p->active = intel_crtc_active(crtc);
> +	p->active = crtc->state->active;
>  	if (p->active) {
>  		p->pipe_htotal = intel_crtc->config->base.adjusted_mode.crtc_htotal;
>  		p->pixel_rate = skl_pipe_pixel_rate(intel_crtc->config);
> @@ -2860,7 +2860,7 @@ static void skl_compute_wm_level(const struct drm_i915_private *dev_priv,
>  static uint32_t
>  skl_compute_linetime_wm(struct drm_crtc *crtc, struct skl_pipe_wm_parameters *p)
>  {
> -	if (!intel_crtc_active(crtc))
> +	if (!crtc->state->active)
>  		return 0;
>  
>  	return DIV_ROUND_UP(8 * p->pipe_htotal * 1000, p->pixel_rate);
> @@ -3191,7 +3191,7 @@ static void skl_update_other_pipe_wm(struct drm_device *dev,
>  		if (this_crtc->pipe == intel_crtc->pipe)
>  			continue;
>  
> -		if (!intel_crtc->base.state->active)
> +		if (!crtc->state->active)
>  			continue;
>  
>  		wm_changed = skl_update_pipe_wm(&intel_crtc->base,
> @@ -3407,7 +3407,7 @@ static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc)
>  		hw->plane_trans[pipe][i] = I915_READ(PLANE_WM_TRANS(pipe, i));
>  	hw->cursor_trans[pipe] = I915_READ(CUR_WM_TRANS(pipe));
>  
> -	if (!intel_crtc_active(crtc))
> +	if (!crtc->state->active)
>  		return;
>  
>  	hw->dirty[pipe] = true;
> @@ -3462,7 +3462,7 @@ static void ilk_pipe_wm_get_hw_state(struct drm_crtc *crtc)
>  	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>  		hw->wm_linetime[pipe] = I915_READ(PIPE_WM_LINETIME(pipe));
>  
> -	active->pipe_enabled = intel_crtc_active(crtc);
> +	active->pipe_enabled = crtc->state->active;
>  
>  	if (active->pipe_enabled) {
>  		u32 tmp = hw->wm_pipe[pipe];
> -- 
> 1.8.5.1

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

[PATCH 5/5] drm/i915: Don't assume primary & cursor are always on for wm calculation (v3)

[Matt Roper]

Current ILK-style watermark code assumes the primary plane and cursor
plane are always enabled.  This assumption, along with the combination
of two independent commits that got merged at the same time, results in
a NULL dereference.  The offending commits are:

        commit fd2d61341bf39d1054256c07d6eddd624ebc4241
        Author: Matt Roper <matthew.d.roper@intel.com>
        Date:   Fri Feb 27 10:12:01 2015 -0800

            drm/i915: Use plane->state->fb in watermark code (v2)

and

        commit 0fda65680e92545caea5be7805a7f0a617fb6c20
        Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
        Date:   Fri Feb 27 15:12:35 2015 +0000

            drm/i915/skl: Update watermarks for Y tiling

The first commit causes us to use the FB from plane->state->fb rather
than the legacy plane->fb, which is updated a bit later in the process.

The second commit includes a change that now triggers watermark
reprogramming on primary plane enable/disable where we didn't have one
before (which wasn't really correct, but we had been getting lucky
because we always calculated as if the primary plane was on).

Together, these two commits cause the watermark calculation to
(properly) see plane->state->fb = NULL when we're in the process of
disabling the primary plane.  However the existing watermark code
assumes there's always a primary fb and tries to dereference it to find
out pixel format / bpp information.

The fix is to make ILK-style watermark calculation actually check the
true status of primary & cursor planes and adjust our watermark logic
accordingly.

v2: Update unchecked uses of state->fb for other platforms (pnv, skl,
    etc.).  Note that this is just a temporary fix.  Ultimately the
    useful information is going to be computed at check time and stored
    right in the state structures so that we don't have to figure this
    all out while we're supposed to be programming the watermarks.
    (caught by Tvrtko)

v3: Fix a couple copy/paste mistakes in SKL code. (Tvrtko)

Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reported-by: Michael Leuchtenburg <michael@slashhome.org>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89388
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 113 +++++++++++++++++++++++++++++-----------
 1 file changed, 82 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index dafd7de..c5d3238 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -608,12 +608,17 @@ static void pineview_update_wm(struct drm_crtc *unused_crtc)
 	crtc = single_enabled_crtc(dev);
 	if (crtc) {
 		const struct drm_display_mode *adjusted_mode;
-		int pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
+		int pixel_size;
 		int clock;
 
 		adjusted_mode = &to_intel_crtc(crtc)->config->base.adjusted_mode;
 		clock = adjusted_mode->crtc_clock;
 
+		if (crtc->primary->state->fb)
+			pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
+		else
+			pixel_size = 4;
+
 		/* Display SR */
 		wm = intel_calculate_wm(clock, &pineview_display_wm,
 					pineview_display_wm.fifo_size,
@@ -684,7 +689,11 @@ static bool g4x_compute_wm0(struct drm_device *dev,
 	clock = adjusted_mode->crtc_clock;
 	htotal = adjusted_mode->crtc_htotal;
 	hdisplay = to_intel_crtc(crtc)->config->pipe_src_w;
-	pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
+
+	if (crtc->primary->state->fb)
+		pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
+	else
+		pixel_size = 4;
 
 	/* Use the small buffer method to calculate plane watermark */
 	entries = ((clock * pixel_size / 1000) * display_latency_ns) / 1000;
@@ -771,7 +780,11 @@ static bool g4x_compute_srwm(struct drm_device *dev,
 	clock = adjusted_mode->crtc_clock;
 	htotal = adjusted_mode->crtc_htotal;
 	hdisplay = to_intel_crtc(crtc)->config->pipe_src_w;
-	pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
+
+	if (crtc->primary->state->fb)
+		pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
+	else
+		pixel_size = 4;
 
 	line_time_us = max(htotal * 1000 / clock, 1);
 	line_count = (latency_ns / line_time_us + 1000) / 1000;
@@ -1118,10 +1131,15 @@ static void i965_update_wm(struct drm_crtc *unused_crtc)
 		int clock = adjusted_mode->crtc_clock;
 		int htotal = adjusted_mode->crtc_htotal;
 		int hdisplay = to_intel_crtc(crtc)->config->pipe_src_w;
-		int pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
+		int pixel_size;
 		unsigned long line_time_us;
 		int entries;
 
+		if (crtc->primary->state->fb)
+			pixel_size = crtc->primary->state->fb->bits_per_pixel / 8;
+		else
+			pixel_size = 4;
+
 		line_time_us = max(htotal * 1000 / clock, 1);
 
 		/* Use ns/us then divide to preserve precision */
@@ -1195,7 +1213,13 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
 	crtc = intel_get_crtc_for_plane(dev, 0);
 	if (intel_crtc_active(crtc)) {
 		const struct drm_display_mode *adjusted_mode;
-		int cpp = crtc->primary->state->fb->bits_per_pixel / 8;
+		int cpp;
+
+		if (crtc->primary->state->fb)
+			cpp = crtc->primary->state->fb->bits_per_pixel / 8;
+		else
+			cpp = 4;
+
 		if (IS_GEN2(dev))
 			cpp = 4;
 
@@ -1217,7 +1241,13 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
 	crtc = intel_get_crtc_for_plane(dev, 1);
 	if (intel_crtc_active(crtc)) {
 		const struct drm_display_mode *adjusted_mode;
-		int cpp = crtc->primary->state->fb->bits_per_pixel / 8;
+		int cpp;
+
+		if (crtc->primary->state->fb)
+			cpp = crtc->primary->state->fb->bits_per_pixel / 8;
+		else
+			cpp = 4;
+
 		if (IS_GEN2(dev))
 			cpp = 4;
 
@@ -1243,7 +1273,7 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
 		obj = intel_fb_obj(enabled->primary->state->fb);
 
 		/* self-refresh seems busted with untiled */
-		if (obj->tiling_mode == I915_TILING_NONE)
+		if (!obj || obj->tiling_mode == I915_TILING_NONE)
 			enabled = NULL;
 	}
 
@@ -1264,10 +1294,15 @@ static void i9xx_update_wm(struct drm_crtc *unused_crtc)
 		int clock = adjusted_mode->crtc_clock;
 		int htotal = adjusted_mode->crtc_htotal;
 		int hdisplay = to_intel_crtc(enabled)->config->pipe_src_w;
-		int pixel_size = enabled->primary->state->fb->bits_per_pixel / 8;
+		int pixel_size;
 		unsigned long line_time_us;
 		int entries;
 
+		if (enabled->primary->state->fb)
+			pixel_size = enabled->primary->state->fb->bits_per_pixel / 8;
+		else
+			pixel_size = 4;
+
 		line_time_us = max(htotal * 1000 / clock, 1);
 
 		/* Use ns/us then divide to preserve precision */
@@ -1962,13 +1997,25 @@ static void ilk_compute_wm_parameters(struct drm_crtc *crtc,
 	p->active = true;
 	p->pipe_htotal = intel_crtc->config->base.adjusted_mode.crtc_htotal;
 	p->pixel_rate = ilk_pipe_pixel_rate(dev, crtc);
-	p->pri.bytes_per_pixel = crtc->primary->state->fb->bits_per_pixel / 8;
-	p->cur.bytes_per_pixel = 4;
+
+	if (crtc->primary->state->fb) {
+		p->pri.enabled = true;
+		p->pri.bytes_per_pixel =
+			crtc->primary->state->fb->bits_per_pixel / 8;
+	} else {
+		p->pri.enabled = false;
+		p->pri.bytes_per_pixel = 0;
+	}
+
+	if (crtc->cursor->state->fb) {
+		p->cur.enabled = true;
+		p->cur.bytes_per_pixel = 4;
+	} else {
+		p->cur.enabled = false;
+		p->cur.bytes_per_pixel = 0;
+	}
 	p->pri.horiz_pixels = intel_crtc->config->pipe_src_w;
 	p->cur.horiz_pixels = intel_crtc->base.cursor->state->crtc_w;
-	/* TODO: for now, assume primary and cursor planes are always enabled. */
-	p->pri.enabled = true;
-	p->cur.enabled = true;
 
 	drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) {
 		struct intel_plane *intel_plane = to_intel_plane(plane);
@@ -2734,27 +2781,31 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
 		p->pipe_htotal = intel_crtc->config->base.adjusted_mode.crtc_htotal;
 		p->pixel_rate = skl_pipe_pixel_rate(intel_crtc->config);
 
-		/*
-		 * For now, assume primary and cursor planes are always enabled.
-		 */
-		p->plane[0].enabled = true;
-		p->plane[0].bytes_per_pixel =
-			crtc->primary->state->fb->bits_per_pixel / 8;
-		p->plane[0].horiz_pixels = intel_crtc->config->pipe_src_w;
-		p->plane[0].vert_pixels = intel_crtc->config->pipe_src_h;
-		p->plane[0].tiling = DRM_FORMAT_MOD_NONE;
 		fb = crtc->primary->state->fb;
-		/*
-		 * Framebuffer can be NULL on plane disable, but it does not
-		 * matter for watermarks if we assume no tiling in that case.
-		 */
-		if (fb)
+		if (fb) {
+			p->plane[0].enabled = true;
+			p->plane[0].bytes_per_pixel = fb->bits_per_pixel / 8;
 			p->plane[0].tiling = fb->modifier[0];
+		} else {
+			p->plane[0].enabled = false;
+			p->plane[0].bytes_per_pixel = 0;
+			p->plane[0].tiling = DRM_FORMAT_MOD_NONE;
+		}
+		p->plane[0].horiz_pixels = intel_crtc->config->pipe_src_w;
+		p->plane[0].vert_pixels = intel_crtc->config->pipe_src_h;
 
-		p->cursor.enabled = true;
-		p->cursor.bytes_per_pixel = 4;
-		p->cursor.horiz_pixels = intel_crtc->base.cursor->state->crtc_w ?
-					 intel_crtc->base.cursor->state->crtc_w : 64;
+		fb = crtc->cursor->state->fb;
+		if (fb) {
+			p->cursor.enabled = true;
+			p->cursor.bytes_per_pixel = fb->bits_per_pixel / 8;
+			p->cursor.horiz_pixels = crtc->cursor->state->crtc_w;
+			p->cursor.vert_pixels = crtc->cursor->state->crtc_h;
+		} else {
+			p->cursor.enabled = false;
+			p->cursor.bytes_per_pixel = 0;
+			p->cursor.horiz_pixels = 64;
+			p->cursor.vert_pixels = 64;
+		}
 	}
 
 	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
-- 
1.8.5.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: Don't assume primary & cursor are always on for wm calculation (v3)

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5912
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  282/282              282/282
ILK                                  308/308              308/308
SNB                                  307/307              307/307
IVB                 -1              375/375              374/375
BYT                                  294/294              294/294
HSW                                  385/385              385/385
BDW                                  315/315              315/315
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*IVB  igt_gem_userptr_blits_minor-normal-sync      PASS(2)      DMESG_WARN(1)PASS(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: Don't assume primary & cursor are always on for wm calculation (v3)

[Chris Wilson]

On Sun, Mar 08, 2015 at 05:17:05PM -0700, shuang.he@intel.com wrote:
> Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
> Task id: 5912
> -------------------------------------Summary-------------------------------------
> Platform          Delta          drm-intel-nightly          Series Applied
> PNV                                  282/282              282/282
> ILK                                  308/308              308/308
> SNB                                  307/307              307/307
> IVB                 -1              375/375              374/375
> BYT                                  294/294              294/294
> HSW                                  385/385              385/385
> BDW                                  315/315              315/315
> -------------------------------------Detailed-------------------------------------

Considering that this series targets a panic that has also been reported
by QA, this result is even more disappointing than usual.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx