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

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

[Felix Kuehling]

[-- Attachment #1.1: Type: text/plain, Size: 3049 bytes --]

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:

 1. Hardware issues vsync IRQ
 2. IRQ handler calls radeon_crtc_handle_flip
 3. radeon_crtc_handle_flip calls radeon_page_flip, which programs MMIO
    to flip pages and returns status whether the flip has been completed
 4. 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
 5. 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.

Regards,
  Felix

-- 
 _____    Felix Kuehling
 \ _  |   MTS Software Development Eng.
 /|_| |   SW-Linux Base Gfx | AMD
|__/ \|   T 905.882.2600 x8928


[-- Attachment #1.2: Type: text/html, Size: 3643 bytes --]

[-- Attachment #2: parse_flip.awk --]
[-- Type: application/x-awk, Size: 486 bytes --]

[-- Attachment #3: radeon_flip_diag.diff --]
[-- Type: text/x-patch, Size: 2866 bytes --]

diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
index 292f73f..b050e11 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -277,7 +277,7 @@ void radeon_crtc_handle_flip(struct radeon_device *rdev, int crtc_id)
 	struct timeval now;
 	unsigned long flags;
 	u32 update_pending;
-	int vpos, hpos;
+	int vpos, hpos, valid;
 
 	spin_lock_irqsave(&rdev->ddev->event_lock, flags);
 	work = radeon_crtc->unpin_work;
@@ -288,8 +288,17 @@ void radeon_crtc_handle_flip(struct radeon_device *rdev, int crtc_id)
 	}
 	/* New pageflip, or just completion of a previous one? */
 	if (!radeon_crtc->deferred_flip_completion) {
+		int vpos0, hpos0, pixels;
+		valid = radeon_get_crtc_scanoutpos(rdev->ddev, crtc_id,
+						   &vpos0, &hpos0);
 		/* do the flip (mmio) */
 		update_pending = radeon_page_flip(rdev, crtc_id, work->new_crtc_base);
+		valid = radeon_get_crtc_scanoutpos(rdev->ddev, crtc_id,
+						   &vpos, &hpos);
+		if (vpos < vpos0)
+			vpos0 -= rdev->mode_info.crtcs[crtc_id]->base.hwmode.crtc_vtotal;
+		pixels = hpos - hpos0 + (vpos - vpos0) * rdev->mode_info.crtcs[crtc_id]->base.hwmode.crtc_htotal;
+		DRM_ERROR ("radeon_page_flip took %d pixels\n", pixels);
 	} else {
 		/* This is just a completion of a flip queued in crtc
 		 * at last invocation. Make sure we go directly to
@@ -302,19 +311,24 @@ void radeon_crtc_handle_flip(struct radeon_device *rdev, int crtc_id)
 	/* Has the pageflip already completed in crtc, or is it certain
 	 * to complete in this vblank?
 	 */
-	if (update_pending &&
-	    (DRM_SCANOUTPOS_VALID & radeon_get_crtc_scanoutpos(rdev->ddev, crtc_id,
-							       &vpos, &hpos)) &&
-	    (vpos >=0) &&
-	    (vpos < (99 * rdev->mode_info.crtcs[crtc_id]->base.hwmode.crtc_vdisplay)/100)) {
-		/* crtc didn't flip in this target vblank interval,
-		 * but flip is pending in crtc. It will complete it
-		 * in next vblank interval, so complete the flip at
-		 * next vblank irq.
-		 */
-		radeon_crtc->deferred_flip_completion = 1;
-		spin_unlock_irqrestore(&rdev->ddev->event_lock, flags);
-		return;
+	if (update_pending) {
+		if ((valid & DRM_SCANOUTPOS_VALID) &&
+		    (vpos >=0) &&
+		    (vpos < (99 * rdev->mode_info.crtcs[crtc_id]->base.hwmode.crtc_vdisplay)/100)) {
+			/* crtc didn't flip in this target vblank interval,
+			 * but flip is pending in crtc. It will complete it
+			 * in next vblank interval, so complete the flip at
+			 * next vblank irq.
+			 */
+			radeon_crtc->deferred_flip_completion = 1;
+			spin_unlock_irqrestore(&rdev->ddev->event_lock, flags);
+			DRM_ERROR ("Pending deferred: %d\n", vpos);
+			return;
+		} else {
+			DRM_ERROR ("Pending not deferred: %d\n", vpos);
+		}
+	} else {
+		DRM_ERROR ("Not pending\n");
 	}
 
 	/* Pageflip (will be) certainly completed in this vblank. Clean up. */

[-- Attachment #4: radeon_flip_fix.diff --]
[-- Type: text/x-patch, Size: 621 bytes --]

diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
index b050e11..66bcfcc 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -313,7 +313,7 @@ void radeon_crtc_handle_flip(struct radeon_device *rdev, int crtc_id)
 	 */
 	if (update_pending) {
 		if ((valid & DRM_SCANOUTPOS_VALID) &&
-		    (vpos >=0) &&
+		    (vpos >=-2) &&
 		    (vpos < (99 * rdev->mode_info.crtcs[crtc_id]->base.hwmode.crtc_vdisplay)/100)) {
 			/* crtc didn't flip in this target vblank interval,
 			 * but flip is pending in crtc. It will complete it

[-- Attachment #5: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Alex Deucher]

On Wed, Feb 1, 2012 at 5:43 PM, Felix Kuehling <felix.kuehling@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@gmail.com>

>
> Regards,
>   Felix
>
> --
>  _____    Felix Kuehling
>  \ _  |   MTS Software Development Eng.
>  /|_| |   SW-Linux Base Gfx | AMD
> |__/ \|   T 905.882.2600 x8928
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

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

[Paul Menzel]

[-- Attachment #1.1: Type: text/plain, Size: 4035 bytes --]

Hi Felix!


Am Dienstag, den 21.02.2012, 15:07 -0500 schrieb Alex Deucher:
> On Wed, Feb 1, 2012 at 5:43 PM, Felix Kuehling <felix.kuehling@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@gmail.com>

A Signed-off-by line is required, is not it? Should it also got to
stable?

Additionally should the diagnose also be committed?

Felix:

        git config user.name "Felix Kühling"
        git config user.email "felix.kuehling@amd.com"
        git clone git://people.freedesktop.org/~airlied/linux
        git checkout -b flicker-fix drm-next # I hope that is the correct one.
        # apply your fix
        git commit -a -s
        # add your commit message with Alex’s Reviewed-by line
        git format-patch -1
        # use git send-email or paste it to your message editor to send the patch to the list


Thanks,

Paul

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: 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@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@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

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

[Felix Kuehling]

On 12-02-21 07:49 PM, Mario Kleiner wrote:
> On 02/21/2012 09:07 PM, Alex Deucher wrote:
[snip]
>> The fix looks ok to me.  Mario any thoughts?
>>
>> Reviewed-by: Alex Deucher<alexdeucher@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.

Agreed. I was going to perform some more experiments on other hardware
to determine what the right threshold is for different hardware
generations. I hope I'll get to that this week.

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

It looks like that.

Thanks for the feedback,
  Felix

>
> thanks,
> -mario
>

-- 
 _____    Felix Kuehling
 \ _  |   MTS Software Development Eng.
 /|_| |   SW-Linux Base Gfx | AMD
|__/ \|   T 905.882.2600 x8928

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

[Felix Kuehling]

[-- Attachment #1: Type: text/plain, Size: 2317 bytes --]

On 12-02-22 11:20 AM, Felix Kuehling wrote:
> On 12-02-21 07:49 PM, Mario Kleiner wrote:
>> On 02/21/2012 09:07 PM, Alex Deucher wrote:
> [snip]
>>> The fix looks ok to me.  Mario any thoughts?
>>>
>>> Reviewed-by: Alex Deucher<alexdeucher@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.
> Agreed. I was going to perform some more experiments on other hardware
> to determine what the right threshold is for different hardware
> generations. I hope I'll get to that this week.

I have a final version of my patch including an explanation of the
observations it's based on (attached). It's against current drm-next
from git://people.freedesktop.org/~airlied/linux.

>
>> 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?
> It looks like that.

Turns out that that's not correct. Smaller negative values of vpos never
showed up in my log output because I didn't print it in case
update_pending was 0. The actual vblank period is 8 scan lines on this
device. Still not much compared to the ~40 I was seeing with an external
monitor. Anything > -4 would result in flickering in my experiments, so
only 5 scan lines worth of time are available for submitting the page
flip in time for the next frame. If I miss that time window, the flip is
deferred by an extra frame. In practice that seems to occur in about 25%
of cases on this particular device.

Regards,
  Felix

> Thanks for the feedback,
>   Felix
>
>> thanks,
>> -mario
>>

-- 
 _____    Felix Kuehling
 \ _  |   MTS Software Development Eng.
 /|_| |   SW-Linux Base Gfx | AMD
|__/ \|   T 905.882.2600 x8928


[-- Attachment #2: 0001-Fix-deferred-page-flip-detection-logic-on-Avivo-base.patch --]
[-- Type: text/x-patch, Size: 2438 bytes --]

>From ce90d636215554042809f8cc0a27dab03ba3ba8a Mon Sep 17 00:00:00 2001
From: Felix Kuehling <Felix.Kuehling@amd.com>
Date: Thu, 23 Feb 2012 19:16:12 -0500
Subject: [PATCH] Fix deferred page-flip detection logic on Avivo-based ASICs

This fixes page-flip-related flickering observed on Iconia Tab W500.

The update_pending status returned by radeon_page_flip is very accurate on
Avivo-based ASICs when vpos is negative.

Experiments were conducted on several ASIC generations ranging from RS690
to Cayman where the page flip was artificially timed to occur at a specific
vpos. With negative vpos, overriding update_pending always lead to
flickering.

The same experiment on RV380 and RV410 showed that update_pending is not
accurate with negative vpos. In most cases update_pending == 1 is returned
although the flip would complete before the start of the next frame.
Therefore I left the behaviour unchanged for pre-AVIVO ASICs for
performance reasons, although this may result in flickering in rare cases.

This change also makes the logic a little easier to understand.

Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/radeon/radeon_display.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
index 7cb062d..1f98e5f 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -303,8 +303,17 @@ void radeon_crtc_handle_flip(struct radeon_device *rdev, int crtc_id)
 	if (update_pending &&
 	    (DRM_SCANOUTPOS_VALID & radeon_get_crtc_scanoutpos(rdev->ddev, crtc_id,
 							       &vpos, &hpos)) &&
-	    (vpos >=0) &&
-	    (vpos < (99 * rdev->mode_info.crtcs[crtc_id]->base.hwmode.crtc_vdisplay)/100)) {
+	    ((vpos >= (99 * rdev->mode_info.crtcs[crtc_id]->base.hwmode.crtc_vdisplay)/100) ||
+	     (vpos < 0 && !ASIC_IS_AVIVO(rdev)))) {
+		/* crtc didn't flip in this target vblank interval,
+		 * but flip is pending in crtc. Based on the current
+		 * scanout position we know that the current frame is
+		 * (nearly) complete and the flip will (likely)
+		 * complete before the start of the next frame.
+		 */
+		update_pending = 0;
+	}
+	if (update_pending) {
 		/* crtc didn't flip in this target vblank interval,
 		 * but flip is pending in crtc. It will complete it
 		 * in next vblank interval, so complete the flip at
-- 
1.7.5.4


[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Mario Kleiner]

On Feb 24, 2012, at 10:20 PM, Felix Kuehling wrote:

> On 12-02-22 11:20 AM, Felix Kuehling wrote:
>> On 12-02-21 07:49 PM, Mario Kleiner wrote:
>>> On 02/21/2012 09:07 PM, Alex Deucher wrote:
>> [snip]
>>>> The fix looks ok to me.  Mario any thoughts?
>>>>
>>>> Reviewed-by: Alex Deucher<alexdeucher@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.
>> Agreed. I was going to perform some more experiments on other  
>> hardware
>> to determine what the right threshold is for different hardware
>> generations. I hope I'll get to that this week.
>
> I have a final version of my patch including an explanation of the
> observations it's based on (attached). It's against current drm-next
> from git://people.freedesktop.org/~airlied/linux.
>

The idea of the current logic was that it happened that the  
update_pending bit isn't read back as zero at the end of the  
page_flip programming function, immediately after clearing the  
graphics hardware update_lock, e.g., maybe because there is some  
delay between clearing that register and the double buffering taking  
place. But according to the specs, if update_pending is high, ie.,  
the hw has "accepted" the request for a page flip, it will complete  
as long as that request still happened within the vblank. So on a  
return from the page_flip function with update_pending == 1 we check  
if we are still inside the vblank area, ie., if the hw will certainly  
complete the double buffering within the current vblank, because the  
request was made in time.

I did all my original testing of these bits with avivo hw (rv530,  
r600) and with one radeon hd 5850, so i'm a bit surprised if the  
avivo path would unconditionally need this adjustment. Could it be  
that the observed accuracy of update_pending depends on the rest of  
the hw, e.g., bus or processor speed, or bus activity? My test  
machines were a MacBookPro with Core2Duo 2.3 Ghz for rv530 and a  
rather old AMD Athlon-64 PC for r600.

I'm worried that your observed reliability of update_pending on >=  
AVIVO asics could be an artifact of the specific computer hw you used  
and that this doesn't generalize on older or different hw. If it  
doesn't generalize then the patch could defer a lot of perfectly good  
pageflips by 1 frame, screwing the timestamps and reducing framerate.

Here is what the rv630 register programming guide says about the  
double-buffering of the surface base address registers:

D1GRPH_SURFACE_UPDATE_PENDING: "the double buffering occurs in  
vertical retrace when D1GRPH_SURFACE_UPDATE_PENDING = 1 and
D1GRPH_UPDATE_LOCK = 0 and V_UPDATE = 1."

D1CRTC_V_UPDATE: "Current vertical position ... 1 =  within the  
V_UPDATE region (between end of vertical active display and start_line)"

For us that would mean the threshold for deferred flip completion  
would need to be whatever that mentioned "start_line" is, and for the  
tested avivo hw, start_line used to be == start of active scanout, so  
the threshold of zero made sense.

Alex: I don't know where start_line is stored. Could it be that this  
value changed due to hw changes or changes in the kms driver or  
atombios? Or would it be possible to read that value back from some  
register and use it as threshold?

If you look at radeon_get_crtc_scanoutpos() you can also see that the  
returned vpos is corrected for some offsets. Could it be that  
something changed there for the DCE4.1 or DCE5 display engine or  
whatever the C-50 APU uses? That could also explain why suddenly such  
a weird threshold as vpos > -4 is needed on your tablet, because the  
returned vpos is offset wrongly by a few scanlines.

-mario


>>
>>> 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?
>> It looks like that.
>
> Turns out that that's not correct. Smaller negative values of vpos  
> never
> showed up in my log output because I didn't print it in case
> update_pending was 0. The actual vblank period is 8 scan lines on this
> device. Still not much compared to the ~40 I was seeing with an  
> external
> monitor. Anything > -4 would result in flickering in my  
> experiments, so
> only 5 scan lines worth of time are available for submitting the page
> flip in time for the next frame. If I miss that time window, the  
> flip is
> deferred by an extra frame. In practice that seems to occur in  
> about 25%
> of cases on this particular device.
>
> Regards,
>   Felix
>
>> Thanks for the feedback,
>>   Felix
>>
>>> thanks,
>>> -mario
>>>
>
> -- 
>  _____    Felix Kuehling
>  \ _  |   MTS Software Development Eng.
>  /|_| |   SW-Linux Base Gfx | AMD
> |__/ \|   T 905.882.2600 x8928
>
> <0001-Fix-deferred-page-flip-detection-logic-on-Avivo-base.patch>

*********************************************************************
Mario Kleiner
Max Planck Institute for Biological Cybernetics
Spemannstr. 38
72076 Tuebingen
Germany

e-mail: mario.kleiner@tuebingen.mpg.de
office: +49 (0)7071/601-1623
fax:    +49 (0)7071/601-616
www:    http://www.kyb.tuebingen.mpg.de/~kleinerm
*********************************************************************
"For a successful technology, reality must take precedence
over public relations, for Nature cannot be fooled."
(Richard Feynman)

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

[Alex Deucher]

On Fri, Feb 24, 2012 at 11:38 PM, Mario Kleiner
<mario.kleiner@tuebingen.mpg.de> wrote:
> On Feb 24, 2012, at 10:20 PM, Felix Kuehling wrote:
>
>> On 12-02-22 11:20 AM, Felix Kuehling wrote:
>>>
>>> On 12-02-21 07:49 PM, Mario Kleiner wrote:
>>>>
>>>> On 02/21/2012 09:07 PM, Alex Deucher wrote:
>>>
>>> [snip]
>>>>>
>>>>> The fix looks ok to me.  Mario any thoughts?
>>>>>
>>>>> Reviewed-by: Alex Deucher<alexdeucher@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.
>>>
>>> Agreed. I was going to perform some more experiments on other hardware
>>> to determine what the right threshold is for different hardware
>>> generations. I hope I'll get to that this week.
>>
>>
>> I have a final version of my patch including an explanation of the
>> observations it's based on (attached). It's against current drm-next
>> from git://people.freedesktop.org/~airlied/linux.
>>
>
> The idea of the current logic was that it happened that the update_pending
> bit isn't read back as zero at the end of the page_flip programming
> function, immediately after clearing the graphics hardware update_lock,
> e.g., maybe because there is some delay between clearing that register and
> the double buffering taking place. But according to the specs, if
> update_pending is high, ie., the hw has "accepted" the request for a page
> flip, it will complete as long as that request still happened within the
> vblank. So on a return from the page_flip function with update_pending == 1
> we check if we are still inside the vblank area, ie., if the hw will
> certainly complete the double buffering within the current vblank, because
> the request was made in time.
>
> I did all my original testing of these bits with avivo hw (rv530, r600) and
> with one radeon hd 5850, so i'm a bit surprised if the avivo path would
> unconditionally need this adjustment. Could it be that the observed accuracy
> of update_pending depends on the rest of the hw, e.g., bus or processor
> speed, or bus activity? My test machines were a MacBookPro with Core2Duo 2.3
> Ghz for rv530 and a rather old AMD Athlon-64 PC for r600.
>
> I'm worried that your observed reliability of update_pending on >= AVIVO
> asics could be an artifact of the specific computer hw you used and that
> this doesn't generalize on older or different hw. If it doesn't generalize
> then the patch could defer a lot of perfectly good pageflips by 1 frame,
> screwing the timestamps and reducing framerate.
>
> Here is what the rv630 register programming guide says about the
> double-buffering of the surface base address registers:
>
> D1GRPH_SURFACE_UPDATE_PENDING: "the double buffering occurs in vertical
> retrace when D1GRPH_SURFACE_UPDATE_PENDING = 1 and
> D1GRPH_UPDATE_LOCK = 0 and V_UPDATE = 1."
>
> D1CRTC_V_UPDATE: "Current vertical position ... 1 =  within the V_UPDATE
> region (between end of vertical active display and start_line)"
>
> For us that would mean the threshold for deferred flip completion would need
> to be whatever that mentioned "start_line" is, and for the tested avivo hw,
> start_line used to be == start of active scanout, so the threshold of zero
> made sense.
>
> Alex: I don't know where start_line is stored. Could it be that this value
> changed due to hw changes or changes in the kms driver or atombios? Or would
> it be possible to read that value back from some register and use it as
> threshold?
>
> If you look at radeon_get_crtc_scanoutpos() you can also see that the
> returned vpos is corrected for some offsets. Could it be that something
> changed there for the DCE4.1 or DCE5 display engine or whatever the C-50 APU
> uses? That could also explain why suddenly such a weird threshold as vpos >
> -4 is needed on your tablet, because the returned vpos is offset wrongly by
> a few scanlines.
>

Looks like there was a change for DCE4.1/5.0.

DCE1/2/3:
CRTC:D1CRTC_START_LINE_CONTROL  ·  [R/W]  ·  32 bits  ·  Access:
8/16/32  ·  MMReg:0x60d8
DESCRIPTION: move start_line signal earlier by 1 line in CRTC1
Field Name   Bits 	     Default 	    Description
D1CRTC_PROGRESSIVE_START_LINE_EARLY 	    0	0x0	move start_line signal
by 1 line eariler in progressive mode

D1CRTC_INTERLACE_START_LINE_EARLY 	    8 	0x1 	move start_line signal
by 1 line earlier in interlaced timing mode


DCE4.0:
CRTC:CRTC_START_LINE_CONTROL  ·  [R/W]  ·  32 bits  ·  Access: 8/16/32
 ·  [INST0] GpuF0MMReg:0x6ecc, [INST1] GpuF0MMReg:0x7acc, [INST2]
GpuF0MMReg:0x106cc, [INST3] GpuF0MMReg:0x112cc, [INST4]
GpuF0MMReg:0x11ecc, [INST5] GpuF0MMReg:0x12acc
DESCRIPTION: move start_line signal earlier by 1 line in CRTC
Field Name   Bits 	     Default 	    Description
CRTC_PROGRESSIVE_START_LINE_EARLY 	    0	0x0	move start_line signal by
1 line eariler in progressive mode

CRTC_INTERLACE_START_LINE_EARLY 	    8 	0x1 	move start_line signal by
1 line earlier in interlaced timing mode


DCE4.1/DCE5:
CRTC:CRTC_START_LINE_CONTROL  ·  [R/W]  ·  32 bits  ·  Access: 8/16/32
 ·  [INST0] GpuF0MMReg:0x6ecc, [INST1] GpuF0MMReg:0x7acc, [INST2]
GpuF0MMReg:0x106cc, [INST3] GpuF0MMReg:0x112cc, [INST4]
GpuF0MMReg:0x11ecc, [INST5] GpuF0MMReg:0x12acc
DESCRIPTION: move start_line signal earlier by 1 line in CRTC
Field Name   Bits 	     Default 	    Description
CRTC_PROGRESSIVE_START_LINE_EARLY 	    0	0x0	move start_line signal by
1 line eariler in progressive mode

CRTC_INTERLACE_START_LINE_EARLY 	    8 	0x1 	move start_line signal by
1 line earlier in interlaced timing mode

CRTC_ADVANCED_START_LINE_POSITION 	    19:16 	0x4  Advanced Start Line
position respect to not VBLANK. Default of 4 means the Advanced Start
Line is 4 lines before the first non VBLANK line

Also bit 4 (CRTC_V_START_LINE) of CRTC_STATUS will tell you when
whether the current line is outside (0) or inside (1) the start line
region.


Alex

> -mario
>
>
>>>
>>>> 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?
>>>
>>> It looks like that.
>>
>>
>> Turns out that that's not correct. Smaller negative values of vpos never
>> showed up in my log output because I didn't print it in case
>> update_pending was 0. The actual vblank period is 8 scan lines on this
>> device. Still not much compared to the ~40 I was seeing with an external
>> monitor. Anything > -4 would result in flickering in my experiments, so
>> only 5 scan lines worth of time are available for submitting the page
>> flip in time for the next frame. If I miss that time window, the flip is
>> deferred by an extra frame. In practice that seems to occur in about 25%
>> of cases on this particular device.
>>
>> Regards,
>>  Felix
>>
>>> Thanks for the feedback,
>>>  Felix
>>>
>>>> thanks,
>>>> -mario
>>>>
>>
>> --
>>  _____    Felix Kuehling
>>  \ _  |   MTS Software Development Eng.
>>  /|_| |   SW-Linux Base Gfx | AMD
>> |__/ \|   T 905.882.2600 x8928
>>
>> <0001-Fix-deferred-page-flip-detection-logic-on-Avivo-base.patch>
>
>
> *********************************************************************
> Mario Kleiner
> Max Planck Institute for Biological Cybernetics
> Spemannstr. 38
> 72076 Tuebingen
> Germany
>
> e-mail: mario.kleiner@tuebingen.mpg.de
> office: +49 (0)7071/601-1623
> fax:    +49 (0)7071/601-616
> www:    http://www.kyb.tuebingen.mpg.de/~kleinerm
> *********************************************************************
> "For a successful technology, reality must take precedence
> over public relations, for Nature cannot be fooled."
> (Richard Feynman)
>

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

[Felix Kuehling]

On 12-02-24 11:38 PM, Mario Kleiner wrote:
> On Feb 24, 2012, at 10:20 PM, Felix Kuehling wrote:
>
>> On 12-02-22 11:20 AM, Felix Kuehling wrote:
>>> On 12-02-21 07:49 PM, Mario Kleiner wrote:
>>>> On 02/21/2012 09:07 PM, Alex Deucher wrote:
>>> [snip]
>>>>> The fix looks ok to me.  Mario any thoughts?
>>>>>
>>>>> Reviewed-by: Alex Deucher<alexdeucher@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.
>>> Agreed. I was going to perform some more experiments on other hardware
>>> to determine what the right threshold is for different hardware
>>> generations. I hope I'll get to that this week.
>>
>> I have a final version of my patch including an explanation of the
>> observations it's based on (attached). It's against current drm-next
>> from git://people.freedesktop.org/~airlied/linux.
>>
>
> The idea of the current logic was that it happened that the
> update_pending bit isn't read back as zero at the end of the page_flip
> programming function, immediately after clearing the graphics hardware
> update_lock, e.g., maybe because there is some delay between clearing
> that register and the double buffering taking place. But according to
> the specs, if update_pending is high, ie., the hw has "accepted" the
> request for a page flip, it will complete as long as that request
> still happened within the vblank. So on a return from the page_flip
> function with update_pending == 1 we check if we are still inside the
> vblank area, ie., if the hw will certainly complete the double
> buffering within the current vblank, because the request was made in
> time.
>
> I did all my original testing of these bits with avivo hw (rv530,
> r600) and with one radeon hd 5850, so i'm a bit surprised if the avivo
> path would unconditionally need this adjustment. Could it be that the
> observed accuracy of update_pending depends on the rest of the hw,
> e.g., bus or processor speed, or bus activity? My test machines were a
> MacBookPro with Core2Duo 2.3 Ghz for rv530 and a rather old AMD
> Athlon-64 PC for r600.

I used a few different systems:

Brazos reference board for Brazos (E-350)
Iconia Tab W500 (C-50)
PhenomII X4 for most of the discrete cards
Athlon64 X2 for RS690

The results I got were consistent across all those systems. The only
differences I saw between different generations of Avivo-based hardware
were about the exact timing of the threshold when I would start seeing
flickering. On R5xx and R6xx it would start flickering at -3. On newer
hardware at -4 (I didn't test on R7xx).

>
> I'm worried that your observed reliability of update_pending on >=
> AVIVO asics could be an artifact of the specific computer hw you used
> and that this doesn't generalize on older or different hw.

Given the number of different ASICs and systems, I think that's
unlikely. It's more likely that this depends on the exact mode timings.
I was running most of my experiments with a 19" DFP 1280x1024 at 60Hz,
connected by DVI if available, or VGA on some of the IGPs.

It's possible that different mode timings would result in a slightly
different threshold.

> If it doesn't generalize then the patch could defer a lot of perfectly
> good pageflips by 1 frame, screwing the timestamps and reducing
> framerate.

I understand your concern. But given my observations, the current
implementation potentially produces much more annoying artefacts.

>
> Here is what the rv630 register programming guide says about the
> double-buffering of the surface base address registers:
>
> D1GRPH_SURFACE_UPDATE_PENDING: "the double buffering occurs in
> vertical retrace when D1GRPH_SURFACE_UPDATE_PENDING = 1 and
> D1GRPH_UPDATE_LOCK = 0 and V_UPDATE = 1."

As far as I can tell, D1GRPH_SURFACE_UPDATE_PENDING will only be 1, if
the page flip is initiated while outside the vertical retrace. The the
actual page flip will occur when V_UPDATE changes to 1, that is, when
the next vertical retrace occurs. So if we read
D1GRPH_SURFACE_UPDATE_PENDING after initiating a page flip, it implies
that we're currently not in a vertical retrace.

>
> D1CRTC_V_UPDATE: "Current vertical position ... 1 =  within the
> V_UPDATE region (between end of vertical active display and start_line)"
>
> For us that would mean the threshold for deferred flip completion
> would need to be whatever that mentioned "start_line" is, and for the
> tested avivo hw, start_line used to be == start of active scanout, so
> the threshold of zero made sense.

In my experiments the start_line seemed to be between -4 and -2. On no
ASIC did I observe a start_line == 0.

>
> Alex: I don't know where start_line is stored. Could it be that this
> value changed due to hw changes or changes in the kms driver or
> atombios? Or would it be possible to read that value back from some
> register and use it as threshold?
>
> If you look at radeon_get_crtc_scanoutpos() you can also see that the
> returned vpos is corrected for some offsets. Could it be that
> something changed there for the DCE4.1 or DCE5 display engine or
> whatever the C-50 APU uses? That could also explain why suddenly such
> a weird threshold as vpos > -4 is needed on your tablet, because the
> returned vpos is offset wrongly by a few scanlines.

I believe the reason I observe flickering on my tablet is due to the
short vsync period. Otherwise there is nothing special about my tablet.
I was able to induce the same flickering by artificially delaying the
page flip on other Avivo hardware. My proposed fix resolves this
potential problem on all Avivo hardware. I am very confident that it
does that without deferring page flip notifications to user space
unnecessarily.

Regards,
  Felix

>
> -mario
>
>
>>>
>>>> 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?
>>> It looks like that.
>>
>> Turns out that that's not correct. Smaller negative values of vpos never
>> showed up in my log output because I didn't print it in case
>> update_pending was 0. The actual vblank period is 8 scan lines on this
>> device. Still not much compared to the ~40 I was seeing with an external
>> monitor. Anything > -4 would result in flickering in my experiments, so
>> only 5 scan lines worth of time are available for submitting the page
>> flip in time for the next frame. If I miss that time window, the flip is
>> deferred by an extra frame. In practice that seems to occur in about 25%
>> of cases on this particular device.
>>
>> Regards,
>>   Felix
>>
>>> Thanks for the feedback,
>>>   Felix
>>>
>>>> thanks,
>>>> -mario
>>>>
>>
>> -- 
>>  _____    Felix Kuehling
>>  \ _  |   MTS Software Development Eng.
>>  /|_| |   SW-Linux Base Gfx | AMD
>> |__/ \|   T 905.882.2600 x8928
>>
>> <0001-Fix-deferred-page-flip-detection-logic-on-Avivo-base.patch>
>
> *********************************************************************
> Mario Kleiner
> Max Planck Institute for Biological Cybernetics
> Spemannstr. 38
> 72076 Tuebingen
> Germany
>
> e-mail: mario.kleiner@tuebingen.mpg.de
> office: +49 (0)7071/601-1623
> fax:    +49 (0)7071/601-616
> www:    http://www.kyb.tuebingen.mpg.de/~kleinerm
> *********************************************************************
> "For a successful technology, reality must take precedence
> over public relations, for Nature cannot be fooled."
> (Richard Feynman)
>
>

-- 
 _____    Felix Kuehling
 \ _  |   MTS Software Development Eng.
 /|_| |   SW-Linux Base Gfx | AMD
|__/ \|   T 905.882.2600 x8928

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

[Mario Kleiner]

On Feb 27, 2012, at 4:47 PM, Felix Kuehling wrote:

> On 12-02-24 11:38 PM, Mario Kleiner wrote:
>> On Feb 24, 2012, at 10:20 PM, Felix Kuehling wrote:
>>
>>> On 12-02-22 11:20 AM, Felix Kuehling wrote:
>>>> On 12-02-21 07:49 PM, Mario Kleiner wrote:
>>>>> On 02/21/2012 09:07 PM, Alex Deucher wrote:
>>>> [snip]
>>>>>> The fix looks ok to me.  Mario any thoughts?
>>>>>>
>>>>>> Reviewed-by: Alex Deucher<alexdeucher@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.
>>>> Agreed. I was going to perform some more experiments on other  
>>>> hardware
>>>> to determine what the right threshold is for different hardware
>>>> generations. I hope I'll get to that this week.
>>>
>>> I have a final version of my patch including an explanation of the
>>> observations it's based on (attached). It's against current drm-next
>>> from git://people.freedesktop.org/~airlied/linux.
>>>
>>
>> The idea of the current logic was that it happened that the
>> update_pending bit isn't read back as zero at the end of the  
>> page_flip
>> programming function, immediately after clearing the graphics  
>> hardware
>> update_lock, e.g., maybe because there is some delay between clearing
>> that register and the double buffering taking place. But according to
>> the specs, if update_pending is high, ie., the hw has "accepted" the
>> request for a page flip, it will complete as long as that request
>> still happened within the vblank. So on a return from the page_flip
>> function with update_pending == 1 we check if we are still inside the
>> vblank area, ie., if the hw will certainly complete the double
>> buffering within the current vblank, because the request was made in
>> time.
>>
>> I did all my original testing of these bits with avivo hw (rv530,
>> r600) and with one radeon hd 5850, so i'm a bit surprised if the  
>> avivo
>> path would unconditionally need this adjustment. Could it be that the
>> observed accuracy of update_pending depends on the rest of the hw,
>> e.g., bus or processor speed, or bus activity? My test machines  
>> were a
>> MacBookPro with Core2Duo 2.3 Ghz for rv530 and a rather old AMD
>> Athlon-64 PC for r600.
>
> I used a few different systems:
>
> Brazos reference board for Brazos (E-350)
> Iconia Tab W500 (C-50)
> PhenomII X4 for most of the discrete cards
> Athlon64 X2 for RS690
>
> The results I got were consistent across all those systems. The only
> differences I saw between different generations of Avivo-based  
> hardware
> were about the exact timing of the threshold when I would start seeing
> flickering. On R5xx and R6xx it would start flickering at -3. On newer
> hardware at -4 (I didn't test on R7xx).
>
>>
>> I'm worried that your observed reliability of update_pending on >=
>> AVIVO asics could be an artifact of the specific computer hw you used
>> and that this doesn't generalize on older or different hw.
>
> Given the number of different ASICs and systems, I think that's
> unlikely. It's more likely that this depends on the exact mode  
> timings.
> I was running most of my experiments with a 19" DFP 1280x1024 at 60Hz,
> connected by DVI if available, or VGA on some of the IGPs.
>
> It's possible that different mode timings would result in a slightly
> different threshold.
>
>> If it doesn't generalize then the patch could defer a lot of  
>> perfectly
>> good pageflips by 1 frame, screwing the timestamps and reducing
>> framerate.
>
> I understand your concern. But given my observations, the current
> implementation potentially produces much more annoying artefacts.
>
>>
>> Here is what the rv630 register programming guide says about the
>> double-buffering of the surface base address registers:
>>
>> D1GRPH_SURFACE_UPDATE_PENDING: "the double buffering occurs in
>> vertical retrace when D1GRPH_SURFACE_UPDATE_PENDING = 1 and
>> D1GRPH_UPDATE_LOCK = 0 and V_UPDATE = 1."
>
> As far as I can tell, D1GRPH_SURFACE_UPDATE_PENDING will only be 1, if
> the page flip is initiated while outside the vertical retrace. The the
> actual page flip will occur when V_UPDATE changes to 1, that is, when
> the next vertical retrace occurs. So if we read
> D1GRPH_SURFACE_UPDATE_PENDING after initiating a page flip, it implies
> that we're currently not in a vertical retrace.
>
>>
>> D1CRTC_V_UPDATE: "Current vertical position ... 1 =  within the
>> V_UPDATE region (between end of vertical active display and  
>> start_line)"
>>
>> For us that would mean the threshold for deferred flip completion
>> would need to be whatever that mentioned "start_line" is, and for the
>> tested avivo hw, start_line used to be == start of active scanout, so
>> the threshold of zero made sense.
>
> In my experiments the start_line seemed to be between -4 and -2. On no
> ASIC did I observe a start_line == 0.
>
>>
>> Alex: I don't know where start_line is stored. Could it be that this
>> value changed due to hw changes or changes in the kms driver or
>> atombios? Or would it be possible to read that value back from some
>> register and use it as threshold?
>>
>> If you look at radeon_get_crtc_scanoutpos() you can also see that the
>> returned vpos is corrected for some offsets. Could it be that
>> something changed there for the DCE4.1 or DCE5 display engine or
>> whatever the C-50 APU uses? That could also explain why suddenly such
>> a weird threshold as vpos > -4 is needed on your tablet, because the
>> returned vpos is offset wrongly by a few scanlines.
>
> I believe the reason I observe flickering on my tablet is due to the
> short vsync period. Otherwise there is nothing special about my  
> tablet.
> I was able to induce the same flickering by artificially delaying the
> page flip on other Avivo hardware. My proposed fix resolves this
> potential problem on all Avivo hardware. I am very confident that it
> does that without deferring page flip notifications to user space
> unnecessarily.
>

Ok, you had a larger test set of machines and more specific test  
cases for this problem, i'm convinced. Thanks for all the testing.
You have my...

Reviewed-by: Mario Kleiner <mario.kleiner@tuebingen.mpg.de>

Wrt. to that new CRTC_ADVANCED_START_LINE_POSITION register on >=  
DCE4.1 that Alex found. Seems its default setting of 0x4 is the  
reason for the flicker threshold at vpos -4. Maybe it would by worth  
trying for the W500 tablet or devices/modes with similar short vblank  
intervals to reprogram that register with 0? That would give more  
headroom and maybe reduce your 25% number of deferred flips? Or is  
this likely to cause some hw trouble or artifacts because some line  
buffer can't fetch pixel data fast enough?

best,
-mario

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

[Felix Kuehling]

On 12-02-28 09:40 AM, Mario Kleiner wrote:
>
> Ok, you had a larger test set of machines and more specific test cases
> for this problem, i'm convinced. Thanks for all the testing.
> You have my...
>
> Reviewed-by: Mario Kleiner <mario.kleiner@tuebingen.mpg.de>

Thanks. Is there anything else I need to do to get this into drm-next?

>
> Wrt. to that new CRTC_ADVANCED_START_LINE_POSITION register on >=
> DCE4.1 that Alex found. Seems its default setting of 0x4 is the reason
> for the flicker threshold at vpos -4. Maybe it would by worth trying
> for the W500 tablet or devices/modes with similar short vblank
> intervals to reprogram that register with 0? That would give more
> headroom and maybe reduce your 25% number of deferred flips? Or is
> this likely to cause some hw trouble or artifacts because some line
> buffer can't fetch pixel data fast enough?

I'll ask our display team for advice.

Regards,
  Felix

>
> best,
> -mario
>
>

-- 
 _____    Felix Kuehling
 \ _  |   MTS Software Development Eng.
 /|_| |   SW-Linux Base Gfx | AMD
|__/ \|   T 905.882.2600 x8928

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

[Dave Airlie]

On Tue, Feb 28, 2012 at 3:06 PM, Felix Kuehling <felix.kuehling@amd.com> wrote:
> On 12-02-28 09:40 AM, Mario Kleiner wrote:
>>
>> Ok, you had a larger test set of machines and more specific test cases
>> for this problem, i'm convinced. Thanks for all the testing.
>> You have my...
>>
>> Reviewed-by: Mario Kleiner <mario.kleiner@tuebingen.mpg.de>
>
> Thanks. Is there anything else I need to do to get this into drm-next?

Nope, its in there now.

Dave.

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

[Paul Menzel]

[-- Attachment #1.1: Type: text/plain, Size: 716 bytes --]

Am Mittwoch, den 29.02.2012, 10:36 +0000 schrieb Dave Airlie:
> On Tue, Feb 28, 2012 at 3:06 PM, Felix Kuehling <felix.kuehling@amd.com> wrote:
> > On 12-02-28 09:40 AM, Mario Kleiner wrote:
> >>
> >> Ok, you had a larger test set of machines and more specific test cases
> >> for this problem, i'm convinced. Thanks for all the testing.
> >> You have my...
> >>
> >> Reviewed-by: Mario Kleiner <mario.kleiner@tuebingen.mpg.de>
> >
> > Thanks. Is there anything else I need to do to get this into drm-next?
> 
> Nope, its in there now.

Great. The URL is [1].


Thanks,

Paul


[1] http://cgit.freedesktop.org/~airlied/linux/commit/?h=drm-core-next&id=81ffbbedc37c6043e5f5b123da926aa7dd8ad60a

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel