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

[Dieter Nützel]

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