[PATCH v3 3/3] fbcon: Add option to enable legacy hardware acceleration

[Helge Deller]

Hello Daniel,

On 2/1/22 21:11, Daniel Vetter wrote:
> On Tue, Feb 1, 2022 at 7:59 PM Helge Deller <deller at gmx.de> wrote:
>>
>> Add a config option CONFIG_FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION to
>> enable bitblt and fillrect hardware acceleration in the framebuffer
>> console. If disabled, such acceleration will not be used, even if it is
>> supported by the graphics hardware driver.
>>
>> If you plan to use DRM as your main graphics output system, you should
>> disable this option since it will prevent compiling in code which isn't
>> used later on when DRM takes over.
>>
>> For all other configurations, e.g. if none of your graphic cards support
>> DRM (yet), DRM isn't available for your architecture, or you can't be
>> sure that the graphic card in the target system will support DRM, you
>> most likely want to enable this option.
>>
>> In the non-accelerated case (e.g. when DRM is used), the inlined
>> fb_scrollmode() function is hardcoded to return SCROLL_REDRAW and as such the
>> compiler is able to optimize much unneccesary code away.
>>
>> In this v3 patch version I additionally changed the GETVYRES() and GETVXRES()
>> macros to take a pointer to the fbcon_display struct. This fixes the build when
>> console rotation is enabled and helps the compiler again to optimize out code.
>>
>> Signed-off-by: Helge Deller <deller at gmx.de>
>> Cc: stable at vger.kernel.org # v5.10+
>> Signed-off-by: Helge Deller <deller at gmx.de>
>> ---
>>  drivers/video/console/Kconfig           | 11 +++++++
>>  drivers/video/fbdev/core/fbcon.c        | 39 ++++++++++++++++++-------
>>  drivers/video/fbdev/core/fbcon.h        | 15 +++++++++-
>>  drivers/video/fbdev/core/fbcon_ccw.c    | 10 +++----
>>  drivers/video/fbdev/core/fbcon_cw.c     | 10 +++----
>>  drivers/video/fbdev/core/fbcon_rotate.h |  4 +--
>>  drivers/video/fbdev/core/fbcon_ud.c     | 20 ++++++-------
>>  7 files changed, 75 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/video/console/Kconfig b/drivers/video/console/Kconfig
>> index 840d9813b0bc..6029fd41d7d0 100644
>> --- a/drivers/video/console/Kconfig
>> +++ b/drivers/video/console/Kconfig
>> @@ -78,6 +78,17 @@ config FRAMEBUFFER_CONSOLE
>>         help
>>           Low-level framebuffer-based console driver.
>>
>> +config FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION
>> +       bool "Framebuffer Console hardware acceleration support"
>> +       depends on FRAMEBUFFER_CONSOLE
>> +       default n if DRM
>> +       default y
>> +       help
>> +         If you use a system on which DRM is fully supported you usually want to say N,
>> +         otherwise you probably want to enable this option.
>> +         If enabled the framebuffer console will utilize the hardware acceleration
>> +         of your graphics card by using hardware bitblt and fillrect features.
>
> This really doesn't have much to do with DRM but more about running
> questionable code. That's why I went with the generalized stern
> warning and default n, and explained when it's ok to do this (single
> user and you care more about fbcon performance than potential issues
> because you only run trusted stuff with access to your vt and fbdev
> /dev node).

I think this is something we both will keep to have different opinion about :-)

This console acceleration code is not exported or visible to userspace,
e.g. you can't access or trigger it via /dev/fb0.
It's only called internally from inside fbcon, so it's independed if
it's a single- or multi-user system.
The only way to "access" it is via standard stdio, where fbcon then
either scrolls the screen via redrawing characters at new positions
or by using hardware bitblt to move screen contents.
IMHO there is nothing which is critical here.
Even when I analyzed the existing bug reports, there was none which
affected that specific code.

Anyway, let's look at that part in your patch:

+       bool "Enable legacy fbcon code for hw acceleration"
+       depends on FRAMEBUFFER_CONSOLE
+       default n
+       help
+        Only enable this options if you are in full control of machine since
+        the fbcon acceleration code is essentially unmaintained and known
+        problematic.
+
+        If unsure, select n.

Since I'm willing to maintain that scrolling/panning code, I'd like to
drop the "is essentially unmaintained" part.
And the "known problematic" part is up to now just speculative (which would be
valid for other parts of the kernel too, btw).

As this whole disussions showed, there are some few architectures/platforms
which really still need this acceleration, while others don't.
So, why not leave the decision up to the arch maintainers, which may opt-in
or opt-out, while keeping the default on "n"?

With that, here is a new proposal:

+config FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION
+       bool "Enable legacy fbcon hardware acceleration code"
+       depends on FRAMEBUFFER_CONSOLE
+       default y if (PARISC) # a few other arches may want to be listed here too...
+       default n
+       help
+         This option enables the fbcon (framebuffer text-based) hardware acceleration for
+	  graphics drivers which were written for the fbdev graphics interface.
+
+	  On modern machines, on mainstream machines (like x86-64) or when using a modern
+	  Linux distribution those fbdev drivers usually aren't used.
+	  So enabling this option wouldn't have any effect, which is why you want
+	  to disable this option on such newer machines.
+
+	  If you compile this kernel for older machines which still require the fbdev
+	  drivers, you may want to say Y.
+
+         If unsure, select n.

Would that be acceptable?

Arch maintainers may then later send (or reply now with) patches, e.g.:
+       default y if (M68K && XYZ)
...


> Also you didn't keep any todo entry for removing DRM accel code,

That wasn't intentional.
I just sent 2 revert-patches and an fbcon-accel-enabling-patch.
I'll look up what you had in your patch series and add it as seperate patch.

> and iirc some nouveau folks also complained that they at least
> once had some kind of accel, so that's another reason to not tie this
> to DRM.

The above proposal to add additional "default y if XYZ" would enable
them to opt-in.

> Anyway aside from this looks reasonable, can you pls respin with a
> more cautious Kconfig text?

Sure, I'll do as soon as we have a decision on the above Kconfig text.

Helge