[PATCH v3 00/20] Convert to atomic, part 4.

[PATCH v3 00/20] Convert to atomic, part 4.

[Maarten Lankhorst]

Atomic suspend/resume, full hardware readout and atomic ioctl support.

Changes from the previous version:
- The fastboot changes from the previous patch have been removed,
  fastboot will have to be a separate patch because of the testing it needs.
- I've cleaned up the changes to planes and split it into separate patches.
  This makes it easier to bisect.
- Some commit logs have been updated.

Maarten Lankhorst (20):
  drm/i915: Only update state on crtc's that are part of the atomic
    state.
  drm/i915: Do not update pfit state when toggling crtc enabled.
  drm/i915: Do not use plane_config in intel_fbdev.c
  drm/i915: Allow fuzzy matching in pipe_config_compare, v2.
  drm/i915: Update missing properties in find_initial_plane_obj
  drm/i915: Remove plane_config from struct intel_crtc.
  drm/i915: Rework plane readout.
  drm/i915: fill in more mode members
  drm/i915: Fill in more crtc state, v3.
  drm/i915: Set csc coefficients in intel_sanitize_crtc.
  drm/i915: Readout initial hw mode.
  drm/i915: Convert resume to atomic.
  drm/i915: Get rid of unused transitional members.
  drm/i915: Update power domains on readout.
  drm/i915: Always reset in intel_crtc_restore_mode
  drm/i915: Make intel_display_suspend atomic, try 2.
  drm/i915: Use full atomic modeset.
  drm/i915: Call plane update functions directly from
    intel_atomic_commit.
  drm/i915: always disable irqs in intel_pipe_update_start
  drm/i915: Remove use of runtime pm in atomic commit functions

 drivers/gpu/drm/i915/i915_drv.c      |    4 +-
 drivers/gpu/drm/i915/i915_drv.h      |    4 +-
 drivers/gpu/drm/i915/i915_params.c   |    5 -
 drivers/gpu/drm/i915/intel_atomic.c  |  144 +----
 drivers/gpu/drm/i915/intel_display.c | 1083 +++++++++++++++-------------------
 drivers/gpu/drm/i915/intel_dp.c      |    2 +-
 drivers/gpu/drm/i915/intel_drv.h     |   25 +-
 drivers/gpu/drm/i915/intel_fbdev.c   |   25 +-
 drivers/gpu/drm/i915/intel_lvds.c    |    2 +-
 drivers/gpu/drm/i915/intel_sprite.c  |   17 +-
 10 files changed, 517 insertions(+), 794 deletions(-)

-- 
2.1.0

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

[PATCH v3 01/20] drm/i915: Only update state on crtc's that are part of the atomic state.

[Maarten Lankhorst]

This is probably hard to hit right now because in most cases all
atomic locks are taken, but after conversion to atomic this will make
it more likely to corrupt the crtc->config pointer, resulting in hard
to find bugs.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 887ba0a54352..f9b1db734502 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12314,6 +12314,7 @@ intel_modeset_update_state(struct drm_atomic_state *state)
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
 	struct drm_connector *connector;
+	int i;
 
 	intel_shared_dpll_commit(state);
 
@@ -12333,7 +12334,7 @@ intel_modeset_update_state(struct drm_atomic_state *state)
 	intel_modeset_update_staged_output_state(state->dev);
 
 	/* Double check state. */
-	for_each_crtc(dev, crtc) {
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
 		WARN_ON(crtc->state->enable != intel_crtc_in_use(crtc));
 
 		to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state);
-- 
2.1.0

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

[PATCH v3 02/20] drm/i915: Do not update pfit state when toggling crtc enabled.

[Maarten Lankhorst]

There's not much point for calculating the changes for the old
state. Instead just disable all scalers when disabling. It's
probably good enough to just disable the crtc_scaler, but just in
case there's a bug disable all scalers.

This means intel_atomic_setup_scalers is only called in the crtc
check function now, so all the transitional code can be removed.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic.c  | 14 ++------
 drivers/gpu/drm/i915/intel_display.c | 68 +++++++++++++++++++++++-------------
 drivers/gpu/drm/i915/intel_dp.c      |  2 +-
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-
 4 files changed, 48 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 5c79a31603af..b92b8581efc2 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -272,17 +272,12 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
 	struct drm_plane *plane = NULL;
 	struct intel_plane *intel_plane;
 	struct intel_plane_state *plane_state = NULL;
-	struct intel_crtc_scaler_state *scaler_state;
-	struct drm_atomic_state *drm_state;
+	struct intel_crtc_scaler_state *scaler_state =
+		&crtc_state->scaler_state;
+	struct drm_atomic_state *drm_state = crtc_state->base.state;
 	int num_scalers_need;
 	int i, j;
 
-	if (INTEL_INFO(dev)->gen < 9 || !intel_crtc || !crtc_state)
-		return 0;
-
-	scaler_state = &crtc_state->scaler_state;
-	drm_state = crtc_state->base.state;
-
 	num_scalers_need = hweight32(scaler_state->scaler_users);
 	DRM_DEBUG_KMS("crtc_state = %p need = %d avail = %d scaler_users = 0x%x\n",
 		crtc_state, num_scalers_need, intel_crtc->num_scalers,
@@ -326,9 +321,6 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
 		} else {
 			name = "PLANE";
 
-			if (!drm_state)
-				continue;
-
 			/* plane scaler case: assign as a plane scaler */
 			/* find the plane that set the bit as scaler_user */
 			plane = drm_state->planes[i];
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f9b1db734502..6eed925f3300 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2909,29 +2909,32 @@ unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
 	return i915_gem_obj_ggtt_offset_view(obj, view);
 }
 
+static void skl_detach_scaler(struct intel_crtc *intel_crtc, int id)
+{
+	struct drm_device *dev = intel_crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	I915_WRITE(SKL_PS_CTRL(intel_crtc->pipe, id), 0);
+	I915_WRITE(SKL_PS_WIN_POS(intel_crtc->pipe, id), 0);
+	I915_WRITE(SKL_PS_WIN_SZ(intel_crtc->pipe, id), 0);
+	DRM_DEBUG_KMS("CRTC:%d Disabled scaler id %u.%u\n",
+		intel_crtc->base.base.id, intel_crtc->pipe, id);
+}
+
 /*
  * This function detaches (aka. unbinds) unused scalers in hardware
  */
 static void skl_detach_scalers(struct intel_crtc *intel_crtc)
 {
-	struct drm_device *dev;
-	struct drm_i915_private *dev_priv;
 	struct intel_crtc_scaler_state *scaler_state;
 	int i;
 
-	dev = intel_crtc->base.dev;
-	dev_priv = dev->dev_private;
 	scaler_state = &intel_crtc->config->scaler_state;
 
 	/* loop through and disable scalers that aren't in use */
 	for (i = 0; i < intel_crtc->num_scalers; i++) {
-		if (!scaler_state->scalers[i].in_use) {
-			I915_WRITE(SKL_PS_CTRL(intel_crtc->pipe, i), 0);
-			I915_WRITE(SKL_PS_WIN_POS(intel_crtc->pipe, i), 0);
-			I915_WRITE(SKL_PS_WIN_SZ(intel_crtc->pipe, i), 0);
-			DRM_DEBUG_KMS("CRTC:%d Disabled scaler id %u.%u\n",
-				intel_crtc->base.base.id, intel_crtc->pipe, i);
-		}
+		if (!scaler_state->scalers[i].in_use)
+			skl_detach_scaler(intel_crtc, i);
 	}
 }
 
@@ -4362,13 +4365,12 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
  * skl_update_scaler_crtc - Stages update to scaler state for a given crtc.
  *
  * @state: crtc's scaler state
- * @force_detach: whether to forcibly disable scaler
  *
  * Return
  *     0 - scaler_usage updated successfully
  *    error - requested scaling cannot be supported or other error condition
  */
-int skl_update_scaler_crtc(struct intel_crtc_state *state, int force_detach)
+int skl_update_scaler_crtc(struct intel_crtc_state *state)
 {
 	struct intel_crtc *intel_crtc = to_intel_crtc(state->base.crtc);
 	struct drm_display_mode *adjusted_mode =
@@ -4377,7 +4379,7 @@ int skl_update_scaler_crtc(struct intel_crtc_state *state, int force_detach)
 	DRM_DEBUG_KMS("Updating scaler for [CRTC:%i] scaler_user index %u.%u\n",
 		      intel_crtc->base.base.id, intel_crtc->pipe, SKL_CRTC_INDEX);
 
-	return skl_update_scaler(state, force_detach, SKL_CRTC_INDEX,
+	return skl_update_scaler(state, !state->base.active, SKL_CRTC_INDEX,
 		&state->scaler_state.scaler_id, DRM_ROTATE_0,
 		state->pipe_src_w, state->pipe_src_h,
 		adjusted_mode->hdisplay, adjusted_mode->vdisplay);
@@ -4451,7 +4453,15 @@ static int skl_update_scaler_plane(struct intel_crtc_state *crtc_state,
 	return 0;
 }
 
-static void skylake_pfit_update(struct intel_crtc *crtc, int enable)
+static void skylake_scaler_disable(struct intel_crtc *crtc)
+{
+	int i;
+
+	for (i = 0; i < crtc->num_scalers; i++)
+		skl_detach_scaler(crtc, i);
+}
+
+static void skylake_pfit_enable(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -4461,13 +4471,6 @@ static void skylake_pfit_update(struct intel_crtc *crtc, int enable)
 
 	DRM_DEBUG_KMS("for crtc_state = %p\n", crtc->config);
 
-	/* To update pfit, first update scaler state */
-	skl_update_scaler_crtc(crtc->config, !enable);
-	intel_atomic_setup_scalers(crtc->base.dev, crtc, crtc->config);
-	skl_detach_scalers(crtc);
-	if (!enable)
-		return;
-
 	if (crtc->config->pch_pfit.enabled) {
 		int id;
 
@@ -4942,7 +4945,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	intel_ddi_enable_pipe_clock(intel_crtc);
 
 	if (INTEL_INFO(dev)->gen == 9)
-		skylake_pfit_update(intel_crtc, 1);
+		skylake_pfit_enable(intel_crtc);
 	else if (INTEL_INFO(dev)->gen < 9)
 		ironlake_pfit_enable(intel_crtc);
 	else
@@ -5079,7 +5082,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 	intel_ddi_disable_transcoder_func(dev_priv, cpu_transcoder);
 
 	if (INTEL_INFO(dev)->gen == 9)
-		skylake_pfit_update(intel_crtc, 0);
+		skylake_scaler_disable(intel_crtc);
 	else if (INTEL_INFO(dev)->gen < 9)
 		ironlake_pfit_disable(intel_crtc);
 	else
@@ -11836,7 +11839,17 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 			return ret;
 	}
 
-	return intel_atomic_setup_scalers(dev, intel_crtc, pipe_config);
+	ret = 0;
+	if (INTEL_INFO(dev)->gen >= 9) {
+		if (mode_changed)
+			ret = skl_update_scaler_crtc(pipe_config);
+
+		if (!ret)
+			ret = intel_atomic_setup_scalers(dev, intel_crtc,
+							 pipe_config);
+	}
+
+	return ret;
 }
 
 static const struct drm_crtc_helper_funcs intel_helper_funcs = {
@@ -15357,6 +15370,11 @@ static void readout_plane_state(struct intel_crtc *crtc,
 			continue;
 
 		drm_plane_state = p->base.state;
+
+		/* Plane scaler state is not touched here. The first atomic
+		 * commit will restore all plane scalers to its old state.
+		 */
+
 		if (active && p->base.type == DRM_PLANE_TYPE_PRIMARY) {
 			visible = primary_get_hw_state(crtc);
 			to_intel_plane_state(drm_plane_state)->visible = visible;
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index fcc64e5ef9c8..f1b9f939b435 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1384,7 +1384,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 
 		if (INTEL_INFO(dev)->gen >= 9) {
 			int ret;
-			ret = skl_update_scaler_crtc(pipe_config, 0);
+			ret = skl_update_scaler_crtc(pipe_config);
 			if (ret)
 				return ret;
 		}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index b9c01c5b881f..09a0a9222a3a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1149,7 +1149,7 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode,
 void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
 void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);
 
-int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state, int force_detach);
+int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state);
 int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state);
 
 unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
-- 
2.1.0

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

[PATCH v3 03/20] drm/i915: Do not use plane_config in intel_fbdev.c

[Maarten Lankhorst]

Use the atomic state instead, this allows removing plane_config
from the crtc after the full hw readout is completed.

The size can be found in the fb, no need for the plane_config.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 33b3c9233eac..b791f2374f3b 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -581,7 +581,6 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
 	struct intel_framebuffer *fb = NULL;
 	struct drm_crtc *crtc;
 	struct intel_crtc *intel_crtc;
-	struct intel_initial_plane_config *plane_config = NULL;
 	unsigned int max_size = 0;
 
 	if (!i915.fastboot)
@@ -589,20 +588,21 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
 
 	/* Find the largest fb */
 	for_each_crtc(dev, crtc) {
+		struct drm_i915_gem_object *obj =
+			intel_fb_obj(crtc->primary->state->fb);
 		intel_crtc = to_intel_crtc(crtc);
 
-		if (!intel_crtc->active || !crtc->primary->fb) {
+		if (!intel_crtc->active || !obj) {
 			DRM_DEBUG_KMS("pipe %c not active or no fb, skipping\n",
 				      pipe_name(intel_crtc->pipe));
 			continue;
 		}
 
-		if (intel_crtc->plane_config.size > max_size) {
+		if (obj->base.size > max_size) {
 			DRM_DEBUG_KMS("found possible fb from plane %c\n",
 				      pipe_name(intel_crtc->pipe));
-			plane_config = &intel_crtc->plane_config;
-			fb = to_intel_framebuffer(crtc->primary->fb);
-			max_size = plane_config->size;
+			fb = to_intel_framebuffer(crtc->primary->state->fb);
+			max_size = obj->base.size;
 		}
 	}
 
@@ -637,7 +637,6 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
 			DRM_DEBUG_KMS("fb not wide enough for plane %c (%d vs %d)\n",
 				      pipe_name(intel_crtc->pipe),
 				      cur_size, fb->base.pitches[0]);
-			plane_config = NULL;
 			fb = NULL;
 			break;
 		}
@@ -658,7 +657,6 @@ static bool intel_fbdev_init_bios(struct drm_device *dev,
 			DRM_DEBUG_KMS("fb not big enough for plane %c (%d vs %d)\n",
 				      pipe_name(intel_crtc->pipe),
 				      cur_size, max_size);
-			plane_config = NULL;
 			fb = NULL;
 			break;
 		}
-- 
2.1.0

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

[PATCH v3 04/20] drm/i915: Allow fuzzy matching in pipe_config_compare, v2.

[Maarten Lankhorst]

Instead of doing ad-hoc checks we already have a way of checking
if the state is compatible or not. Use this to force a modeset.

Only during modesets, or with PIPE_CONFIG_QUIRK_INHERITED_MODE
we should check if a full modeset is really needed.

Fastboot will allow the adjust parameter to ignore some stuff
too, and it will fix up differences in state that are ignored
by the compare function.

Changes since v1:
- Increase the value of the lowest m/n to prevent truncation.
- Dump pipe config when fastboot's used, without a modeset.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 218 +++++++++++++++++++++++++----------
 1 file changed, 157 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6eed925f3300..c3a3d1087777 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12290,19 +12290,6 @@ encoder_retry:
 	DRM_DEBUG_KMS("plane bpp: %i, pipe bpp: %i, dithering: %i\n",
 		      base_bpp, pipe_config->pipe_bpp, pipe_config->dither);
 
-	/* Check if we need to force a modeset */
-	if (pipe_config->has_audio !=
-	    to_intel_crtc_state(crtc->state)->has_audio) {
-		pipe_config->base.mode_changed = true;
-		ret = drm_atomic_add_affected_planes(state, crtc);
-	}
-
-	/*
-	 * Note we have an issue here with infoframes: current code
-	 * only updates them on the full mode set path per hw
-	 * requirements.  So here we should be checking for any
-	 * required changes and forcing a mode set.
-	 */
 fail:
 	return ret;
 }
@@ -12406,27 +12393,133 @@ static bool intel_fuzzy_clock_check(int clock1, int clock2)
 			    base.head) \
 		if (mask & (1 <<(intel_crtc)->pipe))
 
+
+static bool
+intel_compare_m_n(unsigned int m, unsigned int n,
+		  unsigned int m2, unsigned int n2,
+		  bool exact)
+{
+	if (m == m2 && n == n2)
+		return true;
+
+	if (exact || !m || !n || !m2 || !n2)
+		return false;
+
+	BUILD_BUG_ON(DATA_LINK_M_N_MASK > INT_MAX);
+
+	if (m > m2) {
+		while (m > m2) {
+			m2 <<= 1;
+			n2 <<= 1;
+		}
+	} else if (m < m2) {
+		while (m < m2) {
+			m <<= 1;
+			n <<= 1;
+		}
+	}
+
+	return m == m2 && n == n2;
+}
+
+static bool
+intel_compare_link_m_n(const struct intel_link_m_n *m_n,
+		       struct intel_link_m_n *m2_n2,
+		       bool adjust)
+{
+	if (m_n->tu == m2_n2->tu &&
+	    intel_compare_m_n(m_n->gmch_m, m_n->gmch_n,
+			      m2_n2->gmch_m, m2_n2->gmch_n, !adjust) &&
+	    intel_compare_m_n(m_n->link_m, m_n->link_n,
+			      m2_n2->link_m, m2_n2->link_n, !adjust)) {
+		if (adjust)
+			*m2_n2 = *m_n;
+
+		return true;
+	}
+
+	return false;
+}
+
 static bool
 intel_pipe_config_compare(struct drm_device *dev,
 			  struct intel_crtc_state *current_config,
-			  struct intel_crtc_state *pipe_config)
+			  struct intel_crtc_state *pipe_config,
+			  bool adjust)
 {
+	bool ret = true;
+
+#define INTEL_ERR_OR_DBG_KMS(fmt, ...) \
+	do { \
+		if (!adjust) \
+			DRM_ERROR(fmt, ##__VA_ARGS__); \
+		else \
+			DRM_DEBUG_KMS(fmt, ##__VA_ARGS__); \
+	} while (0)
+
 #define PIPE_CONF_CHECK_X(name)	\
 	if (current_config->name != pipe_config->name) { \
-		DRM_ERROR("mismatch in " #name " " \
+		INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
 			  "(expected 0x%08x, found 0x%08x)\n", \
 			  current_config->name, \
 			  pipe_config->name); \
-		return false; \
+		ret = false; \
 	}
 
 #define PIPE_CONF_CHECK_I(name)	\
 	if (current_config->name != pipe_config->name) { \
-		DRM_ERROR("mismatch in " #name " " \
+		INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
 			  "(expected %i, found %i)\n", \
 			  current_config->name, \
 			  pipe_config->name); \
-		return false; \
+		ret = false; \
+	}
+
+#define PIPE_CONF_CHECK_M_N(name) \
+	if (!intel_compare_link_m_n(&current_config->name, \
+				    &pipe_config->name,\
+				    adjust)) { \
+		INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
+			  "(expected tu %i gmch %i/%i link %i/%i, " \
+			  "found tu %i, gmch %i/%i link %i/%i)\n", \
+			  current_config->name.tu, \
+			  current_config->name.gmch_m, \
+			  current_config->name.gmch_n, \
+			  current_config->name.link_m, \
+			  current_config->name.link_n, \
+			  pipe_config->name.tu, \
+			  pipe_config->name.gmch_m, \
+			  pipe_config->name.gmch_n, \
+			  pipe_config->name.link_m, \
+			  pipe_config->name.link_n); \
+		ret = false; \
+	}
+
+#define PIPE_CONF_CHECK_M_N_ALT(name, alt_name) \
+	if (!intel_compare_link_m_n(&current_config->name, \
+				    &pipe_config->name, adjust) && \
+	    !intel_compare_link_m_n(&current_config->alt_name, \
+				    &pipe_config->name, adjust)) { \
+		INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
+			  "(expected tu %i gmch %i/%i link %i/%i, " \
+			  "or tu %i gmch %i/%i link %i/%i, " \
+			  "found tu %i, gmch %i/%i link %i/%i)\n", \
+			  current_config->name.tu, \
+			  current_config->name.gmch_m, \
+			  current_config->name.gmch_n, \
+			  current_config->name.link_m, \
+			  current_config->name.link_n, \
+			  current_config->alt_name.tu, \
+			  current_config->alt_name.gmch_m, \
+			  current_config->alt_name.gmch_n, \
+			  current_config->alt_name.link_m, \
+			  current_config->alt_name.link_n, \
+			  pipe_config->name.tu, \
+			  pipe_config->name.gmch_m, \
+			  pipe_config->name.gmch_n, \
+			  pipe_config->name.link_m, \
+			  pipe_config->name.link_n); \
+		ret = false; \
 	}
 
 /* This is required for BDW+ where there is only one set of registers for
@@ -12437,30 +12530,30 @@ intel_pipe_config_compare(struct drm_device *dev,
 #define PIPE_CONF_CHECK_I_ALT(name, alt_name) \
 	if ((current_config->name != pipe_config->name) && \
 		(current_config->alt_name != pipe_config->name)) { \
-			DRM_ERROR("mismatch in " #name " " \
+			INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
 				  "(expected %i or %i, found %i)\n", \
 				  current_config->name, \
 				  current_config->alt_name, \
 				  pipe_config->name); \
-			return false; \
+			ret = false; \
 	}
 
 #define PIPE_CONF_CHECK_FLAGS(name, mask)	\
 	if ((current_config->name ^ pipe_config->name) & (mask)) { \
-		DRM_ERROR("mismatch in " #name "(" #mask ") "	   \
+		INTEL_ERR_OR_DBG_KMS("mismatch in " #name "(" #mask ") " \
 			  "(expected %i, found %i)\n", \
 			  current_config->name & (mask), \
 			  pipe_config->name & (mask)); \
-		return false; \
+		ret = false; \
 	}
 
 #define PIPE_CONF_CHECK_CLOCK_FUZZY(name) \
 	if (!intel_fuzzy_clock_check(current_config->name, pipe_config->name)) { \
-		DRM_ERROR("mismatch in " #name " " \
+		INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
 			  "(expected %i, found %i)\n", \
 			  current_config->name, \
 			  pipe_config->name); \
-		return false; \
+		ret = false; \
 	}
 
 #define PIPE_CONF_QUIRK(quirk)	\
@@ -12470,35 +12563,18 @@ intel_pipe_config_compare(struct drm_device *dev,
 
 	PIPE_CONF_CHECK_I(has_pch_encoder);
 	PIPE_CONF_CHECK_I(fdi_lanes);
-	PIPE_CONF_CHECK_I(fdi_m_n.gmch_m);
-	PIPE_CONF_CHECK_I(fdi_m_n.gmch_n);
-	PIPE_CONF_CHECK_I(fdi_m_n.link_m);
-	PIPE_CONF_CHECK_I(fdi_m_n.link_n);
-	PIPE_CONF_CHECK_I(fdi_m_n.tu);
+	PIPE_CONF_CHECK_M_N(fdi_m_n);
 
 	PIPE_CONF_CHECK_I(has_dp_encoder);
 
 	if (INTEL_INFO(dev)->gen < 8) {
-		PIPE_CONF_CHECK_I(dp_m_n.gmch_m);
-		PIPE_CONF_CHECK_I(dp_m_n.gmch_n);
-		PIPE_CONF_CHECK_I(dp_m_n.link_m);
-		PIPE_CONF_CHECK_I(dp_m_n.link_n);
-		PIPE_CONF_CHECK_I(dp_m_n.tu);
-
-		if (current_config->has_drrs) {
-			PIPE_CONF_CHECK_I(dp_m2_n2.gmch_m);
-			PIPE_CONF_CHECK_I(dp_m2_n2.gmch_n);
-			PIPE_CONF_CHECK_I(dp_m2_n2.link_m);
-			PIPE_CONF_CHECK_I(dp_m2_n2.link_n);
-			PIPE_CONF_CHECK_I(dp_m2_n2.tu);
-		}
-	} else {
-		PIPE_CONF_CHECK_I_ALT(dp_m_n.gmch_m, dp_m2_n2.gmch_m);
-		PIPE_CONF_CHECK_I_ALT(dp_m_n.gmch_n, dp_m2_n2.gmch_n);
-		PIPE_CONF_CHECK_I_ALT(dp_m_n.link_m, dp_m2_n2.link_m);
-		PIPE_CONF_CHECK_I_ALT(dp_m_n.link_n, dp_m2_n2.link_n);
-		PIPE_CONF_CHECK_I_ALT(dp_m_n.tu, dp_m2_n2.tu);
-	}
+		PIPE_CONF_CHECK_M_N(dp_m_n);
+
+		PIPE_CONF_CHECK_I(has_drrs);
+		if (current_config->has_drrs)
+			PIPE_CONF_CHECK_M_N(dp_m2_n2);
+	} else
+		PIPE_CONF_CHECK_M_N_ALT(dp_m_n, dp_m2_n2);
 
 	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hdisplay);
 	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_htotal);
@@ -12594,8 +12670,9 @@ intel_pipe_config_compare(struct drm_device *dev,
 #undef PIPE_CONF_CHECK_FLAGS
 #undef PIPE_CONF_CHECK_CLOCK_FUZZY
 #undef PIPE_CONF_QUIRK
+#undef INTEL_ERR_OR_DBG_KMS
 
-	return true;
+	return ret;
 }
 
 static void check_wm_state(struct drm_device *dev)
@@ -12787,8 +12864,11 @@ check_crtc_state(struct drm_device *dev)
 		     "transitional active state does not match atomic hw state "
 		     "(expected %i, found %i)\n", crtc->base.state->active, crtc->active);
 
-		if (active &&
-		    !intel_pipe_config_compare(dev, crtc->config, &pipe_config)) {
+		if (!active)
+			continue;
+
+		if (!intel_pipe_config_compare(dev, crtc->config,
+					       &pipe_config, false)) {
 			I915_STATE_WARN(1, "pipe state doesn't match!\n");
 			intel_dump_pipe_config(crtc, &pipe_config,
 					       "[hw state]");
@@ -13089,14 +13169,17 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
 		return ret;
 
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		struct intel_crtc_state *pipe_config =
+			to_intel_crtc_state(crtc_state);
+		bool modeset, recalc;
+
 		if (!crtc_state->enable) {
 			if (needs_modeset(crtc_state))
 				any_ms = true;
 			continue;
 		}
 
-		if (to_intel_crtc_state(crtc_state)->quirks &
-		    PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
+		if (pipe_config->quirks & PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
 			ret = drm_atomic_add_affected_planes(state, crtc);
 			if (ret)
 				return ret;
@@ -13109,23 +13192,36 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
 			 */
 		}
 
-		if (!needs_modeset(crtc_state)) {
+		modeset = needs_modeset(crtc_state);
+		recalc = pipe_config->quirks & PIPE_CONFIG_QUIRK_INHERITED_MODE;
+
+		if (!modeset && !recalc)
+			continue;
+
+		if (recalc) {
 			ret = drm_atomic_add_affected_connectors(state, crtc);
 			if (ret)
 				return ret;
 		}
 
-		ret = intel_modeset_pipe_config(crtc,
-					to_intel_crtc_state(crtc_state));
+		ret = intel_modeset_pipe_config(crtc, pipe_config);
 		if (ret)
 			return ret;
 
-		if (needs_modeset(crtc_state))
-			any_ms = true;
+		if (recalc && !intel_pipe_config_compare(state->dev,
+					to_intel_crtc_state(crtc->state),
+					pipe_config, true)) {
+			modeset = crtc_state->mode_changed = true;
+
+			ret = drm_atomic_add_affected_planes(state, crtc);
+			if (ret)
+				return ret;
+		}
 
+		any_ms = modeset;
 		intel_dump_pipe_config(to_intel_crtc(crtc),
-				       to_intel_crtc_state(crtc_state),
-				       "[modeset]");
+				       pipe_config,
+				       modeset ? "[modeset]" : "[fastboot]");
 	}
 
 	if (any_ms) {
-- 
2.1.0

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

Re: [PATCH v3 04/20] drm/i915: Allow fuzzy matching in pipe_config_compare, v2.

[Daniel Vetter]

On Mon, Jul 13, 2015 at 04:30:17PM +0200, Maarten Lankhorst wrote:
> Instead of doing ad-hoc checks we already have a way of checking
> if the state is compatible or not. Use this to force a modeset.
> 
> Only during modesets, or with PIPE_CONFIG_QUIRK_INHERITED_MODE
> we should check if a full modeset is really needed.
> 
> Fastboot will allow the adjust parameter to ignore some stuff
> too, and it will fix up differences in state that are ignored
> by the compare function.
> 
> Changes since v1:
> - Increase the value of the lowest m/n to prevent truncation.
> - Dump pipe config when fastboot's used, without a modeset.

Needs to mention that the dp M/N comparison helper grew the "exact"
parameter.
-Daniel

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 218 +++++++++++++++++++++++++----------
>  1 file changed, 157 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 6eed925f3300..c3a3d1087777 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12290,19 +12290,6 @@ encoder_retry:
>  	DRM_DEBUG_KMS("plane bpp: %i, pipe bpp: %i, dithering: %i\n",
>  		      base_bpp, pipe_config->pipe_bpp, pipe_config->dither);
>  
> -	/* Check if we need to force a modeset */
> -	if (pipe_config->has_audio !=
> -	    to_intel_crtc_state(crtc->state)->has_audio) {
> -		pipe_config->base.mode_changed = true;
> -		ret = drm_atomic_add_affected_planes(state, crtc);
> -	}
> -
> -	/*
> -	 * Note we have an issue here with infoframes: current code
> -	 * only updates them on the full mode set path per hw
> -	 * requirements.  So here we should be checking for any
> -	 * required changes and forcing a mode set.
> -	 */
>  fail:
>  	return ret;
>  }
> @@ -12406,27 +12393,133 @@ static bool intel_fuzzy_clock_check(int clock1, int clock2)
>  			    base.head) \
>  		if (mask & (1 <<(intel_crtc)->pipe))
>  
> +
> +static bool
> +intel_compare_m_n(unsigned int m, unsigned int n,
> +		  unsigned int m2, unsigned int n2,
> +		  bool exact)
> +{
> +	if (m == m2 && n == n2)
> +		return true;
> +
> +	if (exact || !m || !n || !m2 || !n2)
> +		return false;
> +
> +	BUILD_BUG_ON(DATA_LINK_M_N_MASK > INT_MAX);
> +
> +	if (m > m2) {
> +		while (m > m2) {
> +			m2 <<= 1;
> +			n2 <<= 1;
> +		}
> +	} else if (m < m2) {
> +		while (m < m2) {
> +			m <<= 1;
> +			n <<= 1;
> +		}
> +	}
> +
> +	return m == m2 && n == n2;
> +}
> +
> +static bool
> +intel_compare_link_m_n(const struct intel_link_m_n *m_n,
> +		       struct intel_link_m_n *m2_n2,
> +		       bool adjust)
> +{
> +	if (m_n->tu == m2_n2->tu &&
> +	    intel_compare_m_n(m_n->gmch_m, m_n->gmch_n,
> +			      m2_n2->gmch_m, m2_n2->gmch_n, !adjust) &&
> +	    intel_compare_m_n(m_n->link_m, m_n->link_n,
> +			      m2_n2->link_m, m2_n2->link_n, !adjust)) {
> +		if (adjust)
> +			*m2_n2 = *m_n;
> +
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
>  static bool
>  intel_pipe_config_compare(struct drm_device *dev,
>  			  struct intel_crtc_state *current_config,
> -			  struct intel_crtc_state *pipe_config)
> +			  struct intel_crtc_state *pipe_config,
> +			  bool adjust)
>  {
> +	bool ret = true;
> +
> +#define INTEL_ERR_OR_DBG_KMS(fmt, ...) \
> +	do { \
> +		if (!adjust) \
> +			DRM_ERROR(fmt, ##__VA_ARGS__); \
> +		else \
> +			DRM_DEBUG_KMS(fmt, ##__VA_ARGS__); \
> +	} while (0)
> +
>  #define PIPE_CONF_CHECK_X(name)	\
>  	if (current_config->name != pipe_config->name) { \
> -		DRM_ERROR("mismatch in " #name " " \
> +		INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
>  			  "(expected 0x%08x, found 0x%08x)\n", \
>  			  current_config->name, \
>  			  pipe_config->name); \
> -		return false; \
> +		ret = false; \
>  	}
>  
>  #define PIPE_CONF_CHECK_I(name)	\
>  	if (current_config->name != pipe_config->name) { \
> -		DRM_ERROR("mismatch in " #name " " \
> +		INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
>  			  "(expected %i, found %i)\n", \
>  			  current_config->name, \
>  			  pipe_config->name); \
> -		return false; \
> +		ret = false; \
> +	}
> +
> +#define PIPE_CONF_CHECK_M_N(name) \
> +	if (!intel_compare_link_m_n(&current_config->name, \
> +				    &pipe_config->name,\
> +				    adjust)) { \
> +		INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
> +			  "(expected tu %i gmch %i/%i link %i/%i, " \
> +			  "found tu %i, gmch %i/%i link %i/%i)\n", \
> +			  current_config->name.tu, \
> +			  current_config->name.gmch_m, \
> +			  current_config->name.gmch_n, \
> +			  current_config->name.link_m, \
> +			  current_config->name.link_n, \
> +			  pipe_config->name.tu, \
> +			  pipe_config->name.gmch_m, \
> +			  pipe_config->name.gmch_n, \
> +			  pipe_config->name.link_m, \
> +			  pipe_config->name.link_n); \
> +		ret = false; \
> +	}
> +
> +#define PIPE_CONF_CHECK_M_N_ALT(name, alt_name) \
> +	if (!intel_compare_link_m_n(&current_config->name, \
> +				    &pipe_config->name, adjust) && \
> +	    !intel_compare_link_m_n(&current_config->alt_name, \
> +				    &pipe_config->name, adjust)) { \
> +		INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
> +			  "(expected tu %i gmch %i/%i link %i/%i, " \
> +			  "or tu %i gmch %i/%i link %i/%i, " \
> +			  "found tu %i, gmch %i/%i link %i/%i)\n", \
> +			  current_config->name.tu, \
> +			  current_config->name.gmch_m, \
> +			  current_config->name.gmch_n, \
> +			  current_config->name.link_m, \
> +			  current_config->name.link_n, \
> +			  current_config->alt_name.tu, \
> +			  current_config->alt_name.gmch_m, \
> +			  current_config->alt_name.gmch_n, \
> +			  current_config->alt_name.link_m, \
> +			  current_config->alt_name.link_n, \
> +			  pipe_config->name.tu, \
> +			  pipe_config->name.gmch_m, \
> +			  pipe_config->name.gmch_n, \
> +			  pipe_config->name.link_m, \
> +			  pipe_config->name.link_n); \
> +		ret = false; \
>  	}
>  
>  /* This is required for BDW+ where there is only one set of registers for
> @@ -12437,30 +12530,30 @@ intel_pipe_config_compare(struct drm_device *dev,
>  #define PIPE_CONF_CHECK_I_ALT(name, alt_name) \
>  	if ((current_config->name != pipe_config->name) && \
>  		(current_config->alt_name != pipe_config->name)) { \
> -			DRM_ERROR("mismatch in " #name " " \
> +			INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
>  				  "(expected %i or %i, found %i)\n", \
>  				  current_config->name, \
>  				  current_config->alt_name, \
>  				  pipe_config->name); \
> -			return false; \
> +			ret = false; \
>  	}
>  
>  #define PIPE_CONF_CHECK_FLAGS(name, mask)	\
>  	if ((current_config->name ^ pipe_config->name) & (mask)) { \
> -		DRM_ERROR("mismatch in " #name "(" #mask ") "	   \
> +		INTEL_ERR_OR_DBG_KMS("mismatch in " #name "(" #mask ") " \
>  			  "(expected %i, found %i)\n", \
>  			  current_config->name & (mask), \
>  			  pipe_config->name & (mask)); \
> -		return false; \
> +		ret = false; \
>  	}
>  
>  #define PIPE_CONF_CHECK_CLOCK_FUZZY(name) \
>  	if (!intel_fuzzy_clock_check(current_config->name, pipe_config->name)) { \
> -		DRM_ERROR("mismatch in " #name " " \
> +		INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
>  			  "(expected %i, found %i)\n", \
>  			  current_config->name, \
>  			  pipe_config->name); \
> -		return false; \
> +		ret = false; \
>  	}
>  
>  #define PIPE_CONF_QUIRK(quirk)	\
> @@ -12470,35 +12563,18 @@ intel_pipe_config_compare(struct drm_device *dev,
>  
>  	PIPE_CONF_CHECK_I(has_pch_encoder);
>  	PIPE_CONF_CHECK_I(fdi_lanes);
> -	PIPE_CONF_CHECK_I(fdi_m_n.gmch_m);
> -	PIPE_CONF_CHECK_I(fdi_m_n.gmch_n);
> -	PIPE_CONF_CHECK_I(fdi_m_n.link_m);
> -	PIPE_CONF_CHECK_I(fdi_m_n.link_n);
> -	PIPE_CONF_CHECK_I(fdi_m_n.tu);
> +	PIPE_CONF_CHECK_M_N(fdi_m_n);
>  
>  	PIPE_CONF_CHECK_I(has_dp_encoder);
>  
>  	if (INTEL_INFO(dev)->gen < 8) {
> -		PIPE_CONF_CHECK_I(dp_m_n.gmch_m);
> -		PIPE_CONF_CHECK_I(dp_m_n.gmch_n);
> -		PIPE_CONF_CHECK_I(dp_m_n.link_m);
> -		PIPE_CONF_CHECK_I(dp_m_n.link_n);
> -		PIPE_CONF_CHECK_I(dp_m_n.tu);
> -
> -		if (current_config->has_drrs) {
> -			PIPE_CONF_CHECK_I(dp_m2_n2.gmch_m);
> -			PIPE_CONF_CHECK_I(dp_m2_n2.gmch_n);
> -			PIPE_CONF_CHECK_I(dp_m2_n2.link_m);
> -			PIPE_CONF_CHECK_I(dp_m2_n2.link_n);
> -			PIPE_CONF_CHECK_I(dp_m2_n2.tu);
> -		}
> -	} else {
> -		PIPE_CONF_CHECK_I_ALT(dp_m_n.gmch_m, dp_m2_n2.gmch_m);
> -		PIPE_CONF_CHECK_I_ALT(dp_m_n.gmch_n, dp_m2_n2.gmch_n);
> -		PIPE_CONF_CHECK_I_ALT(dp_m_n.link_m, dp_m2_n2.link_m);
> -		PIPE_CONF_CHECK_I_ALT(dp_m_n.link_n, dp_m2_n2.link_n);
> -		PIPE_CONF_CHECK_I_ALT(dp_m_n.tu, dp_m2_n2.tu);
> -	}
> +		PIPE_CONF_CHECK_M_N(dp_m_n);
> +
> +		PIPE_CONF_CHECK_I(has_drrs);
> +		if (current_config->has_drrs)
> +			PIPE_CONF_CHECK_M_N(dp_m2_n2);
> +	} else
> +		PIPE_CONF_CHECK_M_N_ALT(dp_m_n, dp_m2_n2);
>  
>  	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hdisplay);
>  	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_htotal);
> @@ -12594,8 +12670,9 @@ intel_pipe_config_compare(struct drm_device *dev,
>  #undef PIPE_CONF_CHECK_FLAGS
>  #undef PIPE_CONF_CHECK_CLOCK_FUZZY
>  #undef PIPE_CONF_QUIRK
> +#undef INTEL_ERR_OR_DBG_KMS
>  
> -	return true;
> +	return ret;
>  }
>  
>  static void check_wm_state(struct drm_device *dev)
> @@ -12787,8 +12864,11 @@ check_crtc_state(struct drm_device *dev)
>  		     "transitional active state does not match atomic hw state "
>  		     "(expected %i, found %i)\n", crtc->base.state->active, crtc->active);
>  
> -		if (active &&
> -		    !intel_pipe_config_compare(dev, crtc->config, &pipe_config)) {
> +		if (!active)
> +			continue;
> +
> +		if (!intel_pipe_config_compare(dev, crtc->config,
> +					       &pipe_config, false)) {
>  			I915_STATE_WARN(1, "pipe state doesn't match!\n");
>  			intel_dump_pipe_config(crtc, &pipe_config,
>  					       "[hw state]");
> @@ -13089,14 +13169,17 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
>  		return ret;
>  
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		struct intel_crtc_state *pipe_config =
> +			to_intel_crtc_state(crtc_state);
> +		bool modeset, recalc;
> +
>  		if (!crtc_state->enable) {
>  			if (needs_modeset(crtc_state))
>  				any_ms = true;
>  			continue;
>  		}
>  
> -		if (to_intel_crtc_state(crtc_state)->quirks &
> -		    PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
> +		if (pipe_config->quirks & PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
>  			ret = drm_atomic_add_affected_planes(state, crtc);
>  			if (ret)
>  				return ret;
> @@ -13109,23 +13192,36 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
>  			 */
>  		}
>  
> -		if (!needs_modeset(crtc_state)) {
> +		modeset = needs_modeset(crtc_state);
> +		recalc = pipe_config->quirks & PIPE_CONFIG_QUIRK_INHERITED_MODE;
> +
> +		if (!modeset && !recalc)
> +			continue;
> +
> +		if (recalc) {
>  			ret = drm_atomic_add_affected_connectors(state, crtc);
>  			if (ret)
>  				return ret;
>  		}
>  
> -		ret = intel_modeset_pipe_config(crtc,
> -					to_intel_crtc_state(crtc_state));
> +		ret = intel_modeset_pipe_config(crtc, pipe_config);
>  		if (ret)
>  			return ret;
>  
> -		if (needs_modeset(crtc_state))
> -			any_ms = true;
> +		if (recalc && !intel_pipe_config_compare(state->dev,
> +					to_intel_crtc_state(crtc->state),
> +					pipe_config, true)) {
> +			modeset = crtc_state->mode_changed = true;
> +
> +			ret = drm_atomic_add_affected_planes(state, crtc);
> +			if (ret)
> +				return ret;
> +		}
>  
> +		any_ms = modeset;
>  		intel_dump_pipe_config(to_intel_crtc(crtc),
> -				       to_intel_crtc_state(crtc_state),
> -				       "[modeset]");
> +				       pipe_config,
> +				       modeset ? "[modeset]" : "[fastboot]");
>  	}
>  
>  	if (any_ms) {
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3.1 04/20] drm/i915: Allow fuzzy matching in pipe_config_compare, v2.

[Maarten Lankhorst]

Instead of doing ad-hoc checks we already have a way of checking
if the state is compatible or not. Use this to force a modeset.

Only during modesets, or with PIPE_CONFIG_QUIRK_INHERITED_MODE
we should check if a full modeset is really needed.

Fastboot will allow the adjust parameter to ignore some stuff
too, and it will fix up differences in state that are ignored
by the compare function.

Changes since v1:
- Increase the value of the lowest m/n to prevent truncation.
- Dump pipe config when fastboot's used, without a modeset.
- Add adjust parameter to intel_compare_link_m_n, which is
  used to adjust m2_n2 if it's a multiple of m_n.
- Add exact parameter intel_compare_m_n.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Daniel Stone <daniels@collabora.com>
---
Fixed the commit message.

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0eb1cc548656..109d1a5c6c87 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12292,19 +12292,6 @@ encoder_retry:
 	DRM_DEBUG_KMS("plane bpp: %i, pipe bpp: %i, dithering: %i\n",
 		      base_bpp, pipe_config->pipe_bpp, pipe_config->dither);
 
-	/* Check if we need to force a modeset */
-	if (pipe_config->has_audio !=
-	    to_intel_crtc_state(crtc->state)->has_audio) {
-		pipe_config->base.mode_changed = true;
-		ret = drm_atomic_add_affected_planes(state, crtc);
-	}
-
-	/*
-	 * Note we have an issue here with infoframes: current code
-	 * only updates them on the full mode set path per hw
-	 * requirements.  So here we should be checking for any
-	 * required changes and forcing a mode set.
-	 */
 fail:
 	return ret;
 }
@@ -12408,27 +12395,133 @@ static bool intel_fuzzy_clock_check(int clock1, int clock2)
 			    base.head) \
 		if (mask & (1 <<(intel_crtc)->pipe))
 
+
+static bool
+intel_compare_m_n(unsigned int m, unsigned int n,
+		  unsigned int m2, unsigned int n2,
+		  bool exact)
+{
+	if (m == m2 && n == n2)
+		return true;
+
+	if (exact || !m || !n || !m2 || !n2)
+		return false;
+
+	BUILD_BUG_ON(DATA_LINK_M_N_MASK > INT_MAX);
+
+	if (m > m2) {
+		while (m > m2) {
+			m2 <<= 1;
+			n2 <<= 1;
+		}
+	} else if (m < m2) {
+		while (m < m2) {
+			m <<= 1;
+			n <<= 1;
+		}
+	}
+
+	return m == m2 && n == n2;
+}
+
+static bool
+intel_compare_link_m_n(const struct intel_link_m_n *m_n,
+		       struct intel_link_m_n *m2_n2,
+		       bool adjust)
+{
+	if (m_n->tu == m2_n2->tu &&
+	    intel_compare_m_n(m_n->gmch_m, m_n->gmch_n,
+			      m2_n2->gmch_m, m2_n2->gmch_n, !adjust) &&
+	    intel_compare_m_n(m_n->link_m, m_n->link_n,
+			      m2_n2->link_m, m2_n2->link_n, !adjust)) {
+		if (adjust)
+			*m2_n2 = *m_n;
+
+		return true;
+	}
+
+	return false;
+}
+
 static bool
 intel_pipe_config_compare(struct drm_device *dev,
 			  struct intel_crtc_state *current_config,
-			  struct intel_crtc_state *pipe_config)
+			  struct intel_crtc_state *pipe_config,
+			  bool adjust)
 {
+	bool ret = true;
+
+#define INTEL_ERR_OR_DBG_KMS(fmt, ...) \
+	do { \
+		if (!adjust) \
+			DRM_ERROR(fmt, ##__VA_ARGS__); \
+		else \
+			DRM_DEBUG_KMS(fmt, ##__VA_ARGS__); \
+	} while (0)
+
 #define PIPE_CONF_CHECK_X(name)	\
 	if (current_config->name != pipe_config->name) { \
-		DRM_ERROR("mismatch in " #name " " \
+		INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
 			  "(expected 0x%08x, found 0x%08x)\n", \
 			  current_config->name, \
 			  pipe_config->name); \
-		return false; \
+		ret = false; \
 	}
 
 #define PIPE_CONF_CHECK_I(name)	\
 	if (current_config->name != pipe_config->name) { \
-		DRM_ERROR("mismatch in " #name " " \
+		INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
 			  "(expected %i, found %i)\n", \
 			  current_config->name, \
 			  pipe_config->name); \
-		return false; \
+		ret = false; \
+	}
+
+#define PIPE_CONF_CHECK_M_N(name) \
+	if (!intel_compare_link_m_n(&current_config->name, \
+				    &pipe_config->name,\
+				    adjust)) { \
+		INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
+			  "(expected tu %i gmch %i/%i link %i/%i, " \
+			  "found tu %i, gmch %i/%i link %i/%i)\n", \
+			  current_config->name.tu, \
+			  current_config->name.gmch_m, \
+			  current_config->name.gmch_n, \
+			  current_config->name.link_m, \
+			  current_config->name.link_n, \
+			  pipe_config->name.tu, \
+			  pipe_config->name.gmch_m, \
+			  pipe_config->name.gmch_n, \
+			  pipe_config->name.link_m, \
+			  pipe_config->name.link_n); \
+		ret = false; \
+	}
+
+#define PIPE_CONF_CHECK_M_N_ALT(name, alt_name) \
+	if (!intel_compare_link_m_n(&current_config->name, \
+				    &pipe_config->name, adjust) && \
+	    !intel_compare_link_m_n(&current_config->alt_name, \
+				    &pipe_config->name, adjust)) { \
+		INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
+			  "(expected tu %i gmch %i/%i link %i/%i, " \
+			  "or tu %i gmch %i/%i link %i/%i, " \
+			  "found tu %i, gmch %i/%i link %i/%i)\n", \
+			  current_config->name.tu, \
+			  current_config->name.gmch_m, \
+			  current_config->name.gmch_n, \
+			  current_config->name.link_m, \
+			  current_config->name.link_n, \
+			  current_config->alt_name.tu, \
+			  current_config->alt_name.gmch_m, \
+			  current_config->alt_name.gmch_n, \
+			  current_config->alt_name.link_m, \
+			  current_config->alt_name.link_n, \
+			  pipe_config->name.tu, \
+			  pipe_config->name.gmch_m, \
+			  pipe_config->name.gmch_n, \
+			  pipe_config->name.link_m, \
+			  pipe_config->name.link_n); \
+		ret = false; \
 	}
 
 /* This is required for BDW+ where there is only one set of registers for
@@ -12439,30 +12532,30 @@ intel_pipe_config_compare(struct drm_device *dev,
 #define PIPE_CONF_CHECK_I_ALT(name, alt_name) \
 	if ((current_config->name != pipe_config->name) && \
 		(current_config->alt_name != pipe_config->name)) { \
-			DRM_ERROR("mismatch in " #name " " \
+			INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
 				  "(expected %i or %i, found %i)\n", \
 				  current_config->name, \
 				  current_config->alt_name, \
 				  pipe_config->name); \
-			return false; \
+			ret = false; \
 	}
 
 #define PIPE_CONF_CHECK_FLAGS(name, mask)	\
 	if ((current_config->name ^ pipe_config->name) & (mask)) { \
-		DRM_ERROR("mismatch in " #name "(" #mask ") "	   \
+		INTEL_ERR_OR_DBG_KMS("mismatch in " #name "(" #mask ") " \
 			  "(expected %i, found %i)\n", \
 			  current_config->name & (mask), \
 			  pipe_config->name & (mask)); \
-		return false; \
+		ret = false; \
 	}
 
 #define PIPE_CONF_CHECK_CLOCK_FUZZY(name) \
 	if (!intel_fuzzy_clock_check(current_config->name, pipe_config->name)) { \
-		DRM_ERROR("mismatch in " #name " " \
+		INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
 			  "(expected %i, found %i)\n", \
 			  current_config->name, \
 			  pipe_config->name); \
-		return false; \
+		ret = false; \
 	}
 
 #define PIPE_CONF_QUIRK(quirk)	\
@@ -12472,35 +12565,18 @@ intel_pipe_config_compare(struct drm_device *dev,
 
 	PIPE_CONF_CHECK_I(has_pch_encoder);
 	PIPE_CONF_CHECK_I(fdi_lanes);
-	PIPE_CONF_CHECK_I(fdi_m_n.gmch_m);
-	PIPE_CONF_CHECK_I(fdi_m_n.gmch_n);
-	PIPE_CONF_CHECK_I(fdi_m_n.link_m);
-	PIPE_CONF_CHECK_I(fdi_m_n.link_n);
-	PIPE_CONF_CHECK_I(fdi_m_n.tu);
+	PIPE_CONF_CHECK_M_N(fdi_m_n);
 
 	PIPE_CONF_CHECK_I(has_dp_encoder);
 
 	if (INTEL_INFO(dev)->gen < 8) {
-		PIPE_CONF_CHECK_I(dp_m_n.gmch_m);
-		PIPE_CONF_CHECK_I(dp_m_n.gmch_n);
-		PIPE_CONF_CHECK_I(dp_m_n.link_m);
-		PIPE_CONF_CHECK_I(dp_m_n.link_n);
-		PIPE_CONF_CHECK_I(dp_m_n.tu);
-
-		if (current_config->has_drrs) {
-			PIPE_CONF_CHECK_I(dp_m2_n2.gmch_m);
-			PIPE_CONF_CHECK_I(dp_m2_n2.gmch_n);
-			PIPE_CONF_CHECK_I(dp_m2_n2.link_m);
-			PIPE_CONF_CHECK_I(dp_m2_n2.link_n);
-			PIPE_CONF_CHECK_I(dp_m2_n2.tu);
-		}
-	} else {
-		PIPE_CONF_CHECK_I_ALT(dp_m_n.gmch_m, dp_m2_n2.gmch_m);
-		PIPE_CONF_CHECK_I_ALT(dp_m_n.gmch_n, dp_m2_n2.gmch_n);
-		PIPE_CONF_CHECK_I_ALT(dp_m_n.link_m, dp_m2_n2.link_m);
-		PIPE_CONF_CHECK_I_ALT(dp_m_n.link_n, dp_m2_n2.link_n);
-		PIPE_CONF_CHECK_I_ALT(dp_m_n.tu, dp_m2_n2.tu);
-	}
+		PIPE_CONF_CHECK_M_N(dp_m_n);
+
+		PIPE_CONF_CHECK_I(has_drrs);
+		if (current_config->has_drrs)
+			PIPE_CONF_CHECK_M_N(dp_m2_n2);
+	} else
+		PIPE_CONF_CHECK_M_N_ALT(dp_m_n, dp_m2_n2);
 
 	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hdisplay);
 	PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_htotal);
@@ -12596,8 +12672,9 @@ intel_pipe_config_compare(struct drm_device *dev,
 #undef PIPE_CONF_CHECK_FLAGS
 #undef PIPE_CONF_CHECK_CLOCK_FUZZY
 #undef PIPE_CONF_QUIRK
+#undef INTEL_ERR_OR_DBG_KMS
 
-	return true;
+	return ret;
 }
 
 static void check_wm_state(struct drm_device *dev)
@@ -12789,8 +12866,11 @@ check_crtc_state(struct drm_device *dev)
 		     "transitional active state does not match atomic hw state "
 		     "(expected %i, found %i)\n", crtc->base.state->active, crtc->active);
 
-		if (active &&
-		    !intel_pipe_config_compare(dev, crtc->config, &pipe_config)) {
+		if (!active)
+			continue;
+
+		if (!intel_pipe_config_compare(dev, crtc->config,
+					       &pipe_config, false)) {
 			I915_STATE_WARN(1, "pipe state doesn't match!\n");
 			intel_dump_pipe_config(crtc, &pipe_config,
 					       "[hw state]");
@@ -13091,14 +13171,17 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
 		return ret;
 
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		struct intel_crtc_state *pipe_config =
+			to_intel_crtc_state(crtc_state);
+		bool modeset, recalc;
+
 		if (!crtc_state->enable) {
 			if (needs_modeset(crtc_state))
 				any_ms = true;
 			continue;
 		}
 
-		if (to_intel_crtc_state(crtc_state)->quirks &
-		    PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
+		if (pipe_config->quirks & PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
 			ret = drm_atomic_add_affected_planes(state, crtc);
 			if (ret)
 				return ret;
@@ -13111,23 +13194,36 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
 			 */
 		}
 
-		if (!needs_modeset(crtc_state)) {
+		modeset = needs_modeset(crtc_state);
+		recalc = pipe_config->quirks & PIPE_CONFIG_QUIRK_INHERITED_MODE;
+
+		if (!modeset && !recalc)
+			continue;
+
+		if (recalc) {
 			ret = drm_atomic_add_affected_connectors(state, crtc);
 			if (ret)
 				return ret;
 		}
 
-		ret = intel_modeset_pipe_config(crtc,
-					to_intel_crtc_state(crtc_state));
+		ret = intel_modeset_pipe_config(crtc, pipe_config);
 		if (ret)
 			return ret;
 
-		if (needs_modeset(crtc_state))
-			any_ms = true;
+		if (recalc && !intel_pipe_config_compare(state->dev,
+					to_intel_crtc_state(crtc->state),
+					pipe_config, true)) {
+			modeset = crtc_state->mode_changed = true;
+
+			ret = drm_atomic_add_affected_planes(state, crtc);
+			if (ret)
+				return ret;
+		}
 
+		any_ms = modeset;
 		intel_dump_pipe_config(to_intel_crtc(crtc),
-				       to_intel_crtc_state(crtc_state),
-				       "[modeset]");
+				       pipe_config,
+				       modeset ? "[modeset]" : "[fastboot]");
 	}
 
 	if (any_ms) {

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

[PATCH v3 05/20] drm/i915: Update missing properties in find_initial_plane_obj

[Maarten Lankhorst]

The src and crtc rectangles were never set, resulting in the primary
plane being made invisible on first atomic update.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c3a3d1087777..037a85f1b127 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2586,6 +2586,7 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
 	struct intel_crtc *i;
 	struct drm_i915_gem_object *obj;
 	struct drm_plane *primary = intel_crtc->base.primary;
+	struct drm_plane_state *plane_state = primary->state;
 	struct drm_framebuffer *fb;
 
 	if (!plane_config->fb)
@@ -2625,13 +2626,21 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
 	return;
 
 valid_fb:
+	plane_state->src_x = plane_state->src_y = 0;
+	plane_state->src_w = fb->width << 16;
+	plane_state->src_h = fb->height << 16;
+
+	plane_state->crtc_x = plane_state->src_y = 0;
+	plane_state->crtc_w = fb->width;
+	plane_state->crtc_h = fb->height;
+
 	obj = intel_fb_obj(fb);
 	if (obj->tiling_mode != I915_TILING_NONE)
 		dev_priv->preserve_bios_swizzle = true;
 
-	primary->fb = fb;
+	drm_framebuffer_reference(fb);
+	primary->fb = primary->state->fb = fb;
 	primary->crtc = primary->state->crtc = &intel_crtc->base;
-	update_state_fb(primary);
 	intel_crtc->base.state->plane_mask |= (1 << drm_plane_index(primary));
 	obj->frontbuffer_bits |= to_intel_plane(primary)->frontbuffer_bit;
 }
-- 
2.1.0

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

[PATCH v3 06/20] drm/i915: Remove plane_config from struct intel_crtc.

[Maarten Lankhorst]

Nothing depends on this outside initial hw readout, so keep this
struct on the stack instead.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 25 +++++++++++++++----------
 drivers/gpu/drm/i915/intel_drv.h     |  1 -
 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 037a85f1b127..e4d8acc63823 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15204,7 +15204,9 @@ void intel_modeset_init(struct drm_device *dev)
 	drm_modeset_unlock_all(dev);
 
 	for_each_intel_crtc(dev, crtc) {
-		if (!crtc->active)
+		struct intel_initial_plane_config plane_config;
+
+		if (!crtc->base.state->active)
 			continue;
 
 		/*
@@ -15214,15 +15216,16 @@ void intel_modeset_init(struct drm_device *dev)
 		 * can even allow for smooth boot transitions if the BIOS
 		 * fb is large enough for the active pipe configuration.
 		 */
-		if (dev_priv->display.get_initial_plane_config) {
-			dev_priv->display.get_initial_plane_config(crtc,
-							   &crtc->plane_config);
-			/*
-			 * If the fb is shared between multiple heads, we'll
-			 * just get the first one.
-			 */
-			intel_find_initial_plane_obj(crtc, &crtc->plane_config);
-		}
+
+		plane_config.fb = NULL;
+		dev_priv->display.get_initial_plane_config(crtc,
+							   &plane_config);
+
+		/*
+		 * If the fb is shared between multiple heads, we'll
+		 * just get the first one.
+		 */
+		intel_find_initial_plane_obj(crtc, &plane_config);
 	}
 }
 
@@ -15713,6 +15716,8 @@ void intel_modeset_gem_init(struct drm_device *dev)
 		if (ret) {
 			DRM_ERROR("failed to pin boot fb on pipe %d\n",
 				  to_intel_crtc(c)->pipe);
+			obj->frontbuffer_bits &=
+				~to_intel_plane(c->primary)->frontbuffer_bit;
 			drm_framebuffer_unreference(c->primary->fb);
 			c->primary->fb = NULL;
 			c->primary->crtc = c->primary->state->crtc = NULL;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 09a0a9222a3a..09e3581c8441 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -550,7 +550,6 @@ struct intel_crtc {
 	uint32_t cursor_size;
 	uint32_t cursor_base;
 
-	struct intel_initial_plane_config plane_config;
 	struct intel_crtc_state *config;
 	bool new_enabled;
 
-- 
2.1.0

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

Re: [PATCH v3 06/20] drm/i915: Remove plane_config from struct intel_crtc.

[Daniel Stone]

Hi,

On 13 July 2015 at 15:30, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 037a85f1b127..e4d8acc63823 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15204,7 +15204,9 @@ void intel_modeset_init(struct drm_device *dev)
>         drm_modeset_unlock_all(dev);
>
>         for_each_intel_crtc(dev, crtc) {
> -               if (!crtc->active)
> +               struct intel_initial_plane_config plane_config;
> +
> +               if (!crtc->base.state->active)

Unrelated change from crtc->active to crtc->base.state->active - can
we do this in one of the later patches?

> +               plane_config.fb = NULL;

memset to 0 instead?

> @@ -15713,6 +15716,8 @@ void intel_modeset_gem_init(struct drm_device *dev)
>                 if (ret) {
>                         DRM_ERROR("failed to pin boot fb on pipe %d\n",
>                                   to_intel_crtc(c)->pipe);
> +                       obj->frontbuffer_bits &=
> +                               ~to_intel_plane(c->primary)->frontbuffer_bit;
>                         drm_framebuffer_unreference(c->primary->fb);
>                         c->primary->fb = NULL;
>                         c->primary->crtc = c->primary->state->crtc = NULL;

Unrelated change?

Otherwise:
Reviewed-by: Daniel Stone <daniels@collabora.com>

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

Re: [PATCH v3 06/20] drm/i915: Remove plane_config from struct intel_crtc.

[Maarten Lankhorst]

Op 13-07-15 om 18:30 schreef Daniel Stone:
> Hi,
>
> On 13 July 2015 at 15:30, Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com> wrote:
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 037a85f1b127..e4d8acc63823 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -15204,7 +15204,9 @@ void intel_modeset_init(struct drm_device *dev)
>>         drm_modeset_unlock_all(dev);
>>
>>         for_each_intel_crtc(dev, crtc) {
>> -               if (!crtc->active)
>> +               struct intel_initial_plane_config plane_config;
>> +
>> +               if (!crtc->base.state->active)
> Unrelated change from crtc->active to crtc->base.state->active - can
> we do this in one of the later patches?
Probably, but I'm trying to get rid of crtc->active every time I touch a function.

Eventually this will mean being able to remove it. ;-)

>> +               plane_config.fb = NULL;
> memset to 0 instead?
Well for intel_find_initial_plane_obj it's sufficient, but I can just initialize with plane_config = {}; to keep old behavior.
>> @@ -15713,6 +15716,8 @@ void intel_modeset_gem_init(struct drm_device *dev)
>>                 if (ret) {
>>                         DRM_ERROR("failed to pin boot fb on pipe %d\n",
>>                                   to_intel_crtc(c)->pipe);
>> +                       obj->frontbuffer_bits &=
>> +                               ~to_intel_plane(c->primary)->frontbuffer_bit;
>>                         drm_framebuffer_unreference(c->primary->fb);
>>                         c->primary->fb = NULL;
>>                         c->primary->crtc = c->primary->state->crtc = NULL;
> Unrelated change?
Unrelated perhaps, but it fixes a warn when pinning fails.
Still I guess a WARN won't hurt in that case, I'll drop it.

> Otherwise:
> Reviewed-by: Daniel Stone <daniels@collabora.com>
>
> Cheers,
> Daniel

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

Re: [PATCH v3 06/20] drm/i915: Remove plane_config from struct intel_crtc.

[Daniel Stone]

Hey,

On 14 July 2015 at 09:27, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
> Op 13-07-15 om 18:30 schreef Daniel Stone:
>> On 13 July 2015 at 15:30, Maarten Lankhorst
>> <maarten.lankhorst@linux.intel.com> wrote:
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> index 037a85f1b127..e4d8acc63823 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -15204,7 +15204,9 @@ void intel_modeset_init(struct drm_device *dev)
>>>         drm_modeset_unlock_all(dev);
>>>
>>>         for_each_intel_crtc(dev, crtc) {
>>> -               if (!crtc->active)
>>> +               struct intel_initial_plane_config plane_config;
>>> +
>>> +               if (!crtc->base.state->active)
>> Unrelated change from crtc->active to crtc->base.state->active - can
>> we do this in one of the later patches?
> Probably, but I'm trying to get rid of crtc->active every time I touch a function.

Sure, but it does make bisection a bit more difficult.

> Eventually this will mean being able to remove it. ;-)

Hey, you know I'm in favour of this!

>>> +               plane_config.fb = NULL;
>> memset to 0 instead?
> Well for intel_find_initial_plane_obj it's sufficient, but I can just initialize with plane_config = {}; to keep old behavior.
>>> @@ -15713,6 +15716,8 @@ void intel_modeset_gem_init(struct drm_device *dev)
>>>                 if (ret) {
>>>                         DRM_ERROR("failed to pin boot fb on pipe %d\n",
>>>                                   to_intel_crtc(c)->pipe);
>>> +                       obj->frontbuffer_bits &=
>>> +                               ~to_intel_plane(c->primary)->frontbuffer_bit;
>>>                         drm_framebuffer_unreference(c->primary->fb);
>>>                         c->primary->fb = NULL;
>>>                         c->primary->crtc = c->primary->state->crtc = NULL;
>> Unrelated change?
> Unrelated perhaps, but it fixes a warn when pinning fails.
> Still I guess a WARN won't hurt in that case, I'll drop it.

Yeah, it does make sense to me, but then again it wouldn't be the
first time that a frontbuffer-tracking change I've thought made sense,
actually broke things. ;)

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

[PATCH v3.1 06/20] drm/i915: Remove plane_config from struct intel_crtc, v2.

[Maarten Lankhorst]

commit 4efb477b56ca9ac6ab76136a2801aaee4d3f46c5
Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Date:   Mon Jul 13 15:29:47 2015 +0200

    drm/i915: Remove plane_config from struct intel_crtc, v2.
    
    Nothing depends on this outside initial hw readout, so keep this
    struct on the stack instead.
    
    Changes since v1:
    - Remove unrelated changes.
    
    Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
    Reviewed-by: Daniel Stone <daniels@collabora.com>

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6432c180725f..0ca2b1df7e03 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15206,6 +15206,8 @@ void intel_modeset_init(struct drm_device *dev)
 	drm_modeset_unlock_all(dev);
 
 	for_each_intel_crtc(dev, crtc) {
+		struct intel_initial_plane_config plane_config = {};
+
 		if (!crtc->active)
 			continue;
 
@@ -15216,15 +15218,14 @@ void intel_modeset_init(struct drm_device *dev)
 		 * can even allow for smooth boot transitions if the BIOS
 		 * fb is large enough for the active pipe configuration.
 		 */
-		if (dev_priv->display.get_initial_plane_config) {
-			dev_priv->display.get_initial_plane_config(crtc,
-							   &crtc->plane_config);
-			/*
-			 * If the fb is shared between multiple heads, we'll
-			 * just get the first one.
-			 */
-			intel_find_initial_plane_obj(crtc, &crtc->plane_config);
-		}
+		dev_priv->display.get_initial_plane_config(crtc,
+							   &plane_config);
+
+		/*
+		 * If the fb is shared between multiple heads, we'll
+		 * just get the first one.
+		 */
+		intel_find_initial_plane_obj(crtc, &plane_config);
 	}
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 09a0a9222a3a..09e3581c8441 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -550,7 +550,6 @@ struct intel_crtc {
 	uint32_t cursor_size;
 	uint32_t cursor_base;
 
-	struct intel_initial_plane_config plane_config;
 	struct intel_crtc_state *config;
 	bool new_enabled;
 

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

Re: [PATCH v3.1 06/20] drm/i915: Remove plane_config from struct intel_crtc, v2.

[Daniel Vetter]

On Tue, Jul 14, 2015 at 12:33:29PM +0200, Maarten Lankhorst wrote:
> commit 4efb477b56ca9ac6ab76136a2801aaee4d3f46c5
> Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Date:   Mon Jul 13 15:29:47 2015 +0200
> 
>     drm/i915: Remove plane_config from struct intel_crtc, v2.
>     
>     Nothing depends on this outside initial hw readout, so keep this
>     struct on the stack instead.
>     
>     Changes since v1:
>     - Remove unrelated changes.
>     
>     Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>     Reviewed-by: Daniel Stone <daniels@collabora.com>

Merged up to this one (with the strange indent removed).

Thanks, Daniel

> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 6432c180725f..0ca2b1df7e03 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15206,6 +15206,8 @@ void intel_modeset_init(struct drm_device *dev)
>  	drm_modeset_unlock_all(dev);
>  
>  	for_each_intel_crtc(dev, crtc) {
> +		struct intel_initial_plane_config plane_config = {};
> +
>  		if (!crtc->active)
>  			continue;
>  
> @@ -15216,15 +15218,14 @@ void intel_modeset_init(struct drm_device *dev)
>  		 * can even allow for smooth boot transitions if the BIOS
>  		 * fb is large enough for the active pipe configuration.
>  		 */
> -		if (dev_priv->display.get_initial_plane_config) {
> -			dev_priv->display.get_initial_plane_config(crtc,
> -							   &crtc->plane_config);
> -			/*
> -			 * If the fb is shared between multiple heads, we'll
> -			 * just get the first one.
> -			 */
> -			intel_find_initial_plane_obj(crtc, &crtc->plane_config);
> -		}
> +		dev_priv->display.get_initial_plane_config(crtc,
> +							   &plane_config);
> +
> +		/*
> +		 * If the fb is shared between multiple heads, we'll
> +		 * just get the first one.
> +		 */
> +		intel_find_initial_plane_obj(crtc, &plane_config);
>  	}
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 09a0a9222a3a..09e3581c8441 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -550,7 +550,6 @@ struct intel_crtc {
>  	uint32_t cursor_size;
>  	uint32_t cursor_base;
>  
> -	struct intel_initial_plane_config plane_config;
>  	struct intel_crtc_state *config;
>  	bool new_enabled;
>  
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 07/20] drm/i915: Rework plane readout.

[Maarten Lankhorst]

All non-primary planes get disabled during hw readout,
this reduces complexity and means not having to do some plane
visibility checks during the first commit.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_atomic.c  |  7 ---
 drivers/gpu/drm/i915/intel_display.c | 86 ++++--------------------------------
 drivers/gpu/drm/i915/intel_drv.h     |  1 -
 3 files changed, 8 insertions(+), 86 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index b92b8581efc2..dcf4fb458649 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -98,13 +98,6 @@ int intel_atomic_check(struct drm_device *dev,
 		return -EINVAL;
 	}
 
-	if (crtc_state &&
-	    crtc_state->quirks & PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
-		ret = drm_atomic_add_affected_planes(state, &nuclear_crtc->base);
-		if (ret)
-			return ret;
-	}
-
 	ret = drm_atomic_helper_check_planes(dev, state);
 	if (ret)
 		return ret;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e4d8acc63823..118187dc76be 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11783,34 +11783,6 @@ static bool check_encoder_cloning(struct drm_atomic_state *state,
 	return true;
 }
 
-static void intel_crtc_check_initial_planes(struct drm_crtc *crtc,
-					    struct drm_crtc_state *crtc_state)
-{
-	struct intel_crtc_state *pipe_config =
-		to_intel_crtc_state(crtc_state);
-	struct drm_plane *p;
-	unsigned visible_mask = 0;
-
-	drm_for_each_plane_mask(p, crtc->dev, crtc_state->plane_mask) {
-		struct drm_plane_state *plane_state =
-			drm_atomic_get_existing_plane_state(crtc_state->state, p);
-
-		if (WARN_ON(!plane_state))
-			continue;
-
-		if (!plane_state->fb)
-			crtc_state->plane_mask &=
-				~(1 << drm_plane_index(p));
-		else if (to_intel_plane_state(plane_state)->visible)
-			visible_mask |= 1 << drm_plane_index(p);
-	}
-
-	if (!visible_mask)
-		return;
-
-	pipe_config->quirks &= ~PIPE_CONFIG_QUIRK_INITIAL_PLANES;
-}
-
 static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 				   struct drm_crtc_state *crtc_state)
 {
@@ -11832,10 +11804,6 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 		"[CRTC:%i] mismatch between state->active(%i) and crtc->active(%i)\n",
 		idx, crtc->state->active, intel_crtc->active);
 
-	/* plane mask is fixed up after all initial planes are calculated */
-	if (pipe_config->quirks & PIPE_CONFIG_QUIRK_INITIAL_PLANES)
-		intel_crtc_check_initial_planes(crtc, crtc_state);
-
 	if (mode_changed && !crtc_state->active)
 		intel_crtc->atomic.update_wm_post = true;
 
@@ -13188,19 +13156,6 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
 			continue;
 		}
 
-		if (pipe_config->quirks & PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
-			ret = drm_atomic_add_affected_planes(state, crtc);
-			if (ret)
-				return ret;
-
-			/*
-			 * We ought to handle i915.fastboot here.
-			 * If no modeset is required and the primary plane has
-			 * a fb, update the members of crtc_state as needed,
-			 * and run the necessary updates during vblank evasion.
-			 */
-		}
-
 		modeset = needs_modeset(crtc_state);
 		recalc = pipe_config->quirks & PIPE_CONFIG_QUIRK_INHERITED_MODE;
 
@@ -15460,47 +15415,22 @@ static void readout_plane_state(struct intel_crtc *crtc,
 				struct intel_crtc_state *crtc_state)
 {
 	struct intel_plane *p;
-	struct drm_plane_state *drm_plane_state;
+	struct intel_plane_state *plane_state;
 	bool active = crtc_state->base.active;
 
-	if (active) {
-		crtc_state->quirks |= PIPE_CONFIG_QUIRK_INITIAL_PLANES;
-
-		/* apply to previous sw state too */
-		to_intel_crtc_state(crtc->base.state)->quirks |=
-			PIPE_CONFIG_QUIRK_INITIAL_PLANES;
-	}
-
 	for_each_intel_plane(crtc->base.dev, p) {
-		bool visible = active;
-
 		if (crtc->pipe != p->pipe)
 			continue;
 
-		drm_plane_state = p->base.state;
-
-		/* Plane scaler state is not touched here. The first atomic
-		 * commit will restore all plane scalers to its old state.
-		 */
+		plane_state = to_intel_plane_state(p->base.state);
 
-		if (active && p->base.type == DRM_PLANE_TYPE_PRIMARY) {
-			visible = primary_get_hw_state(crtc);
-			to_intel_plane_state(drm_plane_state)->visible = visible;
-		} else {
-			/*
-			 * unknown state, assume it's off to force a transition
-			 * to on when calculating state changes.
-			 */
-			to_intel_plane_state(drm_plane_state)->visible = false;
-		}
+		if (p->base.type == DRM_PLANE_TYPE_PRIMARY)
+			plane_state->visible = primary_get_hw_state(crtc);
+		else {
+			if (active)
+				p->disable_plane(&p->base, &crtc->base);
 
-		if (visible) {
-			crtc_state->base.plane_mask |=
-				1 << drm_plane_index(&p->base);
-		} else if (crtc_state->base.state) {
-			/* Make this unconditional for atomic hw readout. */
-			crtc_state->base.plane_mask &=
-				~(1 << drm_plane_index(&p->base));
+			plane_state->visible = false;
 		}
 	}
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 09e3581c8441..2c23900b491f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -341,7 +341,6 @@ struct intel_crtc_state {
 	 */
 #define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS	(1<<0) /* unreliable sync mode.flags */
 #define PIPE_CONFIG_QUIRK_INHERITED_MODE	(1<<1) /* mode inherited from firmware */
-#define PIPE_CONFIG_QUIRK_INITIAL_PLANES	(1<<2) /* planes are in unknown state */
 	unsigned long quirks;
 
 	/* Pipe source size (ie. panel fitter input size)
-- 
2.1.0

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

Re: [PATCH v3 07/20] drm/i915: Rework plane readout.

[Daniel Vetter]

On Mon, Jul 13, 2015 at 04:30:20PM +0200, Maarten Lankhorst wrote:
> All non-primary planes get disabled during hw readout,
> this reduces complexity and means not having to do some plane
> visibility checks during the first commit.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

I still think calling readout_plane_state from the hw state readout code
is the wrong approach. Instead we should consolidate all the plane
readout code exclusively into a new intel_plane_readout_hw_state helper
which is called only from driver load paths. That should also contain the
fb/gem_bo reconstruction loop.

For the other 2 users of this code (lid notifiery and resume) we should
just force an unconditional plane restore by setting
crtc_state->planes_chagned. But I thinkt hat's ok as some follow-up work,
once atomic has settled a bit more.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_atomic.c  |  7 ---
>  drivers/gpu/drm/i915/intel_display.c | 86 ++++--------------------------------
>  drivers/gpu/drm/i915/intel_drv.h     |  1 -
>  3 files changed, 8 insertions(+), 86 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index b92b8581efc2..dcf4fb458649 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -98,13 +98,6 @@ int intel_atomic_check(struct drm_device *dev,
>  		return -EINVAL;
>  	}
>  
> -	if (crtc_state &&
> -	    crtc_state->quirks & PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
> -		ret = drm_atomic_add_affected_planes(state, &nuclear_crtc->base);
> -		if (ret)
> -			return ret;
> -	}
> -
>  	ret = drm_atomic_helper_check_planes(dev, state);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e4d8acc63823..118187dc76be 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11783,34 +11783,6 @@ static bool check_encoder_cloning(struct drm_atomic_state *state,
>  	return true;
>  }
>  
> -static void intel_crtc_check_initial_planes(struct drm_crtc *crtc,
> -					    struct drm_crtc_state *crtc_state)
> -{
> -	struct intel_crtc_state *pipe_config =
> -		to_intel_crtc_state(crtc_state);
> -	struct drm_plane *p;
> -	unsigned visible_mask = 0;
> -
> -	drm_for_each_plane_mask(p, crtc->dev, crtc_state->plane_mask) {
> -		struct drm_plane_state *plane_state =
> -			drm_atomic_get_existing_plane_state(crtc_state->state, p);
> -
> -		if (WARN_ON(!plane_state))
> -			continue;
> -
> -		if (!plane_state->fb)
> -			crtc_state->plane_mask &=
> -				~(1 << drm_plane_index(p));
> -		else if (to_intel_plane_state(plane_state)->visible)
> -			visible_mask |= 1 << drm_plane_index(p);
> -	}
> -
> -	if (!visible_mask)
> -		return;
> -
> -	pipe_config->quirks &= ~PIPE_CONFIG_QUIRK_INITIAL_PLANES;
> -}
> -
>  static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>  				   struct drm_crtc_state *crtc_state)
>  {
> @@ -11832,10 +11804,6 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>  		"[CRTC:%i] mismatch between state->active(%i) and crtc->active(%i)\n",
>  		idx, crtc->state->active, intel_crtc->active);
>  
> -	/* plane mask is fixed up after all initial planes are calculated */
> -	if (pipe_config->quirks & PIPE_CONFIG_QUIRK_INITIAL_PLANES)
> -		intel_crtc_check_initial_planes(crtc, crtc_state);
> -
>  	if (mode_changed && !crtc_state->active)
>  		intel_crtc->atomic.update_wm_post = true;
>  
> @@ -13188,19 +13156,6 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
>  			continue;
>  		}
>  
> -		if (pipe_config->quirks & PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
> -			ret = drm_atomic_add_affected_planes(state, crtc);
> -			if (ret)
> -				return ret;
> -
> -			/*
> -			 * We ought to handle i915.fastboot here.
> -			 * If no modeset is required and the primary plane has
> -			 * a fb, update the members of crtc_state as needed,
> -			 * and run the necessary updates during vblank evasion.
> -			 */
> -		}
> -
>  		modeset = needs_modeset(crtc_state);
>  		recalc = pipe_config->quirks & PIPE_CONFIG_QUIRK_INHERITED_MODE;
>  
> @@ -15460,47 +15415,22 @@ static void readout_plane_state(struct intel_crtc *crtc,
>  				struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_plane *p;
> -	struct drm_plane_state *drm_plane_state;
> +	struct intel_plane_state *plane_state;
>  	bool active = crtc_state->base.active;
>  
> -	if (active) {
> -		crtc_state->quirks |= PIPE_CONFIG_QUIRK_INITIAL_PLANES;
> -
> -		/* apply to previous sw state too */
> -		to_intel_crtc_state(crtc->base.state)->quirks |=
> -			PIPE_CONFIG_QUIRK_INITIAL_PLANES;
> -	}
> -
>  	for_each_intel_plane(crtc->base.dev, p) {
> -		bool visible = active;
> -
>  		if (crtc->pipe != p->pipe)
>  			continue;
>  
> -		drm_plane_state = p->base.state;
> -
> -		/* Plane scaler state is not touched here. The first atomic
> -		 * commit will restore all plane scalers to its old state.
> -		 */
> +		plane_state = to_intel_plane_state(p->base.state);
>  
> -		if (active && p->base.type == DRM_PLANE_TYPE_PRIMARY) {
> -			visible = primary_get_hw_state(crtc);
> -			to_intel_plane_state(drm_plane_state)->visible = visible;
> -		} else {
> -			/*
> -			 * unknown state, assume it's off to force a transition
> -			 * to on when calculating state changes.
> -			 */
> -			to_intel_plane_state(drm_plane_state)->visible = false;
> -		}
> +		if (p->base.type == DRM_PLANE_TYPE_PRIMARY)
> +			plane_state->visible = primary_get_hw_state(crtc);
> +		else {
> +			if (active)
> +				p->disable_plane(&p->base, &crtc->base);
>  
> -		if (visible) {
> -			crtc_state->base.plane_mask |=
> -				1 << drm_plane_index(&p->base);
> -		} else if (crtc_state->base.state) {
> -			/* Make this unconditional for atomic hw readout. */
> -			crtc_state->base.plane_mask &=
> -				~(1 << drm_plane_index(&p->base));
> +			plane_state->visible = false;
>  		}
>  	}
>  }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 09e3581c8441..2c23900b491f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -341,7 +341,6 @@ struct intel_crtc_state {
>  	 */
>  #define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS	(1<<0) /* unreliable sync mode.flags */
>  #define PIPE_CONFIG_QUIRK_INHERITED_MODE	(1<<1) /* mode inherited from firmware */
> -#define PIPE_CONFIG_QUIRK_INITIAL_PLANES	(1<<2) /* planes are in unknown state */
>  	unsigned long quirks;
>  
>  	/* Pipe source size (ie. panel fitter input size)
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 07/20] drm/i915: Rework plane readout.

[Maarten Lankhorst]

Op 14-07-15 om 11:53 schreef Daniel Vetter:
> On Mon, Jul 13, 2015 at 04:30:20PM +0200, Maarten Lankhorst wrote:
>> All non-primary planes get disabled during hw readout,
>> this reduces complexity and means not having to do some plane
>> visibility checks during the first commit.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> I still think calling readout_plane_state from the hw state readout code
> is the wrong approach. Instead we should consolidate all the plane
> readout code exclusively into a new intel_plane_readout_hw_state helper
> which is called only from driver load paths. That should also contain the
> fb/gem_bo reconstruction loop.
Is that a nack?

> For the other 2 users of this code (lid notifiery and resume) we should
> just force an unconditional plane restore by setting
> crtc_state->planes_chagned. But I thinkt hat's ok as some follow-up work,
> once atomic has settled a bit more.
>
If you want to force a plane restore, planes_changed won't do what you think it does.
planes_changed is a flag that says one or more planes were added to the crtc_state.

You want to do drm_atomic_add_affected_planes for that, and atomic resume would do that
since it duplicates all plane state.

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

Re: [PATCH v3 07/20] drm/i915: Rework plane readout.

[Daniel Vetter]

On Tue, Jul 14, 2015 at 12:30:38PM +0200, Maarten Lankhorst wrote:
> Op 14-07-15 om 11:53 schreef Daniel Vetter:
> > On Mon, Jul 13, 2015 at 04:30:20PM +0200, Maarten Lankhorst wrote:
> >> All non-primary planes get disabled during hw readout,
> >> this reduces complexity and means not having to do some plane
> >> visibility checks during the first commit.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > I still think calling readout_plane_state from the hw state readout code
> > is the wrong approach. Instead we should consolidate all the plane
> > readout code exclusively into a new intel_plane_readout_hw_state helper
> > which is called only from driver load paths. That should also contain the
> > fb/gem_bo reconstruction loop.
> Is that a nack?

Nope, just a "I think we want to clarify this more". I'd welcome a patch
on top, but can be done as part of converting dpms (since that will result
in lots of cleanups too).

> > For the other 2 users of this code (lid notifiery and resume) we should
> > just force an unconditional plane restore by setting
> > crtc_state->planes_chagned. But I thinkt hat's ok as some follow-up work,
> > once atomic has settled a bit more.
> >
> If you want to force a plane restore, planes_changed won't do what you think it does.
> planes_changed is a flag that says one or more planes were added to the crtc_state.
> 
> You want to do drm_atomic_add_affected_planes for that, and atomic resume would do that
> since it duplicates all plane state.

Ah right, so should be possible to just consolidate the plane readout code
without requiring any changes in the force_restore case.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 08/20] drm/i915: fill in more mode members

[Maarten Lankhorst]

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 118187dc76be..d37f6a93b094 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7754,9 +7754,14 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode,
 	mode->vsync_end = pipe_config->base.adjusted_mode.crtc_vsync_end;
 
 	mode->flags = pipe_config->base.adjusted_mode.flags;
+	mode->type = DRM_MODE_TYPE_DRIVER;
 
 	mode->clock = pipe_config->base.adjusted_mode.crtc_clock;
 	mode->flags |= pipe_config->base.adjusted_mode.flags;
+
+	mode->hsync = drm_mode_hsync(mode);
+	mode->vrefresh = drm_mode_vrefresh(mode);
+	drm_mode_set_name(mode);
 }
 
 static void i9xx_set_pipeconf(struct intel_crtc *intel_crtc)
-- 
2.1.0

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

Re: [PATCH v3 08/20] drm/i915: fill in more mode members

[Daniel Vetter]

On Mon, Jul 13, 2015 at 04:30:21PM +0200, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Needs a summary (or just pasting relevant parts of our mail threads) about
double-modesets, ways to avoid them and why exactly we ended up opting for
this solution here. Especially please highlight the TYPE_DRIVER trick.

Thanks, Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 118187dc76be..d37f6a93b094 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7754,9 +7754,14 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode,
>  	mode->vsync_end = pipe_config->base.adjusted_mode.crtc_vsync_end;
>  
>  	mode->flags = pipe_config->base.adjusted_mode.flags;
> +	mode->type = DRM_MODE_TYPE_DRIVER;
>  
>  	mode->clock = pipe_config->base.adjusted_mode.crtc_clock;
>  	mode->flags |= pipe_config->base.adjusted_mode.flags;
> +
> +	mode->hsync = drm_mode_hsync(mode);
> +	mode->vrefresh = drm_mode_vrefresh(mode);
> +	drm_mode_set_name(mode);
>  }
>  
>  static void i9xx_set_pipeconf(struct intel_crtc *intel_crtc)
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 08/20] drm/i915: fill in more mode members

[Maarten Lankhorst]

Hey,

Op 14-07-15 om 11:50 schreef Daniel Vetter:
> On Mon, Jul 13, 2015 at 04:30:21PM +0200, Maarten Lankhorst wrote:
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Needs a summary (or just pasting relevant parts of our mail threads) about
> double-modesets, ways to avoid them and why exactly we ended up opting for
> this solution here. Especially please highlight the TYPE_DRIVER trick.
Not in this patch I think, that would be for 11/20.

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

Re: [PATCH v3 08/20] drm/i915: fill in more mode members

[Daniel Stone]

Hi,

On 14 July 2015 at 10:50, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Jul 13, 2015 at 04:30:21PM +0200, Maarten Lankhorst wrote:
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>
> Needs a summary (or just pasting relevant parts of our mail threads) about
> double-modesets, ways to avoid them and why exactly we ended up opting for
> this solution here. Especially please highlight the TYPE_DRIVER trick.

Actually, the original motivation for this patch when it was in my
branch was very different: I was seeing failures on commit because
hsync == vsync == 0, and also the lack of name made debugging a bit
more difficult. TYPE_DRIVER is new though, and separately motivated.
:)

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

Re: [PATCH v3 08/20] drm/i915: fill in more mode members

[Maarten Lankhorst]

Op 14-07-15 om 13:21 schreef Daniel Stone:
> Hi,
>
> On 14 July 2015 at 10:50, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Mon, Jul 13, 2015 at 04:30:21PM +0200, Maarten Lankhorst wrote:
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Needs a summary (or just pasting relevant parts of our mail threads) about
>> double-modesets, ways to avoid them and why exactly we ended up opting for
>> this solution here. Especially please highlight the TYPE_DRIVER trick.
> Actually, the original motivation for this patch when it was in my
> branch was very different: I was seeing failures on commit because
> hsync == vsync == 0, and also the lack of name made debugging a bit
> more difficult. TYPE_DRIVER is new though, and separately motivated.
>
Oh, in my branch it was because it made me avoid modesets and to make fastboot work. But fastboot was made to work in a better way.

What about:
"Fill in driver type, hsync, vrefresh and name.
Those members are not read out but can be calculated from the mode."

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

[PATCH v3.1 08/20] drm/i915: fill in more mode members

[Maarten Lankhorst]

Fill in driver type, hsync, vrefresh and name.
Those members are not read out but can be calculated from the mode.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 434b43054d60..fa1cdefa5662 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7756,9 +7756,14 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode,
 	mode->vsync_end = pipe_config->base.adjusted_mode.crtc_vsync_end;
 
 	mode->flags = pipe_config->base.adjusted_mode.flags;
+	mode->type = DRM_MODE_TYPE_DRIVER;
 
 	mode->clock = pipe_config->base.adjusted_mode.crtc_clock;
 	mode->flags |= pipe_config->base.adjusted_mode.flags;
+
+	mode->hsync = drm_mode_hsync(mode);
+	mode->vrefresh = drm_mode_vrefresh(mode);
+	drm_mode_set_name(mode);
 }
 
 static void i9xx_set_pipeconf(struct intel_crtc *intel_crtc)

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

Re: [PATCH v3.1 08/20] drm/i915: fill in more mode members

[Daniel Stone]

On 14 July 2015 at 13:12, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
> Fill in driver type, hsync, vrefresh and name.
> Those members are not read out but can be calculated from the mode.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Reviewed-by: Daniel Stone <daniels@collabora.com>

Should be safe to merge independently.

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

Re: [PATCH v3 08/20] drm/i915: fill in more mode members

[Daniel Vetter]

On Tue, Jul 14, 2015 at 01:40:45PM +0200, Maarten Lankhorst wrote:
> Op 14-07-15 om 13:21 schreef Daniel Stone:
> > Hi,
> >
> > On 14 July 2015 at 10:50, Daniel Vetter <daniel@ffwll.ch> wrote:
> >> On Mon, Jul 13, 2015 at 04:30:21PM +0200, Maarten Lankhorst wrote:
> >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> Needs a summary (or just pasting relevant parts of our mail threads) about
> >> double-modesets, ways to avoid them and why exactly we ended up opting for
> >> this solution here. Especially please highlight the TYPE_DRIVER trick.
> > Actually, the original motivation for this patch when it was in my
> > branch was very different: I was seeing failures on commit because
> > hsync == vsync == 0, and also the lack of name made debugging a bit
> > more difficult. TYPE_DRIVER is new though, and separately motivated.
> >
> Oh, in my branch it was because it made me avoid modesets and to make fastboot work. But fastboot was made to work in a better way.
> 
> What about:
> "Fill in driver type, hsync, vrefresh and name.
> Those members are not read out but can be calculated from the mode."

Maybe add "Note that we mark the mode as TYPE_DRIVER to force a mismatch
with whatever mode userspace provides. This ensures we force a modeset on
the first real modeset, while avoiding spurious modesets on no-op property
changes before that (where the mode is just duplicated exactly)."
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 09/20] drm/i915: Fill in more crtc state, v3.

[Maarten Lankhorst]

There is a small memory leak in intel_modeset_readout_hw_state,
plug it.

intel_sanitize_crtc should set a null mode when disabling the crtc,
this updates crtc_state->enable too.

intel_sanitize_crtc also needs to update the vblank timestamps before
enabling vblank to make it work right.

Changes since v1:
- Moved after reworking primary plane and updating mode members.
- Force a modeset calculation by changing mode->type from what the
  final mode will be.
Changes since v2:
- Do not fill out mode, this should only be done for supporting fastboot.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d37f6a93b094..819a2b551f1f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15248,6 +15248,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) {
+		drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode);
 		update_scanline_offset(crtc);
 		drm_crtc_vblank_on(&crtc->base);
 	}
@@ -15300,7 +15301,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 			      crtc->base.state->enable ? "enabled" : "disabled",
 			      crtc->active ? "enabled" : "disabled");
 
-		crtc->base.state->enable = crtc->active;
+		WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, NULL) < 0);
 		crtc->base.state->active = crtc->active;
 		crtc->base.enabled = crtc->active;
 
@@ -15450,6 +15451,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 	int i;
 
 	for_each_intel_crtc(dev, crtc) {
+		__drm_atomic_helper_crtc_destroy_state(&crtc->base, crtc->base.state);
 		memset(crtc->config, 0, sizeof(*crtc->config));
 		crtc->config->base.crtc = &crtc->base;
 
-- 
2.1.0

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

Re: [PATCH v3 09/20] drm/i915: Fill in more crtc state, v3.

[Daniel Vetter]

On Mon, Jul 13, 2015 at 04:30:22PM +0200, Maarten Lankhorst wrote:
> There is a small memory leak in intel_modeset_readout_hw_state,
> plug it.

This should be a separate patch I think since it seems unrelated to the
other changes. And I think less mystery in the commit message helps, e.g.
"We leak crtc state references in the hw state readout, fix this." instead
of talking about an onimous small leak ;-)
-Daniel

> 
> intel_sanitize_crtc should set a null mode when disabling the crtc,
> this updates crtc_state->enable too.
> 
> intel_sanitize_crtc also needs to update the vblank timestamps before
> enabling vblank to make it work right.
> 
> Changes since v1:
> - Moved after reworking primary plane and updating mode members.
> - Force a modeset calculation by changing mode->type from what the
>   final mode will be.
> Changes since v2:
> - Do not fill out mode, this should only be done for supporting fastboot.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d37f6a93b094..819a2b551f1f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15248,6 +15248,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) {
> +		drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode);
>  		update_scanline_offset(crtc);
>  		drm_crtc_vblank_on(&crtc->base);
>  	}
> @@ -15300,7 +15301,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  			      crtc->base.state->enable ? "enabled" : "disabled",
>  			      crtc->active ? "enabled" : "disabled");
>  
> -		crtc->base.state->enable = crtc->active;
> +		WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, NULL) < 0);
>  		crtc->base.state->active = crtc->active;
>  		crtc->base.enabled = crtc->active;
>  
> @@ -15450,6 +15451,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  	int i;
>  
>  	for_each_intel_crtc(dev, crtc) {
> +		__drm_atomic_helper_crtc_destroy_state(&crtc->base, crtc->base.state);
>  		memset(crtc->config, 0, sizeof(*crtc->config));
>  		crtc->config->base.crtc = &crtc->base;
>  
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 09/20] drm/i915: Fill in more crtc state, v3.

[Maarten Lankhorst]

Op 14-07-15 om 11:49 schreef Daniel Vetter:
> On Mon, Jul 13, 2015 at 04:30:22PM +0200, Maarten Lankhorst wrote:
>> There is a small memory leak in intel_modeset_readout_hw_state,
>> plug it.
> This should be a separate patch I think since it seems unrelated to the
> other changes. And I think less mystery in the commit message helps, e.g.
> "We leak crtc state references in the hw state readout, fix this." instead
> of talking about an onimous small leak ;-)
>
Argh, you caught me on my laziness to split it out to separate patches.
Now I have no choice but to properly split and motivate it!
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/3] drm/i915: Fix reference leak in intel_modeset_readout_hw_state.

[Maarten Lankhorst]

Unreference the old mode_blob by calling the crtc_destroy_state
helper before zeroing the crtc_state.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.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 14ed5e69f275..4934261c8277 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15450,6 +15450,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 	int i;
 
 	for_each_intel_crtc(dev, crtc) {
+		__drm_atomic_helper_crtc_destroy_state(&crtc->base, crtc->base.state);
 		memset(crtc->config, 0, sizeof(*crtc->config));
 		crtc->config->base.crtc = &crtc->base;
 
-- 
2.1.0


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

Re: [PATCH 1/3] drm/i915: Fix reference leak in intel_modeset_readout_hw_state.

[Daniel Stone]

On 14 July 2015 at 12:42, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
> Unreference the old mode_blob by calling the crtc_destroy_state
> helper before zeroing the crtc_state.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Reviewed-by: Daniel Stone <daniels@collabora.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/3] drm/i915: Zero the mode in intel_sanitize_crtc when force disabling.

[Maarten Lankhorst]

There is a WARN_ON in drm_atomic_crtc_check for this when exposing the atomic property.
If the mode_blob still exists, but enable = false then all updates are rejected with -EINVAL.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4934261c8277..a8c8df779eb3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15300,7 +15300,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 			      crtc->base.state->enable ? "enabled" : "disabled",
 			      crtc->active ? "enabled" : "disabled");
 
-		crtc->base.state->enable = crtc->active;
+		WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, NULL) < 0);
 		crtc->base.state->active = crtc->active;
 		crtc->base.enabled = crtc->active;
 
-- 
2.1.0


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

Re: [PATCH 2/3] drm/i915: Zero the mode in intel_sanitize_crtc when force disabling.

[Daniel Stone]

On 14 July 2015 at 12:45, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
> There is a WARN_ON in drm_atomic_crtc_check for this when exposing the atomic property.
> If the mode_blob still exists, but enable = false then all updates are rejected with -EINVAL.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Reviewed-by: Daniel Stone <daniels@collabora.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/3] drm/i915: Calculate vblank timestamping constants before enabling vblank.

[Maarten Lankhorst]

This is required to properly initialize vblanks on the active crtc.
Without it drm_calc_vbltimestamp_from_scanoutpos can fail with
crtc 0: Noop due to uninitialized mode.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.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 a8c8df779eb3..f434a03e12fd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15248,6 +15248,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) {
+		drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode);
 		update_scanline_offset(crtc);
 		drm_crtc_vblank_on(&crtc->base);
 	}
-- 
2.1.0


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

Re: [PATCH 3/3] drm/i915: Calculate vblank timestamping constants before enabling vblank.

[Daniel Stone]

On 14 July 2015 at 12:46, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
> This is required to properly initialize vblanks on the active crtc.
> Without it drm_calc_vbltimestamp_from_scanoutpos can fail with
> crtc 0: Noop due to uninitialized mode.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Reviewed-by: Daniel Stone <daniels@collabora.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 10/20] drm/i915: Set csc coefficients in intel_sanitize_crtc.

[Maarten Lankhorst]

This might not have been set during boot, and when we preserve
the initial mode this can result in a black screen.

Cc: Daniel Stone <daniels@collabora.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 819a2b551f1f..80e878929bab 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15251,6 +15251,9 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 		drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode);
 		update_scanline_offset(crtc);
 		drm_crtc_vblank_on(&crtc->base);
+
+		if (IS_HASWELL(dev) || INTEL_INFO(dev)->gen > 7)
+			intel_set_pipe_csc(&crtc->base);
 	}
 
 	/* We need to sanitize the plane -> pipe mapping first because this will
-- 
2.1.0

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

Re: [PATCH v3 10/20] drm/i915: Set csc coefficients in intel_sanitize_crtc.

[Daniel Stone]

Hi,

On 13 July 2015 at 15:30, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
> This might not have been set during boot, and when we preserve
> the initial mode this can result in a black screen.
>
> Cc: Daniel Stone <daniels@collabora.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 819a2b551f1f..80e878929bab 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15251,6 +15251,9 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>                 drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode);
>                 update_scanline_offset(crtc);
>                 drm_crtc_vblank_on(&crtc->base);
> +
> +               if (IS_HASWELL(dev) || INTEL_INFO(dev)->gen > 7)
> +                       intel_set_pipe_csc(&crtc->base);
>         }

This is a bit of a weird one - and not your fault.

intel_set_pipe_csc is currently only called from haswell_crtc_enable,
which is only called for gen >= 9 or HAS_DDI. The (IS_HASWELL || gen >
7) test covers these devices, plus CHV as well. Does it work on CHV?

I'd be more tempted to just call this unconditionally, and stick an
early-exit test in intel_set_pipe_csc instead of at the callsites. But
intel_set_pipe_csc covers gen >= 6, which can never be triggered
through haswell_crtc_enable. So, if we added the early-exit to
set_pipe_csc, we'd also need to remove the previous-gen codepath, or
add calls to set_pipe_csc which didn't previously exist.

But with the test fixed up to be consistent (gen >= 9 || HAS_DDI):
Reviewed-by: Daniel Stone <daniels@collabora.com>

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

Re: [PATCH v3 10/20] drm/i915: Set csc coefficients in intel_sanitize_crtc.

[Maarten Lankhorst]

Op 13-07-15 om 18:21 schreef Daniel Stone:
> Hi,
>
> On 13 July 2015 at 15:30, Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com> wrote:
>> This might not have been set during boot, and when we preserve
>> the initial mode this can result in a black screen.
>>
>> Cc: Daniel Stone <daniels@collabora.com>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 819a2b551f1f..80e878929bab 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -15251,6 +15251,9 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>>                 drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode);
>>                 update_scanline_offset(crtc);
>>                 drm_crtc_vblank_on(&crtc->base);
>> +
>> +               if (IS_HASWELL(dev) || INTEL_INFO(dev)->gen > 7)
>> +                       intel_set_pipe_csc(&crtc->base);
>>         }
> This is a bit of a weird one - and not your fault.
>
> intel_set_pipe_csc is currently only called from haswell_crtc_enable,
> which is only called for gen >= 9 or HAS_DDI. The (IS_HASWELL || gen >
> 7) test covers these devices, plus CHV as well. Does it work on CHV?
I think testing for HAS_DDI is enough, works for skylake too.
> I'd be more tempted to just call this unconditionally, and stick an
> early-exit test in intel_set_pipe_csc instead of at the callsites. But
> intel_set_pipe_csc covers gen >= 6, which can never be triggered
> through haswell_crtc_enable. So, if we added the early-exit to
> set_pipe_csc, we'd also need to remove the previous-gen codepath, or
> add calls to set_pipe_csc which didn't previously exist.
But CSC is only enabled in update_primary_plane for haswell+, so no point in sanitizing something unused. :-)

> But with the test fixed up to be consistent (gen >= 9 || HAS_DDI):
> Reviewed-by: Daniel Stone <daniels@collabora.com>
>
> Cheers,
> Daniel

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

Re: [PATCH v3 10/20] drm/i915: Set csc coefficients in intel_sanitize_crtc.

[Daniel Vetter]

On Tue, Jul 14, 2015 at 10:50:21AM +0200, Maarten Lankhorst wrote:
> Op 13-07-15 om 18:21 schreef Daniel Stone:
> > Hi,
> >
> > On 13 July 2015 at 15:30, Maarten Lankhorst
> > <maarten.lankhorst@linux.intel.com> wrote:
> >> This might not have been set during boot, and when we preserve
> >> the initial mode this can result in a black screen.
> >>
> >> Cc: Daniel Stone <daniels@collabora.com>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_display.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 819a2b551f1f..80e878929bab 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -15251,6 +15251,9 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
> >>                 drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode);
> >>                 update_scanline_offset(crtc);
> >>                 drm_crtc_vblank_on(&crtc->base);
> >> +
> >> +               if (IS_HASWELL(dev) || INTEL_INFO(dev)->gen > 7)
> >> +                       intel_set_pipe_csc(&crtc->base);
> >>         }
> > This is a bit of a weird one - and not your fault.
> >
> > intel_set_pipe_csc is currently only called from haswell_crtc_enable,
> > which is only called for gen >= 9 or HAS_DDI. The (IS_HASWELL || gen >
> > 7) test covers these devices, plus CHV as well. Does it work on CHV?
> I think testing for HAS_DDI is enough, works for skylake too.
> > I'd be more tempted to just call this unconditionally, and stick an
> > early-exit test in intel_set_pipe_csc instead of at the callsites. But
> > intel_set_pipe_csc covers gen >= 6, which can never be triggered
> > through haswell_crtc_enable. So, if we added the early-exit to
> > set_pipe_csc, we'd also need to remove the previous-gen codepath, or
> > add calls to set_pipe_csc which didn't previously exist.
> But CSC is only enabled in update_primary_plane for haswell+, so no point in sanitizing something unused. :-)

I think we should instead stick the CSC update into the
update_primary_plane hook then? Since before that point we don't care
about CSC state at all.

Also CSC states need to change atomically with plane updates anyway, so
this seems like the right path forward. Can we postpone fixing this until
fastboot happens?
-Daniel

> 
> > But with the test fixed up to be consistent (gen >= 9 || HAS_DDI):
> > Reviewed-by: Daniel Stone <daniels@collabora.com>
> >
> > Cheers,
> > Daniel
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 10/20] drm/i915: Set csc coefficients in intel_sanitize_crtc.

[Maarten Lankhorst]

Op 14-07-15 om 11:55 schreef Daniel Vetter:
> On Tue, Jul 14, 2015 at 10:50:21AM +0200, Maarten Lankhorst wrote:
>> Op 13-07-15 om 18:21 schreef Daniel Stone:
>>> Hi,
>>>
>>> On 13 July 2015 at 15:30, Maarten Lankhorst
>>> <maarten.lankhorst@linux.intel.com> wrote:
>>>> This might not have been set during boot, and when we preserve
>>>> the initial mode this can result in a black screen.
>>>>
>>>> Cc: Daniel Stone <daniels@collabora.com>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/intel_display.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index 819a2b551f1f..80e878929bab 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -15251,6 +15251,9 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>>>>                 drm_calc_timestamping_constants(&crtc->base, &crtc->base.hwmode);
>>>>                 update_scanline_offset(crtc);
>>>>                 drm_crtc_vblank_on(&crtc->base);
>>>> +
>>>> +               if (IS_HASWELL(dev) || INTEL_INFO(dev)->gen > 7)
>>>> +                       intel_set_pipe_csc(&crtc->base);
>>>>         }
>>> This is a bit of a weird one - and not your fault.
>>>
>>> intel_set_pipe_csc is currently only called from haswell_crtc_enable,
>>> which is only called for gen >= 9 or HAS_DDI. The (IS_HASWELL || gen >
>>> 7) test covers these devices, plus CHV as well. Does it work on CHV?
>> I think testing for HAS_DDI is enough, works for skylake too.
>>> I'd be more tempted to just call this unconditionally, and stick an
>>> early-exit test in intel_set_pipe_csc instead of at the callsites. But
>>> intel_set_pipe_csc covers gen >= 6, which can never be triggered
>>> through haswell_crtc_enable. So, if we added the early-exit to
>>> set_pipe_csc, we'd also need to remove the previous-gen codepath, or
>>> add calls to set_pipe_csc which didn't previously exist.
>> But CSC is only enabled in update_primary_plane for haswell+, so no point in sanitizing something unused. :-)
> I think we should instead stick the CSC update into the
> update_primary_plane hook then? Since before that point we don't care
> about CSC state at all.
>
> Also CSC states need to change atomically with plane updates anyway, so
> this seems like the right path forward. Can we postpone fixing this until
> fastboot happens?
>
Postponing is fine.

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

[PATCH v3 11/20] drm/i915: Readout initial hw mode.

[Maarten Lankhorst]

Atomic requires a mode blob when crtc_state->enable is true.

With a few tweaks the mode we read out from hardware could be used
as the real mode without a modeset, but this requires too much
testing, so force a modeset the first time the mode blob's updated.

This preserves the old behavior, because previously we never set
the initial mode, which always meant that a modeset happened
when the mode was first set.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 38 ++++++++++++++++--------------------
 drivers/gpu/drm/i915/intel_fbdev.c   | 11 +++--------
 2 files changed, 20 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 80e878929bab..a4a2c3fbdc2d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13153,7 +13153,7 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
 		struct intel_crtc_state *pipe_config =
 			to_intel_crtc_state(crtc_state);
-		bool modeset, recalc;
+		bool modeset, recalc = false;
 
 		if (!crtc_state->enable) {
 			if (needs_modeset(crtc_state))
@@ -13162,7 +13162,9 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
 		}
 
 		modeset = needs_modeset(crtc_state);
-		recalc = pipe_config->quirks & PIPE_CONFIG_QUIRK_INHERITED_MODE;
+		if (!modeset && crtc_state->mode_blob != crtc->state->mode_blob &&
+		    pipe_config->quirks & PIPE_CONFIG_QUIRK_INHERITED_MODE)
+			recalc = true;
 
 		if (!modeset && !recalc)
 			continue;
@@ -13177,9 +13179,10 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
 		if (ret)
 			return ret;
 
-		if (recalc && !intel_pipe_config_compare(state->dev,
+		if (recalc && (!i915.fastboot ||
+		    !intel_pipe_config_compare(state->dev,
 					to_intel_crtc_state(crtc->state),
-					pipe_config, true)) {
+					pipe_config, true))) {
 			modeset = crtc_state->mode_changed = true;
 
 			ret = drm_atomic_add_affected_planes(state, crtc);
@@ -15463,11 +15466,19 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 		crtc->active = dev_priv->display.get_pipe_config(crtc,
 								 crtc->config);
 
-		crtc->base.state->enable = crtc->active;
 		crtc->base.state->active = crtc->active;
 		crtc->base.enabled = crtc->active;
-		crtc->base.hwmode = crtc->config->base.adjusted_mode;
 
+		memset(&crtc->base.mode, 0, sizeof(crtc->base.mode));
+		if (crtc->base.state->active) {
+			intel_mode_from_pipe_config(&crtc->base.mode, crtc->config);
+			intel_mode_from_pipe_config(&crtc->base.state->adjusted_mode, crtc->config);
+			WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, &crtc->base.mode));
+
+			crtc->base.state->mode.type = 0;
+		}
+
+		crtc->base.hwmode = crtc->config->base.adjusted_mode;
 		readout_plane_state(crtc, to_intel_crtc_state(crtc->base.state));
 
 		DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
@@ -15544,21 +15555,6 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 
 	intel_modeset_readout_hw_state(dev);
 
-	/*
-	 * Now that we have the config, copy it to each CRTC struct
-	 * Note that this could go away if we move to using crtc_config
-	 * checking everywhere.
-	 */
-	for_each_intel_crtc(dev, crtc) {
-		if (crtc->active && i915.fastboot) {
-			intel_mode_from_pipe_config(&crtc->base.mode,
-						    crtc->config);
-			DRM_DEBUG_KMS("[CRTC:%d] found active mode: ",
-				      crtc->base.base.id);
-			drm_mode_debug_printmodeline(&crtc->base.mode);
-		}
-	}
-
 	/* HW state is read out, now we need to sanitize this mess. */
 	for_each_intel_encoder(dev, encoder) {
 		intel_sanitize_encoder(encoder);
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index b791f2374f3b..7eff33ff84f6 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -483,18 +483,13 @@ retry:
 			 * IMPORTANT: We want to use the adjusted mode (i.e.
 			 * after the panel fitter upscaling) as the initial
 			 * config, not the input mode, which is what crtc->mode
-			 * usually contains. But since our current fastboot
+			 * usually contains. But since our current
 			 * code puts a mode derived from the post-pfit timings
-			 * into crtc->mode this works out correctly. We don't
-			 * use hwmode anywhere right now, so use it for this
-			 * since the fb helper layer wants a pointer to
-			 * something we own.
+			 * into crtc->mode this works out correctly.
 			 */
 			DRM_DEBUG_KMS("looking for current mode on connector %s\n",
 				      connector->name);
-			intel_mode_from_pipe_config(&encoder->crtc->hwmode,
-						    to_intel_crtc(encoder->crtc)->config);
-			modes[i] = &encoder->crtc->hwmode;
+			modes[i] = &encoder->crtc->mode;
 		}
 		crtcs[i] = new_crtc;
 
-- 
2.1.0

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

[PATCH v3.1 11/20] drm/i915: Readout initial hw mode.

[Maarten Lankhorst]

drm/i915: Readout initial hw mode, v2.
    
Atomic requires a mode blob when crtc_state->enable is true, or
you get a huge warn_on.
    
With a few tweaks the mode we read out from hardware could be used
as the real mode without a modeset, but this requires too much
testing, so for now force a modeset the first time the mode blob's
updated.
    
This preserves the old behavior, because previously we never set
the initial mode, which always meant that a modeset happened
when the mode was first set.
    
Changes since v1:
- Add a description in intel_modeset_readout_hw_state of how the
  recalculation is done.
    
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6ef3ef4f7be0..a815db672a8a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13152,7 +13152,7 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
 		struct intel_crtc_state *pipe_config =
 			to_intel_crtc_state(crtc_state);
-		bool modeset, recalc;
+		bool modeset, recalc = false;
 
 		if (!crtc_state->enable) {
 			if (needs_modeset(crtc_state))
@@ -13161,7 +13161,10 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
 		}
 
 		modeset = needs_modeset(crtc_state);
-		recalc = pipe_config->quirks & PIPE_CONFIG_QUIRK_INHERITED_MODE;
+		/* see comment in intel_modeset_readout_hw_state */
+		if (!modeset && crtc_state->mode_blob != crtc->state->mode_blob &&
+		    pipe_config->quirks & PIPE_CONFIG_QUIRK_INHERITED_MODE)
+			recalc = true;
 
 		if (!modeset && !recalc)
 			continue;
@@ -13176,9 +13179,10 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
 		if (ret)
 			return ret;
 
-		if (recalc && !intel_pipe_config_compare(state->dev,
+		if (recalc && (!i915.fastboot ||
+		    !intel_pipe_config_compare(state->dev,
 					to_intel_crtc_state(crtc->state),
-					pipe_config, true)) {
+					pipe_config, true))) {
 			modeset = crtc_state->mode_changed = true;
 
 			ret = drm_atomic_add_affected_planes(state, crtc);
@@ -15457,11 +15461,42 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 		crtc->active = dev_priv->display.get_pipe_config(crtc,
 								 crtc->config);
 
-		crtc->base.state->enable = crtc->active;
 		crtc->base.state->active = crtc->active;
 		crtc->base.enabled = crtc->active;
-		crtc->base.hwmode = crtc->config->base.adjusted_mode;
 
+		memset(&crtc->base.mode, 0, sizeof(crtc->base.mode));
+		if (crtc->base.state->active) {
+			intel_mode_from_pipe_config(&crtc->base.mode, crtc->config);
+			intel_mode_from_pipe_config(&crtc->base.state->adjusted_mode, crtc->config);
+			WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, &crtc->base.mode));
+
+			/*
+			 * The initial mode needs to be set in order to keep
+			 * the atomic core happy. It wants a valid mode if the
+			 * crtc's enabled, so we do the above call.
+			 *
+			 * At this point some state updated by the connectors
+			 * in their ->detect() callback has not run yet, so
+			 * no recalculation can be done yet.
+			 *
+			 * Even if we could do a recalculation and modeset
+			 * right now it would cause a double modeset if
+			 * fbdev or userspace chooses a different initial mode.
+			 *
+			 * So to prevent the double modeset, fail the memcmp
+			 * test in drm_atomic_set_mode_for_crtc to get a new
+			 * mode blob, and compare if the mode blob changed
+			 * when the PIPE_CONFIG_QUIRK_INHERITED_MODE quirk is
+			 * set.
+			 *
+			 * If that happens, someone indicated they wanted a
+			 * mode change, which means it's safe to do a full
+			 * recalculation.
+			 */
+			crtc->base.state->mode.private_flags = ~0;
+		}
+
+		crtc->base.hwmode = crtc->config->base.adjusted_mode;
 		readout_plane_state(crtc, to_intel_crtc_state(crtc->base.state));
 
 		DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
@@ -15538,21 +15573,6 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 
 	intel_modeset_readout_hw_state(dev);
 
-	/*
-	 * Now that we have the config, copy it to each CRTC struct
-	 * Note that this could go away if we move to using crtc_config
-	 * checking everywhere.
-	 */
-	for_each_intel_crtc(dev, crtc) {
-		if (crtc->active && i915.fastboot) {
-			intel_mode_from_pipe_config(&crtc->base.mode,
-						    crtc->config);
-			DRM_DEBUG_KMS("[CRTC:%d] found active mode: ",
-				      crtc->base.base.id);
-			drm_mode_debug_printmodeline(&crtc->base.mode);
-		}
-	}
-
 	/* HW state is read out, now we need to sanitize this mess. */
 	for_each_intel_encoder(dev, encoder) {
 		intel_sanitize_encoder(encoder);
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index b791f2374f3b..7eff33ff84f6 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -483,18 +483,13 @@ retry:
 			 * IMPORTANT: We want to use the adjusted mode (i.e.
 			 * after the panel fitter upscaling) as the initial
 			 * config, not the input mode, which is what crtc->mode
-			 * usually contains. But since our current fastboot
+			 * usually contains. But since our current
 			 * code puts a mode derived from the post-pfit timings
-			 * into crtc->mode this works out correctly. We don't
-			 * use hwmode anywhere right now, so use it for this
-			 * since the fb helper layer wants a pointer to
-			 * something we own.
+			 * into crtc->mode this works out correctly.
 			 */
 			DRM_DEBUG_KMS("looking for current mode on connector %s\n",
 				      connector->name);
-			intel_mode_from_pipe_config(&encoder->crtc->hwmode,
-						    to_intel_crtc(encoder->crtc)->config);
-			modes[i] = &encoder->crtc->hwmode;
+			modes[i] = &encoder->crtc->mode;
 		}
 		crtcs[i] = new_crtc;
 

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

[PATCH v3 12/20] drm/i915: Convert resume to atomic.

[Maarten Lankhorst]

Instead of all the ad-hoc updating, duplicate the old state first
before reading out the hw state, then restore it.

intel_display_resume is a new function that duplicates the sw state,
then reads out the hw state, and commits the old state.

intel_display_setup_hw_state now only reads out the atomic state.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90396
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c      |  2 +-
 drivers/gpu/drm/i915/i915_drv.h      |  3 +-
 drivers/gpu/drm/i915/intel_display.c | 77 ++++++++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_lvds.c    |  2 +-
 4 files changed, 60 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e44dc0d6656f..db48aee7f140 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -741,7 +741,7 @@ static int i915_drm_resume(struct drm_device *dev)
 	spin_unlock_irq(&dev_priv->irq_lock);
 
 	drm_modeset_lock_all(dev);
-	intel_modeset_setup_hw_state(dev, true);
+	intel_display_resume(dev);
 	drm_modeset_unlock_all(dev);
 
 	intel_dp_mst_resume(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 38cfd53bc262..d3e14cb43924 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3279,8 +3279,7 @@ extern void intel_modeset_gem_init(struct drm_device *dev);
 extern void intel_modeset_cleanup(struct drm_device *dev);
 extern void intel_connector_unregister(struct intel_connector *);
 extern int intel_modeset_vga_set_state(struct drm_device *dev, bool state);
-extern void intel_modeset_setup_hw_state(struct drm_device *dev,
-					 bool force_restore);
+extern void intel_display_resume(struct drm_device *dev);
 extern void i915_redisable_vga(struct drm_device *dev);
 extern void i915_redisable_vga_power_on(struct drm_device *dev);
 extern bool ironlake_set_drps(struct drm_device *dev, u8 val);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a4a2c3fbdc2d..b8984b4f8081 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -109,6 +109,7 @@ static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_cr
 	struct intel_crtc_state *crtc_state);
 static int i9xx_get_refclk(const struct intel_crtc_state *crtc_state,
 			   int num_connectors);
+static void intel_modeset_setup_hw_state(struct drm_device *dev);
 
 static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe)
 {
@@ -3249,7 +3250,7 @@ void intel_finish_reset(struct drm_device *dev)
 		dev_priv->display.hpd_irq_setup(dev);
 	spin_unlock_irq(&dev_priv->irq_lock);
 
-	intel_modeset_setup_hw_state(dev, true);
+	intel_display_resume(dev);
 
 	intel_hpd_init(dev_priv);
 
@@ -15163,7 +15164,7 @@ void intel_modeset_init(struct drm_device *dev)
 	intel_fbc_disable(dev_priv);
 
 	drm_modeset_lock_all(dev);
-	intel_modeset_setup_hw_state(dev, false);
+	intel_modeset_setup_hw_state(dev);
 	drm_modeset_unlock_all(dev);
 
 	for_each_intel_crtc(dev, crtc) {
@@ -15542,10 +15543,11 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 	}
 }
 
-/* Scan out the current hw modeset state, sanitizes it and maps it into the drm
- * and i915 state tracking structures. */
-void intel_modeset_setup_hw_state(struct drm_device *dev,
-				  bool force_restore)
+/* Scan out the current hw modeset state,
+ * and sanitizes it to the current state
+ */
+static void
+intel_modeset_setup_hw_state(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	enum pipe pipe;
@@ -15588,24 +15590,59 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 	else if (HAS_PCH_SPLIT(dev))
 		ilk_wm_get_hw_state(dev);
 
-	if (force_restore) {
-		i915_redisable_vga(dev);
+	intel_modeset_update_staged_output_state(dev);
+}
 
-		/*
-		 * We need to use raw interfaces for restoring state to avoid
-		 * checking (bogus) intermediate states.
-		 */
-		for_each_pipe(dev_priv, pipe) {
-			struct drm_crtc *crtc =
-				dev_priv->pipe_to_crtc_mapping[pipe];
+void intel_display_resume(struct drm_device *dev)
+{
+	struct drm_atomic_state *state = drm_atomic_state_alloc(dev);
+	struct intel_connector *conn;
+	struct intel_plane *plane;
+	struct drm_crtc *crtc;
+	int ret;
 
-			intel_crtc_restore_mode(crtc);
-		}
-	} else {
-		intel_modeset_update_staged_output_state(dev);
+	if (!state)
+		return;
+
+	state->acquire_ctx = dev->mode_config.acquire_ctx;
+
+	/* preserve complete old state, including dpll */
+	intel_atomic_get_shared_dpll_state(state);
+
+	for_each_crtc(dev, crtc) {
+		struct drm_crtc_state *crtc_state =
+			drm_atomic_get_crtc_state(state, crtc);
+
+		ret = PTR_ERR_OR_ZERO(crtc_state);
+		if (ret)
+			goto err;
+
+		/* force a restore */
+		crtc_state->mode_changed = true;
 	}
 
-	intel_modeset_check_state(dev);
+	for_each_intel_plane(dev, plane) {
+		ret = PTR_ERR_OR_ZERO(drm_atomic_get_plane_state(state, &plane->base));
+		if (ret)
+			goto err;
+	}
+
+	for_each_intel_connector(dev, conn) {
+		ret = PTR_ERR_OR_ZERO(drm_atomic_get_connector_state(state, &conn->base));
+		if (ret)
+			goto err;
+	}
+
+	intel_modeset_setup_hw_state(dev);
+
+	i915_redisable_vga(dev);
+	ret = intel_set_mode(state);
+	if (!ret)
+		return;
+
+err:
+	DRM_ERROR("Restoring old state failed with %i\n", ret);
+	drm_atomic_state_free(state);
 }
 
 void intel_modeset_gem_init(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 937e8216e9d6..cb634f48e7d9 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -473,7 +473,7 @@ static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
 	 */
 	if (!HAS_PCH_SPLIT(dev)) {
 		drm_modeset_lock_all(dev);
-		intel_modeset_setup_hw_state(dev, true);
+		intel_display_resume(dev);
 		drm_modeset_unlock_all(dev);
 	}
 
-- 
2.1.0

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

[PATCH v3 13/20] drm/i915: Get rid of unused transitional members.

[Maarten Lankhorst]

The previous commit converted hw readout to atomic, all the new_*
members were used for restoring the old state, but with the
conversion of suspend to atomic there's no use left for them.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 80 ++++++++----------------------------
 drivers/gpu/drm/i915/intel_drv.h     | 12 ------
 2 files changed, 18 insertions(+), 74 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b8984b4f8081..6bab045b389c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10254,7 +10254,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector,
 retry:
 	ret = drm_modeset_lock(&config->connection_mutex, ctx);
 	if (ret)
-		goto fail_unlock;
+		goto fail;
 
 	/*
 	 * Algorithm gets a little messy:
@@ -10272,10 +10272,10 @@ retry:
 
 		ret = drm_modeset_lock(&crtc->mutex, ctx);
 		if (ret)
-			goto fail_unlock;
+			goto fail;
 		ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
 		if (ret)
-			goto fail_unlock;
+			goto fail;
 
 		old->dpms_mode = connector->dpms;
 		old->load_detect_temp = false;
@@ -10294,9 +10294,6 @@ retry:
 			continue;
 		if (possible_crtc->state->enable)
 			continue;
-		/* This can occur when applying the pipe A quirk on resume. */
-		if (to_intel_crtc(possible_crtc)->new_enabled)
-			continue;
 
 		crtc = possible_crtc;
 		break;
@@ -10307,20 +10304,17 @@ retry:
 	 */
 	if (!crtc) {
 		DRM_DEBUG_KMS("no pipe available for load-detect\n");
-		goto fail_unlock;
+		goto fail;
 	}
 
 	ret = drm_modeset_lock(&crtc->mutex, ctx);
 	if (ret)
-		goto fail_unlock;
+		goto fail;
 	ret = drm_modeset_lock(&crtc->primary->mutex, ctx);
 	if (ret)
-		goto fail_unlock;
-	intel_encoder->new_crtc = to_intel_crtc(crtc);
-	to_intel_connector(connector)->new_encoder = intel_encoder;
+		goto fail;
 
 	intel_crtc = to_intel_crtc(crtc);
-	intel_crtc->new_enabled = true;
 	old->dpms_mode = connector->dpms;
 	old->load_detect_temp = true;
 	old->release_fb = NULL;
@@ -10388,9 +10382,7 @@ retry:
 	intel_wait_for_vblank(dev, intel_crtc->pipe);
 	return true;
 
- fail:
-	intel_crtc->new_enabled = crtc->state->enable;
-fail_unlock:
+fail:
 	drm_atomic_state_free(state);
 	state = NULL;
 
@@ -10436,10 +10428,6 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
 		if (IS_ERR(crtc_state))
 			goto fail;
 
-		to_intel_connector(connector)->new_encoder = NULL;
-		intel_encoder->new_crtc = NULL;
-		intel_crtc->new_enabled = false;
-
 		connector_state->best_encoder = NULL;
 		connector_state->crtc = NULL;
 
@@ -11843,37 +11831,6 @@ static const struct drm_crtc_helper_funcs intel_helper_funcs = {
 	.atomic_check = intel_crtc_atomic_check,
 };
 
-/**
- * intel_modeset_update_staged_output_state
- *
- * Updates the staged output configuration state, e.g. after we've read out the
- * current hw state.
- */
-static void intel_modeset_update_staged_output_state(struct drm_device *dev)
-{
-	struct intel_crtc *crtc;
-	struct intel_encoder *encoder;
-	struct intel_connector *connector;
-
-	for_each_intel_connector(dev, connector) {
-		connector->new_encoder =
-			to_intel_encoder(connector->base.encoder);
-	}
-
-	for_each_intel_encoder(dev, encoder) {
-		encoder->new_crtc =
-			to_intel_crtc(encoder->base.crtc);
-	}
-
-	for_each_intel_crtc(dev, crtc) {
-		crtc->new_enabled = crtc->base.state->enable;
-	}
-}
-
-/* Transitional helper to copy current connector/encoder state to
- * connector->state. This is needed so that code that is partially
- * converted to atomic does the right thing.
- */
 static void intel_modeset_update_connector_atomic_state(struct drm_device *dev)
 {
 	struct intel_connector *connector;
@@ -12314,7 +12271,6 @@ intel_modeset_update_state(struct drm_atomic_state *state)
 	}
 
 	drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
-	intel_modeset_update_staged_output_state(state->dev);
 
 	/* Double check state. */
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
@@ -12714,11 +12670,14 @@ check_connector_state(struct drm_device *dev)
 	struct intel_connector *connector;
 
 	for_each_intel_connector(dev, connector) {
+		struct drm_encoder *encoder = connector->base.encoder;
+		struct drm_connector_state *state = connector->base.state;
+
 		/* This also checks the encoder/connector hw state with the
 		 * ->get_hw_state callbacks. */
 		intel_connector_check_state(connector);
 
-		I915_STATE_WARN(&connector->new_encoder->base != connector->base.encoder,
+		I915_STATE_WARN(state->best_encoder != encoder,
 		     "connector's staged encoder doesn't match current encoder\n");
 	}
 }
@@ -12738,8 +12697,6 @@ check_encoder_state(struct drm_device *dev)
 			      encoder->base.base.id,
 			      encoder->base.name);
 
-		I915_STATE_WARN(&encoder->new_crtc->base != encoder->base.crtc,
-		     "encoder's stage crtc doesn't match current crtc\n");
 		I915_STATE_WARN(encoder->connectors_active && !encoder->base.crtc,
 		     "encoder's active_connectors set, but no crtc\n");
 
@@ -12749,6 +12706,10 @@ check_encoder_state(struct drm_device *dev)
 			enabled = true;
 			if (connector->base.dpms != DRM_MODE_DPMS_OFF)
 				active = true;
+
+			I915_STATE_WARN(connector->base.state->crtc !=
+					encoder->base.crtc,
+			     "connector's crtc doesn't match encoder crtc\n");
 		}
 		/*
 		 * for MST connectors if we unplug the connector is gone
@@ -13318,11 +13279,12 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
 	 * need to copy the staged config to the atomic state, otherwise the
 	 * mode set will just reapply the state the HW is already in. */
 	for_each_intel_encoder(dev, encoder) {
-		if (&encoder->new_crtc->base != crtc)
+		if (encoder->base.crtc != crtc)
 			continue;
 
 		for_each_intel_connector(dev, connector) {
-			if (connector->new_encoder != encoder)
+			if (connector->base.state->best_encoder !=
+			    &encoder->base)
 				continue;
 
 			connector_state = drm_atomic_get_connector_state(state, &connector->base);
@@ -13335,7 +13297,6 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
 			}
 
 			connector_state->crtc = crtc;
-			connector_state->best_encoder = &encoder->base;
 		}
 	}
 
@@ -13347,9 +13308,6 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
 		return;
 	}
 
-	crtc_state->base.active = crtc_state->base.enable =
-		to_intel_crtc(crtc)->new_enabled;
-
 	drm_mode_copy(&crtc_state->base.mode, &crtc->mode);
 
 	intel_modeset_setup_plane_state(state, crtc, &crtc->mode,
@@ -15589,8 +15547,6 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
 		skl_wm_get_hw_state(dev);
 	else if (HAS_PCH_SPLIT(dev))
 		ilk_wm_get_hw_state(dev);
-
-	intel_modeset_update_staged_output_state(dev);
 }
 
 void intel_display_resume(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 2c23900b491f..217b773e0673 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -130,11 +130,6 @@ struct intel_fbdev {
 
 struct intel_encoder {
 	struct drm_encoder base;
-	/*
-	 * The new crtc this encoder will be driven from. Only differs from
-	 * base->crtc while a modeset is in progress.
-	 */
-	struct intel_crtc *new_crtc;
 
 	enum intel_output_type type;
 	unsigned int cloneable;
@@ -195,12 +190,6 @@ struct intel_connector {
 	 */
 	struct intel_encoder *encoder;
 
-	/*
-	 * The new encoder this connector will be driven. Only differs from
-	 * encoder while a modeset is in progress.
-	 */
-	struct intel_encoder *new_encoder;
-
 	/* Reads out the current hw, returning true if the connector is enabled
 	 * and active (i.e. dpms ON state). */
 	bool (*get_hw_state)(struct intel_connector *);
@@ -550,7 +539,6 @@ struct intel_crtc {
 	uint32_t cursor_base;
 
 	struct intel_crtc_state *config;
-	bool new_enabled;
 
 	/* reset counter value when the last flip was submitted */
 	unsigned int reset_counter;
-- 
2.1.0

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

[PATCH v3 14/20] drm/i915: Update power domains on readout.

[Maarten Lankhorst]

This allows us to get rid of the set_init_power in
modeset_update_crtc_domains. The state should be sanitized enough
after setup_hw_state to not need the init power.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 76 ++++++++++++++++++++++++------------
 1 file changed, 50 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6bab045b389c..f6c65706cebf 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5194,6 +5194,9 @@ static unsigned long get_crtc_power_domains(struct drm_crtc *crtc)
 	unsigned long mask;
 	enum transcoder transcoder;
 
+	if (!crtc->state->active)
+		return 0;
+
 	transcoder = intel_pipe_to_cpu_transcoder(dev->dev_private, pipe);
 
 	mask = BIT(POWER_DOMAIN_PIPE(pipe));
@@ -5208,27 +5211,46 @@ static unsigned long get_crtc_power_domains(struct drm_crtc *crtc)
 	return mask;
 }
 
-static void modeset_update_crtc_power_domains(struct drm_atomic_state *state)
+static unsigned long modeset_get_crtc_power_domains(struct drm_crtc *crtc)
 {
-	struct drm_device *dev = state->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	unsigned long pipe_domains[I915_MAX_PIPES] = { 0, };
-	struct intel_crtc *crtc;
+	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	enum intel_display_power_domain domain;
+	unsigned long domains, new_domains, old_domains;
 
-	/*
-	 * First get all needed power domains, then put all unneeded, to avoid
-	 * any unnecessary toggling of the power wells.
-	 */
-	for_each_intel_crtc(dev, crtc) {
-		enum intel_display_power_domain domain;
+	old_domains = intel_crtc->enabled_power_domains;
+	intel_crtc->enabled_power_domains = new_domains = get_crtc_power_domains(crtc);
 
-		if (!crtc->base.state->enable)
-			continue;
+	domains = new_domains & ~old_domains;
+
+	for_each_power_domain(domain, domains)
+		intel_display_power_get(dev_priv, domain);
+
+	return old_domains & ~new_domains;
+}
+
+static void modeset_put_power_domains(struct drm_i915_private *dev_priv,
+				      unsigned long domains)
+{
+	enum intel_display_power_domain domain;
+
+	for_each_power_domain(domain, domains)
+		intel_display_power_put(dev_priv, domain);
+}
 
-		pipe_domains[crtc->pipe] = get_crtc_power_domains(&crtc->base);
+static void modeset_update_crtc_power_domains(struct drm_atomic_state *state)
+{
+	struct drm_device *dev = state->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	unsigned long put_domains[I915_MAX_PIPES] = {};
+	struct drm_crtc_state *crtc_state;
+	struct drm_crtc *crtc;
+	int i;
 
-		for_each_power_domain(domain, pipe_domains[crtc->pipe])
-			intel_display_power_get(dev_priv, domain);
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		if (needs_modeset(crtc->state))
+			put_domains[to_intel_crtc(crtc)->pipe] =
+				modeset_get_crtc_power_domains(crtc);
 	}
 
 	if (dev_priv->display.modeset_commit_cdclk) {
@@ -5239,16 +5261,9 @@ static void modeset_update_crtc_power_domains(struct drm_atomic_state *state)
 			dev_priv->display.modeset_commit_cdclk(state);
 	}
 
-	for_each_intel_crtc(dev, crtc) {
-		enum intel_display_power_domain domain;
-
-		for_each_power_domain(domain, crtc->enabled_power_domains)
-			intel_display_power_put(dev_priv, domain);
-
-		crtc->enabled_power_domains = pipe_domains[crtc->pipe];
-	}
-
-	intel_display_set_init_power(dev_priv, false);
+	for (i = 0; i < I915_MAX_PIPES; i++)
+		if (put_domains[i])
+			modeset_put_power_domains(dev_priv, put_domains[i]);
 }
 
 static void intel_update_max_cdclk(struct drm_device *dev)
@@ -15547,6 +15562,15 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
 		skl_wm_get_hw_state(dev);
 	else if (HAS_PCH_SPLIT(dev))
 		ilk_wm_get_hw_state(dev);
+
+	for_each_intel_crtc(dev, crtc) {
+		unsigned long put_domains;
+
+		put_domains = modeset_get_crtc_power_domains(&crtc->base);
+		if (WARN_ON(put_domains))
+			modeset_put_power_domains(dev_priv, put_domains);
+	}
+	intel_display_set_init_power(dev_priv, false);
 }
 
 void intel_display_resume(struct drm_device *dev)
-- 
2.1.0

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

[PATCH v3 15/20] drm/i915: Always reset in intel_crtc_restore_mode

[Maarten Lankhorst]

And get rid of things that are no longer true. This function is only
used for forcing a modeset when encoder properties are changed.

Because this is not yet done atomically, assume a full modeset is
needed and reset the crtc.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 58 ++++++++++--------------------------
 1 file changed, 16 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f6c65706cebf..599af76d34f6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13273,63 +13273,37 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_atomic_state *state;
-	struct intel_encoder *encoder;
-	struct intel_connector *connector;
-	struct drm_connector_state *connector_state;
-	struct intel_crtc_state *crtc_state;
+	struct drm_crtc_state *crtc_state;
 	int ret;
 
 	state = drm_atomic_state_alloc(dev);
 	if (!state) {
-		DRM_DEBUG_KMS("[CRTC:%d] mode restore failed, out of memory",
+		DRM_DEBUG_KMS("[CRTC:%d] crtc restore failed, out of memory",
 			      crtc->base.id);
 		return;
 	}
 
-	state->acquire_ctx = dev->mode_config.acquire_ctx;
-
-	/* The force restore path in the HW readout code relies on the staged
-	 * config still keeping the user requested config while the actual
-	 * state has been overwritten by the configuration read from HW. We
-	 * need to copy the staged config to the atomic state, otherwise the
-	 * mode set will just reapply the state the HW is already in. */
-	for_each_intel_encoder(dev, encoder) {
-		if (encoder->base.crtc != crtc)
-			continue;
+	state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc);
 
-		for_each_intel_connector(dev, connector) {
-			if (connector->base.state->best_encoder !=
-			    &encoder->base)
-				continue;
-
-			connector_state = drm_atomic_get_connector_state(state, &connector->base);
-			if (IS_ERR(connector_state)) {
-				DRM_DEBUG_KMS("Failed to add [CONNECTOR:%d:%s] to state: %ld\n",
-					      connector->base.base.id,
-					      connector->base.name,
-					      PTR_ERR(connector_state));
-				continue;
-			}
+retry:
+	crtc_state = drm_atomic_get_crtc_state(state, crtc);
+	ret = PTR_ERR_OR_ZERO(crtc_state);
+	if (!ret) {
+		if (!crtc_state->active)
+			goto out;
 
-			connector_state->crtc = crtc;
-		}
+		crtc_state->mode_changed = true;
+		ret = intel_set_mode(state);
 	}
 
-	crtc_state = intel_atomic_get_crtc_state(state, to_intel_crtc(crtc));
-	if (IS_ERR(crtc_state)) {
-		DRM_DEBUG_KMS("Failed to add [CRTC:%d] to state: %ld\n",
-			      crtc->base.id, PTR_ERR(crtc_state));
-		drm_atomic_state_free(state);
-		return;
+	if (ret == -EDEADLK) {
+		drm_atomic_state_clear(state);
+		drm_modeset_backoff(state->acquire_ctx);
+		goto retry;
 	}
 
-	drm_mode_copy(&crtc_state->base.mode, &crtc->mode);
-
-	intel_modeset_setup_plane_state(state, crtc, &crtc->mode,
-					crtc->primary->fb, crtc->x, crtc->y);
-
-	ret = intel_set_mode(state);
 	if (ret)
+out:
 		drm_atomic_state_free(state);
 }
 
-- 
2.1.0

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

Re: [PATCH v3 15/20] drm/i915: Always reset in intel_crtc_restore_mode

[Daniel Vetter]

On Mon, Jul 13, 2015 at 04:30:28PM +0200, Maarten Lankhorst wrote:
> And get rid of things that are no longer true. This function is only
> used for forcing a modeset when encoder properties are changed.
> 
> Because this is not yet done atomically, assume a full modeset is
> needed and reset the crtc.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

s/reset/force modeset/ in the subject? Reset has a different meaning for
me ...
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 58 ++++++++++--------------------------
>  1 file changed, 16 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f6c65706cebf..599af76d34f6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13273,63 +13273,37 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_atomic_state *state;
> -	struct intel_encoder *encoder;
> -	struct intel_connector *connector;
> -	struct drm_connector_state *connector_state;
> -	struct intel_crtc_state *crtc_state;
> +	struct drm_crtc_state *crtc_state;
>  	int ret;
>  
>  	state = drm_atomic_state_alloc(dev);
>  	if (!state) {
> -		DRM_DEBUG_KMS("[CRTC:%d] mode restore failed, out of memory",
> +		DRM_DEBUG_KMS("[CRTC:%d] crtc restore failed, out of memory",
>  			      crtc->base.id);
>  		return;
>  	}
>  
> -	state->acquire_ctx = dev->mode_config.acquire_ctx;
> -
> -	/* The force restore path in the HW readout code relies on the staged
> -	 * config still keeping the user requested config while the actual
> -	 * state has been overwritten by the configuration read from HW. We
> -	 * need to copy the staged config to the atomic state, otherwise the
> -	 * mode set will just reapply the state the HW is already in. */
> -	for_each_intel_encoder(dev, encoder) {
> -		if (encoder->base.crtc != crtc)
> -			continue;
> +	state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc);
>  
> -		for_each_intel_connector(dev, connector) {
> -			if (connector->base.state->best_encoder !=
> -			    &encoder->base)
> -				continue;
> -
> -			connector_state = drm_atomic_get_connector_state(state, &connector->base);
> -			if (IS_ERR(connector_state)) {
> -				DRM_DEBUG_KMS("Failed to add [CONNECTOR:%d:%s] to state: %ld\n",
> -					      connector->base.base.id,
> -					      connector->base.name,
> -					      PTR_ERR(connector_state));
> -				continue;
> -			}
> +retry:
> +	crtc_state = drm_atomic_get_crtc_state(state, crtc);
> +	ret = PTR_ERR_OR_ZERO(crtc_state);
> +	if (!ret) {
> +		if (!crtc_state->active)
> +			goto out;
>  
> -			connector_state->crtc = crtc;
> -		}
> +		crtc_state->mode_changed = true;
> +		ret = intel_set_mode(state);
>  	}
>  
> -	crtc_state = intel_atomic_get_crtc_state(state, to_intel_crtc(crtc));
> -	if (IS_ERR(crtc_state)) {
> -		DRM_DEBUG_KMS("Failed to add [CRTC:%d] to state: %ld\n",
> -			      crtc->base.id, PTR_ERR(crtc_state));
> -		drm_atomic_state_free(state);
> -		return;
> +	if (ret == -EDEADLK) {
> +		drm_atomic_state_clear(state);
> +		drm_modeset_backoff(state->acquire_ctx);
> +		goto retry;
>  	}
>  
> -	drm_mode_copy(&crtc_state->base.mode, &crtc->mode);
> -
> -	intel_modeset_setup_plane_state(state, crtc, &crtc->mode,
> -					crtc->primary->fb, crtc->x, crtc->y);
> -
> -	ret = intel_set_mode(state);
>  	if (ret)
> +out:
>  		drm_atomic_state_free(state);
>  }
>  
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3.1 15/20] drm/i915: Always force a modeset in intel_crtc_restore_mode, v2.

[Maarten Lankhorst]

And get rid of things that are no longer true. This function is only
used for forcing a modeset when encoder properties are changed.

Because this is not yet done atomically, assume a full modeset is
needed and force a modeset on the crtc.

Changes since v1:
- s/reset/force modeset/

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Daniel Stone <daniels@collabora.com>
---
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9537ad8be4a9..499223e6f736 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13273,63 +13273,37 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_atomic_state *state;
-	struct intel_encoder *encoder;
-	struct intel_connector *connector;
-	struct drm_connector_state *connector_state;
-	struct intel_crtc_state *crtc_state;
+	struct drm_crtc_state *crtc_state;
 	int ret;
 
 	state = drm_atomic_state_alloc(dev);
 	if (!state) {
-		DRM_DEBUG_KMS("[CRTC:%d] mode restore failed, out of memory",
+		DRM_DEBUG_KMS("[CRTC:%d] crtc restore failed, out of memory",
 			      crtc->base.id);
 		return;
 	}
 
-	state->acquire_ctx = dev->mode_config.acquire_ctx;
-
-	/* The force restore path in the HW readout code relies on the staged
-	 * config still keeping the user requested config while the actual
-	 * state has been overwritten by the configuration read from HW. We
-	 * need to copy the staged config to the atomic state, otherwise the
-	 * mode set will just reapply the state the HW is already in. */
-	for_each_intel_encoder(dev, encoder) {
-		if (encoder->base.crtc != crtc)
-			continue;
+	state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc);
 
-		for_each_intel_connector(dev, connector) {
-			if (connector->base.state->best_encoder !=
-			    &encoder->base)
-				continue;
-
-			connector_state = drm_atomic_get_connector_state(state, &connector->base);
-			if (IS_ERR(connector_state)) {
-				DRM_DEBUG_KMS("Failed to add [CONNECTOR:%d:%s] to state: %ld\n",
-					      connector->base.base.id,
-					      connector->base.name,
-					      PTR_ERR(connector_state));
-				continue;
-			}
+retry:
+	crtc_state = drm_atomic_get_crtc_state(state, crtc);
+	ret = PTR_ERR_OR_ZERO(crtc_state);
+	if (!ret) {
+		if (!crtc_state->active)
+			goto out;
 
-			connector_state->crtc = crtc;
-		}
+		crtc_state->mode_changed = true;
+		ret = intel_set_mode(state);
 	}
 
-	crtc_state = intel_atomic_get_crtc_state(state, to_intel_crtc(crtc));
-	if (IS_ERR(crtc_state)) {
-		DRM_DEBUG_KMS("Failed to add [CRTC:%d] to state: %ld\n",
-			      crtc->base.id, PTR_ERR(crtc_state));
-		drm_atomic_state_free(state);
-		return;
+	if (ret == -EDEADLK) {
+		drm_atomic_state_clear(state);
+		drm_modeset_backoff(state->acquire_ctx);
+		goto retry;
 	}
 
-	drm_mode_copy(&crtc_state->base.mode, &crtc->mode);
-
-	intel_modeset_setup_plane_state(state, crtc, &crtc->mode,
-					crtc->primary->fb, crtc->x, crtc->y);
-
-	ret = intel_set_mode(state);
 	if (ret)
+out:
 		drm_atomic_state_free(state);
 }
 

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

[PATCH v3 16/20] drm/i915: Make intel_display_suspend atomic, try 2.

[Maarten Lankhorst]

Calculate all state using a normal transition, but afterwards fudge
crtc->state->active back to its old value. This should still allow
state restore in setup_hw_state to work properly.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 52 +++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-
 2 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 599af76d34f6..6e3df10a43b9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6224,12 +6224,58 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
  * turn all crtc's off, but do not adjust state
  * This has to be paired with a call to intel_modeset_setup_hw_state.
  */
-void intel_display_suspend(struct drm_device *dev)
+int intel_display_suspend(struct drm_device *dev)
 {
+	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx;
+	struct drm_atomic_state *state;
 	struct drm_crtc *crtc;
+	unsigned crtc_mask = 0;
+	int ret = 0;
+
+	if (WARN_ON(!ctx))
+		return 0;
+
+	lockdep_assert_held(&ctx->ww_ctx);
+	state = drm_atomic_state_alloc(dev);
+	if (WARN_ON(!state))
+		return -ENOMEM;
+
+	state->acquire_ctx = ctx;
+	state->allow_modeset = true;
+
+	for_each_crtc(dev, crtc) {
+		struct drm_crtc_state *crtc_state =
+			drm_atomic_get_crtc_state(state, crtc);
 
-	for_each_crtc(dev, crtc)
-		intel_crtc_disable_noatomic(crtc);
+		ret = PTR_ERR_OR_ZERO(crtc_state);
+		if (ret)
+			goto free;
+
+		if (!crtc_state->active)
+			continue;
+
+		crtc_state->active = false;
+		crtc_mask |= 1 << drm_crtc_index(crtc);
+	}
+
+	if (crtc_mask) {
+		ret = intel_set_mode(state);
+
+		if (!ret) {
+			for_each_crtc(dev, crtc)
+				if (crtc_mask & (1 << drm_crtc_index(crtc)))
+					crtc->state->active = true;
+
+			return ret;
+		}
+	}
+
+free:
+	if (ret)
+		DRM_ERROR("Suspending crtc's failed with %i\n", ret);
+	drm_atomic_state_free(state);
+	return ret;
 }
 
 /* Master function to enable/disable CRTC and corresponding power wells */
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 217b773e0673..f4abce103221 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -988,7 +988,7 @@ int intel_pch_rawclk(struct drm_device *dev);
 void intel_mark_busy(struct drm_device *dev);
 void intel_mark_idle(struct drm_device *dev);
 void intel_crtc_restore_mode(struct drm_crtc *crtc);
-void intel_display_suspend(struct drm_device *dev);
+int intel_display_suspend(struct drm_device *dev);
 int intel_crtc_control(struct drm_crtc *crtc, bool enable);
 void intel_crtc_update_dpms(struct drm_crtc *crtc);
 void intel_encoder_destroy(struct drm_encoder *encoder);
-- 
2.1.0

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

Re: [PATCH v3 16/20] drm/i915: Make intel_display_suspend atomic, try 2.

[Daniel Vetter]

On Mon, Jul 13, 2015 at 04:30:29PM +0200, Maarten Lankhorst wrote:
> Calculate all state using a normal transition, but afterwards fudge
> crtc->state->active back to its old value. This should still allow
> state restore in setup_hw_state to work properly.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Ok merged up to this patch with the exception of patch 8. I think
reworking that logic to use mode->private_flags to make sure we do a
modeset when userspace changes the mode, but not earlier is a sound
approach which will address all my concerns.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 52 +++++++++++++++++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_drv.h     |  2 +-
>  2 files changed, 50 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 599af76d34f6..6e3df10a43b9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6224,12 +6224,58 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
>   * turn all crtc's off, but do not adjust state
>   * This has to be paired with a call to intel_modeset_setup_hw_state.
>   */
> -void intel_display_suspend(struct drm_device *dev)
> +int intel_display_suspend(struct drm_device *dev)
>  {
> +	struct drm_mode_config *config = &dev->mode_config;
> +	struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx;
> +	struct drm_atomic_state *state;
>  	struct drm_crtc *crtc;
> +	unsigned crtc_mask = 0;
> +	int ret = 0;
> +
> +	if (WARN_ON(!ctx))
> +		return 0;
> +
> +	lockdep_assert_held(&ctx->ww_ctx);
> +	state = drm_atomic_state_alloc(dev);
> +	if (WARN_ON(!state))
> +		return -ENOMEM;
> +
> +	state->acquire_ctx = ctx;
> +	state->allow_modeset = true;
> +
> +	for_each_crtc(dev, crtc) {
> +		struct drm_crtc_state *crtc_state =
> +			drm_atomic_get_crtc_state(state, crtc);
>  
> -	for_each_crtc(dev, crtc)
> -		intel_crtc_disable_noatomic(crtc);
> +		ret = PTR_ERR_OR_ZERO(crtc_state);
> +		if (ret)
> +			goto free;
> +
> +		if (!crtc_state->active)
> +			continue;
> +
> +		crtc_state->active = false;
> +		crtc_mask |= 1 << drm_crtc_index(crtc);
> +	}
> +
> +	if (crtc_mask) {
> +		ret = intel_set_mode(state);
> +
> +		if (!ret) {
> +			for_each_crtc(dev, crtc)
> +				if (crtc_mask & (1 << drm_crtc_index(crtc)))
> +					crtc->state->active = true;
> +
> +			return ret;
> +		}
> +	}
> +
> +free:
> +	if (ret)
> +		DRM_ERROR("Suspending crtc's failed with %i\n", ret);
> +	drm_atomic_state_free(state);
> +	return ret;
>  }
>  
>  /* Master function to enable/disable CRTC and corresponding power wells */
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 217b773e0673..f4abce103221 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -988,7 +988,7 @@ int intel_pch_rawclk(struct drm_device *dev);
>  void intel_mark_busy(struct drm_device *dev);
>  void intel_mark_idle(struct drm_device *dev);
>  void intel_crtc_restore_mode(struct drm_crtc *crtc);
> -void intel_display_suspend(struct drm_device *dev);
> +int intel_display_suspend(struct drm_device *dev);
>  int intel_crtc_control(struct drm_crtc *crtc, bool enable);
>  void intel_crtc_update_dpms(struct drm_crtc *crtc);
>  void intel_encoder_destroy(struct drm_encoder *encoder);
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 16/20] drm/i915: Make intel_display_suspend atomic, try 2.

[Maarten Lankhorst]

Hey,

Op 14-07-15 om 18:14 schreef Daniel Vetter:
> On Mon, Jul 13, 2015 at 04:30:29PM +0200, Maarten Lankhorst wrote:
>> Calculate all state using a normal transition, but afterwards fudge
>> crtc->state->active back to its old value. This should still allow
>> state restore in setup_hw_state to work properly.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Ok merged up to this patch with the exception of patch 8. I think
> reworking that logic to use mode->private_flags to make sure we do a
> modeset when userspace changes the mode, but not earlier is a sound
> approach which will address all my concerns.
Patch 8 doesn't affect whether a modeset will happen and is safe to merge. It just fills in some more mode flags.
What you just described was done in patch 11 you just merged. ;-)

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

[PATCH v3 17/20] drm/i915: Use full atomic modeset.

[Maarten Lankhorst]

Huzzah! \o/

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c      |   2 +-
 drivers/gpu/drm/i915/i915_drv.h      |   1 -
 drivers/gpu/drm/i915/i915_params.c   |   5 -
 drivers/gpu/drm/i915/intel_atomic.c  | 123 ---------------
 drivers/gpu/drm/i915/intel_display.c | 279 ++++++-----------------------------
 drivers/gpu/drm/i915/intel_drv.h     |   5 -
 6 files changed, 43 insertions(+), 372 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index db48aee7f140..f13ed1ef6641 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1731,7 +1731,7 @@ static int __init i915_init(void)
 	 * to the atomic ioctl and the atomic properties.  Only plane operations on
 	 * a single CRTC will actually work.
 	 */
-	if (i915.nuclear_pageflip)
+	if (driver.driver_features & DRIVER_MODESET)
 		driver.driver_features |= DRIVER_ATOMIC;
 
 	return drm_pci_init(&driver, &i915_pci_driver);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d3e14cb43924..3d02d0094132 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2612,7 +2612,6 @@ struct i915_params {
 	int use_mmio_flip;
 	int mmio_debug;
 	bool verbose_state_checks;
-	bool nuclear_pageflip;
 	int edp_vswing;
 };
 extern struct i915_params i915 __read_mostly;
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 7983fe48a654..5f4e7295295f 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -51,7 +51,6 @@ struct i915_params i915 __read_mostly = {
 	.use_mmio_flip = 0,
 	.mmio_debug = 0,
 	.verbose_state_checks = 1,
-	.nuclear_pageflip = 0,
 	.edp_vswing = 0,
 };
 
@@ -176,10 +175,6 @@ module_param_named(verbose_state_checks, i915.verbose_state_checks, bool, 0600);
 MODULE_PARM_DESC(verbose_state_checks,
 	"Enable verbose logs (ie. WARN_ON()) in case of unexpected hw state conditions.");
 
-module_param_named_unsafe(nuclear_pageflip, i915.nuclear_pageflip, bool, 0600);
-MODULE_PARM_DESC(nuclear_pageflip,
-		 "Force atomic modeset functionality; only planes work for now (default: false).");
-
 /* WA to get away with the default setting in VBT for early platforms.Will be removed */
 module_param_named_unsafe(edp_vswing, i915.edp_vswing, int, 0400);
 MODULE_PARM_DESC(edp_vswing,
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index dcf4fb458649..e2531cf59266 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -35,129 +35,6 @@
 #include <drm/drm_plane_helper.h>
 #include "intel_drv.h"
 
-
-/**
- * intel_atomic_check - validate state object
- * @dev: drm device
- * @state: state to validate
- */
-int intel_atomic_check(struct drm_device *dev,
-		       struct drm_atomic_state *state)
-{
-	int nplanes = dev->mode_config.num_total_plane;
-	int ncrtcs = dev->mode_config.num_crtc;
-	int nconnectors = dev->mode_config.num_connector;
-	enum pipe nuclear_pipe = INVALID_PIPE;
-	struct intel_crtc *nuclear_crtc = NULL;
-	struct intel_crtc_state *crtc_state = NULL;
-	int ret;
-	int i;
-	bool not_nuclear = false;
-
-	to_intel_atomic_state(state)->cdclk = to_i915(dev)->cdclk_freq;
-
-	/*
-	 * FIXME:  At the moment, we only support "nuclear pageflip" on a
-	 * single CRTC.  Cross-crtc updates will be added later.
-	 */
-	for (i = 0; i < nplanes; i++) {
-		struct intel_plane *plane = to_intel_plane(state->planes[i]);
-		if (!plane)
-			continue;
-
-		if (nuclear_pipe == INVALID_PIPE) {
-			nuclear_pipe = plane->pipe;
-		} else if (nuclear_pipe != plane->pipe) {
-			DRM_DEBUG_KMS("i915 only support atomic plane operations on a single CRTC at the moment\n");
-			return -EINVAL;
-		}
-	}
-
-	/*
-	 * FIXME:  We only handle planes for now; make sure there are no CRTC's
-	 * or connectors involved.
-	 */
-	state->allow_modeset = false;
-	for (i = 0; i < ncrtcs; i++) {
-		struct intel_crtc *crtc = to_intel_crtc(state->crtcs[i]);
-		if (crtc)
-			memset(&crtc->atomic, 0, sizeof(crtc->atomic));
-		if (crtc && crtc->pipe != nuclear_pipe)
-			not_nuclear = true;
-		if (crtc && crtc->pipe == nuclear_pipe) {
-			nuclear_crtc = crtc;
-			crtc_state = to_intel_crtc_state(state->crtc_states[i]);
-		}
-	}
-	for (i = 0; i < nconnectors; i++)
-		if (state->connectors[i] != NULL)
-			not_nuclear = true;
-
-	if (not_nuclear) {
-		DRM_DEBUG_KMS("i915 only supports atomic plane operations at the moment\n");
-		return -EINVAL;
-	}
-
-	ret = drm_atomic_helper_check_planes(dev, state);
-	if (ret)
-		return ret;
-
-	return ret;
-}
-
-
-/**
- * intel_atomic_commit - commit validated state object
- * @dev: DRM device
- * @state: the top-level driver state object
- * @async: asynchronous commit
- *
- * This function commits a top-level state object that has been validated
- * with drm_atomic_helper_check().
- *
- * FIXME:  Atomic modeset support for i915 is not yet complete.  At the moment
- * we can only handle plane-related operations and do not yet support
- * asynchronous commit.
- *
- * RETURNS
- * Zero for success or -errno.
- */
-int intel_atomic_commit(struct drm_device *dev,
-			struct drm_atomic_state *state,
-			bool async)
-{
-	struct drm_crtc_state *crtc_state;
-	struct drm_crtc *crtc;
-	int ret;
-	int i;
-
-	if (async) {
-		DRM_DEBUG_KMS("i915 does not yet support async commit\n");
-		return -EINVAL;
-	}
-
-	ret = drm_atomic_helper_prepare_planes(dev, state);
-	if (ret)
-		return ret;
-
-	/* Point of no return */
-	drm_atomic_helper_swap_state(dev, state);
-
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state);
-
-		drm_atomic_helper_commit_planes_on_crtc(crtc_state);
-	}
-
-	/* FIXME: This function should eventually call __intel_set_mode when needed */
-
-	drm_atomic_helper_wait_for_vblanks(dev, state);
-	drm_atomic_helper_cleanup_planes(dev, state);
-	drm_atomic_state_free(state);
-
-	return 0;
-}
-
 /**
  * intel_connector_atomic_get_property - fetch connector property value
  * @connector: connector to fetch property for
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6e3df10a43b9..510e31f16135 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -86,7 +86,6 @@ static void i9xx_crtc_clock_get(struct intel_crtc *crtc,
 static void ironlake_pch_clock_get(struct intel_crtc *crtc,
 				   struct intel_crtc_state *pipe_config);
 
-static int intel_set_mode(struct drm_atomic_state *state);
 static int intel_framebuffer_init(struct drm_device *dev,
 				  struct intel_framebuffer *ifb,
 				  struct drm_mode_fb_cmd2 *mode_cmd,
@@ -111,14 +110,6 @@ static int i9xx_get_refclk(const struct intel_crtc_state *crtc_state,
 			   int num_connectors);
 static void intel_modeset_setup_hw_state(struct drm_device *dev);
 
-static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe)
-{
-	if (!connector->mst_port)
-		return connector->encoder;
-	else
-		return &connector->mst_port->mst_encoders[pipe]->base;
-}
-
 typedef struct {
 	int	min, max;
 } intel_range_t;
@@ -6260,7 +6251,7 @@ int intel_display_suspend(struct drm_device *dev)
 	}
 
 	if (crtc_mask) {
-		ret = intel_set_mode(state);
+		ret = drm_atomic_commit(state);
 
 		if (!ret) {
 			for_each_crtc(dev, crtc)
@@ -6314,7 +6305,7 @@ int intel_crtc_control(struct drm_crtc *crtc, bool enable)
 	}
 	pipe_config->base.active = enable;
 
-	ret = intel_set_mode(state);
+	ret = drm_atomic_commit(state);
 	if (!ret)
 		return ret;
 
@@ -10431,7 +10422,7 @@ retry:
 
 	drm_mode_copy(&crtc_state->base.mode, mode);
 
-	if (intel_set_mode(state)) {
+	if (drm_atomic_commit(state)) {
 		DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
 		if (old->release_fb)
 			old->release_fb->funcs->destroy(old->release_fb);
@@ -10499,7 +10490,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
 		if (ret)
 			goto fail;
 
-		ret = intel_set_mode(state);
+		ret = drm_atomic_commit(state);
 		if (ret)
 			goto fail;
 
@@ -13120,7 +13111,6 @@ static int intel_modeset_all_pipes(struct drm_atomic_state *state)
 }
 
 
-/* Code that should eventually be part of atomic_check() */
 static int intel_modeset_checks(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
@@ -13161,15 +13151,20 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
 	return 0;
 }
 
-static int
-intel_modeset_compute_config(struct drm_atomic_state *state)
+/**
+ * intel_atomic_check - validate state object
+ * @dev: drm device
+ * @state: state to validate
+ */
+static int intel_atomic_check(struct drm_device *dev,
+			      struct drm_atomic_state *state)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
 	int ret, i;
 	bool any_ms = false;
 
-	ret = drm_atomic_helper_check_modeset(state->dev, state);
+	ret = drm_atomic_helper_check_modeset(dev, state);
 	if (ret)
 		return ret;
 
@@ -13231,9 +13226,26 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
 	return drm_atomic_helper_check_planes(state->dev, state);
 }
 
-static int __intel_set_mode(struct drm_atomic_state *state)
+/**
+ * intel_atomic_commit - commit validated state object
+ * @dev: DRM device
+ * @state: the top-level driver state object
+ * @async: asynchronous commit
+ *
+ * This function commits a top-level state object that has been validated
+ * with drm_atomic_helper_check().
+ *
+ * FIXME:  Atomic modeset support for i915 is not yet complete.  At the moment
+ * we can only handle plane-related operations and do not yet support
+ * asynchronous commit.
+ *
+ * RETURNS
+ * Zero for success or -errno.
+ */
+static int intel_atomic_commit(struct drm_device *dev,
+			       struct drm_atomic_state *state,
+			       bool async)
 {
-	struct drm_device *dev = state->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
@@ -13241,6 +13253,11 @@ static int __intel_set_mode(struct drm_atomic_state *state)
 	int i;
 	bool any_ms = false;
 
+	if (async) {
+		DRM_DEBUG_KMS("i915 does not yet support async commit\n");
+		return -EINVAL;
+	}
+
 	ret = drm_atomic_helper_prepare_planes(dev, state);
 	if (ret)
 		return ret;
@@ -13285,34 +13302,14 @@ static int __intel_set_mode(struct drm_atomic_state *state)
 
 	/* FIXME: add subpixel order */
 
+	drm_atomic_helper_wait_for_vblanks(dev, state);
 	drm_atomic_helper_cleanup_planes(dev, state);
-
 	drm_atomic_state_free(state);
 
-	return 0;
-}
-
-static int intel_set_mode_checked(struct drm_atomic_state *state)
-{
-	struct drm_device *dev = state->dev;
-	int ret;
-
-	ret = __intel_set_mode(state);
-	if (ret == 0)
+	if (any_ms)
 		intel_modeset_check_state(dev);
 
-	return ret;
-}
-
-static int intel_set_mode(struct drm_atomic_state *state)
-{
-	int ret;
-
-	ret = intel_modeset_compute_config(state);
-	if (ret)
-		return ret;
-
-	return intel_set_mode_checked(state);
+	return 0;
 }
 
 void intel_crtc_restore_mode(struct drm_crtc *crtc)
@@ -13339,7 +13336,7 @@ retry:
 			goto out;
 
 		crtc_state->mode_changed = true;
-		ret = intel_set_mode(state);
+		ret = drm_atomic_commit(state);
 	}
 
 	if (ret == -EDEADLK) {
@@ -13355,201 +13352,9 @@ out:
 
 #undef for_each_intel_crtc_masked
 
-static bool intel_connector_in_mode_set(struct intel_connector *connector,
-					struct drm_mode_set *set)
-{
-	int ro;
-
-	for (ro = 0; ro < set->num_connectors; ro++)
-		if (set->connectors[ro] == &connector->base)
-			return true;
-
-	return false;
-}
-
-static int
-intel_modeset_stage_output_state(struct drm_device *dev,
-				 struct drm_mode_set *set,
-				 struct drm_atomic_state *state)
-{
-	struct intel_connector *connector;
-	struct drm_connector *drm_connector;
-	struct drm_connector_state *connector_state;
-	struct drm_crtc *crtc;
-	struct drm_crtc_state *crtc_state;
-	int i, ret;
-
-	/* The upper layers ensure that we either disable a crtc or have a list
-	 * of connectors. For paranoia, double-check this. */
-	WARN_ON(!set->fb && (set->num_connectors != 0));
-	WARN_ON(set->fb && (set->num_connectors == 0));
-
-	for_each_intel_connector(dev, connector) {
-		bool in_mode_set = intel_connector_in_mode_set(connector, set);
-
-		if (!in_mode_set && connector->base.state->crtc != set->crtc)
-			continue;
-
-		connector_state =
-			drm_atomic_get_connector_state(state, &connector->base);
-		if (IS_ERR(connector_state))
-			return PTR_ERR(connector_state);
-
-		if (in_mode_set) {
-			int pipe = to_intel_crtc(set->crtc)->pipe;
-			connector_state->best_encoder =
-				&intel_find_encoder(connector, pipe)->base;
-		}
-
-		if (connector->base.state->crtc != set->crtc)
-			continue;
-
-		/* If we disable the crtc, disable all its connectors. Also, if
-		 * the connector is on the changing crtc but not on the new
-		 * connector list, disable it. */
-		if (!set->fb || !in_mode_set) {
-			connector_state->best_encoder = NULL;
-
-			DRM_DEBUG_KMS("[CONNECTOR:%d:%s] to [NOCRTC]\n",
-				connector->base.base.id,
-				connector->base.name);
-		}
-	}
-	/* connector->new_encoder is now updated for all connectors. */
-
-	for_each_connector_in_state(state, drm_connector, connector_state, i) {
-		connector = to_intel_connector(drm_connector);
-
-		if (!connector_state->best_encoder) {
-			ret = drm_atomic_set_crtc_for_connector(connector_state,
-								NULL);
-			if (ret)
-				return ret;
-
-			continue;
-		}
-
-		if (intel_connector_in_mode_set(connector, set)) {
-			struct drm_crtc *crtc = connector->base.state->crtc;
-
-			/* If this connector was in a previous crtc, add it
-			 * to the state. We might need to disable it. */
-			if (crtc) {
-				crtc_state =
-					drm_atomic_get_crtc_state(state, crtc);
-				if (IS_ERR(crtc_state))
-					return PTR_ERR(crtc_state);
-			}
-
-			ret = drm_atomic_set_crtc_for_connector(connector_state,
-								set->crtc);
-			if (ret)
-				return ret;
-		}
-
-		/* Make sure the new CRTC will work with the encoder */
-		if (!drm_encoder_crtc_ok(connector_state->best_encoder,
-					 connector_state->crtc)) {
-			return -EINVAL;
-		}
-
-		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] to [CRTC:%d]\n",
-			connector->base.base.id,
-			connector->base.name,
-			connector_state->crtc->base.id);
-
-		if (connector_state->best_encoder != &connector->encoder->base)
-			connector->encoder =
-				to_intel_encoder(connector_state->best_encoder);
-	}
-
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		bool has_connectors;
-
-		ret = drm_atomic_add_affected_connectors(state, crtc);
-		if (ret)
-			return ret;
-
-		has_connectors = !!drm_atomic_connectors_for_crtc(state, crtc);
-		if (has_connectors != crtc_state->enable)
-			crtc_state->enable =
-			crtc_state->active = has_connectors;
-	}
-
-	ret = intel_modeset_setup_plane_state(state, set->crtc, set->mode,
-					      set->fb, set->x, set->y);
-	if (ret)
-		return ret;
-
-	crtc_state = drm_atomic_get_crtc_state(state, set->crtc);
-	if (IS_ERR(crtc_state))
-		return PTR_ERR(crtc_state);
-
-	ret = drm_atomic_set_mode_for_crtc(crtc_state, set->mode);
-	if (ret)
-		return ret;
-
-	if (set->num_connectors)
-		crtc_state->active = true;
-
-	return 0;
-}
-
-static int intel_crtc_set_config(struct drm_mode_set *set)
-{
-	struct drm_device *dev;
-	struct drm_atomic_state *state = NULL;
-	int ret;
-
-	BUG_ON(!set);
-	BUG_ON(!set->crtc);
-	BUG_ON(!set->crtc->helper_private);
-
-	/* Enforce sane interface api - has been abused by the fb helper. */
-	BUG_ON(!set->mode && set->fb);
-	BUG_ON(set->fb && set->num_connectors == 0);
-
-	if (set->fb) {
-		DRM_DEBUG_KMS("[CRTC:%d] [FB:%d] #connectors=%d (x y) (%i %i)\n",
-				set->crtc->base.id, set->fb->base.id,
-				(int)set->num_connectors, set->x, set->y);
-	} else {
-		DRM_DEBUG_KMS("[CRTC:%d] [NOFB]\n", set->crtc->base.id);
-	}
-
-	dev = set->crtc->dev;
-
-	state = drm_atomic_state_alloc(dev);
-	if (!state)
-		return -ENOMEM;
-
-	state->acquire_ctx = dev->mode_config.acquire_ctx;
-
-	ret = intel_modeset_stage_output_state(dev, set, state);
-	if (ret)
-		goto out;
-
-	ret = intel_modeset_compute_config(state);
-	if (ret)
-		goto out;
-
-	intel_update_pipe_size(to_intel_crtc(set->crtc));
-
-	ret = intel_set_mode_checked(state);
-	if (ret) {
-		DRM_DEBUG_KMS("failed to set mode on [CRTC:%d], err = %d\n",
-			      set->crtc->base.id, ret);
-	}
-
-out:
-	if (ret)
-		drm_atomic_state_free(state);
-	return ret;
-}
-
 static const struct drm_crtc_funcs intel_crtc_funcs = {
 	.gamma_set = intel_crtc_gamma_set,
-	.set_config = intel_crtc_set_config,
+	.set_config = drm_atomic_helper_set_config,
 	.destroy = intel_crtc_destroy,
 	.page_flip = intel_crtc_page_flip,
 	.atomic_duplicate_state = intel_crtc_duplicate_state,
@@ -15636,7 +15441,7 @@ void intel_display_resume(struct drm_device *dev)
 	intel_modeset_setup_hw_state(dev);
 
 	i915_redisable_vga(dev);
-	ret = intel_set_mode(state);
+	ret = drm_atomic_commit(state);
 	if (!ret)
 		return;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f4abce103221..cc91ea370c99 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1393,11 +1393,6 @@ void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count);
 void intel_tv_init(struct drm_device *dev);
 
 /* intel_atomic.c */
-int intel_atomic_check(struct drm_device *dev,
-		       struct drm_atomic_state *state);
-int intel_atomic_commit(struct drm_device *dev,
-			struct drm_atomic_state *state,
-			bool async);
 int intel_connector_atomic_get_property(struct drm_connector *connector,
 					const struct drm_connector_state *state,
 					struct drm_property *property,
-- 
2.1.0

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

[PATCH v3 18/20] drm/i915: Call plane update functions directly from intel_atomic_commit.

[Maarten Lankhorst]

Now that there's only a single path for all atomic updates we can call
intel_(pre/post)_plane_update from intel_atomic_commit directly. This
makes the intention more clear.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 510e31f16135..fd41cfa92d3d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13292,12 +13292,19 @@ static int intel_atomic_commit(struct drm_device *dev,
 
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		if (needs_modeset(crtc->state) && crtc->state->active) {
+		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+		bool modeset = needs_modeset(crtc->state);
+
+		if (modeset && crtc->state->active) {
 			update_scanline_offset(to_intel_crtc(crtc));
 			dev_priv->display.crtc_enable(crtc);
 		}
 
+		if (!modeset)
+			intel_pre_plane_update(intel_crtc);
+
 		drm_atomic_helper_commit_planes_on_crtc(crtc_state);
+		intel_post_plane_update(intel_crtc);
 	}
 
 	/* FIXME: add subpixel order */
@@ -13635,9 +13642,6 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
-	if (!needs_modeset(crtc->state))
-		intel_pre_plane_update(intel_crtc);
-
 	if (intel_crtc->atomic.update_wm_pre)
 		intel_update_watermarks(crtc);
 
@@ -13664,8 +13668,6 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
 				      intel_crtc->atomic.start_vbl_count);
 
 	intel_runtime_pm_put(dev_priv);
-
-	intel_post_plane_update(intel_crtc);
 }
 
 /**
-- 
2.1.0

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

[PATCH v3 19/20] drm/i915: always disable irqs in intel_pipe_update_start

[Maarten Lankhorst]

This can only fail because of a bug in the code.

Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 15 +++++----------
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-
 drivers/gpu/drm/i915/intel_sprite.c  | 17 +++++++----------
 3 files changed, 13 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fd41cfa92d3d..cc8ae7601884 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11248,12 +11248,11 @@ static void ilk_do_mmio_flip(struct intel_crtc *intel_crtc)
 static void intel_do_mmio_flip(struct intel_crtc *intel_crtc)
 {
 	struct drm_device *dev = intel_crtc->base.dev;
-	bool atomic_update;
 	u32 start_vbl_count;
 
 	intel_mark_page_flip_active(intel_crtc);
 
-	atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count);
+	intel_pipe_update_start(intel_crtc, &start_vbl_count);
 
 	if (INTEL_INFO(dev)->gen >= 9)
 		skl_do_mmio_flip(intel_crtc);
@@ -11261,8 +11260,7 @@ static void intel_do_mmio_flip(struct intel_crtc *intel_crtc)
 		/* use_mmio_flip() retricts MMIO flips to ilk+ */
 		ilk_do_mmio_flip(intel_crtc);
 
-	if (atomic_update)
-		intel_pipe_update_end(intel_crtc, start_vbl_count);
+	intel_pipe_update_end(intel_crtc, start_vbl_count);
 }
 
 static void intel_mmio_flip_work_func(struct work_struct *work)
@@ -13649,9 +13647,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
 
 	/* Perform vblank evasion around commit operation */
 	if (crtc->state->active)
-		intel_crtc->atomic.evade =
-			intel_pipe_update_start(intel_crtc,
-						&intel_crtc->atomic.start_vbl_count);
+		intel_pipe_update_start(intel_crtc, &intel_crtc->atomic.start_vbl_count);
 
 	if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
 		skl_detach_scalers(intel_crtc);
@@ -13663,9 +13659,8 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
-	if (intel_crtc->atomic.evade)
-		intel_pipe_update_end(intel_crtc,
-				      intel_crtc->atomic.start_vbl_count);
+	if (crtc->state->active)
+		intel_pipe_update_end(intel_crtc, intel_crtc->atomic.start_vbl_count);
 
 	intel_runtime_pm_put(dev_priv);
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index cc91ea370c99..250ee28baff9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1385,7 +1385,7 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob);
 int intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane);
 int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
 			      struct drm_file *file_priv);
-bool intel_pipe_update_start(struct intel_crtc *crtc,
+void intel_pipe_update_start(struct intel_crtc *crtc,
 			     uint32_t *start_vbl_count);
 void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count);
 
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index cd21525df352..9d8af2f8a875 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -75,10 +75,8 @@ static int usecs_to_scanlines(const struct drm_display_mode *mode, int usecs)
  * until a subsequent call to intel_pipe_update_end(). That is done to
  * avoid random delays. The value written to @start_vbl_count should be
  * supplied to intel_pipe_update_end() for error checking.
- *
- * Return: true if the call was successful
  */
-bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count)
+void intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count)
 {
 	struct drm_device *dev = crtc->base.dev;
 	const struct drm_display_mode *mode = &crtc->config->base.adjusted_mode;
@@ -96,13 +94,14 @@ bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count)
 	min = vblank_start - usecs_to_scanlines(mode, 100);
 	max = vblank_start - 1;
 
+	local_irq_disable();
+	*start_vbl_count = 0;
+
 	if (min <= 0 || max <= 0)
-		return false;
+		return;
 
 	if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
-		return false;
-
-	local_irq_disable();
+		return;
 
 	trace_i915_pipe_update_start(crtc, min, max);
 
@@ -138,8 +137,6 @@ bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count)
 	*start_vbl_count = dev->driver->get_vblank_counter(dev, pipe);
 
 	trace_i915_pipe_update_vblank_evaded(crtc, min, max, *start_vbl_count);
-
-	return true;
 }
 
 /**
@@ -161,7 +158,7 @@ void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count)
 
 	local_irq_enable();
 
-	if (start_vbl_count != end_vbl_count)
+	if (start_vbl_count && start_vbl_count != end_vbl_count)
 		DRM_ERROR("Atomic update failure on pipe %c (start=%u end=%u)\n",
 			  pipe_name(pipe), start_vbl_count, end_vbl_count);
 }
-- 
2.1.0

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

Re: [PATCH v3 19/20] drm/i915: always disable irqs in intel_pipe_update_start

[Daniel Stone]

Hi,

On 13 July 2015 at 15:30, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
> @@ -13649,9 +13647,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
>
>         /* Perform vblank evasion around commit operation */
>         if (crtc->state->active)
> -               intel_crtc->atomic.evade =
> -                       intel_pipe_update_start(intel_crtc,
> -                                               &intel_crtc->atomic.start_vbl_count);
> +               intel_pipe_update_start(intel_crtc, &intel_crtc->atomic.start_vbl_count);
>
>         if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
>                 skl_detach_scalers(intel_crtc);
> @@ -13663,9 +13659,8 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>
> -       if (intel_crtc->atomic.evade)
> -               intel_pipe_update_end(intel_crtc,
> -                                     intel_crtc->atomic.start_vbl_count);

Can we get rid of the 'evade' member in the struct now?

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

Re: [PATCH v3 19/20] drm/i915: always disable irqs in intel_pipe_update_start

[Maarten Lankhorst]

Op 13-07-15 om 19:16 schreef Daniel Stone:
> Hi,
>
> On 13 July 2015 at 15:30, Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com> wrote:
>> @@ -13649,9 +13647,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
>>
>>         /* Perform vblank evasion around commit operation */
>>         if (crtc->state->active)
>> -               intel_crtc->atomic.evade =
>> -                       intel_pipe_update_start(intel_crtc,
>> -                                               &intel_crtc->atomic.start_vbl_count);
>> +               intel_pipe_update_start(intel_crtc, &intel_crtc->atomic.start_vbl_count);
>>
>>         if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
>>                 skl_detach_scalers(intel_crtc);
>> @@ -13663,9 +13659,8 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
>>         struct drm_i915_private *dev_priv = dev->dev_private;
>>         struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>
>> -       if (intel_crtc->atomic.evade)
>> -               intel_pipe_update_end(intel_crtc,
>> -                                     intel_crtc->atomic.start_vbl_count);
> Can we get rid of the 'evade' member in the struct now?
>
> Cheers,
> Daniel
Yeah, it's useless now.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 19/20] drm/i915: always disable irqs in intel_pipe_update_start

[Maarten Lankhorst]

Op 13-07-15 om 19:16 schreef Daniel Stone:
> Hi,
>
> On 13 July 2015 at 15:30, Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com> wrote:
>> @@ -13649,9 +13647,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
>>
>>         /* Perform vblank evasion around commit operation */
>>         if (crtc->state->active)
>> -               intel_crtc->atomic.evade =
>> -                       intel_pipe_update_start(intel_crtc,
>> -                                               &intel_crtc->atomic.start_vbl_count);
>> +               intel_pipe_update_start(intel_crtc, &intel_crtc->atomic.start_vbl_count);
>>
>>         if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
>>                 skl_detach_scalers(intel_crtc);
>> @@ -13663,9 +13659,8 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
>>         struct drm_i915_private *dev_priv = dev->dev_private;
>>         struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>
>> -       if (intel_crtc->atomic.evade)
>> -               intel_pipe_update_end(intel_crtc,
>> -                                     intel_crtc->atomic.start_vbl_count);
> Can we get rid of the 'evade' member in the struct now?
>
> Cheers,
> Daniel

What about merging with the below commit?
------>8-----
drm/i915: remove start_vbl_count from intel_crtc_atomic_commit

This is not precalculated, and should just be part of the crtc struct.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 39729592d1b6..ede652867596 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13647,7 +13647,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
 
 	/* Perform vblank evasion around commit operation */
 	if (crtc->state->active)
-		intel_pipe_update_start(intel_crtc, &intel_crtc->atomic.start_vbl_count);
+		intel_pipe_update_start(intel_crtc, &intel_crtc->start_vbl_count);
 
 	if (!needs_modeset(crtc->state) && INTEL_INFO(dev)->gen >= 9)
 		skl_detach_scalers(intel_crtc);
@@ -13658,7 +13658,7 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc)
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
 	if (crtc->state->active)
-		intel_pipe_update_end(intel_crtc, intel_crtc->atomic.start_vbl_count);
+		intel_pipe_update_end(intel_crtc, intel_crtc->start_vbl_count);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 250ee28baff9..0fcfa7f179c4 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -488,10 +488,6 @@ struct skl_pipe_wm {
  * and thus can't be run with interrupts disabled.
  */
 struct intel_crtc_atomic_commit {
-	/* vblank evasion */
-	bool evade;
-	unsigned start_vbl_count;
-
 	/* Sleepable operations to perform before commit */
 	bool wait_for_flips;
 	bool disable_fbc;
@@ -559,6 +555,7 @@ struct intel_crtc {
 
 	int scanline_offset;
 
+	unsigned start_vbl_count;
 	struct intel_crtc_atomic_commit atomic;
 
 	/* scalers available on this crtc */

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

[PATCH v3 20/20] drm/i915: Remove use of runtime pm in atomic commit functions

[Maarten Lankhorst]

We needed this originally for updating pagetables in plane commit
functions. But that's extracted into prepare/cleanup now. The other
issue was running updates when the pipe was off. That's also now
fixed.

Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index cc8ae7601884..9bf67e34961c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13637,14 +13637,11 @@ intel_disable_primary_plane(struct drm_plane *plane,
 static void intel_begin_crtc_commit(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
 	if (intel_crtc->atomic.update_wm_pre)
 		intel_update_watermarks(crtc);
 
-	intel_runtime_pm_get(dev_priv);
-
 	/* Perform vblank evasion around commit operation */
 	if (crtc->state->active)
 		intel_pipe_update_start(intel_crtc, &intel_crtc->atomic.start_vbl_count);
@@ -13655,14 +13652,10 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc)
 
 static void intel_finish_crtc_commit(struct drm_crtc *crtc)
 {
-	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
 	if (crtc->state->active)
 		intel_pipe_update_end(intel_crtc, intel_crtc->atomic.start_vbl_count);
-
-	intel_runtime_pm_put(dev_priv);
 }
 
 /**
-- 
2.1.0

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

Re: [PATCH v3 00/20] Convert to atomic, part 4.

[Daniel Stone]

[-- Attachment #1.1: Type: text/plain, Size: 722 bytes --]

Hi,

On Monday, July 13, 2015, Maarten Lankhorst <
maarten.lankhorst@linux.intel.com> wrote:

> Atomic suspend/resume, full hardware readout and atomic ioctl support.
>
> Changes from the previous version:
> - The fastboot changes from the previous patch have been removed,
>   fastboot will have to be a separate patch because of the testing it
> needs.
> - I've cleaned up the changes to planes and split it into separate patches.
>   This makes it easier to bisect.
> - Some commit logs have been updated.
>

Reviews sent to 6, 10 and 19 separately, but the rest are:
Reviewed-by: Daniel Stone <daniels@collabora.com>

The others are trivial enough that you can slap my R-b on after making the
changes.

Cheers,
Daniel

[-- Attachment #1.2: Type: text/html, Size: 1090 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH v3 00/20] Convert to atomic, part 4.

[Daniel Stone]

[-- Attachment #1.1: Type: text/plain, Size: 722 bytes --]

Hi,

On Monday, July 13, 2015, Maarten Lankhorst <
maarten.lankhorst@linux.intel.com> wrote:

> Atomic suspend/resume, full hardware readout and atomic ioctl support.
>
> Changes from the previous version:
> - The fastboot changes from the previous patch have been removed,
>   fastboot will have to be a separate patch because of the testing it
> needs.
> - I've cleaned up the changes to planes and split it into separate patches.
>   This makes it easier to bisect.
> - Some commit logs have been updated.
>

Reviews sent to 6, 10 and 19 separately, but the rest are:
Reviewed-by: Daniel Stone <daniels@collabora.com>

The others are trivial enough that you can slap my R-b on after making the
changes.

Cheers,
Daniel

[-- Attachment #1.2: Type: text/html, Size: 1090 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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