| Message ID | 20230428092711.406-6-tzimmermann@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | fbdev: Use regular I/O function for framebuffers | expand |
Hi Am 28.04.23 um 15:17 schrieb Arnd Bergmann: > On Fri, Apr 28, 2023, at 13:27, Geert Uytterhoeven wrote: >> On Fri, Apr 28, 2023 at 2:18 PM Robin Murphy <robin.murphy@arm.com> wrote: >>> On 2023-04-28 10:27, Thomas Zimmermann wrote: > >>>> - >>>> -#elif defined(__i386__) || defined(__alpha__) || defined(__x86_64__) || \ >>>> - defined(__hppa__) || defined(__sh__) || defined(__powerpc__) || \ >>>> - defined(__arm__) || defined(__aarch64__) || defined(__mips__) >>>> - >>>> -#define fb_readb __raw_readb >>>> -#define fb_readw __raw_readw >>>> -#define fb_readl __raw_readl >>>> -#define fb_readq __raw_readq >>>> -#define fb_writeb __raw_writeb >>>> -#define fb_writew __raw_writew >>>> -#define fb_writel __raw_writel >>>> -#define fb_writeq __raw_writeq >>> >>> Note that on at least some architectures, the __raw variants are >>> native-endian, whereas the regular accessors are explicitly >>> little-endian, so there is a slight risk of inadvertently changing >>> behaviour on big-endian systems (MIPS most likely, but a few old ARM >>> platforms run BE as well). >> >> Also on m68k, when ISA or PCI are enabled. >> >> In addition, the non-raw variants may do some extras to guarantee >> ordering, which you do not need on a frame buffer. >> >> So I'd go for the __raw_*() variants everywhere. > > The only implementations in fbdev are > > 1) sparc sbus > 2) __raw_writel > 3) direct pointer dereference > > But none use the byte-swapping writel() implementations, and > the only ones that use the direct pointer dereference or sbus > are the ones on which these are defined the same as __raw_writel After thinking a bit more about the requirements, I'd like to got back to v1, but with a different spin. We want to avoid ordering guarantees, so I looked at the _relaxed() helpers, but they seem to swap bytes to little endian. I guess we can remove the fb_mem*() functions entirely. They are the same as the non-fb_ counterparts. For the fb read/write helpers, I'd like to add them to <asm-generic/fb.h> in a platform-neutral way. They'd be wrappers around __raw_(), as I wouldn't want invocations of __raw_() functions in the fbdev drivers. Best regards Thomas > > Arnd
On Sat, Apr 29, 2023, at 14:26, Thomas Zimmermann wrote: > Am 28.04.23 um 15:17 schrieb Arnd Bergmann: >> The only implementations in fbdev are >> >> 1) sparc sbus >> 2) __raw_writel >> 3) direct pointer dereference >> >> But none use the byte-swapping writel() implementations, and >> the only ones that use the direct pointer dereference or sbus >> are the ones on which these are defined the same as __raw_writel > > After thinking a bit more about the requirements, I'd like to got back > to v1, but with a different spin. We want to avoid ordering guarantees, > so I looked at the _relaxed() helpers, but they seem to swap bytes to > little endian. Right, the _relaxed() oens are clearly wrong, aside from the byteswap they also include barriers on some architectures where the __raw_* version is more relaxed than the required semantics for relaxed. > I guess we can remove the fb_mem*() functions entirely. They are the > same as the non-fb_ counterparts. These might actually be different in some cases, or sub-optimal at the moment. memcpy()/memset() don't take __iomem pointers, so they cause sparse warnings, while the memset_io()/memcpy_fromio()/ memcpy_toio() sometimes fall back to bytewise access that is slower than word-sized copy. I only looked at the readl/writel style functions earlier, no idea what we want here. > For the fb read/write helpers, I'd > like to add them to <asm-generic/fb.h> in a platform-neutral way. They'd > be wrappers around __raw_(), as I wouldn't want invocations of __raw_() > functions in the fbdev drivers. That sounds good to me. Arnd
diff --git a/include/linux/fb.h b/include/linux/fb.h index 08cb47da71f8..4aa9e90edd17 100644 --- a/include/linux/fb.h +++ b/include/linux/fb.h @@ -15,7 +15,6 @@ #include <linux/list.h> #include <linux/backlight.h> #include <linux/slab.h> -#include <asm/io.h> struct vm_area_struct; struct fb_info; @@ -511,58 +510,26 @@ struct fb_info { */ #define STUPID_ACCELF_TEXT_SHIT -// This will go away -#if defined(__sparc__) - -/* We map all of our framebuffers such that big-endian accesses - * are what we want, so the following is sufficient. +/* + * TODO: Update fbdev drivers to call the I/O helpers directly and + * remove the fb_() tokens. */ - -// This will go away -#define fb_readb sbus_readb -#define fb_readw sbus_readw -#define fb_readl sbus_readl -#define fb_readq sbus_readq -#define fb_writeb sbus_writeb -#define fb_writew sbus_writew -#define fb_writel sbus_writel -#define fb_writeq sbus_writeq -#define fb_memset sbus_memset_io -#define fb_memcpy_fromfb sbus_memcpy_fromio -#define fb_memcpy_tofb sbus_memcpy_toio - -#elif defined(__i386__) || defined(__alpha__) || defined(__x86_64__) || \ - defined(__hppa__) || defined(__sh__) || defined(__powerpc__) || \ - defined(__arm__) || defined(__aarch64__) || defined(__mips__) - -#define fb_readb __raw_readb -#define fb_readw __raw_readw -#define fb_readl __raw_readl -#define fb_readq __raw_readq -#define fb_writeb __raw_writeb -#define fb_writew __raw_writew -#define fb_writel __raw_writel -#define fb_writeq __raw_writeq +#define fb_readb readb +#define fb_readw readw +#define fb_readl readl +#if defined(CONFIG_64BIT) +#define fb_readq readq +#endif +#define fb_writeb writeb +#define fb_writew writew +#define fb_writel writel +#if defined(CONFIG_64BIT) +#define fb_writeq writeq +#endif #define fb_memset memset_io #define fb_memcpy_fromfb memcpy_fromio #define fb_memcpy_tofb memcpy_toio -#else - -#define fb_readb(addr) (*(volatile u8 *) (addr)) -#define fb_readw(addr) (*(volatile u16 *) (addr)) -#define fb_readl(addr) (*(volatile u32 *) (addr)) -#define fb_readq(addr) (*(volatile u64 *) (addr)) -#define fb_writeb(b,addr) (*(volatile u8 *) (addr) = (b)) -#define fb_writew(b,addr) (*(volatile u16 *) (addr) = (b)) -#define fb_writel(b,addr) (*(volatile u32 *) (addr) = (b)) -#define fb_writeq(b,addr) (*(volatile u64 *) (addr) = (b)) -#define fb_memset memset -#define fb_memcpy_fromfb memcpy -#define fb_memcpy_tofb memcpy - -#endif - #define FB_LEFT_POS(p, bpp) (fb_be_math(p) ? (32 - (bpp)) : 0) #define FB_SHIFT_HIGH(p, val, bits) (fb_be_math(p) ? (val) >> (bits) : \ (val) << (bits))
Implement framebuffer I/O helpers, such as fb_read*() and fb_write*() with Linux' regular I/O functions. Remove all ifdef cases for the various architectures. Most of the supported architectures use __raw_() I/O functions or treat framebuffer memory like regular memory. This is also implemented by the architectures' I/O function, so we can use them instead. Sparc uses SBus to connect to framebuffer devices. It provides respective implementations of the framebuffer I/O helpers. The involved sbus_() I/O helpers map to the same code as Sparc's regular I/O functions. As with other platforms, we can use those instead. We leave a TODO item to replace all fb_() functions with their regular I/O counterparts throughout the fbdev drivers. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- include/linux/fb.h | 63 +++++++++++----------------------------------- 1 file changed, 15 insertions(+), 48 deletions(-)