Hi Andy,

On 7/29/22 00:19, Andy Shevchenko wrote:

On Thu, Jul 28, 2022 at 4:32 PM Markuss Broks [off-list ref] wrote:

quoted

Add early console support for generic linear framebuffer devices.
This driver supports probing from cmdline early parameters
or from the device-tree using information in simple-framebuffer node.
The EFI functionality should be retained in whole.
The driver was disabled on ARM because of a bug in early_ioremap
implementation on ARM.

...

quoted

-               efifb,[options]
+               efifb
                         Start an early, unaccelerated console on the EFI
-                       memory mapped framebuffer (if available). On cache
-                       coherent non-x86 systems that use system memory for
-                       the framebuffer, pass the 'ram' option so that it is
-                       mapped with the correct attributes.
+                       memory mapped framebuffer (if available).

If somebody passes those (legacy) options, what will happen?

Those would be ignored. Instead it would be automatically determined if 
framebuffer is located in RAM or in the I/O space.

...

quoted

  config EFI_EARLYCON
-       def_bool y
-       depends on SERIAL_EARLYCON && !ARM && !IA64
-       select FONT_SUPPORT
-       select ARCH_USE_MEMREMAP_PROT
+       bool "EFI early console support"
+       depends on FB_EARLYCON && !IA64

This doesn't sound right. Previously on my configuration it was
selected automatically, now I need to select it explicitly? I mean
that for me EFI_EARLYCON should be selected by default as it was
before.

The problem I had with EFI_EARLYCON selected by default was that it 
would also carry fbdev with itself. Luckily, that's solved if it's moved 
to console subsystem.

...

quoted

+static int __init simplefb_earlycon_remap_fb(void)
+{
+       int is_ram;

+ blank line.

quoted

+       /* bail if there is no bootconsole or it has been disabled already */
+       if (!earlycon_console || !(earlycon_console->flags & CON_ENABLED))
+               return 0;
+
+       is_ram = region_intersects(info.phys_base, info.size,
+                                  IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE);
+       is_ram = is_ram == REGION_INTERSECTS;

Was it in the original code? Otherwise, I would go with plain conditional:

   if (region_intersects())
     base = ...
   else
     base = ...

It wasn't in original code. Original code assumed MEMREMAP_WC always
unless "ram" was specified as an option to efifb (e.g.
earlycon=efifb,ram). This code instead checks if the framebuffer is in 
RAM, and sets the mapping accordingly.

Another issue is that region_intersects() returns REGION_INTERSECTS 
(defined as 0) when true. I suppose we could use something like:

if (region_intersects() == REGION_INTERSECTS)
...

quoted

+       info.virt_base = memremap(info.phys_base, info.size,
+                                 is_ram ? MEMREMAP_WB : MEMREMAP_WC);
+
+       return info.virt_base ? 0 : -ENOMEM;
+}

...

quoted

+static void simplefb_earlycon_write_char(u8 *dst, unsigned char c, unsigned int h)
+{
+       const u8 *src;
+       int m, n, bytes;
+       u8 x;
+
+       bytes = BITS_TO_BYTES(font->width);
+       src = font->data + c * font->height * bytes + h * bytes;
+
+       for (m = 0; m < font->width; m++) {
+               n = m % 8;
+               x = *(src + m / 8);
+               if ((x >> (7 - n)) & 1)
+                       memset(dst, 0xff, (info.depth / 8));
+               else
+                       memset(dst, 0, (info.depth / 8));
+               dst += (info.depth / 8);
+       }
+}

Wondering if we already have something like this in DRM/fbdev and can
split into a generic helper.

...

quoted

+       ret = sscanf(device->options, "%u,%u,%u", &info.x, &info.y, &info.depth);
+       if (ret != 3)
+               return -ENODEV;

Don't we have some standard template of this, something like XxYxD,
where X, Y, and D are respective decimal numbers?

I'm not aware of this.

-Markuss