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

[Thomas Zimmermann]

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 at gmx.de>
> to the series.
> 
> Helge

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20230729/e9f381de/attachment.sig>