| Message ID | 1453364174-11248-1-git-send-email-mario.kleiner.de@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 21.01.2016 17:16, Mario Kleiner wrote: > > This patch replaces calls to drm_vblank_pre/post_modeset in the > drivers dpms code with calls to drm_vblank_off/on, as recommended > for drivers with hw counters that reset to zero during modeset. Sounds like you fell for the drm_vblank_on/off propaganda. :( This was working fine with drm_vblank_pre/post_modeset, that it broke is simply a regression. I'm not against switching to drm_vblank_on/off for 4.6, but it's not a solution for older kernels.
On 01/21/2016 09:25 AM, Michel Dänzer wrote: > On 21.01.2016 17:16, Mario Kleiner wrote: >> >> This patch replaces calls to drm_vblank_pre/post_modeset in the >> drivers dpms code with calls to drm_vblank_off/on, as recommended >> for drivers with hw counters that reset to zero during modeset. > > Sounds like you fell for the drm_vblank_on/off propaganda. :( This was > working fine with drm_vblank_pre/post_modeset, that it broke is simply a > regression. > I agree with you that pre/post modeset breakage is a regression. It's just that i stumbled over the on/off stuff while searching for a solution and the other sort of hacks i could think of looked similar or more convoluted/hacky/fragile to me. And they probably wouldn't solve that other small race i found as easily - I don't think it's likely to happen (often/at all?) in practice, but i have trouble "forgetting" about its existence now. > > I'm not against switching to drm_vblank_on/off for 4.6, but it's not a > solution for older kernels. > > Linux 4.4 is an especially important stable kernel for me because it's supposed to be the standard distro kernel for Ubuntu 16.04-LTS and siblings/derivatives (Linux Mint) for up to the next 5 years. Having many of my neuroscience users ending on that kernel as their very first impression of Linux with something potentially broken in vblank land scares me. The reliability of timing/timestamping stuff is super-important for them, at the same time hand-holding many of them through non-standard kernel upgrades would be so much not fun. Just to say i'm probably way too biased wrt. what solution for this should get backported into an older kernel. Anyway, urgently need to sleep. -mario
Is there a PATCH 2/2 which I can't find, or is the subject wrong? On 01/21/2016 09:16 AM, Mario Kleiner wrote: > The hardware vblank counter of AMD gpu's resets to zero during a > modeset. The new implementation of drm_update_vblank_count() from > commit 4dfd6486 "drm: Use vblank timestamps to guesstimate how > many vblanks were missed", introduced in Linux 4.4, treats that > as a counter wraparound and causes the software vblank counter > to jump forward by a large distance of up to 2^24 counts. This > interacts badly with 32-bit wraparound handling in > drm_handle_vblank_events(), causing that function to no longer > deliver pending vblank events to clients. > > This leads to client hangs especially if clients perform OpenGL > or DRI3/Present animations while a modeset happens and triggers > the hw vblank counter reset. One prominent example is a hang of > KDE Plasma 5's startup progress splash screen during login, making > the KDE session unuseable. > > Another small potential race exists when executing a modeset while > vblank interrupts are enabled or just get enabled: The modeset updates > radeon_crtc->lb_vblank_lead_lines during radeon_display_bandwidth_update, > so if vblank interrupt handling or enable would try to access that variable > multiple times at the wrong moment as part of drm_update_vblank_counter, > while the scanout happens to be within lb_vblank_lead_lines before the > start of vblank, it could cause inconsistent vblank counting and again > trigger a jump of the software vblank counter, causing similar client > hangs. The most easy way to avoid this small race is to not allow > vblank enable or vblank irq's during modeset. > > This patch replaces calls to drm_vblank_pre/post_modeset in the > drivers dpms code with calls to drm_vblank_off/on, as recommended > for drivers with hw counters that reset to zero during modeset. > Those calls disable vblank interrupts during the modeset sequence > and reinitialize vblank counts and timestamps after the modeset > properly, taking hw counter reset into account, thereby fixing > the problem of forward jumping counters. > > During a modeset, calls to drm_vblank_get() will no-op/intentionally > fail, so no vblank events or pageflips can be queued during modesetting. > > Radeons static and dynpm power management uses drm_vblank_get to enable > vblank irqs to synchronize reclocking to start of vblank. If a modeset > would happen in parallel with such a power management action, drm_vblank_get > would be suppressed, sync to vblank wouldn't work and a visual glitch could > happen. However that glitch would hopefully be hidden by the blanking of > the crtc during modeset. A small fix to power management makes sure to > check for this and prevent unbalanced vblank reference counts due to > mismatched drm_vblank_get/put. > > Reported-by: Vlastimil Babka <vbabka@suse.cz> > Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com> FWIW, this seems to work for the kde5 login issue, thanks. Let me know if you need also some specific testing/debug output, or testing another approach if the "drm_vblank_on/off propaganda" is not acceptable :)
On Thu, Jan 21, 2016 at 10:16:01AM +0100, Mario Kleiner wrote: > On 01/21/2016 09:25 AM, Michel Dänzer wrote: > > On 21.01.2016 17:16, Mario Kleiner wrote: > >> > >> This patch replaces calls to drm_vblank_pre/post_modeset in the > >> drivers dpms code with calls to drm_vblank_off/on, as recommended > >> for drivers with hw counters that reset to zero during modeset. > > > > Sounds like you fell for the drm_vblank_on/off propaganda. :( This was > > working fine with drm_vblank_pre/post_modeset, that it broke is simply a > > regression. > > > > I agree with you that pre/post modeset breakage is a regression. It's > just that i stumbled over the on/off stuff while searching for a > solution and the other sort of hacks i could think of looked similar or > more convoluted/hacky/fragile to me. And they probably wouldn't solve > that other small race i found as easily - I don't think it's likely to > happen (often/at all?) in practice, but i have trouble "forgetting" > about its existence now. > > > > > I'm not against switching to drm_vblank_on/off for 4.6, but it's not a > > solution for older kernels. > > > > > > Linux 4.4 is an especially important stable kernel for me because it's > supposed to be the standard distro kernel for Ubuntu 16.04-LTS and > siblings/derivatives (Linux Mint) for up to the next 5 years. Having > many of my neuroscience users ending on that kernel as their very first > impression of Linux with something potentially broken in vblank land > scares me. The reliability of timing/timestamping stuff is > super-important for them, at the same time hand-holding many of them > through non-standard kernel upgrades would be so much not fun. Just to > say i'm probably way too biased wrt. what solution for this should get > backported into an older kernel. In hindsight I/we should have probably just totally forked the vblank code and leave radeon to do whatever it was doing since people were apparently satisfied with its state at the time. I guess it would still be possible to do that if needed, though I've not looked at how much extra indirection would be required.
On 21.01.2016 18:16, Mario Kleiner wrote: > On 01/21/2016 09:25 AM, Michel Dänzer wrote: >> On 21.01.2016 17:16, Mario Kleiner wrote: >>> >>> This patch replaces calls to drm_vblank_pre/post_modeset in the >>> drivers dpms code with calls to drm_vblank_off/on, as recommended >>> for drivers with hw counters that reset to zero during modeset. >> >> Sounds like you fell for the drm_vblank_on/off propaganda. :( >> >> This was working fine with drm_vblank_pre/post_modeset, that it broke >> is simply a regression. > > I agree with you that pre/post modeset breakage is a regression. It's > just that i stumbled over the on/off stuff while searching for a > solution and the other sort of hacks i could think of looked similar or > more convoluted/hacky/fragile to me. Finding and fixing the cause of a regression isn't a hack, it's established procedure. > And they probably wouldn't solve that other small race i found as easily > - I don't think it's likely to happen (often/at all?) in practice, but i > have trouble "forgetting" about its existence now. That's something which should be addressed independently from the regression fix. Please split up the PM fixes from your patch into one or two separate patches (which may be appropriate for 4.5 / stable trees), and leave the switch to drm_vblank_on/off to Daniel's patch for 4.6. >> I'm not against switching to drm_vblank_on/off for 4.6, but it's not a >> solution for older kernels. > > Linux 4.4 is an especially important stable kernel for me because it's > supposed to be the standard distro kernel for Ubuntu 16.04-LTS and > siblings/derivatives (Linux Mint) for up to the next 5 years. Having > many of my neuroscience users ending on that kernel as their very first > impression of Linux with something potentially broken in vblank land > scares me. The reliability of timing/timestamping stuff is > super-important for them, at the same time hand-holding many of them > through non-standard kernel upgrades would be so much not fun. But fast-tracking the switch to drm_vblank_on/off, which haven't been widely tested with this driver, all the way to 4.4 seems less risky to you? Seriously? > Just to say i'm probably way too biased wrt. what solution for this > should get backported into an older kernel. At the risk of sounding like a broken record: There's a bug in 4.4 (and older/newer kernels) causing the vblank counter to jump forward across DPMS off with drm_vblank_on/off. So it sounds like you don't want to use those at least until we've found and fixed that.
On 01/22/2016 04:17 AM, Michel Dänzer wrote: > On 21.01.2016 18:16, Mario Kleiner wrote: >> On 01/21/2016 09:25 AM, Michel Dänzer wrote: >>> On 21.01.2016 17:16, Mario Kleiner wrote: >>>> >>>> This patch replaces calls to drm_vblank_pre/post_modeset in the >>>> drivers dpms code with calls to drm_vblank_off/on, as recommended >>>> for drivers with hw counters that reset to zero during modeset. >>> >>> Sounds like you fell for the drm_vblank_on/off propaganda. :( >>> >>> This was working fine with drm_vblank_pre/post_modeset, that it broke >>> is simply a regression. >> >> I agree with you that pre/post modeset breakage is a regression. It's >> just that i stumbled over the on/off stuff while searching for a >> solution and the other sort of hacks i could think of looked similar or >> more convoluted/hacky/fragile to me. > > Finding and fixing the cause of a regression isn't a hack, it's > established procedure. > That's not what i meant. I meant i couldn't find something less complicated/risky/without new regression potential, so this looked like a better solution. Of course i would have tested my own patches against at least a couple bits of userspace (ati ddx, modesetting ddx, weston), i just didn't have access to the machine yesterday. Anyway, some more hours of thinking and code browsing later, now i think i have a simple and safe solution which should hopefully restore the drm_vblank_pre/post_modeset behaviour with only a few lines of core code. At the same time it should fix up another bug in that new drm_update_vblank_count code that i just realized, in a way simple enough for a stable fix. Now i just need to actually code and test it first. > >> And they probably wouldn't solve that other small race i found as easily >> - I don't think it's likely to happen (often/at all?) in practice, but i >> have trouble "forgetting" about its existence now. > > That's something which should be addressed independently from the > regression fix. > > Please split up the PM fixes from your patch into one or two separate > patches (which may be appropriate for 4.5 / stable trees), and leave the > switch to drm_vblank_on/off to Daniel's patch for 4.6. > I will look into that once i'm done with the above, and probably got some sleep again. Fixing this race regression without switching to vblank_off/on might need a small bit of extra band aid there. Btw. wrt. the radeon_pm.c fix: It's certainly good to fix that potential drm_vblank_get/put imbalance. I wonder if that "might glitch" DEBUG message makes much sense though. Can that code run during a modeset at all? And if so, i'd almost expect that there won't be any vblank irqs available at that point anyway - once the crtc's are off they don't trigger vblank irqs anymore - so that code might glitch due to lack of vblank sync regardless if drm_vblank_get is successful or not? The other thing is my placement of the radeon_pm_compute_clocks() in the DPMS_ON path. I moved it to fix the potential extra race i described. But thinking about it, wouldn't be the better place at the beginning of the DPMS on path, before the atombios calls reenable the crtcs? I don't know the driver well enough, but it looked a bit suspicious to me that the memory clocks, linebuffer watermarks etc. get updated for thew new video mode after the crtc has been enabled. Won't it then potentially start running for a moment with wrong memory bandwidth etc.? That's probably something for you to check - no idea, just something i noticed as slightly odd to me. Also moving it up might avoid collisions with Daniel's patch, if that move doesn't hurt. > >>> I'm not against switching to drm_vblank_on/off for 4.6, but it's not a >>> solution for older kernels. >> >> Linux 4.4 is an especially important stable kernel for me because it's >> supposed to be the standard distro kernel for Ubuntu 16.04-LTS and >> siblings/derivatives (Linux Mint) for up to the next 5 years. Having >> many of my neuroscience users ending on that kernel as their very first >> impression of Linux with something potentially broken in vblank land >> scares me. The reliability of timing/timestamping stuff is >> super-important for them, at the same time hand-holding many of them >> through non-standard kernel upgrades would be so much not fun. > > But fast-tracking the switch to drm_vblank_on/off, which haven't been > widely tested with this driver, all the way to 4.4 seems less risky to > you? Seriously? > It made a lot of sense after 12 hours of browsing code, thinking about all kind of race conditions and other new personal horrors ;) - Luckily i don't have to decide. >> Just to say i'm probably way too biased wrt. what solution for this >> should get backported into an older kernel. > > At the risk of sounding like a broken record: There's a bug in 4.4 (and > older/newer kernels) causing the vblank counter to jump forward across > DPMS off with drm_vblank_on/off. So it sounds like you don't want to use > those at least until we've found and fixed that. > > Modesetting "jumps" are not a problem for my application, as production or test sessions won't ever run during modesets or other "disruptive" events like dpms on/off. That's also why my own test suite didn't detect such trouble ever despite extensive testing. Obviously not having a well working desktop or non-working logins etc. would be a bad problem for myself and my users. It still surprises me that the bug Vlastimil reported never happened to myself in days, given that i loggedout/in a lot for testing different X-Server xorg.conf configs on KDE Plasma 5. Even now as i see the jumps during modesets in my dmesg i haven't managed to get Plasma or glxgears etc. to lockup during dozens of modesets. -mario
On 01/22/2016 06:08 PM, Mario Kleiner wrote: > Anyway, some more hours of thinking and code browsing later, now i think > i have a simple and safe solution which should hopefully restore the > drm_vblank_pre/post_modeset behaviour with only a few lines of core > code. At the same time it should fix up another bug in that new > drm_update_vblank_count code that i just realized, in a way simple > enough for a stable fix. > > Now i just need to actually code and test it first. Ping, any news? :)
I have a few simple patches which after testing seem to work well enough and fix additional similar problems with nouveau. Got distracted with other stuff last week. I'll try to send them out later today when i'm at the machine. -mario On Sun, Feb 7, 2016 at 12:05 PM, Vlastimil Babka <vbabka@suse.cz> wrote: > On 01/22/2016 06:08 PM, Mario Kleiner wrote: >> Anyway, some more hours of thinking and code browsing later, now i think >> i have a simple and safe solution which should hopefully restore the >> drm_vblank_pre/post_modeset behaviour with only a few lines of core >> code. At the same time it should fix up another bug in that new >> drm_update_vblank_count code that i just realized, in a way simple >> enough for a stable fix. >> >> Now i just need to actually code and test it first. > > Ping, any news? :) >
On 02/07/2016 12:59 PM, Mario Kleiner wrote: > I have a few simple patches which after testing seem to work well > enough and fix additional similar problems with nouveau. Got > distracted with other stuff last week. I'll try to send them out later > today when i'm at the machine. > > -mario > > > On Sun, Feb 7, 2016 at 12:05 PM, Vlastimil Babka <vbabka@suse.cz> wrote: >> On 01/22/2016 06:08 PM, Mario Kleiner wrote: >>> Anyway, some more hours of thinking and code browsing later, now i think >>> i have a simple and safe solution which should hopefully restore the >>> drm_vblank_pre/post_modeset behaviour with only a few lines of core >>> code. At the same time it should fix up another bug in that new >>> drm_update_vblank_count code that i just realized, in a way simple >>> enough for a stable fix. >>> >>> Now i just need to actually code and test it first. >> >> Ping, any news? :) >> Ok, so that series "drm vblank regression fixes for Linux 4.4+" i just sent out should hopefully fix this bug and related bugs. thanks, -mario
On 02/08/2016 02:58 AM, Mario Kleiner wrote: >>>> Now i just need to actually code and test it first. >>> >>> Ping, any news? :) >>> > > Ok, so that series "drm vblank regression fixes for Linux 4.4+" i just > sent out should hopefully fix this bug and related bugs. Thanks, I'll test on Wed hopefully when I'm at the affected desktop. > > thanks, > -mario >
diff --git a/drivers/gpu/drm/radeon/atombios_crtc.c b/drivers/gpu/drm/radeon/atombios_crtc.c index 801dd60..1c853e0 100644 --- a/drivers/gpu/drm/radeon/atombios_crtc.c +++ b/drivers/gpu/drm/radeon/atombios_crtc.c @@ -275,23 +275,25 @@ void atombios_crtc_dpms(struct drm_crtc *crtc, int mode) if (ASIC_IS_DCE3(rdev) && !ASIC_IS_DCE6(rdev)) atombios_enable_crtc_memreq(crtc, ATOM_ENABLE); atombios_blank_crtc(crtc, ATOM_DISABLE); - drm_vblank_post_modeset(dev, radeon_crtc->crtc_id); + /* adjust pm to dpms *before* drm_vblank_on */ + radeon_pm_compute_clocks(rdev); + drm_vblank_on(dev, radeon_crtc->crtc_id); radeon_crtc_load_lut(crtc); break; case DRM_MODE_DPMS_STANDBY: case DRM_MODE_DPMS_SUSPEND: case DRM_MODE_DPMS_OFF: - drm_vblank_pre_modeset(dev, radeon_crtc->crtc_id); + drm_vblank_off(dev, radeon_crtc->crtc_id); if (radeon_crtc->enabled) atombios_blank_crtc(crtc, ATOM_ENABLE); if (ASIC_IS_DCE3(rdev) && !ASIC_IS_DCE6(rdev)) atombios_enable_crtc_memreq(crtc, ATOM_DISABLE); atombios_enable_crtc(crtc, ATOM_DISABLE); radeon_crtc->enabled = false; + /* adjust pm to dpms *after* drm_vblank_off */ + radeon_pm_compute_clocks(rdev); break; } - /* adjust pm to dpms */ - radeon_pm_compute_clocks(rdev); } static void diff --git a/drivers/gpu/drm/radeon/radeon_legacy_crtc.c b/drivers/gpu/drm/radeon/radeon_legacy_crtc.c index 32b338f..24152df 100644 --- a/drivers/gpu/drm/radeon/radeon_legacy_crtc.c +++ b/drivers/gpu/drm/radeon/radeon_legacy_crtc.c @@ -331,13 +331,13 @@ static void radeon_crtc_dpms(struct drm_crtc *crtc, int mode) RADEON_CRTC_DISP_REQ_EN_B)); WREG32_P(RADEON_CRTC_EXT_CNTL, crtc_ext_cntl, ~(mask | crtc_ext_cntl)); } - drm_vblank_post_modeset(dev, radeon_crtc->crtc_id); + drm_vblank_on(dev, radeon_crtc->crtc_id); radeon_crtc_load_lut(crtc); break; case DRM_MODE_DPMS_STANDBY: case DRM_MODE_DPMS_SUSPEND: case DRM_MODE_DPMS_OFF: - drm_vblank_pre_modeset(dev, radeon_crtc->crtc_id); + drm_vblank_off(dev, radeon_crtc->crtc_id); if (radeon_crtc->crtc_id) WREG32_P(RADEON_CRTC2_GEN_CNTL, mask, ~(RADEON_CRTC2_EN | mask)); else { diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c index 59abebd..339a6c5 100644 --- a/drivers/gpu/drm/radeon/radeon_pm.c +++ b/drivers/gpu/drm/radeon/radeon_pm.c @@ -276,8 +276,12 @@ static void radeon_pm_set_clocks(struct radeon_device *rdev) if (rdev->irq.installed) { for (i = 0; i < rdev->num_crtc; i++) { if (rdev->pm.active_crtcs & (1 << i)) { - rdev->pm.req_vblank |= (1 << i); - drm_vblank_get(rdev->ddev, i); + /* This can fail if a modeset is in progress */ + if (0 == drm_vblank_get(rdev->ddev, i)) + rdev->pm.req_vblank |= (1 << i); + else + DRM_DEBUG_DRIVER("crtc %d no vblank, can glitch\n", + i); } } }
The hardware vblank counter of AMD gpu's resets to zero during a modeset. The new implementation of drm_update_vblank_count() from commit 4dfd6486 "drm: Use vblank timestamps to guesstimate how many vblanks were missed", introduced in Linux 4.4, treats that as a counter wraparound and causes the software vblank counter to jump forward by a large distance of up to 2^24 counts. This interacts badly with 32-bit wraparound handling in drm_handle_vblank_events(), causing that function to no longer deliver pending vblank events to clients. This leads to client hangs especially if clients perform OpenGL or DRI3/Present animations while a modeset happens and triggers the hw vblank counter reset. One prominent example is a hang of KDE Plasma 5's startup progress splash screen during login, making the KDE session unuseable. Another small potential race exists when executing a modeset while vblank interrupts are enabled or just get enabled: The modeset updates radeon_crtc->lb_vblank_lead_lines during radeon_display_bandwidth_update, so if vblank interrupt handling or enable would try to access that variable multiple times at the wrong moment as part of drm_update_vblank_counter, while the scanout happens to be within lb_vblank_lead_lines before the start of vblank, it could cause inconsistent vblank counting and again trigger a jump of the software vblank counter, causing similar client hangs. The most easy way to avoid this small race is to not allow vblank enable or vblank irq's during modeset. This patch replaces calls to drm_vblank_pre/post_modeset in the drivers dpms code with calls to drm_vblank_off/on, as recommended for drivers with hw counters that reset to zero during modeset. Those calls disable vblank interrupts during the modeset sequence and reinitialize vblank counts and timestamps after the modeset properly, taking hw counter reset into account, thereby fixing the problem of forward jumping counters. During a modeset, calls to drm_vblank_get() will no-op/intentionally fail, so no vblank events or pageflips can be queued during modesetting. Radeons static and dynpm power management uses drm_vblank_get to enable vblank irqs to synchronize reclocking to start of vblank. If a modeset would happen in parallel with such a power management action, drm_vblank_get would be suppressed, sync to vblank wouldn't work and a visual glitch could happen. However that glitch would hopefully be hidden by the blanking of the crtc during modeset. A small fix to power management makes sure to check for this and prevent unbalanced vblank reference counts due to mismatched drm_vblank_get/put. Reported-by: Vlastimil Babka <vbabka@suse.cz> Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com> Cc: michel@daenzer.net Cc: vbabka@suse.cz Cc: ville.syrjala@linux.intel.com Cc: daniel.vetter@ffwll.ch Cc: dri-devel@lists.freedesktop.org Cc: alexander.deucher@amd.com Cc: christian.koenig@amd.com --- drivers/gpu/drm/radeon/atombios_crtc.c | 10 ++++++---- drivers/gpu/drm/radeon/radeon_legacy_crtc.c | 4 ++-- drivers/gpu/drm/radeon/radeon_pm.c | 8 ++++++-- 3 files changed, 14 insertions(+), 8 deletions(-)