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

[Michel Dänzer]

[ xf86-video-ati patches usually go to the xorg-driver-ati at 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 at 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.


-- 
Earthling Michel Dänzer           |                http://www.vmware.com
Libre software enthusiast         |          Debian, X and DRI developer