[PATCH 1/2] drm/radeon: Only enable and handle pageflip interrupts when needed
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
>>
>> 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 Signed-off-by:
>> Michel D?nzer
> 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
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/dri
[PATCH 1/2] drm/radeon: Only enable and handle pageflip interrupts when needed
Am 30.06.2014 14:31, schrieb Christian K?nig: > 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 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 Signed-off-by: Michel D?nzer >>> 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. Compiling... >>> 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. So please flesh something out (for r600 at least) and let me try ;-) BTW Have someone looked at this (dpm typos): Bug 79071 - Hang with dpm radeon hd 5750, pcie 1.1 motherboard https://bugzilla.kernel.org/show_bug.cgi?id=79071
[PATCH 1/2] drm/radeon: Only enable and handle pageflip interrupts when needed
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 >>> >>> 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 Signed-off-by: >>> Michel D?nzer >> 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 > 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_A
[PATCH 1/2] drm/radeon: Only enable and handle pageflip interrupts when needed
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 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 Signed-off-by: Michel D?nzer >>> Does patch #2 alone fixes the problem as well? >> It should avoid the panic as well. >> >> >>> 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. > this has already lead to quite some additional overhead in the fence > waiting code. radeon_irq_set() should probably be split up to reduce the overhead. >> The diagnostic messages Dieter was getting with only patch #2 show >> that the pflip interrupt often triggers unnecessarily, potentially >> wasting power by waking up the CPU from a power saving state >> pointlessly. > That's a really good point, but my question would rather be why does > the pflip interrupt fires if there isn't any pflip? There is a page flip, but it already completes in the vertical blank interrupt handler in a lot of (most?) cases. Which brings me back to the question: Do we really need the pflip interrupt yet? [0] Since flips are no longer programmed to the hardware in the vertical blank handler but in a work queue, is there actually still any problem with handling the flip completion in the vertical blank interrupt handler? FWIW, by disabling the radeon_crtc_handle_flip() call from the pflip interrupt handler, I no longer seem to be able to reproduce the 'impossible msc' lines in the Xorg log file. [0] Of course the pflip interrupt will be needed for asynchronous flips, but that doesn't mean we have to use it for all flips? -- Earthling Michel D?nzer| http://www.amd.com Libre software enthusiast |Mesa and X developer
[PATCH 1/2] drm/radeon: Only enable and handle pageflip interrupts when needed
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 > > 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 Signed-off-by: > Michel D?nzer Does patch #2 alone fixes the problem as well? >>> It should avoid the panic as well. >>> >>> 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() . > >> this has already lead to quite some additional overhead in the fence >> waiting code. > radeon_irq_set() should probably be split up to reduce the overhead. Yeah, agree totally but didn't had time for this yet (it has a rather low priority). Might be a good task for someone to get his hands dirty on the kernel code for the first time as well. I think turning interrupts on and off at each fence wait is the reason why inlining r100_mm_rreg and r100_mm_wreg again turned out to improve performance for some people quite a bit. >>> The diagnostic messages Dieter was getting with only patch #2 show >>> that the pflip interrupt often triggers unnecessarily, potentially >>> wasting power by waking up the CPU from a power saving state >>> pointlessly. >> That's a really good point, but my question would rather be why does >> the pflip interrupt fires if there isn't any pflip? > There is a page flip, but it already completes in the vertical blank > interrupt handler in a lot of (most?) cases. The issue is still that as far as I understand it when the vblank interrupt fires the flip is actually not completed yet. So we only don't see the pending bit high any more because of the minimal time between vblank fires and we check the bit. 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. > Which brings me back to the question: Do we really need the pflip > interrupt yet? [0] Since flips are no longer programmed to the hardware > in the vertical blank handler but in a work queue, is there actually > still any problem with handling the flip completion in the vertical > blank interrupt handler? Good question, essentially we need to test with a TV capable of the 24Hz modes. I don't have the necessary hardware here and last time was only able to test this by torturing my poor HP monitor with a 24Hz modeline it actually couldn't handle. > FWIW, by disabling the radeon_crtc_handle_flip() call from the pflip > interrupt handler, I no longer seem to be able to reproduce the > 'impossible msc' lines in the Xorg log file. Good to know, that narrows down the scenario how this problem is triggered. > [0] Of course the pflip interrupt will be needed for asynchronous flips, > but that doesn't mean we have to use it for all flips? It seemed to be the right thing for the job, but at this time I planned to use it exclusively and stop bothering with the vblank interrupt at all. But then Alex noted that the pflip interrupt might be unreliable on older system and so I wanted to use the exiting vblank handling as backup. Now I'm not sure any more what's the best approach here. Regards, Christian.
[PATCH 1/2] drm/radeon: Only enable and handle pageflip interrupts when needed
On 27.06.2014 12:03, Michel D?nzer wrote: > On 27.06.2014 11:06, Dieter N?tzel wrote: >> Am 27.06.2014 03:03, schrieb Michel D?nzer: >>> >>> The question is, can you reproduce the panic or the 'impossible msc' >>> lines in the Xorg log with only patch #2? >> >> Here we go (the 'new' #2 alone): >> >> For the panic I need more time. >> But the first 'impossible msc' line arise during X start: >> >> [80.271] (II) RADEON(0): Modeline "1680x1050"x0.0 119.00 1680 1728 >> 1760 1840 1050 1053 1059 1080 +hsync -vsync (64.7 kHz e) >> [80.271] (II) RADEON(0): Modeline "1920x1080"x60.0 172.80 1920 >> 2040 2248 2576 1080 1081 1084 1118 -hsync +vsync (67.1 kHz e) >> [ 100.246] (WW) RADEON(0): radeon_dri2_flip_event_handler: Pageflip >> completion event has impossible msc 5302 < target_msc 5303 >> >> Some arise with KWin cube effects. > > Once you've confirmed patch #2 alone avoids the panic, can you try if > adding patch #1 again avoids the 'impossible msc' lines as well? Never mind, I was able to reproduce them with both patches. -- Earthling Michel D?nzer| http://www.amd.com Libre software enthusiast |Mesa and X developer
[PATCH 1/2] drm/radeon: Only enable and handle pageflip interrupts when needed
On 27.06.2014 11:06, Dieter N?tzel wrote: > Am 27.06.2014 03:03, schrieb Michel D?nzer: >> >> The question is, can you reproduce the panic or the 'impossible msc' >> lines in the Xorg log with only patch #2? > > Here we go (the 'new' #2 alone): > > For the panic I need more time. > But the first 'impossible msc' line arise during X start: > > [80.271] (II) RADEON(0): Modeline "1680x1050"x0.0 119.00 1680 1728 > 1760 1840 1050 1053 1059 1080 +hsync -vsync (64.7 kHz e) > [80.271] (II) RADEON(0): Modeline "1920x1080"x60.0 172.80 1920 > 2040 2248 2576 1080 1081 1084 1118 -hsync +vsync (67.1 kHz e) > [ 100.246] (WW) RADEON(0): radeon_dri2_flip_event_handler: Pageflip > completion event has impossible msc 5302 < target_msc 5303 > > Some arise with KWin cube effects. Once you've confirmed patch #2 alone avoids the panic, can you try if adding patch #1 again avoids the 'impossible msc' lines as well? -- Earthling Michel D?nzer| http://www.amd.com Libre software enthusiast |Mesa and X developer
[PATCH 1/2] drm/radeon: Only enable and handle pageflip interrupts when needed
On 26.06.2014 19:39, Christian K?nig wrote: > Am 26.06.2014 11:29, schrieb Michel D?nzer: >> From: Michel D?nzer >> >> 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 >> Signed-off-by: Michel D?nzer > > Does patch #2 alone fixes the problem as well? It should avoid the panic as well. > 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. The diagnostic messages Dieter was getting with only patch #2 show that the pflip interrupt often triggers unnecessarily, potentially wasting power by waking up the CPU from a power saving state pointlessly. -- Earthling Michel D?nzer| http://www.amd.com Libre software enthusiast |Mesa and X developer
[PATCH 1/2] drm/radeon: Only enable and handle pageflip interrupts when needed
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 >>> >>> 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 >>> Signed-off-by: Michel D?nzer >> Does patch #2 alone fixes the problem as well? > It should avoid the panic as well. > > >> 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, this has already lead to quite some additional overhead in the fence waiting code. > > The diagnostic messages Dieter was getting with only patch #2 show that > the pflip interrupt often triggers unnecessarily, potentially wasting > power by waking up the CPU from a power saving state pointlessly. That's a really good point, but my question would rather be why does the pflip interrupt fires if there isn't any pflip? Turning the vblank and other interrupts off makes sense because they fire anyway, but pflip should only fire if we really made a request for it to do so. Regards, Christian.
[PATCH 1/2] drm/radeon: Only enable and handle pageflip interrupts when needed
On 27.06.2014 09:53, Dieter N?tzel wrote: > Am 26.06.2014 12:39, schrieb Christian K?nig: >> Am 26.06.2014 11:29, schrieb Michel D?nzer: >>> From: Michel D?nzer >>> >>> 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 >>> Signed-off-by: Michel D?nzer >> >> Does patch #2 alone fixes the problem as well? > > With #2 alone I get this during boot up (before plymouth): [...] > [ 15.259867] [drm:radeon_crtc_handle_flip] *ERROR* > radeon_crtc->flip_status = 0 != RADEON_FLIP_SUBMITTED(2) That's the original patch I sent to you along with two others for testing. The patch I submitted in this series has these messages downgraded to debugging messages, as they just show the patch preventing bad stuff from happening as designed. The question is, can you reproduce the panic or the 'impossible msc' lines in the Xorg log with only patch #2? > But with Michel's #1+2 and 3 I got this in Xorg.0.log: > (See Xorg.0.log.old.xz) > > (EE) [mi] EQ overflowing. Additional events will be discarded until > existing events are processed. That may not be directly related to the page flipping issues. -- Earthling Michel D?nzer| http://www.amd.com Libre software enthusiast |Mesa and X developer
[PATCH 1/2] drm/radeon: Only enable and handle pageflip interrupts when needed
Am 27.06.2014 03:03, schrieb Michel D?nzer: > On 27.06.2014 09:53, Dieter N?tzel wrote: >> Am 26.06.2014 12:39, schrieb Christian K?nig: >>> Am 26.06.2014 11:29, schrieb Michel D?nzer: From: Michel D?nzer 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 Signed-off-by: Michel D?nzer >>> >>> Does patch #2 alone fixes the problem as well? >> >> With #2 alone I get this during boot up (before plymouth): > > [...] > >> [ 15.259867] [drm:radeon_crtc_handle_flip] *ERROR* >> radeon_crtc->flip_status = 0 != RADEON_FLIP_SUBMITTED(2) > > That's the original patch I sent to you along with two others for > testing. The patch I submitted in this series has these messages > downgraded to debugging messages, as they just show the patch > preventing > bad stuff from happening as designed. > > The question is, can you reproduce the panic or the 'impossible msc' > lines in the Xorg log with only patch #2? Here we go (the 'new' #2 alone): For the panic I need more time. But the first 'impossible msc' line arise during X start: [80.271] (II) RADEON(0): Modeline "1680x1050"x0.0 119.00 1680 1728 1760 1840 1050 1053 1059 1080 +hsync -vsync (64.7 kHz e) [80.271] (II) RADEON(0): Modeline "1920x1080"x60.0 172.80 1920 2040 2248 2576 1080 1081 1084 1118 -hsync +vsync (67.1 kHz e) [ 100.246] (WW) RADEON(0): radeon_dri2_flip_event_handler: Pageflip completion event has impossible msc 5302 < target_msc 5303 Some arise with KWin cube effects. [ 1059.124] (WW) RADEON(0): radeon_dri2_flip_event_handler: Pageflip completion event has impossible msc 62948 < target_msc 62949 [ 1067.223] (WW) RADEON(0): radeon_dri2_flip_event_handler: Pageflip completion event has impossible msc 63435 < target_msc 63436 [ 1081.458] (WW) RADEON(0): radeon_dri2_flip_event_handler: Pageflip completion event has impossible msc 64290 < target_msc 64291 [ 1233.165] (WW) RADEON(0): radeon_dri2_flip_event_handler: Pageflip completion event has impossible msc 73416 < target_msc 73417 [ 1233.515] (WW) RADEON(0): radeon_dri2_flip_event_handler: Pageflip completion event has impossible msc 73437 < target_msc 73438 [ 1234.330] (WW) RADEON(0): radeon_dri2_flip_event_handler: Pageflip completion event has impossible msc 73486 < target_msc 73487 [ 1235.079] (WW) RADEON(0): radeon_dri2_flip_event_handler: Pageflip completion event has impossible msc 73531 < target_msc 73532 Sleep well! -Dieter >> But with Michel's #1+2 and 3 I got this in Xorg.0.log: >> (See Xorg.0.log.old.xz) >> >> (EE) [mi] EQ overflowing. Additional events will be discarded until >> existing events are processed. > > That may not be directly related to the page flipping issues.
[PATCH 1/2] drm/radeon: Only enable and handle pageflip interrupts when needed
Am 27.06.2014 03:03, schrieb Michel D?nzer: > On 27.06.2014 09:53, Dieter N?tzel wrote: >> Am 26.06.2014 12:39, schrieb Christian K?nig: >>> Am 26.06.2014 11:29, schrieb Michel D?nzer: From: Michel D?nzer 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 Signed-off-by: Michel D?nzer >>> >>> Does patch #2 alone fixes the problem as well? >> >> With #2 alone I get this during boot up (before plymouth): > > [...] > >> [ 15.259867] [drm:radeon_crtc_handle_flip] *ERROR* >> radeon_crtc->flip_status = 0 != RADEON_FLIP_SUBMITTED(2) > > That's the original patch I sent to you along with two others for > testing. The patch I submitted in this series has these messages > downgraded to debugging messages, as they just show the patch > preventing > bad stuff from happening as designed. > > The question is, can you reproduce the panic or the 'impossible msc' > lines in the Xorg log with only patch #2? Oh, shit sorry. I'll redo in a minute before I went to bed! -Dieter >> But with Michel's #1+2 and 3 I got this in Xorg.0.log: >> (See Xorg.0.log.old.xz) >> >> (EE) [mi] EQ overflowing. Additional events will be discarded until >> existing events are processed. > > That may not be directly related to the page flipping issues.
[PATCH 1/2] drm/radeon: Only enable and handle pageflip interrupts when needed
Am 26.06.2014 12:39, schrieb Christian K?nig:
> Am 26.06.2014 11:29, schrieb Michel D?nzer:
>> From: Michel D?nzer
>>
>> 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
>> Signed-off-by: Michel D?nzer
>
> Does patch #2 alone fixes the problem as well?
With #2 alone I get this during boot up (before plymouth):
[ 11.942342] [drm] fb depth is 24
[ 11.942344] [drm]pitch is 7680
[ 11.942739] fbcon: radeondrmfb (fb0) is primary device
[ 11.943090] [drm:radeon_crtc_handle_flip] *ERROR*
radeon_crtc->flip_status = 0 != RADEON_FLIP_SUBMITTED(2)
[ 11.943912] Console: switching to colour frame buffer device 240x67
[ 11.995623] radeon :01:00.0: fb0: radeondrmfb frame buffer device
[ 11.995628] radeon :01:00.0: registered panic notifier
[ 11.998112] [drm] Initialized radeon 2.39.0 20080528 for :01:00.0
on minor 0
[ 15.259867] [drm:radeon_crtc_handle_flip] *ERROR*
radeon_crtc->flip_status = 0 != RADEON_FLIP_SUBMITTED(2)
And during X / KDE start, again:
[ 30.960442] [drm:radeon_crtc_handle_flip] *ERROR*
radeon_crtc->flip_status = 0 != RADEON_FLIP_SUBMITTED(2)
[ 31.343925] [drm:radeon_crtc_handle_flip] *ERROR*
radeon_crtc->flip_status = 0 != RADEON_FLIP_SUBMITTED(2)
[ 37.687138] [drm:radeon_crtc_handle_flip] *ERROR*
radeon_crtc->flip_status = 0 != RADEON_FLIP_SUBMITTED(2)
[ 187.005621] [drm:radeon_crtc_handle_flip] *ERROR*
radeon_crtc->flip_status = 0 != RADEON_FLIP_SUBMITTED(2)
Much more in dmesg.
(See dmesg-3.16-rc2-Michel-2.xz)
But with Michel's #1+2 and 3 I got this in Xorg.0.log:
(See Xorg.0.log.old.xz)
(EE) [mi] EQ overflowing. Additional events will be discarded until
existing events are processed.
(EE)
(EE) Backtrace:
> I would rather want to avoid turning the pflip interrupt on and off.
I'm under the impression, that #2 alone is more responsive, but.
Sorry, after Fu?ball ;-)
Dieter
>> ---
>> 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 0f4b38f..7f522a4 100644
>> --- a/drivers/gpu/drm/radeon/cik.c
>> +++ b/drivers/gpu/drm/radeon/cik.c
>> @@ -7145,21 +7145,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);
>> @@ -7611,8 +7611,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 0443183..fa6e320 100644
>> --- a/drivers/gpu/drm/radeon/evergreen.c
>> +++ b/drivers/gpu/drm/radeon/evergreen.c
>> @@ -4542,20 +4542,20 @@ int evergreen_irq_set(struct radeon_device
>>
[PATCH 1/2] drm/radeon: Only enable and handle pageflip interrupts when needed
From: Michel D?nzer
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
Signed-off-by: Michel D?nzer
---
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 0f4b38f..7f522a4 100644
--- a/drivers/gpu/drm/radeon/cik.c
+++ b/drivers/gpu/drm/radeon/cik.c
@@ -7145,21 +7145,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);
@@ -7611,8 +7611,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 0443183..fa6e320 100644
--- a/drivers/gpu/drm/radeon/evergreen.c
+++ b/drivers/gpu/drm/radeon/evergreen.c
@@ -4542,20 +4542,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);
@@ -4950,8 +4950,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]))
+ radeo
[PATCH 1/2] drm/radeon: Only enable and handle pageflip interrupts when needed
Am 26.06.2014 12:39, schrieb Christian K?nig:
> Am 26.06.2014 11:29, schrieb Michel D?nzer:
>> From: Michel D?nzer
>>
>> 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
>> Signed-off-by: Michel D?nzer
>
> Does patch #2 alone fixes the problem as well?
>
> I would rather want to avoid turning the pflip interrupt on and off.
>
> Christian.
Christian,
I'll try for you after the doctor (my wife and I are sick over the last
several weeks(!) ;-() and before 'Fu?ball' ;-)))
--- That's why I'm on the 'hobel' that much and not in our forest,
etc...
But our two children are OK...
All the above is my 'new/second' life...
...before that I've studied computer science with medicine as second
part and had a deeper look into medical imagery.
Cheers,
Dieter
>> ---
>> 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 0f4b38f..7f522a4 100644
>> --- a/drivers/gpu/drm/radeon/cik.c
>> +++ b/drivers/gpu/drm/radeon/cik.c
>> @@ -7145,21 +7145,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);
>> @@ -7611,8 +7611,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 0443183..fa6e320 100644
>> --- a/drivers/gpu/drm/radeon/evergreen.c
>> +++ b/drivers/gpu/drm/radeon/evergreen.c
>> @@ -4542,20 +4542,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);
>> WREG
[PATCH 1/2] drm/radeon: Only enable and handle pageflip interrupts when needed
Am 26.06.2014 11:29, schrieb Michel D?nzer:
> From: Michel D?nzer
>
> 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
> Signed-off-by: Michel D?nzer
Does patch #2 alone fixes the problem as well?
I would rather want to avoid turning the pflip interrupt on and off.
Christian.
> ---
> 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 0f4b38f..7f522a4 100644
> --- a/drivers/gpu/drm/radeon/cik.c
> +++ b/drivers/gpu/drm/radeon/cik.c
> @@ -7145,21 +7145,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);
> @@ -7611,8 +7611,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 0443183..fa6e320 100644
> --- a/drivers/gpu/drm/radeon/evergreen.c
> +++ b/drivers/gpu/drm/radeon/evergreen.c
> @@ -4542,20 +4542,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);
> @@ -4950,8 +4950,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);
> - ra
