vblank problem (and proposed fix) on crtc > 1

[Ilija Hadzic]

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

>
> I'm afraid I don't really like this. Apart from the ugly bogus error
> message, the probes could fail for other reasons, e.g. at some point we
> might want to return an error when the ioctl is called for a CRTC which
> is currently disabled, to avoid the hang you were getting for CRTC 1.
>

Your example actually speaks in favor of probing. Let's say that the 
future kernel starts returning an error for disabled CRTC and you don't do 
any probing. DDX will start calling vblank waits on disabled CRTC (just 
like it's doing now) and spam the kernel with errors. With probing, error 
will happen only once at screen init time.

> Dave's drm-next tree actually has a new DRM_IOCTL_GET_CAP ioctl, which
> could be used for detecting this capability. It might even be possible
> to handle old kernels transparently in drmWaitVBlank(), but I'm not sure
> offhand if that would be better overall than doing it in the driver
> code.
>

This argument is self defeating. The purpose of the probing is to check 
for the old kernel. If I have DRM_IOCTL_GET_CAP, that means that I have a 
new kernel so probing will just work. If I have an old kernel then I don't 
have DRM_IOCTL_GET_CAP ioctl, so attemting to use it will result in an 
error from the kernel anyway. It makes no sense to me to cause an error 
in one ioctl so that you would prevent an error in other ioctl.

>> +       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;
>
> Waiting on a random other CRTC makes no sense. If you know you can't
> wait on the given CRTC, don't wait at all.
>

It's not random CRTC, but it's what today's DDX and DRM call "secondary" 
CRTC. I intentionally did this for two reasons. First, this is the 
behavior that is identical to what we see today, and I prefer to preserve 
the same behavior if either component is old (DDX or kernel), rather than 
have different behavior for different combinations of old/new. The second 
reason is pragmatic, if you want to not call wait_vblank at all and make 
sure you are safe to whatever the code above you is doing and not 
return an error you have to make up the sequence numbers and timestamps
(and probably keep track of them) and that's much more radical 
modification to the DDX. So I figured this would be conservative.

>> +           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;
>
> Should break out of the for loop at this point.
>

This was also intentional. I want to see in Xorg log file the full list of 
crtcs that rejected the vblank wait request. It comes handy for analyzing 
the problems and/or bugs.