[PATCH 4/5] drm/radeon: rework page flip handling

[Michel Dänzer]

On 02.05.2014 22:29, Christian König wrote:
> Am 02.05.2014 09:25, schrieb Michel Dänzer:
>> On 29.04.2014 23:29, Christian König wrote:
>>>
>>> +static void radeon_flip_work_func(struct work_struct *__work)
>>>   {
>> [...]
>>> +    if (radeon_crtc->flip_work) {
>>> +        DRM_DEBUG_DRIVER("flip queue: crtc already busy\n");
>>> +        spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
>>> +        goto pflip_cleanup1;
>>> +    }
>> I'm a little worried about this case. AFAICT this would drop the flip if
>> a previous one is still pending? I'm not sure current userspace can
>> actually hit this
> Yeah, that concerned me as well. The old code dropped the the new flip
> as well, so I'm pretty sure that the new handling is right and userspace
> won't hit that.
> 
> The only difference to the old code is that I've offloaded it to a
> separate thread and so can't return -EBUSY any more.

That's an important difference though: Userspace can react to the -EBUSY
appropriately, but if flips get dropped silently, bad things will
happen, such as the wrong buffer being scanned out.

Keep in mind that we don't control all userspace, e.g. I expect there
will be an increasing number of Wayland compositors using this
functionality.


>> but sooner or later we'll probably want to support
>> things like triple buffering,
> Triple buffering should still work like expected (at least if userspace
> makes sure to not submit more than one flip at a time).
> 
>> flips replacing previous ones still
>> pending,
> Flips replacing previous one is tricky to implement without causing a
> race condition, so I would rather like to avoid doing it for now.

It may be tricky, but it's important. Otherwise it's not possible to
achieve framerates higher than the refresh rate without tearing and with
low latency (using triple buffering).


>> and asynchronous flips (not synchronized to vertical blank).
> Actually that's one of the things I've implement in 3.15 by using the
> pflip interrupt. All you need to do is to set the appropriate bits in
> the hardware to sync the flip to VBLANK, HBLANK or not at all. For the
> pflip interrupt that doesn't matter it should just fire as soon as the
> new base address is used by the hardware.

Sounds great. :) Then we just need to expose that to userspace somehow.


>> Also, in patch 5, we could stop calling drm_vblank_get/put() when we're
>> not using the vblank interrupt for flipping?
> Mhm, good point. But I think the question is rather why did we needed
> that in the first place?
> 
> The VBLANK interrupt is turned on by radeon_irq_kms_pflip_irq_get
> anyway, so that call seems to be superfluous even on the old system.

Indeed, looks like it.


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