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

[Ville Syrjälä]

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.

[Ville Syrjälä]

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.

[Pavel Machek]

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.

[Pavel Machek]

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.

[Michel Dänzer]

On 25/04/17 07:26 PM, Ville Syrjälä wrote:
> On Tue, Apr 25, 2017 at 10:12:37AM +0900, Michel Dänzer wrote:
>> On 24/04/17 10:03 PM, Ville Syrjälä wrote:
>>> On Mon, Apr 24, 2017 at 03:57:02PM +0900, Michel Dänzer wrote:
 On 22/04/17 07:05 PM, Ville Syrjälä wrote:
> On Fri, Apr 21, 2017 at 06:14:31PM +0200, Gerd Hoffmann wrote:
>>   Hi,
>>
 My personal opinion is that formats in drm_fourcc.h should be 
 independent of the CPU byte order and the function 
 drm_mode_legacy_fb_format() and drivers depending on that incorrect 
 assumption be fixed instead.
>>>
>>> The problem is this isn't a kernel-internal thing any more.  With the
>>> addition of the ADDFB2 ioctl the fourcc codes became part of the
>>> kernel/userspace abi ...
>>
>> Ok, added some printk's to the ADDFB and ADDFB2 code paths and tested a
>> bit.  Apparently pretty much all userspace still uses the ADDFB ioctl.
>> xorg (modesetting driver) does.  gnome-shell in wayland mode does.
>> Seems the big transition to ADDFB2 didn't happen yet.
>>
>> I guess that makes changing drm_mode_legacy_fb_format + drivers a
>> reasonable option ...
>
> Yeah, I came to the same conclusion after chatting with some
> folks on irc.
>
> So my current idea is that we change any driver that wants to follow the
> CPU endianness

 This isn't really optional for various reasons, some of which have been
 covered in this discussion.


> to declare support for big endian formats if the CPU is
> big endian. Presumably these are mostly the virtual GPU drivers.
>
> Additonally we'll make the mapping performed by 
> drm_mode_legacy_fb_format()
> driver controlled. That way drivers that got changed to follow CPU
> endianness can return a framebuffer that matches CPU endianness. And
> drivers that expect the GPU endianness to not depend on the CPU
> endianness will keep working as they do now. The downside is that users
> of the legacy addfb ioctl will need to magically know which endianness
> they will get, but that is apparently already the case. And users of
> addfb2 will keep on specifying the endianness explicitly with
> DRM_FORMAT_BIG_ENDIAN vs. 0.

 I'm afraid it's not that simple.

 The display hardware of older (pre-R600 generation) Radeon GPUs does not
 support the "big endian" formats directly. In order to allow userspace
 to access pixel data in native endianness with the CPU, we instead use
 byte-swapping functionality which only affects CPU access.
>>>
>>> OK, I'm getting confused. Based on our irc discussion I got the
>>> impression you don't byte swap CPU accesses.
>>
>> Sorry for the confusion. The radeon kernel driver does support
>> byte-swapping for CPU access to VRAM with pre-R600 GPUs, and this is
>> used for fbdev emulation. What I meant on IRC is that the xf86-video-ati
>> radeon driver doesn't make use of this, mostly because it only applies
>> while a BO is in VRAM, and userspace can't control when that's the case
>> (while a BO isn't being scanned out).
> 
> So that was my other question. So if someone just creates a bo, I presume
> ttm can more or less move it between system memory and vram at any
> time. So if we then mmap the bo, does it mean the CPU will see the bytes
> in different order depending on where the bo happens to live at
> the time the CPU access happens?

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

> And how would that work wih dumb bos?

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


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

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

[Michel Dänzer]

On 25/04/17 07:26 PM, Ville Syrjälä wrote:
> On Tue, Apr 25, 2017 at 10:12:37AM +0900, Michel Dänzer wrote:
>> On 24/04/17 10:03 PM, Ville Syrjälä wrote:
>>> On Mon, Apr 24, 2017 at 03:57:02PM +0900, Michel Dänzer wrote:
 On 22/04/17 07:05 PM, Ville Syrjälä wrote:
> On Fri, Apr 21, 2017 at 06:14:31PM +0200, Gerd Hoffmann wrote:
>>   Hi,
>>
 My personal opinion is that formats in drm_fourcc.h should be 
 independent of the CPU byte order and the function 
 drm_mode_legacy_fb_format() and drivers depending on that incorrect 
 assumption be fixed instead.
>>>
>>> The problem is this isn't a kernel-internal thing any more.  With the
>>> addition of the ADDFB2 ioctl the fourcc codes became part of the
>>> kernel/userspace abi ...
>>
>> Ok, added some printk's to the ADDFB and ADDFB2 code paths and tested a
>> bit.  Apparently pretty much all userspace still uses the ADDFB ioctl.
>> xorg (modesetting driver) does.  gnome-shell in wayland mode does.
>> Seems the big transition to ADDFB2 didn't happen yet.
>>
>> I guess that makes changing drm_mode_legacy_fb_format + drivers a
>> reasonable option ...
>
> Yeah, I came to the same conclusion after chatting with some
> folks on irc.
>
> So my current idea is that we change any driver that wants to follow the
> CPU endianness

 This isn't really optional for various reasons, some of which have been
 covered in this discussion.


> to declare support for big endian formats if the CPU is
> big endian. Presumably these are mostly the virtual GPU drivers.
>
> Additonally we'll make the mapping performed by 
> drm_mode_legacy_fb_format()
> driver controlled. That way drivers that got changed to follow CPU
> endianness can return a framebuffer that matches CPU endianness. And
> drivers that expect the GPU endianness to not depend on the CPU
> endianness will keep working as they do now. The downside is that users
> of the legacy addfb ioctl will need to magically know which endianness
> they will get, but that is apparently already the case. And users of
> addfb2 will keep on specifying the endianness explicitly with
> DRM_FORMAT_BIG_ENDIAN vs. 0.

 I'm afraid it's not that simple.

 The display hardware of older (pre-R600 generation) Radeon GPUs does not
 support the "big endian" formats directly. In order to allow userspace
 to access pixel data in native endianness with the CPU, we instead use
 byte-swapping functionality which only affects CPU access.
>>>
>>> OK, I'm getting confused. Based on our irc discussion I got the
>>> impression you don't byte swap CPU accesses.
>>
>> Sorry for the confusion. The radeon kernel driver does support
>> byte-swapping for CPU access to VRAM with pre-R600 GPUs, and this is
>> used for fbdev emulation. What I meant on IRC is that the xf86-video-ati
>> radeon driver doesn't make use of this, mostly because it only applies
>> while a BO is in VRAM, and userspace can't control when that's the case
>> (while a BO isn't being scanned out).
> 
> So that was my other question. So if someone just creates a bo, I presume
> ttm can more or less move it between system memory and vram at any
> time. So if we then mmap the bo, does it mean the CPU will see the bytes
> in different order depending on where the bo happens to live at
> the time the CPU access happens?

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

> And how would that work wih dumb bos?

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


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

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

[Ville Syrjälä]

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.

[Ville Syrjälä]

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.

[Michel Dänzer]

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.

[Michel Dänzer]

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.

[Michel Dänzer]

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.

[Michel Dänzer]

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.

[Ville Syrjälä]

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.

[Ville Syrjälä]

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.

[Michel Dänzer]

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.

[Michel Dänzer]

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.

[Michel Dänzer]

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.

[Michel Dänzer]

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.

[Gerd Hoffmann]

  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.

[Gerd Hoffmann]

  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.

[Ilia Mirkin]

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,
> _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
> 

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

[Ilia Mirkin]

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,
> _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.

[Ilia Mirkin]

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,
_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.

[Ilia Mirkin]

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,
_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.

[Ilia Mirkin]

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.

[Ilia Mirkin]

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.

[Ville Syrjälä]

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.

[Ville Syrjälä]

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.

[Ville Syrjälä]

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.

[Ville Syrjälä]

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.

[Ilia Mirkin]

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.

[Ilia Mirkin]

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.

[Gerd Hoffmann]

  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.

[Gerd Hoffmann]

  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.

[Ville Syrjälä]

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 

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

[Ville Syrjälä]

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_RGBX

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

[Ilia Mirkin]

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') /* 

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

[Harry Wentland]

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.

[Ilia Mirkin]

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 

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

[Harry Wentland]

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.

[Christian König]

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.

[Christian König]

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.

[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

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

[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

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

[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

> 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.

[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

> 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.

[Christian König]

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.

[Christian König]

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.

[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.

-- 
Ville Syrjälä
Intel OTC

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

[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.

-- 
Ville Syrjälä
Intel OTC

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

[Christian König]

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.

[Christian König]

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.

[Gerd Hoffmann]

  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.

[Gerd Hoffmann]

  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.

[Pekka Paalanen]

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.

[Pekka Paalanen]

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.

[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.

-- 
Ville Syrjälä
Intel OTC

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

[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.

-- 
Ville Syrjälä
Intel OTC

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

[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.
> 
> > 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.

[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.
> 
> > 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.

[Gerd Hoffmann]

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.

[Gerd Hoffmann]

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.

[Ville Syrjälä]

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.

[Ville Syrjälä]

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.

[Gerd Hoffmann]

  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.

[Gerd Hoffmann]

  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.

[Ville Syrjälä]

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] 

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

[Ville Syrjälä]

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.

[Pekka Paalanen]

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

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

[Pekka Paalanen]

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

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

[Gerd Hoffmann]

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(-)

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_RGBXfourcc_code('R', 'X', '1', '2') /* [15:0] 
R:G:B:x 4:4:4:4 native endian */
+#define DRM_FORMAT_BGRXfourcc_code('B', 'X', '1', '2') /* [15:0] 
B:G:R:x 4:4:4:4 native endian */
 
-#define DRM_FORMAT_ARGBfourcc_code('A', 'R', '1', '2') /* [15:0] 
A:R:G:B 4:4:4:4 little endian */
-#define DRM_FORMAT_ABGRfourcc_code('A', 'B', '1', '2') /* [15:0] 
A:B:G:R 4:4:4:4 little endian */
-#define DRM_FORMAT_RGBAfourcc_code('R', 'A', '1', '2') /* [15:0] 
R:G:B:A 4:4:4:4 little endian */
-#define DRM_FORMAT_BGRAfourcc_code('B', 'A', '1', '2') /* [15:0] 
B:G:R:A 4:4:4:4 little endian */
+#define DRM_FORMAT_ARGBfourcc_code('A', 'R', '1', '2') /* [15:0] 
A:R:G:B 4:4:4:4 native endian */
+#define DRM_FORMAT_ABGRfourcc_code('A', 'B', '1', '2') /* [15:0] 
A:B:G:R 4:4:4:4 native endian */
+#define DRM_FORMAT_RGBAfourcc_code('R', 'A', '1', '2') /* [15:0] 
R:G:B:A 4:4:4:4 native endian */
+#define DRM_FORMAT_BGRAfourcc_code('B', 'A', '1', '2') /* [15:0] 
B:G:R:A 4:4:4:4 native endian */
 
-#define DRM_FORMAT_XRGB1555fourcc_code('X', 'R', '1', '5') /* [15:0] 
x:R:G:B 1:5:5:5 little endian */
-#define DRM_FORMAT_XBGR1555fourcc_code('X', 'B', '1', '5') /* [15:0] 
x:B:G:R 1:5:5:5 little endian */
-#define DRM_FORMAT_RGBX5551fourcc_code('R', 'X', '1', '5') /* [15:0] 
R:G:B:x 5:5:5:1 little endian */
-#define DRM_FORMAT_BGRX5551fourcc_code('B', 'X', 

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

[Gerd Hoffmann]

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(-)

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_RGBXfourcc_code('R', 'X', '1', '2') /* [15:0] 
R:G:B:x 4:4:4:4 native endian */
+#define DRM_FORMAT_BGRXfourcc_code('B', 'X', '1', '2') /* [15:0] 
B:G:R:x 4:4:4:4 native endian */
 
-#define DRM_FORMAT_ARGBfourcc_code('A', 'R', '1', '2') /* [15:0] 
A:R:G:B 4:4:4:4 little endian */
-#define DRM_FORMAT_ABGRfourcc_code('A', 'B', '1', '2') /* [15:0] 
A:B:G:R 4:4:4:4 little endian */
-#define DRM_FORMAT_RGBAfourcc_code('R', 'A', '1', '2') /* [15:0] 
R:G:B:A 4:4:4:4 little endian */
-#define DRM_FORMAT_BGRAfourcc_code('B', 'A', '1', '2') /* [15:0] 
B:G:R:A 4:4:4:4 little endian */
+#define DRM_FORMAT_ARGBfourcc_code('A', 'R', '1', '2') /* [15:0] 
A:R:G:B 4:4:4:4 native endian */
+#define DRM_FORMAT_ABGRfourcc_code('A', 'B', '1', '2') /* [15:0] 
A:B:G:R 4:4:4:4 native endian */
+#define DRM_FORMAT_RGBAfourcc_code('R', 'A', '1', '2') /* [15:0] 
R:G:B:A 4:4:4:4 native endian */
+#define DRM_FORMAT_BGRAfourcc_code('B', 'A', '1', '2') /* [15:0] 
B:G:R:A 4:4:4:4 native endian */
 
-#define DRM_FORMAT_XRGB1555fourcc_code('X', 'R', '1', '5') /* [15:0] 
x:R:G:B 1:5:5:5 little endian */
-#define DRM_FORMAT_XBGR1555fourcc_code('X', 'B', '1', '5') /* [15:0] 
x:B:G:R 1:5:5:5 little endian */
-#define DRM_FORMAT_RGBX5551fourcc_code('R', 'X', '1', '5') /* [15:0] 
R:G:B:x 5:5:5:1 little endian */
-#define DRM_FORMAT_BGRX5551fourcc_code('B', 'X', '1', '5') /* [15:0] 
B:G:R:x 5:5:5:1 little endian */
+#define DRM_FORMAT_XRGB1555fourcc_code('X', 'R', '1', '5') /* [15:0] 
x:R:G:B 1:5:5:5 native endian 

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

[Gerd Hoffmann]

  Hi,

> > >> BTW, this supports Gerd's patch, since the KMS fbdev emulation code uses
> > >> e.g. DRM_FORMAT_XRGB for depth/bpp 24/32, and the fbdev API uses
> > >> native endian packed colour values.  
> > > 
> > > Same is true for DRM_IOCTL_MODE_ADDFB, with depth/bpp 24/32 you'll get
> > > DRM_FORMAT_XRGB (only DRM_IOCTL_MODE_ADDFB2 allows userspace specify
> > > fourcc formats directly).  
> > 
> > Right, and since all major Xorg drivers use DRM_IOCTL_MODE_ADDFB,
> > they're effectively using DRM_FORMAT_XRGB as native endianness as well.
> 
> I sincerely hope this doesn't actually force us into a place where we
> have XRGB (and ARGB?) as native-endian, but the other format
> codes - since being used explicitly - must be kept as little-endian
> because they were used like that honouring the documentation we have
> atm.

My expectation is that the other formats are (almost) unused in
practice.  cairo for example supports XRGB + ARGB (native
endian) only from all depth/bpp 24/32 formats.

IIRC there was a brief discussion how we should handle endianness in
qemu stdvga / bochsdrm.ko before we've added the new (virtual) hardware
register to switch endianness.  The idea to simply run with fixed
endianness (framebuffer is always little endian) was shot down quickly
with the argument that this isn't going to fly due to lack of support
for XRGB in non-native byte order in the whole graphics stack.

> It's starting to resemble the wl_shm format codes problem we have
> on Wayland for BE.
> 
> Has this now turned into a question of what the kernel drivers do
> with the DRM pixel format codes?
> 
> Hmm, I suppose that has been the question all along...

Yep, basically.  I have the impression that drivers are either consider
those formats being native endian or simply don't care because they are
never used in systems with bigendian (-capable) cpus.

Anyone aware of anything else?

Guess I'll go prepare a new version of the patch, declaring all rgb
formats as native endian and putting a bunch of points from this thread
into the commit message.

cheers,
  Gerd

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

[Gerd Hoffmann]

  Hi,

> > >> BTW, this supports Gerd's patch, since the KMS fbdev emulation code uses
> > >> e.g. DRM_FORMAT_XRGB for depth/bpp 24/32, and the fbdev API uses
> > >> native endian packed colour values.  
> > > 
> > > Same is true for DRM_IOCTL_MODE_ADDFB, with depth/bpp 24/32 you'll get
> > > DRM_FORMAT_XRGB (only DRM_IOCTL_MODE_ADDFB2 allows userspace specify
> > > fourcc formats directly).  
> > 
> > Right, and since all major Xorg drivers use DRM_IOCTL_MODE_ADDFB,
> > they're effectively using DRM_FORMAT_XRGB as native endianness as well.
> 
> I sincerely hope this doesn't actually force us into a place where we
> have XRGB (and ARGB?) as native-endian, but the other format
> codes - since being used explicitly - must be kept as little-endian
> because they were used like that honouring the documentation we have
> atm.

My expectation is that the other formats are (almost) unused in
practice.  cairo for example supports XRGB + ARGB (native
endian) only from all depth/bpp 24/32 formats.

IIRC there was a brief discussion how we should handle endianness in
qemu stdvga / bochsdrm.ko before we've added the new (virtual) hardware
register to switch endianness.  The idea to simply run with fixed
endianness (framebuffer is always little endian) was shot down quickly
with the argument that this isn't going to fly due to lack of support
for XRGB in non-native byte order in the whole graphics stack.

> It's starting to resemble the wl_shm format codes problem we have
> on Wayland for BE.
> 
> Has this now turned into a question of what the kernel drivers do
> with the DRM pixel format codes?
> 
> Hmm, I suppose that has been the question all along...

Yep, basically.  I have the impression that drivers are either consider
those formats being native endian or simply don't care because they are
never used in systems with bigendian (-capable) cpus.

Anyone aware of anything else?

Guess I'll go prepare a new version of the patch, declaring all rgb
formats as native endian and putting a bunch of points from this thread
into the commit message.

cheers,
  Gerd

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

[Pekka Paalanen]

On Wed, 19 Apr 2017 10:01:47 +0900
Michel Dänzer  wrote:

> On 18/04/17 07:14 PM, Gerd Hoffmann wrote:
> >   Hi,
> >   
> >>> Quite true that this proves nothing. However one should note that
> >>> fbcon -> fbdev works,  
> >>
> >> BTW, this supports Gerd's patch, since the KMS fbdev emulation code uses
> >> e.g. DRM_FORMAT_XRGB for depth/bpp 24/32, and the fbdev API uses
> >> native endian packed colour values.  
> > 
> > Same is true for DRM_IOCTL_MODE_ADDFB, with depth/bpp 24/32 you'll get
> > DRM_FORMAT_XRGB (only DRM_IOCTL_MODE_ADDFB2 allows userspace specify
> > fourcc formats directly).  
> 
> Right, and since all major Xorg drivers use DRM_IOCTL_MODE_ADDFB,
> they're effectively using DRM_FORMAT_XRGB as native endianness as well.

I sincerely hope this doesn't actually force us into a place where we
have XRGB (and ARGB?) as native-endian, but the other format
codes - since being used explicitly - must be kept as little-endian
because they were used like that honouring the documentation we have
atm. It's starting to resemble the wl_shm format codes problem we have
on Wayland for BE.

Has this now turned into a question of what the kernel drivers do
with the DRM pixel format codes?

Hmm, I suppose that has been the question all along... userspace uses
whatever formats that look right, new kernel drivers come along and fix
themselves to work with whatever userspace uses? And the people who get
it wrong are the ones who only ever test on LE and trust the docs...


Thanks,
pq


pgpUlaftSuc2c.pgp
Description: OpenPGP digital signature

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

[Pekka Paalanen]

On Wed, 19 Apr 2017 10:01:47 +0900
Michel Dänzer  wrote:

> On 18/04/17 07:14 PM, Gerd Hoffmann wrote:
> >   Hi,
> >   
> >>> Quite true that this proves nothing. However one should note that
> >>> fbcon -> fbdev works,  
> >>
> >> BTW, this supports Gerd's patch, since the KMS fbdev emulation code uses
> >> e.g. DRM_FORMAT_XRGB for depth/bpp 24/32, and the fbdev API uses
> >> native endian packed colour values.  
> > 
> > Same is true for DRM_IOCTL_MODE_ADDFB, with depth/bpp 24/32 you'll get
> > DRM_FORMAT_XRGB (only DRM_IOCTL_MODE_ADDFB2 allows userspace specify
> > fourcc formats directly).  
> 
> Right, and since all major Xorg drivers use DRM_IOCTL_MODE_ADDFB,
> they're effectively using DRM_FORMAT_XRGB as native endianness as well.

I sincerely hope this doesn't actually force us into a place where we
have XRGB (and ARGB?) as native-endian, but the other format
codes - since being used explicitly - must be kept as little-endian
because they were used like that honouring the documentation we have
atm. It's starting to resemble the wl_shm format codes problem we have
on Wayland for BE.

Has this now turned into a question of what the kernel drivers do
with the DRM pixel format codes?

Hmm, I suppose that has been the question all along... userspace uses
whatever formats that look right, new kernel drivers come along and fix
themselves to work with whatever userspace uses? And the people who get
it wrong are the ones who only ever test on LE and trust the docs...


Thanks,
pq


pgpUlaftSuc2c.pgp
Description: OpenPGP digital signature

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

[Ilia Mirkin]

On Tue, Apr 18, 2017 at 11:19 PM, Ilia Mirkin  wrote:
> On Tue, Apr 18, 2017 at 9:01 PM, Michel Dänzer  wrote:
>> On 18/04/17 07:14 PM, Gerd Hoffmann wrote:
>>>   Hi,
>>>
> Quite true that this proves nothing. However one should note that
> fbcon -> fbdev works,

 BTW, this supports Gerd's patch, since the KMS fbdev emulation code uses
 e.g. DRM_FORMAT_XRGB for depth/bpp 24/32, and the fbdev API uses
 native endian packed colour values.
>>>
>>> Same is true for DRM_IOCTL_MODE_ADDFB, with depth/bpp 24/32 you'll get
>>> DRM_FORMAT_XRGB (only DRM_IOCTL_MODE_ADDFB2 allows userspace specify
>>> fourcc formats directly).
>>
>> Right, and since all major Xorg drivers use DRM_IOCTL_MODE_ADDFB,
>> they're effectively using DRM_FORMAT_XRGB as native endianness as well.
>
> In the meanwhile, it has been pointed out to me that pre-nv50 display
> code actually doesn't use DRM_FORMAT_* at all -- it uses some helpers
> which end up advertising XR24 / AR24. However from what I can tell,
> that's not a well-reasoned selection. Either way, I'm going to test
> Gerd's patch, hopefully during the week, or weekend at the latest. My
> current suspicion is that it will have no effect on nouveau either
> way. We'll find out.

(And as Michel points out, the patch doesn't actually touch anything,
just comments. I originally thought it changed format -> fourcc
mapping.)

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

[Ilia Mirkin]

On Tue, Apr 18, 2017 at 11:19 PM, Ilia Mirkin  wrote:
> On Tue, Apr 18, 2017 at 9:01 PM, Michel Dänzer  wrote:
>> On 18/04/17 07:14 PM, Gerd Hoffmann wrote:
>>>   Hi,
>>>
> Quite true that this proves nothing. However one should note that
> fbcon -> fbdev works,

 BTW, this supports Gerd's patch, since the KMS fbdev emulation code uses
 e.g. DRM_FORMAT_XRGB for depth/bpp 24/32, and the fbdev API uses
 native endian packed colour values.
>>>
>>> Same is true for DRM_IOCTL_MODE_ADDFB, with depth/bpp 24/32 you'll get
>>> DRM_FORMAT_XRGB (only DRM_IOCTL_MODE_ADDFB2 allows userspace specify
>>> fourcc formats directly).
>>
>> Right, and since all major Xorg drivers use DRM_IOCTL_MODE_ADDFB,
>> they're effectively using DRM_FORMAT_XRGB as native endianness as well.
>
> In the meanwhile, it has been pointed out to me that pre-nv50 display
> code actually doesn't use DRM_FORMAT_* at all -- it uses some helpers
> which end up advertising XR24 / AR24. However from what I can tell,
> that's not a well-reasoned selection. Either way, I'm going to test
> Gerd's patch, hopefully during the week, or weekend at the latest. My
> current suspicion is that it will have no effect on nouveau either
> way. We'll find out.

(And as Michel points out, the patch doesn't actually touch anything,
just comments. I originally thought it changed format -> fourcc
mapping.)

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

[Ilia Mirkin]

On Tue, Apr 18, 2017 at 9:01 PM, Michel Dänzer  wrote:
> On 18/04/17 07:14 PM, Gerd Hoffmann wrote:
>>   Hi,
>>
 Quite true that this proves nothing. However one should note that
 fbcon -> fbdev works,
>>>
>>> BTW, this supports Gerd's patch, since the KMS fbdev emulation code uses
>>> e.g. DRM_FORMAT_XRGB for depth/bpp 24/32, and the fbdev API uses
>>> native endian packed colour values.
>>
>> Same is true for DRM_IOCTL_MODE_ADDFB, with depth/bpp 24/32 you'll get
>> DRM_FORMAT_XRGB (only DRM_IOCTL_MODE_ADDFB2 allows userspace specify
>> fourcc formats directly).
>
> Right, and since all major Xorg drivers use DRM_IOCTL_MODE_ADDFB,
> they're effectively using DRM_FORMAT_XRGB as native endianness as well.

In the meanwhile, it has been pointed out to me that pre-nv50 display
code actually doesn't use DRM_FORMAT_* at all -- it uses some helpers
which end up advertising XR24 / AR24. However from what I can tell,
that's not a well-reasoned selection. Either way, I'm going to test
Gerd's patch, hopefully during the week, or weekend at the latest. My
current suspicion is that it will have no effect on nouveau either
way. We'll find out.

  -ilia

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

[Ilia Mirkin]

On Tue, Apr 18, 2017 at 9:01 PM, Michel Dänzer  wrote:
> On 18/04/17 07:14 PM, Gerd Hoffmann wrote:
>>   Hi,
>>
 Quite true that this proves nothing. However one should note that
 fbcon -> fbdev works,
>>>
>>> BTW, this supports Gerd's patch, since the KMS fbdev emulation code uses
>>> e.g. DRM_FORMAT_XRGB for depth/bpp 24/32, and the fbdev API uses
>>> native endian packed colour values.
>>
>> Same is true for DRM_IOCTL_MODE_ADDFB, with depth/bpp 24/32 you'll get
>> DRM_FORMAT_XRGB (only DRM_IOCTL_MODE_ADDFB2 allows userspace specify
>> fourcc formats directly).
>
> Right, and since all major Xorg drivers use DRM_IOCTL_MODE_ADDFB,
> they're effectively using DRM_FORMAT_XRGB as native endianness as well.

In the meanwhile, it has been pointed out to me that pre-nv50 display
code actually doesn't use DRM_FORMAT_* at all -- it uses some helpers
which end up advertising XR24 / AR24. However from what I can tell,
that's not a well-reasoned selection. Either way, I'm going to test
Gerd's patch, hopefully during the week, or weekend at the latest. My
current suspicion is that it will have no effect on nouveau either
way. We'll find out.

  -ilia

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

[Michel Dänzer]

On 18/04/17 07:14 PM, Gerd Hoffmann wrote:
>   Hi,
> 
>>> Quite true that this proves nothing. However one should note that
>>> fbcon -> fbdev works,
>>
>> BTW, this supports Gerd's patch, since the KMS fbdev emulation code uses
>> e.g. DRM_FORMAT_XRGB for depth/bpp 24/32, and the fbdev API uses
>> native endian packed colour values.
> 
> Same is true for DRM_IOCTL_MODE_ADDFB, with depth/bpp 24/32 you'll get
> DRM_FORMAT_XRGB (only DRM_IOCTL_MODE_ADDFB2 allows userspace specify
> fourcc formats directly).

Right, and since all major Xorg drivers use DRM_IOCTL_MODE_ADDFB,
they're effectively using DRM_FORMAT_XRGB as native endianness as well.


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

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

[Michel Dänzer]

On 18/04/17 07:14 PM, Gerd Hoffmann wrote:
>   Hi,
> 
>>> Quite true that this proves nothing. However one should note that
>>> fbcon -> fbdev works,
>>
>> BTW, this supports Gerd's patch, since the KMS fbdev emulation code uses
>> e.g. DRM_FORMAT_XRGB for depth/bpp 24/32, and the fbdev API uses
>> native endian packed colour values.
> 
> Same is true for DRM_IOCTL_MODE_ADDFB, with depth/bpp 24/32 you'll get
> DRM_FORMAT_XRGB (only DRM_IOCTL_MODE_ADDFB2 allows userspace specify
> fourcc formats directly).

Right, and since all major Xorg drivers use DRM_IOCTL_MODE_ADDFB,
they're effectively using DRM_FORMAT_XRGB as native endianness as well.


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

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

[Gerd Hoffmann]

  Hi,

> Right. Very nice if we can trust the virtual machine at least getting
> things right, gives some chance for people to test anything. Except...
> that's a question of what kind of hardware the virtual machine
> emulates. The display device defines what endianess it uses on
> framebuffers, not the CPU, right?

The display device supports switching the endianess for the framebuffer,
at least with kernel 3.19+ and qemu 2.2+.  Default endianness depends on
the machine type, i.e. ppc64 guests get a bigendian framebuffer and
pretty much everything else a little endian framebuffer on reset.

The bochs-drm driver switches the display into native endian mode, i.e.
big endian for ppc64 and little endian for ppc64le kernels.

See commit 9ecdb039b7517dc10b8c3e6dbeb40859178ac28e

> > Well, I mean color glitches.  But it isn't consistent.  As if some
> > operations operate with the correct byteorder and some don't.
> > alpha/blue being swapped is a problem in some areas.
> > 
> > https://www.kraxel.org/tmp/
> 
> Ooh, yeah, that's definitely bonkers.
> 
> Maybe the 100% blue things are supposed to be a transparent blended
> overlays, like highlights.
> 
> The icons look somehow... not completely right to me. Somehow washed
> out?
> 
> Opaque gray shades are hard to tell right from wrong.
> 
> gnome-terminal and the wallpaper look right, but those might be the
> only things.
> 
> Having a compositing manager complicates things.

In some way yes, in some way no.  Tried wayland meanwhile (using
"gnome-shell --wayland").  Looks pretty much the same.  Window
decorations look a bit different (good on xorg, broken on wayland),
probably because window decorations work completely different.
Otherwise it is bug compatible to xorg.  Probably because gnome-shell
composites everything using llvmpipe, so it's largely the same code
running on both xorg and wayland, which then finally scans out to a dumb
framebuffer.

cheers,
  Gerd

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

[Gerd Hoffmann]

  Hi,

> Right. Very nice if we can trust the virtual machine at least getting
> things right, gives some chance for people to test anything. Except...
> that's a question of what kind of hardware the virtual machine
> emulates. The display device defines what endianess it uses on
> framebuffers, not the CPU, right?

The display device supports switching the endianess for the framebuffer,
at least with kernel 3.19+ and qemu 2.2+.  Default endianness depends on
the machine type, i.e. ppc64 guests get a bigendian framebuffer and
pretty much everything else a little endian framebuffer on reset.

The bochs-drm driver switches the display into native endian mode, i.e.
big endian for ppc64 and little endian for ppc64le kernels.

See commit 9ecdb039b7517dc10b8c3e6dbeb40859178ac28e

> > Well, I mean color glitches.  But it isn't consistent.  As if some
> > operations operate with the correct byteorder and some don't.
> > alpha/blue being swapped is a problem in some areas.
> > 
> > https://www.kraxel.org/tmp/
> 
> Ooh, yeah, that's definitely bonkers.
> 
> Maybe the 100% blue things are supposed to be a transparent blended
> overlays, like highlights.
> 
> The icons look somehow... not completely right to me. Somehow washed
> out?
> 
> Opaque gray shades are hard to tell right from wrong.
> 
> gnome-terminal and the wallpaper look right, but those might be the
> only things.
> 
> Having a compositing manager complicates things.

In some way yes, in some way no.  Tried wayland meanwhile (using
"gnome-shell --wayland").  Looks pretty much the same.  Window
decorations look a bit different (good on xorg, broken on wayland),
probably because window decorations work completely different.
Otherwise it is bug compatible to xorg.  Probably because gnome-shell
composites everything using llvmpipe, so it's largely the same code
running on both xorg and wayland, which then finally scans out to a dumb
framebuffer.

cheers,
  Gerd

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

[Pekka Paalanen]

On Tue, 18 Apr 2017 15:39:53 +0200
Gerd Hoffmann  wrote:

>   Hi,
> 
> > > Historical note:  RHEL-6.9 (gnome 2) works fine.  Not of much interest
> > > here, it drives the qemu stdvga with offb, not bochs-drm.  
> > 
> > I suppose this proves the virtual machine itself is correct about
> > framebuffer endianess? Except you are running it on a little-endian
> > host machine I presume...  
> 
> Yes, little endian host, qemu interprets the framebuffer as
> PIXMAN_b8g8r8x8.  Which should be correct for a xrgb bigendian
> framebuffer as pixman formats are native endian.

Right. Very nice if we can trust the virtual machine at least getting
things right, gives some chance for people to test anything. Except...
that's a question of what kind of hardware the virtual machine
emulates. The display device defines what endianess it uses on
framebuffers, not the CPU, right?

> > > More interesting:  RHEL-7.3 (gnome 3.14) works fine too.  kernel 3.10,
> > > but drm drivers updated to roughly 4.6 level.  Runs bochs-drm.  mesa
> > > 11.2.2.  glamour not used.
> > > 
> > > Most recent:  Fedora 25 (gnome 3.22) looks mostly ok, but there are
> > > rendering glitches, for example in the gnome activities screen (the one
> > > you get when you press the windows key).  kernel 4.10, mesa 13.0.4.
> > > glamor not used, but I think gnome-shell uses opengl (via llvmpipe) for
> > > compositing.  
> > 
> > I believe glitches are irrelevant for this topic, what we are
> > interested in is if the colors are right or byte-swapped (also mind
> > alpha/blue etc. swaps).  
> 
> Well, I mean color glitches.  But it isn't consistent.  As if some
> operations operate with the correct byteorder and some don't.
> alpha/blue being swapped is a problem in some areas.
> 
> https://www.kraxel.org/tmp/

Ooh, yeah, that's definitely bonkers.

Maybe the 100% blue things are supposed to be a transparent blended
overlays, like highlights.

The icons look somehow... not completely right to me. Somehow washed
out?

Opaque gray shades are hard to tell right from wrong.

gnome-terminal and the wallpaper look right, but those might be the
only things.

Having a compositing manager complicates things.


Thanks,
pq


pgpfWwCdXhbZU.pgp
Description: OpenPGP digital signature

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

[Pekka Paalanen]

On Tue, 18 Apr 2017 15:39:53 +0200
Gerd Hoffmann  wrote:

>   Hi,
> 
> > > Historical note:  RHEL-6.9 (gnome 2) works fine.  Not of much interest
> > > here, it drives the qemu stdvga with offb, not bochs-drm.  
> > 
> > I suppose this proves the virtual machine itself is correct about
> > framebuffer endianess? Except you are running it on a little-endian
> > host machine I presume...  
> 
> Yes, little endian host, qemu interprets the framebuffer as
> PIXMAN_b8g8r8x8.  Which should be correct for a xrgb bigendian
> framebuffer as pixman formats are native endian.

Right. Very nice if we can trust the virtual machine at least getting
things right, gives some chance for people to test anything. Except...
that's a question of what kind of hardware the virtual machine
emulates. The display device defines what endianess it uses on
framebuffers, not the CPU, right?

> > > More interesting:  RHEL-7.3 (gnome 3.14) works fine too.  kernel 3.10,
> > > but drm drivers updated to roughly 4.6 level.  Runs bochs-drm.  mesa
> > > 11.2.2.  glamour not used.
> > > 
> > > Most recent:  Fedora 25 (gnome 3.22) looks mostly ok, but there are
> > > rendering glitches, for example in the gnome activities screen (the one
> > > you get when you press the windows key).  kernel 4.10, mesa 13.0.4.
> > > glamor not used, but I think gnome-shell uses opengl (via llvmpipe) for
> > > compositing.  
> > 
> > I believe glitches are irrelevant for this topic, what we are
> > interested in is if the colors are right or byte-swapped (also mind
> > alpha/blue etc. swaps).  
> 
> Well, I mean color glitches.  But it isn't consistent.  As if some
> operations operate with the correct byteorder and some don't.
> alpha/blue being swapped is a problem in some areas.
> 
> https://www.kraxel.org/tmp/

Ooh, yeah, that's definitely bonkers.

Maybe the 100% blue things are supposed to be a transparent blended
overlays, like highlights.

The icons look somehow... not completely right to me. Somehow washed
out?

Opaque gray shades are hard to tell right from wrong.

gnome-terminal and the wallpaper look right, but those might be the
only things.

Having a compositing manager complicates things.


Thanks,
pq


pgpfWwCdXhbZU.pgp
Description: OpenPGP digital signature

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

[Gerd Hoffmann]

  Hi,

> > Historical note:  RHEL-6.9 (gnome 2) works fine.  Not of much interest
> > here, it drives the qemu stdvga with offb, not bochs-drm.
> 
> I suppose this proves the virtual machine itself is correct about
> framebuffer endianess? Except you are running it on a little-endian
> host machine I presume...

Yes, little endian host, qemu interprets the framebuffer as
PIXMAN_b8g8r8x8.  Which should be correct for a xrgb bigendian
framebuffer as pixman formats are native endian.

> > More interesting:  RHEL-7.3 (gnome 3.14) works fine too.  kernel 3.10,
> > but drm drivers updated to roughly 4.6 level.  Runs bochs-drm.  mesa
> > 11.2.2.  glamour not used.
> > 
> > Most recent:  Fedora 25 (gnome 3.22) looks mostly ok, but there are
> > rendering glitches, for example in the gnome activities screen (the one
> > you get when you press the windows key).  kernel 4.10, mesa 13.0.4.
> > glamor not used, but I think gnome-shell uses opengl (via llvmpipe) for
> > compositing.
> 
> I believe glitches are irrelevant for this topic, what we are
> interested in is if the colors are right or byte-swapped (also mind
> alpha/blue etc. swaps).

Well, I mean color glitches.  But it isn't consistent.  As if some
operations operate with the correct byteorder and some don't.
alpha/blue being swapped is a problem in some areas.

https://www.kraxel.org/tmp/

cheers,
  Gerd

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

[Gerd Hoffmann]

  Hi,

> > Historical note:  RHEL-6.9 (gnome 2) works fine.  Not of much interest
> > here, it drives the qemu stdvga with offb, not bochs-drm.
> 
> I suppose this proves the virtual machine itself is correct about
> framebuffer endianess? Except you are running it on a little-endian
> host machine I presume...

Yes, little endian host, qemu interprets the framebuffer as
PIXMAN_b8g8r8x8.  Which should be correct for a xrgb bigendian
framebuffer as pixman formats are native endian.

> > More interesting:  RHEL-7.3 (gnome 3.14) works fine too.  kernel 3.10,
> > but drm drivers updated to roughly 4.6 level.  Runs bochs-drm.  mesa
> > 11.2.2.  glamour not used.
> > 
> > Most recent:  Fedora 25 (gnome 3.22) looks mostly ok, but there are
> > rendering glitches, for example in the gnome activities screen (the one
> > you get when you press the windows key).  kernel 4.10, mesa 13.0.4.
> > glamor not used, but I think gnome-shell uses opengl (via llvmpipe) for
> > compositing.
> 
> I believe glitches are irrelevant for this topic, what we are
> interested in is if the colors are right or byte-swapped (also mind
> alpha/blue etc. swaps).

Well, I mean color glitches.  But it isn't consistent.  As if some
operations operate with the correct byteorder and some don't.
alpha/blue being swapped is a problem in some areas.

https://www.kraxel.org/tmp/

cheers,
  Gerd

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

[Pekka Paalanen]

On Tue, 18 Apr 2017 12:00:17 +0200
Gerd Hoffmann  wrote:

>   Hi,
> 
> > > ppc64 (big endian) virtual machine, running with qemu stdvga & bochs-drm
> > > driver.  Xorg with modesetting driver uses DRM_FORMAT_XRGB (one and
> > > only format supported by bochs-drm), and we have to interpret that in
> > > bigendian byte order on the host side to get a correct display.  
> > 
> > I wonder if that is just an oversight from trying to match OpenGL
> > formats to DRM formats. It's full of gotcha's.
> > 
> > Did you try with GLAMOR? Do you see a difference with and without
> > GLAMOR? Hmm, but you have no GPU support, so GLAMOR would be through a
> > Mesa software renderer? I think I heard someone say something about
> > Mesa software on BE...  
> 
> So, did some more testing to see where we stand.
> 
> Historical note:  RHEL-6.9 (gnome 2) works fine.  Not of much interest
> here, it drives the qemu stdvga with offb, not bochs-drm.

I suppose this proves the virtual machine itself is correct about
framebuffer endianess? Except you are running it on a little-endian
host machine I presume...

> More interesting:  RHEL-7.3 (gnome 3.14) works fine too.  kernel 3.10,
> but drm drivers updated to roughly 4.6 level.  Runs bochs-drm.  mesa
> 11.2.2.  glamour not used.
> 
> Most recent:  Fedora 25 (gnome 3.22) looks mostly ok, but there are
> rendering glitches, for example in the gnome activities screen (the one
> you get when you press the windows key).  kernel 4.10, mesa 13.0.4.
> glamor not used, but I think gnome-shell uses opengl (via llvmpipe) for
> compositing.

I believe glitches are irrelevant for this topic, what we are
interested in is if the colors are right or byte-swapped (also mind
alpha/blue etc. swaps).

> btw: is there some way to start a wayland session from a shell (i.e.
> what startx does for xorg)?

Depends on which display server you want to start I believe. I don't
know about anything else than Weston, which is 'weston' for a
logind-enabled system.


Thanks,
pq


pgpEK9fAiXret.pgp
Description: OpenPGP digital signature

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

[Pekka Paalanen]

On Tue, 18 Apr 2017 12:00:17 +0200
Gerd Hoffmann  wrote:

>   Hi,
> 
> > > ppc64 (big endian) virtual machine, running with qemu stdvga & bochs-drm
> > > driver.  Xorg with modesetting driver uses DRM_FORMAT_XRGB (one and
> > > only format supported by bochs-drm), and we have to interpret that in
> > > bigendian byte order on the host side to get a correct display.  
> > 
> > I wonder if that is just an oversight from trying to match OpenGL
> > formats to DRM formats. It's full of gotcha's.
> > 
> > Did you try with GLAMOR? Do you see a difference with and without
> > GLAMOR? Hmm, but you have no GPU support, so GLAMOR would be through a
> > Mesa software renderer? I think I heard someone say something about
> > Mesa software on BE...  
> 
> So, did some more testing to see where we stand.
> 
> Historical note:  RHEL-6.9 (gnome 2) works fine.  Not of much interest
> here, it drives the qemu stdvga with offb, not bochs-drm.

I suppose this proves the virtual machine itself is correct about
framebuffer endianess? Except you are running it on a little-endian
host machine I presume...

> More interesting:  RHEL-7.3 (gnome 3.14) works fine too.  kernel 3.10,
> but drm drivers updated to roughly 4.6 level.  Runs bochs-drm.  mesa
> 11.2.2.  glamour not used.
> 
> Most recent:  Fedora 25 (gnome 3.22) looks mostly ok, but there are
> rendering glitches, for example in the gnome activities screen (the one
> you get when you press the windows key).  kernel 4.10, mesa 13.0.4.
> glamor not used, but I think gnome-shell uses opengl (via llvmpipe) for
> compositing.

I believe glitches are irrelevant for this topic, what we are
interested in is if the colors are right or byte-swapped (also mind
alpha/blue etc. swaps).

> btw: is there some way to start a wayland session from a shell (i.e.
> what startx does for xorg)?

Depends on which display server you want to start I believe. I don't
know about anything else than Weston, which is 'weston' for a
logind-enabled system.


Thanks,
pq


pgpEK9fAiXret.pgp
Description: OpenPGP digital signature

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

[Gerd Hoffmann]

  Hi,

> > Quite true that this proves nothing. However one should note that
> > fbcon -> fbdev works,
> 
> BTW, this supports Gerd's patch, since the KMS fbdev emulation code uses
> e.g. DRM_FORMAT_XRGB for depth/bpp 24/32, and the fbdev API uses
> native endian packed colour values.

Same is true for DRM_IOCTL_MODE_ADDFB, with depth/bpp 24/32 you'll get
DRM_FORMAT_XRGB (only DRM_IOCTL_MODE_ADDFB2 allows userspace specify
fourcc formats directly).

cheers,
  Gerd

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

[Gerd Hoffmann]

  Hi,

> > Quite true that this proves nothing. However one should note that
> > fbcon -> fbdev works,
> 
> BTW, this supports Gerd's patch, since the KMS fbdev emulation code uses
> e.g. DRM_FORMAT_XRGB for depth/bpp 24/32, and the fbdev API uses
> native endian packed colour values.

Same is true for DRM_IOCTL_MODE_ADDFB, with depth/bpp 24/32 you'll get
DRM_FORMAT_XRGB (only DRM_IOCTL_MODE_ADDFB2 allows userspace specify
fourcc formats directly).

cheers,
  Gerd

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

[Gerd Hoffmann]

  Hi,

> > ppc64 (big endian) virtual machine, running with qemu stdvga & bochs-drm
> > driver.  Xorg with modesetting driver uses DRM_FORMAT_XRGB (one and
> > only format supported by bochs-drm), and we have to interpret that in
> > bigendian byte order on the host side to get a correct display.
> 
> I wonder if that is just an oversight from trying to match OpenGL
> formats to DRM formats. It's full of gotcha's.
> 
> Did you try with GLAMOR? Do you see a difference with and without
> GLAMOR? Hmm, but you have no GPU support, so GLAMOR would be through a
> Mesa software renderer? I think I heard someone say something about
> Mesa software on BE...

So, did some more testing to see where we stand.

Historical note:  RHEL-6.9 (gnome 2) works fine.  Not of much interest
here, it drives the qemu stdvga with offb, not bochs-drm.

More interesting:  RHEL-7.3 (gnome 3.14) works fine too.  kernel 3.10,
but drm drivers updated to roughly 4.6 level.  Runs bochs-drm.  mesa
11.2.2.  glamour not used.

Most recent:  Fedora 25 (gnome 3.22) looks mostly ok, but there are
rendering glitches, for example in the gnome activities screen (the one
you get when you press the windows key).  kernel 4.10, mesa 13.0.4.
glamor not used, but I think gnome-shell uses opengl (via llvmpipe) for
compositing.

btw: is there some way to start a wayland session from a shell (i.e.
what startx does for xorg)?

cheers,
  Gerd

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

[Gerd Hoffmann]

  Hi,

> > ppc64 (big endian) virtual machine, running with qemu stdvga & bochs-drm
> > driver.  Xorg with modesetting driver uses DRM_FORMAT_XRGB (one and
> > only format supported by bochs-drm), and we have to interpret that in
> > bigendian byte order on the host side to get a correct display.
> 
> I wonder if that is just an oversight from trying to match OpenGL
> formats to DRM formats. It's full of gotcha's.
> 
> Did you try with GLAMOR? Do you see a difference with and without
> GLAMOR? Hmm, but you have no GPU support, so GLAMOR would be through a
> Mesa software renderer? I think I heard someone say something about
> Mesa software on BE...

So, did some more testing to see where we stand.

Historical note:  RHEL-6.9 (gnome 2) works fine.  Not of much interest
here, it drives the qemu stdvga with offb, not bochs-drm.

More interesting:  RHEL-7.3 (gnome 3.14) works fine too.  kernel 3.10,
but drm drivers updated to roughly 4.6 level.  Runs bochs-drm.  mesa
11.2.2.  glamour not used.

Most recent:  Fedora 25 (gnome 3.22) looks mostly ok, but there are
rendering glitches, for example in the gnome activities screen (the one
you get when you press the windows key).  kernel 4.10, mesa 13.0.4.
glamor not used, but I think gnome-shell uses opengl (via llvmpipe) for
compositing.

btw: is there some way to start a wayland session from a shell (i.e.
what startx does for xorg)?

cheers,
  Gerd

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

[Michel Dänzer]

On 18/04/17 02:04 PM, Ilia Mirkin wrote:
> On Mon, Apr 17, 2017 at 10:53 PM, Michel Dänzer  wrote:
>> On 17/04/17 03:43 PM, Ilia Mirkin wrote:
>>> On Tue, Apr 11, 2017 at 10:18 AM, Ilia Mirkin  wrote:
> However, I totally agree with Alex that someone with a BE machine
> should review the whole stack before we could be confident with anything.

 Here's what I'm confident about: xf86-video-nouveau worked just fine
 on top of kernel 4.3 on an AGP GeForce FX 5200 (with AGPGART turned
 off because ... well ... uninorth). fbcon/fbdev accel worked,
 xf86-video-nouveau's 2d accel worked, and simple demos (ala glxgears)
 worked after I fixed up mesa and nv30 driver items in version ... 11.1
 it seems. As I recall it had gotten all broken in 10.0 or so by Adam
 Jackson in the name of making llvmpipe work on BE, declaring all other
 drivers broken, with various fixes by Michel Dänzer to get it back to
 working over the years.

 Anyone "fixing" the stack has to maintain that level of functioning
 through their various fixing.

 I will double-check that the above still works with the latest
 kernel/xorg/xf86-video-nouveau/mesa and report back (hopefully by this
 weekend). If there are any patches you'd like me to test, now's the
 time to ask -- getting the box up and running is the hard part,
 booting up an extra kernel -- easy.
>>>
>>> OK, so I revived my PowerMac7,3 G5 setup (PPC64 BE, NV34 GPU). Booted
>>> it with an upstream 4.11-rc7 kernel, loaded up the nouveau kernel
>>> module (which is included in that kernel), updated X to 1.19.2 and
>>> mesa to 17.0.3. Everything works fine. Specifically:
>>>
>>> fbcon on top of fbdev provided by nouveau -- colors are fine
>>> glxgears hw-accelerated by mesa on top of xf86-video-nouveau using
>>> DRI2 -- colors are fine
>>> glxgears softpipe-accelerated by mesa on top of xf86-video-nouveau --
>>> colors are fine
>>> glxgears softpipe-accelerated by mesa on top of xf86-video-modesetting
>>> -- colors are fine
>>> xterm on top of xf86-video-nouveau -- colors are fine
>>> xterm on top of xf86-video-modesetting -- colors are fine
>>>
>>> I couldn't test anything with GLAMOR since GLAMOR requires GL 2.1 or
>>> higher, whereas nouveau's NV3x acceleration only provides GL 1.5 (due
>>> to lacking NPOT and a handful of other things).
>>>
>>> The modetest utility did have trouble with AR24 and I'm pretty sure
>>> the XR24 pattern was off too. However I wouldn't be surprised if the
>>> modetest utility itself had endian issues in the pattern generation
>>> logic. (Seems to be the case, based on a quick glance at the
>>> tests/util/format.c logic and how it's used in pattern.c.)
>>>
>>> So in short, I think the current definitions of format are fine.
>>
>> I agree with Pekka that it's not that simple. What you've established is
>> that things look fine after going through several layers of abstraction.
>> It's possible that multiple bugs in those layers cancel each other out;
>> in particular, it's quite likely that the code dealing with DRM formats
>> is treating them as using native endianness (one possible giveaway for
>> that is using shifts for (un)packing colour components).
> 
> Quite true that this proves nothing. However one should note that
> fbcon -> fbdev works,

BTW, this supports Gerd's patch, since the KMS fbdev emulation code uses
e.g. DRM_FORMAT_XRGB for depth/bpp 24/32, and the fbdev API uses
native endian packed colour values.


> and both mesa hw driver and softpipe driver work, in addition to regular
> DDX accel.

Similarly, the X11 protocol uses native endian packed colour values.


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

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

[Michel Dänzer]

On 18/04/17 02:04 PM, Ilia Mirkin wrote:
> On Mon, Apr 17, 2017 at 10:53 PM, Michel Dänzer  wrote:
>> On 17/04/17 03:43 PM, Ilia Mirkin wrote:
>>> On Tue, Apr 11, 2017 at 10:18 AM, Ilia Mirkin  wrote:
> However, I totally agree with Alex that someone with a BE machine
> should review the whole stack before we could be confident with anything.

 Here's what I'm confident about: xf86-video-nouveau worked just fine
 on top of kernel 4.3 on an AGP GeForce FX 5200 (with AGPGART turned
 off because ... well ... uninorth). fbcon/fbdev accel worked,
 xf86-video-nouveau's 2d accel worked, and simple demos (ala glxgears)
 worked after I fixed up mesa and nv30 driver items in version ... 11.1
 it seems. As I recall it had gotten all broken in 10.0 or so by Adam
 Jackson in the name of making llvmpipe work on BE, declaring all other
 drivers broken, with various fixes by Michel Dänzer to get it back to
 working over the years.

 Anyone "fixing" the stack has to maintain that level of functioning
 through their various fixing.

 I will double-check that the above still works with the latest
 kernel/xorg/xf86-video-nouveau/mesa and report back (hopefully by this
 weekend). If there are any patches you'd like me to test, now's the
 time to ask -- getting the box up and running is the hard part,
 booting up an extra kernel -- easy.
>>>
>>> OK, so I revived my PowerMac7,3 G5 setup (PPC64 BE, NV34 GPU). Booted
>>> it with an upstream 4.11-rc7 kernel, loaded up the nouveau kernel
>>> module (which is included in that kernel), updated X to 1.19.2 and
>>> mesa to 17.0.3. Everything works fine. Specifically:
>>>
>>> fbcon on top of fbdev provided by nouveau -- colors are fine
>>> glxgears hw-accelerated by mesa on top of xf86-video-nouveau using
>>> DRI2 -- colors are fine
>>> glxgears softpipe-accelerated by mesa on top of xf86-video-nouveau --
>>> colors are fine
>>> glxgears softpipe-accelerated by mesa on top of xf86-video-modesetting
>>> -- colors are fine
>>> xterm on top of xf86-video-nouveau -- colors are fine
>>> xterm on top of xf86-video-modesetting -- colors are fine
>>>
>>> I couldn't test anything with GLAMOR since GLAMOR requires GL 2.1 or
>>> higher, whereas nouveau's NV3x acceleration only provides GL 1.5 (due
>>> to lacking NPOT and a handful of other things).
>>>
>>> The modetest utility did have trouble with AR24 and I'm pretty sure
>>> the XR24 pattern was off too. However I wouldn't be surprised if the
>>> modetest utility itself had endian issues in the pattern generation
>>> logic. (Seems to be the case, based on a quick glance at the
>>> tests/util/format.c logic and how it's used in pattern.c.)
>>>
>>> So in short, I think the current definitions of format are fine.
>>
>> I agree with Pekka that it's not that simple. What you've established is
>> that things look fine after going through several layers of abstraction.
>> It's possible that multiple bugs in those layers cancel each other out;
>> in particular, it's quite likely that the code dealing with DRM formats
>> is treating them as using native endianness (one possible giveaway for
>> that is using shifts for (un)packing colour components).
> 
> Quite true that this proves nothing. However one should note that
> fbcon -> fbdev works,

BTW, this supports Gerd's patch, since the KMS fbdev emulation code uses
e.g. DRM_FORMAT_XRGB for depth/bpp 24/32, and the fbdev API uses
native endian packed colour values.


> and both mesa hw driver and softpipe driver work, in addition to regular
> DDX accel.

Similarly, the X11 protocol uses native endian packed colour values.


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

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

[Ilia Mirkin]

On Mon, Apr 17, 2017 at 10:53 PM, Michel Dänzer  wrote:
> On 17/04/17 03:43 PM, Ilia Mirkin wrote:
>> On Tue, Apr 11, 2017 at 10:18 AM, Ilia Mirkin  wrote:
 However, I totally agree with Alex that someone with a BE machine
 should review the whole stack before we could be confident with anything.
>>>
>>> Here's what I'm confident about: xf86-video-nouveau worked just fine
>>> on top of kernel 4.3 on an AGP GeForce FX 5200 (with AGPGART turned
>>> off because ... well ... uninorth). fbcon/fbdev accel worked,
>>> xf86-video-nouveau's 2d accel worked, and simple demos (ala glxgears)
>>> worked after I fixed up mesa and nv30 driver items in version ... 11.1
>>> it seems. As I recall it had gotten all broken in 10.0 or so by Adam
>>> Jackson in the name of making llvmpipe work on BE, declaring all other
>>> drivers broken, with various fixes by Michel Dänzer to get it back to
>>> working over the years.
>>>
>>> Anyone "fixing" the stack has to maintain that level of functioning
>>> through their various fixing.
>>>
>>> I will double-check that the above still works with the latest
>>> kernel/xorg/xf86-video-nouveau/mesa and report back (hopefully by this
>>> weekend). If there are any patches you'd like me to test, now's the
>>> time to ask -- getting the box up and running is the hard part,
>>> booting up an extra kernel -- easy.
>>
>> OK, so I revived my PowerMac7,3 G5 setup (PPC64 BE, NV34 GPU). Booted
>> it with an upstream 4.11-rc7 kernel, loaded up the nouveau kernel
>> module (which is included in that kernel), updated X to 1.19.2 and
>> mesa to 17.0.3. Everything works fine. Specifically:
>>
>> fbcon on top of fbdev provided by nouveau -- colors are fine
>> glxgears hw-accelerated by mesa on top of xf86-video-nouveau using
>> DRI2 -- colors are fine
>> glxgears softpipe-accelerated by mesa on top of xf86-video-nouveau --
>> colors are fine
>> glxgears softpipe-accelerated by mesa on top of xf86-video-modesetting
>> -- colors are fine
>> xterm on top of xf86-video-nouveau -- colors are fine
>> xterm on top of xf86-video-modesetting -- colors are fine
>>
>> I couldn't test anything with GLAMOR since GLAMOR requires GL 2.1 or
>> higher, whereas nouveau's NV3x acceleration only provides GL 1.5 (due
>> to lacking NPOT and a handful of other things).
>>
>> The modetest utility did have trouble with AR24 and I'm pretty sure
>> the XR24 pattern was off too. However I wouldn't be surprised if the
>> modetest utility itself had endian issues in the pattern generation
>> logic. (Seems to be the case, based on a quick glance at the
>> tests/util/format.c logic and how it's used in pattern.c.)
>>
>> So in short, I think the current definitions of format are fine.
>
> I agree with Pekka that it's not that simple. What you've established is
> that things look fine after going through several layers of abstraction.
> It's possible that multiple bugs in those layers cancel each other out;
> in particular, it's quite likely that the code dealing with DRM formats
> is treating them as using native endianness (one possible giveaway for
> that is using shifts for (un)packing colour components).

Quite true that this proves nothing. However one should note that
fbcon -> fbdev works, and both mesa hw driver and softpipe driver
work, in addition to regular DDX accel. Which means that the bugs, if
they exist, are pretty consistent amongst each other, spanning
multiple layers, all agreeing as to what the proper bugginess is. One
could go so far as to declare it to be a feature.

It does show is that things generally work today in at least some, if
not many, setups, and one can't go around breaking them willy nilly.

  -ilia

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

[Ilia Mirkin]

On Mon, Apr 17, 2017 at 10:53 PM, Michel Dänzer  wrote:
> On 17/04/17 03:43 PM, Ilia Mirkin wrote:
>> On Tue, Apr 11, 2017 at 10:18 AM, Ilia Mirkin  wrote:
 However, I totally agree with Alex that someone with a BE machine
 should review the whole stack before we could be confident with anything.
>>>
>>> Here's what I'm confident about: xf86-video-nouveau worked just fine
>>> on top of kernel 4.3 on an AGP GeForce FX 5200 (with AGPGART turned
>>> off because ... well ... uninorth). fbcon/fbdev accel worked,
>>> xf86-video-nouveau's 2d accel worked, and simple demos (ala glxgears)
>>> worked after I fixed up mesa and nv30 driver items in version ... 11.1
>>> it seems. As I recall it had gotten all broken in 10.0 or so by Adam
>>> Jackson in the name of making llvmpipe work on BE, declaring all other
>>> drivers broken, with various fixes by Michel Dänzer to get it back to
>>> working over the years.
>>>
>>> Anyone "fixing" the stack has to maintain that level of functioning
>>> through their various fixing.
>>>
>>> I will double-check that the above still works with the latest
>>> kernel/xorg/xf86-video-nouveau/mesa and report back (hopefully by this
>>> weekend). If there are any patches you'd like me to test, now's the
>>> time to ask -- getting the box up and running is the hard part,
>>> booting up an extra kernel -- easy.
>>
>> OK, so I revived my PowerMac7,3 G5 setup (PPC64 BE, NV34 GPU). Booted
>> it with an upstream 4.11-rc7 kernel, loaded up the nouveau kernel
>> module (which is included in that kernel), updated X to 1.19.2 and
>> mesa to 17.0.3. Everything works fine. Specifically:
>>
>> fbcon on top of fbdev provided by nouveau -- colors are fine
>> glxgears hw-accelerated by mesa on top of xf86-video-nouveau using
>> DRI2 -- colors are fine
>> glxgears softpipe-accelerated by mesa on top of xf86-video-nouveau --
>> colors are fine
>> glxgears softpipe-accelerated by mesa on top of xf86-video-modesetting
>> -- colors are fine
>> xterm on top of xf86-video-nouveau -- colors are fine
>> xterm on top of xf86-video-modesetting -- colors are fine
>>
>> I couldn't test anything with GLAMOR since GLAMOR requires GL 2.1 or
>> higher, whereas nouveau's NV3x acceleration only provides GL 1.5 (due
>> to lacking NPOT and a handful of other things).
>>
>> The modetest utility did have trouble with AR24 and I'm pretty sure
>> the XR24 pattern was off too. However I wouldn't be surprised if the
>> modetest utility itself had endian issues in the pattern generation
>> logic. (Seems to be the case, based on a quick glance at the
>> tests/util/format.c logic and how it's used in pattern.c.)
>>
>> So in short, I think the current definitions of format are fine.
>
> I agree with Pekka that it's not that simple. What you've established is
> that things look fine after going through several layers of abstraction.
> It's possible that multiple bugs in those layers cancel each other out;
> in particular, it's quite likely that the code dealing with DRM formats
> is treating them as using native endianness (one possible giveaway for
> that is using shifts for (un)packing colour components).

Quite true that this proves nothing. However one should note that
fbcon -> fbdev works, and both mesa hw driver and softpipe driver
work, in addition to regular DDX accel. Which means that the bugs, if
they exist, are pretty consistent amongst each other, spanning
multiple layers, all agreeing as to what the proper bugginess is. One
could go so far as to declare it to be a feature.

It does show is that things generally work today in at least some, if
not many, setups, and one can't go around breaking them willy nilly.

  -ilia

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

[Michel Dänzer]

On 17/04/17 03:43 PM, Ilia Mirkin wrote:
> On Tue, Apr 11, 2017 at 10:18 AM, Ilia Mirkin  wrote:
>>> However, I totally agree with Alex that someone with a BE machine
>>> should review the whole stack before we could be confident with anything.
>>
>> Here's what I'm confident about: xf86-video-nouveau worked just fine
>> on top of kernel 4.3 on an AGP GeForce FX 5200 (with AGPGART turned
>> off because ... well ... uninorth). fbcon/fbdev accel worked,
>> xf86-video-nouveau's 2d accel worked, and simple demos (ala glxgears)
>> worked after I fixed up mesa and nv30 driver items in version ... 11.1
>> it seems. As I recall it had gotten all broken in 10.0 or so by Adam
>> Jackson in the name of making llvmpipe work on BE, declaring all other
>> drivers broken, with various fixes by Michel Dänzer to get it back to
>> working over the years.
>>
>> Anyone "fixing" the stack has to maintain that level of functioning
>> through their various fixing.
>>
>> I will double-check that the above still works with the latest
>> kernel/xorg/xf86-video-nouveau/mesa and report back (hopefully by this
>> weekend). If there are any patches you'd like me to test, now's the
>> time to ask -- getting the box up and running is the hard part,
>> booting up an extra kernel -- easy.
> 
> OK, so I revived my PowerMac7,3 G5 setup (PPC64 BE, NV34 GPU). Booted
> it with an upstream 4.11-rc7 kernel, loaded up the nouveau kernel
> module (which is included in that kernel), updated X to 1.19.2 and
> mesa to 17.0.3. Everything works fine. Specifically:
> 
> fbcon on top of fbdev provided by nouveau -- colors are fine
> glxgears hw-accelerated by mesa on top of xf86-video-nouveau using
> DRI2 -- colors are fine
> glxgears softpipe-accelerated by mesa on top of xf86-video-nouveau --
> colors are fine
> glxgears softpipe-accelerated by mesa on top of xf86-video-modesetting
> -- colors are fine
> xterm on top of xf86-video-nouveau -- colors are fine
> xterm on top of xf86-video-modesetting -- colors are fine
> 
> I couldn't test anything with GLAMOR since GLAMOR requires GL 2.1 or
> higher, whereas nouveau's NV3x acceleration only provides GL 1.5 (due
> to lacking NPOT and a handful of other things).
> 
> The modetest utility did have trouble with AR24 and I'm pretty sure
> the XR24 pattern was off too. However I wouldn't be surprised if the
> modetest utility itself had endian issues in the pattern generation
> logic. (Seems to be the case, based on a quick glance at the
> tests/util/format.c logic and how it's used in pattern.c.)
> 
> So in short, I think the current definitions of format are fine.

I agree with Pekka that it's not that simple. What you've established is
that things look fine after going through several layers of abstraction.
It's possible that multiple bugs in those layers cancel each other out;
in particular, it's quite likely that the code dealing with DRM formats
is treating them as using native endianness (one possible giveaway for
that is using shifts for (un)packing colour components).


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

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

[Michel Dänzer]

On 17/04/17 03:43 PM, Ilia Mirkin wrote:
> On Tue, Apr 11, 2017 at 10:18 AM, Ilia Mirkin  wrote:
>>> However, I totally agree with Alex that someone with a BE machine
>>> should review the whole stack before we could be confident with anything.
>>
>> Here's what I'm confident about: xf86-video-nouveau worked just fine
>> on top of kernel 4.3 on an AGP GeForce FX 5200 (with AGPGART turned
>> off because ... well ... uninorth). fbcon/fbdev accel worked,
>> xf86-video-nouveau's 2d accel worked, and simple demos (ala glxgears)
>> worked after I fixed up mesa and nv30 driver items in version ... 11.1
>> it seems. As I recall it had gotten all broken in 10.0 or so by Adam
>> Jackson in the name of making llvmpipe work on BE, declaring all other
>> drivers broken, with various fixes by Michel Dänzer to get it back to
>> working over the years.
>>
>> Anyone "fixing" the stack has to maintain that level of functioning
>> through their various fixing.
>>
>> I will double-check that the above still works with the latest
>> kernel/xorg/xf86-video-nouveau/mesa and report back (hopefully by this
>> weekend). If there are any patches you'd like me to test, now's the
>> time to ask -- getting the box up and running is the hard part,
>> booting up an extra kernel -- easy.
> 
> OK, so I revived my PowerMac7,3 G5 setup (PPC64 BE, NV34 GPU). Booted
> it with an upstream 4.11-rc7 kernel, loaded up the nouveau kernel
> module (which is included in that kernel), updated X to 1.19.2 and
> mesa to 17.0.3. Everything works fine. Specifically:
> 
> fbcon on top of fbdev provided by nouveau -- colors are fine
> glxgears hw-accelerated by mesa on top of xf86-video-nouveau using
> DRI2 -- colors are fine
> glxgears softpipe-accelerated by mesa on top of xf86-video-nouveau --
> colors are fine
> glxgears softpipe-accelerated by mesa on top of xf86-video-modesetting
> -- colors are fine
> xterm on top of xf86-video-nouveau -- colors are fine
> xterm on top of xf86-video-modesetting -- colors are fine
> 
> I couldn't test anything with GLAMOR since GLAMOR requires GL 2.1 or
> higher, whereas nouveau's NV3x acceleration only provides GL 1.5 (due
> to lacking NPOT and a handful of other things).
> 
> The modetest utility did have trouble with AR24 and I'm pretty sure
> the XR24 pattern was off too. However I wouldn't be surprised if the
> modetest utility itself had endian issues in the pattern generation
> logic. (Seems to be the case, based on a quick glance at the
> tests/util/format.c logic and how it's used in pattern.c.)
> 
> So in short, I think the current definitions of format are fine.

I agree with Pekka that it's not that simple. What you've established is
that things look fine after going through several layers of abstraction.
It's possible that multiple bugs in those layers cancel each other out;
in particular, it's quite likely that the code dealing with DRM formats
is treating them as using native endianness (one possible giveaway for
that is using shifts for (un)packing colour components).


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