[PATCH 1/2] drm/radeon: Only enable and handle pageflip interrupts when needed
Michel Dänzer
michel at daenzer.net
Mon Jun 30 02:34:48 PDT 2014
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 at amd.com>
>>>>>>
>>>>>> 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 at nuetzel-hh.de> Signed-off-by:
>>>>>> Michel Dänzer <michel.daenzer at amd.com>
>>>>> 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?
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?
> 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?
commit 7b2861589f05c5c5c752108ce105161ebc0d6bf5
Author: Michel Dänzer <michel.daenzer at amd.com>
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);
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
More information about the dri-devel
mailing list