Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Michel DÃnzer]

On 25/04/17 07:26 PM, Ville SyrjÃlà wrote:
> On Tue, Apr 25, 2017 at 10:12:37AM +0900, Michel DÃnzer wrote:
>> On 24/04/17 10:03 PM, Ville SyrjÃlà wrote:
>>> On Mon, Apr 24, 2017 at 03:57:02PM +0900, Michel DÃnzer wrote:
>>>> On 22/04/17 07:05 PM, Ville SyrjÃlà wrote:
>>>>> On Fri, Apr 21, 2017 at 06:14:31PM +0200, Gerd Hoffmann wrote:
>>>>>>   Hi,
>>>>>>
>>>>>>>> My personal opinion is that formats in drm_fourcc.h should be 
>>>>>>>> independent of the CPU byte order and the function 
>>>>>>>> drm_mode_legacy_fb_format() and drivers depending on that incorrect 
>>>>>>>> assumption be fixed instead.
>>>>>>>
>>>>>>> The problem is this isn't a kernel-internal thing any more.  With the
>>>>>>> addition of the ADDFB2 ioctl the fourcc codes became part of the
>>>>>>> kernel/userspace abi ...
>>>>>>
>>>>>> Ok, added some printk's to the ADDFB and ADDFB2 code paths and tested a
>>>>>> bit.  Apparently pretty much all userspace still uses the ADDFB ioctl.
>>>>>> xorg (modesetting driver) does.  gnome-shell in wayland mode does.
>>>>>> Seems the big transition to ADDFB2 didn't happen yet.
>>>>>>
>>>>>> I guess that makes changing drm_mode_legacy_fb_format + drivers a
>>>>>> reasonable option ...
>>>>>
>>>>> Yeah, I came to the same conclusion after chatting with some
>>>>> folks on irc.
>>>>>
>>>>> So my current idea is that we change any driver that wants to follow the
>>>>> CPU endianness
>>>>
>>>> This isn't really optional for various reasons, some of which have been
>>>> covered in this discussion.
>>>>
>>>>
>>>>> to declare support for big endian formats if the CPU is
>>>>> big endian. Presumably these are mostly the virtual GPU drivers.
>>>>>
>>>>> Additonally we'll make the mapping performed by drm_mode_legacy_fb_format()
>>>>> driver controlled. That way drivers that got changed to follow CPU
>>>>> endianness can return a framebuffer that matches CPU endianness. And
>>>>> drivers that expect the GPU endianness to not depend on the CPU
>>>>> endianness will keep working as they do now. The downside is that users
>>>>> of the legacy addfb ioctl will need to magically know which endianness
>>>>> they will get, but that is apparently already the case. And users of
>>>>> addfb2 will keep on specifying the endianness explicitly with
>>>>> DRM_FORMAT_BIG_ENDIAN vs. 0.
>>>>
>>>> I'm afraid it's not that simple.
>>>>
>>>> The display hardware of older (pre-R600 generation) Radeon GPUs does not
>>>> support the "big endian" formats directly. In order to allow userspace
>>>> to access pixel data in native endianness with the CPU, we instead use
>>>> byte-swapping functionality which only affects CPU access.
>>>
>>> OK, I'm getting confused. Based on our irc discussion I got the
>>> impression you don't byte swap CPU accesses.
>>
>> Sorry for the confusion. The radeon kernel driver does support
>> byte-swapping for CPU access to VRAM with pre-R600 GPUs, and this is
>> used for fbdev emulation. What I meant on IRC is that the xf86-video-ati
>> radeon driver doesn't make use of this, mostly because it only applies
>> while a BO is in VRAM, and userspace can't control when that's the case
>> (while a BO isn't being scanned out).
> 
> So that was my other question. So if someone just creates a bo, I presume
> ttm can more or less move it between system memory and vram at any
> time. So if we then mmap the bo, does it mean the CPU will see the bytes
> in different order depending on where the bo happens to live at
> the time the CPU access happens?

If either of the RADEON_TILING_SWAP_16/32BIT flags was set when the BO
was created, yes. That's why the xf86-video-ati radeon driver doesn't
use this functionality.

> And how would that work wih dumb bos?

radeon_mode_dumb_create doesn't set the RADEON_TILING_SWAP_16/32BIT
flags, so no byte swapping is performed for dumb BOs even in VRAM.


-- 
Earthling Michel DÃnzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer