Am 30.06.2014 11:34, schrieb Michel Dänzer:
On 27.06.2014 19:47, Christian König wrote:
Am 27.06.2014 11:44, schrieb Michel Dänzer:
On 27.06.2014 17:18, Christian König wrote:
Am 27.06.2014 04:58, schrieb Michel Dänzer:
On 26.06.2014 19:39, Christian König wrote:
Am 26.06.2014 11:29, schrieb Michel Dänzer:
From: Michel Dänzer <michel.daenzer@xxxxxxx>
Prevents radeon_crtc_handle_flip() from running before
radeon_flip_work_func(), resulting in a kernel panic due to
the BUG_ON() in drm_vblank_put().
Tested-by: Dieter Nützel <Dieter@xxxxxxxxxxxxx> Signed-off-by:
Michel Dänzer <michel.daenzer@xxxxxxx>
Does patch #2 alone fixes the problem as well?
It should avoid the panic as well.
I've sent a v2 of that patch with an updated commit log. Alex, please get
that into 3.16 ASAP to prevent people from running into the panic.
I would rather want to avoid turning the pflip interrupt on and
off.
What's the problem with that? It's not like we're saving any
register writes by not doing it.
We don't? As far as I can see we reprogram all interrupt registers
if any of the interrupts changed,
Maybe I'm missing something, but: radeon_irq_kms_pflip_irq_get/put()
call radeon_irq_set() every time, as there can only be one active page
flip per CRTC. And radeon_irq_set() always writes the same registers,
only the bits it writes to them change depending on which interrupts the
driver is currently interested in.
We first turn on the vblank interrupt which results in a
radeon_irq_set() and then turn on the pflip, which results in another
radeon_irq_set() .
The DRM core delays disabling the hardware vblank interrupt, so
radeon_irq_set() should only be called once for that.
radeon_irq_kms_pflip_irq_get/put() always call radeon_irq_set() even before
my changes.
Anyway, for the issues surrounding the pflip interrupt, I took a step back
and considered a fundamentally different approach: It occurred to me that a
lot of the issues we've been struggling with are related to programming the
flips to the hardware such that they execute during the vertical blank
period. We don't know exactly when the hardware update happens or when would
be a good time to program the flip. So why bother with that at all?
I've considered this as well, but at this time still hoped that we could
completely replace the vblank interrupt with the pflip interrupt.
The patch below (only fleshed out for CIK) moves the programming of the
flip to the hardware back to the vertical blank interrupt handler, but
changes it to execute during the horizontal blank period. In addition to
allowing the pflip interrupt handling, radeon_crtc_handle_vblank() and
radeon_flip_pending() to be removed, this should make adding support for
asynchronous flips trivial and support for replacing pending flips much
easier. What do you think?
It also allows us to stop fiddling with the update pending bit as well,
e.g. no more busy waiting for it to become high in evergreen_page_flip.
And as far as I can see it still support all not so trivial use cases
like switching to hblank if rendering isn't completed before a certain
timeout and dynamic refresh etc...
Yeah, sounds like a really good idea to me.
The delay between vblank start and the flip being executed seemed to be
depending on the pixel clock (which makes sense because the CRTC is
driven by it), so when it might work ok for a 50Hz mode we can still run
into problems with 24Hz modes.
I couldn't see any tearing with this patch at 640x480 at 60 Hz and reduced
blanking, which should have a lower pixel clock and shorter vertical blank
period than 1920x1080 at 24 Hz?
In theory that should do the trick as well. But let's just make patches
for it and then we can make a request to the people who originally
reported the problem to test the whole implementation.
That should give us enough confidence that it will work correctly,
Christian.
commit 7b2861589f05c5c5c752108ce105161ebc0d6bf5
Author: Michel Dänzer <michel.daenzer@xxxxxxx>
Date: Mon Jun 30 17:57:01 2014 +0900
drm/radeon: Program page flips to execute in hblank instead of vblank
diff --git a/drivers/gpu/drm/radeon/atombios_crtc.c b/drivers/gpu/drm/radeon/atombios_crtc.c
index e911898..2842013 100644
--- a/drivers/gpu/drm/radeon/atombios_crtc.c
+++ b/drivers/gpu/drm/radeon/atombios_crtc.c
@@ -1284,6 +1284,10 @@ static int dce4_crtc_do_set_base(struct drm_crtc *crtc,
break;
}
+ /* Make sure updates happen at vertical blank */
+ WREG32(EVERGREEN_GRPH_FLIP_CONTROL + radeon_crtc->crtc_offset, 0);
+ WREG32(EVERGREEN_MASTER_UPDATE_MODE + radeon_crtc->crtc_offset, 0);
+
WREG32(EVERGREEN_GRPH_PRIMARY_SURFACE_ADDRESS_HIGH + radeon_crtc->crtc_offset,
upper_32_bits(fb_location));
WREG32(EVERGREEN_GRPH_SECONDARY_SURFACE_ADDRESS_HIGH + radeon_crtc->crtc_offset,
@@ -1321,15 +1325,6 @@ static int dce4_crtc_do_set_base(struct drm_crtc *crtc,
WREG32(EVERGREEN_VIEWPORT_SIZE + radeon_crtc->crtc_offset,
(viewport_w << 16) | viewport_h);
- /* pageflip setup */
- /* make sure flip is at vb rather than hb */
- tmp = RREG32(EVERGREEN_GRPH_FLIP_CONTROL + radeon_crtc->crtc_offset);
- tmp &= ~EVERGREEN_GRPH_SURFACE_UPDATE_H_RETRACE_EN;
- WREG32(EVERGREEN_GRPH_FLIP_CONTROL + radeon_crtc->crtc_offset, tmp);
-
- /* set pageflip to happen anywhere in vblank interval */
- WREG32(EVERGREEN_MASTER_UPDATE_MODE + radeon_crtc->crtc_offset, 0);
-
if (!atomic && fb && fb != crtc->primary->fb) {
radeon_fb = to_radeon_framebuffer(fb);
rbo = gem_to_radeon_bo(radeon_fb->obj);
diff --git a/drivers/gpu/drm/radeon/cik.c b/drivers/gpu/drm/radeon/cik.c
index 0f4b38f..d0a994c 100644
--- a/drivers/gpu/drm/radeon/cik.c
+++ b/drivers/gpu/drm/radeon/cik.c
@@ -7143,25 +7143,6 @@ int cik_irq_set(struct radeon_device *rdev)
WREG32(LB_INTERRUPT_MASK + EVERGREEN_CRTC5_REGISTER_OFFSET, crtc6);
}
- if (rdev->num_crtc >= 2) {
- WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC0_REGISTER_OFFSET,
- GRPH_PFLIP_INT_MASK);
- WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC1_REGISTER_OFFSET,
- GRPH_PFLIP_INT_MASK);
- }
- if (rdev->num_crtc >= 4) {
- WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC2_REGISTER_OFFSET,
- GRPH_PFLIP_INT_MASK);
- WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC3_REGISTER_OFFSET,
- GRPH_PFLIP_INT_MASK);
- }
- if (rdev->num_crtc >= 6) {
- WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC4_REGISTER_OFFSET,
- GRPH_PFLIP_INT_MASK);
- WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC5_REGISTER_OFFSET,
- GRPH_PFLIP_INT_MASK);
- }
-
WREG32(DC_HPD1_INT_CONTROL, hpd1);
WREG32(DC_HPD2_INT_CONTROL, hpd2);
WREG32(DC_HPD3_INT_CONTROL, hpd3);
@@ -7215,12 +7196,6 @@ static inline void cik_irq_ack(struct radeon_device *rdev)
EVERGREEN_CRTC5_REGISTER_OFFSET);
}
- if (rdev->irq.stat_regs.cik.d1grph_int & GRPH_PFLIP_INT_OCCURRED)
- WREG32(GRPH_INT_STATUS + EVERGREEN_CRTC0_REGISTER_OFFSET,
- GRPH_PFLIP_INT_CLEAR);
- if (rdev->irq.stat_regs.cik.d2grph_int & GRPH_PFLIP_INT_OCCURRED)
- WREG32(GRPH_INT_STATUS + EVERGREEN_CRTC1_REGISTER_OFFSET,
- GRPH_PFLIP_INT_CLEAR);
if (rdev->irq.stat_regs.cik.disp_int & LB_D1_VBLANK_INTERRUPT)
WREG32(LB_VBLANK_STATUS + EVERGREEN_CRTC0_REGISTER_OFFSET, VBLANK_ACK);
if (rdev->irq.stat_regs.cik.disp_int & LB_D1_VLINE_INTERRUPT)
@@ -7231,12 +7206,6 @@ static inline void cik_irq_ack(struct radeon_device *rdev)
WREG32(LB_VLINE_STATUS + EVERGREEN_CRTC1_REGISTER_OFFSET, VLINE_ACK);
if (rdev->num_crtc >= 4) {
- if (rdev->irq.stat_regs.cik.d3grph_int & GRPH_PFLIP_INT_OCCURRED)
- WREG32(GRPH_INT_STATUS + EVERGREEN_CRTC2_REGISTER_OFFSET,
- GRPH_PFLIP_INT_CLEAR);
- if (rdev->irq.stat_regs.cik.d4grph_int & GRPH_PFLIP_INT_OCCURRED)
- WREG32(GRPH_INT_STATUS + EVERGREEN_CRTC3_REGISTER_OFFSET,
- GRPH_PFLIP_INT_CLEAR);
if (rdev->irq.stat_regs.cik.disp_int_cont2 & LB_D3_VBLANK_INTERRUPT)
WREG32(LB_VBLANK_STATUS + EVERGREEN_CRTC2_REGISTER_OFFSET, VBLANK_ACK);
if (rdev->irq.stat_regs.cik.disp_int_cont2 & LB_D3_VLINE_INTERRUPT)
@@ -7248,12 +7217,6 @@ static inline void cik_irq_ack(struct radeon_device *rdev)
}
if (rdev->num_crtc >= 6) {
- if (rdev->irq.stat_regs.cik.d5grph_int & GRPH_PFLIP_INT_OCCURRED)
- WREG32(GRPH_INT_STATUS + EVERGREEN_CRTC4_REGISTER_OFFSET,
- GRPH_PFLIP_INT_CLEAR);
- if (rdev->irq.stat_regs.cik.d6grph_int & GRPH_PFLIP_INT_OCCURRED)
- WREG32(GRPH_INT_STATUS + EVERGREEN_CRTC5_REGISTER_OFFSET,
- GRPH_PFLIP_INT_CLEAR);
if (rdev->irq.stat_regs.cik.disp_int_cont4 & LB_D5_VBLANK_INTERRUPT)
WREG32(LB_VBLANK_STATUS + EVERGREEN_CRTC4_REGISTER_OFFSET, VBLANK_ACK);
if (rdev->irq.stat_regs.cik.disp_int_cont4 & LB_D5_VLINE_INTERRUPT)
@@ -7459,7 +7422,7 @@ restart_ih:
wake_up(&rdev->irq.vblank_queue);
}
if (atomic_read(&rdev->irq.pflip[0]))
- radeon_crtc_handle_vblank(rdev, 0);
+ radeon_crtc_handle_flip(rdev, 0);
rdev->irq.stat_regs.cik.disp_int &= ~LB_D1_VBLANK_INTERRUPT;
DRM_DEBUG("IH: D1 vblank\n");
}
@@ -7485,7 +7448,7 @@ restart_ih:
wake_up(&rdev->irq.vblank_queue);
}
if (atomic_read(&rdev->irq.pflip[1]))
- radeon_crtc_handle_vblank(rdev, 1);
+ radeon_crtc_handle_flip(rdev, 1);
rdev->irq.stat_regs.cik.disp_int_cont &= ~LB_D2_VBLANK_INTERRUPT;
DRM_DEBUG("IH: D2 vblank\n");
}
@@ -7511,7 +7474,7 @@ restart_ih:
wake_up(&rdev->irq.vblank_queue);
}
if (atomic_read(&rdev->irq.pflip[2]))
- radeon_crtc_handle_vblank(rdev, 2);
+ radeon_crtc_handle_flip(rdev, 2);
rdev->irq.stat_regs.cik.disp_int_cont2 &= ~LB_D3_VBLANK_INTERRUPT;
DRM_DEBUG("IH: D3 vblank\n");
}
@@ -7537,7 +7500,7 @@ restart_ih:
wake_up(&rdev->irq.vblank_queue);
}
if (atomic_read(&rdev->irq.pflip[3]))
- radeon_crtc_handle_vblank(rdev, 3);
+ radeon_crtc_handle_flip(rdev, 3);
rdev->irq.stat_regs.cik.disp_int_cont3 &= ~LB_D4_VBLANK_INTERRUPT;
DRM_DEBUG("IH: D4 vblank\n");
}
@@ -7563,7 +7526,7 @@ restart_ih:
wake_up(&rdev->irq.vblank_queue);
}
if (atomic_read(&rdev->irq.pflip[4]))
- radeon_crtc_handle_vblank(rdev, 4);
+ radeon_crtc_handle_flip(rdev, 4);
rdev->irq.stat_regs.cik.disp_int_cont4 &= ~LB_D5_VBLANK_INTERRUPT;
DRM_DEBUG("IH: D5 vblank\n");
}
@@ -7589,7 +7552,7 @@ restart_ih:
wake_up(&rdev->irq.vblank_queue);
}
if (atomic_read(&rdev->irq.pflip[5]))
- radeon_crtc_handle_vblank(rdev, 5);
+ radeon_crtc_handle_flip(rdev, 5);
rdev->irq.stat_regs.cik.disp_int_cont5 &= ~LB_D6_VBLANK_INTERRUPT;
DRM_DEBUG("IH: D6 vblank\n");
}
@@ -7605,15 +7568,6 @@ restart_ih:
break;
}
break;
- case 8: /* D1 page flip */
- case 10: /* D2 page flip */
- case 12: /* D3 page flip */
- case 14: /* D4 page flip */
- case 16: /* D5 page flip */
- case 18: /* D6 page flip */
- DRM_DEBUG("IH: D%d flip\n", ((src_id - 8) >> 1) + 1);
- radeon_crtc_handle_flip(rdev, (src_id - 8) >> 1);
- break;
case 42: /* HPD hotplug */
switch (src_data) {
case 0:
diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c
index 0443183..4798d61 100644
--- a/drivers/gpu/drm/radeon/evergreen.c
+++ b/drivers/gpu/drm/radeon/evergreen.c
@@ -1317,7 +1317,10 @@ void evergreen_page_flip(struct radeon_device *rdev, int crtc_id, u64 crtc_base)
{
struct radeon_crtc *radeon_crtc = rdev->mode_info.crtcs[crtc_id];
u32 tmp = RREG32(EVERGREEN_GRPH_UPDATE + radeon_crtc->crtc_offset);
- int i;
+
+ /* Take surface updates at horizontal blank */
+ WREG32(EVERGREEN_GRPH_FLIP_CONTROL + radeon_crtc->crtc_offset,
+ EVERGREEN_GRPH_SURFACE_UPDATE_H_RETRACE_EN);
/* Lock the graphics update lock */
tmp |= EVERGREEN_GRPH_UPDATE_LOCK;
@@ -1334,15 +1337,7 @@ void evergreen_page_flip(struct radeon_device *rdev, int crtc_id, u64 crtc_base)
WREG32(EVERGREEN_GRPH_PRIMARY_SURFACE_ADDRESS + radeon_crtc->crtc_offset,
(u32)crtc_base);
- /* Wait for update_pending to go high. */
- for (i = 0; i < rdev->usec_timeout; i++) {
- if (RREG32(EVERGREEN_GRPH_UPDATE + radeon_crtc->crtc_offset) & EVERGREEN_GRPH_SURFACE_UPDATE_PENDING)
- break;
- udelay(1);
- }
- DRM_DEBUG("Update pending now high. Unlocking vupdate_lock.\n");
-
- /* Unlock the lock, so double-buffering can take place inside vblank */
+ /* Unlock the lock, so double-buffering can take place inside hblank */
tmp &= ~EVERGREEN_GRPH_UPDATE_LOCK;
WREG32(EVERGREEN_GRPH_UPDATE + radeon_crtc->crtc_offset, tmp);
}
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
index 8e8ef08..c8b7f26 100644
--- a/drivers/gpu/drm/radeon/radeon.h
+++ b/drivers/gpu/drm/radeon/radeon.h
@@ -681,6 +681,7 @@ struct radeon_flip_work {
struct work_struct unpin_work;
struct radeon_device *rdev;
int crtc_id;
+ uint64_t base;
struct drm_framebuffer *fb;
struct drm_pending_vblank_event *event;
struct radeon_bo *old_rbo;
diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
index 65882cd..9a7554c 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -354,6 +354,9 @@ void radeon_crtc_handle_flip(struct radeon_device *rdev, int crtc_id)
return;
}
+ /* do the flip (mmio) */
+ radeon_page_flip(rdev, radeon_crtc->crtc_id, work->base);
+
/* Pageflip completed. Clean up. */
radeon_crtc->flip_status = RADEON_FLIP_NONE;
radeon_crtc->flip_work = NULL;
@@ -479,9 +482,7 @@ static void radeon_flip_work_func(struct work_struct *__work)
/* set the proper interrupt */
radeon_irq_kms_pflip_irq_get(rdev, radeon_crtc->crtc_id);
- /* do the flip (mmio) */
- radeon_page_flip(rdev, radeon_crtc->crtc_id, base);
-
+ work->base = base;
radeon_crtc->flip_status = RADEON_FLIP_SUBMITTED;
spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
up_read(&rdev->exclusive_lock);
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
