Re: [PATCH] drm/i915: Update state before setting watermarks.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 24/02/16 13:54, Maarten Lankhorst wrote:
When intel_update_watermarks is called on skylake it inspects
crtc->state, which should show as disabled. This wasn't the case,
and this resulted in a divide-by-zero in
skl_ddb_get_pipe_allocation_limits when intel_update_watermarks was called.

  ------------[ cut here ]------------
  WARNING: CPU: 1 PID: 295 at drivers/gpu/drm/i915/intel_pm.c:2834 skl_update_pipe_wm+0x102/0x8c0 [i915]()
  WARN_ON(!config->num_pipes_active)
  Modules linked in: coretemp i915(+) xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
  CPU: 1 PID: 295 Comm: systemd-udevd Tainted: G     U  W       4.5.0-rc4-xxxxxx #25
  Hardware name: xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
   0000000000000000 ffff88003777f5a8 ffffffff813485c2 ffff88003777f5f0
   ffffffffa0236240 ffff88003777f5e0 ffffffff81050fce ffff8800aa420000
   ffff8800aba18000 ffff8800aba18000 ffff880037304c00 ffff8800aa420000
  Call Trace:
   [<ffffffff813485c2>] dump_stack+0x67/0x95
   [<ffffffff81050fce>] warn_slowpath_common+0x9e/0xc0
   [<ffffffff8105103c>] warn_slowpath_fmt+0x4c/0x50
   [<ffffffff8106945e>] ? flush_work+0x8e/0x280
   [<ffffffff810693d5>] ? flush_work+0x5/0x280
   [<ffffffffa016add2>] skl_update_pipe_wm+0x102/0x8c0 [i915]
   [<ffffffffa016b96f>] skl_update_wm+0xff/0x5f0 [i915]
   [<ffffffff810928ee>] ? trace_hardirqs_on_caller+0x15e/0x1d0
   [<ffffffff8109296d>] ? trace_hardirqs_on+0xd/0x10
   [<ffffffffa016ce6e>] intel_update_watermarks+0x1e/0x30 [i915]
   [<ffffffffa01d3ee2>] intel_crtc_disable_noatomic+0xd2/0x150 [i915]
   [<ffffffffa01dd3d2>] intel_modeset_setup_hw_state+0xdd2/0xde0 [i915]
   [<ffffffffa01dfd83>] intel_modeset_init+0x15a3/0x1950 [i915]
   [<ffffffffa02160b6>] i915_driver_load+0x13c6/0x1720 [i915]
   [<ffffffff81522160>] ? add_sysfs_fw_map_entry+0x9b/0x9b
   [<ffffffffa00b15ef>] drm_dev_register+0x6f/0xb0 [drm]
   [<ffffffffa00b3b3a>] drm_get_pci_dev+0x10a/0x1d0 [drm]
   [<ffffffffa01582d9>] i915_pci_probe+0x49/0x50 [i915]
   [<ffffffff8138ae30>] pci_device_probe+0x80/0xf0
   [<ffffffff8143e2ac>] driver_probe_device+0x1bc/0x3d0
   [<ffffffff8143e526>] __driver_attach+0x66/0x90
   [<ffffffff8143e4c0>] ? driver_probe_device+0x3d0/0x3d0
   [<ffffffff8143be3b>] bus_for_each_dev+0x5b/0xa0
   [<ffffffff8143db3e>] driver_attach+0x1e/0x20
   [<ffffffff8143d461>] bus_add_driver+0x151/0x270
   [<ffffffff8143eabc>] driver_register+0x8c/0xd0
   [<ffffffff8138a2ed>] __pci_register_driver+0x5d/0x60
   [<ffffffffa00b3c58>] drm_pci_init+0x58/0xf0 [drm]
   [<ffffffff8109296d>] ? trace_hardirqs_on+0xd/0x10
   [<ffffffffa02aa000>] ? 0xffffffffa02aa000
   [<ffffffffa02aa094>] i915_init+0x94/0x9b [i915]
   [<ffffffff81000423>] do_one_initcall+0x113/0x1f0
   [<ffffffff810a4b21>] ? rcu_read_lock_sched_held+0x61/0x90
   [<ffffffff811601dc>] ? kmem_cache_alloc_trace+0x1cc/0x280
   [<ffffffff8111110a>] do_init_module+0x60/0x1c8
   [<ffffffff810c731b>] load_module+0x1ceb/0x2410
   [<ffffffff810c3a60>] ? store_uevent+0x40/0x40
   [<ffffffff811763d1>] ? kernel_read+0x41/0x60
   [<ffffffff810c7c1d>] SYSC_finit_module+0x8d/0xa0
   [<ffffffff810c7c4e>] SyS_finit_module+0xe/0x10
   [<ffffffff815f1e97>] entry_SYSCALL_64_fastpath+0x12/0x6f
  ---[ end trace 1149e9ab3695a423 ]---
  ------------[ cut here ]------------

Reported-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>

Thanks for looking into this! Also,

Tested-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>

Regards,

Tvrtko

---
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index deee56010eee..d9e0470419a1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6358,6 +6358,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)

  static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
  {
+	struct intel_encoder *encoder;
  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
  	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
  	enum intel_display_power_domain domain;
@@ -6378,6 +6379,19 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
  	dev_priv->display.crtc_disable(crtc);
  	intel_crtc->active = false;
  	intel_fbc_disable(intel_crtc);
+
+	DRM_DEBUG_KMS("[CRTC:%d] hw state adjusted, was enabled, now disabled\n",
+		      crtc->base.id);
+
+	WARN_ON(drm_atomic_set_mode_for_crtc(crtc->state, NULL) < 0);
+	crtc->state->active = false;
+	crtc->enabled = false;
+	crtc->state->connector_mask = 0;
+	crtc->state->encoder_mask = 0;
+
+	for_each_encoder_on_crtc(crtc->dev, crtc, encoder)
+		encoder->base.crtc = NULL;
+
  	intel_update_watermarks(crtc);
  	intel_disable_shared_dpll(intel_crtc);

@@ -15526,38 +15540,9 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)

  	/* Adjust the state of the output pipe according to whether we
  	 * have active connectors/encoders. */
-	if (!intel_crtc_has_encoders(crtc))
+	if (crtc->active && !intel_crtc_has_encoders(crtc))
  		intel_crtc_disable_noatomic(&crtc->base);

-	if (crtc->active != crtc->base.state->active) {
-		struct intel_encoder *encoder;
-
-		/* This can happen either due to bugs in the get_hw_state
-		 * functions or because of calls to intel_crtc_disable_noatomic,
-		 * or because the pipe is force-enabled due to the
-		 * pipe A quirk. */
-		DRM_DEBUG_KMS("[CRTC:%d] hw state adjusted, was %s, now %s\n",
-			      crtc->base.base.id,
-			      crtc->base.state->enable ? "enabled" : "disabled",
-			      crtc->active ? "enabled" : "disabled");
-
-		WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, NULL) < 0);
-		crtc->base.state->active = crtc->active;
-		crtc->base.enabled = crtc->active;
-		crtc->base.state->connector_mask = 0;
-		crtc->base.state->encoder_mask = 0;
-
-		/* Because we only establish the connector -> encoder ->
-		 * crtc links if something is active, this means the
-		 * crtc is now deactivated. Break the links. connector
-		 * -> encoder links are only establish when things are
-		 *  actually up, hence no need to break them. */
-		WARN_ON(crtc->active);
-
-		for_each_encoder_on_crtc(dev, &crtc->base, encoder)
-			encoder->base.crtc = NULL;
-	}
-
  	if (crtc->active || HAS_GMCH_DISPLAY(dev)) {
  		/*
  		 * We start out with underrun reporting disabled to avoid races.


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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux