| Message ID | 20230502130223.14719-6-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fbdev: Move framebuffer I/O helpers to <asm/fb.h> | expand |
Hi Thomas, On Tue, May 02, 2023 at 03:02:22PM +0200, Thomas Zimmermann wrote: > Implement framebuffer I/O helpers, such as fb_read*() and fb_write*(), > in the architecture's <asm/fb.h> header file or the generic one. In reality they are now all implemented in the generic one. > > The common case has been the use of regular I/O functions, such as > __raw_readb() or memset_io(). A few architectures used plain system- > memory reads and writes. Sparc used helpers for its SBus. > > The architectures that used special cases provide the same code in > their __raw_*() I/O helpers. So the patch replaces this code with the > __raw_*() functions and moves it to <asm-generic/fb.h> for all > architectures. Which is also documented here. > > v3: > * implement all architectures with generic helpers > * support reordering and native byte order (Geert, Arnd) > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > include/asm-generic/fb.h | 101 +++++++++++++++++++++++++++++++++++++++ > include/linux/fb.h | 53 -------------------- > 2 files changed, 101 insertions(+), 53 deletions(-) > > diff --git a/include/asm-generic/fb.h b/include/asm-generic/fb.h > index 6922dd248c51..0540eccdbeca 100644 > --- a/include/asm-generic/fb.h > +++ b/include/asm-generic/fb.h > @@ -31,4 +31,105 @@ static inline int fb_is_primary_device(struct fb_info *info) > } > #endif > > +/* > + * I/O helpers for the framebuffer. Prefer these functions over their > + * regular counterparts. The regular I/O functions provide in-order > + * access and swap bytes to/from little-endian ordering. Neither is > + * required for framebuffers. Instead, the helpers read and write > + * raw framebuffer data. Independent operations can be reordered for > + * improved performance. > + */ > + > +#ifndef fb_readb > +static inline u8 fb_readb(const volatile void __iomem *addr) > +{ > + return __raw_readb(addr); > +} > +#define fb_readb fb_readb > +#endif When we need to provide an architecture specific variant the #ifndef foo ... #define foo foo can be added. Right now it is just noise as no architectures provide their own variants. But I am missing something somewhere as I cannot see how this builds. asm-generic now provide the fb_read/fb_write helpers. But for example sparc has an architecture specifc fb.h so it will not use the asm-generic variant. So I wonder how sparc get hold of the asm-generic fb.h file? Maybe it is obvious, but I miss it. Sam
On Tue, May 2, 2023, at 15:02, Thomas Zimmermann wrote: > Implement framebuffer I/O helpers, such as fb_read*() and fb_write*(), > in the architecture's <asm/fb.h> header file or the generic one. > > The common case has been the use of regular I/O functions, such as > __raw_readb() or memset_io(). A few architectures used plain system- > memory reads and writes. Sparc used helpers for its SBus. > > The architectures that used special cases provide the same code in > their __raw_*() I/O helpers. So the patch replaces this code with the > __raw_*() functions and moves it to <asm-generic/fb.h> for all > architectures. > > v3: > * implement all architectures with generic helpers > * support reordering and native byte order (Geert, Arnd) This looks good for the read/write helpers, but I'm a little worried about the memset and memcpy functions, since they do change behavior on some architectures: - on sparc64, fb_mem{set,cpy} uses ASI_PHYS_BYPASS_EC_E (like __raw_readb) while mem{set_,cpy_from,cpy_to} uses ASI_PHYS_BYPASS_EC_E_L (like readb) I don't know the effect of that, but it seems intentional - on loongarch and csky, the _io variants avoid unaligned access, while the normal memcpy/memset is probably broken, so your patch is a bugfix - on ia64, the _io variants use bytewise access and avoid any longer loads and stores, so your patch probably makes things slower. It's probably safe to deal with all the above by either adding architecture specific overrides to the current version, or by doing the semantic changes before the move to asm/fb.h, but one way or the other I'd prefer this to be separate from the consolidation patch that should not have any changes in behavior. Arnd
Hi Sam Am 02.05.23 um 22:03 schrieb Sam Ravnborg: > Hi Thomas, > > On Tue, May 02, 2023 at 03:02:22PM +0200, Thomas Zimmermann wrote: >> Implement framebuffer I/O helpers, such as fb_read*() and fb_write*(), >> in the architecture's <asm/fb.h> header file or the generic one. > > In reality they are now all implemented in the generic one. > >> >> The common case has been the use of regular I/O functions, such as >> __raw_readb() or memset_io(). A few architectures used plain system- >> memory reads and writes. Sparc used helpers for its SBus. >> >> The architectures that used special cases provide the same code in >> their __raw_*() I/O helpers. So the patch replaces this code with the >> __raw_*() functions and moves it to <asm-generic/fb.h> for all >> architectures. > Which is also documented here. The first paragraph documents the design intention, the other one the implementation. But I agree that it's not well phrased. I'll see if I can improve the text. > >> >> v3: >> * implement all architectures with generic helpers >> * support reordering and native byte order (Geert, Arnd) >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> include/asm-generic/fb.h | 101 +++++++++++++++++++++++++++++++++++++++ >> include/linux/fb.h | 53 -------------------- >> 2 files changed, 101 insertions(+), 53 deletions(-) >> >> diff --git a/include/asm-generic/fb.h b/include/asm-generic/fb.h >> index 6922dd248c51..0540eccdbeca 100644 >> --- a/include/asm-generic/fb.h >> +++ b/include/asm-generic/fb.h >> @@ -31,4 +31,105 @@ static inline int fb_is_primary_device(struct fb_info *info) >> } >> #endif >> >> +/* >> + * I/O helpers for the framebuffer. Prefer these functions over their >> + * regular counterparts. The regular I/O functions provide in-order >> + * access and swap bytes to/from little-endian ordering. Neither is >> + * required for framebuffers. Instead, the helpers read and write >> + * raw framebuffer data. Independent operations can be reordered for >> + * improved performance. >> + */ >> + >> +#ifndef fb_readb >> +static inline u8 fb_readb(const volatile void __iomem *addr) >> +{ >> + return __raw_readb(addr); >> +} >> +#define fb_readb fb_readb >> +#endif > > When we need to provide an architecture specific variant the > #ifndef foo > ... > #define foo foo > can be added. Right now it is just noise as no architectures provide > their own variants. Given Arnd's comments, this might change. It also makes sense from a design POV. > > But I am missing something somewhere as I cannot see how this builds. > asm-generic now provide the fb_read/fb_write helpers. > But for example sparc has an architecture specifc fb.h so it will not > use the asm-generic variant. So I wonder how sparc get hold of the > asm-generic fb.h file? All architecture's <asm/fb.h> files include <asm-generic/fb.h>, so that they all get the interfaces which they don't define themselves. For Sparc, this is at [1]. Best regards Thomas [1] https://cgit.freedesktop.org/drm/drm-tip/tree/arch/sparc/include/asm/fb.h#n19 > > Maybe it is obvious, but I miss it. > > Sam
Hi Am 02.05.23 um 22:06 schrieb Arnd Bergmann: > On Tue, May 2, 2023, at 15:02, Thomas Zimmermann wrote: >> Implement framebuffer I/O helpers, such as fb_read*() and fb_write*(), >> in the architecture's <asm/fb.h> header file or the generic one. >> >> The common case has been the use of regular I/O functions, such as >> __raw_readb() or memset_io(). A few architectures used plain system- >> memory reads and writes. Sparc used helpers for its SBus. >> >> The architectures that used special cases provide the same code in >> their __raw_*() I/O helpers. So the patch replaces this code with the >> __raw_*() functions and moves it to <asm-generic/fb.h> for all >> architectures. >> >> v3: >> * implement all architectures with generic helpers >> * support reordering and native byte order (Geert, Arnd) > > This looks good for the read/write helpers, but I'm a little > worried about the memset and memcpy functions, since they do > change behavior on some architectures: > > - on sparc64, fb_mem{set,cpy} uses ASI_PHYS_BYPASS_EC_E (like __raw_readb) > while mem{set_,cpy_from,cpy_to} uses ASI_PHYS_BYPASS_EC_E_L (like readb) > I don't know the effect of that, but it seems intentional > > - on loongarch and csky, the _io variants avoid unaligned access, > while the normal memcpy/memset is probably broken, so your > patch is a bugfix > > - on ia64, the _io variants use bytewise access and avoid any longer > loads and stores, so your patch probably makes things slower. > > It's probably safe to deal with all the above by either adding > architecture specific overrides to the current version, or > by doing the semantic changes before the move to asm/fb.h, but > one way or the other I'd prefer this to be separate from the > consolidation patch that should not have any changes in behavior. I think I'll add architecture overrides that contain the current code, even if they contain some force-casting wrt __iomem. If anyone wants to fix the issues, they can then address them easily. Best regards Thomas > > Arnd
On Wed, May 3, 2023, at 16:55, Thomas Zimmermann wrote: > Am 02.05.23 um 22:06 schrieb Arnd Bergmann: >> It's probably safe to deal with all the above by either adding >> architecture specific overrides to the current version, or >> by doing the semantic changes before the move to asm/fb.h, but >> one way or the other I'd prefer this to be separate from the >> consolidation patch that should not have any changes in behavior. > > I think I'll add architecture overrides that contain the current code, > even if they contain some force-casting wrt __iomem. If anyone wants to > fix the issues, they can then address them easily. Ok, sounds good, Arnd
Hi Thomas, > > But I am missing something somewhere as I cannot see how this builds. > > asm-generic now provide the fb_read/fb_write helpers. > > But for example sparc has an architecture specifc fb.h so it will not > > use the asm-generic variant. So I wonder how sparc get hold of the > > asm-generic fb.h file? > > All architecture's <asm/fb.h> files include <asm-generic/fb.h>, so that they > all get the interfaces which they don't define themselves. For Sparc, this > is at [1]. > > Best regards > Thomas > > > [1] > https://cgit.freedesktop.org/drm/drm-tip/tree/arch/sparc/include/asm/fb.h#n19 > > > > > Maybe it is obvious, but I miss it. OK, it was obvious and I missed it. I looked at the mainline kernel, and not the drm-tip variant. Sorry for the noise. Sam
diff --git a/include/asm-generic/fb.h b/include/asm-generic/fb.h index 6922dd248c51..0540eccdbeca 100644 --- a/include/asm-generic/fb.h +++ b/include/asm-generic/fb.h @@ -31,4 +31,105 @@ static inline int fb_is_primary_device(struct fb_info *info) } #endif +/* + * I/O helpers for the framebuffer. Prefer these functions over their + * regular counterparts. The regular I/O functions provide in-order + * access and swap bytes to/from little-endian ordering. Neither is + * required for framebuffers. Instead, the helpers read and write + * raw framebuffer data. Independent operations can be reordered for + * improved performance. + */ + +#ifndef fb_readb +static inline u8 fb_readb(const volatile void __iomem *addr) +{ + return __raw_readb(addr); +} +#define fb_readb fb_readb +#endif + +#ifndef fb_readw +static inline u16 fb_readw(const volatile void __iomem *addr) +{ + return __raw_readw(addr); +} +#define fb_readw fb_readw +#endif + +#ifndef fb_readl +static inline u32 fb_readl(const volatile void __iomem *addr) +{ + return __raw_readl(addr); +} +#define fb_readl fb_readl +#endif + +#ifndef fb_readq +#if defined(__raw_readq) +static inline u64 fb_readq(const volatile void __iomem *addr) +{ + return __raw_readq(addr); +} +#define fb_readq fb_readq +#endif +#endif + +#ifndef fb_writeb +static inline void fb_writeb(u8 b, volatile void __iomem *addr) +{ + __raw_writeb(b, addr); +} +#define fb_writeb fb_writeb +#endif + +#ifndef fb_writew +static inline void fb_writew(u16 b, volatile void __iomem *addr) +{ + __raw_writew(b, addr); +} +#define fb_writew fb_writew +#endif + +#ifndef fb_writel +static inline void fb_writel(u32 b, volatile void __iomem *addr) +{ + __raw_writel(b, addr); +} +#define fb_writel fb_writel +#endif + +#ifndef fb_writeq +#if defined(__raw_writeq) +static inline void fb_writeq(u64 b, volatile void __iomem *addr) +{ + __raw_writeq(b, addr); +} +#define fb_writeq fb_writeq +#endif +#endif + +#ifndef fb_memcpy_fromfb +static inline void fb_memcpy_fromfb(void *to, const volatile void __iomem *from, size_t n) +{ + memcpy_fromio(to, from, n); +} +#define fb_memcpy_fromfb fb_memcpy_fromfb +#endif + +#ifndef fb_memcpy_tofb +static inline void fb_memcpy_tofb(volatile void __iomem *to, const void *from, size_t n) +{ + memcpy_toio(to, from, n); +} +#define fb_memcpy_tofb fb_memcpy_tofb +#endif + +#ifndef fb_memset +static inline void fb_memset(volatile void __iomem *addr, int c, size_t n) +{ + memset_io(addr, c, n); +} +#define fb_memset fb_memset +#endif + #endif /* __ASM_GENERIC_FB_H_ */ diff --git a/include/linux/fb.h b/include/linux/fb.h index 08cb47da71f8..7d80ee62a9d5 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,6 @@ 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. - */ - -// 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_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*(), in the architecture's <asm/fb.h> header file or the generic one. The common case has been the use of regular I/O functions, such as __raw_readb() or memset_io(). A few architectures used plain system- memory reads and writes. Sparc used helpers for its SBus. The architectures that used special cases provide the same code in their __raw_*() I/O helpers. So the patch replaces this code with the __raw_*() functions and moves it to <asm-generic/fb.h> for all architectures. v3: * implement all architectures with generic helpers * support reordering and native byte order (Geert, Arnd) Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- include/asm-generic/fb.h | 101 +++++++++++++++++++++++++++++++++++++++ include/linux/fb.h | 53 -------------------- 2 files changed, 101 insertions(+), 53 deletions(-)