Bug#572311: No display output from Radeon RV610 on Alpha

[Michael Cree]

On 13/03/10 06:57, Alex Deucher wrote:
> On Fri, Mar 12, 2010 at 4:57 AM, Michael Cree<mcree at orcon.net.nz>  wrote:
>> On 10/03/10 08:44, Alex Deucher wrote:
>>>>>>>
>>>>>>> On Sun, Mar 7, 2010 at 3:47 AM, Michael Cree<mcree at orcon.net.nz>
>>>>>>>   wrote:
>>>>>>>>
>>>>>>>> Thanks, that hint was helpful.  I have drummed up a patch (attached)
>>>>>>>> that
>>>>>>>> replaces some use of the UINT16LE_TO_CPU(), etc., macros with generic
>>>>>>>> interfaces from the Xserver's compiler.h header file.  Now works
>>>>>>>> correctly
>>>>>>>> on RV610 video card on an Alpha XP1000.  Have also verified that the
>>>>>>>> driver
>>>>>>>> still works on an RV710 card on AMD64 architecture.
>>>
>>> Can you add the alignment stuff to the ATOM_BSWAP16/32 functions in
>>> radeon_atombios.c?
>>> e.g.,
>>> return ldw_u(bswap_16(x));
>>
>> That's a good idea, however I think the ldw_u() must be inside the byte swap
>> as the (mis)alignment issues must be dealt with at the point of loading the
>> datum, whereas endianess can be fixed later.
>>
>> Attached is a new patch that uses the ldw_u() macros and also leaves the
>> UINT16LE_TO_CPU, etc., macros in place.  Verified working on Alpha and AMD64
>> architectures, but I don't have a suitable big-endian machine to test this.
>
> Wouldn't it be cleaner to just put the ldw_u() in ATOM_BSWAP16()?
>
> --- a/src/radeon_atombios.c
> +++ b/src/radeon_atombios.c
> @@ -28,6 +28,7 @@
>   #endif
>   #include "xf86.h"
>   #include "xf86_OSproc.h"
> +#include "compiler.h"
>
>   #include "radeon.h"
>   #include "radeon_reg.h"
> @@ -2981,7 +2982,7 @@
> atombios_get_command_table_version(atomBiosHandlePtr atomBIOS, int
> index, int *m
>
>   UINT16 ATOM_BSWAP16(UINT16 x)
>   {
> -    return bswap_16(x);
> +    return bswap_16(ldw_u(x));
>   }

No, that won't work for two reasons:

1) It's only in the big endian code path.  There are little endian 
architectures that need the ldw_u().

2) ldw_u()'s argument is a pointer (i.e. the address from which the word 
is to be loaded) not a value.  This is also the reason that I didn't 
include the ldw_u()s in the UINT16LE_TO_CPU, etc., macro definitions in 
Decoder.h.  Those macros sometimes wrap an access via a pointer and at 
other times they wrap an actual value.  Use of ldw_u() is only 
appropriate at the actual access of a datum from the AtomBios, i.e., 
those cases that perform an access through a pointer.

Cheers
Michael.