mbox series

[00/47] fbdev: Use I/O helpers

Message ID 20230728182234.10680-1-tzimmermann@suse.de (mailing list archive)
Headers show
Series fbdev: Use I/O helpers | expand

Message

Thomas Zimmermann July 28, 2023, 4:39 p.m. UTC
Most fbdev drivers operate on I/O memory. And most of those use the
default implementations for file I/O and console drawing. Convert all
these low-hanging fruits to the fb_ops initializer macro and Kconfig
token for fbdev I/O helpers.

The fbdev I/O helpers are easily grep-able. In a later patch, they can
be left to empty values if the rsp. funtionality, such as file I/O or
console, has been disabled.

There are no functional changes. The helpers set the defaults that
the drivers already use.

Thomas Zimmermann (47):
  media/vivid: Use fbdev I/O helpers
  fbdev/acornfb: Use fbdev I/O helpers
  fbdev/asiliantfb: Use fbdev I/O helpers
  fbdev/atmel_lcdfb: Use fbdev I/O helpers
  fbdev/aty128fb: Use fbdev I/O helpers
  fbdev/carminefb: Use fbdev I/O helpers
  fbdev/chipsfb: Use fbdev I/O helpers
  fbdev/da8xx-fb: Use fbdev I/O helpers
  fbdev/efifb: Use fbdev I/O helpers
  fbdev/fm2fb: Use fbdev I/O helpers
  fbdev/fsl-diu-fb: Use fbdev I/O helpers
  fbdev/g364fb: Use fbdev I/O helpers
  fbdev/geode/gx1fb: Use fbdev I/O helpers
  fbdev/geode/gxfb: Use fbdev I/O helpers
  fbdev/geode/lxfb: Use fbdev I/O helpers
  fbdev/goldfishfb: Use fbdev I/O helpers
  fbdev/grvga: Use fbdev I/O helpers
  fbdev/gxt4500: Use fbdev I/O helpers
  fbdev/i740fb: Use fbdev I/O helpers
  fbdev/imxfb: Use fbdev I/O helpers
  fbdev/kyro: Use fbdev I/O helpers
  fbdev/macfb: Use fbdev I/O helpers
  fbdev/maxinefb: Use fbdev I/O helpers
  fbdev/mb862xxfb: Use fbdev I/O helpers
  fbdev/mmpfb: Use fbdev I/O helpers
  fbdev/mx3fb: Use fbdev I/O helpers
  fbdev/ocfb: Use fbdev I/O helpers
  fbdev/offb: Use fbdev I/O helpers
  fbdev/omapfb: Use fbdev I/O helpers
  fbdev/platinumfb: Use fbdev I/O helpers
  fbdev/pmag-aa-fb: Use fbdev I/O helpers
  fbdev/pmag-ba-fb: Use fbdev I/O helpers
  fbdev/pmag-b-fb: Use fbdev I/O helpers
  fbdev/pxa168fb: Use fbdev I/O helpers
  fbdev/pxafb: Use fbdev I/O helpers
  fbdev/q40fb: Use fbdev I/O helpers
  fbdev/s3cfb: Use fbdev I/O helpers
  fbdev/sh7760fb: Use fbdev I/O helpers
  fbdev/simplefb: Use fbdev I/O helpers
  fbdev/sstfb: Use fbdev I/O helpers
  fbdev/sunxvr1000: Use fbdev I/O helpers
  fbdev/sunxvr2500: Use fbdev I/O helpers
  fbdev/uvesafb: Use fbdev I/O helpers
  fbdev/valkyriefb: Use fbdev I/O helpers
  fbdev/vesafb: Use fbdev I/O helpers
  fbdev/xilinxfb: Use fbdev I/O helpers
  vfio-dev/mdpy-fb: Use fbdev I/O helpers

 drivers/media/test-drivers/vivid/Kconfig     |   4 +-
 drivers/media/test-drivers/vivid/vivid-osd.c |   4 +-
 drivers/video/fbdev/Kconfig                  | 160 +++++--------------
 drivers/video/fbdev/acornfb.c                |   4 +-
 drivers/video/fbdev/asiliantfb.c             |   4 +-
 drivers/video/fbdev/atmel_lcdfb.c            |   4 +-
 drivers/video/fbdev/aty/aty128fb.c           |   4 +-
 drivers/video/fbdev/carminefb.c              |   5 +-
 drivers/video/fbdev/chipsfb.c                |   4 +-
 drivers/video/fbdev/da8xx-fb.c               |   4 +-
 drivers/video/fbdev/efifb.c                  |   4 +-
 drivers/video/fbdev/fm2fb.c                  |   4 +-
 drivers/video/fbdev/fsl-diu-fb.c             |   4 +-
 drivers/video/fbdev/g364fb.c                 |   4 +-
 drivers/video/fbdev/geode/Kconfig            |  12 +-
 drivers/video/fbdev/geode/gx1fb_core.c       |   5 +-
 drivers/video/fbdev/geode/gxfb_core.c        |   5 +-
 drivers/video/fbdev/geode/lxfb_core.c        |   5 +-
 drivers/video/fbdev/goldfishfb.c             |   4 +-
 drivers/video/fbdev/grvga.c                  |   4 +-
 drivers/video/fbdev/gxt4500.c                |   4 +-
 drivers/video/fbdev/i740fb.c                 |   4 +-
 drivers/video/fbdev/imxfb.c                  |   4 +-
 drivers/video/fbdev/kyro/fbdev.c             |   4 +-
 drivers/video/fbdev/macfb.c                  |   4 +-
 drivers/video/fbdev/maxinefb.c               |   4 +-
 drivers/video/fbdev/mb862xx/mb862xxfbdrv.c   |   4 +-
 drivers/video/fbdev/mmp/fb/Kconfig           |   4 +-
 drivers/video/fbdev/mmp/fb/mmpfb.c           |   4 +-
 drivers/video/fbdev/mx3fb.c                  |   4 +-
 drivers/video/fbdev/ocfb.c                   |   4 +-
 drivers/video/fbdev/offb.c                   |   4 +-
 drivers/video/fbdev/omap/Kconfig             |   4 +-
 drivers/video/fbdev/omap/omapfb_main.c       |   4 +-
 drivers/video/fbdev/platinumfb.c             |   4 +-
 drivers/video/fbdev/pmag-aa-fb.c             |   4 +-
 drivers/video/fbdev/pmag-ba-fb.c             |   4 +-
 drivers/video/fbdev/pmagb-b-fb.c             |   4 +-
 drivers/video/fbdev/pxa168fb.c               |   4 +-
 drivers/video/fbdev/pxafb.c                  |   4 +-
 drivers/video/fbdev/q40fb.c                  |   4 +-
 drivers/video/fbdev/s3c-fb.c                 |   4 +-
 drivers/video/fbdev/sh7760fb.c               |   4 +-
 drivers/video/fbdev/simplefb.c               |   4 +-
 drivers/video/fbdev/sstfb.c                  |   4 +-
 drivers/video/fbdev/sunxvr1000.c             |   4 +-
 drivers/video/fbdev/sunxvr2500.c             |   4 +-
 drivers/video/fbdev/uvesafb.c                |   4 +-
 drivers/video/fbdev/valkyriefb.c             |   4 +-
 drivers/video/fbdev/vesafb.c                 |   4 +-
 drivers/video/fbdev/xilinxfb.c               |   4 +-
 samples/Kconfig                              |   4 +-
 samples/vfio-mdev/mdpy-fb.c                  |   4 +-
 53 files changed, 94 insertions(+), 286 deletions(-)


base-commit: fba8a13ec9ae1a7175cc0dda7235b3d2df0f0f90

Comments

Sam Ravnborg July 28, 2023, 6:35 p.m. UTC | #1
Hi Thomas,

On Fri, Jul 28, 2023 at 06:39:43PM +0200, Thomas Zimmermann wrote:
> Most fbdev drivers operate on I/O memory. And most of those use the
> default implementations for file I/O and console drawing. Convert all
> these low-hanging fruits to the fb_ops initializer macro and Kconfig
> token for fbdev I/O helpers.
> 
> The fbdev I/O helpers are easily grep-able. In a later patch, they can
> be left to empty values if the rsp. funtionality, such as file I/O or
> console, has been disabled.
> 
> There are no functional changes. The helpers set the defaults that
> the drivers already use.

I have browsed all patches - they all looks good.
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
Sam Ravnborg July 28, 2023, 6:39 p.m. UTC | #2
Hi Thomas,

On Fri, Jul 28, 2023 at 06:39:43PM +0200, Thomas Zimmermann wrote:
> Most fbdev drivers operate on I/O memory. And most of those use the
> default implementations for file I/O and console drawing. Convert all
> these low-hanging fruits to the fb_ops initializer macro and Kconfig
> token for fbdev I/O helpers.
> 
> The fbdev I/O helpers are easily grep-able. In a later patch, they can
> be left to empty values if the rsp. funtionality, such as file I/O or
> console, has been disabled.

Did you miss sm750 or was it left out on purpose?
As it hide in staging it is easy to miss.

	Sam
Helge Deller July 28, 2023, 6:46 p.m. UTC | #3
On 7/28/23 18:39, Thomas Zimmermann wrote:
> Most fbdev drivers operate on I/O memory.

Just nitpicking here:
What is I/O memory?
Isn't it either memory, or I/O ?
I mean, I would never think of the cfb* draw functions under I/O.

> And most of those use the
> default implementations for file I/O and console drawing. Convert all
> these low-hanging fruits to the fb_ops initializer macro and Kconfig
> token for fbdev I/O helpers.

I do see the motivation for your patch, but I think the
macro names are very misleading.

You have:
#define __FB_DEFAULT_IO_OPS_RDWR \
         .fb_read        = fb_io_read, \
         .fb_write       = fb_io_write

#define __FB_DEFAULT_IO_OPS_DRAW \
         .fb_fillrect    = cfb_fillrect, \
         .fb_copyarea    = cfb_copyarea, \
         .fb_imageblit   = cfb_imageblit

#define __FB_DEFAULT_IO_OPS_MMAP \
         .fb_mmap        = NULL /* default implementation */

#define FB_DEFAULT_IO_OPS \
         __FB_DEFAULT_IO_OPS_RDWR, \
         __FB_DEFAULT_IO_OPS_DRAW, \
         __FB_DEFAULT_IO_OPS_MMAP

I think FB_DEFAULT_IO_OPS is OK for read/write/mmap.
But I would suggest to split out __FB_DEFAULT_IO_OPS_DRAW.
Something like:
#define FB_DEFAULT_IO_OPS \
         __FB_DEFAULT_IO_OPS_RDWR, \
         __FB_DEFAULT_IO_OPS_MMAP
#define FB_DEFAULT_CFB_OPS \
         .fb_fillrect    = cfb_fillrect, \
         .fb_copyarea    = cfb_copyarea, \
         .fb_imageblit   = cfb_imageblit

and then add FB_DEFAULT_IO_OPS *and* FB_DEFAULT_CFB_OPS
to the various struct fb_ops in the drivers.

Helge
Sam Ravnborg July 28, 2023, 9:01 p.m. UTC | #4
Hi Helge,

On Fri, Jul 28, 2023 at 08:46:59PM +0200, Helge Deller wrote:
> On 7/28/23 18:39, Thomas Zimmermann wrote:
> > Most fbdev drivers operate on I/O memory.
> 
> Just nitpicking here:
> What is I/O memory?
> Isn't it either memory, or I/O ?
> I mean, I would never think of the cfb* draw functions under I/O.
> 
> > And most of those use the
> > default implementations for file I/O and console drawing. Convert all
> > these low-hanging fruits to the fb_ops initializer macro and Kconfig
> > token for fbdev I/O helpers.
> 
> I do see the motivation for your patch, but I think the
> macro names are very misleading.
> 
> You have:
> #define __FB_DEFAULT_IO_OPS_RDWR \
>         .fb_read        = fb_io_read, \
>         .fb_write       = fb_io_write
> 
> #define __FB_DEFAULT_IO_OPS_DRAW \
>         .fb_fillrect    = cfb_fillrect, \
>         .fb_copyarea    = cfb_copyarea, \
>         .fb_imageblit   = cfb_imageblit
> 
> #define __FB_DEFAULT_IO_OPS_MMAP \
>         .fb_mmap        = NULL /* default implementation */
> 
> #define FB_DEFAULT_IO_OPS \
>         __FB_DEFAULT_IO_OPS_RDWR, \
>         __FB_DEFAULT_IO_OPS_DRAW, \
>         __FB_DEFAULT_IO_OPS_MMAP
> 
> I think FB_DEFAULT_IO_OPS is OK for read/write/mmap.
> But I would suggest to split out __FB_DEFAULT_IO_OPS_DRAW.
> Something like:
> #define FB_DEFAULT_IO_OPS \
>         __FB_DEFAULT_IO_OPS_RDWR, \
>         __FB_DEFAULT_IO_OPS_MMAP


> #define FB_DEFAULT_CFB_OPS \
>         .fb_fillrect    = cfb_fillrect, \
>         .fb_copyarea    = cfb_copyarea, \
>         .fb_imageblit   = cfb_imageblit

The prefix cfb, I have recently learned, equals color frame buffer.
They are named such for purely historical reasons.

What is important is where the data are copied as we have two
implementations of for example copyarea - one using system memory, the
other using IO memory.

The naming FB_DEFAULT_IO_OPS says this is the defaults to IO memory
operations, which tell what they do and avoid the strange cfb acronym.

Reserve cfb for color frame buffers only - and maybe in the end rename
the three cfbcopyarea, cfbfillrect, cfbimgblt to use the io prefix.
Which is much simpler to do after this series - and nice extra benefit.

I hope this properly explains why I like the current naming and
acked it when the macros were introduced.

	Sam
Helge Deller July 29, 2023, 6:51 a.m. UTC | #5
On 7/28/23 23:01, Sam Ravnborg wrote:
> Hi Helge,
>
> On Fri, Jul 28, 2023 at 08:46:59PM +0200, Helge Deller wrote:
>> On 7/28/23 18:39, Thomas Zimmermann wrote:
>>> Most fbdev drivers operate on I/O memory.
>>
>> Just nitpicking here:
>> What is I/O memory?
>> Isn't it either memory, or I/O ?
>> I mean, I would never think of the cfb* draw functions under I/O.
>>
>>> And most of those use the
>>> default implementations for file I/O and console drawing. Convert all
>>> these low-hanging fruits to the fb_ops initializer macro and Kconfig
>>> token for fbdev I/O helpers.
>>
>> I do see the motivation for your patch, but I think the
>> macro names are very misleading.
>>
>> You have:
>> #define __FB_DEFAULT_IO_OPS_RDWR \
>>          .fb_read        = fb_io_read, \
>>          .fb_write       = fb_io_write
>>
>> #define __FB_DEFAULT_IO_OPS_DRAW \
>>          .fb_fillrect    = cfb_fillrect, \
>>          .fb_copyarea    = cfb_copyarea, \
>>          .fb_imageblit   = cfb_imageblit
>>
>> #define __FB_DEFAULT_IO_OPS_MMAP \
>>          .fb_mmap        = NULL /* default implementation */
>>
>> #define FB_DEFAULT_IO_OPS \
>>          __FB_DEFAULT_IO_OPS_RDWR, \
>>          __FB_DEFAULT_IO_OPS_DRAW, \
>>          __FB_DEFAULT_IO_OPS_MMAP
>>
>> I think FB_DEFAULT_IO_OPS is OK for read/write/mmap.
>> But I would suggest to split out __FB_DEFAULT_IO_OPS_DRAW.
>> Something like:
>> #define FB_DEFAULT_IO_OPS \
>>          __FB_DEFAULT_IO_OPS_RDWR, \
>>          __FB_DEFAULT_IO_OPS_MMAP
>
>
>> #define FB_DEFAULT_CFB_OPS \
>>          .fb_fillrect    = cfb_fillrect, \
>>          .fb_copyarea    = cfb_copyarea, \
>>          .fb_imageblit   = cfb_imageblit
>
> The prefix cfb, I have recently learned, equals color frame buffer.

correct.

> They are named such for purely historical reasons.

well, they operate on MEMORY which represents a (color) frame buffer,
either in host memory or memory on some card on some bus.
So, the naming cfb is not historical, but even today correct.

> What is important is where the data are copied as we have two
> implementations of for example copyarea - one using system memory, the
> other using IO memory.

sys_copyarea() and cfb_copyarea().

> The naming FB_DEFAULT_IO_OPS says this is the defaults to IO memory
> operations, which tell what they do

This is exactly what I find misleading. IO_OPS sounds that it operates
on file I/O (like file read/write), but not on iomem.

> and avoid the strange cfb acronym.

> Reserve cfb for color frame buffers only - and maybe in the end rename
> the three cfbcopyarea, cfbfillrect, cfbimgblt to use the io prefix.

Again, the io prefix is what I think is misleading.
Why not name it what it really is and what is used in the kernel already, e.g.
iomem_copyarea() and sysmem_copyarea().
which would lead to
FB_DEFAULT_IOMEM_OPS and FB_DEFAULT_SYSMEM_OPS.

> Which is much simpler to do after this series - and nice extra benefit.
>
> I hope this properly explains why I like the current naming and
> acked it when the macros were introduced.

IMHO the naming isn't perfect, but that's just nitpicking.
Besides that, Thomas' patches are a nice cleanup.
So, if you want add a
Acked-by: Helge Deller <deller@gmx.de>
to the series.

Helge
Thomas Zimmermann July 29, 2023, 1:13 p.m. UTC | #6
Hi Sam

Am 28.07.23 um 20:39 schrieb Sam Ravnborg:
> Hi Thomas,
> 
> On Fri, Jul 28, 2023 at 06:39:43PM +0200, Thomas Zimmermann wrote:
>> Most fbdev drivers operate on I/O memory. And most of those use the
>> default implementations for file I/O and console drawing. Convert all
>> these low-hanging fruits to the fb_ops initializer macro and Kconfig
>> token for fbdev I/O helpers.
>>
>> The fbdev I/O helpers are easily grep-able. In a later patch, they can
>> be left to empty values if the rsp. funtionality, such as file I/O or
>> console, has been disabled.
> 
> Did you miss sm750 or was it left out on purpose?

I did miss it indeed. I'll add the patch to the next iteration. Thanks 
for noticing.

Best regards
Thomas

> As it hide in staging it is easy to miss.
> 
> 	Sam
Thomas Zimmermann July 29, 2023, 1:21 p.m. UTC | #7
Hi Helge

Am 29.07.23 um 08:51 schrieb Helge Deller:
> On 7/28/23 23:01, Sam Ravnborg wrote:
>> Hi Helge,
>>
>> On Fri, Jul 28, 2023 at 08:46:59PM +0200, Helge Deller wrote:
>>> On 7/28/23 18:39, Thomas Zimmermann wrote:
>>>> Most fbdev drivers operate on I/O memory.
>>>
>>> Just nitpicking here:
>>> What is I/O memory?
>>> Isn't it either memory, or I/O ?
>>> I mean, I would never think of the cfb* draw functions under I/O.
>>>
>>>> And most of those use the
>>>> default implementations for file I/O and console drawing. Convert all
>>>> these low-hanging fruits to the fb_ops initializer macro and Kconfig
>>>> token for fbdev I/O helpers.
>>>
>>> I do see the motivation for your patch, but I think the
>>> macro names are very misleading.
>>>
>>> You have:
>>> #define __FB_DEFAULT_IO_OPS_RDWR \
>>>          .fb_read        = fb_io_read, \
>>>          .fb_write       = fb_io_write
>>>
>>> #define __FB_DEFAULT_IO_OPS_DRAW \
>>>          .fb_fillrect    = cfb_fillrect, \
>>>          .fb_copyarea    = cfb_copyarea, \
>>>          .fb_imageblit   = cfb_imageblit
>>>
>>> #define __FB_DEFAULT_IO_OPS_MMAP \
>>>          .fb_mmap        = NULL /* default implementation */
>>>
>>> #define FB_DEFAULT_IO_OPS \
>>>          __FB_DEFAULT_IO_OPS_RDWR, \
>>>          __FB_DEFAULT_IO_OPS_DRAW, \
>>>          __FB_DEFAULT_IO_OPS_MMAP
>>>
>>> I think FB_DEFAULT_IO_OPS is OK for read/write/mmap.
>>> But I would suggest to split out __FB_DEFAULT_IO_OPS_DRAW.
>>> Something like:
>>> #define FB_DEFAULT_IO_OPS \
>>>          __FB_DEFAULT_IO_OPS_RDWR, \
>>>          __FB_DEFAULT_IO_OPS_MMAP
>>
>>
>>> #define FB_DEFAULT_CFB_OPS \
>>>          .fb_fillrect    = cfb_fillrect, \
>>>          .fb_copyarea    = cfb_copyarea, \
>>>          .fb_imageblit   = cfb_imageblit
>>
>> The prefix cfb, I have recently learned, equals color frame buffer.
> 
> correct.
> 
>> They are named such for purely historical reasons.
> 
> well, they operate on MEMORY which represents a (color) frame buffer,
> either in host memory or memory on some card on some bus.
> So, the naming cfb is not historical, but even today correct.
> 
>> What is important is where the data are copied as we have two
>> implementations of for example copyarea - one using system memory, the
>> other using IO memory.
> 
> sys_copyarea() and cfb_copyarea().
> 
>> The naming FB_DEFAULT_IO_OPS says this is the defaults to IO memory
>> operations, which tell what they do
> 
> This is exactly what I find misleading. IO_OPS sounds that it operates
> on file I/O (like file read/write), but not on iomem.
> 
>> and avoid the strange cfb acronym.
> 
>> Reserve cfb for color frame buffers only - and maybe in the end rename
>> the three cfbcopyarea, cfbfillrect, cfbimgblt to use the io prefix.
> 
> Again, the io prefix is what I think is misleading.
> Why not name it what it really is and what is used in the kernel 
> already, e.g.
> iomem_copyarea() and sysmem_copyarea().
> which would lead to
> FB_DEFAULT_IOMEM_OPS and FB_DEFAULT_SYSMEM_OPS.

Yes there's been a bit of confusion and discussion on the naming before. 
I'd be happy if we can standardize on sysmem and iomem.

I can add a patch to this patchset to rename the _IO_ macros and tokens 
to use _IOMEM_. That's not too much change. A later patchset can address 
sysmem and deferred I/O helpers separately.

On motivation: for now it's a cleanup to make the a bit code easier to 
understand. But once all drivers set their callbacks correctly, we can 
make the I/O mem code optional. It's currently the default and built-in 
unconditionally. But it's not uncommon that it's unused entirely.

Best regards
Thomas

>  
>> Which is much simpler to do after this series - and nice extra benefit.
>>
>> I hope this properly explains why I like the current naming and
>> acked it when the macros were introduced.
> 
> IMHO the naming isn't perfect, but that's just nitpicking.
> Besides that, Thomas' patches are a nice cleanup.
> So, if you want add a
> Acked-by: Helge Deller <deller@gmx.de>
> to the series.
> 
> Helge
Helge Deller July 29, 2023, 1:53 p.m. UTC | #8
On 7/29/23 15:21, Thomas Zimmermann wrote:
> Hi Helge
>
> Am 29.07.23 um 08:51 schrieb Helge Deller:
>> On 7/28/23 23:01, Sam Ravnborg wrote:
>>> Hi Helge,
>>>
>>> On Fri, Jul 28, 2023 at 08:46:59PM +0200, Helge Deller wrote:
>>>> On 7/28/23 18:39, Thomas Zimmermann wrote:
>>>>> Most fbdev drivers operate on I/O memory.
>>>>
>>>> Just nitpicking here:
>>>> What is I/O memory?
>>>> Isn't it either memory, or I/O ?
>>>> I mean, I would never think of the cfb* draw functions under I/O.
>>>>
>>>>> And most of those use the
>>>>> default implementations for file I/O and console drawing. Convert all
>>>>> these low-hanging fruits to the fb_ops initializer macro and Kconfig
>>>>> token for fbdev I/O helpers.
>>>>
>>>> I do see the motivation for your patch, but I think the
>>>> macro names are very misleading.
>>>>
>>>> You have:
>>>> #define __FB_DEFAULT_IO_OPS_RDWR \
>>>>          .fb_read        = fb_io_read, \
>>>>          .fb_write       = fb_io_write
>>>>
>>>> #define __FB_DEFAULT_IO_OPS_DRAW \
>>>>          .fb_fillrect    = cfb_fillrect, \
>>>>          .fb_copyarea    = cfb_copyarea, \
>>>>          .fb_imageblit   = cfb_imageblit
>>>>
>>>> #define __FB_DEFAULT_IO_OPS_MMAP \
>>>>          .fb_mmap        = NULL /* default implementation */
>>>>
>>>> #define FB_DEFAULT_IO_OPS \
>>>>          __FB_DEFAULT_IO_OPS_RDWR, \
>>>>          __FB_DEFAULT_IO_OPS_DRAW, \
>>>>          __FB_DEFAULT_IO_OPS_MMAP
>>>>
>>>> I think FB_DEFAULT_IO_OPS is OK for read/write/mmap.
>>>> But I would suggest to split out __FB_DEFAULT_IO_OPS_DRAW.
>>>> Something like:
>>>> #define FB_DEFAULT_IO_OPS \
>>>>          __FB_DEFAULT_IO_OPS_RDWR, \
>>>>          __FB_DEFAULT_IO_OPS_MMAP
>>>
>>>
>>>> #define FB_DEFAULT_CFB_OPS \
>>>>          .fb_fillrect    = cfb_fillrect, \
>>>>          .fb_copyarea    = cfb_copyarea, \
>>>>          .fb_imageblit   = cfb_imageblit
>>>
>>> The prefix cfb, I have recently learned, equals color frame buffer.
>>
>> correct.
>>
>>> They are named such for purely historical reasons.
>>
>> well, they operate on MEMORY which represents a (color) frame buffer,
>> either in host memory or memory on some card on some bus.
>> So, the naming cfb is not historical, but even today correct.
>>
>>> What is important is where the data are copied as we have two
>>> implementations of for example copyarea - one using system memory, the
>>> other using IO memory.
>>
>> sys_copyarea() and cfb_copyarea().
>>
>>> The naming FB_DEFAULT_IO_OPS says this is the defaults to IO memory
>>> operations, which tell what they do
>>
>> This is exactly what I find misleading. IO_OPS sounds that it operates
>> on file I/O (like file read/write), but not on iomem.
>>
>>> and avoid the strange cfb acronym.
>>
>>> Reserve cfb for color frame buffers only - and maybe in the end rename
>>> the three cfbcopyarea, cfbfillrect, cfbimgblt to use the io prefix.
>>
>> Again, the io prefix is what I think is misleading.
>> Why not name it what it really is and what is used in the kernel already, e.g.
>> iomem_copyarea() and sysmem_copyarea().
>> which would lead to
>> FB_DEFAULT_IOMEM_OPS and FB_DEFAULT_SYSMEM_OPS.
>
> Yes there's been a bit of confusion and discussion on the naming before. I'd be happy if we can standardize on sysmem and iomem.
>
> I can add a patch to this patchset to rename the _IO_ macros and tokens to use _IOMEM_. That's not too much change. A later patchset can address sysmem and deferred I/O helpers separately.

Sound good.

> On motivation: for now it's a cleanup to make the a bit code easier to understand. But once all drivers set their callbacks correctly, we can make the I/O mem code optional. It's currently the default and built-in unconditionally. But it's not uncommon that it's unused entirely.

Probably true for x86, on others it might be useful to make the sysmem optional as well. I did not checked.

Anyway, thanks for your work!

Helge

>
> Best regards
> Thomas
>
>>
>>> Which is much simpler to do after this series - and nice extra benefit.
>>>
>>> I hope this properly explains why I like the current naming and
>>> acked it when the macros were introduced.
>>
>> IMHO the naming isn't perfect, but that's just nitpicking.
>> Besides that, Thomas' patches are a nice cleanup.
>> So, if you want add a
>> Acked-by: Helge Deller <deller@gmx.de>
>> to the series.
>>
>> Helge
>
Sam Ravnborg July 30, 2023, 8:55 a.m. UTC | #9
On Fri, Jul 28, 2023 at 08:35:41PM +0200, Sam Ravnborg wrote:
> Hi Thomas,
> 
> On Fri, Jul 28, 2023 at 06:39:43PM +0200, Thomas Zimmermann wrote:
> > Most fbdev drivers operate on I/O memory. And most of those use the
> > default implementations for file I/O and console drawing. Convert all
> > these low-hanging fruits to the fb_ops initializer macro and Kconfig
> > token for fbdev I/O helpers.
> > 
> > The fbdev I/O helpers are easily grep-able. In a later patch, they can
> > be left to empty values if the rsp. funtionality, such as file I/O or
> > console, has been disabled.
> > 
> > There are no functional changes. The helpers set the defaults that
> > the drivers already use.
> 
> I have browsed all patches - they all looks good.
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

When you post v2 with MEM added the review still holds true.

	Sam
Thomas Zimmermann Aug. 1, 2023, 8:59 a.m. UTC | #10
Hi Sam

Am 28.07.23 um 20:39 schrieb Sam Ravnborg:
> Hi Thomas,
> 
> On Fri, Jul 28, 2023 at 06:39:43PM +0200, Thomas Zimmermann wrote:
>> Most fbdev drivers operate on I/O memory. And most of those use the
>> default implementations for file I/O and console drawing. Convert all
>> these low-hanging fruits to the fb_ops initializer macro and Kconfig
>> token for fbdev I/O helpers.
>>
>> The fbdev I/O helpers are easily grep-able. In a later patch, they can
>> be left to empty values if the rsp. funtionality, such as file I/O or
>> console, has been disabled.
> 
> Did you miss sm750 or was it left out on purpose?
> As it hide in staging it is easy to miss.

Now I remembered why I left out sm750fb. It modifies the function 
pointers at some point at

https://elixir.bootlin.com/linux/latest/source/drivers/staging/sm750fb/sm750.c#L741

So the driver uses a non-trivial fb_ops setup and is worth a different fix.

Best regards
Thomas

> 
> 	Sam