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

[Christian König]

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 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.