On 25.06.2014 03:13, Dieter Nützel wrote:
> Am 24.06.2014 12:05, schrieb Michel Dänzer:
>> On 24.06.2014 05:32, Dieter Nützel wrote:
>>> Am 23.06.2014 21:46, schrieb Dieter Nützel:
>>>> Am 23.06.2014 11:34, schrieb Michel Dänzer:
>>>>> On 18.06.2014 18:14, Christian König wrote:
>>>>>> Am 18.06.2014 07:53, schrieb Michel Dänzer:
>>>>>>>
>>>>>>> (WW) RADEON(0): radeon_dri2_flip_event_handler: Pageflip
>>>>>>> completion
>>>>>>> event has impossible msc [x-1] < target_msc [x]
>>> [...]
>>> I can reliable generate such lines in Xorg.0.log with KWin cube desktop
>>> effect.
>>>
>>> Rotate screens with mouse wheel or screen switcher => new entry in
>>> Xorg.0.log. If it happens I notice ('see') flip delay.
>>
>> I was only able to reproduce it a couple of times even with that, but not
>> at all yet with the patch below. Does it help for you as well?
>
> Will try in the next run.
>
> My daughter generated kernel crash for us.;-)
> See would open up a zoom image in Konqi of a new Waveboard for here girl
> friends...
>
> But I could only take images with my mobile.
> kernel BUG at drivers/gpu/drm/drm_irq.c:976!
I was able to reproduce all these issues, and the attached three patches
fix them for me. Please let me know if you can still trigger the panic
or the diagnostic error messages in patch 2 somehow. If everything works
fine for you as well with these, I'll submit them with the error
messages in patch 2 changed to debug messages.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
>From 6de625e9c52ce4c8d8833de8f49cb97882c3c9d1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michel=20D=C3=A4nzer?= <michel.daenzer@xxxxxxx>
Date: Tue, 17 Jun 2014 16:33:16 +0900
Subject: [PATCH 1/3] drm/radeon: Only enable and handle pageflip interrupts
when needed
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
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().
Signed-off-by: Michel Dänzer <michel.daenzer@xxxxxxx>
---
drivers/gpu/drm/radeon/cik.c | 18 ++++++++++--------
drivers/gpu/drm/radeon/evergreen.c | 18 ++++++++++--------
drivers/gpu/drm/radeon/r600.c | 12 ++++++++----
drivers/gpu/drm/radeon/si.c | 18 ++++++++++--------
4 files changed, 38 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/radeon/cik.c b/drivers/gpu/drm/radeon/cik.c
index aea1478..228ccad 100644
--- a/drivers/gpu/drm/radeon/cik.c
+++ b/drivers/gpu/drm/radeon/cik.c
@@ -7136,21 +7136,21 @@ int cik_irq_set(struct radeon_device *rdev)
if (rdev->num_crtc >= 2) {
WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC0_REGISTER_OFFSET,
- GRPH_PFLIP_INT_MASK);
+ atomic_read(&rdev->irq.pflip[0]) ? GRPH_PFLIP_INT_MASK : 0);
WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC1_REGISTER_OFFSET,
- GRPH_PFLIP_INT_MASK);
+ atomic_read(&rdev->irq.pflip[1]) ? GRPH_PFLIP_INT_MASK : 0);
}
if (rdev->num_crtc >= 4) {
WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC2_REGISTER_OFFSET,
- GRPH_PFLIP_INT_MASK);
+ atomic_read(&rdev->irq.pflip[2]) ? GRPH_PFLIP_INT_MASK : 0);
WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC3_REGISTER_OFFSET,
- GRPH_PFLIP_INT_MASK);
+ atomic_read(&rdev->irq.pflip[3]) ? GRPH_PFLIP_INT_MASK : 0);
}
if (rdev->num_crtc >= 6) {
WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC4_REGISTER_OFFSET,
- GRPH_PFLIP_INT_MASK);
+ atomic_read(&rdev->irq.pflip[4]) ? GRPH_PFLIP_INT_MASK : 0);
WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC5_REGISTER_OFFSET,
- GRPH_PFLIP_INT_MASK);
+ atomic_read(&rdev->irq.pflip[5]) ? GRPH_PFLIP_INT_MASK : 0);
}
WREG32(DC_HPD1_INT_CONTROL, hpd1);
@@ -7602,8 +7602,10 @@ restart_ih:
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);
+ src_id = (src_id - 8) >> 1;
+ DRM_DEBUG("IH: D%d flip\n", src_id + 1);
+ if (atomic_read(&rdev->irq.pflip[src_id]))
+ radeon_crtc_handle_flip(rdev, src_id);
break;
case 42: /* HPD hotplug */
switch (src_data) {
diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c
index 653eff8..0fa0d19 100644
--- a/drivers/gpu/drm/radeon/evergreen.c
+++ b/drivers/gpu/drm/radeon/evergreen.c
@@ -4538,20 +4538,20 @@ int evergreen_irq_set(struct radeon_device *rdev)
}
WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC0_REGISTER_OFFSET,
- GRPH_PFLIP_INT_MASK);
+ atomic_read(&rdev->irq.pflip[0]) ? GRPH_PFLIP_INT_MASK : 0);
WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC1_REGISTER_OFFSET,
- GRPH_PFLIP_INT_MASK);
+ atomic_read(&rdev->irq.pflip[1]) ? GRPH_PFLIP_INT_MASK : 0);
if (rdev->num_crtc >= 4) {
WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC2_REGISTER_OFFSET,
- GRPH_PFLIP_INT_MASK);
+ atomic_read(&rdev->irq.pflip[2]) ? GRPH_PFLIP_INT_MASK : 0);
WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC3_REGISTER_OFFSET,
- GRPH_PFLIP_INT_MASK);
+ atomic_read(&rdev->irq.pflip[3]) ? GRPH_PFLIP_INT_MASK : 0);
}
if (rdev->num_crtc >= 6) {
WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC4_REGISTER_OFFSET,
- GRPH_PFLIP_INT_MASK);
+ atomic_read(&rdev->irq.pflip[4]) ? GRPH_PFLIP_INT_MASK : 0);
WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC5_REGISTER_OFFSET,
- GRPH_PFLIP_INT_MASK);
+ atomic_read(&rdev->irq.pflip[5]) ? GRPH_PFLIP_INT_MASK : 0);
}
WREG32(DC_HPD1_INT_CONTROL, hpd1);
@@ -4946,8 +4946,10 @@ restart_ih:
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);
+ src_id = (src_id - 8) >> 1;
+ DRM_DEBUG("IH: D%d flip\n", src_id + 1);
+ if (atomic_read(&rdev->irq.pflip[src_id]))
+ radeon_crtc_handle_flip(rdev, src_id);
break;
case 42: /* HPD hotplug */
switch (src_data) {
diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c
index c758812..8fa171c 100644
--- a/drivers/gpu/drm/radeon/r600.c
+++ b/drivers/gpu/drm/radeon/r600.c
@@ -3614,8 +3614,10 @@ int r600_irq_set(struct radeon_device *rdev)
WREG32(CP_INT_CNTL, cp_int_cntl);
WREG32(DMA_CNTL, dma_cntl);
WREG32(DxMODE_INT_MASK, mode_int);
- WREG32(D1GRPH_INTERRUPT_CONTROL, DxGRPH_PFLIP_INT_MASK);
- WREG32(D2GRPH_INTERRUPT_CONTROL, DxGRPH_PFLIP_INT_MASK);
+ WREG32(D1GRPH_INTERRUPT_CONTROL,
+ atomic_read(&rdev->irq.pflip[0]) ? DxGRPH_PFLIP_INT_MASK : 0);
+ WREG32(D2GRPH_INTERRUPT_CONTROL,
+ atomic_read(&rdev->irq.pflip[1]) ? DxGRPH_PFLIP_INT_MASK : 0);
WREG32(GRBM_INT_CNTL, grbm_int_cntl);
if (ASIC_IS_DCE3(rdev)) {
WREG32(DC_HPD1_INT_CONTROL, hpd1);
@@ -3920,11 +3922,13 @@ restart_ih:
break;
case 9: /* D1 pflip */
DRM_DEBUG("IH: D1 flip\n");
- radeon_crtc_handle_flip(rdev, 0);
+ if (atomic_read(&rdev->irq.pflip[0]))
+ radeon_crtc_handle_flip(rdev, 0);
break;
case 11: /* D2 pflip */
DRM_DEBUG("IH: D2 flip\n");
- radeon_crtc_handle_flip(rdev, 1);
+ if (atomic_read(&rdev->irq.pflip[1]))
+ radeon_crtc_handle_flip(rdev, 1);
break;
case 19: /* HPD/DAC hotplug */
switch (src_data) {
diff --git a/drivers/gpu/drm/radeon/si.c b/drivers/gpu/drm/radeon/si.c
index c128bde..5d37ea5 100644
--- a/drivers/gpu/drm/radeon/si.c
+++ b/drivers/gpu/drm/radeon/si.c
@@ -5919,21 +5919,21 @@ int si_irq_set(struct radeon_device *rdev)
if (rdev->num_crtc >= 2) {
WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC0_REGISTER_OFFSET,
- GRPH_PFLIP_INT_MASK);
+ atomic_read(&rdev->irq.pflip[0]) ? GRPH_PFLIP_INT_MASK : 0);
WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC1_REGISTER_OFFSET,
- GRPH_PFLIP_INT_MASK);
+ atomic_read(&rdev->irq.pflip[1]) ? GRPH_PFLIP_INT_MASK : 0);
}
if (rdev->num_crtc >= 4) {
WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC2_REGISTER_OFFSET,
- GRPH_PFLIP_INT_MASK);
+ atomic_read(&rdev->irq.pflip[2]) ? GRPH_PFLIP_INT_MASK : 0);
WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC3_REGISTER_OFFSET,
- GRPH_PFLIP_INT_MASK);
+ atomic_read(&rdev->irq.pflip[3]) ? GRPH_PFLIP_INT_MASK : 0);
}
if (rdev->num_crtc >= 6) {
WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC4_REGISTER_OFFSET,
- GRPH_PFLIP_INT_MASK);
+ atomic_read(&rdev->irq.pflip[4]) ? GRPH_PFLIP_INT_MASK : 0);
WREG32(GRPH_INT_CONTROL + EVERGREEN_CRTC5_REGISTER_OFFSET,
- GRPH_PFLIP_INT_MASK);
+ atomic_read(&rdev->irq.pflip[5]) ? GRPH_PFLIP_INT_MASK : 0);
}
if (!ASIC_IS_NODCE(rdev)) {
@@ -6303,8 +6303,10 @@ restart_ih:
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);
+ src_id = (src_id - 8) >> 1;
+ DRM_DEBUG("IH: D%d flip\n", src_id + 1);
+ if (atomic_read(&rdev->irq.pflip[src_id]))
+ radeon_crtc_handle_flip(rdev, src_id);
break;
case 42: /* HPD hotplug */
switch (src_data) {
--
2.0.0
>From a092643e1c39622c41609aa3e7eeb7f127542ef2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michel=20D=C3=A4nzer?= <michel.daenzer@xxxxxxx>
Date: Wed, 25 Jun 2014 15:25:36 +0900
Subject: [PATCH 2/3] drm/radeon: Track the status of a page flip more
explicitly
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This is another line of defence against the panic fixed by the previous
change, and should make the life cycle of a page flip clearer.
Signed-off-by: Michel Dänzer <michel.daenzer@xxxxxxx>
---
drivers/gpu/drm/radeon/radeon_display.c | 17 ++++++++++++-----
drivers/gpu/drm/radeon/radeon_mode.h | 7 +++++++
2 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
index 8b575a4..69e1efd 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -284,7 +284,6 @@ static void radeon_unpin_work_func(struct work_struct *__work)
void radeon_crtc_handle_vblank(struct radeon_device *rdev, int crtc_id)
{
struct radeon_crtc *radeon_crtc = rdev->mode_info.crtcs[crtc_id];
- struct radeon_flip_work *work;
unsigned long flags;
u32 update_pending;
int vpos, hpos;
@@ -294,8 +293,10 @@ void radeon_crtc_handle_vblank(struct radeon_device *rdev, int crtc_id)
return;
spin_lock_irqsave(&rdev->ddev->event_lock, flags);
- work = radeon_crtc->flip_work;
- if (work == NULL) {
+ if (radeon_crtc->flip_status != RADEON_FLIP_SUBMITTED) {
+ DRM_ERROR("radeon_crtc->flip_status = %d != "
+ "RADEON_FLIP_SUBMITTED(%d)\n",
+ radeon_crtc->flip_status, RADEON_FLIP_SUBMITTED);
spin_unlock_irqrestore(&rdev->ddev->event_lock, flags);
return;
}
@@ -343,12 +344,16 @@ void radeon_crtc_handle_flip(struct radeon_device *rdev, int crtc_id)
spin_lock_irqsave(&rdev->ddev->event_lock, flags);
work = radeon_crtc->flip_work;
- if (work == NULL) {
+ if (radeon_crtc->flip_status != RADEON_FLIP_SUBMITTED) {
+ DRM_ERROR("radeon_crtc->flip_status = %d != "
+ "RADEON_FLIP_SUBMITTED(%d)\n",
+ radeon_crtc->flip_status, RADEON_FLIP_SUBMITTED);
spin_unlock_irqrestore(&rdev->ddev->event_lock, flags);
return;
}
/* Pageflip completed. Clean up. */
+ radeon_crtc->flip_status = RADEON_FLIP_NONE;
radeon_crtc->flip_work = NULL;
/* wakeup userspace */
@@ -475,6 +480,7 @@ static void radeon_flip_work_func(struct work_struct *__work)
/* do the flip (mmio) */
radeon_page_flip(rdev, radeon_crtc->crtc_id, base);
+ radeon_crtc->flip_status = RADEON_FLIP_SUBMITTED;
spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
up_read(&rdev->exclusive_lock);
@@ -543,7 +549,7 @@ static int radeon_crtc_page_flip(struct drm_crtc *crtc,
/* We borrow the event spin lock for protecting flip_work */
spin_lock_irqsave(&crtc->dev->event_lock, flags);
- if (radeon_crtc->flip_work) {
+ if (radeon_crtc->flip_status != RADEON_FLIP_NONE) {
DRM_DEBUG_DRIVER("flip queue: crtc already busy\n");
spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
drm_gem_object_unreference_unlocked(&work->old_rbo->gem_base);
@@ -551,6 +557,7 @@ static int radeon_crtc_page_flip(struct drm_crtc *crtc,
kfree(work);
return -EBUSY;
}
+ radeon_crtc->flip_status = RADEON_FLIP_PENDING;
radeon_crtc->flip_work = work;
/* update crtc fb */
diff --git a/drivers/gpu/drm/radeon/radeon_mode.h b/drivers/gpu/drm/radeon/radeon_mode.h
index b59096f..ed6b6e0 100644
--- a/drivers/gpu/drm/radeon/radeon_mode.h
+++ b/drivers/gpu/drm/radeon/radeon_mode.h
@@ -301,6 +301,12 @@ struct radeon_atom_ss {
uint16_t amount;
};
+enum radeon_flip_status {
+ RADEON_FLIP_NONE,
+ RADEON_FLIP_PENDING,
+ RADEON_FLIP_SUBMITTED
+};
+
struct radeon_crtc {
struct drm_crtc base;
int crtc_id;
@@ -326,6 +332,7 @@ struct radeon_crtc {
/* page flipping */
struct workqueue_struct *flip_queue;
struct radeon_flip_work *flip_work;
+ enum radeon_flip_status flip_status;
/* pll sharing */
struct radeon_atom_ss ss;
bool ss_enabled;
--
2.0.0
>From 697c07badc5930708d04f55ae5e8ac3561edc478 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michel=20D=C3=A4nzer?= <michel.daenzer@xxxxxxx>
Date: Tue, 24 Jun 2014 18:38:54 +0900
Subject: [PATCH 3/3] drm/radeon: Avoid sending page flip completion event
prematurely
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Userspace expects page flips to only complete after the next vertical blank
interval. Don't send the page flip completion event before the vertical
blank sequence number has been incremented at least once.
Hopefully avoids lines like
(WW) RADEON(0): radeon_dri2_flip_event_handler: Pageflip completion event has impossible msc [x-1] < target_msc [x]
in the Xorg log file, and any corresponding breakage.
Signed-off-by: Michel Dänzer <michel.daenzer@xxxxxxx>
---
drivers/gpu/drm/radeon/radeon_display.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
index 69e1efd..f1bf514 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -337,11 +337,15 @@ void radeon_crtc_handle_flip(struct radeon_device *rdev, int crtc_id)
struct radeon_crtc *radeon_crtc = rdev->mode_info.crtcs[crtc_id];
struct radeon_flip_work *work;
unsigned long flags;
+ struct timeval vblank_time;
+ u32 vblank_seq;
/* this can happen at init */
if (radeon_crtc == NULL)
return;
+ vblank_seq = drm_vblank_count_and_time(rdev->ddev, crtc_id, &vblank_time);
+
spin_lock_irqsave(&rdev->ddev->event_lock, flags);
work = radeon_crtc->flip_work;
if (radeon_crtc->flip_status != RADEON_FLIP_SUBMITTED) {
@@ -352,6 +356,14 @@ void radeon_crtc_handle_flip(struct radeon_device *rdev, int crtc_id)
return;
}
+ work = radeon_crtc->flip_work;
+ if ((vblank_seq - work->event->event.sequence) > (1<<23)) {
+ DRM_DEBUG_DRIVER("vblank_seq = %u < %u\n",
+ vblank_seq, work->event->event.sequence);
+ spin_unlock_irqrestore(&rdev->ddev->event_lock, flags);
+ return;
+ }
+
/* Pageflip completed. Clean up. */
radeon_crtc->flip_status = RADEON_FLIP_NONE;
radeon_crtc->flip_work = NULL;
@@ -515,6 +527,7 @@ static int radeon_crtc_page_flip(struct drm_crtc *crtc,
struct radeon_framebuffer *new_radeon_fb;
struct drm_gem_object *obj;
struct radeon_flip_work *work;
+ struct timeval vblank_time;
unsigned long flags;
work = kzalloc(sizeof *work, GFP_KERNEL);
@@ -565,6 +578,10 @@ static int radeon_crtc_page_flip(struct drm_crtc *crtc,
spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
+ work->event->event.sequence =
+ drm_vblank_count_and_time(crtc->dev, radeon_crtc->crtc_id,
+ &vblank_time) + 1;
+
queue_work(radeon_crtc->flip_queue, &work->flip_work);
return 0;
--
2.0.0
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
