On Tue, Mar 24, 2009 at 1:57 PM, Michael Cree wrote:

...

I attach a patch to modify the _RHDRegRead() and _RHDRegWrite() functions (and their debug versions) to use the MMIO macros provided by the compiler.h header file.

On 25/03/2009, at 10:20 AM, Conn Clark wrote:

It would be better if you also redefined the RHDRegRead and RHDRegWrite macros in rhd.h to do the same thing. This would allow the compiler to inline the instructions instead of calling a function.

True. Admittedly this patch was the easiest approach to get the radeonhd driver to work on other architectures, and was done because I was too lazy to look through to see where the global variable used in _RHDRegRead(), etc., was defined and whether it is available throughout the code. If the radeonhd developers indicate that they are prepared to merge such patches then I am happy to spend more time on writing a new patch that redefines the macro definitions in rhd.h. BTW, top-posting is generally frowned upon in these lists (and for good reason!). Cheers Michael. -- To unsubscribe, e-mail: radeonhd+unsubscribe@opensuse.org For additional commands, e-mail: radeonhd+help@opensuse.org

On 25/03/2009, at 10:45 AM, Luc Verhaegen wrote:

On Wed, Mar 25, 2009 at 10:39:25AM +1300, Michael Cree wrote:

...

...

On Tue, Mar 24, 2009 at 1:57 PM, Michael Cree wrote:

...

I attach a patch to modify the _RHDRegRead() and _RHDRegWrite() functions (and their debug versions) to use the MMIO macros provided by the compiler.h header file.

On 25/03/2009, at 10:20 AM, Conn Clark wrote:

...

It would be better if you also redefined the RHDRegRead and RHDRegWrite macros in rhd.h to do the same thing. This would allow the compiler to inline the instructions instead of calling a function.

If the radeonhd developers indicate that they are prepared to merge such patches then I am happy to spend more time on writing a new patch that redefines the macro definitions in rhd.h.

Sure, send it along.

Thanks, will do. Probably be at least the weekend before I can work on it. Michael. -- To unsubscribe, e-mail: radeonhd+unsubscribe@opensuse.org For additional commands, e-mail: radeonhd+help@opensuse.org

On 26/03/2009, at 3:36 AM, Matthias Hopf wrote:

On Mar 25, 09 10:52:36 +1300, Michael Cree wrote:

...

...

...

...

...

I attach a patch to modify the _RHDRegRead() and _RHDRegWrite() functions On 25/03/2009, at 10:20 AM, Conn Clark wrote: It would be better if you also redefined the RHDRegRead and RHDRegWrite macros in rhd.h to do the same thing. This would allow the compiler to inline the instructions instead of calling a function.

If the radeonhd developers indicate that they are prepared to merge such patches then I am happy to spend more time on writing a new patch that redefines the macro definitions in rhd.h.

No probs. I think there aren't too many Alpha users out there besides you ;-)

I suspect you have missed the point. It is not relevant to Alpha users only - it is relevant to users of a number of different architectures. Memory barriers, at least of some sort, are defined to be non-empty in compiler.h for the following architectures: alpha, ia64, amd64, mips and powerpc. Note the presence of amd64 in that list! With the growing number of people using amd64 I would want to know exactly what the issues are with the amd64 memory barrier before writing code that uses direct memory access only to I/O ports. Special register access routines (rather than just macros) are provided for at least: alpha, sparc, mips, and powerpc. These seem predominantly to provide for big-endian architectures (in the case of Alpha it is for other reasons which probably have no relevance to the radeonhd driver). Alignment seems not to be an issue since the radeonhd driver appears to use 32 bit accesses on 32 bit aligned addresses only (ignoring AtomBIOS). So by doing direct memory access (which the radeonhd code uses at present) there is very real potential radeonhd breaks on every architecture listed above. Cheers Michael. -- To unsubscribe, e-mail: radeonhd+unsubscribe@opensuse.org For additional commands, e-mail: radeonhd+help@opensuse.org

On Mar 26, 09 09:45:46 +1300, Michael Cree wrote:

Memory barriers, at least of some sort, are defined to be non-empty in compiler.h for the following architectures: alpha, ia64, amd64, mips and powerpc.

I just checked the linux kernel headers (mb macro): It's a NOP on cris, m32r, m68k, mn10300, parisc, and xtensa. It's actual code on frv, mips, and x86. I stand corrected, though I'm puzzled which x86 processors need barriers (the comments in the code states "Some non-Intel clones support out of order store. wmb() ceases to be a * nop for these."). I don't think Athlons do that. BTW - what architecture is frv? How is powerpc, alpha, and ia64 called in kernel land? Do you happen to know? CU Matthias -- Matthias Hopf __ __ __ Maxfeldstr. 5 / 90409 Nuernberg (_ | | (_ |__ mat@mshopf.de Phone +49-911-74053-715 __) |_| __) |__ R & D www.mshopf.de -- To unsubscribe, e-mail: radeonhd+unsubscribe@opensuse.org For additional commands, e-mail: radeonhd+help@opensuse.org

Yang Zhao wrote:

I wanted to see if using the portable MMIO macros would improve the resume-from-suspend situation (it didn't) so I spent a bit of time getting rid of the old _RHDReg{Read/Write} calls. See attached patches.

You've beaten me to the gun! I was just about to submit such patches. I can report that your patch fixes both issues I had reported before on the Alpha architecture (mucked up colours on one Alpha and a failed assertion of MC Idle on another Alpha). Some further suggestions: I would put brackets around use of the arguments of RHDRegMask() in expressions, just in case expressions get passed as arguments. I.e.: #define RHDRegMask(ptr, offset, value, mask) \ do { \ CARD32 tmp; \ tmp = RHDRegRead(ptr, offset); \ tmp &= ~(mask); \ tmp |= ((value) & (mask)); \ RHDRegWrite(ptr, offset, tmp); \ } while(0) I would also like to suggest to the radeonhd developers the implementation of a new interface to reading I/O registers to further improve code efficiency. The current routines do a pointer dereference, an array indexing (equivalent to a pointer dereference in C), them two more pointer dereferences to get the MMIO address, for an effective total of four pointer dereferences every time an I/O register is accessed. This is recalculated every time a register is read or written, and this may be done many times within one subroutine. It is inefficient to be doing the same calculation over and over again. So I suggest adding register read/write routines like so: #define RHDRegReadIO(io, offset) MMIO_IN32(io, offset) #define RHDRegWriteIO(io, offset, value) MMIO_OUT32(io, offset, value) #define RHDRegMaskIO(io, offset, value, mask) \ do { \ CARD32 tmp; \ tmp = RHDRegReadIO(io, offset); \ tmp &= ~(mask); \ tmp |= ((value) & (mask)); \ RHDRegWriteIO(io, offset, tmp); \ } while(0) and then each subroutine does something like: some_function_call(RHDSomeObject *SomeObject) { pointer io = RHDPTRI(SomeObject)->MMIOBase; /* The code of the function with a register access ... */ value = RHDRegReadIO(io, THE_PARTICULAR_REGISTER_OF_INTEREST); /* More register accesses follow using the io pointer, but you probably now get the point. */ } Thoughts? Michael. -- To unsubscribe, e-mail: radeonhd+unsubscribe@opensuse.org For additional commands, e-mail: radeonhd+help@opensuse.org

On Sun, Mar 29, 2009 at 11:23:13AM +0200, Luc Verhaegen wrote:

On Sun, Mar 29, 2009 at 09:07:32PM +1300, Michael Cree wrote:

...

Yang Zhao wrote:

...

I wanted to see if using the portable MMIO macros would improve the resume-from-suspend situation (it didn't) so I spent a bit of time getting rid of the old _RHDReg{Read/Write} calls. See attached patches.

You've beaten me to the gun! I was just about to submit such patches.

I can report that your patch fixes both issues I had reported before on the Alpha architecture (mucked up colours on one Alpha and a failed assertion of MC Idle on another Alpha).

Some further suggestions:

I would put brackets around use of the arguments of RHDRegMask() in expressions, just in case expressions get passed as arguments. I.e.:

#define RHDRegMask(ptr, offset, value, mask) \ do { \ CARD32 tmp; \ tmp = RHDRegRead(ptr, offset); \ tmp &= ~(mask); \ tmp |= ((value) & (mask)); \ RHDRegWrite(ptr, offset, tmp); \ } while(0)

I would also like to suggest to the radeonhd developers the implementation of a new interface to reading I/O registers to further improve code efficiency. The current routines do a pointer dereference, an array indexing (equivalent to a pointer dereference in C), them two more pointer dereferences to get the MMIO address, for an effective total of four pointer dereferences every time an I/O register is accessed. This is recalculated every time a register is read or written, and this may be done many times within one subroutine. It is inefficient to be doing the same calculation over and over again. So I suggest adding register read/write routines like so:

#define RHDRegReadIO(io, offset) MMIO_IN32(io, offset) #define RHDRegWriteIO(io, offset, value) MMIO_OUT32(io, offset, value) #define RHDRegMaskIO(io, offset, value, mask) \ do { \ CARD32 tmp; \ tmp = RHDRegReadIO(io, offset); \ tmp &= ~(mask); \ tmp |= ((value) & (mask)); \ RHDRegWriteIO(io, offset, tmp); \ } while(0)

and then each subroutine does something like:

some_function_call(RHDSomeObject *SomeObject) { pointer io = RHDPTRI(SomeObject)->MMIOBase;

/* The code of the function with a register access ... */

value = RHDRegReadIO(io, THE_PARTICULAR_REGISTER_OF_INTEREST);

/* More register accesses follow using the io pointer, but you probably now get the point. */ }

Thoughts?

Michael.

This inefficiency is overrated. Most of these calls are used for modesetting, where you have little throughput and spend most time waiting on hw to respond anyway.

What the dereferencing brings us is the following; Every structure, at least where i could help it, has a member called scrnIndex. scrnIndex is pretty much a unique identifier in X for a ScrnInfoPtr, or the upper level of what is the device specific information. We can log messages through scrnIndex, we can access any other structure in the device specific information (if we really need to), but we mostly use it to hide away the knowledge of other structures so that every block can live on its own, pretty much oblivious of everything else (where it can be).

There has been a lot of discussion about whether this should be scrnIndex, but the answer to that is; in the current X infrastructure, there is no other candidate.

This structuring you can see throughout our codetree (where i could help it, i'm sure some recently added code broke that), and it is one of the main features of the radeonhd driver. Clean, selfcontained blocks.

RadeonHD, as it was built up from scratch, is pretty unique in this respect. And i came to this way of working after many years of actual X driver development, where a lot of time was spent finding commonality in bad code and restructuring until it made perfect sense. RadeonHD was where we could start from scratch.

So there is a very good reason why these io access routines are written like that, and it has helped us make the driver what it is today; the most modular, clean and hackable piece of X driver code around.

The efficiency gain in register access execution, overal, is overrated. We gained a lot, lot more by having a clear and well separated structure and good readable and solid code. In radeonhd you never have to wonder for long where you need to look, or where what bits of code are spread. While we have heard in the past that others find it easier to navigate competing code, this can only be an outrageous lie, or based upon nothing more than xenophobia and shortsighted (corporate) politics.

Luc Verhaegen.

That being said, there are a few cases where faster accesses make sense or already have workarounds; 2d code and cs has faster routines, cursor was pointed out by someone before, and in case of the dac, there are 768 accesses, to it definitely makes sense too. If you provide a secondary set of io access functions/macros, and convert a select few routines, i will be happy to accept it. Luc Verhaegen. -- To unsubscribe, e-mail: radeonhd+unsubscribe@opensuse.org For additional commands, e-mail: radeonhd+help@opensuse.org

2009/3/29 Michael Cree :

I would also like to suggest to the radeonhd developers the implementation of a new interface to reading I/O registers to further improve code efficiency... ... #define RHDRegReadIO(io, offset) MMIO_IN32(io, offset) #define RHDRegWriteIO(io, offset, value) MMIO_OUT32(io, offset, value) #define RHDRegMaskIO(io, offset, value, mask)   \ do {                                            \   CARD32 tmp;                                 \   tmp = RHDRegReadIO(io, offset);             \   tmp &= ~(mask);                             \   tmp |= ((value) & (mask));                  \   RHDRegWriteIO(io, offset, tmp);             \ } while(0)

At this point, the functions that want it might as well make calls to MMIO_*32() directly, and spare the unnecessary wrapping. It's easy enough to provide a MMIO_OUT32_MASKED() macro to give RHDRegMask functionality. -- Yang Zhao http://yangman.ca -- To unsubscribe, e-mail: radeonhd+unsubscribe@opensuse.org For additional commands, e-mail: radeonhd+help@opensuse.org

On Tue, Mar 24, 2009 at 2:39 PM, Michael Cree wrote:

...

On Tue, Mar 24, 2009 at 1:57 PM, Michael Cree wrote:

...

I attach a patch to modify the _RHDRegRead() and _RHDRegWrite() functions (and their debug versions) to use the MMIO macros provided by the compiler.h header file.

On 25/03/2009, at 10:20 AM, Conn Clark wrote:

...

It would be better if you also redefined the RHDRegRead and RHDRegWrite macros in rhd.h to do the same thing. This would allow the compiler to inline the instructions instead of calling a function.

True.  Admittedly this patch was the easiest approach to get the radeonhd driver to work on other architectures, and was done because I was too lazy to look through to see where the global variable used in _RHDRegRead(), etc., was defined and whether it is available throughout the code.

I'll send you the two macros I wrote to make it easier for you to modify them when I get home.

If the radeonhd developers indicate that they are prepared to merge such patches then I am happy to spend more time on writing a new patch that redefines the macro definitions in rhd.h.

BTW, top-posting is generally frowned upon in these lists (and for good reason!).

Cheers Michael.

And sorry about the top posting. At work here we have a lot of non techies that prefer top posting. I was in top posting mode <blush> Conn -- Conn O. Clark Observation: In formal computer science advances are made by standing on the shoulders of giants. Linux has proved that if there are enough of you, you can advance just as far by stepping on each others toes. -- To unsubscribe, e-mail: radeonhd+unsubscribe@opensuse.org For additional commands, e-mail: radeonhd+help@opensuse.org