I would like to propose an extension of the interface between the libdrm and drm kernel module for VBLANK wait to address a problem that I am seeing on screens with more than two displays. Below, I describe the problem, propose a (backwards compatible) solution and provide a set of patches that implement it. I am looking for some feedback on whether this would be a reasonable and acceptable fix. Sorry for the length of this note.
Specific setup I use is Radeon HD5870 (Evergreen/Cypress) GPU in Zaphod mode with three displays (DVI-0, DVI-1 and DisplayPort-0), but the problem applies to any configuration with more than two displays. A sample xorg.conf is pasted at the end of this note for reference. Everything works fine if all three displays are connected to the monitor.
However, if I yank out DVI-1 connector, leaving DVI-0 and DisplayPort-0 plugged in and restart X without changing the xorg.conf (fairly legitimate action of an ordinary user) any application that needs VBLANK synchronization (e.g. glxgears) will get stuck when started in the desktop associated with DisplayPort-0. It will still progress on DVI-0 and DVI-1 is (of course) not visible so it doesn't matter.
Tracing the userland and the kernel, I have found out that the reason this happens is because in this configuration DisplayPort-0 gets assigned to crtc-2 (crtc-0 is on DVI-0 and crtc-1 is still on unconnected DVI-1). Now the problem is that DRM_IOCTL_WAIT_VBLANK only understands primary and secondary crtc and everything that is not crtc-0 is considered secondary. Then in the kernel, drm module maps the secondary flag to crtc 1, but that is a disconnected crtc on which VBLANK interrupts never come.
I am under impression that this limitation is a legacy from the days when GPUs had at most two connectors. However, analyzing the whole VBLANK mechanism in the kernel, it looks like it is ready supporting VBLANKs on higher crtc numbers (at least I can tell that with confidence for radeon driver, which is the one I am using as well as for the drm module itself; didn't check other GPUs). Also, it looks like at least when DRI2 is used, the userland fully preserves CRTC information all the way from GLX/mesa down to the call into drmWaitVBlank, which is the function in libdrm that issues the ioctl.
Specifically in case of DRI2, all calls into drmWaitVBlank are done through three functions: radeon_dri2_get_msc, radeon_dri2_schedule_wait_msc and radeon_dri2_schedule_swap in DDX (xf86-video-ati library). These functions have information about the exact CRTC in which the drawable is, but they "destroy" it by mapping it to primary/secondary flag in the vbl.request.type. In the kernel, the flag is mapped back to crtc number (0 or 1), so crtc > 1 is never used.
The fix/improvement I propose is to extend the request.type field in drmVBlank structure with additional 5 bits that I call high_crtc (there are lots of unused bits in that field). 5 bits covers for 32 CRTCs, which seems to be the hard limit imposed by various parts of the Xorg and DDX (e.g. possible_crtcs mask in the display descriptor, and the like). If high_crtc is zero, then DRM (kernel module) looks at the primary/secondary flag and maps them to crtc 0 and 1 (backwards compatibility with older DDX or DDX for other device that does not use the new high_crtc field). If it's not zero then it means that the higher CRTC number is specified by DDX (i.e. userland is a new DDX) and vblank is waited on the specified CRTC (this is used only for crtc > 1, crtc 0 and 1, still use the old style).
In case that DDX is ahead of the kernel and tries to use the high_crtc field, kernel will return -EINVAL (due to failed mask check), but the application will progress without waiting on VBLANK, which is still better than being stuck as it is now. On the other hand, if DDX that is ahead of the kernel uses CRTC 0 or 1, this won't cause the old kernel to complain because these two CRTCs would still use the old-style with primary/secondary flag.
So the solution is in my opinion as graceful towards as it can be towards old kernel and fully backwards-compatible with old DDX.
Things seem to test very well on my machines (I am currently working with GPUs with many displays, so I care about this support a lot), at least when it comes to using Radeon cards. I would like some feedback on whether there could be any issues that I am overseeing and also whether it would break anyone else's DDX. I had a private conversation with Alex Deucher and he seems to be fine as far as Radeon GPUs are concerned. What do other folks think ? I believe that eliminating the above-described limitation has to start at least with some GPU and others will follow if it works out well.
There is also a potential issue a few places in Mesa that call drmWaitVBlank directly from there (and they lose the CRTC information quite early in the call stack), but I don't think they are used at all in DRI2 case (so we can limit the fix to DRI2).
Anyway, I'll stop here; hopefully at least few folks made it this far in reading this ;-) and solicit some feedback. For deference, below is the xorg.conf that I mentioned above and three patches that implement the proposed fix. They are to be applied to the kernel, libdrm and xf86-video-ati, respectively.
-- Ilija
------------------- xorg.conf (for reference) --------------------- Section "Monitor" Identifier "Monitor 0" EndSection
Section "Monitor" Identifier "Monitor 1" EndSection
Section "Monitor" Identifier "Monitor 2" EndSection
Section "Device" Identifier "GPU 0" Driver "radeon" Screen 0 Option "AccelMethod" "EXA" Option "ZaphodHeads" "DVI-0" BusID "PCI:8:0:0" EndSection
Section "Device" Identifier "GPU 1" Driver "radeon" Screen 1 Option "AccelMethod" "EXA" Option "ZaphodHeads" "DVI-1" BusID "PCI:8:0:0" EndSection
Section "Device" Identifier "GPU 2" Driver "radeon" Screen 2 Option "AccelMethod" "EXA" Option "ZaphodHeads" "DisplayPort-0" BusID "PCI:8:0:0" EndSection
Section "Screen" Identifier "Screen 0" Device "GPU 0" Monitor "Monitor 0" EndSection
Section "Screen" Identifier "Screen 1" Device "GPU 1" Monitor "Monitor 1" EndSection
Section "Screen" Identifier "Screen 2" Device "GPU 2" Monitor "Monitor 2" EndSection
Section "ServerLayout" Identifier "Layout 0" Screen 0 "Screen 0" Screen 1 "Screen 1" Above "Screen 0" Screen 2 "Screen 2" RightOf "Screen 0" EndSection
------------------------- kernel patch ----------------------------------------
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 16d5155..3b0abae 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -668,7 +668,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data, { union drm_wait_vblank *vblwait = data; int ret = 0; - unsigned int flags, seq, crtc; + unsigned int flags, seq, crtc, high_crtc;
if ((!drm_dev_to_irq(dev)) || (!dev->irq_enabled)) return -EINVAL; @@ -677,16 +677,21 @@ int drm_wait_vblank(struct drm_device *dev, void *data, return -EINVAL;
if (vblwait->request.type & - ~(_DRM_VBLANK_TYPES_MASK | _DRM_VBLANK_FLAGS_MASK)) { + ~(_DRM_VBLANK_TYPES_MASK | _DRM_VBLANK_FLAGS_MASK | + _DRM_VBLANK_HIGH_CRTC_MASK)) { DRM_ERROR("Unsupported type value 0x%x, supported mask 0x%x\n", vblwait->request.type, - (_DRM_VBLANK_TYPES_MASK | _DRM_VBLANK_FLAGS_MASK)); + (_DRM_VBLANK_TYPES_MASK | _DRM_VBLANK_FLAGS_MASK | + _DRM_VBLANK_HIGH_CRTC_MASK)); return -EINVAL; }
flags = vblwait->request.type & _DRM_VBLANK_FLAGS_MASK; - crtc = flags & _DRM_VBLANK_SECONDARY ? 1 : 0; - + high_crtc = (vblwait->request.type & _DRM_VBLANK_HIGH_CRTC_MASK); + if (high_crtc) + crtc = high_crtc >> _DRM_VBLANK_HIGH_CRTC_SHIFT; + else + crtc = flags & _DRM_VBLANK_SECONDARY ? 1 : 0; if (crtc >= dev->num_crtcs) return -EINVAL;
diff --git a/include/drm/drm.h b/include/drm/drm.h index e5f7061..d950581 100644 --- a/include/drm/drm.h +++ b/include/drm/drm.h @@ -469,6 +469,8 @@ enum drm_vblank_seq_type { _DRM_VBLANK_SECONDARY = 0x20000000, /**< Secondary display controller */ _DRM_VBLANK_SIGNAL = 0x40000000 /**< Send signal instead of blocking, unsupported */ }; +#define _DRM_VBLANK_HIGH_CRTC_SHIFT 16 +#define _DRM_VBLANK_HIGH_CRTC_MASK 0x001F0000
#define _DRM_VBLANK_TYPES_MASK (_DRM_VBLANK_ABSOLUTE | _DRM_VBLANK_RELATIVE) #define _DRM_VBLANK_FLAGS_MASK (_DRM_VBLANK_EVENT | _DRM_VBLANK_SIGNAL | \
------------------------------- libdrm patch -----------------------------------
diff --git a/xf86drm.h b/xf86drm.h index 9b89f56..65a68bf 100644 --- a/xf86drm.h +++ b/xf86drm.h @@ -302,6 +302,8 @@ typedef enum { DRM_VBLANK_SECONDARY = 0x20000000, /**< Secondary display controller */ DRM_VBLANK_SIGNAL = 0x40000000 /* Send signal instead of blocking */ } drmVBlankSeqType; +#define DRM_VBLANK_HIGH_CRTC_SHIFT 16 +#define DRM_VBLANK_HIGH_CRTC_MASK 0x001F0000
typedef struct _drmVBlankReq { drmVBlankSeqType type;
------------------------------- xf86-video-ati (DDX) patch ---------------------
diff --git a/src/radeon_dri2.c b/src/radeon_dri2.c index 66df03c..5cbe544 100644 --- a/src/radeon_dri2.c +++ b/src/radeon_dri2.c @@ -783,6 +783,7 @@ static int radeon_dri2_get_msc(DrawablePtr draw, CARD64 *ust, CARD64 *msc) drmVBlank vbl; int ret; int crtc = radeon_dri2_drawable_crtc(draw); + int high_crtc = 0;
/* Drawable not displayed, make up a value */ if (crtc == -1) { @@ -791,8 +792,12 @@ 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) + high_crtc = (crtc << DRM_VBLANK_HIGH_CRTC_SHIFT) & + DRM_VBLANK_HIGH_CRTC_MASK; + vbl.request.type |= high_crtc; vbl.request.sequence = 0;
ret = drmWaitVBlank(info->dri2.drm_fd, &vbl); @@ -825,6 +830,7 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, DrawablePtr draw, drmVBlank vbl; int ret, crtc = radeon_dri2_drawable_crtc(draw); CARD64 current_msc; + int high_crtc = 0;
/* Truncate to match kernel interfaces; means occasional overflow * misses, but that's generally not a big deal */ @@ -855,8 +861,12 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, DrawablePtr draw,
/* Get current count */ vbl.request.type = DRM_VBLANK_RELATIVE; - if (crtc > 0) + if (crtc == 1) vbl.request.type |= DRM_VBLANK_SECONDARY; + else if (crtc > 1) + high_crtc = (crtc << DRM_VBLANK_HIGH_CRTC_SHIFT) & + DRM_VBLANK_HIGH_CRTC_MASK; + vbl.request.type |= high_crtc; vbl.request.sequence = 0; ret = drmWaitVBlank(info->dri2.drm_fd, &vbl); if (ret) { @@ -882,8 +892,12 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, DrawablePtr draw, if (current_msc >= target_msc) target_msc = current_msc; vbl.request.type = DRM_VBLANK_ABSOLUTE | DRM_VBLANK_EVENT; - if (crtc > 0) + if (crtc == 1) vbl.request.type |= DRM_VBLANK_SECONDARY; + else if (crtc > 1) + high_crtc = (crtc << DRM_VBLANK_HIGH_CRTC_SHIFT) & + DRM_VBLANK_HIGH_CRTC_MASK; + vbl.request.type |= high_crtc; vbl.request.sequence = target_msc; vbl.request.signal = (unsigned long)wait_info; ret = drmWaitVBlank(info->dri2.drm_fd, &vbl); @@ -903,8 +917,12 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, DrawablePtr draw, * so we queue an event that will satisfy the divisor/remainder equation. */ vbl.request.type = DRM_VBLANK_ABSOLUTE | DRM_VBLANK_EVENT; - if (crtc > 0) + if (crtc == 1) vbl.request.type |= DRM_VBLANK_SECONDARY; + else if (crtc > 1) + high_crtc = (crtc << DRM_VBLANK_HIGH_CRTC_SHIFT) & + DRM_VBLANK_HIGH_CRTC_MASK; + vbl.request.type |= high_crtc;
vbl.request.sequence = current_msc - (current_msc % divisor) + remainder; @@ -1029,6 +1047,7 @@ static int radeon_dri2_schedule_swap(ClientPtr client, DrawablePtr draw, CARD64 current_msc; BoxRec box; RegionRec region; + int high_crtc = 0;
/* Truncate to match kernel interfaces; means occasional overflow * misses, but that's generally not a big deal */ @@ -1068,8 +1087,12 @@ static int radeon_dri2_schedule_swap(ClientPtr client, DrawablePtr draw,
/* Get current count */ vbl.request.type = DRM_VBLANK_RELATIVE; - if (crtc > 0) + if (crtc == 1) vbl.request.type |= DRM_VBLANK_SECONDARY; + else if (crtc > 1) + high_crtc = (crtc << DRM_VBLANK_HIGH_CRTC_SHIFT) & + DRM_VBLANK_HIGH_CRTC_MASK; + vbl.request.type |= high_crtc; vbl.request.sequence = 0; ret = drmWaitVBlank(info->dri2.drm_fd, &vbl); if (ret) { @@ -1111,8 +1134,12 @@ static int radeon_dri2_schedule_swap(ClientPtr client, DrawablePtr draw, */ if (flip == 0) vbl.request.type |= DRM_VBLANK_NEXTONMISS; - if (crtc > 0) + if (crtc == 1) vbl.request.type |= DRM_VBLANK_SECONDARY; + else if (crtc > 1) + high_crtc = (crtc << DRM_VBLANK_HIGH_CRTC_SHIFT) & + DRM_VBLANK_HIGH_CRTC_MASK; + vbl.request.type |= high_crtc;
/* If target_msc already reached or passed, set it to * current_msc to ensure we return a reasonable value back @@ -1145,8 +1172,12 @@ static int radeon_dri2_schedule_swap(ClientPtr client, DrawablePtr draw, vbl.request.type = DRM_VBLANK_ABSOLUTE | DRM_VBLANK_EVENT; if (flip == 0) vbl.request.type |= DRM_VBLANK_NEXTONMISS; - if (crtc > 0) + if (crtc == 1) vbl.request.type |= DRM_VBLANK_SECONDARY; + else if (crtc > 1) + high_crtc = (crtc << DRM_VBLANK_HIGH_CRTC_SHIFT) & + DRM_VBLANK_HIGH_CRTC_MASK; + vbl.request.type |= high_crtc;
vbl.request.sequence = current_msc - (current_msc % divisor) + remainder;
On Thu, 3 Mar 2011 17:34:53 -0600 (CST) Ilija Hadzic ihadzic@research.bell-labs.com wrote:
The fix/improvement I propose is to extend the request.type field in drmVBlank structure with additional 5 bits that I call high_crtc (there are lots of unused bits in that field). 5 bits covers for 32 CRTCs, which seems to be the hard limit imposed by various parts of the Xorg and DDX (e.g. possible_crtcs mask in the display descriptor, and the like). If high_crtc is zero, then DRM (kernel module) looks at the primary/secondary flag and maps them to crtc 0 and 1 (backwards compatibility with older DDX or DDX for other device that does not use the new high_crtc field). If it's not zero then it means that the higher CRTC number is specified by DDX (i.e. userland is a new DDX) and vblank is waited on the specified CRTC (this is used only for crtc > 1, crtc 0 and 1, still use the old style).
Yeah, I think that should work, though another option would be to just add a new ioctl. That would make compat checking easy for new code; it could just call the new ioctl and if that returned -ENOTTY it could fall back to the old one and throw away the CRTC info or complain if the count was too high.
But you're right that when we re-wrote the code we fixed it to handle > 2 CRTCs, so it should be mostly ready for that (modulo testing, which it sounds like you're doing already).
Jesse
On Don, 2011-03-03 at 17:34 -0600, Ilija Hadzic wrote:
Now the problem is that DRM_IOCTL_WAIT_VBLANK only understands primary and secondary crtc and everything that is not crtc-0 is considered secondary. Then in the kernel, drm module maps the secondary flag to crtc 1, but that is a disconnected crtc on which VBLANK interrupts never come.
I am under impression that this limitation is a legacy from the days when GPUs had at most two connectors.
At most two CRTCs, but yeah.
My initial reaction to your proposal was similar to Jesse's, that there should be a new, cleaner ioctl for this instead. However, it looks like this can actually be done without making the existing ioctl too much messier than it is already. Given this and that it'll be hard to make a new ioctl perfect or at least cleanly extensible in the future, it may make sense to take this approach.
In case that DDX is ahead of the kernel and tries to use the high_crtc field, kernel will return -EINVAL (due to failed mask check), but the application will progress without waiting on VBLANK, which is still better than being stuck as it is now.
However, this will result in the kernel output getting spammed with 'Unsupported type value' error messages, won't it? One way to prevent this would be to bump include/drm/drm_core.h:DRM_IF_MINOR or drivers/gpu/drm/radeon/radeon_drv.c:KMS_DRIVER_MINOR and check for that before using the extended functionality in userspace. That would also prevent the hangs.
------------------------- kernel patch ----------------------------------------
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 16d5155..3b0abae 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -677,16 +677,21 @@ int drm_wait_vblank(struct drm_device *dev, void *data, return -EINVAL;
if (vblwait->request.type &
~(_DRM_VBLANK_TYPES_MASK | _DRM_VBLANK_FLAGS_MASK)) {
~(_DRM_VBLANK_TYPES_MASK | _DRM_VBLANK_FLAGS_MASK |_DRM_VBLANK_HIGH_CRTC_MASK)) {
(_DRM_VBLANK_TYPES_MASK | _DRM_VBLANK_FLAGS_MASK));
(_DRM_VBLANK_TYPES_MASK | _DRM_VBLANK_FLAGS_MASK |_DRM_VBLANK_HIGH_CRTC_MASK));
If _DRM_VBLANK_HIGH_CRTC_MASK were included in _DRM_VBLANK_FLAGS_MASK (or _DRM_VBLANK_TYPES_MASK, but that would make less sense), these changes shouldn't be necessary.
diff --git a/include/drm/drm.h b/include/drm/drm.h index e5f7061..d950581 100644 --- a/include/drm/drm.h +++ b/include/drm/drm.h @@ -469,6 +469,8 @@ enum drm_vblank_seq_type { _DRM_VBLANK_SECONDARY = 0x20000000, /**< Secondary display controller */ _DRM_VBLANK_SIGNAL = 0x40000000 /**< Send signal instead of blocking, unsupported */ }; +#define _DRM_VBLANK_HIGH_CRTC_SHIFT 16 +#define _DRM_VBLANK_HIGH_CRTC_MASK 0x001F0000
I'd suggest making _DRM_VBLANK_HIGH_CRTC_SHIFT either 21 (so _DRM_VBLANK_HIGH_CRTC_MASK is adjacent to _DRM_VBLANK_EVENT) or much lower, say 8 or even 4, as the flags are more likely to get extended than the types, if history is any indication.
Also,
#define _DRM_VBLANK_HIGH_CRTC_MASK (0x1F << _DRM_VBLANK_HIGH_CRTC_SHIFT)
would make it obvious how these values are related and decrease the likelihood of divergence during development of the patch.
------------------------------- xf86-video-ati (DDX) patch ---------------------
diff --git a/src/radeon_dri2.c b/src/radeon_dri2.c index 66df03c..5cbe544 100644 --- a/src/radeon_dri2.c +++ b/src/radeon_dri2.c @@ -1068,8 +1087,12 @@ static int radeon_dri2_schedule_swap(ClientPtr client, DrawablePtr draw,
/* Get current count */ vbl.request.type = DRM_VBLANK_RELATIVE;
- if (crtc > 0)
- if (crtc == 1) vbl.request.type |= DRM_VBLANK_SECONDARY;
- else if (crtc > 1)
- high_crtc = (crtc << DRM_VBLANK_HIGH_CRTC_SHIFT) &
DRM_VBLANK_HIGH_CRTC_MASK;- vbl.request.type |= high_crtc; vbl.request.sequence = 0; ret = drmWaitVBlank(info->dri2.drm_fd, &vbl); if (ret) {
Please try to preserve the indentation of surrounding code.
(this is a cumulative response to all comments that came in on this thread).
My opinion is that extending the existing ioctl is better than introducing the new one given that they will be doing the same thing. Also there are fewer kernel changes so it's safer (it opens fewer opportunities to screw up and it will be easier to review and vet the changes). We probably shouldn't start the new vs. not-new ioctl debate, since even those who advocate the former seem to agree that my proposal is a pragmatic one.
I agree that it is not a good idea to "spam" the old kernel with 'unsupported type value', so I'll add a hook to the DDX to check the version and not issue the ioctl at all if it is requested on a higher CRTC. I think that's better than falling back to the old style and issuing the system call on (potentially wrong) CRTC #1 because that can block the application (and I'd rather see it proceed without attempting vblank synchronization, then block).
I'll modify my patches to include the above stated check and retest when I get a little time and post them to this list. In the meantime, if anyone wants to venture into using the patches as I sent yesterday, please report the experience. On my end things seem to be working well, my specific problems are solved and it doesn't look like I broke anything, but more ppl. test, the better. That will take care of ATI DDX and general support in DRM; I presume that someone will follow up on other DDXs (I only deal with Radeon GPUs at the moment, so I am not familiar with other DDXs).
-- Ilija
On Fre, 2011-03-04 at 19:27 -0600, Ilija Hadzic wrote:
That will take care of ATI DDX and general support in DRM; I presume that someone will follow up on other DDXs (I only deal with Radeon GPUs at the moment, so I am not familiar with other DDXs).
Are there any other GPUs with more than 2 CRTCs (and KMS drivers) yet?
Below is an updated patch for ATI DDX (xf86-video-ati library) that reflects the discussion of this thread. The patch is *cumulative* (i.e., it includes the changes from a few days ago, so it should be applied to plain-vanilla DDX, not the one you may have patched with my patch from last week). I figure it's easier to review it that way, but if anyone wants an incremental patch, pls. let me know. The kernel patch and libdrm patch remain the same, so I am not repeating them here.
The new thing in this patch addresses Michel's concern about spamming the kernel with ''Unsupported type value' when DDX is ahead of the kernel. However, I don't think that this can be done by checking KMS_DRIVER_MINOR nor DRM_IF_MINOR. The reason is that the first version number pertains to the device driver module (radeon.ko), and there is no change in it addressed by my patches. So it's inappropriate to bump up this version number. On the other hand, as far as I could tell I don't see a viable mechanism to ask the kernel for DRM_IF_MINOR (DRM_IOCTL_SET_VERSION is the only call that remotely resembles what would be needed and it does more than just querying). Also, I believe that this version number pertains to the interface between the drm module and the device driver, not the userland/kernel interface (if I am wrong, I would appreciate enlightening from someone who knows better).
So what I did is to actually probe the kernel at screen init time. When the screen is initialized, the DDX checks if the GPU has more than two CRTCs and tries a dummy vblank wait on all crtcs > 1. If any of them fails, it falls back to the old method of using only primary/secondary flag. That way, an error messsage will appear in the kernel only once at start up time and never again, which I think is acceptable (I am sure that at least someone will disagree, but to me this looks like the best and the most reliable compromise). Anyway, it only happens when DDX is ahead of the kernel, which should be less.
So in summary the new behavior is that the new DDX when paired with a new kernel, VBLANKs will work on all CRTCs the way they are supposed to. If any of the two components is old, the behavior is identical to the one we see now (VBLANKs for crtc>1, taken from crtc==1, and if that one is disconnected, blocking of the application can happen).
-- Ilija
---------------- patch for xf86-video-ati -------------------------------
diff --git a/src/radeon.h b/src/radeon.h index 4c43717..ad80889 100644 --- a/src/radeon.h +++ b/src/radeon.h @@ -930,6 +930,9 @@ typedef struct {
RADEONFBLayout CurrentLayout;
+#ifdef RADEON_DRI2 + Bool high_crtc_works; +#endif #ifdef XF86DRI Bool directRenderingEnabled; Bool directRenderingInited; diff --git a/src/radeon_dri2.c b/src/radeon_dri2.c index 66df03c..7d77a6b 100644 --- a/src/radeon_dri2.c +++ b/src/radeon_dri2.c @@ -783,6 +783,7 @@ static int radeon_dri2_get_msc(DrawablePtr draw, CARD64 *ust, CARD64 *msc) drmVBlank vbl; int ret; int crtc = radeon_dri2_drawable_crtc(draw); + int high_crtc = 0;
/* Drawable not displayed, make up a value */ if (crtc == -1) { @@ -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; + } + vbl.request.type |= high_crtc; vbl.request.sequence = 0;
ret = drmWaitVBlank(info->dri2.drm_fd, &vbl); @@ -825,6 +834,7 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, DrawablePtr draw, drmVBlank vbl; int ret, crtc = radeon_dri2_drawable_crtc(draw); CARD64 current_msc; + int high_crtc = 0;
/* Truncate to match kernel interfaces; means occasional overflow * misses, but that's generally not a big deal */ @@ -855,8 +865,16 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, DrawablePtr draw,
/* Get current count */ 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; + } + vbl.request.type |= high_crtc; vbl.request.sequence = 0; ret = drmWaitVBlank(info->dri2.drm_fd, &vbl); if (ret) { @@ -882,8 +900,16 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, DrawablePtr draw, if (current_msc >= target_msc) target_msc = current_msc; vbl.request.type = DRM_VBLANK_ABSOLUTE | DRM_VBLANK_EVENT; - 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; + } + vbl.request.type |= high_crtc; vbl.request.sequence = target_msc; vbl.request.signal = (unsigned long)wait_info; ret = drmWaitVBlank(info->dri2.drm_fd, &vbl); @@ -903,8 +929,15 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, DrawablePtr draw, * so we queue an event that will satisfy the divisor/remainder equation. */ vbl.request.type = DRM_VBLANK_ABSOLUTE | DRM_VBLANK_EVENT; - 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; + } + vbl.request.type |= high_crtc;
vbl.request.sequence = current_msc - (current_msc % divisor) + remainder; @@ -1029,6 +1062,7 @@ static int radeon_dri2_schedule_swap(ClientPtr client, DrawablePtr draw, CARD64 current_msc; BoxRec box; RegionRec region; + int high_crtc = 0;
/* Truncate to match kernel interfaces; means occasional overflow * misses, but that's generally not a big deal */ @@ -1068,8 +1102,15 @@ static int radeon_dri2_schedule_swap(ClientPtr client, DrawablePtr draw,
/* Get current count */ 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; + } + vbl.request.type |= high_crtc; vbl.request.sequence = 0; ret = drmWaitVBlank(info->dri2.drm_fd, &vbl); if (ret) { @@ -1111,8 +1152,15 @@ static int radeon_dri2_schedule_swap(ClientPtr client, DrawablePtr draw, */ if (flip == 0) vbl.request.type |= DRM_VBLANK_NEXTONMISS; - 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; + } + vbl.request.type |= high_crtc;
/* If target_msc already reached or passed, set it to * current_msc to ensure we return a reasonable value back @@ -1145,8 +1193,15 @@ static int radeon_dri2_schedule_swap(ClientPtr client, DrawablePtr draw, vbl.request.type = DRM_VBLANK_ABSOLUTE | DRM_VBLANK_EVENT; if (flip == 0) vbl.request.type |= DRM_VBLANK_NEXTONMISS; - 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; + } + vbl.request.type |= high_crtc;
vbl.request.sequence = current_msc - (current_msc % divisor) + remainder; @@ -1217,6 +1272,8 @@ radeon_dri2_screen_init(ScreenPtr pScreen) DRI2InfoRec dri2_info = { 0 }; #ifdef USE_DRI2_SCHEDULING const char *driverNames[1]; + drmVBlank vbl; + int c; #endif
if (!info->useEXA) { @@ -1248,6 +1305,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 +1319,22 @@ 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) { + xf86DrvMsg(pScrn->scrnIndex, X_INFO, "GPU with %d CRTCs found, probing VBLANKs on CRTC>1\n", + info->drmmode.mode_res->count_crtcs); + info->high_crtc_works = TRUE; + for (c=2; c<info->drmmode.mode_res->count_crtcs; c++) { + vbl.request.type = DRM_VBLANK_RELATIVE; + vbl.request.type |= (c << DRM_VBLANK_HIGH_CRTC_SHIFT) & DRM_VBLANK_HIGH_CRTC_MASK; + 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; + } + } + if (info->high_crtc_works) xf86DrvMsg(pScrn->scrnIndex, X_INFO, "VBLANK request accepted on all CRTCs\n"); + } + if (pRADEONEnt->dri2_info_cnt == 0) { #if HAS_DIXREGISTERPRIVATEKEY if (!dixRegisterPrivateKey(DRI2ClientEventsPrivateKey, PRIVATE_CLIENT, sizeof(DRI2ClientEventsRec))) {
oops, just realized that I didn't include one final change to the patch I have just sent, so here is the real one; disregard the previous one (sorry wasting bandwidth).
---------------------------- xf86-video-ati.patch ----------------------
diff --git a/src/radeon.h b/src/radeon.h index 4c43717..ad80889 100644 --- a/src/radeon.h +++ b/src/radeon.h @@ -930,6 +930,9 @@ typedef struct {
RADEONFBLayout CurrentLayout;
+#ifdef RADEON_DRI2 + Bool high_crtc_works; +#endif #ifdef XF86DRI Bool directRenderingEnabled; Bool directRenderingInited; diff --git a/src/radeon_dri2.c b/src/radeon_dri2.c index 66df03c..c45abe6 100644 --- a/src/radeon_dri2.c +++ b/src/radeon_dri2.c @@ -783,6 +783,7 @@ static int radeon_dri2_get_msc(DrawablePtr draw, CARD64 *ust, CARD64 *msc) drmVBlank vbl; int ret; int crtc = radeon_dri2_drawable_crtc(draw); + int high_crtc = 0;
/* Drawable not displayed, make up a value */ if (crtc == -1) { @@ -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; + } + vbl.request.type |= high_crtc; vbl.request.sequence = 0;
ret = drmWaitVBlank(info->dri2.drm_fd, &vbl); @@ -825,6 +834,7 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, DrawablePtr draw, drmVBlank vbl; int ret, crtc = radeon_dri2_drawable_crtc(draw); CARD64 current_msc; + int high_crtc = 0;
/* Truncate to match kernel interfaces; means occasional overflow * misses, but that's generally not a big deal */ @@ -855,8 +865,16 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, DrawablePtr draw,
/* Get current count */ 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; + } + vbl.request.type |= high_crtc; vbl.request.sequence = 0; ret = drmWaitVBlank(info->dri2.drm_fd, &vbl); if (ret) { @@ -882,8 +900,16 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, DrawablePtr draw, if (current_msc >= target_msc) target_msc = current_msc; vbl.request.type = DRM_VBLANK_ABSOLUTE | DRM_VBLANK_EVENT; - 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; + } + vbl.request.type |= high_crtc; vbl.request.sequence = target_msc; vbl.request.signal = (unsigned long)wait_info; ret = drmWaitVBlank(info->dri2.drm_fd, &vbl); @@ -903,8 +929,15 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, DrawablePtr draw, * so we queue an event that will satisfy the divisor/remainder equation. */ vbl.request.type = DRM_VBLANK_ABSOLUTE | DRM_VBLANK_EVENT; - 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; + } + vbl.request.type |= high_crtc;
vbl.request.sequence = current_msc - (current_msc % divisor) + remainder; @@ -1029,6 +1062,7 @@ static int radeon_dri2_schedule_swap(ClientPtr client, DrawablePtr draw, CARD64 current_msc; BoxRec box; RegionRec region; + int high_crtc = 0;
/* Truncate to match kernel interfaces; means occasional overflow * misses, but that's generally not a big deal */ @@ -1068,8 +1102,15 @@ static int radeon_dri2_schedule_swap(ClientPtr client, DrawablePtr draw,
/* Get current count */ 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; + } + vbl.request.type |= high_crtc; vbl.request.sequence = 0; ret = drmWaitVBlank(info->dri2.drm_fd, &vbl); if (ret) { @@ -1111,8 +1152,15 @@ static int radeon_dri2_schedule_swap(ClientPtr client, DrawablePtr draw, */ if (flip == 0) vbl.request.type |= DRM_VBLANK_NEXTONMISS; - 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; + } + vbl.request.type |= high_crtc;
/* If target_msc already reached or passed, set it to * current_msc to ensure we return a reasonable value back @@ -1145,8 +1193,15 @@ static int radeon_dri2_schedule_swap(ClientPtr client, DrawablePtr draw, vbl.request.type = DRM_VBLANK_ABSOLUTE | DRM_VBLANK_EVENT; if (flip == 0) vbl.request.type |= DRM_VBLANK_NEXTONMISS; - 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; + } + vbl.request.type |= high_crtc;
vbl.request.sequence = current_msc - (current_msc % divisor) + remainder; @@ -1217,6 +1272,8 @@ radeon_dri2_screen_init(ScreenPtr pScreen) DRI2InfoRec dri2_info = { 0 }; #ifdef USE_DRI2_SCHEDULING const char *driverNames[1]; + drmVBlank vbl; + int c; #endif
if (!info->useEXA) { @@ -1248,6 +1305,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 +1319,22 @@ 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) { + xf86DrvMsg(pScrn->scrnIndex, X_INFO, "GPU with %d CRTCs found, probing VBLANKs on CRTC>1\n", + info->drmmode.mode_res->count_crtcs); + info->high_crtc_works = TRUE; + for (c=2; c<info->drmmode.mode_res->count_crtcs; c++) { + vbl.request.type = DRM_VBLANK_RELATIVE; + vbl.request.type |= (c << DRM_VBLANK_HIGH_CRTC_SHIFT) & DRM_VBLANK_HIGH_CRTC_MASK; + 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; + } + } + if (info->high_crtc_works) xf86DrvMsg(pScrn->scrnIndex, X_INFO, "VBLANK request accepted on all CRTCs\n"); + } + if (pRADEONEnt->dri2_info_cnt == 0) { #if HAS_DIXREGISTERPRIVATEKEY if (!dixRegisterPrivateKey(DRI2ClientEventsPrivateKey, PRIVATE_CLIENT, sizeof(DRI2ClientEventsRec))) {
On Mit, 2011-03-09 at 11:33 -0600, Ilija Hadzic wrote:
So what I did is to actually probe the kernel at screen init time. When the screen is initialized, the DDX checks if the GPU has more than two CRTCs and tries a dummy vblank wait on all crtcs > 1. If any of them fails, it falls back to the old method of using only primary/secondary flag.
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.
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.
Some detailed technical comments:
diff --git a/src/radeon_dri2.c b/src/radeon_dri2.c index 66df03c..c45abe6 100644 --- a/src/radeon_dri2.c +++ b/src/radeon_dri2.c @@ -1145,8 +1193,15 @@ static int radeon_dri2_schedule_swap(ClientPtr client, DrawablePtr draw, vbl.request.type = DRM_VBLANK_ABSOLUTE | DRM_VBLANK_EVENT; if (flip == 0) vbl.request.type |= DRM_VBLANK_NEXTONMISS;
- 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;
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.
@@ -1261,6 +1319,22 @@ 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) {
xf86DrvMsg(pScrn->scrnIndex, X_INFO, "GPU with %d CRTCs found, probing VBLANKs on CRTC>1\n",info->drmmode.mode_res->count_crtcs);info->high_crtc_works = TRUE;for (c=2; c<info->drmmode.mode_res->count_crtcs; c++) {vbl.request.type = DRM_VBLANK_RELATIVE;vbl.request.type |= (c << DRM_VBLANK_HIGH_CRTC_SHIFT) & DRM_VBLANK_HIGH_CRTC_MASK;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.
}}if (info->high_crtc_works) xf86DrvMsg(pScrn->scrnIndex, X_INFO, "VBLANK request accepted on all CRTCs\n");
The statement guarded by the if condition needs to go on a new line, and please don't add trailing whitespace. Also, again, please match the indentation of the surrounding code.
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.
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.
On Thu, 10 Mar 2011, Michel [ISO-8859-1] D�nzer wrote:
No, userspace shouldn't call the ioctl for a disabled CRTC during normal operation, that would be a bug of its own.
Well, that's exactly what DDX is doing today. Try this experiment: open a terminal window and run glxgears in it and observer the frame rate synced up to your monitor. Then in another terminal window type 'xset dpms force off' (or go to dpms off state any other way you want). The screen goes blank (disabled CRTC). Wait a while and then move your mouse to exit the disabled state. Notice the low frame rate reported by glxgears in one line. That's because glxSwapBuffers was blocked while screen was in power-down state.
Now I don't know if this behavior is intentional (I know your opinion based on the above), but it looks logical to me: if the screen is off and GLX attempts to wait on it, it gets what it deserves. If this is the real intent, then kernel returning error on disabled screen would break it (the application would just start to "rush" through rendering at max speed and eat up the I/O and CPU bandwidth for something that you can't see on the screen). Anyway, this is a topic for a separate thread.
I tend to agree that one day maybe the error is returned for some other reason (where your example may have been a bad one) so I will look into DRM_IOCTL_GET_CAP.
The difference is that there should be no bogus error message in dmesg, avoiding spurious bug reports.
That I agree and if GET_CAP ioctl is appropriate here, I'll change to that. There will still be an error message in the kernel at probing time, but hopefully a more meaningfull one. I guess the Xorg log message should then be something along the "you need a newer kernel" lines. Agreed ?
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.
There is even less value in introducing a wider variety of broken behaviors. Returning immediately, would cause the application to rush at full speed through buffer swaps and eat up the CPU and I/O bandwidth. It think that there are equal number of ppl. who would argue for that as those who would argue for the opposite.
You could probably always return 0, or previous value + 1 or whatever, that's no more wrong than the values from a different CRTC.
What I could do doesn't matter that much, because whatever I do would not result it fully functional system unless both the DDX and the kernel match in capabilities. So I go for the one that is less likely to introduce new bugs or unpredictable behavior.
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.
More precisely: num_crtcs - 2 times. Use word 'once' used in a liberal sense ;-) it doesn't happen repetatively during the normal operation. At least, if I can use DRM_IOCTL_GET_CAP (which I still have to check), the message will be more meaningful.
dri-devel@lists.freedesktop.org
-
Ilija Hadzic -
Jesse Barnes -
Michel Dänzer