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

[Christian König]

> 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.

> 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.

> 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.

> 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.

Thanks for the review, going to work on v2 now,
Christian.

Am 02.05.2014 09:25, schrieb Michel Dänzer:
> On 29.04.2014 23:29, Christian König wrote:
>> From: Christian König <christian.koenig at amd.com>
>>
>> Instead of trying to flip inside the vblank period when
>> the buffer is idle, offload blocking for idle to a kernel
>> thread and program the flip directly into the hardware.
>>
>> Signed-off-by: Christian König <christian.koenig at amd.com>
> [...]
>> +/**
>> + * radeon_flip_work_func - page flip framebuffer
>> + *
>> + * @work - kernel work item
>> + *
>> + * Wait for the buffer object to become idle and do the actual page flip
>> + */
>> +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, but sooner or later we'll probably want to support
> things like triple buffering, flips replacing previous ones still
> pending, and asynchronous flips (not synchronized to vertical blank).
> Those will probably require changes to the kernel/user interface, but it
> might be good to at least keep them in mind already for the infrastructure.
>
>
> Also, in patch 5, we could stop calling drm_vblank_get/put() when we're
> not using the vblank interrupt for flipping?
>
>
> The series looks good to me other than that.
>
>