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

[Alex Deucher]

On Sat, Mar 13, 2010 at 3:57 AM, Michael Cree <mcree at orcon.net.nz> wrote:
> 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.

Thanks,  I've gone ahead and pushed it to master and 6.12-branch.

Alex