[Intel-gfx] ✗ Fi.CI.IGT: warning for PSR lag fixes
== Series Details == Series: PSR lag fixes URL : https://patchwork.freedesktop.org/series/38067/ State : warning == Summary == Test perf: Subgroup oa-exponents: pass -> FAIL (shard-apl) fdo#102254 Subgroup polling: fail -> PASS (shard-hsw) fdo#102252 Test kms_pipe_crc_basic: Subgroup suspend-read-crc-pipe-b: pass -> DMESG-WARN (shard-snb) fdo#103375 Test kms_frontbuffer_tracking: Subgroup fbc-rgb565-draw-mmap-cpu: pass -> SKIP (shard-snb) Test kms_flip: Subgroup 2x-flip-vs-absolute-wf_vblank-interruptible: fail -> PASS (shard-hsw) fdo#100368 fdo#102254 https://bugs.freedesktop.org/show_bug.cgi?id=102254 fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252 fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375 fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368 shard-apltotal:3422 pass:1773 dwarn:1 dfail:0 fail:21 skip:1626 time:12416s shard-hswtotal:3444 pass:1761 dwarn:1 dfail:0 fail:10 skip:1671 time:11970s shard-snbtotal:3444 pass:1349 dwarn:2 dfail:0 fail:10 skip:2083 time:6604s Blacklisted hosts: shard-kbltotal:3444 pass:1899 dwarn:13 dfail:0 fail:25 skip:1507 time:9794s == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7975/shards.html ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✓ Fi.CI.BAT: success for PSR lag fixes
== Series Details == Series: PSR lag fixes URL : https://patchwork.freedesktop.org/series/38067/ State : success == Summary == Series 38067v1 PSR lag fixes https://patchwork.freedesktop.org/api/1.0/series/38067/revisions/1/mbox/ Test debugfs_test: Subgroup read_all_entries: pass -> INCOMPLETE (fi-snb-2520m) fdo#103713 Test kms_pipe_crc_basic: Subgroup suspend-read-crc-pipe-a: dmesg-warn -> PASS (fi-cnl-y3) fdo#103191 fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713 fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191 fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:418s fi-bdw-gvtdvmtotal:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:423s fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:375s fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:481s fi-bwr-2160 total:288 pass:183 dwarn:0 dfail:0 fail:0 skip:105 time:286s fi-bxt-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:482s fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:483s fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:469s fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:456s fi-cfl-s2total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:565s fi-cnl-y3total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:581s fi-elk-e7500 total:288 pass:229 dwarn:0 dfail:0 fail:0 skip:59 time:411s fi-gdg-551 total:288 pass:180 dwarn:0 dfail:0 fail:0 skip:108 time:285s fi-glk-1 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:508s fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:387s fi-ilk-650 total:288 pass:228 dwarn:0 dfail:0 fail:0 skip:60 time:419s fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:446s fi-ivb-3770 total:288 pass:255 dwarn:0 dfail:0 fail:0 skip:33 time:410s fi-kbl-7500u total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:462s fi-kbl-7560u total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:496s fi-kbl-r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:501s fi-pnv-d510 total:288 pass:222 dwarn:1 dfail:0 fail:0 skip:65 time:599s fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:432s fi-skl-6600u total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:504s fi-skl-6700hqtotal:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:527s fi-skl-6700k2total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:488s fi-skl-6770hqtotal:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:472s fi-skl-guc total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:413s fi-skl-gvtdvmtotal:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:431s fi-snb-2520m total:3pass:2dwarn:0 dfail:0 fail:0 skip:0 fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:394s Blacklisted hosts: fi-glk-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:466s 4d059b70de402af68016510ae19a70c750806159 drm-tip: 2018y-02m-11d-17h-16m-33s UTC integration manifest 949c0e7a3444 drm/i915/psr: Wait for PSR transition to complete before exiting. a084e969804e drm/i915/psr: HW tracking for cursor moves to fix lags. 6c0f580e8e39 drm/i915/psr: Use more PSR HW tracking. == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7975/issues.html ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/3] drm/i915/psr: Use more PSR HW tracking.
From: Rodrigo ViviSo far we are using frontbuffer tracking for everything and ignoring that PSR has a HW capable HW tracking for many modern usages of GPU on Core platforms and newer Atom ones. One reason for that is that we were trying to keep same infrastructure in place for VLV/CHV than the rest of platforms. But also because when this infrastructure was created the front-buffer-tracking origin wasn't that good and stable how it is today after Paulo reworked it to attend FBC cases. However this PSR implementation without HW tracking died on gen8LP. And newer platforms are starting to demand more HW tracking specially with PSR2 cases in mind. By disabling and re-enabling PSR totally every time we believe someone is going to change the front buffer content we don't allow PSR HW tracking to do this job and specially compromising the whole idea of PSR2 case where the HW tracking detect only the damaged area and do a partial screen update. So, from now on, on the platforms that has hw_tracking let's rely more on HW tracking. This also is the case in used by other drivers and more validated by SV teams. So I hope that this will lead us to less misterious bugs. v2: Only do this for platform that actually has hw tracking. v3 from DK Do this only for flips, small gradual changes are better. Cc: Dhinakaran Pandiyan Cc: Jim Bride Cc: Vathsala Nagaraju Signed-off-by: Rodrigo Vivi Signed-off-by: Dhinakaran Pandiyan --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_drv.h | 3 ++- drivers/gpu/drm/i915/intel_frontbuffer.c | 2 +- drivers/gpu/drm/i915/intel_psr.c | 10 +- 4 files changed, 13 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 70cf289855af..19d5ac4921e5 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -770,6 +770,7 @@ struct i915_psr { bool y_cord_support; bool colorimetry_support; bool alpm; + bool has_hw_tracking; void (*enable_source)(struct intel_dp *, const struct intel_crtc_state *); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 4b1d357a43ae..6f9a7a81005f 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1860,7 +1860,8 @@ void intel_psr_enable(struct intel_dp *intel_dp, void intel_psr_disable(struct intel_dp *intel_dp, const struct intel_crtc_state *old_crtc_state); void intel_psr_invalidate(struct drm_i915_private *dev_priv, - unsigned frontbuffer_bits); + unsigned frontbuffer_bits, + enum fb_op_origin origin); void intel_psr_flush(struct drm_i915_private *dev_priv, unsigned frontbuffer_bits, enum fb_op_origin origin); diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c index fcfc217e754e..efda1af9a5b3 100644 --- a/drivers/gpu/drm/i915/intel_frontbuffer.c +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c @@ -79,7 +79,7 @@ void __intel_fb_obj_invalidate(struct drm_i915_gem_object *obj, spin_unlock(_priv->fb_tracking.lock); } - intel_psr_invalidate(dev_priv, frontbuffer_bits); + intel_psr_invalidate(dev_priv, frontbuffer_bits, origin); intel_edp_drrs_invalidate(dev_priv, frontbuffer_bits); intel_fbc_invalidate(dev_priv, frontbuffer_bits, origin); } diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index 2ef374f936b9..2a31c7cbdb41 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -824,6 +824,7 @@ void intel_psr_single_frame_update(struct drm_i915_private *dev_priv, * intel_psr_invalidate - Invalidade PSR * @dev_priv: i915 device * @frontbuffer_bits: frontbuffer plane tracking bits + * @origin: which operation caused the invalidate * * Since the hardware frontbuffer tracking has gaps we need to integrate * with the software frontbuffer tracking. This function gets called every @@ -833,7 +834,7 @@ void intel_psr_single_frame_update(struct drm_i915_private *dev_priv, * Dirty frontbuffers relevant to PSR are tracked in busy_frontbuffer_bits." */ void intel_psr_invalidate(struct drm_i915_private *dev_priv, - unsigned frontbuffer_bits) + unsigned frontbuffer_bits, enum fb_op_origin origin) { struct drm_crtc *crtc; enum pipe pipe; @@ -841,6 +842,9 @@ void intel_psr_invalidate(struct drm_i915_private *dev_priv, if (!CAN_PSR(dev_priv)) return; + if (dev_priv->psr.has_hw_tracking && origin == ORIGIN_FLIP) +
[Intel-gfx] [PATCH 3/3] drm/i915/psr: Wait for PSR transition to complete before exiting.
With fbdev, screen freezes after a few continuous PSR exit->enter cycles. Printing out the PSR status register clearly showed this freeze coincided with exiting when the hardware is in a transitory state. So wait for 100 ms (~6 frames) for PSR to become active and then exit. Cc: Rodrigo ViviSigned-off-by: Dhinakaran Pandiyan --- drivers/gpu/drm/i915/intel_psr.c | 12 1 file changed, 12 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index ddfabdff3dea..7ef6bd01014d 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -738,6 +738,18 @@ static void intel_psr_exit(struct drm_i915_private *dev_priv) WARN_ON(!(val & EDP_PSR2_ENABLE)); I915_WRITE(EDP_PSR2_CTL, val & ~EDP_PSR2_ENABLE); } else { + + /* Wait for about 6 frames in case we just enabled PSR, +* this prevents the screen from freezing as the HW does +* not seem to be able to back off cleanly it is already +* trying to enter PSR. +*/ + intel_wait_for_register(dev_priv, + EDP_PSR_STATUS, + EDP_PSR_STATUS_STATE_MASK, + EDP_PSR_STATUS_STATE_SRDENT, + 100); + val = I915_READ(EDP_PSR_CTL); WARN_ON(!(val & EDP_PSR_ENABLE)); I915_WRITE(EDP_PSR_CTL, val & ~EDP_PSR_ENABLE); -- 2.14.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 0/3] PSR lag fixes
PSR currently when enabled results in semi-permanent freezes or noticeable cursor lags. https://patchwork.freedesktop.org/series/37598/ will fix long freezes due to frame counter resets. This series has three more fixes - Patch 1 eliminates PSR exit for flips and makes us rely on the HW to do it. Patch 2 fixes cusor move lag by relying on HW to exit PSR. Patch 3 fixes temporary freeze seen with fbdev. With both the series applied, PSR on my SKL ThinkPad feels pretty good. Dhinakaran Pandiyan (2): drm/i915/psr: HW tracking for cursor moves to fix lags. drm/i915/psr: Wait for PSR transition to complete before exiting. Rodrigo Vivi (1): drm/i915/psr: Use more PSR HW tracking. drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/i915_gem.c | 2 +- drivers/gpu/drm/i915/intel_drv.h | 3 ++- drivers/gpu/drm/i915/intel_frontbuffer.c | 2 +- drivers/gpu/drm/i915/intel_psr.c | 24 +++- 5 files changed, 29 insertions(+), 4 deletions(-) -- 2.14.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/3] drm/i915/psr: HW tracking for cursor moves to fix lags.
DRM_IOCTL_MODE_CURSOR results in a frontbuffer flush before the cursor plane MMIOs are written to. But this flush is not necessary for PSR as hardware tracking takes care of exiting PSR when the MMIO's are written. Introduce a new fb_op_origin enum to differentiate these flushes from those originating due to a dirty fbdev buffer and ignore this enum in psr_flush and psr_invalidate. Cc: Rodrigo ViviSigned-off-by: Dhinakaran Pandiyan --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_gem.c | 2 +- drivers/gpu/drm/i915/intel_psr.c | 6 -- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 19d5ac4921e5..158e774ed2e6 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -637,6 +637,7 @@ enum fb_op_origin { ORIGIN_CS, ORIGIN_FLIP, ORIGIN_DIRTYFB, + ORIGIN_PINNEDFB, }; struct intel_fbc { diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index fc68b35854df..43146699c497 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4141,7 +4141,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj, /* Treat this as an end-of-frame, like intel_user_framebuffer_dirty() */ __i915_gem_object_flush_for_display(obj); - intel_fb_obj_flush(obj, ORIGIN_DIRTYFB); + intel_fb_obj_flush(obj, ORIGIN_PINNEDFB); /* It should now be out of any other write domains, and we can update * the domain values for our changes. diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index 2a31c7cbdb41..ddfabdff3dea 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -842,7 +842,8 @@ void intel_psr_invalidate(struct drm_i915_private *dev_priv, if (!CAN_PSR(dev_priv)) return; - if (dev_priv->psr.has_hw_tracking && origin == ORIGIN_FLIP) + if (dev_priv->psr.has_hw_tracking && + (origin == ORIGIN_FLIP || origin == ORIGIN_PINNEDFB)) return; mutex_lock(_priv->psr.lock); @@ -885,7 +886,8 @@ void intel_psr_flush(struct drm_i915_private *dev_priv, if (!CAN_PSR(dev_priv)) return; - if (dev_priv->psr.has_hw_tracking && origin == ORIGIN_FLIP) + if (dev_priv->psr.has_hw_tracking && + (origin == ORIGIN_FLIP || origin == ORIGIN_PINNEDFB)) return; mutex_lock(_priv->psr.lock); -- 2.14.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/5] Fix deadlock on runtime suspend in DRM drivers
On Mon, Feb 12, 2018 at 12:35:51AM +, Mike Lothian wrote: > I've not been able to reproduce the original problem you're trying to > solve on amdgpu thats with or without your patch set and the above > "trigger" too > > Is anything else required to trigger it, I started multiple DRI_PRIME > glxgears, in parallel, serial waiting the 12 seconds and serial within > the 12 seconds and I couldn't reproduce it The discrete GPU needs to runtime suspend, that's the trigger, so no DRI_PRIME executables should be running. Just let it autosuspend after boot. Do you see "waiting 12 sec" messages in dmesg? If not it's not autosuspending. Thanks, Lukas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/5] Fix deadlock on runtime suspend in DRM drivers
Hi I've not been able to reproduce the original problem you're trying to solve on amdgpu thats with or without your patch set and the above "trigger" too Is anything else required to trigger it, I started multiple DRI_PRIME glxgears, in parallel, serial waiting the 12 seconds and serial within the 12 seconds and I couldn't reproduce it Regards Mike ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/5] Fix deadlock on runtime suspend in DRM drivers
On Sun, Feb 11, 2018 at 08:23:14PM +0100, Lukas Wunner wrote: > On Sun, Feb 11, 2018 at 06:58:11PM +, Mike Lothian wrote: > > On 11 February 2018 at 09:38, Lukas Wunnerwrote: > > > The patches for radeon and amdgpu are compile-tested only, I only have a > > > MacBook Pro with an Nvidia GK107 to test. To test the patches, add an > > > "msleep(12*1000);" at the top of the driver's ->runtime_suspend hook. > > > This ensures that the poll worker runs after ->runtime_suspend has begun. > > > Wait 12 sec after the GPU has begun runtime suspend, then check > > > /sys/bus/pci/devices/:01:00.0/power/runtime_status. Without this > > > series, the status will be stuck at "suspending" and you'll get hung task > > > errors in dmesg after a few minutes. > > > > I wasn't quite sure where to add that msleep. I've tested the patches > > as is on top of agd5f's wip branch without ill effects > > > > I've had a radeon and now a amdgpu PRIME setup and don't believe I've > > ever seen this issue > > > > If you could pop a patch together for the msleep I'll give it a test on > > amdgpu > > Here you go, this is for all 3 drivers. > Should deadlock without the series. > Thanks! Sorry, I missed that amdgpu_drv.c and radeon_drv.c don't include delay.h, rectified testing patch below: diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index 50afcf6..beaaf2c 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -36,6 +36,7 @@ #include #include +#include #include #include #include @@ -718,6 +719,9 @@ static int amdgpu_pmops_runtime_suspend(struct device *dev) return -EBUSY; } + printk("waiting 12 sec\n"); + msleep(12*1000); + printk("done waiting 12 sec\n"); drm_dev->switch_power_state = DRM_SWITCH_POWER_CHANGING; drm_kms_helper_poll_disable(drm_dev); vga_switcheroo_set_dynamic_switch(pdev, VGA_SWITCHEROO_OFF); diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 555fbe5..ee7cf0d 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -586,6 +586,7 @@ static void output_poll_execute(struct work_struct *work) repoll = true; goto out; } + dev_info(>pdev->dev, "begin poll\n"); drm_connector_list_iter_begin(dev, _iter); drm_for_each_connector_iter(connector, _iter) { @@ -651,6 +652,7 @@ static void output_poll_execute(struct work_struct *work) if (repoll) schedule_delayed_work(delayed_work, DRM_OUTPUT_POLL_PERIOD); + dev_info(>pdev->dev, "end poll\n"); } /** diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c index 3e29302..f9da5bc 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drm.c +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c @@ -855,6 +855,9 @@ static int nouveau_drm_probe(struct pci_dev *pdev, return -EBUSY; } + printk("waiting 12 sec\n"); + msleep(12*1000); + printk("done waiting 12 sec\n"); drm_kms_helper_poll_disable(drm_dev); vga_switcheroo_set_dynamic_switch(pdev, VGA_SWITCHEROO_OFF); nouveau_switcheroo_optimus_dsm(); diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c index 31dd04f..2b4e7e0 100644 --- a/drivers/gpu/drm/radeon/radeon_drv.c +++ b/drivers/gpu/drm/radeon/radeon_drv.c @@ -35,6 +35,7 @@ #include #include +#include #include #include #include @@ -413,6 +414,9 @@ static int radeon_pmops_runtime_suspend(struct device *dev) return -EBUSY; } + printk("waiting 12 sec\n"); + msleep(12*1000); + printk("done waiting 12 sec\n"); drm_dev->switch_power_state = DRM_SWITCH_POWER_CHANGING; drm_kms_helper_poll_disable(drm_dev); vga_switcheroo_set_dynamic_switch(pdev, VGA_SWITCHEROO_OFF); ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/5] Fix deadlock on runtime suspend in DRM drivers
On Sun, Feb 11, 2018 at 06:58:11PM +, Mike Lothian wrote: > On 11 February 2018 at 09:38, Lukas Wunnerwrote: > > The patches for radeon and amdgpu are compile-tested only, I only have a > > MacBook Pro with an Nvidia GK107 to test. To test the patches, add an > > "msleep(12*1000);" at the top of the driver's ->runtime_suspend hook. > > This ensures that the poll worker runs after ->runtime_suspend has begun. > > Wait 12 sec after the GPU has begun runtime suspend, then check > > /sys/bus/pci/devices/:01:00.0/power/runtime_status. Without this > > series, the status will be stuck at "suspending" and you'll get hung task > > errors in dmesg after a few minutes. > > I wasn't quite sure where to add that msleep. I've tested the patches > as is on top of agd5f's wip branch without ill effects > > I've had a radeon and now a amdgpu PRIME setup and don't believe I've > ever seen this issue > > If you could pop a patch together for the msleep I'll give it a test on > amdgpu Here you go, this is for all 3 drivers. Should deadlock without the series. Thanks! diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index 50afcf6..eefa0d4 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -718,6 +718,9 @@ static int amdgpu_pmops_runtime_suspend(struct device *dev) return -EBUSY; } + printk("waiting 12 sec\n"); + msleep(12*1000); + printk("done waiting 12 sec\n"); drm_dev->switch_power_state = DRM_SWITCH_POWER_CHANGING; drm_kms_helper_poll_disable(drm_dev); vga_switcheroo_set_dynamic_switch(pdev, VGA_SWITCHEROO_OFF); diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 555fbe5..ee7cf0d 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -586,6 +586,7 @@ static void output_poll_execute(struct work_struct *work) repoll = true; goto out; } + dev_info(>pdev->dev, "begin poll\n"); drm_connector_list_iter_begin(dev, _iter); drm_for_each_connector_iter(connector, _iter) { @@ -651,6 +652,7 @@ static void output_poll_execute(struct work_struct *work) if (repoll) schedule_delayed_work(delayed_work, DRM_OUTPUT_POLL_PERIOD); + dev_info(>pdev->dev, "end poll\n"); } /** diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c index 3e29302..f9da5bc 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drm.c +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c @@ -855,6 +855,9 @@ static int nouveau_drm_probe(struct pci_dev *pdev, return -EBUSY; } + printk("waiting 12 sec\n"); + msleep(12*1000); + printk("done waiting 12 sec\n"); drm_kms_helper_poll_disable(drm_dev); vga_switcheroo_set_dynamic_switch(pdev, VGA_SWITCHEROO_OFF); nouveau_switcheroo_optimus_dsm(); diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c index 31dd04f..707b8aa 100644 --- a/drivers/gpu/drm/radeon/radeon_drv.c +++ b/drivers/gpu/drm/radeon/radeon_drv.c @@ -413,6 +413,9 @@ static int radeon_pmops_runtime_suspend(struct device *dev) return -EBUSY; } + printk("waiting 12 sec\n"); + msleep(12*1000); + printk("done waiting 12 sec\n"); drm_dev->switch_power_state = DRM_SWITCH_POWER_CHANGING; drm_kms_helper_poll_disable(drm_dev); vga_switcheroo_set_dynamic_switch(pdev, VGA_SWITCHEROO_OFF); ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/5] Fix deadlock on runtime suspend in DRM drivers
On 11 February 2018 at 09:38, Lukas Wunnerwrote: > Fix a deadlock on hybrid graphics laptops that's been present since 2013: > > DRM drivers poll connectors in 10 sec intervals. The poll worker is > stopped on ->runtime_suspend with cancel_delayed_work_sync(). However > the poll worker invokes the DRM drivers' ->detect callbacks, which call > pm_runtime_get_sync(). If the poll worker starts after runtime suspend > has begun, pm_runtime_get_sync() will wait for runtime suspend to finish > with the intention of runtime resuming the device afterwards. The result > is a circular wait between poll worker and autosuspend worker. > > I'm seeing this deadlock so often it's not funny anymore. I've also found > 3 nouveau bugzillas about it and 1 radeon bugzilla. (See patch [3/5] and > [4/5].) > > One theoretical solution would be to add a flag to the ->detect callback > to tell it that it's running in the poll worker's context. In that case > it doesn't need to call pm_runtime_get_sync() because the poll worker is > only enabled while runtime active and we know that ->runtime_suspend > waits for it to finish. But this approach would require touching every > single DRM driver's ->detect hook. Moreover the ->detect hook is called > from numerous other places, both in the DRM library as well as in each > driver, making it necessary to trace every possible call chain and check > if it's coming from the poll worker or not. I've tried to do that for > nouveau (see the call sites listed in the commit message of patch [3/5]) > and concluded that this approach is a nightmare to implement. > > Another reason for the infeasibility of this approach is that ->detect > is called from within the DRM library without driver involvement, e.g. > via DRM's sysfs interface. In those cases, pm_runtime_get_sync() would > have to be called by the DRM library, but the DRM library is supposed to > stay generic, wheras runtime PM is driver-specific. > > So instead, I've come up with this much simpler solution which gleans > from the current task's flags if it's a workqueue worker, retrieves the > work_struct and checks if it's the poll worker. All that's needed is > one small helper in the workqueue code and another small helper in the > DRM library. This solution lends itself nicely to stable-backporting. > > The patches for radeon and amdgpu are compile-tested only, I only have a > MacBook Pro with an Nvidia GK107 to test. To test the patches, add an > "msleep(12*1000);" at the top of the driver's ->runtime_suspend hook. > This ensures that the poll worker runs after ->runtime_suspend has begun. > Wait 12 sec after the GPU has begun runtime suspend, then check > /sys/bus/pci/devices/:01:00.0/power/runtime_status. Without this > series, the status will be stuck at "suspending" and you'll get hung task > errors in dmesg after a few minutes. > > i915, malidp and msm "solved" this issue by not stopping the poll worker > on runtime suspend. But this results in the GPU bouncing back and forth > between D0 and D3 continuously. That's a total no-go for GPUs which > runtime suspend to D3cold since every suspend/resume cycle costs a > significant amount of time and energy. (i915 and malidp do not seem > to acquire a runtime PM ref in the ->detect callbacks, which seems > questionable. msm however does and would also deadlock if it disabled > the poll worker on runtime suspend. cc += Archit, Liviu, intel-gfx) > > Please review. Thanks, > > Lukas > > Lukas Wunner (5): > workqueue: Allow retrieval of current task's work struct > drm: Allow determining if current task is output poll worker > drm/nouveau: Fix deadlock on runtime suspend > drm/radeon: Fix deadlock on runtime suspend > drm/amdgpu: Fix deadlock on runtime suspend > > drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c | 58 +--- > drivers/gpu/drm/drm_probe_helper.c | 14 + > drivers/gpu/drm/nouveau/nouveau_connector.c| 18 +-- > drivers/gpu/drm/radeon/radeon_connectors.c | 74 > +- > include/drm/drm_crtc_helper.h | 1 + > include/linux/workqueue.h | 1 + > kernel/workqueue.c | 16 ++ > 7 files changed, 132 insertions(+), 50 deletions(-) > > -- > 2.15.1 > > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel I wasn't quite sure where to add that msleep. I've tested the patches as is on top of agd5f's wip branch without ill effects I've had a radeon and now a amdgpu PRIME setup and don't believe I've ever seen this issue If you could pop a patch together for the msleep I'll give it a test on amdgpu Cheers Mike ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH igt] igt/kms_frontbuffer_tracking: Disable FBC testing for -ENODEV
Signed-off-by: Chris Wilson--- tests/kms_frontbuffer_tracking.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c index 602707f98..6d3fe8d02 100644 --- a/tests/kms_frontbuffer_tracking.c +++ b/tests/kms_frontbuffer_tracking.c @@ -1588,6 +1588,9 @@ static bool fbc_supported_on_chipset(void) char buf[128]; debugfs_read("i915_fbc_status", buf); + if (*buf == '\0') + return false; + return !strstr(buf, "FBC unsupported on this chipset\n"); } -- 2.16.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4 1/3] drm/i915: Add intel_bios_cleanup() function
Hi, On 06-02-18 15:12, Hans de Goede wrote: Add an intel_bios_cleanup() function to act as counterpart of intel_bios_init() and move the cleanup of vbt related resources there, putting it in the same file as the allocation. Signed-off-by: Hans de GoedeCan I get an Acked-by for this patch and 2/3 please? Then I can push this series. Regards, Hans --- drivers/gpu/drm/i915/i915_drv.c | 14 +- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_bios.c | 21 + 3 files changed, 23 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index e9f1daf258fe..7f094bbc2a7f 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1446,19 +1446,7 @@ void i915_driver_unload(struct drm_device *dev) intel_modeset_cleanup(dev); - /* -* free the memory space allocated for the child device -* config parsed from VBT -*/ - if (dev_priv->vbt.child_dev && dev_priv->vbt.child_dev_num) { - kfree(dev_priv->vbt.child_dev); - dev_priv->vbt.child_dev = NULL; - dev_priv->vbt.child_dev_num = 0; - } - kfree(dev_priv->vbt.sdvo_lvds_vbt_mode); - dev_priv->vbt.sdvo_lvds_vbt_mode = NULL; - kfree(dev_priv->vbt.lfp_lvds_vbt_mode); - dev_priv->vbt.lfp_lvds_vbt_mode = NULL; + intel_bios_cleanup(dev_priv); vga_switcheroo_unregister_client(pdev); vga_client_register(pdev, NULL, NULL, NULL); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index d6b5ac2a563d..1cccea1b87bc 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3675,6 +3675,7 @@ extern void intel_i2c_reset(struct drm_i915_private *dev_priv); /* intel_bios.c */ void intel_bios_init(struct drm_i915_private *dev_priv); +void intel_bios_cleanup(struct drm_i915_private *dev_priv); bool intel_bios_is_valid_vbt(const void *buf, size_t size); bool intel_bios_is_tv_present(struct drm_i915_private *dev_priv); bool intel_bios_is_lvds_present(struct drm_i915_private *dev_priv, u8 *i2c_pin); diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c index 4e74aa2f16bc..f9550507bb9f 100644 --- a/drivers/gpu/drm/i915/intel_bios.c +++ b/drivers/gpu/drm/i915/intel_bios.c @@ -1610,6 +1610,27 @@ void intel_bios_init(struct drm_i915_private *dev_priv) pci_unmap_rom(pdev, bios); } +/** + * intel_bios_cleanup - Free any resources allocated by intel_bios_init() + * @dev_priv: i915 device instance + */ +void intel_bios_cleanup(struct drm_i915_private *dev_priv) +{ + /* +* free the memory space allocated for the child device +* config parsed from VBT +*/ + if (dev_priv->vbt.child_dev && dev_priv->vbt.child_dev_num) { + kfree(dev_priv->vbt.child_dev); + dev_priv->vbt.child_dev = NULL; + dev_priv->vbt.child_dev_num = 0; + } + kfree(dev_priv->vbt.sdvo_lvds_vbt_mode); + dev_priv->vbt.sdvo_lvds_vbt_mode = NULL; + kfree(dev_priv->vbt.lfp_lvds_vbt_mode); + dev_priv->vbt.lfp_lvds_vbt_mode = NULL; +} + /** * intel_bios_is_tv_present - is integrated TV present in VBT * @dev_priv: i915 device instance ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 0/5] Fix deadlock on runtime suspend in DRM drivers
Fix a deadlock on hybrid graphics laptops that's been present since 2013: DRM drivers poll connectors in 10 sec intervals. The poll worker is stopped on ->runtime_suspend with cancel_delayed_work_sync(). However the poll worker invokes the DRM drivers' ->detect callbacks, which call pm_runtime_get_sync(). If the poll worker starts after runtime suspend has begun, pm_runtime_get_sync() will wait for runtime suspend to finish with the intention of runtime resuming the device afterwards. The result is a circular wait between poll worker and autosuspend worker. I'm seeing this deadlock so often it's not funny anymore. I've also found 3 nouveau bugzillas about it and 1 radeon bugzilla. (See patch [3/5] and [4/5].) One theoretical solution would be to add a flag to the ->detect callback to tell it that it's running in the poll worker's context. In that case it doesn't need to call pm_runtime_get_sync() because the poll worker is only enabled while runtime active and we know that ->runtime_suspend waits for it to finish. But this approach would require touching every single DRM driver's ->detect hook. Moreover the ->detect hook is called from numerous other places, both in the DRM library as well as in each driver, making it necessary to trace every possible call chain and check if it's coming from the poll worker or not. I've tried to do that for nouveau (see the call sites listed in the commit message of patch [3/5]) and concluded that this approach is a nightmare to implement. Another reason for the infeasibility of this approach is that ->detect is called from within the DRM library without driver involvement, e.g. via DRM's sysfs interface. In those cases, pm_runtime_get_sync() would have to be called by the DRM library, but the DRM library is supposed to stay generic, wheras runtime PM is driver-specific. So instead, I've come up with this much simpler solution which gleans from the current task's flags if it's a workqueue worker, retrieves the work_struct and checks if it's the poll worker. All that's needed is one small helper in the workqueue code and another small helper in the DRM library. This solution lends itself nicely to stable-backporting. The patches for radeon and amdgpu are compile-tested only, I only have a MacBook Pro with an Nvidia GK107 to test. To test the patches, add an "msleep(12*1000);" at the top of the driver's ->runtime_suspend hook. This ensures that the poll worker runs after ->runtime_suspend has begun. Wait 12 sec after the GPU has begun runtime suspend, then check /sys/bus/pci/devices/:01:00.0/power/runtime_status. Without this series, the status will be stuck at "suspending" and you'll get hung task errors in dmesg after a few minutes. i915, malidp and msm "solved" this issue by not stopping the poll worker on runtime suspend. But this results in the GPU bouncing back and forth between D0 and D3 continuously. That's a total no-go for GPUs which runtime suspend to D3cold since every suspend/resume cycle costs a significant amount of time and energy. (i915 and malidp do not seem to acquire a runtime PM ref in the ->detect callbacks, which seems questionable. msm however does and would also deadlock if it disabled the poll worker on runtime suspend. cc += Archit, Liviu, intel-gfx) Please review. Thanks, Lukas Lukas Wunner (5): workqueue: Allow retrieval of current task's work struct drm: Allow determining if current task is output poll worker drm/nouveau: Fix deadlock on runtime suspend drm/radeon: Fix deadlock on runtime suspend drm/amdgpu: Fix deadlock on runtime suspend drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c | 58 +--- drivers/gpu/drm/drm_probe_helper.c | 14 + drivers/gpu/drm/nouveau/nouveau_connector.c| 18 +-- drivers/gpu/drm/radeon/radeon_connectors.c | 74 +- include/drm/drm_crtc_helper.h | 1 + include/linux/workqueue.h | 1 + kernel/workqueue.c | 16 ++ 7 files changed, 132 insertions(+), 50 deletions(-) -- 2.15.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 5/5] drm/amdgpu: Fix deadlock on runtime suspend
amdgpu's ->runtime_suspend hook calls drm_kms_helper_poll_disable(), which waits for the output poll worker to finish if it's running. The output poll worker meanwhile calls pm_runtime_get_sync() in amdgpu's ->detect hooks, which waits for the ongoing suspend to finish, causing a deadlock. Fix by not acquiring a runtime PM ref if the ->detect hooks are called in the output poll worker's context. This is safe because the poll worker is only enabled while runtime active and we know that ->runtime_suspend waits for it to finish. Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)") Cc: sta...@vger.kernel.org # v4.2+: 1234567890ab: workqueue: Allow retrieval of current task's work struct Cc: sta...@vger.kernel.org # v4.2+: 1234567890ab: drm: Allow determining if current task is output poll worker Cc: Alex DeucherSigned-off-by: Lukas Wunner --- drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c | 58 +- 1 file changed, 38 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c index 8ca3783f2deb..74d2efaec52f 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c @@ -736,9 +736,11 @@ amdgpu_connector_lvds_detect(struct drm_connector *connector, bool force) enum drm_connector_status ret = connector_status_disconnected; int r; - r = pm_runtime_get_sync(connector->dev->dev); - if (r < 0) - return connector_status_disconnected; + if (!drm_kms_helper_is_poll_worker()) { + r = pm_runtime_get_sync(connector->dev->dev); + if (r < 0) + return connector_status_disconnected; + } if (encoder) { struct amdgpu_encoder *amdgpu_encoder = to_amdgpu_encoder(encoder); @@ -757,8 +759,12 @@ amdgpu_connector_lvds_detect(struct drm_connector *connector, bool force) /* check acpi lid status ??? */ amdgpu_connector_update_scratch_regs(connector, ret); - pm_runtime_mark_last_busy(connector->dev->dev); - pm_runtime_put_autosuspend(connector->dev->dev); + + if (!drm_kms_helper_is_poll_worker()) { + pm_runtime_mark_last_busy(connector->dev->dev); + pm_runtime_put_autosuspend(connector->dev->dev); + } + return ret; } @@ -868,9 +874,11 @@ amdgpu_connector_vga_detect(struct drm_connector *connector, bool force) enum drm_connector_status ret = connector_status_disconnected; int r; - r = pm_runtime_get_sync(connector->dev->dev); - if (r < 0) - return connector_status_disconnected; + if (!drm_kms_helper_is_poll_worker()) { + r = pm_runtime_get_sync(connector->dev->dev); + if (r < 0) + return connector_status_disconnected; + } encoder = amdgpu_connector_best_single_encoder(connector); if (!encoder) @@ -924,8 +932,10 @@ amdgpu_connector_vga_detect(struct drm_connector *connector, bool force) amdgpu_connector_update_scratch_regs(connector, ret); out: - pm_runtime_mark_last_busy(connector->dev->dev); - pm_runtime_put_autosuspend(connector->dev->dev); + if (!drm_kms_helper_is_poll_worker()) { + pm_runtime_mark_last_busy(connector->dev->dev); + pm_runtime_put_autosuspend(connector->dev->dev); + } return ret; } @@ -988,9 +998,11 @@ amdgpu_connector_dvi_detect(struct drm_connector *connector, bool force) enum drm_connector_status ret = connector_status_disconnected; bool dret = false, broken_edid = false; - r = pm_runtime_get_sync(connector->dev->dev); - if (r < 0) - return connector_status_disconnected; + if (!drm_kms_helper_is_poll_worker()) { + r = pm_runtime_get_sync(connector->dev->dev); + if (r < 0) + return connector_status_disconnected; + } if (!force && amdgpu_connector_check_hpd_status_unchanged(connector)) { ret = connector->status; @@ -1115,8 +1127,10 @@ amdgpu_connector_dvi_detect(struct drm_connector *connector, bool force) amdgpu_connector_update_scratch_regs(connector, ret); exit: - pm_runtime_mark_last_busy(connector->dev->dev); - pm_runtime_put_autosuspend(connector->dev->dev); + if (!drm_kms_helper_is_poll_worker()) { + pm_runtime_mark_last_busy(connector->dev->dev); + pm_runtime_put_autosuspend(connector->dev->dev); + } return ret; } @@ -1359,9 +1373,11 @@ amdgpu_connector_dp_detect(struct drm_connector *connector, bool force) struct drm_encoder *encoder = amdgpu_connector_best_single_encoder(connector); int r; - r = pm_runtime_get_sync(connector->dev->dev); - if (r < 0) -
[Intel-gfx] [PATCH 2/5] drm: Allow determining if current task is output poll worker
Introduce a helper to determine if the current task is an output poll worker. This allows us to fix a long-standing deadlock in several DRM drivers wherein the ->runtime_suspend callback waits for the output poll worker to finish and the worker in turn calls a ->detect callback which waits for runtime suspend to finish. The ->detect callback is invoked from multiple call sites and waiting for runtime suspend to finish is the correct thing to do except if it's executing in the context of the worker. Cc: Dave AirlieCc: Ben Skeggs Cc: Alex Deucher Signed-off-by: Lukas Wunner --- drivers/gpu/drm/drm_probe_helper.c | 14 ++ include/drm/drm_crtc_helper.h | 1 + 2 files changed, 15 insertions(+) diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c index 555fbe54d6e2..019881d15ce1 100644 --- a/drivers/gpu/drm/drm_probe_helper.c +++ b/drivers/gpu/drm/drm_probe_helper.c @@ -653,6 +653,20 @@ static void output_poll_execute(struct work_struct *work) schedule_delayed_work(delayed_work, DRM_OUTPUT_POLL_PERIOD); } +/** + * drm_kms_helper_is_poll_worker - is %current task an output poll worker? + * + * Determine if %current task is an output poll worker. This can be used + * to select distinct code paths for output polling versus other contexts. + */ +bool drm_kms_helper_is_poll_worker(void) +{ + struct work_struct *work = current_work(); + + return work && work->func == output_poll_execute; +} +EXPORT_SYMBOL(drm_kms_helper_is_poll_worker); + /** * drm_kms_helper_poll_disable - disable output polling * @dev: drm_device diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h index 76e237bd989b..6914633037a5 100644 --- a/include/drm/drm_crtc_helper.h +++ b/include/drm/drm_crtc_helper.h @@ -77,5 +77,6 @@ void drm_kms_helper_hotplug_event(struct drm_device *dev); void drm_kms_helper_poll_disable(struct drm_device *dev); void drm_kms_helper_poll_enable(struct drm_device *dev); +bool drm_kms_helper_is_poll_worker(void); #endif -- 2.15.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 4/5] drm/radeon: Fix deadlock on runtime suspend
radeon's ->runtime_suspend hook calls drm_kms_helper_poll_disable(), which waits for the output poll worker to finish if it's running. The output poll worker meanwhile calls pm_runtime_get_sync() in radeon's ->detect hooks, which waits for the ongoing suspend to finish, causing a deadlock. Fix by not acquiring a runtime PM ref if the ->detect hooks are called in the output poll worker's context. This is safe because the poll worker is only enabled while runtime active and we know that ->runtime_suspend waits for it to finish. Stack trace for posterity: INFO: task kworker/0:3:31847 blocked for more than 120 seconds Workqueue: events output_poll_execute [drm_kms_helper] Call Trace: schedule+0x3c/0x90 rpm_resume+0x1e2/0x690 __pm_runtime_resume+0x3f/0x60 radeon_lvds_detect+0x39/0xf0 [radeon] output_poll_execute+0xda/0x1e0 [drm_kms_helper] process_one_work+0x14b/0x440 worker_thread+0x48/0x4a0 INFO: task kworker/2:0:10493 blocked for more than 120 seconds. Workqueue: pm pm_runtime_work Call Trace: schedule+0x3c/0x90 schedule_timeout+0x1b3/0x240 wait_for_common+0xc2/0x180 wait_for_completion+0x1d/0x20 flush_work+0xfc/0x1a0 __cancel_work_timer+0xa5/0x1d0 cancel_delayed_work_sync+0x13/0x20 drm_kms_helper_poll_disable+0x1f/0x30 [drm_kms_helper] radeon_pmops_runtime_suspend+0x3d/0xa0 [radeon] pci_pm_runtime_suspend+0x61/0x1a0 vga_switcheroo_runtime_suspend+0x21/0x70 __rpm_callback+0x32/0x70 rpm_callback+0x24/0x80 rpm_suspend+0x12b/0x640 pm_runtime_work+0x6f/0xb0 process_one_work+0x14b/0x440 worker_thread+0x48/0x4a0 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94147 Fixes: 10ebc0bc0934 ("drm/radeon: add runtime PM support (v2)") Cc: sta...@vger.kernel.org # v3.13+: 1234567890ab: workqueue: Allow retrieval of current task's work struct Cc: sta...@vger.kernel.org # v3.13+: 1234567890ab: drm: Allow determining if current task is output poll worker Cc: Ismo ToijalaCc: Alex Deucher Cc: Dave Airlie Signed-off-by: Lukas Wunner --- drivers/gpu/drm/radeon/radeon_connectors.c | 74 -- 1 file changed, 49 insertions(+), 25 deletions(-) diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c index 5012f5e47a1e..2e2ca3c6b47d 100644 --- a/drivers/gpu/drm/radeon/radeon_connectors.c +++ b/drivers/gpu/drm/radeon/radeon_connectors.c @@ -899,9 +899,11 @@ radeon_lvds_detect(struct drm_connector *connector, bool force) enum drm_connector_status ret = connector_status_disconnected; int r; - r = pm_runtime_get_sync(connector->dev->dev); - if (r < 0) - return connector_status_disconnected; + if (!drm_kms_helper_is_poll_worker()) { + r = pm_runtime_get_sync(connector->dev->dev); + if (r < 0) + return connector_status_disconnected; + } if (encoder) { struct radeon_encoder *radeon_encoder = to_radeon_encoder(encoder); @@ -924,8 +926,12 @@ radeon_lvds_detect(struct drm_connector *connector, bool force) /* check acpi lid status ??? */ radeon_connector_update_scratch_regs(connector, ret); - pm_runtime_mark_last_busy(connector->dev->dev); - pm_runtime_put_autosuspend(connector->dev->dev); + + if (!drm_kms_helper_is_poll_worker()) { + pm_runtime_mark_last_busy(connector->dev->dev); + pm_runtime_put_autosuspend(connector->dev->dev); + } + return ret; } @@ -1039,9 +1045,11 @@ radeon_vga_detect(struct drm_connector *connector, bool force) enum drm_connector_status ret = connector_status_disconnected; int r; - r = pm_runtime_get_sync(connector->dev->dev); - if (r < 0) - return connector_status_disconnected; + if (!drm_kms_helper_is_poll_worker()) { + r = pm_runtime_get_sync(connector->dev->dev); + if (r < 0) + return connector_status_disconnected; + } encoder = radeon_best_single_encoder(connector); if (!encoder) @@ -1108,8 +1116,10 @@ radeon_vga_detect(struct drm_connector *connector, bool force) radeon_connector_update_scratch_regs(connector, ret); out: - pm_runtime_mark_last_busy(connector->dev->dev); - pm_runtime_put_autosuspend(connector->dev->dev); + if (!drm_kms_helper_is_poll_worker()) { + pm_runtime_mark_last_busy(connector->dev->dev); + pm_runtime_put_autosuspend(connector->dev->dev); + } return ret; } @@ -1173,9 +1183,11 @@ radeon_tv_detect(struct drm_connector *connector, bool force) if (!radeon_connector->dac_load_detect) return ret; - r = pm_runtime_get_sync(connector->dev->dev); - if (r < 0) - return
[Intel-gfx] [PATCH 1/5] workqueue: Allow retrieval of current task's work struct
Introduce a helper to retrieve the current task's work struct if it is a workqueue worker. This allows us to fix a long-standing deadlock in several DRM drivers wherein the ->runtime_suspend callback waits for a specific worker to finish and that worker in turn calls a function which waits for runtime suspend to finish. That function is invoked from multiple call sites and waiting for runtime suspend to finish is the correct thing to do except if it's executing in the context of the worker. Cc: Tejun HeoCc: Lai Jiangshan Cc: Dave Airlie Cc: Ben Skeggs Cc: Alex Deucher Signed-off-by: Lukas Wunner --- include/linux/workqueue.h | 1 + kernel/workqueue.c| 16 2 files changed, 17 insertions(+) diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h index 4a54ef96aff5..bc0cda180c8b 100644 --- a/include/linux/workqueue.h +++ b/include/linux/workqueue.h @@ -465,6 +465,7 @@ extern bool cancel_delayed_work_sync(struct delayed_work *dwork); extern void workqueue_set_max_active(struct workqueue_struct *wq, int max_active); +extern struct work_struct *current_work(void); extern bool current_is_workqueue_rescuer(void); extern bool workqueue_congested(int cpu, struct workqueue_struct *wq); extern unsigned int work_busy(struct work_struct *work); diff --git a/kernel/workqueue.c b/kernel/workqueue.c index 017044c26233..bb9a519cbf50 100644 --- a/kernel/workqueue.c +++ b/kernel/workqueue.c @@ -4179,6 +4179,22 @@ void workqueue_set_max_active(struct workqueue_struct *wq, int max_active) } EXPORT_SYMBOL_GPL(workqueue_set_max_active); +/** + * current_work - retrieve %current task's work struct + * + * Determine if %current task is a workqueue worker and what it's working on. + * Useful to find out the context that the %current task is running in. + * + * Return: work struct if %current task is a workqueue worker, %NULL otherwise. + */ +struct work_struct *current_work(void) +{ + struct worker *worker = current_wq_worker(); + + return worker ? worker->current_work : NULL; +} +EXPORT_SYMBOL(current_work); + /** * current_is_workqueue_rescuer - is %current workqueue rescuer? * -- 2.15.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/5] drm/nouveau: Fix deadlock on runtime suspend
nouveau's ->runtime_suspend hook calls drm_kms_helper_poll_disable(), which waits for the output poll worker to finish if it's running. The output poll worker meanwhile calls pm_runtime_get_sync() in nouveau_connector_detect() which waits for the ongoing suspend to finish, causing a deadlock. Fix by not acquiring a runtime PM ref if nouveau_connector_detect() is called in the output poll worker's context. This is safe because the poll worker is only enabled while runtime active and we know that ->runtime_suspend waits for it to finish. Other contexts calling nouveau_connector_detect() do require a runtime PM ref, these comprise: status_store() drm sysfs interface ->fill_modes drm callback drm_fb_helper_probe_connector_modes() drm_mode_getconnector() nouveau_connector_hotplug() nouveau_display_hpd_work() nv17_tv_set_property() Stack trace for posterity: INFO: task kworker/0:1:58 blocked for more than 120 seconds. Workqueue: events output_poll_execute [drm_kms_helper] Call Trace: schedule+0x28/0x80 rpm_resume+0x107/0x6e0 __pm_runtime_resume+0x47/0x70 nouveau_connector_detect+0x7e/0x4a0 [nouveau] nouveau_connector_detect_lvds+0x132/0x180 [nouveau] drm_helper_probe_detect_ctx+0x85/0xd0 [drm_kms_helper] output_poll_execute+0x11e/0x1c0 [drm_kms_helper] process_one_work+0x184/0x380 worker_thread+0x2e/0x390 INFO: task kworker/0:2:252 blocked for more than 120 seconds. Workqueue: pm pm_runtime_work Call Trace: schedule+0x28/0x80 schedule_timeout+0x1e3/0x370 wait_for_completion+0x123/0x190 flush_work+0x142/0x1c0 nouveau_pmops_runtime_suspend+0x7e/0xd0 [nouveau] pci_pm_runtime_suspend+0x5c/0x180 vga_switcheroo_runtime_suspend+0x1e/0xa0 __rpm_callback+0xc1/0x200 rpm_callback+0x1f/0x70 rpm_suspend+0x13c/0x640 pm_runtime_work+0x6e/0x90 process_one_work+0x184/0x380 worker_thread+0x2e/0x390 Bugzilla: https://bugs.archlinux.org/task/53497 Bugzilla: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=870523 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=70388#c33 Fixes: 5addcf0a5f0f ("nouveau: add runtime PM support (v0.9)") Cc: sta...@vger.kernel.org # v3.12+: 1234567890ab: workqueue: Allow retrieval of current task's work struct Cc: sta...@vger.kernel.org # v3.12+: 1234567890ab: drm: Allow determining if current task is output poll worker Cc: Ben SkeggsCc: Dave Airlie Signed-off-by: Lukas Wunner --- drivers/gpu/drm/nouveau/nouveau_connector.c | 18 +- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c index 69d6e61a01ec..6ed9cb053dfa 100644 --- a/drivers/gpu/drm/nouveau/nouveau_connector.c +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c @@ -570,9 +570,15 @@ nouveau_connector_detect(struct drm_connector *connector, bool force) nv_connector->edid = NULL; } - ret = pm_runtime_get_sync(connector->dev->dev); - if (ret < 0 && ret != -EACCES) - return conn_status; + /* Outputs are only polled while runtime active, so acquiring a +* runtime PM ref here is unnecessary (and would deadlock upon +* runtime suspend because it waits for polling to finish). +*/ + if (!drm_kms_helper_is_poll_worker()) { + ret = pm_runtime_get_sync(connector->dev->dev); + if (ret < 0 && ret != -EACCES) + return conn_status; + } nv_encoder = nouveau_connector_ddc_detect(connector); if (nv_encoder && (i2c = nv_encoder->i2c) != NULL) { @@ -647,8 +653,10 @@ nouveau_connector_detect(struct drm_connector *connector, bool force) out: - pm_runtime_mark_last_busy(connector->dev->dev); - pm_runtime_put_autosuspend(connector->dev->dev); + if (!drm_kms_helper_is_poll_worker()) { + pm_runtime_mark_last_busy(connector->dev->dev); + pm_runtime_put_autosuspend(connector->dev->dev); + } return conn_status; } -- 2.15.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx