On 2/2/22 12:05, Daniel Vetter wrote: > On Tue, Feb 1, 2022 at 11:52 PM Helge Deller <deller@xxxxxx> wrote: >> >> Hello Daniel, >> >> On 2/1/22 21:11, Daniel Vetter wrote: >>> On Tue, Feb 1, 2022 at 7:59 PM Helge Deller <deller@xxxxxx> 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@xxxxxx> >>>> Cc: stable@xxxxxxxxxxxxxxx # v5.10+ >>>> Signed-off-by: Helge Deller <deller@xxxxxx> >>>> --- >>>> 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. > > Maybe to clarify. The issues that generally result in this code going > boom in syzbot are when you race console activity (which can be > largely controlled through VT ioctls from userspace too, plus > /dev/kmsg) against fbdev resizing on the /dev/fb/0 nodes. The locking > there is kinda wild, and the code is very hard to understand. This is > why we've resorted to just disabling/deleting this code because most > cases we have no idea what's actually going on, aside from something > is clearly not going right. Yes, that's clear. But that (missing) locking needs to be on a higher level in the call chain, so the same lock fixes would fix scroll-redraw and hw acceleration. > Also the multi-user here means "you run untrusted code from other > people", not "you run multiple things in parallel" or "multiple people > are logged in at the same time". > >> 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? > > Perfect. Great!! >> Arch maintainers may then later send (or reply now with) patches, e.g.: >> + default y if (M68K && XYZ) >> ... > > Yeah makes sense. > >>> 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. > > tbh I think it's fine either way. I think it's still rather unclear > which way drm will go, least because there's not many people who can > occasionally squeeze some time away to move things forward. Could be > that we add a new emergency logging console in the kernel for drm, and > distros switch over to kmscon (which is in userspace, and accelerated > iirc if you have gl, so most modern systems). Or whether we just > improve fbcon until it's solid enough. Or some other option. > > So given that just dropping the todo sounds ok, it was just a bit > inconsistent with the Kconfig :-) Ok, I leave it out. >>> 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. > > Ideally if you can send them out today so it's not missing the > drm-misc-fixes train that leaves tomorrow, that would be best. Cool. I'll send the new series in a few minutes. Thanks! Helge
