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

[Michel Dänzer]

On 28.05.2015 18:03, Michel Dänzer wrote:
> 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).

I've fixed up the DRI3/Present code as well. This patch (with maybe some
cleanups to the commit log, in particular I'm not sure the third
paragraph should be there) is

Acked-and-Tested-by: Michel Dänzer <michel.daenzer at amd.com>


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