Guys,
I'm following the DRM kernel changes, and over the last few days code
appeared that I really don't think should be in the kernel, and that I
really don't want to merge.
The problem appears to be that the DRM people are used to using semaphores
to protect kernel data structures. That is WRONG. It leads to all kinds of
stupid problems, like realizing that you cannot use semaphores from
interrupts, so when interrupts need to use the data structures the code
tries to do all kinds of strange stuff, like using task-queues to do the
work that should have been done in the interrupt.
The fact is, that for data structure integrity, you should just use a
spinlock. And if the data structure needs to be accessed from interrupt
context, just use the irq-safe versions of the spinlock. Please don't
start using things like task-queues just because the locking was done
wrong in the first place.
Spinlocks are faster and simpler than semaphores, and optimize away on UP.
They also end up being a lot more straightforward to use - they have no
subtle issues. Yes, they require that you do all user-level accesses and
any allocations that can sleep outside the spinlocked region, but that's a
_goodness_, not a problem: it just forces the user to not make the
critical region bigger than it need be.
So _please_, whoever did this (Michel?):
- remove "dev->vbl_sem", and replace it with a "dev->vbl_lock" spinlock
- use "spin_lock_irqsave()/spun_unlock_irqrestore()" to protect the vbl
list.
- rename "vbl_immediate_bh()" as "vbl_send_signals()", and just call it
directly from the interrupt handler rather than playing all the games
with task-queues.
- remove all the vbl_tq stuff.
Quite frankly, I'd rather get rid of some of the old TQ stuff in DRM
(called "work queues" in modern 2.5.x), and I definitely do not want to
see _more_ of it. I suspect that the only reason the new code uses them is
that it was was what the DRM code already did from before.
(In other words: bonus points for doing the same thing for
"dma_immediate_bh" too - it looks like the only user of that is
"gamma_dma_immediate_bh()", and the locking in that chain of functions
looks so suspicious that I seriously doubt it can work reliably on SMP
anyway).
Linus
-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
_______________________________________________
Dri-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/dri-devel
