The radeon pageflip ioctl handler delayed submitting the pageflip to hardware until the vblank IRQ handler. On AMD Fusion (PALM GPU, G-T56N CPU), when using a reduced blanking CVT mode, a pageflip submitted to hardware in the IRQ handler failed to complete before the end of the vblank, resulting in a guaranteed halving of frame rate despite having plenty of spare CPU and GPU resource.
Fix this by moving the pageflip request to hardware into the pageflip ioctl, waiting until we are outside a vblank period so that pageflip timing is still accurate.
This doubles my frame rate in reduced blanking modes, and does not have an impact on CPU usage in normal blanking modes.
Signed-off-by: Simon Farnsworth simon.farnsworth@onelan.co.uk ---
This is merely a first attempt, and while I've tested it, I expect there to be bugs I've not thought about in here. I've only previously worked on the Intel driver, and that's simpler hardware.
In particular, I'm a bit hazy about what the fence in pageflip is doing - I assume it's there to synchronize drawing and scanout, so that I don't flip to a buffer that's still being drawn on by the GPU.
All review is welcome; if there's anything wrong here, I can easily produce a new version of this patch.
drivers/gpu/drm/radeon/radeon.h | 2 - drivers/gpu/drm/radeon/radeon_display.c | 59 ++++++++++-------------------- drivers/gpu/drm/radeon/radeon_mode.h | 1 - 3 files changed, 20 insertions(+), 42 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index ef0e0e0..1bd13aa 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -397,10 +397,8 @@ struct radeon_unpin_work { struct work_struct work; struct radeon_device *rdev; int crtc_id; - struct radeon_fence *fence; struct drm_pending_vblank_event *event; struct radeon_bo *old_rbo; - u64 new_crtc_base; };
struct r500_irq_stat_regs { diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c index 292f73f..93da446 100644 --- a/drivers/gpu/drm/radeon/radeon_display.c +++ b/drivers/gpu/drm/radeon/radeon_display.c @@ -276,47 +276,13 @@ void radeon_crtc_handle_flip(struct radeon_device *rdev, int crtc_id) struct drm_pending_vblank_event *e; struct timeval now; unsigned long flags; - u32 update_pending; - int vpos, hpos;
spin_lock_irqsave(&rdev->ddev->event_lock, flags); work = radeon_crtc->unpin_work; - if (work == NULL || - !radeon_fence_signaled(work->fence)) { - spin_unlock_irqrestore(&rdev->ddev->event_lock, flags); - return; - } - /* New pageflip, or just completion of a previous one? */ - if (!radeon_crtc->deferred_flip_completion) { - /* do the flip (mmio) */ - update_pending = radeon_page_flip(rdev, crtc_id, work->new_crtc_base); - } else { - /* This is just a completion of a flip queued in crtc - * at last invocation. Make sure we go directly to - * completion routine. - */ - update_pending = 0; - radeon_crtc->deferred_flip_completion = 0; - } - - /* 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; + if (work == NULL) { spin_unlock_irqrestore(&rdev->ddev->event_lock, flags); return; } - /* Pageflip (will be) certainly completed in this vblank. Clean up. */ radeon_crtc->unpin_work = NULL;
@@ -332,7 +298,6 @@ void radeon_crtc_handle_flip(struct radeon_device *rdev, int crtc_id) spin_unlock_irqrestore(&rdev->ddev->event_lock, flags);
drm_vblank_put(rdev->ddev, radeon_crtc->crtc_id); - radeon_fence_unref(&work->fence); radeon_post_page_flip(work->rdev, work->crtc_id); schedule_work(&work->work); } @@ -350,9 +315,11 @@ static int radeon_crtc_page_flip(struct drm_crtc *crtc, struct radeon_bo *rbo; struct radeon_fence *fence; struct radeon_unpin_work *work; + unsigned long flags; u32 tiling_flags, pitch_pixels; u64 base; + int vpos, hpos; int r;
work = kzalloc(sizeof *work, GFP_KERNEL); @@ -368,7 +335,7 @@ static int radeon_crtc_page_flip(struct drm_crtc *crtc, work->event = event; work->rdev = rdev; work->crtc_id = radeon_crtc->crtc_id; - work->fence = radeon_fence_ref(fence); + radeon_fence_ref(fence); old_radeon_fb = to_radeon_framebuffer(crtc->fb); new_radeon_fb = to_radeon_framebuffer(fb); /* schedule unpin of the old buffer */ @@ -387,7 +354,6 @@ static int radeon_crtc_page_flip(struct drm_crtc *crtc, goto unlock_free; } radeon_crtc->unpin_work = work; - radeon_crtc->deferred_flip_completion = 0; spin_unlock_irqrestore(&dev->event_lock, flags);
/* pin the new buffer */ @@ -449,7 +415,6 @@ static int radeon_crtc_page_flip(struct drm_crtc *crtc, }
spin_lock_irqsave(&dev->event_lock, flags); - work->new_crtc_base = base; spin_unlock_irqrestore(&dev->event_lock, flags);
/* update crtc fb */ @@ -475,6 +440,22 @@ static int radeon_crtc_page_flip(struct drm_crtc *crtc, /* fire the ring */ radeon_ring_unlock_commit(rdev);
+ /* Wait for our fence */ + r = radeon_fence_wait(fence, 1); + if (r) { + DRM_ERROR("failed to wait for flip fence\n"); + goto pflip_cleanup2; + } + radeon_fence_unref(&fence); + + /* Wait until we are out of vblank */ + /* Enhancement note: you could calculate how long to sleep for, based on vpos */ + while(radeon_get_crtc_scanoutpos(dev, radeon_crtc->crtc_id, &vpos, &hpos) & DRM_SCANOUTPOS_INVBL) + msleep(1); + + /* Request the flip */ + radeon_page_flip(rdev, radeon_crtc->crtc_id, base); + return 0;
pflip_cleanup2: diff --git a/drivers/gpu/drm/radeon/radeon_mode.h b/drivers/gpu/drm/radeon/radeon_mode.h index 6df4e3c..89d5a4c 100644 --- a/drivers/gpu/drm/radeon/radeon_mode.h +++ b/drivers/gpu/drm/radeon/radeon_mode.h @@ -282,7 +282,6 @@ struct radeon_crtc { int pll_id; /* page flipping */ struct radeon_unpin_work *unpin_work; - int deferred_flip_completion; };
struct radeon_encoder_primary_dac {
On Wed, Jul 6, 2011 at 12:29 PM, Simon Farnsworth simon.farnsworth@onelan.co.uk wrote:
The radeon pageflip ioctl handler delayed submitting the pageflip to hardware until the vblank IRQ handler. On AMD Fusion (PALM GPU, G-T56N CPU), when using a reduced blanking CVT mode, a pageflip submitted to hardware in the IRQ handler failed to complete before the end of the vblank, resulting in a guaranteed halving of frame rate despite having plenty of spare CPU and GPU resource.
Fix this by moving the pageflip request to hardware into the pageflip ioctl, waiting until we are outside a vblank period so that pageflip timing is still accurate.
This doubles my frame rate in reduced blanking modes, and does not have an impact on CPU usage in normal blanking modes.
Signed-off-by: Simon Farnsworth simon.farnsworth@onelan.co.uk
This is merely a first attempt, and while I've tested it, I expect there to be bugs I've not thought about in here. I've only previously worked on the Intel driver, and that's simpler hardware.
In particular, I'm a bit hazy about what the fence in pageflip is doing - I assume it's there to synchronize drawing and scanout, so that I don't flip to a buffer that's still being drawn on by the GPU.
All review is welcome; if there's anything wrong here, I can easily produce a new version of this patch.
In my view all the fence stuff become useless as when the irq handler call we know we can unpin previous scanout (the crtc is scanning from new buffer). Otherwise it looks good (beside the optimization of computing how much time we should sleep instead of while msleep but this can be left as a latter patch).
Cheers, Jerome
drivers/gpu/drm/radeon/radeon.h | 2 - drivers/gpu/drm/radeon/radeon_display.c | 59 ++++++++++-------------------- drivers/gpu/drm/radeon/radeon_mode.h | 1 - 3 files changed, 20 insertions(+), 42 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index ef0e0e0..1bd13aa 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -397,10 +397,8 @@ struct radeon_unpin_work { struct work_struct work; struct radeon_device *rdev; int crtc_id;
- struct radeon_fence *fence;
struct drm_pending_vblank_event *event; struct radeon_bo *old_rbo;
- u64 new_crtc_base;
};
struct r500_irq_stat_regs { diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c index 292f73f..93da446 100644 --- a/drivers/gpu/drm/radeon/radeon_display.c +++ b/drivers/gpu/drm/radeon/radeon_display.c @@ -276,47 +276,13 @@ void radeon_crtc_handle_flip(struct radeon_device *rdev, int crtc_id) struct drm_pending_vblank_event *e; struct timeval now; unsigned long flags;
- u32 update_pending;
- int vpos, hpos;
spin_lock_irqsave(&rdev->ddev->event_lock, flags); work = radeon_crtc->unpin_work;
- if (work == NULL ||
- !radeon_fence_signaled(work->fence)) {
- spin_unlock_irqrestore(&rdev->ddev->event_lock, flags);
- return;
- }
- /* New pageflip, or just completion of a previous one? */
- if (!radeon_crtc->deferred_flip_completion) {
- /* do the flip (mmio) */
- update_pending = radeon_page_flip(rdev, crtc_id, work->new_crtc_base);
- } else {
- /* This is just a completion of a flip queued in crtc
- * at last invocation. Make sure we go directly to
- * completion routine.
- */
- update_pending = 0;
- radeon_crtc->deferred_flip_completion = 0;
- }
- /* 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;
- if (work == NULL) {
spin_unlock_irqrestore(&rdev->ddev->event_lock, flags); return; }
/* Pageflip (will be) certainly completed in this vblank. Clean up. */ radeon_crtc->unpin_work = NULL;
@@ -332,7 +298,6 @@ void radeon_crtc_handle_flip(struct radeon_device *rdev, int crtc_id) spin_unlock_irqrestore(&rdev->ddev->event_lock, flags);
drm_vblank_put(rdev->ddev, radeon_crtc->crtc_id);
- radeon_fence_unref(&work->fence);
radeon_post_page_flip(work->rdev, work->crtc_id); schedule_work(&work->work); } @@ -350,9 +315,11 @@ static int radeon_crtc_page_flip(struct drm_crtc *crtc, struct radeon_bo *rbo; struct radeon_fence *fence; struct radeon_unpin_work *work;
unsigned long flags; u32 tiling_flags, pitch_pixels; u64 base;
- int vpos, hpos;
int r;
work = kzalloc(sizeof *work, GFP_KERNEL); @@ -368,7 +335,7 @@ static int radeon_crtc_page_flip(struct drm_crtc *crtc, work->event = event; work->rdev = rdev; work->crtc_id = radeon_crtc->crtc_id;
- work->fence = radeon_fence_ref(fence);
- radeon_fence_ref(fence);
old_radeon_fb = to_radeon_framebuffer(crtc->fb); new_radeon_fb = to_radeon_framebuffer(fb); /* schedule unpin of the old buffer */ @@ -387,7 +354,6 @@ static int radeon_crtc_page_flip(struct drm_crtc *crtc, goto unlock_free; } radeon_crtc->unpin_work = work;
- radeon_crtc->deferred_flip_completion = 0;
spin_unlock_irqrestore(&dev->event_lock, flags);
/* pin the new buffer */ @@ -449,7 +415,6 @@ static int radeon_crtc_page_flip(struct drm_crtc *crtc, }
spin_lock_irqsave(&dev->event_lock, flags);
- work->new_crtc_base = base;
spin_unlock_irqrestore(&dev->event_lock, flags);
/* update crtc fb */ @@ -475,6 +440,22 @@ static int radeon_crtc_page_flip(struct drm_crtc *crtc, /* fire the ring */ radeon_ring_unlock_commit(rdev);
- /* Wait for our fence */
- r = radeon_fence_wait(fence, 1);
- if (r) {
- DRM_ERROR("failed to wait for flip fence\n");
- goto pflip_cleanup2;
- }
- radeon_fence_unref(&fence);
- /* Wait until we are out of vblank */
- /* Enhancement note: you could calculate how long to sleep for, based on vpos */
- while(radeon_get_crtc_scanoutpos(dev, radeon_crtc->crtc_id, &vpos, &hpos) & DRM_SCANOUTPOS_INVBL)
- msleep(1);
- /* Request the flip */
- radeon_page_flip(rdev, radeon_crtc->crtc_id, base);
return 0;
pflip_cleanup2: diff --git a/drivers/gpu/drm/radeon/radeon_mode.h b/drivers/gpu/drm/radeon/radeon_mode.h index 6df4e3c..89d5a4c 100644 --- a/drivers/gpu/drm/radeon/radeon_mode.h +++ b/drivers/gpu/drm/radeon/radeon_mode.h @@ -282,7 +282,6 @@ struct radeon_crtc { int pll_id; /* page flipping */ struct radeon_unpin_work *unpin_work;
- int deferred_flip_completion;
};
struct radeon_encoder_primary_dac {
1.7.5.1
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wednesday 6 July 2011, Jerome Glisse j.glisse@gmail.com wrote:
On Wed, Jul 6, 2011 at 12:29 PM, Simon Farnsworth
In particular, I'm a bit hazy about what the fence in pageflip is doing - I assume it's there to synchronize drawing and scanout, so that I don't flip to a buffer that's still being drawn on by the GPU.
In my view all the fence stuff become useless as when the irq handler call we know we can unpin previous scanout (the crtc is scanning from new buffer). Otherwise it looks good (beside the optimization of computing how much time we should sleep instead of while msleep but this can be left as a latter patch).
So, I don't know why this makes a difference - but removing the fence completely causes the second pageflip to stall forever. Changing it for a radeon_bo_wait on the new frontbuffer still results in infinite stalls.
I've noticed a couple of minor issues in the patch anyway (a redundant lock/unlock pair left in, and I should be using usleep_range not msleep, given that I want to wait for less than a millisecond normally), so I'll respin anyway.
dri-devel@lists.freedesktop.org
-
Jerome Glisse -
Simon Farnsworth