drm/i915: Convert to atomic, part 2.

This is a function used to disable all crtc's. This makes it clearer to distinguish between when mode needs to be preserved and when it can be trashed.

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

---
Oops, I was trashing all state during suspend and on gpu reset. I will send an amended intel_crtc_control patch too with the suspend and prepare_reset parts taken out.

 drivers/gpu/drm/i915/i915_drv.c      |  4 +---
 drivers/gpu/drm/i915/intel_display.c | 29 +++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 3 files changed, 21 insertions(+), 13 deletions(-)

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

index 5cc57f2ec192..d1a090a9f653 100644
@@ -600,7 +600,6 @@ static int skl_resume_prepare(struct drm_i915_private *dev_priv);
 static int i915_drm_suspend(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_crtc *crtc;
 	pci_power_t opregion_target_state;
 	int error;
 

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

 	drm_modeset_unlock_all(dev);
 
 	intel_dp_mst_suspend(dev);

drivers/gpu/drm/i915/intel_display.c +19 -10

index cae36ec1c2ef..8d50c4ca561f 100644
@@ -3204,9 +3204,6 @@ void intel_crtc_reset(struct intel_crtc *crtc)
 
 void intel_prepare_reset(struct drm_device *dev)
 {
-	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct intel_crtc *crtc;
-
 	/* no reset support for gen2 */
 	if (IS_GEN2(dev))
 		return;

@@ -3221,13 +3218,7 @@ void intel_prepare_reset(struct drm_device *dev)
 	 * Disabling the crtcs gracefully seems nicer. Also the
 	 * g33 docs say we should at least disable all the planes.
 	 */
-	for_each_intel_crtc(dev, crtc) {
-		if (!crtc->active)
-			continue;
-
-		intel_crtc_disable_planes(&crtc->base);
-		dev_priv->display.crtc_disable(&crtc->base);
-	}
+	intel_display_suspend(dev);
 }
 
 void intel_finish_reset(struct drm_device *dev)

@@ -6018,6 +6009,24 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	mutex_unlock(&dev->struct_mutex);
 }
 
+/*
+ * 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)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_crtc *crtc;
+
+	for_each_crtc(dev, crtc) {
+		if (!to_intel_crtc(crtc)->active)
+			continue;
+
+		intel_crtc_disable_planes(crtc);
+		dev_priv->display.crtc_disable(crtc);
+	}
+}
+
 /* Master function to enable/disable CRTC and corresponding power wells */
 void intel_crtc_control(struct drm_crtc *crtc, bool enable)
 {

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

index 70d0cc8fe70e..c24d670529e3 100644
@@ -992,6 +992,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);
 void intel_crtc_control(struct drm_crtc *crtc, bool enable);
 void intel_crtc_reset(struct intel_crtc *crtc);
 void intel_crtc_update_dpms(struct drm_crtc *crtc);

Signature · 7 lines

2.1.0


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

Quoted 3 lines

This is a function used to disable all crtc's. This makes it clearer to distinguish between when mode needs to be preserved and when it can be trashed.

To clarify, when you talk about mode being preserved or trashed here, you're talking about the hardware's idea of the mode, not the driver's software state, right? I.e., because when we shut down a power well the registers vanish and whatever was programmed in them is lost?

See my comments farther down.

Quoted 24 lines

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

---
Oops, I was trashing all state during suspend and on gpu reset. I will send an amended intel_crtc_control patch too with the suspend and prepare_reset parts taken out.

 drivers/gpu/drm/i915/i915_drv.c      |  4 +---
 drivers/gpu/drm/i915/intel_display.c | 29 +++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 3 files changed, 21 insertions(+), 13 deletions(-)

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

index 5cc57f2ec192..d1a090a9f653 100644
@@ -600,7 +600,6 @@ static int skl_resume_prepare(struct drm_i915_private *dev_priv);
 static int i915_drm_suspend(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_crtc *crtc;
 	pci_power_t opregion_target_state;
 	int error;
 

drivers/gpu/drm/i915/i915_drv.c static int i915_drm_suspend(struct drm_device *dev) · line 633

Quoted 6 lines

drivers/gpu/drm/i915/i915_drv.c +1 -2

@@ -631,8 +630,7 @@ static int i915_drm_suspend(struct drm_device *dev)
 	 * for _thaw. Also, power gate the CRTC power wells.
 	 */
 	drm_modeset_lock_all(dev);
-	for_each_crtc(dev, crtc)
-		intel_crtc_control(crtc, false);
+	intel_display_suspend(dev);

I'm not terribly familiar with the power well details, but it looks like part of the motivation of commit

commit b04c5bd6fda54703e56f29569e4bca489d6c5a5c
Author: Borun Fu <[email protected]>
Date: Sat Jul 12 10:02:27 2014 +0530

drm/i915: Power gating display wells during i915_pm_suspend

which added intel_crtc_control() was to ensure the power wells were gated at this point; by replacing the intel_crtc_control() with intel_display_suspend() here, you're removing that power well programming...is that intentional (and is it going to cause the display to stay in D0 state)?

If it is intentional, the comment above this block is out of date now. Since this patch (and the following one) seem to change the semantics of when we're touching power wells at various points in the code, maybe you can elaborate a little bit on that in the commit message of one or both commits.

The primary plane can still be configured when crtc is off, furthermore this is also a noop now that affected planes are added on modesets.

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

---
 drivers/gpu/drm/i915/intel_display.c | 2 --
 1 file changed, 2 deletions(-)

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

index 9e1723e20377..d738230bedf8 100644
@@ -12265,8 +12265,6 @@ static int __intel_set_mode(struct drm_atomic_state *state)
 
 		intel_crtc_disable_planes(crtc);
 		dev_priv->display.crtc_disable(crtc);
-		if (!crtc_state->enable)
-			drm_plane_helper_disable(crtc->primary);
 	}
 
 	/* Only after disabling all output pipelines that will be changed can we

Signature · 6 lines

2.1.0

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

Quoted 5 lines

The primary plane can still be configured when crtc is off, furthermore this is also a noop now that affected planes are added on modesets.

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

This was added in patch #2. Can we squash this change up, or was there a reason we needed it for the intermediate patches?

Actually, I'm not sure this is really quite a noop yet. prepare_plane_fb is never called on the primary plane as far as I can see, so I think our frontbuffer tracking and such might get confused. In the regular plane update codepath, we have to handle this with a special case by doing:

        intel_crtc->atomic.disabled_planes |= (1 << drm_plane_index(plane));

in the intel_plane_atomic_check function. But from what I can see, we bypass that in this codepath. igt/kms_universal_plane shows that we do run into frontbuffer tracking warnings if we don't put an equivalent update to the 'disabled_planes' flag here.

From: Maarten Lankhorst <[email protected]>

Repeated calls to begin_crtc_commit can cause warnings like this:

[  169.127746] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:616
[  169.127835] in_atomic(): 0, irqs_disabled(): 1, pid: 1947, name: kms_flip
[  169.127840] 3 locks held by kms_flip/1947:
[  169.127843]  #0:  (&dev->mode_config.mutex){+.+.+.}, at: [<ffffffff814774bc>] __drm_modeset_lock_all+0x9c/0x130
[  169.127860]  #1:  (crtc_ww_class_acquire){+.+.+.}, at: [<ffffffff814774cd>] __drm_modeset_lock_all+0xad/0x130
[  169.127870]  #2:  (crtc_ww_class_mutex){+.+.+.}, at: [<ffffffff81477178>] drm_modeset_lock+0x38/0x110
[  169.127879] irq event stamp: 665690
[  169.127882] hardirqs last  enabled at (665689): [<ffffffff817ffdb5>] _raw_spin_unlock_irqrestore+0x55/0x70
[  169.127889] hardirqs last disabled at (665690): [<ffffffffc0197a23>] intel_pipe_update_start+0x113/0x5c0 [i915]
[  169.127936] softirqs last  enabled at (665470): [<ffffffff8108a766>] __do_softirq+0x236/0x650
[  169.127942] softirqs last disabled at (665465): [<ffffffff8108ae75>] irq_exit+0xc5/0xd0
[  169.127951] CPU: 1 PID: 1947 Comm: kms_flip Not tainted 4.1.0-rc4-patser+ #4039
[  169.127954] Hardware name: LENOVO 2349AV8/2349AV8, BIOS G1ETA5WW (2.65 ) 04/15/2014
[  169.127957]  ffff8800c49036f0 ffff8800cde5fa28 ffffffff817f6907 0000000080000001
[  169.127964]  0000000000000000 ffff8800cde5fa58 ffffffff810aebed 0000000000000046
[  169.127970]  ffffffff81c5d518 0000000000000268 0000000000000000 ffff8800cde5fa88
[  169.127981] Call Trace:
[  169.127992]  [<ffffffff817f6907>] dump_stack+0x4f/0x7b
[  169.128001]  [<ffffffff810aebed>] ___might_sleep+0x16d/0x270
[  169.128008]  [<ffffffff810aed38>] __might_sleep+0x48/0x90
[  169.128017]  [<ffffffff817fc359>] mutex_lock_nested+0x29/0x410
[  169.128073]  [<ffffffffc01635f0>] ? vgpu_write64+0x220/0x220 [i915]
[  169.128138]  [<ffffffffc017fddf>] ? ironlake_update_primary_plane+0x2ff/0x410 [i915]
[  169.128198]  [<ffffffffc0190e75>] intel_frontbuffer_flush+0x25/0x70 [i915]
[  169.128253]  [<ffffffffc01831ac>] intel_finish_crtc_commit+0x4c/0x180 [i915]
[  169.128279]  [<ffffffffc00784ac>] drm_atomic_helper_commit_planes+0x12c/0x240 [drm_kms_helper]
[  169.128338]  [<ffffffffc0184264>] __intel_set_mode+0x684/0x830 [i915]
[  169.128378]  [<ffffffffc018a84a>] intel_crtc_set_config+0x49a/0x620 [i915]
[  169.128385]  [<ffffffff817fdd39>] ? mutex_unlock+0x9/0x10
[  169.128391]  [<ffffffff81467b69>] drm_mode_set_config_internal+0x69/0x120
[  169.128398]  [<ffffffff8119b547>] ? might_fault+0x57/0xb0
[  169.128403]  [<ffffffff8146bf93>] drm_mode_setcrtc+0x253/0x620
[  169.128409]  [<ffffffff8145c600>] drm_ioctl+0x1a0/0x6a0
[  169.128415]  [<ffffffff810b3b41>] ? get_parent_ip+0x11/0x50
[  169.128424]  [<ffffffff811e9ab8>] do_vfs_ioctl+0x2f8/0x530
[  169.128429]  [<ffffffff810d0fcd>] ? trace_hardirqs_on+0xd/0x10
[  169.128435]  [<ffffffff812e7676>] ? selinux_file_ioctl+0x56/0x100
[  169.128439]  [<ffffffff811e9d71>] SyS_ioctl+0x81/0xa0
[  169.128445]  [<ffffffff81800697>] system_call_fastpath+0x12/0x6f

Solve it by using the newly introduced drm_atomic_helper_commit_planes_on_crtc.

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

---
 drivers/gpu/drm/i915/intel_atomic.c  | 13 +++----------
 drivers/gpu/drm/i915/intel_display.c |  5 +++--
 2 files changed, 6 insertions(+), 12 deletions(-)

drivers/gpu/drm/i915/intel_atomic.c +3 -10

index e93aade0f2dc..83078763ba14 100644
@@ -151,18 +151,11 @@ int intel_atomic_commit(struct drm_device *dev,
 
 		if (INTEL_INFO(dev)->gen >= 9)
 			skl_detach_scalers(to_intel_crtc(crtc));
+
+		drm_atomic_helper_commit_planes_on_crtc(crtc_state);
 	}
 
-	/*
-	 * FIXME:  The proper sequence here will eventually be:
-	 *
-	 * drm_atomic_helper_commit_modeset_disables(dev, state);
-	 * drm_atomic_helper_commit_planes(dev, state);
-	 * drm_atomic_helper_commit_modeset_enables(dev, state);
-	 *
-	 * once we have full atomic modeset.
-	 */
-	drm_atomic_helper_commit_planes(dev, 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);

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

index adb19c3c9172..81d5358efdde 100644
@@ -12225,10 +12225,10 @@ static int __intel_set_mode(struct drm_atomic_state *state)
 
 	modeset_update_crtc_power_domains(state);
 
-	drm_atomic_helper_commit_planes(dev, state);
-
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		drm_atomic_helper_commit_planes_on_crtc(crtc_state);
+
 		if (!needs_modeset(crtc->state) || !crtc->state->active)
 			continue;
 

@@ -14572,6 +14572,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 		struct intel_plane_state *plane_state;
 
 		memset(crtc->config, 0, sizeof(*crtc->config));
+		crtc->config->base.crtc = &crtc->base;
 
 		crtc->config->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE;
 

Signature · 6 lines

2.1.0

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

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

It makes more sense there, since these are computation steps that can fail.

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

---
 drivers/gpu/drm/i915/intel_display.c | 70 ++++++++++++++++++------------------
 1 file changed, 35 insertions(+), 35 deletions(-)

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

index 81d5358efdde..e7aa8610cbdc 100644
@@ -12085,37 +12085,6 @@ static void update_scanline_offset(struct intel_crtc *crtc)
 		crtc->scanline_offset = 1;
 }
 
-static int
-intel_modeset_compute_config(struct drm_atomic_state *state)
-{
-	struct drm_crtc *crtc;
-	struct drm_crtc_state *crtc_state;
-	int ret, i;
-
-	ret = drm_atomic_helper_check_modeset(state->dev, state);
-	if (ret)
-		return ret;
-
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		if (!crtc_state->enable &&
-		    WARN_ON(crtc_state->active))
-			crtc_state->active = false;
-
-		if (!crtc_state->enable)
-			continue;
-
-		ret = intel_modeset_pipe_config(crtc, state);
-		if (ret)
-			return ret;
-
-		intel_dump_pipe_config(to_intel_crtc(crtc),
-				       to_intel_crtc_state(crtc_state),
-				       "[modeset]");
-	}
-
-	return drm_atomic_helper_check_planes(state->dev, state);
-}
-
 static int __intel_set_mode_setup_plls(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;

@@ -12191,6 +12160,41 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state)
 	return 0;
 }
 
+static int
+intel_modeset_compute_config(struct drm_atomic_state *state)
+{
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
+	int ret, i;
+
+	ret = drm_atomic_helper_check_modeset(state->dev, state);
+	if (ret)
+		return ret;
+
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		if (!crtc_state->enable &&
+		    WARN_ON(crtc_state->active))
+			crtc_state->active = false;
+
+		if (!crtc_state->enable)
+			continue;
+
+		ret = intel_modeset_pipe_config(crtc, state);
+		if (ret)
+			return ret;
+

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

+}
+
 static int __intel_set_mode(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;

@@ -12200,10 +12204,6 @@ static int __intel_set_mode(struct drm_atomic_state *state)
 	int ret = 0;
 	int i;
 
-	ret = __intel_set_mode_checks(state);
-	if (ret < 0)
-		return ret;
-
 	ret = drm_atomic_helper_prepare_planes(dev, state);
 	if (ret)
 		return ret;

Signature · 6 lines

2.1.0

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

Quoted 6 lines

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

It makes more sense there, since these are computation steps that can fail.

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

I've noticed that a few of the patches in this series were originally written by Ander, but seem to be missing his s-o-b line. I think you generally want to just append your line after his in that case.

One other cosmetic note farther down.

Quoted 79 lines

---
 drivers/gpu/drm/i915/intel_display.c | 70 ++++++++++++++++++------------------
 1 file changed, 35 insertions(+), 35 deletions(-)

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

index 81d5358efdde..e7aa8610cbdc 100644
@@ -12085,37 +12085,6 @@ static void update_scanline_offset(struct intel_crtc *crtc)
 		crtc->scanline_offset = 1;
 }
 
-static int
-intel_modeset_compute_config(struct drm_atomic_state *state)
-{
-	struct drm_crtc *crtc;
-	struct drm_crtc_state *crtc_state;
-	int ret, i;
-
-	ret = drm_atomic_helper_check_modeset(state->dev, state);
-	if (ret)
-		return ret;
-
-	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		if (!crtc_state->enable &&
-		    WARN_ON(crtc_state->active))
-			crtc_state->active = false;
-
-		if (!crtc_state->enable)
-			continue;
-
-		ret = intel_modeset_pipe_config(crtc, state);
-		if (ret)
-			return ret;
-
-		intel_dump_pipe_config(to_intel_crtc(crtc),
-				       to_intel_crtc_state(crtc_state),
-				       "[modeset]");
-	}
-
-	return drm_atomic_helper_check_planes(state->dev, state);
-}
-
 static int __intel_set_mode_setup_plls(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;

@@ -12191,6 +12160,41 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state)
 	return 0;
 }
 
+static int
+intel_modeset_compute_config(struct drm_atomic_state *state)
+{
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
+	int ret, i;
+
+	ret = drm_atomic_helper_check_modeset(state->dev, state);
+	if (ret)
+		return ret;
+
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		if (!crtc_state->enable &&
+		    WARN_ON(crtc_state->active))
+			crtc_state->active = false;
+
+		if (!crtc_state->enable)
+			continue;
+
+		ret = intel_modeset_pipe_config(crtc, state);
+		if (ret)
+			return ret;
+
+		intel_dump_pipe_config(to_intel_crtc(crtc),
+				       to_intel_crtc_state(crtc_state),
+				       "[modeset]");
+	}
+
+	ret = drm_atomic_helper_check_planes(state->dev, state);

drivers/gpu/drm/i915/intel_display.c static int __intel_set_mode_checks(struct drm_atomic_state *state) · line 12195

Quoted 4 lines

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

+	if (ret)
+		return ret;
+
+	return __intel_set_mode_checks(state);

Just a cosmetic note, but maybe we should rename this function now? It's not called from __intel_set_mode anymore and it isn't really 'checks' (but rather setup that we intend to be done during the check phase), so the whole name seems a bit misleading now.

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

To make this work we load the new hardware state into the atomic_state, then swap it with the sw state.

This lets us change the force restore path in setup_hw_state() to use a single call to intel_mode_set() to restore all the previous state.

As a nice bonus this kills off encoder->new_encoder, connector->new_enabled and crtc->new_enabled. They were used only to restore the state after a modeset.

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

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

---
 drivers/gpu/drm/i915/intel_atomic.c  |   2 +-
 drivers/gpu/drm/i915/intel_display.c | 377 ++++++++++++++++++++++-------------
 drivers/gpu/drm/i915/intel_drv.h     |  14 +-
 3 files changed, 241 insertions(+), 152 deletions(-)

Hey,

Op 20-05-15 om 18:04 schreef [email protected]:

Quoted 13 lines

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

To make this work we load the new hardware state into the atomic_state, then swap it with the sw state.

This lets us change the force restore path in setup_hw_state() to use a single call to intel_mode_set() to restore all the previous state.

As a nice bonus this kills off encoder->new_encoder, connector->new_enabled and crtc->new_enabled. They were used only to restore the state after a modeset.

Quoted 8 lines

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

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

---
 drivers/gpu/drm/i915/intel_atomic.c  |   2 +-
 drivers/gpu/drm/i915/intel_display.c | 377 ++++++++++++++++++++++-------------
 drivers/gpu/drm/i915/intel_drv.h     |  14 +-
 3 files changed, 241 insertions(+), 152 deletions(-)

Small fix up. This patch may force the modeset code to no longer do a modeset on boot, causing a black screen on Daniel Stone's machine. Fixup below disables all crtc's for the !i915.fastboot case.

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

index 13b09efeb2a0..b70e3890e2bf 100644
@@ -14138,7 +14138,7 @@ void intel_modeset_init(struct drm_device *dev)
 	intel_fbc_disable(dev);
 
 	drm_modeset_lock_all(dev);
-	intel_modeset_setup_hw_state(dev, false);
+	intel_modeset_setup_hw_state(dev, !i915.fastboot);
 	drm_modeset_unlock_all(dev);
 
 	for_each_intel_crtc(dev, crtc) {

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

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

To make this work we load the new hardware state into the atomic_state, then swap it with the sw state.

This lets us change the force restore path in setup_hw_state() to use a single call to intel_mode_set() to restore all the previous state.

As a nice bonus this kills off encoder->new_encoder, connector->new_enabled and crtc->new_enabled. They were used only to restore the state after a modeset.

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

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

---
 drivers/gpu/drm/i915/intel_atomic.c  |   2 +-
 drivers/gpu/drm/i915/intel_display.c | 377 ++++++++++++++++++++++-------------
 drivers/gpu/drm/i915/intel_drv.h     |  14 +-
 3 files changed, 241 insertions(+), 152 deletions(-)

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

index 83078763ba14..17bf9e80c557 100644
@@ -395,7 +395,7 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
 	return 0;
 }
 
-static void
+void
 intel_atomic_duplicate_dpll_state(struct drm_i915_private *dev_priv,
 				  struct intel_shared_dpll_config *shared_dpll)
 {

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

index e7aa8610cbdc..a42e7d7bf86b 100644
@@ -9639,7 +9639,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:

@@ -9657,10 +9657,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;

@@ -9679,9 +9679,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;

@@ -9692,20 +9689,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;

@@ -9773,9 +9767,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;
 

@@ -9821,10 +9813,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;
 

@@ -10969,33 +10957,6 @@ static const struct drm_crtc_helper_funcs intel_helper_funcs = {
 	.atomic_flush = intel_finish_crtc_commit,
 };
 
-/**
- * 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.

@@ -11526,7 +11487,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(dev, crtc) {

@@ -11832,11 +11792,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");
 	}
 }

@@ -11856,8 +11819,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");
 

@@ -11867,6 +11828,9 @@ 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,
+			     "encoder's stage crtc doesn't match current crtc\n");
 		}
 		/*
 		 * for MST connectors if we unplug the connector is gone

@@ -12296,11 +12260,11 @@ 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);

@@ -12313,14 +12277,10 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
 			}
 
 			connector_state->crtc = crtc;
-			connector_state->best_encoder = &encoder->base;
 		}
 	}
 
 	for_each_intel_crtc(dev, intel_crtc) {
-		if (intel_crtc->new_enabled == intel_crtc->base.enabled)
-			continue;
-
 		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
 		if (IS_ERR(crtc_state)) {
 			DRM_DEBUG_KMS("Failed to add [CRTC:%d] to state: %ld\n",

@@ -12329,9 +12289,6 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
 			continue;
 		}
 
-		crtc_state->base.active = crtc_state->base.enable =
-			intel_crtc->new_enabled;
-
 		if (&intel_crtc->base == crtc)
 			drm_mode_copy(&crtc_state->base.mode, &crtc->mode);
 	}

@@ -14552,99 +14509,224 @@ static bool primary_get_hw_state(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
 
-	if (!crtc->active)
+	if (!crtc->base.enabled)
 		return false;
 
 	return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE;
 }
 
-static void intel_modeset_readout_hw_state(struct drm_device *dev)
+static int readout_hw_crtc_state(struct drm_atomic_state *state,
+				 struct intel_crtc *crtc)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	enum pipe pipe;
-	struct intel_crtc *crtc;
-	struct intel_encoder *encoder;
-	struct intel_connector *connector;
-	int i;
+	struct drm_i915_private *dev_priv = to_i915(state->dev);
+	struct intel_crtc_state *crtc_state;
+	struct drm_plane *primary = crtc->base.primary;
+	struct drm_plane_state *drm_plane_state;
+	struct intel_plane_state *plane_state;
+	int ret;
 
-	for_each_intel_crtc(dev, crtc) {
-		struct drm_plane *primary = crtc->base.primary;

drivers/gpu/drm/i915/intel_display.c +207 -67

-		crtc->config->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE;
+	memset(crtc_state, 0, sizeof(*crtc_state));
+	crtc_state->base.crtc = &crtc->base;
+	crtc_state->base.state = state;
 
-		crtc->active = dev_priv->display.get_pipe_config(crtc,
-								 crtc->config);
+	crtc_state->quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE;
 
-		crtc->base.state->enable = crtc->active;
-		crtc->base.state->active = crtc->active;
-		crtc->base.enabled = crtc->active;
+	crtc_state->base.enable = crtc_state->base.active =
+	crtc->base.enabled = dev_priv->display.get_pipe_config(crtc, crtc_state);
 
-		plane_state = to_intel_plane_state(primary->state);
-		plane_state->visible = primary_get_hw_state(crtc);
+	/* update transitional state */
+	crtc->active = crtc_state->base.active;
+	crtc->config = crtc_state;
 
-		DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
-			      crtc->base.base.id,
-			      crtc->active ? "enabled" : "disabled");
-	}
+	drm_plane_state = drm_atomic_get_plane_state(state, primary);
+	if (IS_ERR(drm_plane_state))
+		return PTR_ERR(drm_plane_state);
+
+	plane_state = to_intel_plane_state(drm_plane_state);
+	plane_state->visible = primary_get_hw_state(crtc);
+
+	if (plane_state->visible)
+		crtc_state->base.plane_mask |= 1 << drm_plane_index(primary);
+	else
+		crtc_state->base.plane_mask &= ~(1 << drm_plane_index(primary));
+
+	DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
+		      crtc->base.base.id,
+		      crtc_state->base.active ? "enabled" : "disabled");
+
+	return 0;
+}
+
+static int readout_hw_pll_state(struct drm_atomic_state *state)
+{
+	struct drm_i915_private *dev_priv = to_i915(state->dev);
+	struct intel_shared_dpll_config *shared_dpll;
+	struct intel_crtc *crtc;
+	struct intel_crtc_state *crtc_state;
+	int i;
 
+	shared_dpll = intel_atomic_get_shared_dpll_state(state);
 	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
 		struct intel_shared_dpll *pll = &dev_priv->shared_dplls[i];
 
 		pll->on = pll->get_hw_state(dev_priv, pll,
-					    &pll->config.hw_state);
+					    &shared_dpll[i].hw_state);
+
 		pll->active = 0;
-		pll->config.crtc_mask = 0;
-		for_each_intel_crtc(dev, crtc) {
-			if (crtc->active && intel_crtc_to_shared_dpll(crtc) == pll) {
+		shared_dpll[i].crtc_mask = 0;
+
+		for_each_intel_crtc(state->dev, crtc) {
+			crtc_state = intel_atomic_get_crtc_state(state, crtc);
+			if (IS_ERR(crtc_state))
+				return PTR_ERR(crtc_state);
+
+			if (crtc_state->base.active &&
+			    crtc_state->shared_dpll == i) {
 				pll->active++;
-				pll->config.crtc_mask |= 1 << crtc->pipe;
+				shared_dpll[i].crtc_mask |=
+					1 << crtc->pipe;
 			}
 		}
 
 		DRM_DEBUG_KMS("%s hw state readout: crtc_mask 0x%08x, on %i\n",
-			      pll->name, pll->config.crtc_mask, pll->on);
+			      pll->name, shared_dpll[i].crtc_mask,
+			      pll->on);
 
-		if (pll->config.crtc_mask)
+		if (shared_dpll[i].crtc_mask)
 			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
 	}
 
-	for_each_intel_encoder(dev, encoder) {
-		pipe = 0;
+	return 0;
+}
 
-		if (encoder->get_hw_state(encoder, &pipe)) {
-			crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
-			encoder->base.crtc = &crtc->base;
-			encoder->get_config(encoder, crtc->config);
-		} else {
-			encoder->base.crtc = NULL;
-		}
+static struct drm_connector_state *
+get_connector_state_for_encoder(struct drm_atomic_state *state,
+				struct intel_encoder *encoder)
+{
+	struct drm_connector *connector;
+	struct drm_connector_state *connector_state;
+	int i;
 
-		encoder->connectors_active = false;
-		DRM_DEBUG_KMS("[ENCODER:%d:%s] hw state readout: %s, pipe %c\n",
-			      encoder->base.base.id,
-			      encoder->base.name,
-			      encoder->base.crtc ? "enabled" : "disabled",
-			      pipe_name(pipe));
-	}
+	for_each_connector_in_state(state, connector, connector_state, i)
+		if (connector_state->best_encoder == &encoder->base)
+			return connector_state;
+
+	return NULL;
+}
+
+static int readout_hw_connector_encoder_state(struct drm_atomic_state *state)
+{
+	struct drm_device *dev = state->dev;
+	struct drm_i915_private *dev_priv = to_i915(state->dev);
+	struct intel_crtc *crtc;
+	struct drm_crtc_state *drm_crtc_state;
+	struct intel_crtc_state *crtc_state;
+	struct intel_encoder *encoder;
+	struct intel_connector *connector;
+	struct drm_connector_state *connector_state;
+	enum pipe pipe;
 
 	for_each_intel_connector(dev, connector) {
+		connector_state =
+			drm_atomic_get_connector_state(state, &connector->base);
+		if (IS_ERR(connector_state))
+			return PTR_ERR(connector_state);
+
 		if (connector->get_hw_state(connector)) {
 			connector->base.dpms = DRM_MODE_DPMS_ON;
-			connector->encoder->connectors_active = true;
 			connector->base.encoder = &connector->encoder->base;
 		} else {
 			connector->base.dpms = DRM_MODE_DPMS_OFF;
 			connector->base.encoder = NULL;
 		}
+
+		/* We'll update the crtc field when reading encoder state */
+		connector_state->crtc = NULL;
+
+		connector_state->best_encoder = connector->base.encoder;
+
 		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] hw state readout: %s\n",
 			      connector->base.base.id,
 			      connector->base.name,
 			      connector->base.encoder ? "enabled" : "disabled");
 	}
+
+	for_each_intel_encoder(dev, encoder) {
+		pipe = 0;
+
+		connector_state =
+			get_connector_state_for_encoder(state, encoder);
+
+		encoder->connectors_active = !!connector_state;
+
+		if (encoder->get_hw_state(encoder, &pipe)) {
+			encoder->base.crtc =
+				dev_priv->pipe_to_crtc_mapping[pipe];
+			crtc = to_intel_crtc(encoder->base.crtc);
+
+			drm_crtc_state =
+				state->crtc_states[drm_crtc_index(&crtc->base)];
+			crtc_state = to_intel_crtc_state(drm_crtc_state);
+
+			encoder->get_config(encoder, crtc_state);
+
+			if (connector_state)
+				connector_state->crtc = &crtc->base;
+		} else {
+			encoder->base.crtc = NULL;
+		}
+
+		DRM_DEBUG_KMS("[ENCODER:%d:%s] hw state readout: %s, pipe %c\n",
+			      encoder->base.base.id,
+			      encoder->base.name,
+			      encoder->base.crtc ? "enabled" : "disabled",
+			      pipe_name(pipe));
+	}
+
+	return 0;
+}
+
+static struct drm_atomic_state *
+intel_modeset_readout_hw_state(struct drm_device *dev)
+{
+	struct intel_crtc *crtc;
+	int ret = 0;
+
+	struct drm_atomic_state *state;
+
+	state = drm_atomic_state_alloc(dev);
+	if (!state)
+		return ERR_PTR(-ENOMEM);
+
+	state->acquire_ctx = dev->mode_config.acquire_ctx;
+
+	for_each_intel_crtc(dev, crtc) {
+		ret = readout_hw_crtc_state(state, crtc);
+		if (ret)
+			goto err_free;
+	}
+
+	ret = readout_hw_pll_state(state);
+	if (ret)
+		goto err_free;
+
+	ret = readout_hw_connector_encoder_state(state);
+	if (ret)
+		goto err_free;
+
+	return state;
+
+err_free:
+	drm_atomic_state_free(state);
+	return ERR_PTR(ret);
 }
 
 /* Scan out the current hw modeset state, sanitizes it and maps it into the drm

@@ -14653,37 +14735,57 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 				  bool force_restore)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	enum pipe pipe;
-	struct intel_crtc *crtc;
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
 	struct intel_encoder *encoder;
+	struct drm_atomic_state *state;
+	struct intel_shared_dpll_config shared_dplls[I915_NUM_PLLS];
 	int i;
 
-	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);
-		}
+	state = intel_modeset_readout_hw_state(dev);
+	if (IS_ERR(state)) {
+		DRM_ERROR("Failed to read out hw state\n");
+		return;
 	}
 
+	drm_atomic_helper_swap_state(dev, state);
+
+	/* swap sw/hw dpll state */
+	intel_atomic_duplicate_dpll_state(dev_priv, shared_dplls);
+	intel_shared_dpll_commit(state);
+	memcpy(to_intel_atomic_state(state)->shared_dpll,
+	       shared_dplls, sizeof(*shared_dplls) * dev_priv->num_shared_dpll);
+
 	/* HW state is read out, now we need to sanitize this mess. */
 	for_each_intel_encoder(dev, encoder) {
 		intel_sanitize_encoder(encoder);
 	}
 
-	for_each_pipe(dev_priv, pipe) {
-		crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
-		intel_sanitize_crtc(crtc);
-		intel_dump_pipe_config(crtc, crtc->config,
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+
+		/* prevent unnneeded restores with force_restore */
+		crtc_state->active_changed =
+		crtc_state->mode_changed =
+		crtc_state->planes_changed = false;
+
+		if (crtc->enabled) {
+			intel_mode_from_pipe_config(&crtc->state->mode,
+				to_intel_crtc_state(crtc->state));
+
+			drm_mode_copy(&crtc->mode, &crtc->state->mode);
+			drm_mode_copy(&crtc->hwmode,
+				      &crtc->state->adjusted_mode);
+		}
+
+		intel_sanitize_crtc(intel_crtc);
+
+		/*
+		 * sanitize_crtc may have forced an update of crtc->state,
+		 * so reload in intel_dump_pipe_config
+		 */
+		intel_dump_pipe_config(intel_crtc,
+				       to_intel_crtc_state(crtc->state),
 				       "[setup_hw_state]");
 	}
 

@@ -14707,20 +14809,17 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 		ilk_wm_get_hw_state(dev);
 
 	if (force_restore) {
-		i915_redisable_vga(dev);
+		int ret;
 
-		/*
-		 * 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];
+		i915_redisable_vga(dev);
 
-			intel_crtc_restore_mode(crtc);
+		ret = intel_set_mode(state);
+		if (ret) {
+			DRM_ERROR("Failed to restore previous mode\n");
+			drm_atomic_state_free(state);
 		}
 	} else {
-		intel_modeset_update_staged_output_state(dev);
+		drm_atomic_state_free(state);
 	}
 
 	intel_modeset_check_state(dev);

drivers/gpu/drm/i915/intel_drv.h +2 -12

index 7012c67de8d5..65f5f3d41b5a 100644
@@ -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 *);

@@ -534,7 +523,6 @@ struct intel_crtc {
 
 	struct intel_initial_plane_config plane_config;
 	struct intel_crtc_state *config;
-	bool new_enabled;
 
 	/* reset counter value when the last flip was submitted */
 	unsigned int reset_counter;

@@ -1414,6 +1402,8 @@ 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);
+void intel_atomic_duplicate_dpll_state(struct drm_i915_private *,
+				       struct intel_shared_dpll_config *);
 
 static inline struct intel_crtc_state *
 intel_atomic_get_crtc_state(struct drm_atomic_state *state,

Signature · 6 lines

2.1.0

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

Quoted 267 lines

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

To make this work we load the new hardware state into the atomic_state, then swap it with the sw state.

This lets us change the force restore path in setup_hw_state() to use a single call to intel_mode_set() to restore all the previous state.

As a nice bonus this kills off encoder->new_encoder, connector->new_enabled and crtc->new_enabled. They were used only to restore the state after a modeset.

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

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

---
 drivers/gpu/drm/i915/intel_atomic.c  |   2 +-
 drivers/gpu/drm/i915/intel_display.c | 377 ++++++++++++++++++++++-------------
 drivers/gpu/drm/i915/intel_drv.h     |  14 +-
 3 files changed, 241 insertions(+), 152 deletions(-)

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

index 83078763ba14..17bf9e80c557 100644
@@ -395,7 +395,7 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
 	return 0;
 }
 
-static void
+void
 intel_atomic_duplicate_dpll_state(struct drm_i915_private *dev_priv,
 				  struct intel_shared_dpll_config *shared_dpll)
 {

drivers/gpu/drm/i915/intel_display.c +28 -70

index e7aa8610cbdc..a42e7d7bf86b 100644
@@ -9639,7 +9639,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:

@@ -9657,10 +9657,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;

@@ -9679,9 +9679,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;

@@ -9692,20 +9689,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;

@@ -9773,9 +9767,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;
 

@@ -9821,10 +9813,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;
 

@@ -10969,33 +10957,6 @@ static const struct drm_crtc_helper_funcs intel_helper_funcs = {
 	.atomic_flush = intel_finish_crtc_commit,
 };
 
-/**
- * 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.

@@ -11526,7 +11487,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(dev, crtc) {

@@ -11832,11 +11792,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");
 	}
 }

@@ -11856,8 +11819,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");
 

@@ -11867,6 +11828,9 @@ 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,
+			     "encoder's stage crtc doesn't match current crtc\n");
 		}
 		/*
 		 * for MST connectors if we unplug the connector is gone

@@ -12296,11 +12260,11 @@ 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);

@@ -12313,14 +12277,10 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
 			}
 
 			connector_state->crtc = crtc;
-			connector_state->best_encoder = &encoder->base;
 		}
 	}
 
 	for_each_intel_crtc(dev, intel_crtc) {
-		if (intel_crtc->new_enabled == intel_crtc->base.enabled)
-			continue;
-
 		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
 		if (IS_ERR(crtc_state)) {
 			DRM_DEBUG_KMS("Failed to add [CRTC:%d] to state: %ld\n",

@@ -12329,9 +12289,6 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
 			continue;
 		}
 
-		crtc_state->base.active = crtc_state->base.enable =
-			intel_crtc->new_enabled;
-
 		if (&intel_crtc->base == crtc)
 			drm_mode_copy(&crtc_state->base.mode, &crtc->mode);
 	}

@@ -14552,99 +14509,224 @@ static bool primary_get_hw_state(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
 
-	if (!crtc->active)
+	if (!crtc->base.enabled)
 		return false;
 
 	return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE;
 }
 
-static void intel_modeset_readout_hw_state(struct drm_device *dev)
+static int readout_hw_crtc_state(struct drm_atomic_state *state,
+				 struct intel_crtc *crtc)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	enum pipe pipe;
-	struct intel_crtc *crtc;
-	struct intel_encoder *encoder;
-	struct intel_connector *connector;
-	int i;
+	struct drm_i915_private *dev_priv = to_i915(state->dev);
+	struct intel_crtc_state *crtc_state;
+	struct drm_plane *primary = crtc->base.primary;
+	struct drm_plane_state *drm_plane_state;
+	struct intel_plane_state *plane_state;
+	int ret;
 
-	for_each_intel_crtc(dev, crtc) {
-		struct drm_plane *primary = crtc->base.primary;
-		struct intel_plane_state *plane_state;
+	crtc_state = intel_atomic_get_crtc_state(state, crtc);
+	if (IS_ERR(crtc_state))
+		return PTR_ERR(crtc_state);
 

drivers/gpu/drm/i915/intel_display.c static bool primary_get_hw_state(struct intel_crtc *crtc) · line 14534

Quoted 5 lines

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

-		memset(crtc->config, 0, sizeof(*crtc->config));
-		crtc->config->base.crtc = &crtc->base;
+	ret = drm_atomic_add_affected_planes(state, &crtc->base);
+	if (ret)
+		return ret;

Do we actually try to handle non-primary planes in this function? If I'm following this properly, at bootup we won't add any sprite or cursor planes here, even though it's possible that a graphics firmware has turned some of them on. It seems like our sw and hw states will still be a bit inconsistent in that case.

Assume the callers lock everything with drm_modeset_lock_all.

This change had to be done after converting suspend/resume to use atomic_state so the atomic state is preserved, otherwise all transitional state is erased.

Now all callers of .crtc_enable and .crtc_disable go through atomic modeset! :-D

Changes since v1:
- Only check for crtc_state->active in valleyview_modeset_global_pipes.
- Only check for crtc_state->active in modeset_update_crtc_power_domains. Changes since v2:
- Rework on top of the changed patch order.
Changes since v3:
- Rename intel_crtc_toggle in description to *_control
- Change return value to int.
- Do not add plane state, should be done implicitly already.

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

---
 drivers/gpu/drm/i915/intel_display.c | 53 ++++++++++++++++++++++--------------
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-
 2 files changed, 33 insertions(+), 22 deletions(-)

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

index e9680eb85bd0..32bab28432f7 100644

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

 {
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	enum intel_display_power_domain domain;
-	unsigned long domains;
+	struct intel_crtc_state *pipe_config;
+	struct drm_atomic_state *state;
+	int ret;
 
 	if (enable == intel_crtc->active)
-		return;
+		return 0;
 
 	if (enable && !crtc->state->enable)
-		return;
+		return 0;

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

+	lockdep_assert_held(&ctx->ww_ctx);
 
-		dev_priv->display.crtc_enable(crtc);
-		intel_crtc_enable_planes(crtc);
-	} else {
-		intel_crtc_disable_planes(crtc);
-		dev_priv->display.crtc_disable(crtc);
+	state = drm_atomic_state_alloc(dev);
+	if (WARN_ON(!state))
+		return -ENOMEM;
 
-		domains = intel_crtc->enabled_power_domains;
-		for_each_power_domain(domain, domains)
-			intel_display_power_put(dev_priv, domain);
-		intel_crtc->enabled_power_domains = 0;
+	state->acquire_ctx = ctx;
+	state->allow_modeset = true;
+
+	pipe_config = intel_atomic_get_crtc_state(state, intel_crtc);
+	if (IS_ERR(pipe_config)) {
+		ret = PTR_ERR(pipe_config);
+		goto err;
 	}
+	pipe_config->base.active = enable;
+
+	ret = intel_set_mode(state);
+	if (!ret)
+		return ret;
+
+err:
+	DRM_ERROR("Updating crtc active failed with %i\n", ret);
+	drm_atomic_state_free(state);
+	return ret;
 }
 
 /**

drivers/gpu/drm/i915/intel_drv.h +1 -1

index 60a29ff65f1f..c113f187090f 100644
@@ -989,7 +989,7 @@ 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);
-void intel_crtc_control(struct drm_crtc *crtc, bool enable);
+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);
 int intel_connector_init(struct intel_connector *);

Signature · 7 lines

2.1.0


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

Quoted 32 lines

Assume the callers lock everything with drm_modeset_lock_all.

This change had to be done after converting suspend/resume to use atomic_state so the atomic state is preserved, otherwise all transitional state is erased.

Now all callers of .crtc_enable and .crtc_disable go through atomic modeset! :-D

Changes since v1:
- Only check for crtc_state->active in valleyview_modeset_global_pipes.
- Only check for crtc_state->active in modeset_update_crtc_power_domains. Changes since v2:
- Rework on top of the changed patch order.
Changes since v3:
- Rename intel_crtc_toggle in description to *_control
- Change return value to int.
- Do not add plane state, should be done implicitly already.

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

---
 drivers/gpu/drm/i915/intel_display.c | 53 ++++++++++++++++++++++--------------
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-
 2 files changed, 33 insertions(+), 22 deletions(-)

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

index e9680eb85bd0..32bab28432f7 100644
@@ -5973,38 +5973,49 @@ void intel_display_suspend(struct drm_device *dev)
 }
 

drivers/gpu/drm/i915/intel_display.c void intel_display_suspend(struct drm_device *dev) · line 5976

Quoted 3 lines

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

 /* Master function to enable/disable CRTC and corresponding power wells */
-void intel_crtc_control(struct drm_crtc *crtc, bool enable)
+int intel_crtc_control(struct drm_crtc *crtc, bool enable)

We change the return value to 'int' here, but we never check the error code anywhere. Maybe we should hold this off for another patch and then also propagate the return value back up the call chain? It seems like at least some of the connector-specific DPMS functions should recognize the failure of intel_crtc_update_dpms() before trying to continue with other operations.

Quoted 20 lines

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

 {
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_mode_config *config = &dev->mode_config;
+	struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	enum intel_display_power_domain domain;
-	unsigned long domains;
+	struct intel_crtc_state *pipe_config;
+	struct drm_atomic_state *state;
+	int ret;
 
 	if (enable == intel_crtc->active)
-		return;
+		return 0;
 
 	if (enable && !crtc->state->enable)
-		return;
+		return 0;
 

drivers/gpu/drm/i915/intel_display.c void intel_display_suspend(struct drm_device *dev) · line 5994

Quoted 9 lines

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

-	crtc->state->active = enable;
-	if (enable) {
-		domains = get_crtc_power_domains(crtc);
-		for_each_power_domain(domain, domains)
-			intel_display_power_get(dev_priv, domain);
-		intel_crtc->enabled_power_domains = domains;
+	/* this function should be called with drm_modeset_lock_all for now */
+	if (WARN_ON(!ctx))
+		return -EIO;

EIO seems like an unusual choice here (but I'm not really sure what the best option would be).