Hi Alex,
Below is a patch against the master branch of xf86-video-ati that adds support for waits on vblank events on CRTCs that are greater than 1 (and thus cannot be represented using current primary/secondary flags interface). The patch makes use of GET_CAP ioctl to check whether vblanks on crtc > 1 are supported by the kernel. If they are not falls back to legacy behavior. Otherwise, it uses correct crtc numbers when waiting on vblank and thus corrects the problem seen in certain multiscreen configurations.
The issue was discussed on the dri-devel list in these two threads
http://lists.freedesktop.org/archives/dri-devel/2011-March/009009.html http://lists.freedesktop.org/archives/dri-devel/2011-March/009025.html
regards,
Ilija
Reviewed-by: Mario Kleiner <mario.kleiner at tuebingen.mpg.de> Acked-by: Mario Kleiner <mario.kleiner at tuebingen.mpg.de>
diff --git a/src/radeon.h b/src/radeon.h index a6d20d7..1a746c7 100644 --- a/src/radeon.h +++ b/src/radeon.h @@ -931,6 +931,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..ed27dad 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,16 @@ 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 +1063,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 +1103,16 @@ 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 +1154,16 @@ 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 +1196,16 @@ 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 +1276,7 @@ radeon_dri2_screen_init(ScreenPtr pScreen) DRI2InfoRec dri2_info = { 0 }; #ifdef USE_DRI2_SCHEDULING const char *driverNames[1]; + uint64_t cap_value; #endif
if (!info->useEXA) { @@ -1248,6 +1308,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 +1322,20 @@ 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 > 2) { + if (drmGetCap(info->dri2.drm_fd, DRM_CAP_HIGH_CRTC, &cap_value)) { + info->high_crtc_works = FALSE; + xf86DrvMsg(pScrn->scrnIndex, X_WARNING, "You need a newer kernel for VBLANKs on CRTC>1\n"); + } else { + if (cap_value) { + info->high_crtc_works = TRUE; + } else { + xf86DrvMsg(pScrn->scrnIndex, X_WARNING, "Your kernel does not handle VBLANKs on CRTC>1\n"); + info->high_crtc_works = FALSE; + } + } + } + if (pRADEONEnt->dri2_info_cnt == 0) { #if HAS_DIXREGISTERPRIVATEKEY if (!dixRegisterPrivateKey(DRI2ClientEventsPrivateKey, PRIVATE_CLIENT, sizeof(DRI2ClientEventsRec))) {
On Fri, 18 Mar 2011 16:58:39 -0500 (CDT) Ilija Hadzic ihadzic@research.bell-labs.com wrote:
Hi Alex,
Below is a patch against the master branch of xf86-video-ati that adds support for waits on vblank events on CRTCs that are greater than 1 (and thus cannot be represented using current primary/secondary flags interface). The patch makes use of GET_CAP ioctl to check whether vblanks on crtc > 1 are supported by the kernel. If they are not falls back to legacy behavior. Otherwise, it uses correct crtc numbers when waiting on vblank and thus corrects the problem seen in certain multiscreen configurations.
The issue was discussed on the dri-devel list in these two threads
http://lists.freedesktop.org/archives/dri-devel/2011-March/009009.html http://lists.freedesktop.org/archives/dri-devel/2011-March/009025.html
The duplicated code in each function is begging to get pulled out into a separate function...
On Fri, 18 Mar 2011, Jesse Barnes wrote:
The duplicated code in each function is begging to get pulled out into a separate function...
How about this then ?
diff --git a/src/radeon.h b/src/radeon.h index a6d20d7..1a746c7 100644 --- a/src/radeon.h +++ b/src/radeon.h @@ -931,6 +931,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..c461469 100644 --- a/src/radeon_dri2.c +++ b/src/radeon_dri2.c @@ -771,6 +771,24 @@ cleanup: free(event); }
+static drmVBlankSeqType populate_vbl_request_type(RADEONInfoPtr info, int crtc) +{ + int high_crtc = 0; + drmVBlankSeqType type = 0; + + if (crtc == 1) + 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 + type |= DRM_VBLANK_SECONDARY; + } + type |= high_crtc; + return type; +} + /* * Get current frame count and frame count timestamp, based on drawable's * crtc. @@ -791,8 +809,7 @@ static int radeon_dri2_get_msc(DrawablePtr draw, CARD64 *ust, CARD64 *msc) return TRUE; } vbl.request.type = DRM_VBLANK_RELATIVE; - if (crtc > 0) - vbl.request.type |= DRM_VBLANK_SECONDARY; + vbl.request.type |= populate_vbl_request_type(info, crtc); vbl.request.sequence = 0;
ret = drmWaitVBlank(info->dri2.drm_fd, &vbl); @@ -855,8 +872,7 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, DrawablePtr draw,
/* Get current count */ vbl.request.type = DRM_VBLANK_RELATIVE; - if (crtc > 0) - vbl.request.type |= DRM_VBLANK_SECONDARY; + vbl.request.type |= populate_vbl_request_type(info, crtc); vbl.request.sequence = 0; ret = drmWaitVBlank(info->dri2.drm_fd, &vbl); if (ret) { @@ -882,8 +898,7 @@ 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) - vbl.request.type |= DRM_VBLANK_SECONDARY; + vbl.request.type |= populate_vbl_request_type(info, crtc); vbl.request.sequence = target_msc; vbl.request.signal = (unsigned long)wait_info; ret = drmWaitVBlank(info->dri2.drm_fd, &vbl); @@ -903,8 +918,7 @@ 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) - vbl.request.type |= DRM_VBLANK_SECONDARY; + vbl.request.type |= populate_vbl_request_type(info, crtc);
vbl.request.sequence = current_msc - (current_msc % divisor) + remainder; @@ -1068,8 +1082,7 @@ static int radeon_dri2_schedule_swap(ClientPtr client, DrawablePtr draw,
/* Get current count */ vbl.request.type = DRM_VBLANK_RELATIVE; - if (crtc > 0) - vbl.request.type |= DRM_VBLANK_SECONDARY; + vbl.request.type |= populate_vbl_request_type(info, crtc); vbl.request.sequence = 0; ret = drmWaitVBlank(info->dri2.drm_fd, &vbl); if (ret) { @@ -1111,8 +1124,7 @@ static int radeon_dri2_schedule_swap(ClientPtr client, DrawablePtr draw, */ if (flip == 0) vbl.request.type |= DRM_VBLANK_NEXTONMISS; - if (crtc > 0) - vbl.request.type |= DRM_VBLANK_SECONDARY; + vbl.request.type |= populate_vbl_request_type(info, crtc);
/* If target_msc already reached or passed, set it to * current_msc to ensure we return a reasonable value back @@ -1145,8 +1157,7 @@ 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) - vbl.request.type |= DRM_VBLANK_SECONDARY; + vbl.request.type |= populate_vbl_request_type(info, crtc);
vbl.request.sequence = current_msc - (current_msc % divisor) + remainder; @@ -1217,6 +1228,7 @@ radeon_dri2_screen_init(ScreenPtr pScreen) DRI2InfoRec dri2_info = { 0 }; #ifdef USE_DRI2_SCHEDULING const char *driverNames[1]; + uint64_t cap_value; #endif
if (!info->useEXA) { @@ -1248,6 +1260,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 +1274,20 @@ 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 > 2) { + if (drmGetCap(info->dri2.drm_fd, DRM_CAP_HIGH_CRTC, &cap_value)) { + info->high_crtc_works = FALSE; + xf86DrvMsg(pScrn->scrnIndex, X_WARNING, "You need a newer kernel for VBLANKs on CRTC>1\n"); + } else { + if (cap_value) { + info->high_crtc_works = TRUE; + } else { + xf86DrvMsg(pScrn->scrnIndex, X_WARNING, "Your kernel does not handle VBLANKs on CRTC>1\n"); + info->high_crtc_works = FALSE; + } + } + } + if (pRADEONEnt->dri2_info_cnt == 0) { #if HAS_DIXREGISTERPRIVATEKEY if (!dixRegisterPrivateKey(DRI2ClientEventsPrivateKey, PRIVATE_CLIENT, sizeof(DRI2ClientEventsRec))) {
On Fri, Mar 18, 2011 at 5:58 PM, Ilija Hadzic ihadzic@research.bell-labs.com wrote:
Hi Alex,
Below is a patch against the master branch of xf86-video-ati that adds support for waits on vblank events on CRTCs that are greater than 1 (and thus cannot be represented using current primary/secondary flags interface). The patch makes use of GET_CAP ioctl to check whether vblanks on crtc > 1 are supported by the kernel. If they are not falls back to legacy behavior. Otherwise, it uses correct crtc numbers when waiting on vblank and thus corrects the problem seen in certain multiscreen configurations.
The issue was discussed on the dri-devel list in these two threads
http://lists.freedesktop.org/archives/dri-devel/2011-March/009009.html http://lists.freedesktop.org/archives/dri-devel/2011-March/009025.html
regards,
Ilija
Reviewed-by: Mario Kleiner <mario.kleiner at tuebingen.mpg.de> Acked-by: Mario Kleiner <mario.kleiner at tuebingen.mpg.de>
Reviewed-by: Alex Deucher alexdeucher@gmail.com Tested-by: Alex Deucher alexdeucher@gmail.com
diff --git a/src/radeon.h b/src/radeon.h index a6d20d7..1a746c7 100644 --- a/src/radeon.h +++ b/src/radeon.h @@ -931,6 +931,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..ed27dad 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,16 @@ 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 +1063,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 +1103,16 @@ 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 +1154,16 @@ 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 +1196,16 @@ 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 +1276,7 @@ radeon_dri2_screen_init(ScreenPtr pScreen) DRI2InfoRec dri2_info = { 0 }; #ifdef USE_DRI2_SCHEDULING const char *driverNames[1];
- uint64_t cap_value;
#endif
if (!info->useEXA) { @@ -1248,6 +1308,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 +1322,20 @@ 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 > 2) {
- if (drmGetCap(info->dri2.drm_fd, DRM_CAP_HIGH_CRTC, &cap_value)) {
- info->high_crtc_works = FALSE;
- xf86DrvMsg(pScrn->scrnIndex, X_WARNING, "You need a newer kernel
for VBLANKs on CRTC>1\n");
- } else {
- if (cap_value) {
- info->high_crtc_works = TRUE;
- } else {
- xf86DrvMsg(pScrn->scrnIndex, X_WARNING, "Your kernel does
not handle VBLANKs on CRTC>1\n");
- info->high_crtc_works = FALSE;
- }
- }
- }
if (pRADEONEnt->dri2_info_cnt == 0) { #if HAS_DIXREGISTERPRIVATEKEY if (!dixRegisterPrivateKey(DRI2ClientEventsPrivateKey, PRIVATE_CLIENT, sizeof(DRI2ClientEventsRec))) {
On Sat, 19 Mar 2011, Alex Deucher wrote:
On Fri, Mar 18, 2011 at 5:58 PM, Ilija Hadzic ihadzic@research.bell-labs.com wrote:
Hi Alex,
Below is a patch against the master branch of xf86-video-ati that adds support for waits on vblank events on CRTCs that are greater than 1 (and thus cannot be represented using current primary/secondary flags interface). The patch makes use of GET_CAP ioctl to check whether vblanks on crtc > 1 are supported by the kernel. If they are not falls back to legacy behavior. Otherwise, it uses correct crtc numbers when waiting on vblank and thus corrects the problem seen in certain multiscreen configurations.
The issue was discussed on the dri-devel list in these two threads
http://lists.freedesktop.org/archives/dri-devel/2011-March/009009.html http://lists.freedesktop.org/archives/dri-devel/2011-March/009025.html
regards,
Ilija
Reviewed-by: Mario Kleiner <mario.kleiner at tuebingen.mpg.de> Acked-by: Mario Kleiner <mario.kleiner at tuebingen.mpg.de>
Reviewed-by: Alex Deucher alexdeucher@gmail.com Tested-by: Alex Deucher alexdeucher@gmail.com
Signed-off-by: Ilija Hadzic ihadzic@research.bell-labs.com
diff --git a/src/radeon.h b/src/radeon.h index a6d20d7..1a746c7 100644 --- a/src/radeon.h +++ b/src/radeon.h @@ -931,6 +931,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..ed27dad 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,16 @@ 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 +1063,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 +1103,16 @@ 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 +1154,16 @@ 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 +1196,16 @@ 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 +1276,7 @@ radeon_dri2_screen_init(ScreenPtr pScreen) DRI2InfoRec dri2_info = { 0 }; #ifdef USE_DRI2_SCHEDULING const char *driverNames[1];
- uint64_t cap_value;
#endif
if (!info->useEXA) { @@ -1248,6 +1308,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 +1322,20 @@ 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 > 2) {
- if (drmGetCap(info->dri2.drm_fd, DRM_CAP_HIGH_CRTC, &cap_value)) {
- info->high_crtc_works = FALSE;
- xf86DrvMsg(pScrn->scrnIndex, X_WARNING, "You need a newer kernel
for VBLANKs on CRTC>1\n");
- } else {
- if (cap_value) {
- info->high_crtc_works = TRUE;
- } else {
- xf86DrvMsg(pScrn->scrnIndex, X_WARNING, "Your kernel does
not handle VBLANKs on CRTC>1\n");
- info->high_crtc_works = FALSE;
- }
- }
- }
if (pRADEONEnt->dri2_info_cnt == 0) { #if HAS_DIXREGISTERPRIVATEKEY if (!dixRegisterPrivateKey(DRI2ClientEventsPrivateKey, PRIVATE_CLIENT, sizeof(DRI2ClientEventsRec))) {
[ xf86-video-ati patches usually go to the xorg-driver-ati@lists.x.org list ]
On Fre, 2011-03-18 at 16:58 -0500, Ilija Hadzic wrote:
diff --git a/src/radeon_dri2.c b/src/radeon_dri2.c index 66df03c..ed27dad 100644 --- a/src/radeon_dri2.c +++ b/src/radeon_dri2.c @@ -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;
I'm still against this. At this point we know with certainty that DRM_VBLANK_SECONDARY won't do what we want. In particular, if CRTC 1 is disabled, the ioctl will time out, which I thought was a significant part of your motivation for these changes.
You seemed to agree with this in Pine.GSO.4.62.1103041912320.20023@umail .
- }
- vbl.request.type |= high_crtc;
Also, the high_crtc local variable seems rather pointless, and I agree with others that the common logic should be factored out into a helper function.
@@ -1248,6 +1308,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 +1322,20 @@ 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 > 2) {
- if (drmGetCap(info->dri2.drm_fd, DRM_CAP_HIGH_CRTC, &cap_value)) {
info->high_crtc_works = FALSE;
This assignment is redundant from above.
- } else {
if (cap_value) {info->high_crtc_works = TRUE;} else {xf86DrvMsg(pScrn->scrnIndex, X_WARNING, "Your kernel does not handle VBLANKs on CRTC>1\n");info->high_crtc_works = FALSE;
Is there any point in having two different warning messages? I think 'CRTC > 1' could use spaces.
On Tue, 22 Mar 2011, Michel [ISO-8859-1] D�nzer wrote:
[ xf86-video-ati patches usually go to the xorg-driver-ati@lists.x.org list ]
I was told it should go to Alex and CC dri-devel. Next time I'll include the other list.
I'm still against this. At this point we know with certainty that DRM_VBLANK_SECONDARY won't do what we want. In particular, if CRTC 1 is disabled, the ioctl will time out, which I thought was a significant part of your motivation for these changes.
You seemed to agree with this in Pine.GSO.4.62.1103041912320.20023@umail .
Not quite. What I said is that I want to achieve is the following behavior:
a) legacy anything (kernel or DDX), unchanged behavior from what we are seeing now b) new everything (kernel and DDX), vblanks use the right CRTC.
The above code achieves that. I explained the motivation in my previous posts and I won't repeat here.
- }
- vbl.request.type |= high_crtc;
Also, the high_crtc local variable seems rather pointless, and I agree with others that the common logic should be factored out into a helper function.
An alternative patch with repeated code factored out was offered to the list as a follow up on Jesse's comment on that. Alex decides which one to accept.
@@ -1248,6 +1308,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 +1322,20 @@ 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 > 2) {
- if (drmGetCap(info->dri2.drm_fd, DRM_CAP_HIGH_CRTC, &cap_value)) {
info->high_crtc_works = FALSE;This assignment is redundant from above.
Speaking from functionality perspective, yes it's redundant, but having it makes the code more readable and maintenable (i.e. you see exactly what the intended value of the flag should be if the condition is taken as opposed to having to trace it up. The assignment up, however, is necessary to make it safe if the code is taken out by the pre-processor.
- } else {
if (cap_value) {info->high_crtc_works = TRUE;} else {xf86DrvMsg(pScrn->scrnIndex, X_WARNING, "Your kernel does not handle VBLANKs on CRTC>1\n");info->high_crtc_works = FALSE;Is there any point in having two different warning messages? I think 'CRTC > 1' could use spaces.
There is a point: one warning tells you that the kernel is old and you have to upgrade. The other warning tells you that the kernel is new enough (it has the GET_CAP ioctl), but for some other reason refused to handle high-crtcs (which at this time doesn't exist, but it should not be the reason to "destroy" the information).
I bet the change on my desk in my office that if I had the blankspace, someone would have responded with an opposite suggestion ;-).
-- Ilija
On Die, 2011-03-22 at 09:03 -0500, Ilija Hadzic wrote:
On Tue, 22 Mar 2011, Michel [ISO-8859-1] Dnzer wrote:
I'm still against this. At this point we know with certainty that DRM_VBLANK_SECONDARY won't do what we want. In particular, if CRTC 1 is disabled, the ioctl will time out, which I thought was a significant part of your motivation for these changes.
You seemed to agree with this in Pine.GSO.4.62.1103041912320.20023@umail .
Not quite. What I said is that I want to achieve is the following behavior:
a) legacy anything (kernel or DDX), unchanged behavior from what we are seeing now b) new everything (kernel and DDX), vblanks use the right CRTC.
In the post I referenced above, you said:
[...] I'll add a hook to the DDX to check the version and not issue the ioctl at all if it is requested on a higher CRTC. I think that's better than falling back to the old style and issuing the system call on (potentially wrong) CRTC #1 because that can block the application (and I'd rather see it proceed without attempting vblank synchronization, then block).
Which made sense then and still does now, in contrast to trying to preserve ill-defined broken behaviour.
- } else {
if (cap_value) {info->high_crtc_works = TRUE;} else {xf86DrvMsg(pScrn->scrnIndex, X_WARNING, "Your kernel does not handle VBLANKs on CRTC>1\n");info->high_crtc_works = FALSE;Is there any point in having two different warning messages? I think 'CRTC > 1' could use spaces.
There is a point: one warning tells you that the kernel is old and you have to upgrade. The other warning tells you that the kernel is new enough (it has the GET_CAP ioctl), but for some other reason refused to handle high-crtcs (which at this time doesn't exist, but it should not be the reason to "destroy" the information).
Fair enough.
I bet the change on my desk in my office that if I had the blankspace, someone would have responded with an opposite suggestion ;-).
That's bold of you. I stand by my request.
On Tue, 22 Mar 2011, Michel [ISO-8859-1] D�nzer wrote:
In the post I referenced above, you said:
[...] I'll add a hook to the DDX to check the version and not issue the ioctl at all if it is requested on a higher CRTC. I think that's better than falling back to the old style and issuing the system call on (potentially wrong) CRTC #1 because that can block the application (and I'd rather see it proceed without attempting vblank synchronization, then block).
Which made sense then and still does now, in contrast to trying to preserve ill-defined broken behaviour.
... and then as I started to write code I changed my mind (am I forgiven ? ;-) ) because I realized that three things would happen:
a) just not issuing the ioctl will cause an application to "firehose" the kernel with rendering commands and potentially impact the performance of other processes.
b) both behaviors (not issuing the ioctl and thus causing application to keep coming back after glxSwap or just blocking the application on bad CRTC) are broken anyway so introducing a wider variety of broken behaviors was probably worse as far as user's experience is concerned.
c) the change the way I made it is safer wrt. introducing new bugs; you can't as easily fake out the sequence number and timestamp as you may think.
I explained all of the above in my followup posts and although I didn't want to repeat them now, I now find myself repeating it anyway ;-)
- } else {
if (cap_value) {info->high_crtc_works = TRUE;} else {xf86DrvMsg(pScrn->scrnIndex, X_WARNING, "Your kernel does not handle VBLANKs on CRTC>1\n");info->high_crtc_works = FALSE;Is there any point in having two different warning messages? I think 'CRTC > 1' could use spaces.
There is a point: one warning tells you that the kernel is old and you have to upgrade. The other warning tells you that the kernel is new enough (it has the GET_CAP ioctl), but for some other reason refused to handle high-crtcs (which at this time doesn't exist, but it should not be the reason to "destroy" the information).
Fair enough.
I bet the change on my desk in my office that if I had the blankspace, someone would have responded with an opposite suggestion ;-).
That's bold of you. I stand by my request.
Next time I touch this file, I'll "smuggle" the blankspace, but let's say that this is a rock bottom of the priority list (just as fixing grammar, style and spelling in any message would be ... and there is plenty).
-- Ilija
On Die, 2011-03-22 at 09:53 -0500, Ilija Hadzic wrote:
On Tue, 22 Mar 2011, Michel [ISO-8859-1] Dnzer wrote:
In the post I referenced above, you said:
[...] I'll add a hook to the DDX to check the version and not issue the ioctl at all if it is requested on a higher CRTC. I think that's better than falling back to the old style and issuing the system call on (potentially wrong) CRTC #1 because that can block the application (and I'd rather see it proceed without attempting vblank synchronization, then block).
Which made sense then and still does now, in contrast to trying to preserve ill-defined broken behaviour.
... and then as I started to write code I changed my mind (am I forgiven ? ;-) ) because I realized that three things would happen:
a) just not issuing the ioctl will cause an application to "firehose" the kernel with rendering commands and potentially impact the performance of other processes.
b) both behaviors (not issuing the ioctl and thus causing application to keep coming back after glxSwap or just blocking the application on bad CRTC) are broken anyway so introducing a wider variety of broken behaviors was probably worse as far as user's experience is concerned.
Not calling the ioctl doesn't imply returning immediately.
Your changes only fix the bug you found (the X radeon driver calls the ioctl when that doesn't make sense) when both the kernel and X driver are updated, but it would be possible to also fix it when only the X driver is updated.
I bet the change on my desk in my office that if I had the blankspace, someone would have responded with an opposite suggestion ;-).
That's bold of you. I stand by my request.
Next time I touch this file, I'll "smuggle" the blankspace, but let's say that this is a rock bottom of the priority list (just as fixing grammar, style and spelling in any message would be ... and there is plenty).
Then I suppose applying this patch is rock bottom of my priority list... But, as it seems you'd rather argue in your changes than adjust them to reviews, I can also just fix this part up after it goes in in the worst case.
On Tue, 22 Mar 2011, Michel [ISO-8859-1] D�nzer wrote:
Not calling the ioctl doesn't imply returning immediately.
Your changes only fix the bug you found (the X radeon driver calls the ioctl when that doesn't make sense) when both the kernel and X driver are updated, but it would be possible to also fix it when only the X driver is updated.
At the risk of being called ignorant, I'll admit that I don't know how to fix it when only DDX is updated; at least not without introducing too much of the new stuff and even then, it won't be done right because for a process to properly wait on vblank it must cross into the kernel because that's where the vblank interrupts from the hardware are coming in.
If it does not return immediately, then what is the process going to wait on if glxSwap is called and DDX realizes that it does not want to call the ioctl ? Does it just sleep ? For how long ? Do we need to fake out vblank sequence numbers and/or timestamps ?
If you have a good idea, I'll listen, I'll try to understand, and I can look into implementing it in the next iteration. However, that should not be the reason for delaying this fix. There are multi-screen applications (including mine) that need proper functionality on higher-numbered CRTCs.
Then I suppose applying this patch is rock bottom of my priority list... But, as it seems you'd rather argue in your changes than adjust them to reviews, I can also just fix this part up after it goes in in the worst case.
Deciding which patch to apply based on emotions does not serve good to anybody, so let's "reboot" ourselves on this particular item.
I am not arguing in my changes, I am just saying that formatting the log message (where both formats are perfectly legitimate and it's only a matter of taste) is something that can go in later and either I can submit the follow-up patch next time something else is in the file is changed or anyone else can change it to the taste (including you).... and if it's changed I definitely won't argue it back because it won't be worth it.
-- Ilija
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Ilija Hadzic -
Jesse Barnes -
Michel Dänzer