From: "Michel Dänzer" <michel@daenzer.net>
To: "Christian König" <deathsimple@vodafone.de>
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 1/2] drm/radeon: Only enable and handle pageflip interrupts when needed
Date: Mon, 30 Jun 2014 18:34:48 +0900 [thread overview]
Message-ID: <53B12F38.2050208@daenzer.net> (raw)
In-Reply-To: <53AD4BD6.5080401@vodafone.de>
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@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@nuetzel-hh.de> Signed-off-by:
>>>>>> Michel Dänzer <michel.daenzer@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@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2014-06-30 9:34 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-06-26 9:29 [PATCH 1/2] drm/radeon: Only enable and handle pageflip interrupts when needed Michel Dänzer
2014-06-26 9:29 ` [PATCH 2/2] drm/radeon: Track the status of a page flip more explicitly Michel Dänzer
2014-06-30 9:12 ` [PATCH v2] " Michel Dänzer
2014-06-30 9:14 ` Christian König
2014-07-01 16:23 ` Alex Deucher
2014-06-26 10:39 ` [PATCH 1/2] drm/radeon: Only enable and handle pageflip interrupts when needed Christian König
2014-06-26 11:51 ` Dieter Nützel
2014-06-27 0:53 ` Dieter Nützel
2014-06-27 1:03 ` Michel Dänzer
2014-06-27 1:08 ` Dieter Nützel
2014-06-27 2:06 ` Dieter Nützel
2014-06-27 3:03 ` Michel Dänzer
2014-06-27 3:17 ` Michel Dänzer
2014-06-27 2:58 ` Michel Dänzer
2014-06-27 8:18 ` Christian König
2014-06-27 9:44 ` Michel Dänzer
2014-06-27 10:47 ` Christian König
2014-06-30 9:34 ` Michel Dänzer [this message]
2014-06-30 12:31 ` Christian König
2014-06-30 16:22 ` Dieter Nützel
2014-07-01 8:14 ` [PATCH 1/2] drm/radeon: Program page flips to execute in hblank instead of vblank Michel Dänzer
2014-07-01 8:14 ` [PATCH 2/2] drm/radeon: Remove radeon_kms_pflip_irq_get/put() Michel Dänzer
2014-07-01 10:12 ` [PATCH 1/2] drm/radeon: Program page flips to execute in hblank instead of vblank Christian König
2014-07-02 3:55 ` [PATCH v2 " Michel Dänzer
2014-07-02 3:55 ` [PATCH v2 2/2] drm/radeon: Remove radeon_kms_pflip_irq_get/put() and the pflip atomic Michel Dänzer
2014-07-02 10:10 ` [PATCH 1/2] drm/radeon: Move pinning the BO back to radeon_crtc_page_flip() Michel Dänzer
2014-07-02 10:10 ` [PATCH 2/2] drm/radeon: Complete page flip even if waiting on the BO fence fails Michel Dänzer
2014-07-02 11:42 ` [PATCH 1/2] drm/radeon: Move pinning the BO back to radeon_crtc_page_flip() Christian König
2014-07-02 11:35 ` [PATCH v2 1/2] drm/radeon: Program page flips to execute in hblank instead of vblank Christian König
2014-07-01 16:46 ` [PATCH " Dieter Nützel
2014-07-02 0:29 ` Michel Dänzer
2014-07-02 3:01 ` Dieter Nützel
2014-07-02 3:11 ` Michel Dänzer
2014-07-02 3:52 ` Michel Dänzer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=53B12F38.2050208@daenzer.net \
--to=michel@daenzer.net \
--cc=deathsimple@vodafone.de \
--cc=dri-devel@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.