* [PATCH] drm/i915: Update state before setting watermarks.
@ 2016-02-24 13:54 Maarten Lankhorst
2016-02-24 14:13 ` Tvrtko Ursulin
2016-02-29 14:08 ` Ander Conselvan De Oliveira
0 siblings, 2 replies; 4+ messages in thread
From: Maarten Lankhorst @ 2016-02-24 13:54 UTC (permalink / raw)
To: Intel Graphics Development
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@linux.intel.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] drm/i915: Update state before setting watermarks.
2016-02-24 13:54 [PATCH] drm/i915: Update state before setting watermarks Maarten Lankhorst
@ 2016-02-24 14:13 ` Tvrtko Ursulin
2016-02-29 14:08 ` Ander Conselvan De Oliveira
1 sibling, 0 replies; 4+ messages in thread
From: Tvrtko Ursulin @ 2016-02-24 14:13 UTC (permalink / raw)
To: Maarten Lankhorst, Intel Graphics Development
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@linux.intel.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Thanks for looking into this! Also,
Tested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] drm/i915: Update state before setting watermarks.
2016-02-24 13:54 [PATCH] drm/i915: Update state before setting watermarks Maarten Lankhorst
2016-02-24 14:13 ` Tvrtko Ursulin
@ 2016-02-29 14:08 ` Ander Conselvan De Oliveira
2016-03-02 14:48 ` [PATCH v1.1] drm/i915: Update state before setting watermarks, v2 Maarten Lankhorst
1 sibling, 1 reply; 4+ messages in thread
From: Ander Conselvan De Oliveira @ 2016-02-29 14:08 UTC (permalink / raw)
To: Maarten Lankhorst, Intel Graphics Development
On Wed, 2016-02-24 at 14:54 +0100, 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.
I think there's a "in hw state readout" missing somewhere in the commit message.
Even better if the commit message explained that the state was fixed up too
late, after the call to intel_update_watermarks() was already done. It would be
more informative than "should show as disabled".
>
> ------------[ 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@linux.intel.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 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);
Would it make sense to do this before intel_fbc_disable() and ->crtc_disable()?
The bug here was a problem of calling into the lower layers with a bad state, so
I think it would be good to first take care of that before calling any other
functions.
With improved commit message, this is
Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>
Ander
> +
> + 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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH v1.1] drm/i915: Update state before setting watermarks, v2.
2016-02-29 14:08 ` Ander Conselvan De Oliveira
@ 2016-03-02 14:48 ` Maarten Lankhorst
0 siblings, 0 replies; 4+ messages in thread
From: Maarten Lankhorst @ 2016-03-02 14:48 UTC (permalink / raw)
To: Ander Conselvan De Oliveira, Intel Graphics Development
When intel_update_watermarks is called on skylake from the hw
state readout disable function it calls intel_update_watermarks.
intel_update_watermarks inspects crtc->state, which should be
set to disabled.
This wasn't the case, and this resulted in a divide-by-zero in
skl_update_wm when intel_update_watermarks got 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 ]------------
Changes since v1:
- Clear state before calling any function after .crtc_disable.
Reported-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3e6d51e00f56..61b473dc8ed4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6486,6 +6486,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;
@@ -6504,7 +6505,20 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
}
dev_priv->display.crtc_disable(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;
intel_crtc->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_fbc_disable(intel_crtc);
intel_update_watermarks(crtc);
intel_disable_shared_dpll(intel_crtc);
@@ -15800,38 +15814,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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-03-02 14:48 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-02-24 13:54 [PATCH] drm/i915: Update state before setting watermarks Maarten Lankhorst
2016-02-24 14:13 ` Tvrtko Ursulin
2016-02-29 14:08 ` Ander Conselvan De Oliveira
2016-03-02 14:48 ` [PATCH v1.1] drm/i915: Update state before setting watermarks, v2 Maarten Lankhorst
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.