mbox series

[0/5] fbdev: Move framebuffer I/O helpers to <asm/fb.h>

Message ID 20230426130420.19942-1-tzimmermann@suse.de (mailing list archive)
Headers show
Series fbdev: Move framebuffer I/O helpers to <asm/fb.h> | expand

Message

Thomas Zimmermann April 26, 2023, 1:04 p.m. UTC
Fbdev provides helpers for framebuffer I/O, such as fb_readl(),
fb_writel() or fb_memcpy_to_fb(). The implementation of each helper
depends on the architecture. It's still all located in fbdev's main
header file <linux/fb.h>. Move all of it into each archtecture's
<asm/fb.h>, with shared code in <asm-generic/fb.h>.

The first patch a simple whitespace cleanup.

Until now, <linux/fb.h> contained an include of <asm/io.h>. As this
will go away patches 2 to 4 prepare include statements in the various
drivers. Source files that use regular I/O helpers, such as readl(),
now include <linux/io.h>. Source files that use framebuffer I/O
helpers, such as fb_readl(), now include <asm/fb.h>. The latter now
includes <linux/io.h> internally.

Patch 5 moves the framebuffer I/O helpers from <linux/fb.h> into
the various architecture headers. The common case, where the framebuffer
is located in I/O memory, serves as the generic implemenation.

The patchset has been built for a variety of platforms, such as x86-64,
arm, aarch64, ppc64, parisc, m64k, mips and sparc.

Thomas Zimmermann (5):
  fbdev/matrox: Remove trailing whitespaces
  ipu-v3: Include <linux/io.h>
  fbdev: Include <linux/io.h> in various drivers
  fbdev: Include <linux/io.h> via <asm/fb.h>
  fbdev: Move framebuffer I/O helpers into <asm/fb.h>

 arch/arc/include/asm/fb.h                   | 29 +++++++
 arch/ia64/include/asm/fb.h                  | 28 +++++++
 arch/loongarch/include/asm/fb.h             | 29 +++++++
 arch/m68k/include/asm/fb.h                  | 29 +++++++
 arch/sparc/include/asm/fb.h                 | 77 +++++++++++++++++
 drivers/gpu/ipu-v3/ipu-prv.h                |  1 +
 drivers/video/fbdev/arcfb.c                 |  1 +
 drivers/video/fbdev/arkfb.c                 |  2 +
 drivers/video/fbdev/aty/atyfb.h             |  2 +
 drivers/video/fbdev/aty/mach64_cursor.c     |  2 +-
 drivers/video/fbdev/chipsfb.c               |  1 +
 drivers/video/fbdev/cirrusfb.c              |  2 +
 drivers/video/fbdev/core/cfbcopyarea.c      |  2 +-
 drivers/video/fbdev/core/cfbfillrect.c      |  1 +
 drivers/video/fbdev/core/cfbimgblt.c        |  1 +
 drivers/video/fbdev/core/svgalib.c          |  3 +-
 drivers/video/fbdev/cyber2000fb.c           |  2 +
 drivers/video/fbdev/ep93xx-fb.c             |  2 +
 drivers/video/fbdev/hgafb.c                 |  3 +-
 drivers/video/fbdev/hitfb.c                 |  2 +-
 drivers/video/fbdev/kyro/fbdev.c            |  3 +-
 drivers/video/fbdev/matrox/matroxfb_accel.c |  8 +-
 drivers/video/fbdev/matrox/matroxfb_base.h  |  6 +-
 drivers/video/fbdev/pm2fb.c                 |  3 +
 drivers/video/fbdev/pm3fb.c                 |  2 +
 drivers/video/fbdev/pvr2fb.c                |  2 +
 drivers/video/fbdev/s3fb.c                  |  2 +
 drivers/video/fbdev/sm712fb.c               |  2 +
 drivers/video/fbdev/sstfb.c                 |  2 +-
 drivers/video/fbdev/stifb.c                 |  2 +
 drivers/video/fbdev/tdfxfb.c                |  3 +-
 drivers/video/fbdev/tridentfb.c             |  2 +
 drivers/video/fbdev/vga16fb.c               |  3 +-
 drivers/video/fbdev/vt8623fb.c              |  2 +
 drivers/video/fbdev/wmt_ge_rops.c           |  2 +
 include/asm-generic/fb.h                    | 93 +++++++++++++++++++++
 include/linux/fb.h                          | 53 ------------
 37 files changed, 340 insertions(+), 69 deletions(-)

Comments

Sam Ravnborg April 26, 2023, 7:21 p.m. UTC | #1
Hi Thomas.

On Wed, Apr 26, 2023 at 03:04:15PM +0200, Thomas Zimmermann wrote:
> Fbdev provides helpers for framebuffer I/O, such as fb_readl(),
> fb_writel() or fb_memcpy_to_fb(). The implementation of each helper
> depends on the architecture. It's still all located in fbdev's main
> header file <linux/fb.h>. Move all of it into each archtecture's
> <asm/fb.h>, with shared code in <asm-generic/fb.h>.

For once I think this cleanup is moving things in the wrong direction.

The fb_* helpers predates the generic io.h support and try to
add a generic layer for read read / write operations.

The right fix would be to migrate fb_* to use the io helpers
we have today - so we use the existing way to handle the architecture
specific details.

From a quick look there seems to be some challenges but the current
helpers that re-do part of io.h is not the way forward and hiding them
in arch/include/asm/fb.h seems counter productive.

	Sam
Thomas Zimmermann April 27, 2023, 7:22 a.m. UTC | #2
Hi Sam

Am 26.04.23 um 21:21 schrieb Sam Ravnborg:
> Hi Thomas.
> 
> On Wed, Apr 26, 2023 at 03:04:15PM +0200, Thomas Zimmermann wrote:
>> Fbdev provides helpers for framebuffer I/O, such as fb_readl(),
>> fb_writel() or fb_memcpy_to_fb(). The implementation of each helper
>> depends on the architecture. It's still all located in fbdev's main
>> header file <linux/fb.h>. Move all of it into each archtecture's
>> <asm/fb.h>, with shared code in <asm-generic/fb.h>.
> 
> For once I think this cleanup is moving things in the wrong direction.
> 
> The fb_* helpers predates the generic io.h support and try to
> add a generic layer for read read / write operations.
> 
> The right fix would be to migrate fb_* to use the io helpers
> we have today - so we use the existing way to handle the architecture
> specific details.

I looked through the existing versions of the fb_() I/O helpers. They 
can apparently be implemented with the regular helpers of similar names.

I'm not sure, but even Sparc looks compatible. At least these sbus_ 
functions seem to be equivalent to the __raw_() I/O helpers of similar 
names. Do you still have that Sparc emulator?

> 
>  From a quick look there seems to be some challenges but the current
> helpers that re-do part of io.h is not the way forward and hiding them
> in arch/include/asm/fb.h seems counter productive.

Which challenges did you see?

Best regards
Thomas

> 
> 	Sam
Arnd Bergmann April 27, 2023, 10:20 a.m. UTC | #3
On Thu, Apr 27, 2023, at 08:22, Thomas Zimmermann wrote:
> Am 26.04.23 um 21:21 schrieb Sam Ravnborg:
>> On Wed, Apr 26, 2023 at 03:04:15PM +0200, Thomas Zimmermann wrote:
>>> Fbdev provides helpers for framebuffer I/O, such as fb_readl(),
>>> fb_writel() or fb_memcpy_to_fb(). The implementation of each helper
>>> depends on the architecture. It's still all located in fbdev's main
>>> header file <linux/fb.h>. Move all of it into each archtecture's
>>> <asm/fb.h>, with shared code in <asm-generic/fb.h>.
>> 
>> For once I think this cleanup is moving things in the wrong direction.
>> 
>> The fb_* helpers predates the generic io.h support and try to
>> add a generic layer for read read / write operations.
>> 
>> The right fix would be to migrate fb_* to use the io helpers
>> we have today - so we use the existing way to handle the architecture
>> specific details.
>
> I looked through the existing versions of the fb_() I/O helpers. They 
> can apparently be implemented with the regular helpers of similar names.
>
> I'm not sure, but even Sparc looks compatible. At least these sbus_ 
> functions seem to be equivalent to the __raw_() I/O helpers of similar 
> names. Do you still have that Sparc emulator?

I looked at the current code and came to the same conclusion: all
architectures we support today do the same thing in __raw_readl()
and fb_readl() etc, so we can completely remove the latter without
changing semantics.

I think the original list was necessary since not all architectures
supported the __raw_ accessors in the past, so they were open-coded
here for the rest. I thought there were also architectures on which
__raw_readl() does a byteswap to reverse the swap done in a PCI
host bridge, but it apears that none of those remain now, if we ever
had them.

     Arnd
Sam Ravnborg April 27, 2023, 5:17 p.m. UTC | #4
Hi Thomas,

On Thu, Apr 27, 2023 at 09:22:47AM +0200, Thomas Zimmermann wrote:
> Hi Sam
> 
> Am 26.04.23 um 21:21 schrieb Sam Ravnborg:
> > Hi Thomas.
> > 
> > On Wed, Apr 26, 2023 at 03:04:15PM +0200, Thomas Zimmermann wrote:
> > > Fbdev provides helpers for framebuffer I/O, such as fb_readl(),
> > > fb_writel() or fb_memcpy_to_fb(). The implementation of each helper
> > > depends on the architecture. It's still all located in fbdev's main
> > > header file <linux/fb.h>. Move all of it into each archtecture's
> > > <asm/fb.h>, with shared code in <asm-generic/fb.h>.
> > 
> > For once I think this cleanup is moving things in the wrong direction.
> > 
> > The fb_* helpers predates the generic io.h support and try to
> > add a generic layer for read read / write operations.
> > 
> > The right fix would be to migrate fb_* to use the io helpers
> > we have today - so we use the existing way to handle the architecture
> > specific details.
> 
> I looked through the existing versions of the fb_() I/O helpers. They can
> apparently be implemented with the regular helpers of similar names.
> 
> I'm not sure, but even Sparc looks compatible. At least these sbus_
> functions seem to be equivalent to the __raw_() I/O helpers of similar
> names.

> Do you still have that Sparc emulator?
I used qemu the last time I played with sparc and saved the instructions
somewhere how to redo it - but that would use to bohcs driver only I think.
I have saprc machines, but none of these are easy to get operational.
We can always ask on sparclinux to get some testing feedback.

> 
> > 
> >  From a quick look there seems to be some challenges but the current
> > helpers that re-do part of io.h is not the way forward and hiding them
> > in arch/include/asm/fb.h seems counter productive.
> 
> Which challenges did you see?
sparc was the main thing - but maybe I did not look close enough.
And then I tried to map the macros to some of the more highlevel ones
from io.h, but as Arnd says the __raw* is the way to go here.

	Sam