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

[Helge Deller]

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