* [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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox