On Sat, Mar 13, 2010 at 3:57 AM, Michael Cree <mc...@orcon.net.nz> wrote:
> On 13/03/10 06:57, Alex Deucher wrote:
>>
>> On Fri, Mar 12, 2010 at 4:57 AM, Michael Cree<mc...@orcon.net.nz> wrote:
>>>
>>> On 10/03/10 08:44, Alex Deucher wrote:
>>>>>>>>
>>>>>>>> On Sun, Mar 7, 2010 at 3:47 AM, Michael Cree<mc...@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
--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
