Re: [PATCH] xf86-video-ati: vblank wait on crtc > 1

[ 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.