Re: [PATCH 0/6] drm: tackle byteorder issues, take two
On Tue, Apr 25, 2017 at 09:49:38AM +0900, Michel Dänzer wrote: > On 24/04/17 11:26 PM, Ville Syrjälä wrote: > > On Mon, Apr 24, 2017 at 04:54:25PM +0900, Michel Dänzer wrote: > >> On 24/04/17 04:36 PM, Gerd Hoffmann wrote: > >>> > > drm: fourcc byteorder: add DRM_FORMAT_CPU_* > > drm: fourcc byteorder: add bigendian support to > > drm_mode_legacy_fb_format > > As I explained in my last followup in the "[PATCH] drm: fourcc > byteorder: brings header file comments in line with reality." thread, > the mapping between GPU and CPU formats has to be provided by the > driver, it cannot be done statically. > >>> > >>> Well, the drm fourcc codes represent the cpu view (i.e. what userspace > >>> will fill the ADDFB2-created framebuffers with). > >> > >> Ville is adamant that they represent the GPU view. This needs to be > >> resolved one way or the other. > > > > Since the byte swapping can happen either for CPU or display access > > I guess we can't just consider the GPU and display as a single entity. > > > > We may need to consider several agents: > > 1. display > > 2. GPU > > 3. CPU > > 4. other DMA > > > > Not sure what we can say about 4. I presume it's going to be like the > > GPU or the CPU in the sense that it might go through the CPU byte > > swapping logic or not. I'm just going to ignore it. > > > > Let's say we have the following bytes in memory > > (in order of increasing address): A,B,C,D > > We'll assume GPU and display are LE natively. Each component will see > > the resulting 32bpp pixel as follows (msb left->lsb right): > > > > LE CPU w/ no byte swapping: > > display: DCBA > > GPU: DCBA > > CPU: DCBA > > = everyone agrees > > > > BE CPU w/ no byte swapping: > > display: DCBA > > GPU: DCBA > > CPU: ABCD > > = GPU and display agree > > > > BE CPU w/ display byte swapping: > > display: ABCD > > GPU: DCBA > > CPU: ABCD > > = CPU and display agree > > > > BE CPU w/ CPU access byte swapping: > > display: DCBA > > GPU: DCBA > > CPU: DCBA > > = everyone agrees > > Beware that for this list, you're using a format definition Actually I'm not using any format definitions here. > which is > based on a packed 32-bit value. This does *not* match the current > DRM_FORMAT_* definitions. E.g. in the last case, display and GPU use > the same DRM_FORMAT, but the CPU uses the "inverse" one. I wasn't concerned about the specific drm format, just what kind of a 32bpp value everyone will see given the same memory contents. -- Ville Syrjälä Intel OTC ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 0/6] drm: tackle byteorder issues, take two
On 24/04/17 11:26 PM, Ville Syrjälä wrote: > On Mon, Apr 24, 2017 at 04:54:25PM +0900, Michel Dänzer wrote: >> On 24/04/17 04:36 PM, Gerd Hoffmann wrote: >>> > drm: fourcc byteorder: add DRM_FORMAT_CPU_* > drm: fourcc byteorder: add bigendian support to > drm_mode_legacy_fb_format As I explained in my last followup in the "[PATCH] drm: fourcc byteorder: brings header file comments in line with reality." thread, the mapping between GPU and CPU formats has to be provided by the driver, it cannot be done statically. >>> >>> Well, the drm fourcc codes represent the cpu view (i.e. what userspace >>> will fill the ADDFB2-created framebuffers with). >> >> Ville is adamant that they represent the GPU view. This needs to be >> resolved one way or the other. > > Since the byte swapping can happen either for CPU or display access > I guess we can't just consider the GPU and display as a single entity. > > We may need to consider several agents: > 1. display > 2. GPU > 3. CPU > 4. other DMA > > Not sure what we can say about 4. I presume it's going to be like the > GPU or the CPU in the sense that it might go through the CPU byte > swapping logic or not. I'm just going to ignore it. > > Let's say we have the following bytes in memory > (in order of increasing address): A,B,C,D > We'll assume GPU and display are LE natively. Each component will see > the resulting 32bpp pixel as follows (msb left->lsb right): > > LE CPU w/ no byte swapping: > display: DCBA > GPU: DCBA > CPU: DCBA > = everyone agrees > > BE CPU w/ no byte swapping: > display: DCBA > GPU: DCBA > CPU: ABCD > = GPU and display agree > > BE CPU w/ display byte swapping: > display: ABCD > GPU: DCBA > CPU: ABCD > = CPU and display agree > > BE CPU w/ CPU access byte swapping: > display: DCBA > GPU: DCBA > CPU: DCBA > = everyone agrees Beware that for this list, you're using a format definition which is based on a packed 32-bit value. This does *not* match the current DRM_FORMAT_* definitions. E.g. in the last case, display and GPU use the same DRM_FORMAT, but the CPU uses the "inverse" one. -- 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 0/6] drm: tackle byteorder issues, take two
On Mon, Apr 24, 2017 at 05:26:03PM +0300, Ville Syrjälä wrote: > On Mon, Apr 24, 2017 at 04:54:25PM +0900, Michel Dänzer wrote: > > On 24/04/17 04:36 PM, Gerd Hoffmann wrote: > > > > > >>> drm: fourcc byteorder: add DRM_FORMAT_CPU_* > > >>> drm: fourcc byteorder: add bigendian support to > > >>> drm_mode_legacy_fb_format > > >> > > >> As I explained in my last followup in the "[PATCH] drm: fourcc > > >> byteorder: brings header file comments in line with reality." thread, > > >> the mapping between GPU and CPU formats has to be provided by the > > >> driver, it cannot be done statically. > > > > > > Well, the drm fourcc codes represent the cpu view (i.e. what userspace > > > will fill the ADDFB2-created framebuffers with). > > > > Ville is adamant that they represent the GPU view. This needs to be > > resolved one way or the other. > > Since the byte swapping can happen either for CPU or display access > I guess we can't just consider the GPU and display as a single entity. > > We may need to consider several agents: > 1. display > 2. GPU > 3. CPU > 4. other DMA > > Not sure what we can say about 4. I presume it's going to be like the > GPU or the CPU in the sense that it might go through the CPU byte > swapping logic or not. I'm just going to ignore it. > > Let's say we have the following bytes in memory > (in order of increasing address): A,B,C,D > We'll assume GPU and display are LE natively. Each component will see > the resulting 32bpp pixel as follows (msb left->lsb right): > > LE CPU w/ no byte swapping: > display: DCBA > GPU: DCBA > CPU: DCBA > = everyone agrees > > BE CPU w/ no byte swapping: > display: DCBA > GPU: DCBA > CPU: ABCD > = GPU and display agree > > BE CPU w/ display byte swapping: > display: ABCD > GPU: DCBA > CPU: ABCD > = CPU and display agree So after some further thought this seems like a somewhat crazy combination. It does make sense from the simplicity POV in that the CPU byte swapping isn't needed, and thus the problems with concurrent access to buffers with different pixel sizes vanish. However the GPU has to somehow be able to produce data the display can consume, so presumably there must be some knobs in the GPU to do the opposite byte swapping that the display does, or the GPU must be restricted to only use framebuffers in formats like . > > BE CPU w/ CPU access byte swapping: > display: DCBA > GPU: DCBA > CPU: DCBA > = everyone agrees > > BE CPU w/ both display and CPU byte swapping: > display: ABCD > GPU: DCBA > CPU: DCBA > = CPU and GPU agree (doesn't seem all that useful) > > The different byte swapping tricks must have seemed like a great idea to > someone, but in the end they're just making our life more miserable. > > > > The gpu view can certainly differ from that. Implementing this is up > > > to the driver IMO. > > > > > > When running on dumb framebuffers userspace doesn't need to know what > > > the gpu view is. > > True. So for that we'd just need to consider whether the CPU and display > agree or disagree on the byte order. And I guess we'd have to pick from > the following choices for a BE CPU: > > CPU and display agree: > * FB is big endian, or FB is host endian (or whatever we would call it) > CPU and display disagree: > * FB is little endian, or FB is foreign endian (or whatever) > > > > > > > When running in opengl mode there will be a hardware-specific mesa > > > driver in userspace, which will either know what the gpu view is (for > > > example because there is only one way to implement this in hardware) or > > > it can use hardware-specific ioctls to ask the kernel driver what the > > > gpu view is. > > > > Not sure this can be hidden in the OpenGL driver. How would e.g. a > > Wayland compositor or the Xorg modesetting driver know which OpenGL > > format corresponds to a given DRM_FORMAT? > > How are GL formats defined? /me needs to go read the spec again. > > -- > Ville Syrjälä > Intel OTC -- Ville Syrjälä Intel OTC ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 0/6] drm: tackle byteorder issues, take two
Hi, On 24 April 2017 at 15:26, Ville Syrjälä wrote: > On Mon, Apr 24, 2017 at 04:54:25PM +0900, Michel Dänzer wrote: >> On 24/04/17 04:36 PM, Gerd Hoffmann wrote: >> > When running in opengl mode there will be a hardware-specific mesa >> > driver in userspace, which will either know what the gpu view is (for >> > example because there is only one way to implement this in hardware) or >> > it can use hardware-specific ioctls to ask the kernel driver what the >> > gpu view is. >> >> Not sure this can be hidden in the OpenGL driver. How would e.g. a >> Wayland compositor or the Xorg modesetting driver know which OpenGL >> format corresponds to a given DRM_FORMAT? > > How are GL formats defined? /me needs to go read the spec again. They aren't, per se. Only relative to 'native formats', which for this discussion is the set of GBM formats, which is in turn just drm_fourcc.h. Cheers, Daniel ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 0/6] drm: tackle byteorder issues, take two
On Mon, Apr 24, 2017 at 04:54:25PM +0900, Michel Dänzer wrote: > On 24/04/17 04:36 PM, Gerd Hoffmann wrote: > > > >>> drm: fourcc byteorder: add DRM_FORMAT_CPU_* > >>> drm: fourcc byteorder: add bigendian support to > >>> drm_mode_legacy_fb_format > >> > >> As I explained in my last followup in the "[PATCH] drm: fourcc > >> byteorder: brings header file comments in line with reality." thread, > >> the mapping between GPU and CPU formats has to be provided by the > >> driver, it cannot be done statically. > > > > Well, the drm fourcc codes represent the cpu view (i.e. what userspace > > will fill the ADDFB2-created framebuffers with). > > Ville is adamant that they represent the GPU view. This needs to be > resolved one way or the other. Since the byte swapping can happen either for CPU or display access I guess we can't just consider the GPU and display as a single entity. We may need to consider several agents: 1. display 2. GPU 3. CPU 4. other DMA Not sure what we can say about 4. I presume it's going to be like the GPU or the CPU in the sense that it might go through the CPU byte swapping logic or not. I'm just going to ignore it. Let's say we have the following bytes in memory (in order of increasing address): A,B,C,D We'll assume GPU and display are LE natively. Each component will see the resulting 32bpp pixel as follows (msb left->lsb right): LE CPU w/ no byte swapping: display: DCBA GPU: DCBA CPU: DCBA = everyone agrees BE CPU w/ no byte swapping: display: DCBA GPU: DCBA CPU: ABCD = GPU and display agree BE CPU w/ display byte swapping: display: ABCD GPU: DCBA CPU: ABCD = CPU and display agree BE CPU w/ CPU access byte swapping: display: DCBA GPU: DCBA CPU: DCBA = everyone agrees BE CPU w/ both display and CPU byte swapping: display: ABCD GPU: DCBA CPU: DCBA = CPU and GPU agree (doesn't seem all that useful) The different byte swapping tricks must have seemed like a great idea to someone, but in the end they're just making our life more miserable. > > The gpu view can certainly differ from that. Implementing this is up > > to the driver IMO. > > > > When running on dumb framebuffers userspace doesn't need to know what > > the gpu view is. True. So for that we'd just need to consider whether the CPU and display agree or disagree on the byte order. And I guess we'd have to pick from the following choices for a BE CPU: CPU and display agree: * FB is big endian, or FB is host endian (or whatever we would call it) CPU and display disagree: * FB is little endian, or FB is foreign endian (or whatever) > > > > When running in opengl mode there will be a hardware-specific mesa > > driver in userspace, which will either know what the gpu view is (for > > example because there is only one way to implement this in hardware) or > > it can use hardware-specific ioctls to ask the kernel driver what the > > gpu view is. > > Not sure this can be hidden in the OpenGL driver. How would e.g. a > Wayland compositor or the Xorg modesetting driver know which OpenGL > format corresponds to a given DRM_FORMAT? How are GL formats defined? /me needs to go read the spec again. -- Ville Syrjälä Intel OTC ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 0/6] drm: tackle byteorder issues, take two
On 24/04/17 04:36 PM, Gerd Hoffmann wrote: > >>> drm: fourcc byteorder: add DRM_FORMAT_CPU_* >>> drm: fourcc byteorder: add bigendian support to >>> drm_mode_legacy_fb_format >> >> As I explained in my last followup in the "[PATCH] drm: fourcc >> byteorder: brings header file comments in line with reality." thread, >> the mapping between GPU and CPU formats has to be provided by the >> driver, it cannot be done statically. > > Well, the drm fourcc codes represent the cpu view (i.e. what userspace > will fill the ADDFB2-created framebuffers with). Ville is adamant that they represent the GPU view. This needs to be resolved one way or the other. > The gpu view can certainly differ from that. Implementing this is up > to the driver IMO. > > When running on dumb framebuffers userspace doesn't need to know what > the gpu view is. > > When running in opengl mode there will be a hardware-specific mesa > driver in userspace, which will either know what the gpu view is (for > example because there is only one way to implement this in hardware) or > it can use hardware-specific ioctls to ask the kernel driver what the > gpu view is. Not sure this can be hidden in the OpenGL driver. How would e.g. a Wayland compositor or the Xorg modesetting driver know which OpenGL format corresponds to a given DRM_FORMAT? -- 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 0/6] drm: tackle byteorder issues, take two
Hi, > > Gerd Hoffmann (6): > > drm: fourcc byteorder: drop DRM_FORMAT_BIG_ENDIAN > > I don't see how it can be dropped. It's only optional for formats where > all components have 8 bits. Well, we can, because it is simply not used anywhere ... We indeed can't specify RGB565 or XRGB2101010 in bigendian byte order without this. Apparently nobody needed that so far. Should the need to support this arise I'd rather define new fourcc codes, because I think we should have exactly one fourcc per format. With the bigendian flag all 8bit component formats have two: XRGB in bigendian can be "XRGB | BIGENDIAN" and "BGRX". > > drm: fourcc byteorder: add DRM_FORMAT_CPU_* > > drm: fourcc byteorder: add bigendian support to > > drm_mode_legacy_fb_format > > As I explained in my last followup in the "[PATCH] drm: fourcc > byteorder: brings header file comments in line with reality." thread, > the mapping between GPU and CPU formats has to be provided by the > driver, it cannot be done statically. Well, the drm fourcc codes represent the cpu view (i.e. what userspace will fill the ADDFB2-created framebuffers with). The gpu view can certainly differ from that. Implementing this is up to the driver IMO. When running on dumb framebuffers userspace doesn't need to know what the gpu view is. When running in opengl mode there will be a hardware-specific mesa driver in userspace, which will either know what the gpu view is (for example because there is only one way to implement this in hardware) or it can use hardware-specific ioctls to ask the kernel driver what the gpu view is. So, I can't see the problem you are seeing here. Did I miss something? cheers, Gerd ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 0/6] drm: tackle byteorder issues, take two
On 24/04/17 03:25 PM, Gerd Hoffmann wrote: > Hi, > > Ok, different approach up for discussion. Given that userspace didn't > made the transition from ADDFB to ADDFB2 yet it seems we still can muck > with the fourcc codes without breaking everything, as long as we > maintain ADDFB and fbdev behavior (use cpu byte order format) so nothing > changes for userspace. > > So, this series basically makes drm_mode_legacy_fb_format return correct > formats in bigendian mode and adapts the bochs and virtio drivers to > this change. Other drivers must be adapted to this change too. > > Ilia Mirkin figured the dispnv04 backend in nouveau turns on/off > byteswapping depending on cpu byte order. So, one way to adapt the > driver would be to simply use the new #defines added by patch #2. The > other way would be to support both XRGB and BGRX and turn on/off > byteswapping depending on framebuffer format instead of cpu byte order. > > cheers, > Gerd > > Gerd Hoffmann (6): > drm: fourcc byteorder: drop DRM_FORMAT_BIG_ENDIAN I don't see how it can be dropped. It's only optional for formats where all components have 8 bits. > drm: fourcc byteorder: add DRM_FORMAT_CPU_* > drm: fourcc byteorder: add bigendian support to > drm_mode_legacy_fb_format As I explained in my last followup in the "[PATCH] drm: fourcc byteorder: brings header file comments in line with reality." thread, the mapping between GPU and CPU formats has to be provided by the driver, it cannot be done statically. -- 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
[PATCH 0/6] drm: tackle byteorder issues, take two
Hi, Ok, different approach up for discussion. Given that userspace didn't made the transition from ADDFB to ADDFB2 yet it seems we still can muck with the fourcc codes without breaking everything, as long as we maintain ADDFB and fbdev behavior (use cpu byte order format) so nothing changes for userspace. So, this series basically makes drm_mode_legacy_fb_format return correct formats in bigendian mode and adapts the bochs and virtio drivers to this change. Other drivers must be adapted to this change too. Ilia Mirkin figured the dispnv04 backend in nouveau turns on/off byteswapping depending on cpu byte order. So, one way to adapt the driver would be to simply use the new #defines added by patch #2. The other way would be to support both XRGB and BGRX and turn on/off byteswapping depending on framebuffer format instead of cpu byte order. cheers, Gerd Gerd Hoffmann (6): drm: fourcc byteorder: drop DRM_FORMAT_BIG_ENDIAN drm: fourcc byteorder: add DRM_FORMAT_CPU_* drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format drm: fourcc byteorder: adapt bochs-drm to drm_mode_legacy_fb_format update drm: fourcc byteorder: adapt virtio to drm_mode_legacy_fb_format update drm: fourcc byteorder: virtio restrict to XRGB include/drm/drm_fourcc.h | 12 ++ include/uapi/drm/drm_fourcc.h | 2 -- drivers/gpu/drm/bochs/bochs_mm.c | 2 +- drivers/gpu/drm/drm_fourcc.c | 27 +-- drivers/gpu/drm/drm_framebuffer.c | 2 +- drivers/gpu/drm/virtio/virtgpu_gem.c | 7 -- drivers/gpu/drm/virtio/virtgpu_plane.c | 40 +- 7 files changed, 45 insertions(+), 47 deletions(-) -- 2.9.3 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
