drm/i915: Convert to atomic, part 2.

This is useful when calculating vblank in drivers that support it. During a modeset the atomic state may not match the hardware state, so if the driver wants to wait on a vblank they'll want to use crtc->hwmode rather than crtc->state->adjusted_mode.

Signed-off-by Maarten Lankhorst <[email protected]>

Drivers may need to store the state of shared resources, such as PLLs or FIFO space, into the atomic state. Allow this by making it possible to subclass drm_atomic_state.

Changes since v1:
- Change member names for functions to atomic_state_(alloc,clear)
- Change __drm_atomic_state_new to drm_atomic_state_init
- Allow free function to be overridden too, in case extra memory is allocated in alloc.
Changes since v2:
- Rename *_default_free to default_release, to make clear it doesn't free the state object itself.

Cc [email protected]

Acked-by Ander Conselvan de Oliveira <[email protected]>

Signed-off-by Maarten Lankhorst <[email protected]>

drivers/gpu/drm/drm_atomic.c +88 -28

index c6277a4a1f2f..cd1b16b25716 100644
@@ -30,7 +30,15 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_plane_helper.h>
 
-static void kfree_state(struct drm_atomic_state *state)
+/**
+ * drm_atomic_state_default_release -
+ * release memory initialized by drm_atomic_state_init
+ * @state: atomic state
+ *
+ * Free all the memory allocated by drm_atomic_state_init.
+ * This is useful for drivers that subclass the atomic state.
+ */
+void drm_atomic_state_default_release(struct drm_atomic_state *state)
 {
 	kfree(state->connectors);
 	kfree(state->connector_states);

@@ -38,24 +46,20 @@ static void kfree_state(struct drm_atomic_state *state)
 	kfree(state->crtc_states);
 	kfree(state->planes);
 	kfree(state->plane_states);
-	kfree(state);
 }
+EXPORT_SYMBOL(drm_atomic_state_default_release);
 
 /**
- * drm_atomic_state_alloc - allocate atomic state
+ * drm_atomic_state_init - init new atomic state
  * @dev: DRM device
+ * @state: atomic state
  *
- * This allocates an empty atomic state to track updates.
+ * Default implementation for filling in a new atomic state.
+ * This is useful for drivers that subclass the atomic state.
  */
-struct drm_atomic_state *
-drm_atomic_state_alloc(struct drm_device *dev)
+int
+drm_atomic_state_init(struct drm_device *dev, struct drm_atomic_state *state)
 {
-	struct drm_atomic_state *state;
-
-	state = kzalloc(sizeof(*state), GFP_KERNEL);
-	if (!state)
-		return NULL;
-
 	/* TODO legacy paths should maybe do a better job about
 	 * setting this appropriately?
 	 */

@@ -92,31 +96,50 @@ drm_atomic_state_alloc(struct drm_device *dev)
 
 	state->dev = dev;
 
-	DRM_DEBUG_ATOMIC("Allocate atomic state %p\n", state);
+	DRM_DEBUG_ATOMIC("Allocated atomic state %p\n", state);
 
-	return state;
+	return 0;
 fail:
-	kfree_state(state);
+	drm_atomic_state_default_release(state);
+	return -ENOMEM;
+}
+EXPORT_SYMBOL(drm_atomic_state_init);
+
+/**
+ * drm_atomic_state_alloc - allocate atomic state
+ * @dev: DRM device
+ *
+ * This allocates an empty atomic state to track updates.
+ */
+struct drm_atomic_state *
+drm_atomic_state_alloc(struct drm_device *dev)
+{
+	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_atomic_state *state;
+
+	if (!config->funcs->atomic_state_alloc) {
+		state = kzalloc(sizeof(*state), GFP_KERNEL);
+		if (!state)
+			return NULL;
+		if (drm_atomic_state_init(dev, state) < 0) {
+			kfree(state);
+			return NULL;
+		}
+		return state;
+	}
 
-	return NULL;
+	return config->funcs->atomic_state_alloc(dev);
 }
 EXPORT_SYMBOL(drm_atomic_state_alloc);
 
 /**
- * drm_atomic_state_clear - clear state object
+ * drm_atomic_state_default_clear - clear base atomic state
  * @state: atomic state
  *
- * When the w/w mutex algorithm detects a deadlock we need to back off and drop
- * all locks. So someone else could sneak in and change the current modeset
- * configuration. Which means that all the state assembled in @state is no
- * longer an atomic update to the current state, but to some arbitrary earlier
- * state. Which could break assumptions the driver's ->atomic_check likely
- * relies on.
- *
- * Hence we must clear all cached state and completely start over, using this
- * function.
+ * Default implementation for clearing atomic state.
+ * This is useful for drivers that subclass the atomic state.
  */
-void drm_atomic_state_clear(struct drm_atomic_state *state)
+void drm_atomic_state_default_clear(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
 	struct drm_mode_config *config = &dev->mode_config;

@@ -162,6 +185,32 @@ void drm_atomic_state_clear(struct drm_atomic_state *state)
 		state->plane_states[i] = NULL;
 	}
 }
+EXPORT_SYMBOL(drm_atomic_state_default_clear);
+
+/**
+ * drm_atomic_state_clear - clear state object
+ * @state: atomic state
+ *
+ * When the w/w mutex algorithm detects a deadlock we need to back off and drop
+ * all locks. So someone else could sneak in and change the current modeset
+ * configuration. Which means that all the state assembled in @state is no
+ * longer an atomic update to the current state, but to some arbitrary earlier
+ * state. Which could break assumptions the driver's ->atomic_check likely
+ * relies on.
+ *
+ * Hence we must clear all cached state and completely start over, using this
+ * function.
+ */
+void drm_atomic_state_clear(struct drm_atomic_state *state)
+{
+	struct drm_device *dev = state->dev;
+	struct drm_mode_config *config = &dev->mode_config;
+
+	if (config->funcs->atomic_state_clear)
+		config->funcs->atomic_state_clear(state);
+	else
+		drm_atomic_state_default_clear(state);
+}
 EXPORT_SYMBOL(drm_atomic_state_clear);
 
 /**

@@ -173,14 +222,25 @@ EXPORT_SYMBOL(drm_atomic_state_clear);
  */
 void drm_atomic_state_free(struct drm_atomic_state *state)
 {
+	struct drm_device *dev;
+	struct drm_mode_config *config;
+
 	if (!state)
 		return;
 
+	dev = state->dev;
+	config = &dev->mode_config;
+
 	drm_atomic_state_clear(state);
 
 	DRM_DEBUG_ATOMIC("Freeing atomic state %p\n", state);
 
-	kfree_state(state);
+	if (config->funcs->atomic_state_free) {
+		config->funcs->atomic_state_free(state);
+	} else {
+		drm_atomic_state_default_release(state);
+		kfree(state);
+	}
 }
 EXPORT_SYMBOL(drm_atomic_state_free);
 

include/drm/drm_atomic.h +5 -0

index d78543067700..f0d3a7387d99 100644
@@ -35,6 +35,11 @@ drm_atomic_state_alloc(struct drm_device *dev);
 void drm_atomic_state_clear(struct drm_atomic_state *state);
 void drm_atomic_state_free(struct drm_atomic_state *state);
 
+int  __must_check
+drm_atomic_state_init(struct drm_device *dev, struct drm_atomic_state *state);
+void drm_atomic_state_default_clear(struct drm_atomic_state *state);
+void drm_atomic_state_default_release(struct drm_atomic_state *state);
+
 struct drm_crtc_state * __must_check
 drm_atomic_get_crtc_state(struct drm_atomic_state *state,
 			  struct drm_crtc *crtc);

include/drm/drm_crtc.h +6 -0

index 840e9e62878c..bff25b0cada9 100644
@@ -985,6 +985,9 @@ struct drm_mode_set {
  * @atomic_check: check whether a given atomic state update is possible
  * @atomic_commit: commit an atomic state update previously verified with
  * 	atomic_check()
+ * @atomic_state_alloc: allocate a new atomic state
+ * @atomic_state_clear: clear the atomic state
+ * @atomic_state_free: free the atomic state
  *
  * Some global (i.e. not per-CRTC, connector, etc) mode setting functions that
  * involve drivers.

@@ -1000,6 +1003,9 @@ struct drm_mode_config_funcs {
 	int (*atomic_commit)(struct drm_device *dev,
 			     struct drm_atomic_state *a,
 			     bool async);
+	struct drm_atomic_state *(*atomic_state_alloc)(struct drm_device *dev);
+	void (*atomic_state_clear)(struct drm_atomic_state *state);
+	void (*atomic_state_free)(struct drm_atomic_state *state);
 };
 
 /**

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

crtc_state->enable means a crtc is configured, but it may be turned off for dpms. Until the previous commit crtc_state->active was not updated on crtc off, but now that we do we should use that for tracking whether a crtc is scanning out or not.

At this point crtc->active should mirror crtc_state->active, so some paranoia from the crtc_disable functions can be removed.

Note that intel_set_mode_setup_plls still checks for ->enable, because all resources that are needed have to be calculated, so dpms changes will still succeed.

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/i915_irq.c      |  2 +-
 drivers/gpu/drm/i915/intel_display.c | 44 ++++++++++++++++++------------------
 2 files changed, 23 insertions(+), 23 deletions(-)

drivers/gpu/drm/i915/i915_irq.c +0 -0

index 557af8877a2e..ca457317a8ac 100644

drivers/gpu/drm/i915/i915_irq.c +0 -0

 		DRM_DEBUG_KMS("crtc %d is disabled\n", pipe);
 		return -EBUSY;
 	}

drivers/gpu/drm/i915/intel_display.c +5 -9

index db1b47febb27..583c9105cf49 100644
@@ -4667,7 +4667,7 @@ static void intel_crtc_load_lut(struct drm_crtc *crtc)
 	bool reenable_ips = false;
 
 	/* The clocks have to be on to load the palette. */
-	if (!crtc->state->enable || !intel_crtc->active)
+	if (!crtc->state->active)
 		return;
 
 	if (HAS_GMCH_DISPLAY(dev_priv->dev)) {

@@ -4872,9 +4872,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	struct intel_encoder *encoder;
 	int pipe = intel_crtc->pipe;
 
-	WARN_ON(!crtc->state->enable);
-
-	if (intel_crtc->active)
+	if (WARN_ON(intel_crtc->active))
 		return;
 
 	if (intel_crtc->config->has_pch_encoder)

@@ -4978,9 +4976,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	struct intel_encoder *encoder;
 	int pipe = intel_crtc->pipe;
 
-	WARN_ON(!crtc->state->enable);
-
-	if (intel_crtc->active)
+	if (WARN_ON(intel_crtc->active))
 		return;
 
 	if (intel_crtc_to_shared_dpll(intel_crtc))

@@ -5082,7 +5078,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 	int pipe = intel_crtc->pipe;
 	u32 reg, temp;
 
-	if (!intel_crtc->active)
+	if (WARN_ON(!intel_crtc->active))
 		return;
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)

@@ -5144,7 +5140,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 	struct intel_encoder *encoder;
 	enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
 
-	if (!intel_crtc->active)
+	if (WARN_ON(!intel_crtc->active))
 		return;
 
 	for_each_encoder_on_crtc(dev, crtc, encoder) {

drivers/gpu/drm/i915/intel_display.c +15 -11

 	return 0;

@@ -5840,9 +5836,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 	int pipe = intel_crtc->pipe;
 	bool is_dsi;
 
-	WARN_ON(!crtc->state->enable);
-
-	if (intel_crtc->active)
+	if (WARN_ON(intel_crtc->active))
 		return;
 
 	is_dsi = intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DSI);

@@ -5918,9 +5912,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 	struct intel_encoder *encoder;
 	int pipe = intel_crtc->pipe;
 
-	WARN_ON(!crtc->state->enable);
-
-	if (intel_crtc->active)
+	if (WARN_ON(intel_crtc->active))
 		return;
 
 	i9xx_set_pll_dividers(intel_crtc);

@@ -5980,7 +5972,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	struct intel_encoder *encoder;
 	int pipe = intel_crtc->pipe;
 
-	if (!intel_crtc->active)
+	if (WARN_ON(!intel_crtc->active))
 		return;
 
 	/*

@@ -11553,7 +11545,7 @@ intel_modeset_update_state(struct drm_atomic_state *state)
 		if (!crtc_state || !needs_modeset(crtc->state))
 			continue;
 
-		if (crtc->state->enable) {
+		if (crtc->state->active) {
 			struct drm_property *dpms_property =
 				dev->mode_config.dpms_property;
 

@@ -11970,6 +11962,10 @@ check_crtc_state(struct drm_device *dev)
 		     "crtc active state doesn't match with hw state "
 		     "(expected %i, found %i)\n", crtc->active, active);
 
+		I915_STATE_WARN(crtc->active != crtc->base.state->active,
+		     "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)) {
 			I915_STATE_WARN(1, "pipe state doesn't match!\n");

@@ -12115,6 +12111,10 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
 	if (IS_ERR(pipe_config))
 		return pipe_config;
 
+	if (!pipe_config->base.enable &&
+	    WARN_ON(pipe_config->base.active))
+		pipe_config->base.active = false;
+
 	if (!pipe_config->base.enable)
 		return pipe_config;
 

@@ -12239,7 +12239,7 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
 		return ret;
 
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		if (!needs_modeset(crtc_state))
+		if (!needs_modeset(crtc_state) || !crtc->state->active)
 			continue;
 
 		intel_crtc_disable_planes(crtc);

@@ -12261,7 +12261,7 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
 
 	/* 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->enable)
+		if (!needs_modeset(crtc->state) || !crtc->state->active)
 			continue;
 
 		update_scanline_offset(to_intel_crtc(crtc));

@@ -14494,7 +14494,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 	 * have active connectors/encoders. */
 	intel_crtc_update_dpms(&crtc->base);
 
-	if (crtc->active != crtc->base.state->enable) {
+	if (crtc->active != crtc->base.state->active) {
 		struct intel_encoder *encoder;
 
 		/* This can happen either due to bugs in the get_hw_state

Signature · 6 lines

2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 5 lines

crtc_state->enable means a crtc is configured, but it may be turned off for dpms. Until the previous commit crtc_state->active was not updated on crtc off, but now that we do we should use that for tracking whether a crtc is scanning out or not.

Quoted 8 lines

At this point crtc->active should mirror crtc_state->active, so some paranoia from the crtc_disable functions can be removed.

Note that intel_set_mode_setup_plls still checks for ->enable, because all resources that are needed have to be calculated, so dpms changes will still succeed.

Signed-off-by Maarten Lankhorst <[email protected]>

A few detail comments below.
-Daniel

Quoted 13 lines

---
 drivers/gpu/drm/i915/i915_irq.c      |  2 +-
 drivers/gpu/drm/i915/intel_display.c | 44 ++++++++++++++++++------------------
 2 files changed, 23 insertions(+), 23 deletions(-)

drivers/gpu/drm/i915/i915_irq.c +0 -0

index 557af8877a2e..ca457317a8ac 100644
@@ -796,7 +796,7 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
 		return -EINVAL;
 	}
 

drivers/gpu/drm/i915/i915_irq.c static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe, · line 799

Quoted 2 lines

drivers/gpu/drm/i915/i915_irq.c +1 -1

-	if (!crtc->state->enable) {
+	if (!crtc->state->active) {

This change looks unjustified I think.

drivers/gpu/drm/i915/i915_irq.c static int valleyview_modeset_global_pipes(struct drm_atomic_state *state) · line 5741

Quoted 7 lines

<snip>

drivers/gpu/drm/i915/i915_irq.c +1 -1

@@ -5742,7 +5738,7 @@ static int valleyview_modeset_global_pipes(struct drm_atomic_state *state)
 
 	/* add all active pipes to the state */
 	for_each_crtc(state->dev, crtc) {
-		if (!crtc->state->enable)
+		if (!crtc->state->active)

This is a functional change to the cdclk code and might break it and/or conflict with the ongoing cdclk work from Ville/Mika. Definitely needs to be split out.

drivers/gpu/drm/i915/i915_irq.c static int valleyview_modeset_global_pipes(struct drm_atomic_state *state) · line 5752

Quoted 10 lines

drivers/gpu/drm/i915/i915_irq.c +1 -1

 			continue;
 
 		crtc_state = drm_atomic_get_crtc_state(state, crtc);

@@ -5752,7 +5748,7 @@ static int valleyview_modeset_global_pipes(struct drm_atomic_state *state)
 
 	/* disable/enable all currently active pipes while we change cdclk */
 	for_each_crtc_in_state(state, crtc, crtc_state, i)
-		if (crtc_state->enable)
+		if (crtc_state->active)
 			crtc_state->mode_changed = true;

Same here.

Hm, aside of all that maybe we should drop vlv_modeset_global_pipes and instead just look at crtc_state->mode_changed? That way we don't need to duplicate the same checks twice, once to set ->mode_changed and once to for the prepare_pipes mask. Or is that duplication already getting removed?

Op 18-05-15 om 17:30 schreef Daniel Vetter:

Quoted 30 lines · Maarten Lankhorst

Quoted 5 lines

crtc_state->enable means a crtc is configured, but it may be turned off for dpms. Until the previous commit crtc_state->active was not updated on crtc off, but now that we do we should use that for tracking whether a crtc is scanning out or not.

Quoted 8 lines

At this point crtc->active should mirror crtc_state->active, so some paranoia from the crtc_disable functions can be removed.

Note that intel_set_mode_setup_plls still checks for ->enable, because all resources that are needed have to be calculated, so dpms changes will still succeed.

Signed-off-by Maarten Lankhorst <[email protected]>

A few detail comments below.
-Daniel

Quoted 13 lines

---
 drivers/gpu/drm/i915/i915_irq.c      |  2 +-
 drivers/gpu/drm/i915/intel_display.c | 44 ++++++++++++++++++------------------
 2 files changed, 23 insertions(+), 23 deletions(-)

drivers/gpu/drm/i915/i915_irq.c +0 -0

index 557af8877a2e..ca457317a8ac 100644
@@ -796,7 +796,7 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
 		return -EINVAL;
 	}
 

Quoted 3 lines

drivers/gpu/drm/i915/i915_irq.c static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe, · line 799

Quoted 2 lines

drivers/gpu/drm/i915/i915_irq.c +1 -1

-	if (!crtc->state->enable) {
+	if (!crtc->state->active) {

This change looks unjustified I think.

Why? Can you get a vblank on crtc that is dpms off? I think not..

Either way later on I use hwmode->crtc_clock which renders this moot.

Quoted 10 lines

drivers/gpu/drm/i915/i915_irq.c static int valleyview_modeset_global_pipes(struct drm_atomic_state *state) · line 5741

Quoted 7 lines

<snip>

drivers/gpu/drm/i915/i915_irq.c +1 -1

@@ -5742,7 +5738,7 @@ static int valleyview_modeset_global_pipes(struct drm_atomic_state *state)
 
 	/* add all active pipes to the state */
 	for_each_crtc(state->dev, crtc) {
-		if (!crtc->state->enable)
+		if (!crtc->state->active)

This is a functional change to the cdclk code and might break it and/or conflict with the ongoing cdclk work from Ville/Mika. Definitely needs to be split out.

No this function just sets mode_changed on all active crtc's.

This is done to turn them off while changing the clock. In case they're already turned off you don't have to turn them off.

Quoted 12 lines

drivers/gpu/drm/i915/i915_irq.c static int valleyview_modeset_global_pipes(struct drm_atomic_state *state) · line 5752

Quoted 10 lines

drivers/gpu/drm/i915/i915_irq.c +1 -1

 			continue;
 
 		crtc_state = drm_atomic_get_crtc_state(state, crtc);

@@ -5752,7 +5748,7 @@ static int valleyview_modeset_global_pipes(struct drm_atomic_state *state)
 
 	/* disable/enable all currently active pipes while we change cdclk */
 	for_each_crtc_in_state(state, crtc, crtc_state, i)
-		if (crtc_state->enable)
+		if (crtc_state->active)
 			crtc_state->mode_changed = true;

Same here.

Quoted 5 lines

Hm, aside of all that maybe we should drop vlv_modeset_global_pipes and instead just look at crtc_state->mode_changed? That way we don't need to duplicate the same checks twice, once to set ->mode_changed and once to for the prepare_pipes mask. Or is that duplication already getting removed?

I think we could get rid of a lot of those extra loops if we want to later, but for now readability is more important.

From: Ander Conselvan de Oliveira <[email protected]>

A follow up patch will make intel_modeset_compute_config() deal with multiple crtcs, so move crtc specific stuff into the lower level crtc specific function.

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 51 ++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 23 deletions(-)

drivers/gpu/drm/i915/intel_display.c +6 -0

index 583c9105cf49..ec548cbb06ee 100644
@@ -427,6 +427,12 @@ static void vlv_clock(int refclk, intel_clock_t *clock)
 	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
 }
 
+static bool
+needs_modeset(struct drm_crtc_state *state)
+{
+	return state->mode_changed || state->active_changed;
+}
+
 /**
  * Returns whether any output on the specified pipe is of the specified type
  */

@@ -11387,6 +11393,15 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
 		return -EINVAL;

drivers/gpu/drm/i915/intel_display.c +14 -23

+
 	clear_intel_crtc_state(pipe_config);
 
 	pipe_config->cpu_transcoder =

@@ -11478,6 +11493,18 @@ 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;
+
+	/*
+	 * 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.
+	 */
+
 	return 0;
 fail:
 	return ret;

@@ -11495,12 +11522,6 @@ static bool intel_crtc_in_use(struct drm_crtc *crtc)
 	return false;
 }
 
-static bool
-needs_modeset(struct drm_crtc_state *state)
-{
-	return state->mode_changed || state->active_changed;
-}
-
 static void
 intel_modeset_update_state(struct drm_atomic_state *state)
 {

@@ -12093,10 +12114,6 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
 	struct intel_crtc_state *pipe_config;
 	int ret = 0;
 
-	ret = drm_atomic_add_affected_connectors(state, crtc);
-	if (ret)
-		return ERR_PTR(ret);
-
 	ret = drm_atomic_helper_check_modeset(state->dev, state);
 	if (ret)
 		return ERR_PTR(ret);

@@ -12122,19 +12139,7 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
 	if (ret)
 		return ERR_PTR(ret);
 
-	/* Check things that can only be changed through modeset */
-	if (pipe_config->has_audio !=
-	    to_intel_crtc(crtc)->config->has_audio)
-		pipe_config->base.mode_changed = true;
-
-	/*
-	 * 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.
-	 */
-
-	intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,"[modeset]");
+	intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config, "[modeset]");
 
 	ret = drm_atomic_helper_check_planes(state->dev, state);
 	if (ret)

Signature · 6 lines

2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 30 lines

From: Ander Conselvan de Oliveira <[email protected]>

A follow up patch will make intel_modeset_compute_config() deal with multiple crtcs, so move crtc specific stuff into the lower level crtc specific function.

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 51 ++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 23 deletions(-)

drivers/gpu/drm/i915/intel_display.c +6 -0

index 583c9105cf49..ec548cbb06ee 100644
@@ -427,6 +427,12 @@ static void vlv_clock(int refclk, intel_clock_t *clock)
 	clock->dot = DIV_ROUND_CLOSEST(clock->vco, clock->p);
 }
 
+static bool
+needs_modeset(struct drm_crtc_state *state)
+{
+	return state->mode_changed || state->active_changed;
+}
+
 /**
  * Returns whether any output on the specified pipe is of the specified type
  */

@@ -11387,6 +11393,15 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
 		return -EINVAL;

drivers/gpu/drm/i915/intel_display.c intel_modeset_pipe_config(struct drm_crtc *crtc, · line 11403

Quoted 10 lines

drivers/gpu/drm/i915/intel_display.c +8 -0

 	}
 
+	/*
+	 * XXX: Add all connectors to make the crtc state match the encoders.
+	 */
+	if (!needs_modeset(&pipe_config->base)) {
+		ret = drm_atomic_add_affected_connectors(state, crtc);
+		if (ret)
+			return ret;
+	}

Comment aside: Eventually we need to invert this check and use intel_pipe_config_compare with a special mode to a) not scream into dmesg

b) ignore any changes we can fixup without a full modeset (i.e. pfit). And then use that to decide whether we need a modeset or not for the crtc. We still rely upon our internal set_mode functions to at least in some cases do an unconditional modeset on the passed-in crtc for property updates. But once converted to atomic we need to be more intelligent, especially since userspace expects no-op changes to get filtered out. So unconditionally doing a modeset wont cut it either (with the current code all the set_prop hooks have such checks hand-rolled).

From: Ander Conselvan de Oliveira <[email protected]>

The last remaining portion that required the modeset_crtc argument is converted to deal with all crtcs in the state that need_modeset(). By doing that, __intel_set_mode() is generic enough, behaving like a commit function for the atomic state.

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

drivers/gpu/drm/i915/intel_display.c +2 -4

index ec548cbb06ee..1c706623f99c 100644
@@ -12224,12 +12224,10 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state)
 	return 0;
 }
 
-static int __intel_set_mode(struct drm_crtc *modeset_crtc,
-			    struct intel_crtc_state *pipe_config)
+static int __intel_set_mode(struct drm_atomic_state *state)
 {
-	struct drm_device *dev = modeset_crtc->dev;
+	struct drm_device *dev = state->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_atomic_state *state = pipe_config->base.state;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
 	int ret = 0;

From: Ander Conselvan de Oliveira <[email protected]>

Now that we can subclass drm_atomic_state we can also use it to keep track of all the pll settings. atomic_state is a better place to hold all shared state than keeping pll->new_config everywhere.

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.h      |   1 -
 drivers/gpu/drm/i915/intel_atomic.c  |  49 ++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c | 111 +++++++++++------------------------
 drivers/gpu/drm/i915/intel_drv.h     |  13 ++++
 4 files changed, 95 insertions(+), 79 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +0 -1

index a0e4868653f2..0e200018c9aa 100644
@@ -319,7 +319,6 @@ struct intel_shared_dpll_config {
 
 struct intel_shared_dpll {
 	struct intel_shared_dpll_config config;
-	struct intel_shared_dpll_config *new_config;
 
 	int active; /* count of number of active CRTCs (i.e. DPMS on) */
 	bool on; /* is the PLL actually active? Disabled during modeset */

drivers/gpu/drm/i915/intel_atomic.c +0 -0

index 7ed8033aae60..aff87054406c 100644
@@ -421,3 +421,52 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
 
 	return 0;

drivers/gpu/drm/i915/intel_atomic.c +40 -0

+
+	/* Copy shared dpll state */
+	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
+		pll = &dev_priv->shared_dplls[i];
+
+		memcpy(&state->shared_dpll[i],
+		       &pll->config, sizeof(pll->config));
+	}
+}
+
+struct intel_shared_dpll_config *
+intel_atomic_get_shared_dpll_state(struct drm_atomic_state *s)
+{
+	struct intel_atomic_state *state = to_intel_atomic_state(s);
+
+	if (!state->dpll_set)
+		intel_atomic_duplicate_dpll_state(state->base.dev, state);
+
+	return state->shared_dpll;
+}
+
+struct drm_atomic_state *
+intel_atomic_state_alloc(struct drm_device *dev)
+{
+	struct intel_atomic_state *state = kzalloc(GFP_KERNEL, sizeof(*state));
+
+	if (!state || drm_atomic_state_init(dev, &state->base) < 0) {
+		kfree(state);
+		return NULL;
+	}
+
+	return &state->base;
+}
+
+void intel_atomic_state_clear(struct drm_atomic_state *s)
+{
+	struct intel_atomic_state *state = to_intel_atomic_state(s);
+	drm_atomic_state_default_clear(&state->base);
+	state->dpll_set = false;
+}

drivers/gpu/drm/i915/intel_display.c +33 -78

index 0a2a7b6e198c..ed69eddf457e 100644
@@ -4229,8 +4229,11 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
 	struct intel_shared_dpll *pll;
+	struct intel_shared_dpll_config *shared_dpll;
 	enum intel_dpll_id i;
 
+	shared_dpll = intel_atomic_get_shared_dpll_state(crtc_state->base.state);
+
 	if (HAS_PCH_IBX(dev_priv->dev)) {
 		/* Ironlake PCH has a fixed PLL->PCH pipe mapping. */
 		i = (enum intel_dpll_id) crtc->pipe;

@@ -4239,7 +4242,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
 		DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n",
 			      crtc->base.base.id, pll->name);
 
-		WARN_ON(pll->new_config->crtc_mask);
+		WARN_ON(shared_dpll[i].crtc_mask);
 
 		goto found;
 	}

@@ -4259,7 +4262,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
 		pll = &dev_priv->shared_dplls[i];
 		DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n",
 			crtc->base.base.id, pll->name);
-		WARN_ON(pll->new_config->crtc_mask);
+		WARN_ON(shared_dpll[i].crtc_mask);
 
 		goto found;
 	}

@@ -4268,15 +4271,15 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
 		pll = &dev_priv->shared_dplls[i];
 
 		/* Only want to check enabled timings first */
-		if (pll->new_config->crtc_mask == 0)
+		if (shared_dpll[i].crtc_mask == 0)
 			continue;
 
 		if (memcmp(&crtc_state->dpll_hw_state,
-			   &pll->new_config->hw_state,
-			   sizeof(pll->new_config->hw_state)) == 0) {
+			   &shared_dpll[i].hw_state,
+			   sizeof(crtc_state->dpll_hw_state)) == 0) {
 			DRM_DEBUG_KMS("CRTC:%d sharing existing %s (crtc mask 0x%08x, ative %d)\n",
 				      crtc->base.base.id, pll->name,
-				      pll->new_config->crtc_mask,
+				      shared_dpll[i].crtc_mask,
 				      pll->active);
 			goto found;
 		}

@@ -4285,7 +4288,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
 	/* Ok no matching timings, maybe there's a free one? */
 	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
 		pll = &dev_priv->shared_dplls[i];
-		if (pll->new_config->crtc_mask == 0) {
+		if (shared_dpll[i].crtc_mask == 0) {
 			DRM_DEBUG_KMS("CRTC:%d allocated %s\n",
 				      crtc->base.base.id, pll->name);
 			goto found;

@@ -4295,83 +4298,33 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
 	return NULL;
 
 found:
-	if (pll->new_config->crtc_mask == 0)
-		pll->new_config->hw_state = crtc_state->dpll_hw_state;
+	if (shared_dpll[i].crtc_mask == 0)
+		shared_dpll[i].hw_state =
+			crtc_state->dpll_hw_state;
 
 	crtc_state->shared_dpll = i;
 	DRM_DEBUG_DRIVER("using %s for pipe %c\n", pll->name,
 			 pipe_name(crtc->pipe));
 
-	pll->new_config->crtc_mask |= 1 << crtc->pipe;
+	shared_dpll[i].crtc_mask |= 1 << crtc->pipe;
 
 	return pll;
 }
 
-/**
- * intel_shared_dpll_start_config - start a new PLL staged config
- * @dev_priv: DRM device
- * @clear_pipes: mask of pipes that will have their PLLs freed
- *
- * Starts a new PLL staged config, copying the current config but
- * releasing the references of pipes specified in clear_pipes.
- */
-static int intel_shared_dpll_start_config(struct drm_i915_private *dev_priv,
-					  unsigned clear_pipes)
-{
-	struct intel_shared_dpll *pll;
-	enum intel_dpll_id i;
-
-	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
-		pll = &dev_priv->shared_dplls[i];
-
-		pll->new_config = kmemdup(&pll->config, sizeof pll->config,
-					  GFP_KERNEL);
-		if (!pll->new_config)
-			goto cleanup;
-
-		pll->new_config->crtc_mask &= ~clear_pipes;
-	}
-
-	return 0;
-
-cleanup:
-	while (--i >= 0) {
-		pll = &dev_priv->shared_dplls[i];
-		kfree(pll->new_config);
-		pll->new_config = NULL;
-	}
-
-	return -ENOMEM;
-}
-
-static void intel_shared_dpll_commit(struct drm_i915_private *dev_priv)
+static void intel_shared_dpll_commit(struct drm_atomic_state *state)
 {
+	struct drm_i915_private *dev_priv = to_i915(state->dev);
+	struct intel_shared_dpll_config *shared_dpll;
 	struct intel_shared_dpll *pll;
 	enum intel_dpll_id i;
 
-	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
-		pll = &dev_priv->shared_dplls[i];
-
-		WARN_ON(pll->new_config == &pll->config);
-
-		pll->config = *pll->new_config;
-		kfree(pll->new_config);
-		pll->new_config = NULL;
-	}
-}
-
-static void intel_shared_dpll_abort_config(struct drm_i915_private *dev_priv)
-{
-	struct intel_shared_dpll *pll;
-	enum intel_dpll_id i;
+	if (!to_intel_atomic_state(state)->dpll_set)
+		return;
 
+	shared_dpll = to_intel_atomic_state(state)->shared_dpll;
 	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
 		pll = &dev_priv->shared_dplls[i];
-
-		WARN_ON(pll->new_config == &pll->config);
-
-		kfree(pll->new_config);
-		pll->new_config = NULL;
+		pll->config = shared_dpll[i];
 	}
 }
 

@@ -11532,13 +11485,12 @@ static void
 intel_modeset_update_state(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_encoder *intel_encoder;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
 	struct drm_connector *connector;
 
-	intel_shared_dpll_commit(dev_priv);
+	intel_shared_dpll_commit(state);
 	drm_atomic_helper_swap_state(state->dev, state);
 
 	for_each_intel_encoder(dev, intel_encoder) {

@@ -12171,9 +12123,13 @@ static int __intel_set_mode_setup_plls(struct drm_atomic_state *state)
 		}
 	}
 
-	ret = intel_shared_dpll_start_config(dev_priv, clear_pipes);
-	if (ret)
-		goto done;
+	if (clear_pipes) {
+		struct intel_shared_dpll_config *shared_dpll =
+			intel_atomic_get_shared_dpll_state(state);
+
+		for (i = 0; i < dev_priv->num_shared_dpll; i++)
+			shared_dpll[i].crtc_mask &= ~clear_pipes;
+	}
 
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
 		if (!needs_modeset(crtc_state) || !crtc_state->enable)

@@ -12184,13 +12140,10 @@ static int __intel_set_mode_setup_plls(struct drm_atomic_state *state)
 
 		ret = dev_priv->display.crtc_compute_clock(intel_crtc,
 							   intel_crtc_state);
-		if (ret) {
-			intel_shared_dpll_abort_config(dev_priv);
-			goto done;
-		}
+		if (ret)
+			return ret;
 	}
 
-done:
 	return ret;
 }
 

@@ -13887,6 +13840,8 @@ static const struct drm_mode_config_funcs intel_mode_funcs = {
 	.output_poll_changed = intel_fbdev_output_poll_changed,
 	.atomic_check = intel_atomic_check,
 	.atomic_commit = intel_atomic_commit,
+	.atomic_state_alloc = intel_atomic_state_alloc,
+	.atomic_state_clear = intel_atomic_state_clear,
 };
 
 /* Set up chip specific display functions */

drivers/gpu/drm/i915/intel_drv.h +13 -0

index d0d99669aa00..7012c67de8d5 100644
@@ -241,6 +241,13 @@ typedef struct dpll {
 	int	p;
 } intel_clock_t;
 
+struct intel_atomic_state {
+	struct drm_atomic_state base;
+
+	bool dpll_set;
+	struct intel_shared_dpll_config shared_dpll[I915_NUM_PLLS];
+};
+
 struct intel_plane_state {
 	struct drm_plane_state base;
 	struct drm_rect src;

@@ -627,6 +634,7 @@ struct cxsr_latency {
 	unsigned long cursor_hpll_disable;
 };
 
+#define to_intel_atomic_state(x) container_of(x, struct intel_atomic_state, base)
 #define to_intel_crtc(x) container_of(x, struct intel_crtc, base)
 #define to_intel_crtc_state(x) container_of(x, struct intel_crtc_state, base)
 #define to_intel_connector(x) container_of(x, struct intel_connector, base)

@@ -1402,6 +1410,11 @@ int intel_connector_atomic_get_property(struct drm_connector *connector,
 struct drm_crtc_state *intel_crtc_duplicate_state(struct drm_crtc *crtc);
 void intel_crtc_destroy_state(struct drm_crtc *crtc,
 			       struct drm_crtc_state *state);
+struct drm_atomic_state *intel_atomic_state_alloc(struct drm_device *dev);
+void intel_atomic_state_clear(struct drm_atomic_state *);
+struct intel_shared_dpll_config *
+intel_atomic_get_shared_dpll_state(struct drm_atomic_state *s);
+
 static inline struct intel_crtc_state *
 intel_atomic_get_crtc_state(struct drm_atomic_state *state,
 			    struct intel_crtc *crtc)

Signature · 6 lines

2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 33 lines

From: Ander Conselvan de Oliveira <[email protected]>

Now that we can subclass drm_atomic_state we can also use it to keep track of all the pll settings. atomic_state is a better place to hold all shared state than keeping pll->new_config everywhere.

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.h      |   1 -
 drivers/gpu/drm/i915/intel_atomic.c  |  49 ++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c | 111 +++++++++++------------------------
 drivers/gpu/drm/i915/intel_drv.h     |  13 ++++
 4 files changed, 95 insertions(+), 79 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +0 -1

index a0e4868653f2..0e200018c9aa 100644
@@ -319,7 +319,6 @@ struct intel_shared_dpll_config {
 
 struct intel_shared_dpll {
 	struct intel_shared_dpll_config config;
-	struct intel_shared_dpll_config *new_config;
 
 	int active; /* count of number of active CRTCs (i.e. DPMS on) */
 	bool on; /* is the PLL actually active? Disabled during modeset */

drivers/gpu/drm/i915/intel_atomic.c +0 -0

index 7ed8033aae60..aff87054406c 100644
@@ -421,3 +421,52 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
 
 	return 0;

drivers/gpu/drm/i915/intel_atomic.c int intel_atomic_setup_scalers(struct drm_device *dev, · line 432

Quoted 10 lines

drivers/gpu/drm/i915/intel_atomic.c +9 -0

 }
+
+static void intel_atomic_duplicate_dpll_state(struct drm_device *dev,
+					      struct intel_atomic_state *state)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_shared_dpll *pll;
+	enum intel_dpll_id i;
+
+	state->dpll_set = true;

The ww mutex locking dance here is missing. For simplicity I think we could just repurpose the dev->mode_config.connection_mutex. We need that anyway full modesets, and dpll changes should only be required in those. Which means this wont introduce any unecessary parallelism.

It means though that we need to wire up a proper error code to all callers of duplicate/get_dpll_state, like with all the other atomic states. Might be best to follow the design for connector/crtc/planes an pass around pointers with error codes explicitly (instead of returning state->shared_dpll below since that can only cope with NULL and not with -EDEADLK).

Sorry that I didn't spot this earlier.

Op 18-05-15 om 17:45 schreef Daniel Vetter:

Quoted 56 lines · Maarten Lankhorst

Quoted 33 lines

From: Ander Conselvan de Oliveira <[email protected]>

Now that we can subclass drm_atomic_state we can also use it to keep track of all the pll settings. atomic_state is a better place to hold all shared state than keeping pll->new_config everywhere.

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.h      |   1 -
 drivers/gpu/drm/i915/intel_atomic.c  |  49 ++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c | 111 +++++++++++------------------------
 drivers/gpu/drm/i915/intel_drv.h     |  13 ++++
 4 files changed, 95 insertions(+), 79 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +0 -1

index a0e4868653f2..0e200018c9aa 100644
@@ -319,7 +319,6 @@ struct intel_shared_dpll_config {
 
 struct intel_shared_dpll {
 	struct intel_shared_dpll_config config;
-	struct intel_shared_dpll_config *new_config;
 
 	int active; /* count of number of active CRTCs (i.e. DPMS on) */
 	bool on; /* is the PLL actually active? Disabled during modeset */

drivers/gpu/drm/i915/intel_atomic.c +0 -0

index 7ed8033aae60..aff87054406c 100644
@@ -421,3 +421,52 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
 
 	return 0;

drivers/gpu/drm/i915/intel_atomic.c int intel_atomic_setup_scalers(struct drm_device *dev, · line 432

Quoted 10 lines

drivers/gpu/drm/i915/intel_atomic.c +9 -0

 }
+
+static void intel_atomic_duplicate_dpll_state(struct drm_device *dev,
+					      struct intel_atomic_state *state)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_shared_dpll *pll;
+	enum intel_dpll_id i;
+
+	state->dpll_set = true;

The ww mutex locking dance here is missing. For simplicity I think we could just repurpose the dev->mode_config.connection_mutex. We need that anyway full modesets, and dpll changes should only be required in those. Which means this wont introduce any unecessary parallelism.

It means though that we need to wire up a proper error code to all callers of duplicate/get_dpll_state, like with all the other atomic states. Might be best to follow the design for connector/crtc/planes an pass around pointers with error codes explicitly (instead of returning state->shared_dpll below since that can only cope with NULL and not with -EDEADLK).

Quoted 2 lines

Sorry that I didn't spot this earlier.

The dpll state should only ever be done during a modeset in which case the connection_mutex is guaranteed to be held. Instead of making this return a value can we add a lockdep_assert_held ?

This needs to be a global check because at the time of crtc checking not all modesets have to be calculated yet. Use intel_crtc->atomic because there's no reason to keep it in state.

Changes since v1:
- Use intel_crtc->atomic as a place to put hsw_workaround_pipe.
- Make sure quirk only applies to haswell.
- Use first loop to iterate over newly enabled crtc's only. This increases readability.

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 100 ++++++++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_drv.h     |   3 ++
 2 files changed, 72 insertions(+), 31 deletions(-)

drivers/gpu/drm/i915/intel_display.c +69 -31

index 1d3b0233f070..9316b0be4f5b 100644
@@ -4897,42 +4897,13 @@ static bool hsw_crtc_supports_ips(struct intel_crtc *crtc)
 	return HAS_IPS(crtc->base.dev) && crtc->pipe == PIPE_A;
 }
 
-/*
- * This implements the workaround described in the "notes" section of the mode
- * set sequence documentation. When going from no pipes or single pipe to
- * multiple pipes, and planes are enabled after the pipe, we need to wait at
- * least 2 vblanks on the first pipe before enabling planes on the second pipe.
- */
-static void haswell_mode_set_planes_workaround(struct intel_crtc *crtc)
-{
-	struct drm_device *dev = crtc->base.dev;
-	struct intel_crtc *crtc_it, *other_active_crtc = NULL;
-
-	/* We want to get the other_active_crtc only if there's only 1 other
-	 * active crtc. */
-	for_each_intel_crtc(dev, crtc_it) {
-		if (!crtc_it->active || crtc_it == crtc)
-			continue;
-
-		if (other_active_crtc)
-			return;
-
-		other_active_crtc = crtc_it;
-	}
-	if (!other_active_crtc)
-		return;
-
-	intel_wait_for_vblank(dev, other_active_crtc->pipe);
-	intel_wait_for_vblank(dev, other_active_crtc->pipe);
-}
-
 static void haswell_crtc_enable(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);
 	struct intel_encoder *encoder;
-	int pipe = intel_crtc->pipe;
+	int pipe = intel_crtc->pipe, hsw_workaround_pipe;
 
 	if (WARN_ON(intel_crtc->active))
 		return;

@@ -5009,7 +4980,11 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 
 	/* If we change the relative order between pipe/planes enabling, we need
 	 * to change the workaround. */
-	haswell_mode_set_planes_workaround(intel_crtc);
+	hsw_workaround_pipe = intel_crtc->atomic.hsw_workaround_pipe;
+	if (IS_HASWELL(dev) && hsw_workaround_pipe != INVALID_PIPE) {
+		intel_wait_for_vblank(dev, hsw_workaround_pipe);
+		intel_wait_for_vblank(dev, hsw_workaround_pipe);
+	}
 }
 
 static void ironlake_pfit_disable(struct intel_crtc *crtc)

@@ -12099,6 +12074,66 @@ static int __intel_set_mode_setup_plls(struct drm_atomic_state *state)
 	return ret;
 }
 
+/*
+ * This implements the workaround described in the "notes" section of the mode
+ * set sequence documentation. When going from no pipes or single pipe to
+ * multiple pipes, and planes are enabled after the pipe, we need to wait at
+ * least 2 vblanks on the first pipe before enabling planes on the second pipe.
+ */
+static int haswell_mode_set_planes_workaround(struct drm_atomic_state *state)
+{
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
+	struct intel_crtc *intel_crtc, *enabled_crtc;
+	struct intel_crtc *first_crtc = NULL, *other_crtc = NULL;
+	int enabled_crtcs = 0, i;
+
+	/* look at all crtc's that are going to be enabled in during modeset */
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		intel_crtc = to_intel_crtc(crtc);
+
+		if (!crtc_state->active || !needs_modeset(crtc_state))
+			continue;
+
+		if (first_crtc) {
+			other_crtc = intel_crtc;
+			break;
+		} else {
+			first_crtc = intel_crtc;
+		}
+	}
+
+	/* No workaround needed? */
+	if (!first_crtc)
+		return 0;
+
+	/* w/a possibly needed, check how many crtc's are already enabled. */
+	for_each_intel_crtc(state->dev, intel_crtc) {
+		intel_crtc->atomic.hsw_workaround_pipe = INVALID_PIPE;
+
+		crtc_state = drm_atomic_get_crtc_state(state,
+						       &intel_crtc->base);
+		if (IS_ERR(crtc_state))
+			return PTR_ERR(crtc_state);
+
+		if (!crtc_state->active || needs_modeset(crtc_state))
+			continue;
+
+		/* 2 or more enabled crtcs means no need for w/a */
+		if (++enabled_crtcs >= 2)
+			return 0;
+
+		enabled_crtc = intel_crtc;
+	}
+
+	if (enabled_crtcs == 1)
+		first_crtc->atomic.hsw_workaround_pipe = enabled_crtc->pipe;
+	else if (other_crtc)
+		other_crtc->atomic.hsw_workaround_pipe = first_crtc->pipe;
+
+	return 0;
+}
+
 /* Code that should eventually be part of atomic_check() */
 static int __intel_set_mode_checks(struct drm_atomic_state *state)
 {

@@ -12122,6 +12157,9 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state)
 	if (ret)
 		return ret;
 
+	if (IS_HASWELL(dev))
+		haswell_mode_set_planes_workaround(state);
+
 	return 0;
 }
 

drivers/gpu/drm/i915/intel_drv.h +3 -0

index 65f5f3d41b5a..1aa10a9445de 100644
@@ -490,6 +490,9 @@ struct intel_crtc_atomic_commit {
 	bool update_fbc;
 	bool post_enable_primary;
 	unsigned update_sprite_watermarks;
+
+	/* Operations to perform during modeset */
+	enum pipe hsw_workaround_pipe;
 };
 
 struct intel_crtc {

Signature · 6 lines

2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 3 lines · Maarten Lankhorst

This needs to be a global check because at the time of crtc checking not all modesets have to be calculated yet. Use intel_crtc->atomic because there's no reason to keep it in state.

Hm, intel_crtc->atomic needs to be moved into intel_crtc_state eventually, so I don't really understand your reasoning here. Yes it's not really a static state, but we carry all the other state transition hints like ->need_modeset or ->active_changed around in state objects too. Imo it's the right place for this stuff, the only tricky part is that we must clear them all in the relevant duplicate_state hooks.

Anyway nothing that blocks this one, just something to keep in mind for the road ahead.

intel_crtc->config will be removed eventually, so use crtc->hwmode. drm_atomic_helper_update_legacy_modeset_state updates hwmode, but crtc->active will eventually be gone too. Set dotclock to zero to indicate the crtc is inactive.

Signed-off-by Maarten Lankhorst <[email protected]>

---
 drivers/gpu/drm/i915/i915_irq.c      | 13 ++++++-------
 drivers/gpu/drm/i915/intel_display.c |  3 +++
 2 files changed, 9 insertions(+), 7 deletions(-)

drivers/gpu/drm/i915/i915_irq.c +6 -7

index ca457317a8ac..9359ea2399f1 100644
@@ -564,8 +564,7 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, int pipe)
 	u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal;
 	struct intel_crtc *intel_crtc =
 		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
-	const struct drm_display_mode *mode =
-		&intel_crtc->config->base.adjusted_mode;
+	const struct drm_display_mode *mode = &intel_crtc->base.hwmode;
 
 	htotal = mode->crtc_htotal;
 	hsync_start = mode->crtc_hsync_start;

@@ -620,7 +619,7 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	const struct drm_display_mode *mode = &crtc->config->base.adjusted_mode;
+	const struct drm_display_mode *mode = &crtc->base.hwmode;
 	enum pipe pipe = crtc->pipe;
 	int position, vtotal;
 

@@ -647,14 +646,14 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	const struct drm_display_mode *mode = &intel_crtc->config->base.adjusted_mode;
+	const struct drm_display_mode *mode = &intel_crtc->base.hwmode;
 	int position;
 	int vbl_start, vbl_end, hsync_start, htotal, vtotal;
 	bool in_vbl = true;
 	int ret = 0;
 	unsigned long irqflags;
 
-	if (!intel_crtc->active) {
+	if (WARN_ON(!mode->crtc_clock)) {
 		DRM_DEBUG_DRIVER("trying to get scanoutpos for disabled "
 				 "pipe %c\n", pipe_name(pipe));
 		return 0;

@@ -796,7 +795,7 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
 		return -EINVAL;
 	}
 
-	if (!crtc->state->active) {
+	if (!crtc->hwmode.crtc_clock) {
 		DRM_DEBUG_KMS("crtc %d is disabled\n", pipe);
 		return -EBUSY;
 	}

@@ -805,7 +804,7 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
 	return drm_calc_vbltimestamp_from_scanoutpos(dev, pipe, max_error,
 						     vblank_time, flags,
 						     crtc,
-						     &to_intel_crtc(crtc)->config->base.adjusted_mode);
+						     &crtc->hwmode);
 }
 
 static bool intel_hpd_irq_event(struct drm_device *dev,

drivers/gpu/drm/i915/intel_display.c +3 -0

index 9316b0be4f5b..5063ac3d028d 100644
@@ -11467,6 +11467,9 @@ intel_modeset_update_state(struct drm_atomic_state *state)
 		WARN_ON(crtc->state->enable != intel_crtc_in_use(crtc));
 
 		to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state);
+
+		if (!crtc->state->active)
+			crtc->hwmode.crtc_clock = 0;
 	}
 
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {

Signature · 6 lines

2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 5 lines · Maarten Lankhorst

intel_crtc->config will be removed eventually, so use crtc->hwmode. drm_atomic_helper_update_legacy_modeset_state updates hwmode, but crtc->active will eventually be gone too. Set dotclock to zero to indicate the crtc is inactive.

Quoted 1 line

Signed-off-by Maarten Lankhorst <[email protected]>

I think adding a code comment to our assignment of crtc->hw_mode that we need this for i915_get_vblank_timestamp (and only for that) would be really good. Especially since I can't find it with a quick grep, at least in current upstream ;-)

-Daniel

Quoted 84 lines

---
 drivers/gpu/drm/i915/i915_irq.c      | 13 ++++++-------
 drivers/gpu/drm/i915/intel_display.c |  3 +++
 2 files changed, 9 insertions(+), 7 deletions(-)

drivers/gpu/drm/i915/i915_irq.c +6 -7

index ca457317a8ac..9359ea2399f1 100644
@@ -564,8 +564,7 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, int pipe)
 	u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal;
 	struct intel_crtc *intel_crtc =
 		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
-	const struct drm_display_mode *mode =
-		&intel_crtc->config->base.adjusted_mode;
+	const struct drm_display_mode *mode = &intel_crtc->base.hwmode;
 
 	htotal = mode->crtc_htotal;
 	hsync_start = mode->crtc_hsync_start;

@@ -620,7 +619,7 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	const struct drm_display_mode *mode = &crtc->config->base.adjusted_mode;
+	const struct drm_display_mode *mode = &crtc->base.hwmode;
 	enum pipe pipe = crtc->pipe;
 	int position, vtotal;
 

@@ -647,14 +646,14 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	const struct drm_display_mode *mode = &intel_crtc->config->base.adjusted_mode;
+	const struct drm_display_mode *mode = &intel_crtc->base.hwmode;
 	int position;
 	int vbl_start, vbl_end, hsync_start, htotal, vtotal;
 	bool in_vbl = true;
 	int ret = 0;
 	unsigned long irqflags;
 
-	if (!intel_crtc->active) {
+	if (WARN_ON(!mode->crtc_clock)) {
 		DRM_DEBUG_DRIVER("trying to get scanoutpos for disabled "
 				 "pipe %c\n", pipe_name(pipe));
 		return 0;

@@ -796,7 +795,7 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
 		return -EINVAL;
 	}
 
-	if (!crtc->state->active) {
+	if (!crtc->hwmode.crtc_clock) {
 		DRM_DEBUG_KMS("crtc %d is disabled\n", pipe);
 		return -EBUSY;
 	}

@@ -805,7 +804,7 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
 	return drm_calc_vbltimestamp_from_scanoutpos(dev, pipe, max_error,
 						     vblank_time, flags,
 						     crtc,
-						     &to_intel_crtc(crtc)->config->base.adjusted_mode);
+						     &crtc->hwmode);
 }
 
 static bool intel_hpd_irq_event(struct drm_device *dev,

drivers/gpu/drm/i915/intel_display.c +3 -0

index 9316b0be4f5b..5063ac3d028d 100644
@@ -11467,6 +11467,9 @@ intel_modeset_update_state(struct drm_atomic_state *state)
 		WARN_ON(crtc->state->enable != intel_crtc_in_use(crtc));
 
 		to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state);
+
+		if (!crtc->state->active)
+			crtc->hwmode.crtc_clock = 0;
 	}
 
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {

Signature · 6 lines

2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 7 lines

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

Quoted 6 lines · Maarten Lankhorst

Quoted 5 lines

intel_crtc->config will be removed eventually, so use crtc->hwmode. drm_atomic_helper_update_legacy_modeset_state updates hwmode, but crtc->active will eventually be gone too. Set dotclock to zero to indicate the crtc is inactive.

Quoted 5 lines

Quoted 1 line

Signed-off-by Maarten Lankhorst <[email protected]>

I think adding a code comment to our assignment of crtc->hw_mode that we need this for i915_get_vblank_timestamp (and only for that) would be really good. Especially since I can't find it with a quick grep, at least in current upstream ;-)

I don't particularly like resurrecting this zombie. Why we can't just use crtc->state->adjusted_mode (or wherever the current adjusted mode is kept)?

Op 18-05-15 om 18:28 schreef Ville Syrjälä:

Quoted 7 lines · Daniel Vetter

Quoted 6 lines · Maarten Lankhorst

Quoted 5 lines

intel_crtc->config will be removed eventually, so use crtc->hwmode. drm_atomic_helper_update_legacy_modeset_state updates hwmode, but crtc->active will eventually be gone too. Set dotclock to zero to indicate the crtc is inactive.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

Signed-off-by Maarten Lankhorst <[email protected]>

I think adding a code comment to our assignment of crtc->hw_mode that we need this for i915_get_vblank_timestamp (and only for that) would be really good. Especially since I can't find it with a quick grep, at least in current upstream ;-)

I don't particularly like resurrecting this zombie. Why we can't just use crtc->state->adjusted_mode (or wherever the current adjusted mode is kept)?

Because we want to get rid of intel_crtc->config, and if drm_atomic_swap_state is moved to be done before any call to then crtc->state->adjusted_mode will not be in sync with the hw state, and any wai tfor vblank will produce funny results.

Since I don't think you should want to pass a state to vblank you would have to use some crtc local variable somewhere, in this case I chose to use hwmode for that.