vblank problem (and proposed fix) on crtc > 1
Ilija Hadzic
ihadzic at research.bell-labs.com
Wed Mar 9 09:33:27 PST 2011
Below is an updated patch for ATI DDX (xf86-video-ati library) that
reflects the discussion of this thread. The patch is *cumulative*
(i.e., it includes the changes from a few days ago, so it should
be applied to plain-vanilla DDX, not the one you may have patched
with my patch from last week). I figure it's easier to review it
that way, but if anyone wants an incremental patch, pls. let me know.
The kernel patch and libdrm patch remain the same, so I am not
repeating them here.
The new thing in this patch addresses Michel's concern about spamming the
kernel with ''Unsupported type value' when DDX is ahead of the kernel.
However, I don't think that this can be done by checking KMS_DRIVER_MINOR
nor DRM_IF_MINOR. The reason is that the first version number pertains to
the device driver module (radeon.ko), and there is no change in it
addressed by my patches. So it's inappropriate to bump up this version
number. On the other hand, as far as I could tell I don't see a viable
mechanism to ask the kernel for DRM_IF_MINOR (DRM_IOCTL_SET_VERSION is the
only call that remotely resembles what would be needed and it does more
than just querying). Also, I believe that this version number pertains to
the interface between the drm module and the device driver, not the
userland/kernel interface (if I am wrong, I would appreciate enlightening
from someone who knows better).
So what I did is to actually probe the kernel at screen init time.
When the screen is initialized, the DDX checks if the GPU has more than
two CRTCs and tries a dummy vblank wait on all crtcs > 1. If any
of them fails, it falls back to the old method of using only
primary/secondary flag. That way, an error messsage will appear
in the kernel only once at start up time and never again, which
I think is acceptable (I am sure that at least someone will disagree,
but to me this looks like the best and the most reliable compromise).
Anyway, it only happens when DDX is ahead of the kernel, which should be
less.
So in summary the new behavior is that the new DDX when paired with a new
kernel, VBLANKs will work on all CRTCs the way they are supposed to.
If any of the two components is old, the behavior is identical to the one
we see now (VBLANKs for crtc>1, taken from crtc==1, and if that one
is disconnected, blocking of the application can happen).
-- Ilija
---------------- patch for xf86-video-ati -------------------------------
diff --git a/src/radeon.h b/src/radeon.h
index 4c43717..ad80889 100644
--- a/src/radeon.h
+++ b/src/radeon.h
@@ -930,6 +930,9 @@ typedef struct {
RADEONFBLayout CurrentLayout;
+#ifdef RADEON_DRI2
+ Bool high_crtc_works;
+#endif
#ifdef XF86DRI
Bool directRenderingEnabled;
Bool directRenderingInited;
diff --git a/src/radeon_dri2.c b/src/radeon_dri2.c
index 66df03c..7d77a6b 100644
--- a/src/radeon_dri2.c
+++ b/src/radeon_dri2.c
@@ -783,6 +783,7 @@ static int radeon_dri2_get_msc(DrawablePtr draw, CARD64 *ust, CARD64 *msc)
drmVBlank vbl;
int ret;
int crtc = radeon_dri2_drawable_crtc(draw);
+ int high_crtc = 0;
/* Drawable not displayed, make up a value */
if (crtc == -1) {
@@ -791,8 +792,16 @@ static int radeon_dri2_get_msc(DrawablePtr draw, CARD64 *ust, CARD64 *msc)
return TRUE;
}
vbl.request.type = DRM_VBLANK_RELATIVE;
- if (crtc > 0)
+ if (crtc == 1)
vbl.request.type |= DRM_VBLANK_SECONDARY;
+ else if (crtc > 1) {
+ if (info->high_crtc_works) {
+ high_crtc = (crtc << DRM_VBLANK_HIGH_CRTC_SHIFT) &
+ DRM_VBLANK_HIGH_CRTC_MASK;
+ }
+ else vbl.request.type |= DRM_VBLANK_SECONDARY;
+ }
+ vbl.request.type |= high_crtc;
vbl.request.sequence = 0;
ret = drmWaitVBlank(info->dri2.drm_fd, &vbl);
@@ -825,6 +834,7 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, DrawablePtr draw,
drmVBlank vbl;
int ret, crtc = radeon_dri2_drawable_crtc(draw);
CARD64 current_msc;
+ int high_crtc = 0;
/* Truncate to match kernel interfaces; means occasional overflow
* misses, but that's generally not a big deal */
@@ -855,8 +865,16 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, DrawablePtr draw,
/* Get current count */
vbl.request.type = DRM_VBLANK_RELATIVE;
- if (crtc > 0)
+ if (crtc == 1)
vbl.request.type |= DRM_VBLANK_SECONDARY;
+ else if (crtc > 1) {
+ if (info->high_crtc_works) {
+ high_crtc = (crtc << DRM_VBLANK_HIGH_CRTC_SHIFT) &
+ DRM_VBLANK_HIGH_CRTC_MASK;
+ }
+ else vbl.request.type |= DRM_VBLANK_SECONDARY;
+ }
+ vbl.request.type |= high_crtc;
vbl.request.sequence = 0;
ret = drmWaitVBlank(info->dri2.drm_fd, &vbl);
if (ret) {
@@ -882,8 +900,16 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, DrawablePtr draw,
if (current_msc >= target_msc)
target_msc = current_msc;
vbl.request.type = DRM_VBLANK_ABSOLUTE | DRM_VBLANK_EVENT;
- if (crtc > 0)
+ if (crtc == 1)
vbl.request.type |= DRM_VBLANK_SECONDARY;
+ else if (crtc > 1) {
+ if (info->high_crtc_works) {
+ high_crtc = (crtc << DRM_VBLANK_HIGH_CRTC_SHIFT) &
+ DRM_VBLANK_HIGH_CRTC_MASK;
+ }
+ else vbl.request.type |= DRM_VBLANK_SECONDARY;
+ }
+ vbl.request.type |= high_crtc;
vbl.request.sequence = target_msc;
vbl.request.signal = (unsigned long)wait_info;
ret = drmWaitVBlank(info->dri2.drm_fd, &vbl);
@@ -903,8 +929,15 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, DrawablePtr draw,
* so we queue an event that will satisfy the divisor/remainder equation.
*/
vbl.request.type = DRM_VBLANK_ABSOLUTE | DRM_VBLANK_EVENT;
- if (crtc > 0)
+ if (crtc == 1)
vbl.request.type |= DRM_VBLANK_SECONDARY;
+ else if (crtc > 1) {
+ if (info->high_crtc_works) {
+ high_crtc = (crtc << DRM_VBLANK_HIGH_CRTC_SHIFT) &
+ DRM_VBLANK_HIGH_CRTC_MASK;
+ } else vbl.request.type |= DRM_VBLANK_SECONDARY;
+ }
+ vbl.request.type |= high_crtc;
vbl.request.sequence = current_msc - (current_msc % divisor) +
remainder;
@@ -1029,6 +1062,7 @@ static int radeon_dri2_schedule_swap(ClientPtr client, DrawablePtr draw,
CARD64 current_msc;
BoxRec box;
RegionRec region;
+ int high_crtc = 0;
/* Truncate to match kernel interfaces; means occasional overflow
* misses, but that's generally not a big deal */
@@ -1068,8 +1102,15 @@ static int radeon_dri2_schedule_swap(ClientPtr client, DrawablePtr draw,
/* Get current count */
vbl.request.type = DRM_VBLANK_RELATIVE;
- if (crtc > 0)
+ if (crtc == 1)
vbl.request.type |= DRM_VBLANK_SECONDARY;
+ else if (crtc > 1) {
+ if (info->high_crtc_works) {
+ high_crtc = (crtc << DRM_VBLANK_HIGH_CRTC_SHIFT) &
+ DRM_VBLANK_HIGH_CRTC_MASK;
+ } else vbl.request.type |= DRM_VBLANK_SECONDARY;
+ }
+ vbl.request.type |= high_crtc;
vbl.request.sequence = 0;
ret = drmWaitVBlank(info->dri2.drm_fd, &vbl);
if (ret) {
@@ -1111,8 +1152,15 @@ static int radeon_dri2_schedule_swap(ClientPtr client, DrawablePtr draw,
*/
if (flip == 0)
vbl.request.type |= DRM_VBLANK_NEXTONMISS;
- if (crtc > 0)
+ if (crtc == 1)
vbl.request.type |= DRM_VBLANK_SECONDARY;
+ else if (crtc > 1) {
+ if (info->high_crtc_works) {
+ high_crtc = (crtc << DRM_VBLANK_HIGH_CRTC_SHIFT) &
+ DRM_VBLANK_HIGH_CRTC_MASK;
+ } else vbl.request.type |= DRM_VBLANK_SECONDARY;
+ }
+ vbl.request.type |= high_crtc;
/* If target_msc already reached or passed, set it to
* current_msc to ensure we return a reasonable value back
@@ -1145,8 +1193,15 @@ static int radeon_dri2_schedule_swap(ClientPtr client, DrawablePtr draw,
vbl.request.type = DRM_VBLANK_ABSOLUTE | DRM_VBLANK_EVENT;
if (flip == 0)
vbl.request.type |= DRM_VBLANK_NEXTONMISS;
- if (crtc > 0)
+ if (crtc == 1)
vbl.request.type |= DRM_VBLANK_SECONDARY;
+ else if (crtc > 1) {
+ if (info->high_crtc_works) {
+ high_crtc = (crtc << DRM_VBLANK_HIGH_CRTC_SHIFT) &
+ DRM_VBLANK_HIGH_CRTC_MASK;
+ } else vbl.request.type |= DRM_VBLANK_SECONDARY;
+ }
+ vbl.request.type |= high_crtc;
vbl.request.sequence = current_msc - (current_msc % divisor) +
remainder;
@@ -1217,6 +1272,8 @@ radeon_dri2_screen_init(ScreenPtr pScreen)
DRI2InfoRec dri2_info = { 0 };
#ifdef USE_DRI2_SCHEDULING
const char *driverNames[1];
+ drmVBlank vbl;
+ int c;
#endif
if (!info->useEXA) {
@@ -1248,6 +1305,7 @@ radeon_dri2_screen_init(ScreenPtr pScreen)
#endif
dri2_info.CopyRegion = radeon_dri2_copy_region;
+ info->high_crtc_works = FALSE;
#ifdef USE_DRI2_SCHEDULING
if (info->dri->pKernelDRMVersion->version_minor >= 4) {
dri2_info.version = 4;
@@ -1261,6 +1319,22 @@ radeon_dri2_screen_init(ScreenPtr pScreen)
xf86DrvMsg(pScrn->scrnIndex, X_WARNING, "You need a newer kernel for sync extension\n");
}
+ if (info->drmmode.mode_res->count_crtcs) {
+ xf86DrvMsg(pScrn->scrnIndex, X_INFO, "GPU with %d CRTCs found, probing VBLANKs on CRTC>1\n",
+ info->drmmode.mode_res->count_crtcs);
+ info->high_crtc_works = TRUE;
+ for (c=2; c<info->drmmode.mode_res->count_crtcs; c++) {
+ vbl.request.type = DRM_VBLANK_RELATIVE;
+ vbl.request.type |= (c << DRM_VBLANK_HIGH_CRTC_SHIFT) & DRM_VBLANK_HIGH_CRTC_MASK;
+ vbl.request.sequence = 0;
+ if (drmWaitVBlank(info->dri2.drm_fd, &vbl)) {
+ xf86DrvMsg(pScrn->scrnIndex, X_WARNING, "Kernel rejects VBLANK request on CRTC %d\n", c);
+ info->high_crtc_works = FALSE;
+ }
+ }
+ if (info->high_crtc_works) xf86DrvMsg(pScrn->scrnIndex, X_INFO, "VBLANK request accepted on all CRTCs\n");
+ }
+
if (pRADEONEnt->dri2_info_cnt == 0) {
#if HAS_DIXREGISTERPRIVATEKEY
if (!dixRegisterPrivateKey(DRI2ClientEventsPrivateKey, PRIVATE_CLIENT, sizeof(DRI2ClientEventsRec))) {
More information about the dri-devel
mailing list