[PATCH xserver] modesetting: Remove ms_crtc_msc_to_kernel_msc().
Mario Kleiner
mario.kleiner.de at gmail.com
Fri May 4 12:14:10 UTC 2018
The function is ported from intel-ddx uxa backend around
2013, where its stated purpose was to apply a vblank_offset
to msc values to correct for problems with those kernel
provided msc values. Some (somewhat magic and puzzling to
myself) heuristic tried to guess if provided values were
unreasonable and tried to adapt the corrective vblank_offset
to account for that.
Except: It wasn't applied to kernel provided msc values,
but the values delivered by clients via DRI2 or Present,
so valid client targetmsc values, e.g., requesting a vblank
event > 1000 vblanks in the future, triggered the offset
correction in arbitrarily wrong ways, leading to wrong msc
values being returned and thereby vblank events queued to
the kernel for the wrong time. This causes glXSwapBuffersMscOML
and glXWaitForMscOML to swap / return immediately whenever a
swap/wait in > 1000 vblanks is requested.
The original code was also written to only deal with 32 bit mscs,
but server 1.20 modesetting ddx can now use new Linux 4.15+ kernel
vblank api to process true 64 bit msc's, which may confuse the
heuristic even more due to 32 bit integer truncation/wrapping.
This code caused various problems in the intel-ddx in the
past since year 2013, and was removed there in 2015 by Chris
Wilson in commit 42ebe2ef9646be5c4586868cf332b4cd79bb4618:
" uxa: Remove the filtering of bogus Present MSC values
If the intention was to filter the return values from the kernel, the
filtering would have been applied to the kernel values and not to the
incoming values from Present. This filtering introduces crazy integer
promotion and truncation bugs all because Present feeds garbage into its
vblank requests.
"
Indeed, i found a Mesa bug yesterday which can cause Mesa's
PresentPixmap request to spuriously feed garbage targetMSC's
into the driver under some conditions. However, while other
video drivers seem to cope relatively well with that, modesetting
ddx causes KDE-5's plasmashell to lock up badly quite frequently,
and my suspicion is that the code removed in this commit is one
major source of the extra fragility.
Also my own tests fail for any swap scheduled more than 1000
vblanks into the future, which is not uncommon for some scientific
applications.
Iow. modesetting's swap scheduling seems to be more robust without
this function afaics.
Signed-off-by: Mario Kleiner <mario.kleiner.de at gmail.com>
Cc: Chris Wilson <chris at chris-wilson.co.uk>
Cc: Keith Packard <keithp at keithp.com>
Cc: Adam Jackson <ajax at redhat.com>
---
hw/xfree86/drivers/modesetting/driver.h | 1 -
hw/xfree86/drivers/modesetting/drmmode_display.h | 1 -
hw/xfree86/drivers/modesetting/vblank.c | 45 ++----------------------
3 files changed, 3 insertions(+), 44 deletions(-)
diff --git a/hw/xfree86/drivers/modesetting/driver.h b/hw/xfree86/drivers/modesetting/driver.h
index b2a4e52..3a81d4d 100644
--- a/hw/xfree86/drivers/modesetting/driver.h
+++ b/hw/xfree86/drivers/modesetting/driver.h
@@ -149,7 +149,6 @@ xf86CrtcPtr ms_dri2_crtc_covering_drawable(DrawablePtr pDraw);
int ms_get_crtc_ust_msc(xf86CrtcPtr crtc, CARD64 *ust, CARD64 *msc);
-uint64_t ms_crtc_msc_to_kernel_msc(xf86CrtcPtr crtc, uint64_t expect);
uint64_t ms_kernel_msc_to_crtc_msc(xf86CrtcPtr crtc, uint64_t sequence);
diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.h b/hw/xfree86/drivers/modesetting/drmmode_display.h
index 74954b8..e46a292 100644
--- a/hw/xfree86/drivers/modesetting/drmmode_display.h
+++ b/hw/xfree86/drivers/modesetting/drmmode_display.h
@@ -180,7 +180,6 @@ typedef struct {
* lies, and we need to give a reliable 64-bit msc for GL, so we
* have to track and convert to a userland-tracked 64-bit msc.
*/
- int32_t vblank_offset;
uint32_t msc_prev;
uint64_t msc_high;
/** @} */
diff --git a/hw/xfree86/drivers/modesetting/vblank.c b/hw/xfree86/drivers/modesetting/vblank.c
index ae3018b..d1a6fc1 100644
--- a/hw/xfree86/drivers/modesetting/vblank.c
+++ b/hw/xfree86/drivers/modesetting/vblank.c
@@ -226,7 +226,6 @@ ms_queue_vblank(xf86CrtcPtr crtc, ms_queue_flag flags,
/* Queue an event at the specified sequence */
if (ms->has_queue_sequence || !ms->tried_queue_sequence) {
uint32_t drm_flags = 0;
- uint64_t kernel;
uint64_t kernel_queued;
ms->tried_queue_sequence = TRUE;
@@ -236,10 +235,8 @@ ms_queue_vblank(xf86CrtcPtr crtc, ms_queue_flag flags,
if (flags & MS_QUEUE_NEXT_ON_MISS)
drm_flags |= DRM_CRTC_SEQUENCE_NEXT_ON_MISS;
- kernel = ms_crtc_msc_to_kernel_msc(crtc, msc);
ret = drmCrtcQueueSequence(ms->fd, drmmode_crtc->mode_crtc->crtc_id,
- drm_flags,
- kernel, &kernel_queued, seq);
+ drm_flags, msc, &kernel_queued, seq);
if (ret == 0) {
if (msc_queued)
*msc_queued = ms_kernel_msc_to_crtc_msc(crtc, kernel_queued);
@@ -260,8 +257,7 @@ ms_queue_vblank(xf86CrtcPtr crtc, ms_queue_flag flags,
if (flags & MS_QUEUE_NEXT_ON_MISS)
vbl.request.type |= DRM_VBLANK_NEXTONMISS;
- vbl.request.sequence = (flags & MS_QUEUE_RELATIVE) ?
- msc : ms_crtc_msc_to_kernel_msc(crtc, msc);
+ vbl.request.sequence = msc;
vbl.request.signal = seq;
ret = drmWaitVBlank(ms->fd, &vbl);
if (ret == 0) {
@@ -280,14 +276,12 @@ ms_queue_vblank(xf86CrtcPtr crtc, ms_queue_flag flags,
/**
* Convert a 32-bit kernel MSC sequence number to a 64-bit local sequence
- * number, adding in the vblank_offset and high 32 bits, and dealing
- * with 64-bit wrapping
+ * number, adding in the high 32 bits, and dealing with 64-bit wrapping.
*/
uint64_t
ms_kernel_msc_to_crtc_msc(xf86CrtcPtr crtc, uint64_t sequence)
{
drmmode_crtc_private_rec *drmmode_crtc = crtc->driver_private;
- sequence += drmmode_crtc->vblank_offset;
if ((int32_t) (sequence - drmmode_crtc->msc_prev) < -0x40000000)
drmmode_crtc->msc_high += 0x100000000L;
@@ -307,39 +301,6 @@ ms_get_crtc_ust_msc(xf86CrtcPtr crtc, CARD64 *ust, CARD64 *msc)
return Success;
}
-#define MAX_VBLANK_OFFSET 1000
-
-/**
- * Convert a 64-bit adjusted MSC value into a 64-bit kernel sequence number,
- * by subtracting out the vblank_offset term.
- *
- * This also updates the vblank_offset when it notices that the value should
- * change.
- */
-uint64_t
-ms_crtc_msc_to_kernel_msc(xf86CrtcPtr crtc, uint64_t expect)
-{
- drmmode_crtc_private_rec *drmmode_crtc = crtc->driver_private;
- uint64_t msc;
- uint64_t ust;
- int64_t diff;
-
- if (ms_get_crtc_ust_msc(crtc, &ust, &msc) == Success) {
- diff = expect - msc;
-
- /* We're way off here, assume that the kernel has lost its mind
- * and smack the vblank back to something sensible
- */
- if (diff < -MAX_VBLANK_OFFSET || MAX_VBLANK_OFFSET < diff) {
- drmmode_crtc->vblank_offset += (int32_t) diff;
- if (drmmode_crtc->vblank_offset > -MAX_VBLANK_OFFSET &&
- drmmode_crtc->vblank_offset < MAX_VBLANK_OFFSET)
- drmmode_crtc->vblank_offset = 0;
- }
- }
- return (expect - drmmode_crtc->vblank_offset);
-}
-
/**
* Check for pending DRM events and process them.
*/
--
2.7.4
More information about the xorg-devel
mailing list