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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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.

___
amd-gfx mailing list
amd-gfx@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 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
___
amd-gfx mailing list
amd-gfx@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 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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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
___
amd-gfx mailing list
amd-gfx@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 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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[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-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx




___
amd-gfx mailing list
amd-gfx@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-gfx@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.

[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-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx



___
amd-gfx mailing list
amd-gfx@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



___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

___
amd-gfx mailing list
amd-gfx@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 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.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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


pgpGEBgIeZhUU.pgp
Description: OpenPGP digital signature
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[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-gfx@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


pgp_WMozPJwPC.pgp
Description: OpenPGP digital signature
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx