Flickering with page-flipping on Acer Iconia W500 (AMD C-50 APU)

[Mario Kleiner]

On 02/21/2012 09:07 PM, Alex Deucher wrote:
> On Wed, Feb 1, 2012 at 5:43 PM, Felix Kuehling<felix.kuehling at amd.com>  wrote:
>> Following up on my message from Jan 19, now with a lot more hard data and a
>> less intrusive modification. Still a prototype though. CC-ing DRI-devel and
>> Mario Kleiner for a larger audience.
>>
>> To recap, I was seeing consistent flickering with Mesa/KMS on Android on my
>> Iconia Tab W500 tablet with an AMD C-50 APU. I was also noticing occasional
>> flickering under Ubuntu on the same system when maximizing/restoring windows
>> and releasing windows after moving them.
>>
>> I believe that flickering is related to page flipping and the associated
>> notifications to user space. A small modification to the page flipping code
>> in the Radeon driver made the problem disappear on both Ubuntu and Android.
>> As I understand it, the page flip notification logic works as follows:
>>
>> Hardware issues vsync IRQ
>> IRQ handler calls radeon_crtc_handle_flip
>> radeon_crtc_handle_flip calls radeon_page_flip, which programs MMIO to flip
>> pages and returns status whether the flip has been completed
>> if flip has not been completed, radeon_crtc_handle_flip uses current scanout
>> position to predict whether flip will complete in the current frame or not
>> if flip is predicted to complete, signal user space, otherwise defer until
>> next vsync IRQ
>>
>> The condition in step 4 needed a slight modification on my hardware. If the
>> current scanout position is negative (inside vblank interval), the page flip
>> will not complete until the next vsync on my hardware.
>>
>> I'm attaching two patches. radeon_flip_diag.diff adds some debug output to
>> the kernel log that helped me understand the timing of VSync IRQs used for
>> handling page-flips relative to the screen refresh in progress. It also
>> measures the time it takes to program the page flip in MMIO in pixels
>> scanned out (typically about 300 pixels, so relatively insignificant
>> compared to the vertical refresh).
>>
>> On my system it turned out that the scanout position at the time
>> radeon_crtc_handle_flip was called was somewhere between 798-800 and -2-0
>> (the LVDS screen having 800 visible rows). I used an awk script (also
>> attached) to compute some statistics. With the original condition almost no
>> page flips were detected as deferred to the next vsync IRQ. With the
>> modified condition about 50% page flips were completed immediately according
>> to radeon_page_flip and of the remaining ones about 50% were correctly
>> predicted to complete based on the scanout position. In total about 25% were
>> deferred until the next vsync. These 25% must have been causing flickering
>> with the original condition.
>>
>> radeon_flip_fix.diff shows the minor modification to the condition used to
>> decide whether a page-flip has been completed or will complete in the
>> current screen refresh.
>>
>> My conclusion is that on this particular hardware the condition that
>> predicts page flip completion must be modified in order to avoid notifying
>> user space of completed page flips prematurely.
>
> The fix looks ok to me.  Mario any thoughts?
>
> Reviewed-by: Alex Deucher<alexdeucher at gmail.com>
>

Hi,

the fix looks ok to me for that device, but could we make it conditional 
on the AMD C-50 APU and similar pieces? It is the right thing to do for 
that gpu, but for regular desktop gpus it is too pessimistic if it 
defers the pageflip timestamping and completion event for an already 
completed flip:

1. Makes the timestamps 1 refresh too late, causing timing sensitive 
software like mine to detect false positives -- reporting skipped frames 
were there weren't any. Not as bad as missing a really skipped frame, 
but still not great.

2. Can reduce the framerate due to throttling the client, especially on 
systems that are already challenged wrt. to their irq timing.

Is the vblank period very short on these kind of devices? From Felix 
description is sounds as if it is only 2 scanlines?

thanks,
-mario