[PATCH] kernel/drm: vblank wait on crtc > 1

[Ilija Hadzic]

Sorry about overseeing additional comments. It definitely wasn't 
intentional.

On Tue, 22 Mar 2011, Michel [ISO-8859-1] Dänzer 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 0x2
>>>
>>> Seems like a rather generic name, something like
>>> DRM_CAP_VBLANK_HIGH_CRTC might be better.
>

will be in the follow-up patch mentioned above.