vblank problem (and proposed fix) on crtc > 1

[Michel Dänzer]

On Don, 2011-03-10 at 07:32 -0600, Ilija Hadzic wrote: 
> 
> On Thu, 10 Mar 2011, Michel [ISO-8859-1] Dnzer 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.

No, userspace shouldn't call the ioctl for a disabled CRTC during normal
operation, that would be a bug of its own. However, a CRTC could be
disabled during the probe but get enabled later, e.g. due to
hot-plugging a display.


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

The difference is that there should be no bogus error message in dmesg,
avoiding spurious bug reports.


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

I don't see the value in conserving fundamentally broken behaviour.

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

You could probably always return 0, or previous value + 1 or whatever,
that's no more wrong than the values from a different CRTC.


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

Then at the very least the claim that 'an error message will appear in
the kernel only once at start up time' is wrong, there will actually be
four of them on old kernels.


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