[RFC PATCH 00/12] Deduplicate cfb/sys drawing fbops

[Helge Deller]

CC'ed: driv-devel

On 2/2/25 13:39, Kajtár Zsolt wrote:
> A series on de-duplicating the common cfb and sys drawing routines will
> follow.
>
> Some background:
>
> It happens that I need to use both cfb and sys drawing routines.

For which driver do you need this?

> At low resolution where the aperture is large enough the framebuffer
> memory is directly mapped. As usual the cfb routines are used.
>
> In the high resolution scenario defio is used to make banking
> transparent, there the sys drawing routines are used.
>
> There are packed pixels, of course in the wrong order. So that needs
> CFB_REV_PIXELS_IN_BYTE, fine. Or almost.
>
> While the sys routines are based on cfb for some reason the former lacks
> pixel order reversing support. The result is that at low resolution the
> console is fine, but it's unreadable at high resolution due to the wrong
> pixel ordering.
>
> First I added the pixel reversal support to sys, console looks fine.
> Still I might have made mistakes when doing so, that might need
> further testing just to be sure. Hacked fillrect to run in userspace,
> wasn't easy and now I have to come up with edge cases...
>
> Had another look at the cfb routines and sys is basically a straight
> copy. Minus the pixel reversing, FB_READ/WRITE macro inlining by hand,
> comment style updates and a few changes here and there for I/O vs.
> system memory. The memory access differences could have been easily
> covered with a few small macros, strange.
>
> Cfb has a working pixel reversal as far as I know. Now all it needs is a
> few changes for the memory access differences and then I have the sys
> routines with pixel reversal. And can also be reasonably confident that
> it actually does what it needs to in special drawing scenarios.
>
> So the patches below take a copy of the cfb routines as header files,
> and add macros for the access, text and other differences. The comment
> style changes were taken from sys so that it's less different when
> compared. Then the cfb and sys files were cut down to just an include
> of the common code in the header plus a few defines for the macros used
> in the headers.
>
> I was thinking what to do with copyright/credits now. On the new headers
> it's clear as it's basically cfb, but the new cfb and sys suffered
> significant changes and not much remained. I kept the original authors
> but it might be questionable on sys, it just an includes cfb code now.
>
> I know at the moment there are no users for the pixel reversal function
> in sys and could have sent such changes later when truly required.
>
> However there are some maintainability benefits as it removes lots of
> duplicate code which might be worth to have meanwhile. The pixel
> reversal gets optimized out when not in use so I don't worry about that
> much.
>
> Zsolt Kajtar (12):
>    fbdev: core: Copy cfbcopyarea to fb_copyarea
>    fbdev: core: Make fb_copyarea generic
>    fbdev: core: Use generic copyarea for as cfb_copyarea
>    fbdev: core: Use generic copyarea for as sys_copyarea
>    fbdev: core: Copy cfbfillrect to fb_fillrect
>    fbdev: core: Make fb_fillrect generic
>    fbdev: core: Use generic fillrect for as cfb_fillrect
>    fbdev: core: Use generic fillrect for as sys_fillrect
>    fbdev: core: Copy cfbimgblt to fb_imageblit
>    fbdev: core: Make fb_imageblit generic
>    fbdev: core: Use generic imageblit for as cfb_imageblit
>    fbdev: core: Use generic imageblit for as sys_imageblit
>
>   drivers/video/fbdev/core/cfbcopyarea.c  | 425 +-----------------------
>   drivers/video/fbdev/core/cfbfillrect.c  | 362 +-------------------
>   drivers/video/fbdev/core/cfbimgblt.c    | 356 +-------------------
>   drivers/video/fbdev/core/fb_copyarea.h  | 421 +++++++++++++++++++++++
>   drivers/video/fbdev/core/fb_fillrect.h  | 359 ++++++++++++++++++++
>   drivers/video/fbdev/core/fb_imageblit.h | 356 ++++++++++++++++++++
>   drivers/video/fbdev/core/syscopyarea.c  | 356 +-------------------
>   drivers/video/fbdev/core/sysfillrect.c  | 314 +----------------
>   drivers/video/fbdev/core/sysimgblt.c    | 324 +-----------------
>   9 files changed, 1190 insertions(+), 2083 deletions(-)
>   create mode 100644 drivers/video/fbdev/core/fb_copyarea.h
>   create mode 100644 drivers/video/fbdev/core/fb_fillrect.h
>   create mode 100644 drivers/video/fbdev/core/fb_imageblit.h

Some notes regarding your patches:
- please add dri-devel at lists.freedesktop.org mailing list
- it seems you sent your patches manually.
   Using b4 on your Message-ID gives:
b4 am f921492d-6c53-ce68-16b6-dc9c21f2b738 at c64.rulez.org
Grabbing thread from lore.kernel.org/all/f921492d-6c53-ce68-16b6-dc9c21f2b738 at c64.rulez.org/t.mbox.gz
Analyzing 13 messages in the thread
Analyzing 0 code-review messages
Checking attestation on all messages, may take a moment...
---
   [PATCH RFC 1/12] Deduplicate cfb/sys drawing fbops
   ERROR: missing [2/12]!
   [PATCH RFC 3/12] Deduplicate cfb/sys drawing fbops
   [PATCH RFC 4/12] Deduplicate cfb/sys drawing fbops
   [PATCH RFC 5/12] Deduplicate cfb/sys drawing fbops
   [PATCH RFC 6/12] fbdev: core: Make fb_fillrect generic
   [PATCH RFC 7/12] fbdev: core: Use generic fillrect for as, cfb_fillrect
   [PATCH RFC 8/12] fbdev: core: Use generic fillrect for as, sys_fillrect
   [PATCH RFC 9/12] fbdev: core: Copy cfbimgblt to fb_imageblit
   [PATCH RFC 10/12] fbdev: core: Make fb_imageblit generic
   [PATCH RFC 11/12] fbdev: core: Use generic imageblit for as, cfb_imageblit
   [PATCH RFC 12/12] fbdev: core: Use generic imageblit for as, sys_imageblit
Total patches: 11
WARNING: Thread incomplete!
---
- patch #2 is missing, and patches 1-5 have the same subject.
- When applying there are warnings:
git am ./20250202_soci_deduplicate_cfb_sys_drawing_fbops.mbx
Applying: Deduplicate cfb/sys drawing fbops
.git/rebase-apply/patch:451: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
Applying: Deduplicate cfb/sys drawing fbops
Applying: Deduplicate cfb/sys drawing fbops
Applying: Deduplicate cfb/sys drawing fbops
Applying: fbdev: core: Make fb_fillrect generic
Applying: fbdev: core: Use generic fillrect for as, cfb_fillrect
Applying: fbdev: core: Use generic fillrect for as, sys_fillrect
Applying: fbdev: core: Copy cfbimgblt to fb_imageblit
.git/rebase-apply/patch:199: space before tab in indent.
                 if (shift) {
.git/rebase-apply/patch:380: new blank line at EOF.
+
warning: 2 lines add whitespace errors.
Applying: fbdev: core: Make fb_imageblit generic
Applying: fbdev: core: Use generic imageblit for as, cfb_imageblit
Applying: fbdev: core: Use generic imageblit for as, sys_imageblit

I suggest you make yourself familiar with "git-publish" (https://github.com/stefanha/git-publish).
It's a great tool which helps a lot for sending patches.

Helge