Re: [PATCH] drm/radeon/kms: add dynamic engine reclocking (V9)

[Luca T. <kro...@gm...>]

Il Tue, Dec 22, 2009 at 11:06:16PM +0100, Rafał Miłecki ha scritto: 
> V2: reorganize functions, fix modesetting calls
> V3: rebase patch, use radeon's workqueue
> V4: enable on tested chipsets only, request VBLANK IRQs
> V5: enable PM on older hardware (IRQs, mode_fixup, dpms)
> V6: use separate dynpm module parameter
> V7: drop RADEON_ prefix, set minimum mode for dpms off
> V8: update legacy encoder call, fix order in rs600 IRQ
> V9: update compute_clocks call in legacy, not only DPMS_OFF

Hello,
I've yet to find time to test this patch, but I have a big architectural
doubt:

+                       if (rdev->pm.vblank_callback)
+                               queue_work(rdev->wq, &rdev->pm.reclock_work);
+

IIRC the reason to wait for VBLANK is to avoid reclocking the GPU while
it's drawing; this is especially important when reclocking the memory
(which is not done yet). Here however you defer the recloking to some
indeterminate future...
I think it would be safer to execute that code in the IH (or in a tasklet,
which is guaranteed to run after the handler); the only problem I see is
that atom_execute_table allocates some memory (ws? workspace?), but the
GFP flag can be easily adjusted; the rest of the ops seem safe.
What do you think?

Luca
-- 
Ci sono due cose che l'uomo non puo` nascondere:
essere ubriaco ed essere innamorato.