Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
On Thu, May 11, 2017 at 11:23:11PM +0200, Pavel Machek wrote: > On Fri 2017-04-21 14:08:04, Ville Syrjälä wrote: > > On Fri, Apr 21, 2017 at 11:50:18AM +0200, Gerd Hoffmann wrote: > > > On Fr, 2017-04-21 at 12:25 +0300, Ville Syrjälä wrote: > > > > On Fri, Apr 21, 2017 at 09:58:24AM +0200, Gerd Hoffmann wrote: > > > > > While working on graphics support for virtual machines on ppc64 (which > > > > > exists in both little and big endian variants) I've figured the > > > > > comments > > > > > for various drm fourcc formats in the header file don't match reality. > > > > > > > > > > Comments says the RGB formats are little endian, but in practice they > > > > > are native endian. Look at the drm_mode_legacy_fb_format() helper. > > > > > It > > > > > maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB, no > > > > > matter > > > > > whenever the machine is little endian or big endian. The users of > > > > > this > > > > > function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the > > > > > framebuffer > > > > > is native endian, not little endian. Most userspace also operates on > > > > > native endian only. > > > > > > > > I'm not a fan of "native". Native to what? "CPU" or "host" is what I'd > > > > call it. > > > > > > native == whatever the cpu is using. > > > > > > I personally find "native" more intuitive, but at the end of the day I > > > don't mind much. If people prefer "host" over "native" I'll change it. > > > > "native" to me feels more like "native to the GPU" since these things > > really are tied to the GPU not the CPU. That's also why I went with the > > explicit endianness originally so that the driver could properly declare > > what the GPU supports. > > You can easily have more than one GPU in the system. Plus these are > used by cameras / frame grabbers, too. So anything else than CPU > endianness is badly defined. The framebuffer has very little to do with the CPU. The display controller is the only consumer, and the producer could be whatever. -- Ville Syrjälä Intel OTC
Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
On Fri 2017-04-21 14:08:04, Ville Syrjälä wrote: > On Fri, Apr 21, 2017 at 11:50:18AM +0200, Gerd Hoffmann wrote: > > On Fr, 2017-04-21 at 12:25 +0300, Ville Syrjälä wrote: > > > On Fri, Apr 21, 2017 at 09:58:24AM +0200, Gerd Hoffmann wrote: > > > > While working on graphics support for virtual machines on ppc64 (which > > > > exists in both little and big endian variants) I've figured the comments > > > > for various drm fourcc formats in the header file don't match reality. > > > > > > > > Comments says the RGB formats are little endian, but in practice they > > > > are native endian. Look at the drm_mode_legacy_fb_format() helper. It > > > > maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB, no matter > > > > whenever the machine is little endian or big endian. The users of this > > > > function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer > > > > is native endian, not little endian. Most userspace also operates on > > > > native endian only. > > > > > > I'm not a fan of "native". Native to what? "CPU" or "host" is what I'd > > > call it. > > > > native == whatever the cpu is using. > > > > I personally find "native" more intuitive, but at the end of the day I > > don't mind much. If people prefer "host" over "native" I'll change it. > > "native" to me feels more like "native to the GPU" since these things > really are tied to the GPU not the CPU. That's also why I went with the > explicit endianness originally so that the driver could properly declare > what the GPU supports. You can easily have more than one GPU in the system. Plus these are used by cameras / frame grabbers, too. So anything else than CPU endianness is badly defined. (And I agree with the rest of the thread -- we should really be explicit; fourcc should specify what format the image data are in, and it should be possible to write fourcc + raw data into file and transfer it between machines.) Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
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
Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
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? And how would that work wih dumb bos? Would they be forced to live in vram? I see it's passing VRAM as the initial domain, but I can't quickly see whether that would mean it can't even be moved out. > > > > But since you do, how do you deal with mixing 8bpp vs. 16bpp vs. 32bpp? > > The byte-swapping is configured per-BO via the > RADEON_TILING_SWAP_16/32BIT flags. Which translates into usage of the surface regs it seems. So I wasn't totally crazy to think that such things existed :) -- Ville Syrjälä Intel OTC
Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
On 25/04/17 10:12 AM, 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). > > >> But since you do, how do you deal with mixing 8bpp vs. 16bpp vs. 32bpp? > > The byte-swapping is configured per-BO via the > RADEON_TILING_SWAP_16/32BIT flags. ... which means that it's disabled by default, so it shouldn't affect generic userspace. So exposing the GPU format directly should be feasible in this case as well after all. Sorry for the noise. :( -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer
Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
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). > But since you do, how do you deal with mixing 8bpp vs. 16bpp vs. 32bpp? The byte-swapping is configured per-BO via the RADEON_TILING_SWAP_16/32BIT flags. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer
Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
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. But since you do, how do you deal with mixing 8bpp vs. 16bpp vs. 32bpp? > This means > that the GPU and CPU effectively see different representations of the > same video memory contents. > > Userspace code dealing with GPU access to pixel data needs to know the > format as seen by the GPU, whereas code dealing with CPU access needs to > know the format as seen by the CPU. I don't see any way to express this > with a single format definition. Hmm. Well that certainly makes life even more interesting. -- Ville Syrjälä Intel OTC
Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
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. This means that the GPU and CPU effectively see different representations of the same video memory contents. Userspace code dealing with GPU access to pixel data needs to know the format as seen by the GPU, whereas code dealing with CPU access needs to know the format as seen by the CPU. I don't see any way to express this with a single format definition. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer
Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
On 23/04/17 04:24 AM, Ilia Mirkin wrote: > > fbdev also creates fb's that expect cpu endianness, as disabling the > byteswap logic caused a green fbcon terminal to show up. (So at least > something somewhere in the fbcon -> nouveau's fbdev emulation pipeline > is expecting cpu endianness. This happens both with nouveau's fbdev > accel logic and without.) In theory, there's FB_FOREIGN_ENDIAN for that. But in practice it's probably useless because little if any userspace even checks for it, let alone handles it correctly. > So I think the current situation, at least wrt pre-nv50 nouveau, is > that XRGB/ARGB are "special", since they are the only things > exposed by drm_crtc_init. I believe those definitions should be > updated to note that they're cpu-endian-specific (or another way of > phrasing it more diplomatically is that they're array formats rather > than packed formats). That would be incorrect. :) The memory layout of 8-bit-per-component array formats doesn't depend on endianness, that of packed formats does. (DRM_FORMAT_*8 as currently defined are thus effectively array formats) -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer
Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
Hi, > > 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 to declare support for big endian formats if the CPU is > big endian. Presumably these are mostly the virtual GPU drivers. Agree. Easy. > Additonally we'll make the mapping performed by drm_mode_legacy_fb_format() > driver controlled. I don't think this is useful. IMO drm_mode_legacy_fb_format should return host endian formats unconditionally. > 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. Existing userspace expects host endian, and IMO we should maintain that behavior. > And users of > addfb2 will keep on specifying the endianness explicitly with > DRM_FORMAT_BIG_ENDIAN vs. 0. I'd drop DRM_FORMAT_BIG_ENDIAN. At least for the virt drivers it doesn't buy us anything. They support 32bpp / 8 bpc formats only[1], and for those I can specify the byteswapped format version without a bigendian flag because we have fourccs for everything we need. There is a WIP patch series at https://www.kraxel.org/cgit/linux/log/?h=drm-byteorder Needs more testing and better commit messages. /me plans to polish & post next week, but feel free to look and comment. cheers, Gerd [1] Everything else is a PITA to deal with on the host side because I can't offload that to pixman. There is no support for PIXMAN_r5g6b5 or PIXMAN_x2b10g10r10 in non-host byte order.
Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
On Sat, Apr 22, 2017 at 9:48 AM, Ilia Mirkin wrote:
> On Sat, Apr 22, 2017 at 9:40 AM, Ilia Mirkin wrote:
>> On Sat, Apr 22, 2017 at 5:50 AM, Ville Syrjälä
>> wrote:
>>> On Sat, Apr 22, 2017 at 01:07:57AM -0400, Ilia Mirkin wrote:
On Fri, Apr 21, 2017 at 12:59 PM, Ville Syrjälä
wrote:
> On Fri, Apr 21, 2017 at 10:49:49AM -0400, Ilia Mirkin wrote:
>> On Fri, Apr 21, 2017 at 3:58 AM, Gerd Hoffmann
>> wrote:
>> > While working on graphics support for virtual machines on ppc64 (which
>> > exists in both little and big endian variants) I've figured the
>> > comments
>> > for various drm fourcc formats in the header file don't match reality.
>> >
>> > Comments says the RGB formats are little endian, but in practice they
>> > are native endian. Look at the drm_mode_legacy_fb_format() helper.
>> > It
>> > maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB, no
>> > matter
>> > whenever the machine is little endian or big endian. The users of
>> > this
>> > function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the
>> > framebuffer
>> > is native endian, not little endian. Most userspace also operates on
>> > native endian only.
>> >
>> > So, go update the comments for all 16+24+32 bpp RGB formats.
>> >
>> > Leaving the yuv formats as-is. I have no idea if and how those are
>> > used
>> > on bigendian machines.
>>
>> I think this is premature. The current situation is that I can't get
>> modetest to work *at all* on my NV34 / BE setup (I mean, it runs, just
>> the colors displayed are wrong). I believe that currently it packs
>> things in "cpu native endian". I've tried futzing with that without
>> much success, although I didn't spend too much time on it. I have a
>> NV34 plugged into my LE setup as well although I haven't tested to
>> double-check that it all works there. However I'm quite sure it used
>> to, as I used modetest to help develop the YUV overlay support for
>> those GPUs.
>
> I just took a quick stab at fixing modetest to respect the current
> wording in drm_fourcc.h:
>
> git://github.com/vsyrjala/libdrm.git modetest_endian
Looks like there was some careless testing on my part :( So ... it
looks like the current modetest without those changes does, in fact,
work on NV34/BE. With the changes, it breaks (and the handling of the
b* modes is a little broken in those patches -- they're not selectable
from the cmdline.) Which means that, as Michel & co predicted, it
appears to be taking BE input not LE input. This is very surprising to
me, but it is what it is. As I mentioned before, the details of how
the "BE" mode works on the GPUs is largely unknown to us beyond a few
basics. Note that only XR24 works, AR24 ends up with all black
displayed. This also happens on LE.
>>>
>>> Did you try 8bpp or 16bpp formats? I expect that if you've just blindly
>>> enabled some magic byte swapper in the hardware it will only for
>>> a specific pixel size.
>>
>> Thankfully dispnv04 exposes no such madness - just XR24 (and AR24,
>> although that doesn't appear functional). Yes, it's likely that
>> there's a byteswap happening somewhere. In fact the copy engines have
>> parameters somewhere to tell how the swap should be done (basically
>> what the element size is). I don't quite know how to set that though
>> on this generation. I should poke at VRAM via the mmio peephole and
>> see what's actually being stored. Although of course MMIO accesses are
>> also auto-byteswapped. It's all just one big massive headache.
>
> Or it could just be the obvious:
>
> static void nv_crtc_commit(struct drm_crtc *crtc)
> {
> struct drm_device *dev = crtc->dev;
> const struct drm_crtc_helper_funcs *funcs = crtc->helper_private;
> struct nouveau_crtc *nv_crtc = nouveau_crtc(crtc);
>
> nouveau_hw_load_state(dev, nv_crtc->index,
> &nv04_display(dev)->mode_reg);
> nv04_crtc_mode_set_base(crtc, crtc->x, crtc->y, NULL);
>
> #ifdef __BIG_ENDIAN
> /* turn on LFB swapping */
> {
> uint8_t tmp = NVReadVgaCrtc(dev, nv_crtc->index,
> NV_CIO_CRE_RCR);
> tmp |= MASK(NV_CIO_CRE_RCR_ENDIAN_BIG);
> NVWriteVgaCrtc(dev, nv_crtc->index, NV_CIO_CRE_RCR, tmp);
> }
> #endif
>
> funcs->dpms(crtc, DRM_MODE_DPMS_ON);
> drm_crtc_vblank_on(crtc);
> }
>
> So presumably instead of that __BIG_ENDIAN thing, it should instead be
> if crtc->primary->fb->fourcc & BIG_ENDIAN. (Although that's probably
> going to break the universe.) There's also some questionable support
> for 16-bit modes in the code, I'm going to see how easy it is to flip
> on.
OK, so just to follow up, I'd like to note a few things that were not
obvious to me, but perhaps were to all of you
Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
On Sat, Apr 22, 2017 at 9:40 AM, Ilia Mirkin wrote:
> On Sat, Apr 22, 2017 at 5:50 AM, Ville Syrjälä
> wrote:
>> On Sat, Apr 22, 2017 at 01:07:57AM -0400, Ilia Mirkin wrote:
>>> On Fri, Apr 21, 2017 at 12:59 PM, Ville Syrjälä
>>> wrote:
>>> > On Fri, Apr 21, 2017 at 10:49:49AM -0400, Ilia Mirkin wrote:
>>> >> On Fri, Apr 21, 2017 at 3:58 AM, Gerd Hoffmann wrote:
>>> >> > While working on graphics support for virtual machines on ppc64 (which
>>> >> > exists in both little and big endian variants) I've figured the
>>> >> > comments
>>> >> > for various drm fourcc formats in the header file don't match reality.
>>> >> >
>>> >> > Comments says the RGB formats are little endian, but in practice they
>>> >> > are native endian. Look at the drm_mode_legacy_fb_format() helper. It
>>> >> > maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB, no
>>> >> > matter
>>> >> > whenever the machine is little endian or big endian. The users of this
>>> >> > function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer
>>> >> > is native endian, not little endian. Most userspace also operates on
>>> >> > native endian only.
>>> >> >
>>> >> > So, go update the comments for all 16+24+32 bpp RGB formats.
>>> >> >
>>> >> > Leaving the yuv formats as-is. I have no idea if and how those are
>>> >> > used
>>> >> > on bigendian machines.
>>> >>
>>> >> I think this is premature. The current situation is that I can't get
>>> >> modetest to work *at all* on my NV34 / BE setup (I mean, it runs, just
>>> >> the colors displayed are wrong). I believe that currently it packs
>>> >> things in "cpu native endian". I've tried futzing with that without
>>> >> much success, although I didn't spend too much time on it. I have a
>>> >> NV34 plugged into my LE setup as well although I haven't tested to
>>> >> double-check that it all works there. However I'm quite sure it used
>>> >> to, as I used modetest to help develop the YUV overlay support for
>>> >> those GPUs.
>>> >
>>> > I just took a quick stab at fixing modetest to respect the current
>>> > wording in drm_fourcc.h:
>>> >
>>> > git://github.com/vsyrjala/libdrm.git modetest_endian
>>>
>>> Looks like there was some careless testing on my part :( So ... it
>>> looks like the current modetest without those changes does, in fact,
>>> work on NV34/BE. With the changes, it breaks (and the handling of the
>>> b* modes is a little broken in those patches -- they're not selectable
>>> from the cmdline.) Which means that, as Michel & co predicted, it
>>> appears to be taking BE input not LE input. This is very surprising to
>>> me, but it is what it is. As I mentioned before, the details of how
>>> the "BE" mode works on the GPUs is largely unknown to us beyond a few
>>> basics. Note that only XR24 works, AR24 ends up with all black
>>> displayed. This also happens on LE.
>>
>> Did you try 8bpp or 16bpp formats? I expect that if you've just blindly
>> enabled some magic byte swapper in the hardware it will only for
>> a specific pixel size.
>
> Thankfully dispnv04 exposes no such madness - just XR24 (and AR24,
> although that doesn't appear functional). Yes, it's likely that
> there's a byteswap happening somewhere. In fact the copy engines have
> parameters somewhere to tell how the swap should be done (basically
> what the element size is). I don't quite know how to set that though
> on this generation. I should poke at VRAM via the mmio peephole and
> see what's actually being stored. Although of course MMIO accesses are
> also auto-byteswapped. It's all just one big massive headache.
Or it could just be the obvious:
static void nv_crtc_commit(struct drm_crtc *crtc)
{
struct drm_device *dev = crtc->dev;
const struct drm_crtc_helper_funcs *funcs = crtc->helper_private;
struct nouveau_crtc *nv_crtc = nouveau_crtc(crtc);
nouveau_hw_load_state(dev, nv_crtc->index,
&nv04_display(dev)->mode_reg);
nv04_crtc_mode_set_base(crtc, crtc->x, crtc->y, NULL);
#ifdef __BIG_ENDIAN
/* turn on LFB swapping */
{
uint8_t tmp = NVReadVgaCrtc(dev, nv_crtc->index,
NV_CIO_CRE_RCR);
tmp |= MASK(NV_CIO_CRE_RCR_ENDIAN_BIG);
NVWriteVgaCrtc(dev, nv_crtc->index, NV_CIO_CRE_RCR, tmp);
}
#endif
funcs->dpms(crtc, DRM_MODE_DPMS_ON);
drm_crtc_vblank_on(crtc);
}
So presumably instead of that __BIG_ENDIAN thing, it should instead be
if crtc->primary->fb->fourcc & BIG_ENDIAN. (Although that's probably
going to break the universe.) There's also some questionable support
for 16-bit modes in the code, I'm going to see how easy it is to flip
on.
-ilia
Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
On Sat, Apr 22, 2017 at 5:50 AM, Ville Syrjälä wrote: > On Sat, Apr 22, 2017 at 01:07:57AM -0400, Ilia Mirkin wrote: >> On Fri, Apr 21, 2017 at 12:59 PM, Ville Syrjälä >> wrote: >> > On Fri, Apr 21, 2017 at 10:49:49AM -0400, Ilia Mirkin wrote: >> >> On Fri, Apr 21, 2017 at 3:58 AM, Gerd Hoffmann wrote: >> >> > While working on graphics support for virtual machines on ppc64 (which >> >> > exists in both little and big endian variants) I've figured the comments >> >> > for various drm fourcc formats in the header file don't match reality. >> >> > >> >> > Comments says the RGB formats are little endian, but in practice they >> >> > are native endian. Look at the drm_mode_legacy_fb_format() helper. It >> >> > maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB, no matter >> >> > whenever the machine is little endian or big endian. The users of this >> >> > function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer >> >> > is native endian, not little endian. Most userspace also operates on >> >> > native endian only. >> >> > >> >> > So, go update the comments for all 16+24+32 bpp RGB formats. >> >> > >> >> > Leaving the yuv formats as-is. I have no idea if and how those are used >> >> > on bigendian machines. >> >> >> >> I think this is premature. The current situation is that I can't get >> >> modetest to work *at all* on my NV34 / BE setup (I mean, it runs, just >> >> the colors displayed are wrong). I believe that currently it packs >> >> things in "cpu native endian". I've tried futzing with that without >> >> much success, although I didn't spend too much time on it. I have a >> >> NV34 plugged into my LE setup as well although I haven't tested to >> >> double-check that it all works there. However I'm quite sure it used >> >> to, as I used modetest to help develop the YUV overlay support for >> >> those GPUs. >> > >> > I just took a quick stab at fixing modetest to respect the current >> > wording in drm_fourcc.h: >> > >> > git://github.com/vsyrjala/libdrm.git modetest_endian >> >> Looks like there was some careless testing on my part :( So ... it >> looks like the current modetest without those changes does, in fact, >> work on NV34/BE. With the changes, it breaks (and the handling of the >> b* modes is a little broken in those patches -- they're not selectable >> from the cmdline.) Which means that, as Michel & co predicted, it >> appears to be taking BE input not LE input. This is very surprising to >> me, but it is what it is. As I mentioned before, the details of how >> the "BE" mode works on the GPUs is largely unknown to us beyond a few >> basics. Note that only XR24 works, AR24 ends up with all black >> displayed. This also happens on LE. > > Did you try 8bpp or 16bpp formats? I expect that if you've just blindly > enabled some magic byte swapper in the hardware it will only for > a specific pixel size. Thankfully dispnv04 exposes no such madness - just XR24 (and AR24, although that doesn't appear functional). Yes, it's likely that there's a byteswap happening somewhere. In fact the copy engines have parameters somewhere to tell how the swap should be done (basically what the element size is). I don't quite know how to set that though on this generation. I should poke at VRAM via the mmio peephole and see what's actually being stored. Although of course MMIO accesses are also auto-byteswapped. It's all just one big massive headache. -ilia
Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
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 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. Does that sound like a workable solution? -- Ville Syrjälä Intel OTC
Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
On Sat, Apr 22, 2017 at 01:07:57AM -0400, Ilia Mirkin wrote: > On Fri, Apr 21, 2017 at 12:59 PM, Ville Syrjälä > wrote: > > On Fri, Apr 21, 2017 at 10:49:49AM -0400, Ilia Mirkin wrote: > >> On Fri, Apr 21, 2017 at 3:58 AM, Gerd Hoffmann wrote: > >> > While working on graphics support for virtual machines on ppc64 (which > >> > exists in both little and big endian variants) I've figured the comments > >> > for various drm fourcc formats in the header file don't match reality. > >> > > >> > Comments says the RGB formats are little endian, but in practice they > >> > are native endian. Look at the drm_mode_legacy_fb_format() helper. It > >> > maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB, no matter > >> > whenever the machine is little endian or big endian. The users of this > >> > function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer > >> > is native endian, not little endian. Most userspace also operates on > >> > native endian only. > >> > > >> > So, go update the comments for all 16+24+32 bpp RGB formats. > >> > > >> > Leaving the yuv formats as-is. I have no idea if and how those are used > >> > on bigendian machines. > >> > >> I think this is premature. The current situation is that I can't get > >> modetest to work *at all* on my NV34 / BE setup (I mean, it runs, just > >> the colors displayed are wrong). I believe that currently it packs > >> things in "cpu native endian". I've tried futzing with that without > >> much success, although I didn't spend too much time on it. I have a > >> NV34 plugged into my LE setup as well although I haven't tested to > >> double-check that it all works there. However I'm quite sure it used > >> to, as I used modetest to help develop the YUV overlay support for > >> those GPUs. > > > > I just took a quick stab at fixing modetest to respect the current > > wording in drm_fourcc.h: > > > > git://github.com/vsyrjala/libdrm.git modetest_endian > > Looks like there was some careless testing on my part :( So ... it > looks like the current modetest without those changes does, in fact, > work on NV34/BE. With the changes, it breaks (and the handling of the > b* modes is a little broken in those patches -- they're not selectable > from the cmdline.) Which means that, as Michel & co predicted, it > appears to be taking BE input not LE input. This is very surprising to > me, but it is what it is. As I mentioned before, the details of how > the "BE" mode works on the GPUs is largely unknown to us beyond a few > basics. Note that only XR24 works, AR24 ends up with all black > displayed. This also happens on LE. Did you try 8bpp or 16bpp formats? I expect that if you've just blindly enabled some magic byte swapper in the hardware it will only for a specific pixel size. -- Ville Syrjälä Intel OTC
Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
On Fri, Apr 21, 2017 at 12:59 PM, Ville Syrjälä wrote: > On Fri, Apr 21, 2017 at 10:49:49AM -0400, Ilia Mirkin wrote: >> On Fri, Apr 21, 2017 at 3:58 AM, Gerd Hoffmann wrote: >> > While working on graphics support for virtual machines on ppc64 (which >> > exists in both little and big endian variants) I've figured the comments >> > for various drm fourcc formats in the header file don't match reality. >> > >> > Comments says the RGB formats are little endian, but in practice they >> > are native endian. Look at the drm_mode_legacy_fb_format() helper. It >> > maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB, no matter >> > whenever the machine is little endian or big endian. The users of this >> > function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer >> > is native endian, not little endian. Most userspace also operates on >> > native endian only. >> > >> > So, go update the comments for all 16+24+32 bpp RGB formats. >> > >> > Leaving the yuv formats as-is. I have no idea if and how those are used >> > on bigendian machines. >> >> I think this is premature. The current situation is that I can't get >> modetest to work *at all* on my NV34 / BE setup (I mean, it runs, just >> the colors displayed are wrong). I believe that currently it packs >> things in "cpu native endian". I've tried futzing with that without >> much success, although I didn't spend too much time on it. I have a >> NV34 plugged into my LE setup as well although I haven't tested to >> double-check that it all works there. However I'm quite sure it used >> to, as I used modetest to help develop the YUV overlay support for >> those GPUs. > > I just took a quick stab at fixing modetest to respect the current > wording in drm_fourcc.h: > > git://github.com/vsyrjala/libdrm.git modetest_endian Looks like there was some careless testing on my part :( So ... it looks like the current modetest without those changes does, in fact, work on NV34/BE. With the changes, it breaks (and the handling of the b* modes is a little broken in those patches -- they're not selectable from the cmdline.) Which means that, as Michel & co predicted, it appears to be taking BE input not LE input. This is very surprising to me, but it is what it is. As I mentioned before, the details of how the "BE" mode works on the GPUs is largely unknown to us beyond a few basics. Note that only XR24 works, AR24 ends up with all black displayed. This also happens on LE. Furthermore, all of YUYV, UYVY, and NV12 plane overlays don't appear to work properly. The first half of the overlay is OK (but I think compressed), while the second half is gibberish. Testing this on my board plugged into a LE CPU, I also get the same type of issue, so I'm guessing that it's just bitrot of the feature. (Or modetest gained a bug.) Cheers, -ilia
Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
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 ... cheers, Gerd
Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
On Fri, Apr 21, 2017 at 10:49:49AM -0400, Ilia Mirkin wrote:
> On Fri, Apr 21, 2017 at 3:58 AM, Gerd Hoffmann wrote:
> > While working on graphics support for virtual machines on ppc64 (which
> > exists in both little and big endian variants) I've figured the comments
> > for various drm fourcc formats in the header file don't match reality.
> >
> > Comments says the RGB formats are little endian, but in practice they
> > are native endian. Look at the drm_mode_legacy_fb_format() helper. It
> > maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB, no matter
> > whenever the machine is little endian or big endian. The users of this
> > function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer
> > is native endian, not little endian. Most userspace also operates on
> > native endian only.
> >
> > So, go update the comments for all 16+24+32 bpp RGB formats.
> >
> > Leaving the yuv formats as-is. I have no idea if and how those are used
> > on bigendian machines.
>
> I think this is premature. The current situation is that I can't get
> modetest to work *at all* on my NV34 / BE setup (I mean, it runs, just
> the colors displayed are wrong). I believe that currently it packs
> things in "cpu native endian". I've tried futzing with that without
> much success, although I didn't spend too much time on it. I have a
> NV34 plugged into my LE setup as well although I haven't tested to
> double-check that it all works there. However I'm quite sure it used
> to, as I used modetest to help develop the YUV overlay support for
> those GPUs.
I just took a quick stab at fixing modetest to respect the current
wording in drm_fourcc.h:
git://github.com/vsyrjala/libdrm.git modetest_endian
>
> Does modetest work for you, under any interpretation of the formats?
>
> There's an additional complication wrt looking at what fbcon does,
> since it will feed the data in via special accel interfaces in fbdev,
> which at least on nouveau, may end up byteswapping the data on upload
> to VRAM (I'm not 100% clear on whether they do or not). However
> modetest, which is creating its own fb, likely won't get such
> treatment.
>
> This is a shitty situation - we have hardware we don't know how it
> works, tools we don't know whether they're broken, and comments we're
> pretty sure are at least somewhat wrong. Furthermore the hardware is
> relatively rare and developers with time to work on improving it are
> even rarer.
>
> I'd like to reiterate that the status quo does end up in a functioning
> system. Let's try not to break that.
>
> Cheers,
>
> -ilia
>
> >
> > Cc: Ville Syrjälä
> > Cc: Daniel Vetter
> > Cc: Pekka Paalanen
> > Cc: Ilia Mirkin
> > Cc: Michel Dänzer
> > Cc: Alex Deucher
> > Cc: amd-...@lists.freedesktop.org
> > Signed-off-by: Gerd Hoffmann
> > ---
> > include/uapi/drm/drm_fourcc.h | 82
> > +--
> > 1 file changed, 41 insertions(+), 41 deletions(-)
> >
> > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > index 995c8f9..1579765 100644
> > --- a/include/uapi/drm/drm_fourcc.h
> > +++ b/include/uapi/drm/drm_fourcc.h
> > @@ -42,68 +42,68 @@ extern "C" {
> > #define DRM_FORMAT_R8 fourcc_code('R', '8', ' ', ' ') /* [7:0] R
> > */
> >
> > /* 16 bpp Red */
> > -#define DRM_FORMAT_R16 fourcc_code('R', '1', '6', ' ') /* [15:0] R
> > little endian */
> > +#define DRM_FORMAT_R16 fourcc_code('R', '1', '6', ' ') /* [15:0] R
> > native endian */
> >
> > /* 16 bpp RG */
> > -#define DRM_FORMAT_RG88fourcc_code('R', 'G', '8', '8') /*
> > [15:0] R:G 8:8 little endian */
> > -#define DRM_FORMAT_GR88fourcc_code('G', 'R', '8', '8') /*
> > [15:0] G:R 8:8 little endian */
> > +#define DRM_FORMAT_RG88fourcc_code('R', 'G', '8', '8') /*
> > [15:0] R:G 8:8 native endian */
> > +#define DRM_FORMAT_GR88fourcc_code('G', 'R', '8', '8') /*
> > [15:0] G:R 8:8 native endian */
> >
> > /* 32 bpp RG */
> > -#define DRM_FORMAT_RG1616 fourcc_code('R', 'G', '3', '2') /* [31:0]
> > R:G 16:16 little endian */
> > -#define DRM_FORMAT_GR1616 fourcc_code('G', 'R', '3', '2') /* [31:0]
> > G:R 16:16 little endian */
> > +#define DRM_FORMAT_RG1616 fourcc_code('R', 'G', '3', '2') /* [31:0]
> > R:G 16:16 native endian */
> > +#define DRM_FORMAT_GR1616 fourcc_code('G', 'R', '3', '2') /* [31:0]
> > G:R 16:16 native endian */
> >
> > /* 8 bpp RGB */
> > #define DRM_FORMAT_RGB332 fourcc_code('R', 'G', 'B', '8') /* [7:0]
> > R:G:B 3:3:2 */
> > #define DRM_FORMAT_BGR233 fourcc_code('B', 'G', 'R', '8') /* [7:0]
> > B:G:R 2:3:3 */
> >
> > /* 16 bpp RGB */
> > -#define DRM_FORMAT_XRGBfourcc_code('X', 'R', '1', '2') /* [15:0]
> > x:R:G:B 4:4:4:4 little endian */
> > -#define DRM_FORMAT_XBGRfourcc_code('X', 'B', '1', '2') /* [15:0]
> > x:B:G:R 4:4:4:4 little endian */
> > -#define DRM_FORMAT_RGBXfourcc_cod
Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
On Fri, Apr 21, 2017 at 3:58 AM, Gerd Hoffmann wrote:
> While working on graphics support for virtual machines on ppc64 (which
> exists in both little and big endian variants) I've figured the comments
> for various drm fourcc formats in the header file don't match reality.
>
> Comments says the RGB formats are little endian, but in practice they
> are native endian. Look at the drm_mode_legacy_fb_format() helper. It
> maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB, no matter
> whenever the machine is little endian or big endian. The users of this
> function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer
> is native endian, not little endian. Most userspace also operates on
> native endian only.
>
> So, go update the comments for all 16+24+32 bpp RGB formats.
>
> Leaving the yuv formats as-is. I have no idea if and how those are used
> on bigendian machines.
I think this is premature. The current situation is that I can't get
modetest to work *at all* on my NV34 / BE setup (I mean, it runs, just
the colors displayed are wrong). I believe that currently it packs
things in "cpu native endian". I've tried futzing with that without
much success, although I didn't spend too much time on it. I have a
NV34 plugged into my LE setup as well although I haven't tested to
double-check that it all works there. However I'm quite sure it used
to, as I used modetest to help develop the YUV overlay support for
those GPUs.
Does modetest work for you, under any interpretation of the formats?
There's an additional complication wrt looking at what fbcon does,
since it will feed the data in via special accel interfaces in fbdev,
which at least on nouveau, may end up byteswapping the data on upload
to VRAM (I'm not 100% clear on whether they do or not). However
modetest, which is creating its own fb, likely won't get such
treatment.
This is a shitty situation - we have hardware we don't know how it
works, tools we don't know whether they're broken, and comments we're
pretty sure are at least somewhat wrong. Furthermore the hardware is
relatively rare and developers with time to work on improving it are
even rarer.
I'd like to reiterate that the status quo does end up in a functioning
system. Let's try not to break that.
Cheers,
-ilia
>
> Cc: Ville Syrjälä
> Cc: Daniel Vetter
> Cc: Pekka Paalanen
> Cc: Ilia Mirkin
> Cc: Michel Dänzer
> Cc: Alex Deucher
> Cc: amd-...@lists.freedesktop.org
> Signed-off-by: Gerd Hoffmann
> ---
> include/uapi/drm/drm_fourcc.h | 82
> +--
> 1 file changed, 41 insertions(+), 41 deletions(-)
>
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 995c8f9..1579765 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -42,68 +42,68 @@ extern "C" {
> #define DRM_FORMAT_R8 fourcc_code('R', '8', ' ', ' ') /* [7:0] R */
>
> /* 16 bpp Red */
> -#define DRM_FORMAT_R16 fourcc_code('R', '1', '6', ' ') /* [15:0] R
> little endian */
> +#define DRM_FORMAT_R16 fourcc_code('R', '1', '6', ' ') /* [15:0] R
> native endian */
>
> /* 16 bpp RG */
> -#define DRM_FORMAT_RG88fourcc_code('R', 'G', '8', '8') /*
> [15:0] R:G 8:8 little endian */
> -#define DRM_FORMAT_GR88fourcc_code('G', 'R', '8', '8') /*
> [15:0] G:R 8:8 little endian */
> +#define DRM_FORMAT_RG88fourcc_code('R', 'G', '8', '8') /*
> [15:0] R:G 8:8 native endian */
> +#define DRM_FORMAT_GR88fourcc_code('G', 'R', '8', '8') /*
> [15:0] G:R 8:8 native endian */
>
> /* 32 bpp RG */
> -#define DRM_FORMAT_RG1616 fourcc_code('R', 'G', '3', '2') /* [31:0] R:G
> 16:16 little endian */
> -#define DRM_FORMAT_GR1616 fourcc_code('G', 'R', '3', '2') /* [31:0] G:R
> 16:16 little endian */
> +#define DRM_FORMAT_RG1616 fourcc_code('R', 'G', '3', '2') /* [31:0] R:G
> 16:16 native endian */
> +#define DRM_FORMAT_GR1616 fourcc_code('G', 'R', '3', '2') /* [31:0] G:R
> 16:16 native endian */
>
> /* 8 bpp RGB */
> #define DRM_FORMAT_RGB332 fourcc_code('R', 'G', 'B', '8') /* [7:0]
> R:G:B 3:3:2 */
> #define DRM_FORMAT_BGR233 fourcc_code('B', 'G', 'R', '8') /* [7:0]
> B:G:R 2:3:3 */
>
> /* 16 bpp RGB */
> -#define DRM_FORMAT_XRGBfourcc_code('X', 'R', '1', '2') /* [15:0]
> x:R:G:B 4:4:4:4 little endian */
> -#define DRM_FORMAT_XBGRfourcc_code('X', 'B', '1', '2') /* [15:0]
> x:B:G:R 4:4:4:4 little endian */
> -#define DRM_FORMAT_RGBXfourcc_code('R', 'X', '1', '2') /* [15:0]
> R:G:B:x 4:4:4:4 little endian */
> -#define DRM_FORMAT_BGRXfourcc_code('B', 'X', '1', '2') /* [15:0]
> B:G:R:x 4:4:4:4 little endian */
> +#define DRM_FORMAT_XRGBfourcc_code('X', 'R', '1', '2') /* [15:0]
> x:R:G:B 4:4:4:4 native endian */
> +#define DRM_FORMAT_XBGRfourcc_code('X', 'B', '1', '2') /* [15:0]
> x:B:G:R 4:4:4:4 native endian */
> +#define DRM_FORMAT_RGBX44
Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
Thanks, Christian for adding me. On 2017-04-21 09:27 AM, Christian König wrote: Adding Harry to this mail thread as well, cause is one of the people really affected by this. Christian. Am 21.04.2017 um 15:21 schrieb Christian König: Am 21.04.2017 um 15:12 schrieb Gerd Hoffmann: Hi, "native" to me feels more like "native to the GPU" since these things really are tied to the GPU not the CPU. That's also why I went with the explicit endianness originally so that the driver could properly declare what the GPU supports. And to be honest I would really prefer to stick with that approach for exactly that reason. I strongly agree with Christian and Ville on this. I understand fourcc endianness as GPU endianness. Usermode needs to be clear on the framebuffer format, whether it's GPU or CPU rendered, so kernel should define this clearly. In practice it probably doesn't currently make much difference for AMD GPUs. I've never heard of anyone using them on big-endian systems. Harry The proposed change would require that drivers have different code path for different CPU byte order. Those code path tend to be not tested very well and are additional complexity we probably don't want inside the driver. We can add fixed-endian #defines without too much effort, at least for the 8 bits per color formats. In qemu we have the same problem, only with pixman. Those formats are native endian too, but often we have to handle a fixed format, so we did this: /* * pixman image formats are defined to be native endian, * that means host byte order on qemu. So we go define * fixed formats here for cases where it is needed, like * feeding libjpeg / libpng and writing screenshots. */ #ifdef HOST_WORDS_BIGENDIAN # define PIXMAN_BE_r8g8b8 PIXMAN_r8g8b8 # define PIXMAN_BE_x8r8g8b8 PIXMAN_x8r8g8b8 # define PIXMAN_BE_a8r8g8b8 PIXMAN_a8r8g8b8 # define PIXMAN_BE_b8g8r8x8 PIXMAN_b8g8r8x8 # define PIXMAN_BE_b8g8r8a8 PIXMAN_b8g8r8a8 # define PIXMAN_BE_r8g8b8x8 PIXMAN_r8g8b8x8 # define PIXMAN_BE_r8g8b8a8 PIXMAN_r8g8b8a8 # define PIXMAN_BE_x8b8g8r8 PIXMAN_x8b8g8r8 # define PIXMAN_BE_a8b8g8r8 PIXMAN_a8b8g8r8 # define PIXMAN_LE_x8r8g8b8 PIXMAN_b8g8r8x8 #else # define PIXMAN_BE_r8g8b8 PIXMAN_b8g8r8 # define PIXMAN_BE_x8r8g8b8 PIXMAN_b8g8r8x8 # define PIXMAN_BE_a8r8g8b8 PIXMAN_b8g8r8a8 # define PIXMAN_BE_b8g8r8x8 PIXMAN_x8r8g8b8 # define PIXMAN_BE_b8g8r8a8 PIXMAN_a8r8g8b8 # define PIXMAN_BE_r8g8b8x8 PIXMAN_x8b8g8r8 # define PIXMAN_BE_r8g8b8a8 PIXMAN_a8b8g8r8 # define PIXMAN_BE_x8b8g8r8 PIXMAN_r8g8b8x8 # define PIXMAN_BE_a8b8g8r8 PIXMAN_r8g8b8a8 # define PIXMAN_LE_x8r8g8b8 PIXMAN_x8r8g8b8 #endif Exactly what Mesa did as well. 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 ... I know and that's exactly the reason I'm going to object those changes. The kernel/userspace abi is fixed and changing it like this could potentially break drivers I'm the co-maintainer of. So that whole approach is a clear NAK from my side. If you find a driver or userspace which doesn't use the formats as defined in the comments of drm_fourcc.h the fix the driver instead of trying to adjust the common header to broken behavior. Cause the later will clearly cause problems with drivers who correctly implemented the interface. Regards, Christian. cheers, Gerd ___ dri-devel mailing list dri-de...@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ___ amd-gfx mailing list amd-...@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
Adding Harry to this mail thread as well, cause is one of the people really affected by this. Christian. Am 21.04.2017 um 15:21 schrieb Christian König: Am 21.04.2017 um 15:12 schrieb Gerd Hoffmann: Hi, "native" to me feels more like "native to the GPU" since these things really are tied to the GPU not the CPU. That's also why I went with the explicit endianness originally so that the driver could properly declare what the GPU supports. And to be honest I would really prefer to stick with that approach for exactly that reason. The proposed change would require that drivers have different code path for different CPU byte order. Those code path tend to be not tested very well and are additional complexity we probably don't want inside the driver. We can add fixed-endian #defines without too much effort, at least for the 8 bits per color formats. In qemu we have the same problem, only with pixman. Those formats are native endian too, but often we have to handle a fixed format, so we did this: /* * pixman image formats are defined to be native endian, * that means host byte order on qemu. So we go define * fixed formats here for cases where it is needed, like * feeding libjpeg / libpng and writing screenshots. */ #ifdef HOST_WORDS_BIGENDIAN # define PIXMAN_BE_r8g8b8 PIXMAN_r8g8b8 # define PIXMAN_BE_x8r8g8b8 PIXMAN_x8r8g8b8 # define PIXMAN_BE_a8r8g8b8 PIXMAN_a8r8g8b8 # define PIXMAN_BE_b8g8r8x8 PIXMAN_b8g8r8x8 # define PIXMAN_BE_b8g8r8a8 PIXMAN_b8g8r8a8 # define PIXMAN_BE_r8g8b8x8 PIXMAN_r8g8b8x8 # define PIXMAN_BE_r8g8b8a8 PIXMAN_r8g8b8a8 # define PIXMAN_BE_x8b8g8r8 PIXMAN_x8b8g8r8 # define PIXMAN_BE_a8b8g8r8 PIXMAN_a8b8g8r8 # define PIXMAN_LE_x8r8g8b8 PIXMAN_b8g8r8x8 #else # define PIXMAN_BE_r8g8b8 PIXMAN_b8g8r8 # define PIXMAN_BE_x8r8g8b8 PIXMAN_b8g8r8x8 # define PIXMAN_BE_a8r8g8b8 PIXMAN_b8g8r8a8 # define PIXMAN_BE_b8g8r8x8 PIXMAN_x8r8g8b8 # define PIXMAN_BE_b8g8r8a8 PIXMAN_a8r8g8b8 # define PIXMAN_BE_r8g8b8x8 PIXMAN_x8b8g8r8 # define PIXMAN_BE_r8g8b8a8 PIXMAN_a8b8g8r8 # define PIXMAN_BE_x8b8g8r8 PIXMAN_r8g8b8x8 # define PIXMAN_BE_a8b8g8r8 PIXMAN_r8g8b8a8 # define PIXMAN_LE_x8r8g8b8 PIXMAN_x8r8g8b8 #endif Exactly what Mesa did as well. 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 ... I know and that's exactly the reason I'm going to object those changes. The kernel/userspace abi is fixed and changing it like this could potentially break drivers I'm the co-maintainer of. So that whole approach is a clear NAK from my side. If you find a driver or userspace which doesn't use the formats as defined in the comments of drm_fourcc.h the fix the driver instead of trying to adjust the common header to broken behavior. Cause the later will clearly cause problems with drivers who correctly implemented the interface. Regards, Christian. cheers, Gerd ___ dri-devel mailing list dri-de...@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ___ amd-gfx mailing list amd-...@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
Am 21.04.2017 um 15:12 schrieb Gerd Hoffmann: Hi, "native" to me feels more like "native to the GPU" since these things really are tied to the GPU not the CPU. That's also why I went with the explicit endianness originally so that the driver could properly declare what the GPU supports. And to be honest I would really prefer to stick with that approach for exactly that reason. The proposed change would require that drivers have different code path for different CPU byte order. Those code path tend to be not tested very well and are additional complexity we probably don't want inside the driver. We can add fixed-endian #defines without too much effort, at least for the 8 bits per color formats. In qemu we have the same problem, only with pixman. Those formats are native endian too, but often we have to handle a fixed format, so we did this: /* * pixman image formats are defined to be native endian, * that means host byte order on qemu. So we go define * fixed formats here for cases where it is needed, like * feeding libjpeg / libpng and writing screenshots. */ #ifdef HOST_WORDS_BIGENDIAN # define PIXMAN_BE_r8g8b8 PIXMAN_r8g8b8 # define PIXMAN_BE_x8r8g8b8 PIXMAN_x8r8g8b8 # define PIXMAN_BE_a8r8g8b8 PIXMAN_a8r8g8b8 # define PIXMAN_BE_b8g8r8x8 PIXMAN_b8g8r8x8 # define PIXMAN_BE_b8g8r8a8 PIXMAN_b8g8r8a8 # define PIXMAN_BE_r8g8b8x8 PIXMAN_r8g8b8x8 # define PIXMAN_BE_r8g8b8a8 PIXMAN_r8g8b8a8 # define PIXMAN_BE_x8b8g8r8 PIXMAN_x8b8g8r8 # define PIXMAN_BE_a8b8g8r8 PIXMAN_a8b8g8r8 # define PIXMAN_LE_x8r8g8b8 PIXMAN_b8g8r8x8 #else # define PIXMAN_BE_r8g8b8 PIXMAN_b8g8r8 # define PIXMAN_BE_x8r8g8b8 PIXMAN_b8g8r8x8 # define PIXMAN_BE_a8r8g8b8 PIXMAN_b8g8r8a8 # define PIXMAN_BE_b8g8r8x8 PIXMAN_x8r8g8b8 # define PIXMAN_BE_b8g8r8a8 PIXMAN_a8r8g8b8 # define PIXMAN_BE_r8g8b8x8 PIXMAN_x8b8g8r8 # define PIXMAN_BE_r8g8b8a8 PIXMAN_a8b8g8r8 # define PIXMAN_BE_x8b8g8r8 PIXMAN_r8g8b8x8 # define PIXMAN_BE_a8b8g8r8 PIXMAN_r8g8b8a8 # define PIXMAN_LE_x8r8g8b8 PIXMAN_x8r8g8b8 #endif Exactly what Mesa did as well. 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 ... I know and that's exactly the reason I'm going to object those changes. The kernel/userspace abi is fixed and changing it like this could potentially break drivers I'm the co-maintainer of. So that whole approach is a clear NAK from my side. If you find a driver or userspace which doesn't use the formats as defined in the comments of drm_fourcc.h the fix the driver instead of trying to adjust the common header to broken behavior. Cause the later will clearly cause problems with drivers who correctly implemented the interface. Regards, Christian. cheers, Gerd ___ dri-devel mailing list dri-de...@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
Hi, > > "native" to me feels more like "native to the GPU" since these things > > really are tied to the GPU not the CPU. That's also why I went with the > > explicit endianness originally so that the driver could properly declare > > what the GPU supports. > And to be honest I would really prefer to stick with that approach for > exactly that reason. > > The proposed change would require that drivers have different code path > for different CPU byte order. Those code path tend to be not tested very > well and are additional complexity we probably don't want inside the driver. We can add fixed-endian #defines without too much effort, at least for the 8 bits per color formats. In qemu we have the same problem, only with pixman. Those formats are native endian too, but often we have to handle a fixed format, so we did this: /* * pixman image formats are defined to be native endian, * that means host byte order on qemu. So we go define * fixed formats here for cases where it is needed, like * feeding libjpeg / libpng and writing screenshots. */ #ifdef HOST_WORDS_BIGENDIAN # define PIXMAN_BE_r8g8b8 PIXMAN_r8g8b8 # define PIXMAN_BE_x8r8g8b8 PIXMAN_x8r8g8b8 # define PIXMAN_BE_a8r8g8b8 PIXMAN_a8r8g8b8 # define PIXMAN_BE_b8g8r8x8 PIXMAN_b8g8r8x8 # define PIXMAN_BE_b8g8r8a8 PIXMAN_b8g8r8a8 # define PIXMAN_BE_r8g8b8x8 PIXMAN_r8g8b8x8 # define PIXMAN_BE_r8g8b8a8 PIXMAN_r8g8b8a8 # define PIXMAN_BE_x8b8g8r8 PIXMAN_x8b8g8r8 # define PIXMAN_BE_a8b8g8r8 PIXMAN_a8b8g8r8 # define PIXMAN_LE_x8r8g8b8 PIXMAN_b8g8r8x8 #else # define PIXMAN_BE_r8g8b8 PIXMAN_b8g8r8 # define PIXMAN_BE_x8r8g8b8 PIXMAN_b8g8r8x8 # define PIXMAN_BE_a8r8g8b8 PIXMAN_b8g8r8a8 # define PIXMAN_BE_b8g8r8x8 PIXMAN_x8r8g8b8 # define PIXMAN_BE_b8g8r8a8 PIXMAN_a8r8g8b8 # define PIXMAN_BE_r8g8b8x8 PIXMAN_x8b8g8r8 # define PIXMAN_BE_r8g8b8a8 PIXMAN_a8b8g8r8 # define PIXMAN_BE_x8b8g8r8 PIXMAN_r8g8b8x8 # define PIXMAN_BE_a8b8g8r8 PIXMAN_r8g8b8a8 # define PIXMAN_LE_x8r8g8b8 PIXMAN_x8r8g8b8 #endif > 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 ... cheers, Gerd
Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
Am 21.04.2017 um 13:49 schrieb Ville Syrjälä: On Fri, Apr 21, 2017 at 02:40:18PM +0300, Pekka Paalanen wrote: On Fri, 21 Apr 2017 14:08:04 +0300 Ville Syrjälä wrote: On Fri, Apr 21, 2017 at 11:50:18AM +0200, Gerd Hoffmann wrote: On Fr, 2017-04-21 at 12:25 +0300, Ville Syrjälä wrote: On Fri, Apr 21, 2017 at 09:58:24AM +0200, Gerd Hoffmann wrote: While working on graphics support for virtual machines on ppc64 (which exists in both little and big endian variants) I've figured the comments for various drm fourcc formats in the header file don't match reality. Comments says the RGB formats are little endian, but in practice they are native endian. Look at the drm_mode_legacy_fb_format() helper. It maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB, no matter whenever the machine is little endian or big endian. The users of this function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer is native endian, not little endian. Most userspace also operates on native endian only. I'm not a fan of "native". Native to what? "CPU" or "host" is what I'd call it. native == whatever the cpu is using. I personally find "native" more intuitive, but at the end of the day I don't mind much. If people prefer "host" over "native" I'll change it. "native" to me feels more like "native to the GPU" since these things really are tied to the GPU not the CPU. That's also why I went with the explicit endianness originally so that the driver could properly declare what the GPU supports. Hi, yeah, one should really be explicit on which component's endianess does "native" refer to. I just can't imagine "GPU native" to ever be an option, because then userspace needs a way to discover what the GPU endianess is, It has to know that. How else would it know how to write the bytes into memory in the right order for the GPU to consume, or read the stuff the GPU produced? and I believe that would only deepen the swamp, not drain it, because suddenly you need two enums to describe one format. Ville, wording aside, what do think about changing the endianess definition? Is it going in the right direction? I don't think so, but I guess I'm in the minority. I don't think your are in the minority. At least I would clearly say those formats should be in a fixed byte order and don't care about the CPU in the system. What I need from the driver side is a consistent description of how the bytes in memory map to my hardware. What CPU is in use in the system is completely irrelevant for that. Regards, Christian.
Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
On Fri, Apr 21, 2017 at 02:40:18PM +0300, Pekka Paalanen wrote: > On Fri, 21 Apr 2017 14:08:04 +0300 > Ville Syrjälä wrote: > > > On Fri, Apr 21, 2017 at 11:50:18AM +0200, Gerd Hoffmann wrote: > > > On Fr, 2017-04-21 at 12:25 +0300, Ville Syrjälä wrote: > > > > On Fri, Apr 21, 2017 at 09:58:24AM +0200, Gerd Hoffmann wrote: > > > > > While working on graphics support for virtual machines on ppc64 (which > > > > > exists in both little and big endian variants) I've figured the > > > > > comments > > > > > for various drm fourcc formats in the header file don't match reality. > > > > > > > > > > Comments says the RGB formats are little endian, but in practice they > > > > > are native endian. Look at the drm_mode_legacy_fb_format() helper. > > > > > It > > > > > maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB, no > > > > > matter > > > > > whenever the machine is little endian or big endian. The users of > > > > > this > > > > > function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the > > > > > framebuffer > > > > > is native endian, not little endian. Most userspace also operates on > > > > > native endian only. > > > > > > > > I'm not a fan of "native". Native to what? "CPU" or "host" is what I'd > > > > call it. > > > > > > native == whatever the cpu is using. > > > > > > I personally find "native" more intuitive, but at the end of the day I > > > don't mind much. If people prefer "host" over "native" I'll change it. > > > > "native" to me feels more like "native to the GPU" since these things > > really are tied to the GPU not the CPU. That's also why I went with the > > explicit endianness originally so that the driver could properly declare > > what the GPU supports. > > Hi, > > yeah, one should really be explicit on which component's endianess does > "native" refer to. I just can't imagine "GPU native" to ever be an > option, because then userspace needs a way to discover what the > GPU endianess is, It has to know that. How else would it know how to write the bytes into memory in the right order for the GPU to consume, or read the stuff the GPU produced? > and I believe that would only deepen the swamp, not > drain it, because suddenly you need two enums to describe one format. > > Ville, wording aside, what do think about changing the endianess > definition? Is it going in the right direction? I don't think so, but I guess I'm in the minority. -- Ville Syrjälä Intel OTC
Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
Am 21.04.2017 um 13:08 schrieb Ville Syrjälä: On Fri, Apr 21, 2017 at 11:50:18AM +0200, Gerd Hoffmann wrote: On Fr, 2017-04-21 at 12:25 +0300, Ville Syrjälä wrote: On Fri, Apr 21, 2017 at 09:58:24AM +0200, Gerd Hoffmann wrote: While working on graphics support for virtual machines on ppc64 (which exists in both little and big endian variants) I've figured the comments for various drm fourcc formats in the header file don't match reality. Comments says the RGB formats are little endian, but in practice they are native endian. Look at the drm_mode_legacy_fb_format() helper. It maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB, no matter whenever the machine is little endian or big endian. The users of this function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer is native endian, not little endian. Most userspace also operates on native endian only. I'm not a fan of "native". Native to what? "CPU" or "host" is what I'd call it. native == whatever the cpu is using. I personally find "native" more intuitive, but at the end of the day I don't mind much. If people prefer "host" over "native" I'll change it. "native" to me feels more like "native to the GPU" since these things really are tied to the GPU not the CPU. That's also why I went with the explicit endianness originally so that the driver could properly declare what the GPU supports. And to be honest I would really prefer to stick with that approach for exactly that reason. The proposed change would require that drivers have different code path for different CPU byte order. Those code path tend to be not tested very well and are additional complexity we probably don't want inside the driver. 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. Regards, Christian.
Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
Hi, > > I personally find "native" more intuitive, but at the end of the day I > > don't mind much. If people prefer "host" over "native" I'll change it. > > "native" to me feels more like "native to the GPU" since these things > really are tied to the GPU not the CPU. Ok, then maybe "host" is the better term then, to make clear we talk about cpu/kernel/userspace byteorder here. cheers, Gerd
Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
On Fri, 21 Apr 2017 14:08:04 +0300 Ville Syrjälä wrote: > On Fri, Apr 21, 2017 at 11:50:18AM +0200, Gerd Hoffmann wrote: > > On Fr, 2017-04-21 at 12:25 +0300, Ville Syrjälä wrote: > > > On Fri, Apr 21, 2017 at 09:58:24AM +0200, Gerd Hoffmann wrote: > > > > While working on graphics support for virtual machines on ppc64 (which > > > > exists in both little and big endian variants) I've figured the comments > > > > for various drm fourcc formats in the header file don't match reality. > > > > > > > > Comments says the RGB formats are little endian, but in practice they > > > > are native endian. Look at the drm_mode_legacy_fb_format() helper. It > > > > maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB, no matter > > > > whenever the machine is little endian or big endian. The users of this > > > > function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer > > > > is native endian, not little endian. Most userspace also operates on > > > > native endian only. > > > > > > I'm not a fan of "native". Native to what? "CPU" or "host" is what I'd > > > call it. > > > > native == whatever the cpu is using. > > > > I personally find "native" more intuitive, but at the end of the day I > > don't mind much. If people prefer "host" over "native" I'll change it. > > "native" to me feels more like "native to the GPU" since these things > really are tied to the GPU not the CPU. That's also why I went with the > explicit endianness originally so that the driver could properly declare > what the GPU supports. Hi, yeah, one should really be explicit on which component's endianess does "native" refer to. I just can't imagine "GPU native" to ever be an option, because then userspace needs a way to discover what the GPU endianess is, and I believe that would only deepen the swamp, not drain it, because suddenly you need two enums to describe one format. Ville, wording aside, what do think about changing the endianess definition? Is it going in the right direction? Thanks, pq pgpgh22lIHfTL.pgp Description: OpenPGP digital signature
Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
On Fri, Apr 21, 2017 at 11:50:18AM +0200, Gerd Hoffmann wrote: > On Fr, 2017-04-21 at 12:25 +0300, Ville Syrjälä wrote: > > On Fri, Apr 21, 2017 at 09:58:24AM +0200, Gerd Hoffmann wrote: > > > While working on graphics support for virtual machines on ppc64 (which > > > exists in both little and big endian variants) I've figured the comments > > > for various drm fourcc formats in the header file don't match reality. > > > > > > Comments says the RGB formats are little endian, but in practice they > > > are native endian. Look at the drm_mode_legacy_fb_format() helper. It > > > maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB, no matter > > > whenever the machine is little endian or big endian. The users of this > > > function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer > > > is native endian, not little endian. Most userspace also operates on > > > native endian only. > > > > I'm not a fan of "native". Native to what? "CPU" or "host" is what I'd > > call it. > > native == whatever the cpu is using. > > I personally find "native" more intuitive, but at the end of the day I > don't mind much. If people prefer "host" over "native" I'll change it. "native" to me feels more like "native to the GPU" since these things really are tied to the GPU not the CPU. That's also why I went with the explicit endianness originally so that the driver could properly declare what the GPU supports. -- Ville Syrjälä Intel OTC
Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
On Fri, Apr 21, 2017 at 11:50:18AM +0200, Gerd Hoffmann wrote: > On Fr, 2017-04-21 at 12:25 +0300, Ville Syrjälä wrote: > > On Fri, Apr 21, 2017 at 09:58:24AM +0200, Gerd Hoffmann wrote: > > > While working on graphics support for virtual machines on ppc64 (which > > > exists in both little and big endian variants) I've figured the comments > > > for various drm fourcc formats in the header file don't match reality. > > > > > > Comments says the RGB formats are little endian, but in practice they > > > are native endian. Look at the drm_mode_legacy_fb_format() helper. It > > > maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB, no matter > > > whenever the machine is little endian or big endian. The users of this > > > function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer > > > is native endian, not little endian. Most userspace also operates on > > > native endian only. > > > > I'm not a fan of "native". Native to what? "CPU" or "host" is what I'd > > call it. > > native == whatever the cpu is using. > > I personally find "native" more intuitive, but at the end of the day I > don't mind much. If people prefer "host" over "native" I'll change it. > > > And what about the mxied endian case? Are you just going to pretend it > > doesn't exist or what? > > What exactly do you mean with "mixed endian"? The powerpc case, where > kernel + userspace can run in either big or little endian mode? Or > something else? Big endian CPU and little endian GPU. I think that should be the most common case these days. -- Ville Syrjälä Intel OTC
Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
On Fr, 2017-04-21 at 12:25 +0300, Ville Syrjälä wrote: > On Fri, Apr 21, 2017 at 09:58:24AM +0200, Gerd Hoffmann wrote: > > While working on graphics support for virtual machines on ppc64 (which > > exists in both little and big endian variants) I've figured the comments > > for various drm fourcc formats in the header file don't match reality. > > > > Comments says the RGB formats are little endian, but in practice they > > are native endian. Look at the drm_mode_legacy_fb_format() helper. It > > maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB, no matter > > whenever the machine is little endian or big endian. The users of this > > function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer > > is native endian, not little endian. Most userspace also operates on > > native endian only. > > I'm not a fan of "native". Native to what? "CPU" or "host" is what I'd > call it. native == whatever the cpu is using. I personally find "native" more intuitive, but at the end of the day I don't mind much. If people prefer "host" over "native" I'll change it. > And what about the mxied endian case? Are you just going to pretend it > doesn't exist or what? What exactly do you mean with "mixed endian"? The powerpc case, where kernel + userspace can run in either big or little endian mode? Or something else? cheers, Gerd
Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
On Fri, Apr 21, 2017 at 11:38:28AM +0200, Gerd Hoffmann wrote: > Hi, > > > > Leaving the yuv formats as-is. I have no idea if and how those are used > > > on bigendian machines. > > > just an idea - since we are not sure how the remaining formats are being > > used, should those be marked somehow uncertain whether they are little > > or native endian? > > ATM the yuv don't have any byte order annotations, and I simply left > them that way. So it is as clear/unclear as before. Eh? Everything that is affected by byte order has the relevant comments. If they don't, then that's a bug. > > IIRC someone mentioned that for the yuv fourccs there actually is some > standard about the exact ordering. Anyone has a good reference? We > could stick a link to it into a comment. The "standard" is fourcc. Whether there is any official reference for that is unclear. That's exactly why I added the explicit comments into drm_fourcc.h so that people don't have to go trawling the internets looking for information on what each pixel format might mean. -- Ville Syrjälä Intel OTC
Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
Hi, > > Leaving the yuv formats as-is. I have no idea if and how those are used > > on bigendian machines. > just an idea - since we are not sure how the remaining formats are being > used, should those be marked somehow uncertain whether they are little > or native endian? ATM the yuv don't have any byte order annotations, and I simply left them that way. So it is as clear/unclear as before. IIRC someone mentioned that for the yuv fourccs there actually is some standard about the exact ordering. Anyone has a good reference? We could stick a link to it into a comment. cheers, Gerd
Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
On Fri, Apr 21, 2017 at 09:58:24AM +0200, Gerd Hoffmann wrote:
> While working on graphics support for virtual machines on ppc64 (which
> exists in both little and big endian variants) I've figured the comments
> for various drm fourcc formats in the header file don't match reality.
>
> Comments says the RGB formats are little endian, but in practice they
> are native endian. Look at the drm_mode_legacy_fb_format() helper. It
> maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB, no matter
> whenever the machine is little endian or big endian. The users of this
> function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer
> is native endian, not little endian. Most userspace also operates on
> native endian only.
I'm not a fan of "native". Native to what? "CPU" or "host" is what I'd
call it.
And what about the mxied endian case? Are you just going to pretend it
doesn't exist or what?
>
> So, go update the comments for all 16+24+32 bpp RGB formats.
>
> Leaving the yuv formats as-is. I have no idea if and how those are used
> on bigendian machines.
>
> Cc: Ville Syrjälä
> Cc: Daniel Vetter
> Cc: Pekka Paalanen
> Cc: Ilia Mirkin
> Cc: Michel Dänzer
> Cc: Alex Deucher
> Cc: amd-...@lists.freedesktop.org
> Signed-off-by: Gerd Hoffmann
> ---
> include/uapi/drm/drm_fourcc.h | 82
> +--
> 1 file changed, 41 insertions(+), 41 deletions(-)
>
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 995c8f9..1579765 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -42,68 +42,68 @@ extern "C" {
> #define DRM_FORMAT_R8fourcc_code('R', '8', ' ', ' ') /*
> [7:0] R */
>
> /* 16 bpp Red */
> -#define DRM_FORMAT_R16 fourcc_code('R', '1', '6', ' ') /*
> [15:0] R little endian */
> +#define DRM_FORMAT_R16 fourcc_code('R', '1', '6', ' ') /*
> [15:0] R native endian */
>
> /* 16 bpp RG */
> -#define DRM_FORMAT_RG88 fourcc_code('R', 'G', '8', '8') /*
> [15:0] R:G 8:8 little endian */
> -#define DRM_FORMAT_GR88 fourcc_code('G', 'R', '8', '8') /*
> [15:0] G:R 8:8 little endian */
> +#define DRM_FORMAT_RG88 fourcc_code('R', 'G', '8', '8') /*
> [15:0] R:G 8:8 native endian */
> +#define DRM_FORMAT_GR88 fourcc_code('G', 'R', '8', '8') /*
> [15:0] G:R 8:8 native endian */
>
> /* 32 bpp RG */
> -#define DRM_FORMAT_RG1616fourcc_code('R', 'G', '3', '2') /* [31:0] R:G
> 16:16 little endian */
> -#define DRM_FORMAT_GR1616fourcc_code('G', 'R', '3', '2') /* [31:0] G:R
> 16:16 little endian */
> +#define DRM_FORMAT_RG1616fourcc_code('R', 'G', '3', '2') /* [31:0] R:G
> 16:16 native endian */
> +#define DRM_FORMAT_GR1616fourcc_code('G', 'R', '3', '2') /* [31:0] G:R
> 16:16 native endian */
>
> /* 8 bpp RGB */
> #define DRM_FORMAT_RGB332fourcc_code('R', 'G', 'B', '8') /* [7:0] R:G:B
> 3:3:2 */
> #define DRM_FORMAT_BGR233fourcc_code('B', 'G', 'R', '8') /* [7:0] B:G:R
> 2:3:3 */
>
> /* 16 bpp RGB */
> -#define DRM_FORMAT_XRGB fourcc_code('X', 'R', '1', '2') /* [15:0]
> x:R:G:B 4:4:4:4 little endian */
> -#define DRM_FORMAT_XBGR fourcc_code('X', 'B', '1', '2') /* [15:0]
> x:B:G:R 4:4:4:4 little endian */
> -#define DRM_FORMAT_RGBX fourcc_code('R', 'X', '1', '2') /* [15:0]
> R:G:B:x 4:4:4:4 little endian */
> -#define DRM_FORMAT_BGRX fourcc_code('B', 'X', '1', '2') /* [15:0]
> B:G:R:x 4:4:4:4 little endian */
> +#define DRM_FORMAT_XRGB fourcc_code('X', 'R', '1', '2') /* [15:0]
> x:R:G:B 4:4:4:4 native endian */
> +#define DRM_FORMAT_XBGR fourcc_code('X', 'B', '1', '2') /* [15:0]
> x:B:G:R 4:4:4:4 native endian */
> +#define DRM_FORMAT_RGBX fourcc_code('R', 'X', '1', '2') /* [15:0]
> R:G:B:x 4:4:4:4 native endian */
> +#define DRM_FORMAT_BGRX fourcc_code('B', 'X', '1', '2') /* [15:0]
> B:G:R:x 4:4:4:4 native endian */
>
> -#define DRM_FORMAT_ARGB fourcc_code('A', 'R', '1', '2') /* [15:0]
> A:R:G:B 4:4:4:4 little endian */
> -#define DRM_FORMAT_ABGR fourcc_code('A', 'B', '1', '2') /* [15:0]
> A:B:G:R 4:4:4:4 little endian */
> -#define DRM_FORMAT_RGBA fourcc_code('R', 'A', '1', '2') /* [15:0]
> R:G:B:A 4:4:4:4 little endian */
> -#define DRM_FORMAT_BGRA fourcc_code('B', 'A', '1', '2') /* [15:0]
> B:G:R:A 4:4:4:4 little endian */
> +#define DRM_FORMAT_ARGB fourcc_code('A', 'R', '1', '2') /* [15:0]
> A:R:G:B 4:4:4:4 native endian */
> +#define DRM_FORMAT_ABGR fourcc_code('A', 'B', '1', '2') /* [15:0]
> A:B:G:R 4:4:4:4 native endian */
> +#define DRM_FORMAT_RGBA fourcc_code('R', 'A', '1', '2') /* [15:0]
> R:G:B:A 4:4:4:4 native endian */
> +#define DRM_FORMAT_BGRA fourcc_code('B', 'A', '1', '2') /* [15:0]
> B:G:R:A 4:4:4:4 native endian */
>
> -#define DRM_FORMAT_XRGB1555 fourcc_code('X', 'R', '1', '5') /* [15:0]
> x:R:G:B 1:5:5:5 little endian */
> -#define
Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.
On Fri, 21 Apr 2017 09:58:24 +0200 Gerd Hoffmann wrote: > While working on graphics support for virtual machines on ppc64 (which > exists in both little and big endian variants) I've figured the comments > for various drm fourcc formats in the header file don't match reality. > > Comments says the RGB formats are little endian, but in practice they > are native endian. Look at the drm_mode_legacy_fb_format() helper. It > maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB, no matter > whenever the machine is little endian or big endian. The users of this > function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer > is native endian, not little endian. Most userspace also operates on > native endian only. > > So, go update the comments for all 16+24+32 bpp RGB formats. > > Leaving the yuv formats as-is. I have no idea if and how those are used > on bigendian machines. > > Cc: Ville Syrjälä > Cc: Daniel Vetter > Cc: Pekka Paalanen > Cc: Ilia Mirkin > Cc: Michel Dänzer > Cc: Alex Deucher > Cc: amd-...@lists.freedesktop.org > Signed-off-by: Gerd Hoffmann > --- > include/uapi/drm/drm_fourcc.h | 82 > +-- > 1 file changed, 41 insertions(+), 41 deletions(-) Hi, just an idea - since we are not sure how the remaining formats are being used, should those be marked somehow uncertain whether they are little or native endian? Otherwise the documentation will guide people to believe those are certain, which OTOH might not be bad, because then it will make them certain over time. Unless we would prefer everything to be native endian? This might be a chance to choose the endianess (native vs. little) for all formats. Should we take it? Prefer native? Thanks, pq pgpXyafRn9bNQ.pgp Description: OpenPGP digital signature
