diff mbox series

[v2,5/5] fbdev: Define framebuffer I/O from Linux' I/O functions

Message ID 20230428092711.406-6-tzimmermann@suse.de (mailing list archive)
State Not Applicable
Headers show
Series fbdev: Use regular I/O function for framebuffers | expand

Commit Message

Thomas Zimmermann April 28, 2023, 9:27 a.m. UTC
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(-)

Comments

Robin Murphy April 28, 2023, 12:18 p.m. UTC | #1
On 2023-04-28 10:27, Thomas Zimmermann wrote:
> 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(-)
> 
> 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

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

> +#define fb_readb readb
> +#define fb_readw readw
> +#define fb_readl readl
> +#if defined(CONFIG_64BIT)
> +#define fb_readq readq
> +#endif

You probably don't need to bother making these conditional - 32-bit 
architectures aren't forbidden from providing readq/writeq if they 
really want to, and drivers can also use the io-64-nonatomic headers for 
portability. The build will still fail in a sufficiently obvious manner 
if neither is true.

Thanks,
Robin.

> +#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))
Geert Uytterhoeven April 28, 2023, 12:27 p.m. UTC | #2
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:
> > 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(-)
> >
> > 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
>
> 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.

Gr{oetje,eeting}s,

                        Geert
Thomas Zimmermann April 28, 2023, 12:59 p.m. UTC | #3
Hi Robin, Geert

Am 28.04.23 um 14:27 schrieb Geert Uytterhoeven:
> 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:
>>> 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(-)
>>>
>>> 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
>>
>> 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.

Ok, makes sense. But it also means that we won't be able to remove the 
fb_() helpers. If we go with this proposal, I'll add your comments to 
the header file, so it's clear why they're still there.

Best regards
Thomas

> 
> Gr{oetje,eeting}s,
> 
>                          Geert
>
Thomas Zimmermann April 28, 2023, 1:10 p.m. UTC | #4
Hi

Am 28.04.23 um 14:27 schrieb Geert Uytterhoeven:
[...]
> 
> In addition, the non-raw variants may do some extras to guarantee
> ordering, which you do not need on a frame buffer.

Given this comment, should we declare the fb_() helpers in 
<asm-generic/io.h> or <linux/io.h>?

I still don't like the idea of having the functions in <linux/fb.h>. We 
have code in DRM that also accesses framebuffer memory (via 
memcpy_toio()). It would make sense to use the fb_() helpers, if they 
are tailored towards this usecase.

Best regards
Thomas

> 
> So I'd go for the __raw_*() variants everywhere.
> 
> Gr{oetje,eeting}s,
> 
>                          Geert
>
Sam Ravnborg April 28, 2023, 1:12 p.m. UTC | #5
Hi Thomas,

On Fri, Apr 28, 2023 at 11:27:11AM +0200, Thomas Zimmermann wrote:
> 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(-)
> 
> 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.
>   */
When the __raw_* variants are used, as Geert points out, then I think
the memcpy / memset can be replaced, but the rest seems fine to keep.

My personal opinion is that __raw_* is for macro use etc, and not
something to use everywhere. So I like the fb_read/fb_write macros.
But that is just my color of the bikeshed.

	Sam
Arnd Bergmann April 28, 2023, 1:17 p.m. UTC | #6
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

      Arnd
Thomas Zimmermann April 28, 2023, 2:18 p.m. UTC | #7
Hi

Am 28.04.23 um 15:12 schrieb Sam Ravnborg:
> Hi Thomas,
> 
> On Fri, Apr 28, 2023 at 11:27:11AM +0200, Thomas Zimmermann wrote:
>> 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(-)
>>
>> 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.
>>    */
> When the __raw_* variants are used, as Geert points out, then I think
> the memcpy / memset can be replaced, but the rest seems fine to keep.

I'd either want the regular I/O functions or the fb_() wrappers, but not 
the __raw_() function. I'd also prefer to keep fb_ in front of memset 
and memcpy.  I'd be happy to have fb_() wrappers that are I/O helpers 
without ordering guarantees. I'd just wouldn't want them in <linux/fb.h>

Best regards
Thomas

> 
> My personal opinion is that __raw_* is for macro use etc, and not
> something to use everywhere. So I like the fb_read/fb_write macros.
> But that is just my color of the bikeshed.
> 
> 	Sam
Sam Ravnborg April 28, 2023, 4:54 p.m. UTC | #8
Hi Thomas,

On Fri, Apr 28, 2023 at 04:18:38PM +0200, Thomas Zimmermann wrote:
> I'd be happy to have fb_() wrappers that are I/O helpers without
> ordering guarantees. I'd just wouldn't want them in <linux/fb.h>

How about throwing them into a new drm_fb.h header file.
This header file could be the home for all the fb stuff that is
shared between drm and the legacy fbdev.

Then we may slowly migrate more fbdev stuff to drm and let the legacy
fbdev stuff use the maintained drm stuff.
Dunno, the pain may not be worth it.

	Sam
Thomas Zimmermann April 29, 2023, 12:26 p.m. UTC | #9
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
Thomas Zimmermann April 29, 2023, 12:28 p.m. UTC | #10
Hi Sam

Am 28.04.23 um 18:54 schrieb Sam Ravnborg:
> Hi Thomas,
> 
> On Fri, Apr 28, 2023 at 04:18:38PM +0200, Thomas Zimmermann wrote:
>> I'd be happy to have fb_() wrappers that are I/O helpers without
>> ordering guarantees. I'd just wouldn't want them in <linux/fb.h>
> 
> How about throwing them into a new drm_fb.h header file.
> This header file could be the home for all the fb stuff that is
> shared between drm and the legacy fbdev.
> 
> Then we may slowly migrate more fbdev stuff to drm and let the legacy
> fbdev stuff use the maintained drm stuff.
> Dunno, the pain may not be worth it.

DRM might not be relevant if we can remove fb_mem*(). So I'd like to go 
back to something closer to v1 of these patches. See my reply to Arnd.

Best regards
Thomas

> 
> 	Sam
Arnd Bergmann April 29, 2023, 2:11 p.m. UTC | #11
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 mbox series

Patch

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