drm/radeon: Fixup hw vblank counter/ts for new drm_update_vblank_count() (v2)

commit 4dfd6486 "drm: Use vblank timestamps to guesstimate how many
vblanks were missed" introduced in Linux 4.4-rc1 makes the drm core
more fragile to drivers which don't update hw vblank counters and
vblank timestamps in sync with firing of the vblank irq and
essentially at leading edge of vblank.

This exposed a problem with radeon-kms/amdgpu-kms which do not
satisfy above requirements:

The vblank irq fires a few scanlines before start of vblank, but
programmed pageflips complete at start of vblank and
vblank timestamps update at start of vblank, whereas the
hw vblank counter increments only later, at start of vsync.

This leads to problems like off by one errors for vblank counter
updates, vblank counters apparently going backwards or vblank
timestamps apparently having time going backwards. The net result
is stuttering of graphics in games, or little hangs, as well as
total failure of timing sensitive applications.

See bug #93147 for an example of the regression on Linux 4.4-rc:

https://bugs.freedesktop.org/show_bug.cgi?id=93147

This patch tries to align all above events better from the
viewpoint of the drm core / of external callers to fix the problem:

1. The apparent start of vblank is shifted a few scanlines earlier,
so the vblank irq now always happens after start of this extended
vblank interval and thereby drm_update_vblank_count() always samples
the updated vblank count and timestamp of the new vblank interval.

To achieve this, the reporting of scanout positions by
radeon_get_crtc_scanoutpos() now operates as if the vblank starts
radeon_crtc->lb_vblank_lead_lines before the real start of the hw
vblank interval. This means that the vblank timestamps which are based
on these scanout positions will now update at this earlier start of
vblank.

2. The driver->get_vblank_counter() function will bump the returned
vblank count as read from the hw by +1 if the query happens after
the shifted earlier start of the vblank, but before the real hw increment
at start of vsync, so the counter appears to increment at start of vblank
in sync with the timestamp update.

3. Calls from vblank irq-context and regular non-irq calls are now
treated identical, always simulating the shifted vblank start, to
avoid inconsistent results for queries happening from vblank irq vs.
happening from drm_vblank_enable() or vblank_disable_fn().

4. The radeon_flip_work_func will delay mmio programming a pageflip until
the start of the real vblank iff it happens to execute inside the shifted
earlier start of the vblank, so pageflips now also appear to execute at
start of the shifted vblank, in sync with vblank counter and timestamp
updates. This to avoid some races between updates of vblank count and
timestamps that are used for swap scheduling and pageflip execution which
could cause pageflips to execute before the scheduled target vblank.

The lb_vblank_lead_lines "fudge" value is calculated as the size of
the display controllers line buffer in scanlines for the given video
mode: Vblank irq's are triggered by the line buffer logic when the line
buffer refill for a video frame ends, ie. when the line buffer source read
position enters the hw vblank. This means that a vblank irq could fire at
most as many scanlines before the current reported scanout position of the
crtc timing generator as the number of scanlines the line buffer can
maximally hold for a given video mode.

This patch has been successfully tested on a RV730 card with DCE-3 display
engine and on a evergreen card with DCE-4 display engine, in single-display
and dual-display configuration, with different video modes.

A similar patch is needed for amdgpu-kms to fix the same problem.

Limitations:

- Line buffer sizes in pixels are hard-coded on < DCE-4 to a value
  i just guessed to be high enough to work ok, lacking info on the true
  sizes atm.

Fixes: fdo#93147

Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Michel Dänzer <michel.daenzer@amd.com>
Cc: Harry Wentland <Harry.Wentland@amd.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>

(v1) Tested-by: Dave Witbrodt <dawitbro@sbcglobal.net>

(v2) Refine radeon_flip_work_func() for better efficiency:

     In radeon_flip_work_func, replace the busy waiting udelay(5)
     with event lock held by a more performance and energy efficient
     usleep_range() until at least predicted true start of hw vblank,
     with some slack for scheduler happiness. Release the event lock
     during waits to not delay other outputs in doing their stuff, as
     the waiting can last up to 200 usecs in some cases.

     Retested on DCE-3 and DCE-4 to verify it still works nicely.

(v2) Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
Signed-off-by: Alex Deucher <alexander.deucher@amd.com>

9 files changed, 164 insertions, 29 deletions

diff --git a/drivers/gpu/drm/radeon/cik.c b/drivers/gpu/drm/radeon/cik.c
index 6801a0c2631b..0154db43860c 100644
--- a/drivers/gpu/drm/radeon/cik.c
+++ b/drivers/gpu/drm/radeon/cik.c
@@ -9630,6 +9630,9 @@ static void dce8_program_watermarks(struct radeon_device *rdev,
                     (rdev->disp_priority == 2)) {
                         DRM_DEBUG_KMS("force priority to high\n");
                 }
+
+                /* Save number of lines the linebuffer leads before the scanout */
+                radeon_crtc->lb_vblank_lead_lines = DIV_ROUND_UP(lb_size, mode->crtc_hdisplay);
         }
 
         /* select wm A */
diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c
index f61d66495ba2..2ad462896896 100644
--- a/drivers/gpu/drm/radeon/evergreen.c
+++ b/drivers/gpu/drm/radeon/evergreen.c
@@ -2372,6 +2372,9 @@ static void evergreen_program_watermarks(struct radeon_device *rdev,
                 c.full = dfixed_div(c, a);
                 priority_b_mark = dfixed_trunc(c);
                 priority_b_cnt |= priority_b_mark & PRIORITY_MARK_MASK;
+
+                /* Save number of lines the linebuffer leads before the scanout */
+                radeon_crtc->lb_vblank_lead_lines = DIV_ROUND_UP(lb_size, mode->crtc_hdisplay);
         }
 
         /* select wm A */
diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c
index 2df3c860beb3..9e7e2bf03b81 100644
--- a/drivers/gpu/drm/radeon/r100.c
+++ b/drivers/gpu/drm/radeon/r100.c
@@ -3217,6 +3217,9 @@ void r100_bandwidth_update(struct radeon_device *rdev)
         uint32_t pixel_bytes1 = 0;
         uint32_t pixel_bytes2 = 0;
 
+        /* Guess line buffer size to be 8192 pixels */
+        u32 lb_size = 8192;
+
         if (!rdev->mode_info.mode_config_initialized)
                 return;
 
@@ -3631,6 +3634,13 @@ void r100_bandwidth_update(struct radeon_device *rdev)
                 DRM_DEBUG_KMS("GRPH2_BUFFER_CNTL from to %x\n",
                           (unsigned int)RREG32(RADEON_GRPH2_BUFFER_CNTL));
         }
+
+        /* Save number of lines the linebuffer leads before the scanout */
+        if (mode1)
+            rdev->mode_info.crtcs[0]->lb_vblank_lead_lines = DIV_ROUND_UP(lb_size, mode1->crtc_hdisplay);
+
+        if (mode2)
+            rdev->mode_info.crtcs[1]->lb_vblank_lead_lines = DIV_ROUND_UP(lb_size, mode2->crtc_hdisplay);
 }
 
 int r100_ring_test(struct radeon_device *rdev, struct radeon_ring *ring)
diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
index a8d9927ed9eb..1eca0acac016 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -322,7 +322,9 @@ void radeon_crtc_handle_vblank(struct radeon_device *rdev, int crtc_id)
          * to complete in this vblank?
          */
         if (update_pending &&
-            (DRM_SCANOUTPOS_VALID & radeon_get_crtc_scanoutpos(rdev->ddev, crtc_id, 0,
+            (DRM_SCANOUTPOS_VALID & radeon_get_crtc_scanoutpos(rdev->ddev,
+                                                               crtc_id,
+                                                               USE_REAL_VBLANKSTART,
                                                                &vpos, &hpos, NULL, NULL,
                                                                &rdev->mode_info.crtcs[crtc_id]->base.hwmode)) &&
             ((vpos >= (99 * rdev->mode_info.crtcs[crtc_id]->base.hwmode.crtc_vdisplay)/100) ||
@@ -401,6 +403,8 @@ static void radeon_flip_work_func(struct work_struct *__work)
         struct drm_crtc *crtc = &radeon_crtc->base;
         unsigned long flags;
         int r;
+        int vpos, hpos, stat, min_udelay;
+        struct drm_vblank_crtc *vblank = &crtc->dev->vblank[work->crtc_id];
 
         down_read(&rdev->exclusive_lock);
         if (work->fence) {
@@ -437,6 +441,41 @@ static void radeon_flip_work_func(struct work_struct *__work)
         /* set the proper interrupt */
         radeon_irq_kms_pflip_irq_get(rdev, radeon_crtc->crtc_id);
 
+        /* If this happens to execute within the "virtually extended" vblank
+         * interval before the start of the real vblank interval then it needs
+         * to delay programming the mmio flip until the real vblank is entered.
+         * This prevents completing a flip too early due to the way we fudge
+         * our vblank counter and vblank timestamps in order to work around the
+         * problem that the hw fires vblank interrupts before actual start of
+         * vblank (when line buffer refilling is done for a frame). It
+         * complements the fudging logic in radeon_get_crtc_scanoutpos() for
+         * timestamping and radeon_get_vblank_counter_kms() for vblank counts.
+         *
+         * In practice this won't execute very often unless on very fast
+         * machines because the time window for this to happen is very small.
+         */
+        for (;;) {
+                /* GET_DISTANCE_TO_VBLANKSTART returns distance to real vblank
+                 * start in hpos, and to the "fudged earlier" vblank start in
+                 * vpos.
+                 */
+                stat = radeon_get_crtc_scanoutpos(rdev->ddev, work->crtc_id,
+                                                  GET_DISTANCE_TO_VBLANKSTART,
+                                                  &vpos, &hpos, NULL, NULL,
+                                                  &crtc->hwmode);
+
+                if ((stat & (DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_ACCURATE)) !=
+                    (DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_ACCURATE) ||
+                    !(vpos >= 0 && hpos <= 0))
+                        break;
+
+                /* Sleep at least until estimated real start of hw vblank */
+                spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
+                min_udelay = (-hpos + 1) * max(vblank->linedur_ns / 1000, 5);
+                usleep_range(min_udelay, 2 * min_udelay);
+                spin_lock_irqsave(&crtc->dev->event_lock, flags);
+        };
+
         /* do the flip (mmio) */
         radeon_page_flip(rdev, radeon_crtc->crtc_id, work->base);
 
@@ -1768,6 +1807,15 @@ bool radeon_crtc_scaling_mode_fixup(struct drm_crtc *crtc,
  * \param dev Device to query.
  * \param crtc Crtc to query.
  * \param flags Flags from caller (DRM_CALLED_FROM_VBLIRQ or 0).
+ *              For driver internal use only also supports these flags:
+ *
+ *              USE_REAL_VBLANKSTART to use the real start of vblank instead
+ *              of a fudged earlier start of vblank.
+ *
+ *              GET_DISTANCE_TO_VBLANKSTART to return distance to the
+ *              fudged earlier start of vblank in *vpos and the distance
+ *              to true start of vblank in *hpos.
+ *
  * \param *vpos Location where vertical scanout position should be stored.
  * \param *hpos Location where horizontal scanout position should go.
  * \param *stime Target location for timestamp taken immediately before
@@ -1911,10 +1959,40 @@ int radeon_get_crtc_scanoutpos(struct drm_device *dev, unsigned int pipe,
                 vbl_end = 0;
         }
 
+        /* Called from driver internal vblank counter query code? */
+        if (flags & GET_DISTANCE_TO_VBLANKSTART) {
+            /* Caller wants distance from real vbl_start in *hpos */
+            *hpos = *vpos - vbl_start;
+        }
+
+        /* Fudge vblank to start a few scanlines earlier to handle the
+         * problem that vblank irqs fire a few scanlines before start
+         * of vblank. Some driver internal callers need the true vblank
+         * start to be used and signal this via the USE_REAL_VBLANKSTART flag.
+         *
+         * The cause of the "early" vblank irq is that the irq is triggered
+         * by the line buffer logic when the line buffer read position enters
+         * the vblank, whereas our crtc scanout position naturally lags the
+         * line buffer read position.
+         */
+        if (!(flags & USE_REAL_VBLANKSTART))
+                vbl_start -= rdev->mode_info.crtcs[pipe]->lb_vblank_lead_lines;
+
         /* Test scanout position against vblank region. */
         if ((*vpos < vbl_start) && (*vpos >= vbl_end))
                 in_vbl = false;
 
+        /* In vblank? */
+        if (in_vbl)
+            ret |= DRM_SCANOUTPOS_IN_VBLANK;
+
+        /* Called from driver internal vblank counter query code? */
+        if (flags & GET_DISTANCE_TO_VBLANKSTART) {
+                /* Caller wants distance from fudged earlier vbl_start */
+                *vpos -= vbl_start;
+                return ret;
+        }
+
         /* Check if inside vblank area and apply corrective offsets:
          * vpos will then be >=0 in video scanout area, but negative
          * within vblank area, counting down the number of lines until
@@ -1930,31 +2008,5 @@ int radeon_get_crtc_scanoutpos(struct drm_device *dev, unsigned int pipe,
         /* Correct for shifted end of vbl at vbl_end. */
         *vpos = *vpos - vbl_end;
 
-        /* In vblank? */
-        if (in_vbl)
-                ret |= DRM_SCANOUTPOS_IN_VBLANK;
-
-        /* Is vpos outside nominal vblank area, but less than
-         * 1/100 of a frame height away from start of vblank?
-         * If so, assume this isn't a massively delayed vblank
-         * interrupt, but a vblank interrupt that fired a few
-         * microseconds before true start of vblank. Compensate
-         * by adding a full frame duration to the final timestamp.
-         * Happens, e.g., on ATI R500, R600.
-         *
-         * We only do this if DRM_CALLED_FROM_VBLIRQ.
-         */
-        if ((flags & DRM_CALLED_FROM_VBLIRQ) && !in_vbl) {
-                vbl_start = mode->crtc_vdisplay;
-                vtotal = mode->crtc_vtotal;
-
-                if (vbl_start - *vpos < vtotal / 100) {
-                        *vpos -= vtotal;
-
-                        /* Signal this correction as "applied". */
-                        ret |= 0x8;
-                }
-        }
-
         return ret;
 }
diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c
index 0ec6fcca16d3..d290a8a09036 100644
--- a/drivers/gpu/drm/radeon/radeon_kms.c
+++ b/drivers/gpu/drm/radeon/radeon_kms.c
@@ -755,6 +755,8 @@ void radeon_driver_preclose_kms(struct drm_device *dev,
  */
 u32 radeon_get_vblank_counter_kms(struct drm_device *dev, int crtc)
 {
+        int vpos, hpos, stat;
+        u32 count;
         struct radeon_device *rdev = dev->dev_private;
 
         if (crtc < 0 || crtc >= rdev->num_crtc) {
@@ -762,7 +764,53 @@ u32 radeon_get_vblank_counter_kms(struct drm_device *dev, int crtc)
                 return -EINVAL;
         }
 
-        return radeon_get_vblank_counter(rdev, crtc);
+        /* The hw increments its frame counter at start of vsync, not at start
+         * of vblank, as is required by DRM core vblank counter handling.
+         * Cook the hw count here to make it appear to the caller as if it
+         * incremented at start of vblank. We measure distance to start of
+         * vblank in vpos. vpos therefore will be >= 0 between start of vblank
+         * and start of vsync, so vpos >= 0 means to bump the hw frame counter
+         * result by 1 to give the proper appearance to caller.
+         */
+        if (rdev->mode_info.crtcs[crtc]) {
+                /* Repeat readout if needed to provide stable result if
+                 * we cross start of vsync during the queries.
+                 */
+                do {
+                        count = radeon_get_vblank_counter(rdev, crtc);
+                        /* Ask radeon_get_crtc_scanoutpos to return vpos as
+                         * distance to start of vblank, instead of regular
+                         * vertical scanout pos.
+                         */
+                        stat = radeon_get_crtc_scanoutpos(
+                                dev, crtc, GET_DISTANCE_TO_VBLANKSTART,
+                                &vpos, &hpos, NULL, NULL,
+                                &rdev->mode_info.crtcs[crtc]->base.hwmode);
+                } while (count != radeon_get_vblank_counter(rdev, crtc));
+
+                if (((stat & (DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_ACCURATE)) !=
+                    (DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_ACCURATE))) {
+                        DRM_DEBUG_VBL("Query failed! stat %d\n", stat);
+                }
+                else {
+                        DRM_DEBUG_VBL("crtc %d: dist from vblank start %d\n",
+                                      crtc, vpos);
+
+                        /* Bump counter if we are at >= leading edge of vblank,
+                         * but before vsync where vpos would turn negative and
+                         * the hw counter really increments.
+                         */
+                        if (vpos >= 0)
+                                count++;
+                }
+        }
+        else {
+            /* Fallback to use value as is. */
+            count = radeon_get_vblank_counter(rdev, crtc);
+            DRM_DEBUG_VBL("NULL mode info! Returned count may be wrong.\n");
+        }
+
+        return count;
 }
 
 /**
diff --git a/drivers/gpu/drm/radeon/radeon_mode.h b/drivers/gpu/drm/radeon/radeon_mode.h
index 0fa3e255a545..bba112628b47 100644
--- a/drivers/gpu/drm/radeon/radeon_mode.h
+++ b/drivers/gpu/drm/radeon/radeon_mode.h
@@ -367,6 +367,7 @@ struct radeon_crtc {
         u32 line_time;
         u32 wm_low;
         u32 wm_high;
+        u32 lb_vblank_lead_lines;
         struct drm_display_mode hw_mode;
         enum radeon_output_csc output_csc;
 };
@@ -687,6 +688,9 @@ struct atom_voltage_table
         struct atom_voltage_table_entry entries[MAX_VOLTAGE_ENTRIES];
 };
 
+/* Driver internal use only flags of radeon_get_crtc_scanoutpos() */
+#define USE_REAL_VBLANKSTART            (1 << 30)
+#define GET_DISTANCE_TO_VBLANKSTART     (1 << 31)
 
 extern void
 radeon_add_atom_connector(struct drm_device *dev,
diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c
index f4f03dcc1530..59abebd6b5dc 100644
--- a/drivers/gpu/drm/radeon/radeon_pm.c
+++ b/drivers/gpu/drm/radeon/radeon_pm.c
@@ -1756,7 +1756,9 @@ static bool radeon_pm_in_vbl(struct radeon_device *rdev)
          */
         for (crtc = 0; (crtc < rdev->num_crtc) && in_vbl; crtc++) {
                 if (rdev->pm.active_crtcs & (1 << crtc)) {
-                        vbl_status = radeon_get_crtc_scanoutpos(rdev->ddev, crtc, 0,
+                        vbl_status = radeon_get_crtc_scanoutpos(rdev->ddev,
+                                                                crtc,
+                                                                USE_REAL_VBLANKSTART,
                                                                 &vpos, &hpos, NULL, NULL,
                                                                 &rdev->mode_info.crtcs[crtc]->base.hwmode);
                         if ((vbl_status & DRM_SCANOUTPOS_VALID) &&
diff --git a/drivers/gpu/drm/radeon/rs690.c b/drivers/gpu/drm/radeon/rs690.c
index 516ca27cfa12..6bc44c24e837 100644
--- a/drivers/gpu/drm/radeon/rs690.c
+++ b/drivers/gpu/drm/radeon/rs690.c
@@ -207,6 +207,9 @@ void rs690_line_buffer_adjust(struct radeon_device *rdev,
 {
         u32 tmp;
 
+        /* Guess line buffer size to be 8192 pixels */
+        u32 lb_size = 8192;
+
         /*
          * Line Buffer Setup
          * There is a single line buffer shared by both display controllers.
@@ -243,6 +246,13 @@ void rs690_line_buffer_adjust(struct radeon_device *rdev,
                 tmp |= V_006520_DC_LB_MEMORY_SPLIT_D1_1Q_D2_3Q;
         }
         WREG32(R_006520_DC_LB_MEMORY_SPLIT, tmp);
+
+        /* Save number of lines the linebuffer leads before the scanout */
+        if (mode1)
+                rdev->mode_info.crtcs[0]->lb_vblank_lead_lines = DIV_ROUND_UP(lb_size, mode1->crtc_hdisplay);
+
+        if (mode2)
+                rdev->mode_info.crtcs[1]->lb_vblank_lead_lines = DIV_ROUND_UP(lb_size, mode2->crtc_hdisplay);
 }
 
 struct rs690_watermark {
diff --git a/drivers/gpu/drm/radeon/si.c b/drivers/gpu/drm/radeon/si.c
index fb1a7ec1a81c..f878d6962da5 100644
--- a/drivers/gpu/drm/radeon/si.c
+++ b/drivers/gpu/drm/radeon/si.c
@@ -2376,6 +2376,9 @@ static void dce6_program_watermarks(struct radeon_device *rdev,
                 c.full = dfixed_div(c, a);
                 priority_b_mark = dfixed_trunc(c);
                 priority_b_cnt |= priority_b_mark & PRIORITY_MARK_MASK;
+
+                /* Save number of lines the linebuffer leads before the scanout */
+                radeon_crtc->lb_vblank_lead_lines = DIV_ROUND_UP(lb_size, mode->crtc_hdisplay);
         }
 
         /* select wm A */