[DRIVER UPDATE] aty128fb

[Benjamin Herrenschmidt]

>Well then I suggest fixing it instead ;-)

Yep ;) I'm discussing with Paul about this issue. My current
understanding is that the few cases where sync is actually needed are
arch-specific anyway (TLB management, spinlock implementation, ...) and
so doesn't require this generic macro, but I want to be sure first.

>Benjamin> More simply, I beleive you can directly use readl/writel
>Benjamin> instead of using your own asm accessors. (Yours allow to use
>Benjamin> the index regiser. So they may actually be good for perfs).
>
>I don't think understand this one, sorry.

Using lwbrx/stwbrx allow to have separate registers for the base address
and the offset of the register. That means that a smart compiler can put
the base address in a register once and keep it for all the subsequent
accesses. But that's probably not relevant in real-life performances when
doing a register access in a driver. (It's more interesting when using
those functions to byte-swap a bunch of stuffs in memory, but that's a
different matter).

>The problem of putting them in front is that you may set a register
>and you have no idea when the operation will take place, thus the card
>may run with stale information in it's registers for a while.

This problem is true, regardless of the position of eieio. It's even a
non-platform specific problem since PCI write posting will cause this to
happen, regardless of the CPU ordering or eieio's.

If you need to have a known state and make sure the register was actually
written to the card, you need to do a read after the write from the same
address if possible (or from an other address on the same bus path).
Since this read will have it's eieio, things will then be completely
safe. This typically happens when dealing with on-board interrupt
masking/unmasking registers, when you want to make sure the interrupt is
actually masked before proceeding. (In this specific case of interrupt
management, you may also need an isync, but that's yet-another problem. I
beleive we lack a cross-platform macro for instruction-barrier, but I
still need to think more about this).

>With
>regard to filling the cursor then that particular example is not very
>good since opdating the cursor data is rarely a performance critical
>item ;-) However even for the performance critical cases (like
>printing data on the screen), you'd rather use the
>__raw_{read,write}l() macros and explicitly add the [wm]mb() macros.

Yep. Or you can eventually use normal memory accesses like it's done
usually when accessing the fb. datas for the cursor pixels. In any case,
you need wmb() macro between filling the cursor datas and enabling it,
and this is not ppc-specific.

>Why is it not fixed then? Or is there a reason for this, ie. does it
>need a "sync" on rmb for the SMP case of writing pointers to real
>memory. In that case we may need tp add pci_[rw]mb() instead.

I need to think more about this (and discuss with some people having more
knowledge thatn I do) before giving you a definitive answer on this. I
just noticed that they were defined as sync.

>I never got far enough in the PPC User's Manual to comment on how the
>specific instructions work. However if {read,write}l() is done as
>inline macros you still let the compiler optimize away. Though, even
>if they are not, then the calculations you perform to get to the
>proper address are quickly lost in the noise of the time it actually
>takes to access the PCI shared memory.

In this case, the compiler can't help since the readl/writel macro don't
use the index argument of the lwbrx/stwbrx instructions at all. But you
are probably right about the irrelevance of such an optimisation in the
driver. It may help keeing the code a bit smaller, which is always a good
thing IMHO, but I agree we could stick with readl/writel for now.




-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo en vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/