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

[Ilija Hadzic]

On Tue, 22 Mar 2011, Michel [ISO-8859-1] Dänzer wrote:

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