On Thu, 10 Mar 2011, Michel [ISO-8859-1] Däer 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._______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
