Sorry about overseeing additional comments. It definitely wasn't intentional.
On Tue, 22 Mar 2011, Michel [ISO-8859-1] Däer wrote:
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.
HIGH_CRTC does not logically belong either flags nor types, but it's a third thing and hence the separate mask. If I stashed it under either, then this would really be an "abuse" (as Jesse nicely put it) of an existing ioctl. This way it is just adding a new bit section/mask to a filed that already has multiple of them.
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.
I can't have an opinion on that because I wasn't around (at least not in the graphics subsystem world) to see the historic development of this ioctl. However, I'd say that the motivation is rather speculative.
However, from pragmatic angle, at this point the change is already in (it was in drm-core-next, last time in checked) and it is probably not a good idea to shift things around and thus potentially create multiple incompatible combinations of user spaces and kernels (even if they are just test builds). Especially that the suggestion is based more on what *may* happen in the future evolution of this ioctl rather than some funamental problem. Furthermore, we have heard on the list that at the end of the day, the "evolution" of this ioctl will be the basis for (later) redesigning the new one and getting it better in many other aspects. If that's indeed so, that's even less of a motivation to split hair.
Unless I hear strong voice from others on the list to change the position of HIGH_CRTC mask, I am reluctant to touch it at this point.
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.
I agree, it's a "cosmetic" change, though, but it indeed improves the code readibility/maintainability, so I will submit a follow-up patch (again, when I get some spare time to attend to this, given my day-job engagements).
[...]@@ -753,6 +755,7 @@ struct drm_event_vblank { }; #define DRM_CAP_DUMB_BUFFER 0x1 +#define DRM_CAP_HIGH_CRTC 0x2Seems like a rather generic name, something like DRM_CAP_VBLANK_HIGH_CRTC might be better.
will be in the follow-up patch mentioned above.
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
