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

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, May 05, 2014 at 06:27:17PM +0900, Michel Dänzer wrote:
> 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.

Rules on i915 are that if the pageflip confirmation event hasn't been sent
out yet the driver returns -EBUSY. The locking for that is done using
irq-save spinlocks, so should be doable to check this synchronously before
latching some kinda of async worker.

I didnt read one line of radeon code to check this though ;-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch