[PATCH] xv: fix non-kms/non-dri Xv column ordering on big endian systems

[Alex Deucher]

On Fri, Aug 27, 2010 at 2:09 AM, Heikki Lindholm <holindho at saunalahti.fi> wrote:
> 27.8.2010 7.46, Alex Deucher kirjoitti:
>>
>> On Mon, Aug 23, 2010 at 2:52 AM, Heikki Lindholm<holin at iki.fi>  wrote:
>>>
>>> Column order is wrong on big endian systems, primarly because of a
>>> bits / bytes mix up with the bpp variable. Fix tested with r100 and
>>> r300, screen depth 16 and 32 with YV12 and YUY2 (overlay, textured
>>> video),
>>> RGBA and RGBT (overlay).
>>>
>>> Should fix: https://bugs.freedesktop.org/show_bug.cgi?id=29041
>>>
>>> Signed-off-by: Heikki Lindholm<holin at iki.fi>
>>> ---
>>>  src/radeon_video.c |   12 ++++++++----
>>>  1 files changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/src/radeon_video.c b/src/radeon_video.c
>>> index dc75279..1a42951 100644
>>> --- a/src/radeon_video.c
>>> +++ b/src/radeon_video.c
>>> @@ -2216,11 +2216,15 @@ RADEONCopyData(
>>>                swap = RADEON_HOST_DATA_SWAP_32BIT;
>>>                break;
>>>            }
>>> -       } else if (bpp != pScrn->bitsPerPixel) {
>>> -           if (bpp == 8)
>>> +       } else {
>>> +           switch (pScrn->bitsPerPixel) {
>>
>> don't we want to check the bpp rather then the scrn depth?
>
> I would have to guess no, because it just didn't seem to work that way. The
> cases I listed as tested behaved (with the patch applied) identical to what
> I saw on x86 with M7, whereas using bpp would quite definitely look
> different, for example, in the bpp = 4 (RGBA), depth = 16 case. Maybe
> there's byteswapping going on elsewhere as well?
>
> Btw. at least the YUV cases used to work with the 1.6.x driver versions. I
> wouldn't be surprised if the RGB overlays were never really tested on big
> endian at all.
>

Sounds good.  I've gone ahead and pushed it.  Thanks!

Alex

>> Wouldn't something like this work and be cleaner?
>>
>> diff --git a/src/radeon_video.c b/src/radeon_video.c
>> index dc75279..df0faf8 100644
>> --- a/src/radeon_video.c
>> +++ b/src/radeon_video.c
>> @@ -2207,20 +2207,13 @@ RADEONCopyData(
>>         int swap = RADEON_HOST_DATA_SWAP_NONE;
>>
>>  #if X_BYTE_ORDER == X_BIG_ENDIAN
>> -       if (info->kms_enabled) {
>> -           switch(bpp) {
>> -           case 2:
>> -               swap = RADEON_HOST_DATA_SWAP_16BIT;
>> -               break;
>> -           case 4:
>> -               swap = RADEON_HOST_DATA_SWAP_32BIT;
>> -               break;
>> -           }
>> -       } else if (bpp != pScrn->bitsPerPixel) {
>> -           if (bpp == 8)
>> -               swap = RADEON_HOST_DATA_SWAP_32BIT;
>> -           else
>> -               swap = RADEON_HOST_DATA_SWAP_HDW;
>> +       switch(bpp) {
>> +       case 2:
>> +         swap = RADEON_HOST_DATA_SWAP_16BIT;
>> +         break;
>> +       case 4:
>> +         swap = RADEON_HOST_DATA_SWAP_32BIT;
>> +         break;
>>         }
>>  #endif
>
> See above, and the bpp == 2 case is never hit, if you look at the very top
> of the function. Can't test the kms path myself, because kms enabled just
> hangs the gpu here.
>
> -- Heikki Lindholm
>