[Intel-gfx] [PATCH 2/3] drm/radeon: Switch to drm_vblank_on/off

[Michel Dänzer]

On 28.05.2015 17:38, Daniel Vetter wrote:
> On Thu, May 28, 2015 at 04:11:53PM +0900, Michel Dänzer wrote:
>> On 27.05.2015 18:41, Daniel Vetter wrote:
>>> On Wed, May 27, 2015 at 06:21:24PM +0900, Michel Dänzer wrote:
>>>> On 27.05.2015 18:04, Daniel Vetter wrote:
>>>>> These should be functionally equivalent to the older per/post modeset
>>>>> functions, except that they block out drm_vblank_get right away.
>>>>> There's only the clock adjusting code (outside of pageflips) in
>>>>> readone which uses drm_vblank_get. But that code doesn't synchronize
>>>>> against concurrent modesets and instead handles any such races by
>>>>> waiting for the right vblank to arrive with a short timetout.
>>>>>
>>>>> The longer-term plan here is to switch all kms drivers to
>>>>> drm_vblank_on/off so that common code like pending event cleanup can
>>>>> be done there, while drm_vblank_pre/post_modeset will be purely
>>>>> drm internal for the old UMS ioctl.
>>>>>
>>>>> Note that the kerneldoc for pre/post_modeset is wrong since as Michel
>>>>> Dänzer correctly pointed out it works if only using pre/post_modeset.
>>>>> The trouble that lead to this comment is the very old version of
>>>>> drm_vblank_off to clear out pending events when disabling a pipe,
>>>>> which did seem to wreak havoc with the trick used by pre/post_modeset.
>>>>> Michel also expressed dissatisfaction with intel folks pushing new
>>>>> interfaces with bogus justifications. I still maintain that having a
>>>>> consistent set of vblank behaviour across kms drivers, separate from
>>>>> any old UMS functions is a useful goal.
>>>>>
>>>>> Cc: Michel Dänzer <michel.daenzer at amd.com>
>>>>> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
>>>>
>>>> Can you describe at least one tangible benefit this change provides for
>>>> the radeon driver?
>>>>
>>>> Because I'm afraid that this might cause subtle breakage, and since we
>>>> don't have any rigorous tests for this like in intel-gpu-tools (yet?),
>>>> it might be painful to track it down.
>>>>
>>>> So, I'd like to have a good reason for taking the risk.
>>>
>>> right now at most a bit of code to clean out pending events on modeset
>>> disable, for somewhat consistent behaviour with other drivers. But in
>>> general it's fairly ill-defined what happens with vblank events.
>>
>> Yeah, while that's nice to have, I don't think it makes too much
>> difference in practice.
>>
>> Anyway, I'm giving this patch a spin, and it does indeed cause userspace
>> fallout, at least with DRI3/Present enabled, because the vblank and
>> pageflip ioctls now return -EINVAL while the CRTC is off. However, it
>> looks like fixing that up might not be too bad, so I'm cautiously
>> optimistic for this change. But I'd like some more time for testing and
>> fixing userspace.

[...]

> Otoh asking for a vblank event on a dead pipe smells like a userspace bug
> and could result in stuck compositors. Not sure what's best here really.

Agreed, and we're already careful not to do that with DRI2, just not yet
with DRI3/Present (which isn't in any xf86-video-ati release yet).


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer