drm/i915: Update state before setting watermarks.
Subsystems (2)
PATCH 1/1 drm/i915: Update state before setting watermarks.
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 <[email protected]>
Signed-off-by Maarten Lankhorst <[email protected]>
---
drivers/gpu/drm/i915/intel_display.c +15 -30
@@ -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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 59 lines · Maarten Lankhorst
When
intel_update_watermarksis called on skylake it inspectscrtc->state, which should show as disabled. This wasn't the case, and this resulted in a divide-by-zero inskl_ddb_get_pipe_allocation_limitswhenintel_update_watermarkswas 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 ]------------
Quoted 2 lines
Reported-by Tvrtko Ursulin <[email protected]>Signed-off-by Maarten Lankhorst <[email protected]>
Thanks for looking into this! Also,
Tested-by Tvrtko Ursulin <[email protected]>
Regards,
Tvrtko
Quoted 65 lines
---
drivers/gpu/drm/i915/intel_display.c +15
-23@@ -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
drivers/gpu/drm/i915/intel_display.c static void intel_sanitize_crtc(struct intel_crtc *crtc) · line 15548
Quoted 10 lines
drivers/gpu/drm/i915/intel_display.c +0
-7-* 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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 4 lines · Maarten Lankhorst
When
intel_update_watermarksis called on skylake it inspectscrtc->state, which should show as disabled. This wasn't the case, and this resulted in a divide-by-zero inskl_ddb_get_pipe_allocation_limitswhenintel_update_watermarkswas 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".
Quoted 75 lines
------------[ 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 <[email protected]>Signed-off-by Maarten Lankhorst <[email protected]>---
drivers/gpu/drm/i915/intel_display.c +1
-0@@ -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;
drivers/gpu/drm/i915/intel_display.c static void intel_crtc_disable_noatomic(struct drm_crtc · line 6382
Quoted 4 lines
drivers/gpu/drm/i915/intel_display.c +0
-0@@ -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 <[email protected]>
Ander
Quoted 64 lines
drivers/gpu/drm/i915/intel_display.c +13
-0+ +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);drivers/gpu/drm/i915/intel_display.c +1
-11intel_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",
drivers/gpu/drm/i915/intel_display.c +0
-3- crtc->active ? "enabled" : "disabled"); - - WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, NULL)< 0);
drivers/gpu/drm/i915/intel_display.c +0
-16- 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.
Quoted 4 lines
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 1/1 drm/i915: Update state before setting watermarks, v2.
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 <[email protected]>
Signed-off-by Maarten Lankhorst <[email protected]>
---
drivers/gpu/drm/i915/intel_display.c +15 -30
@@ -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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx