Re: [PATCH 1/2] drm/radeon: Only enable and handle pageflip interrupts when needed

["Michel Dänzer" <michel@daenzer.net>]

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 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel