Attachment 120301 Details for Bug 93147 – First proposed patch to fix this on radeon-kms

[patch] First proposed patch to fix this on radeon-kms

0001-drm-radeon-Fixup-hw-vblank-counter-ts-for-new-drm_up.patch (text/plain), 18.25 KB, created by Mario Kleiner on 2015-12-03 15:02:23 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Mario Kleiner

Created: 2015-12-03 15:02:23 UTC

Size: 18.25 KB

patch

obsolete

>From 22e4f9f8ef0d8016565132efd5000133f01f9aaa Mon Sep 17 00:00:00 2001
>From: Mario Kleiner <mario.kleiner.de@gmail.com>
>Date: Wed, 25 Nov 2015 20:14:31 +0100
>Subject: [PATCH] drm/radeon: Fixup hw vblank counter/ts for new
> drm_update_vblank_count()
>MIME-Version: 1.0
>Content-Type: text/plain; charset=UTF-8
>Content-Transfer-Encoding: 8bit
>
>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:
>
>- Maybe replace the udelay() in the flip_work_func() by a suitable
>  usleep_range() for a bit better efficiency? Will try that.
>
>- 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.
>
>Probably 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>
>---
> drivers/gpu/drm/radeon/cik.c            |   3 +
> drivers/gpu/drm/radeon/evergreen.c      |   3 +
> drivers/gpu/drm/radeon/r100.c           |  10 ++++
> drivers/gpu/drm/radeon/radeon_display.c | 101 +++++++++++++++++++++++---------
> drivers/gpu/drm/radeon/radeon_kms.c     |  50 +++++++++++++++-
> drivers/gpu/drm/radeon/radeon_mode.h    |   4 ++
> drivers/gpu/drm/radeon/radeon_pm.c      |   4 +-
> drivers/gpu/drm/radeon/rs690.c          |  10 ++++
> drivers/gpu/drm/radeon/si.c             |   3 +
> 9 files changed, 159 insertions(+), 29 deletions(-)
>
>diff --git a/drivers/gpu/drm/radeon/cik.c b/drivers/gpu/drm/radeon/cik.c
>index 248953d..356ac7c 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 7f33767..e533db1 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 238b13f..aff1e4d 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 a8d9927..489394b 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,7 @@ 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;
> 
>         down_read(&rdev->exclusive_lock);
> 	if (work->fence) {
>@@ -437,6 +440,37 @@ 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,
>+						  &rdev->mode_info.crtcs[work->crtc_id]->base.hwmode);
>+
>+		if ((stat & (DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_ACCURATE)) !=
>+		    (DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_ACCURATE) ||
>+		    !(vpos >= 0 && hpos <= 0))
>+			break;
>+
>+		udelay(5);
>+	};
>+
> 	/* do the flip (mmio) */
> 	radeon_page_flip(rdev, radeon_crtc->crtc_id, work->base);
> 
>@@ -1768,6 +1802,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 +1954,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 +2003,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 0ec6fcc..d290a8a 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 830e171..f381e99 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;
> };
>@@ -686,6 +687,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 bcdc508..77ca5f2 100644
>--- a/drivers/gpu/drm/radeon/radeon_pm.c
>+++ b/drivers/gpu/drm/radeon/radeon_pm.c
>@@ -1746,7 +1746,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 516ca27..6bc44c2 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 07037e3..3d5d412 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 */
>-- 
>1.9.1
>

Actions: View | Splinter Review

Attachments on bug 93147: 120187 | 120188 | 120301 | 120310 | 120350 | 120351 | 120356

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.