Re: [PATCH 2/2] Revert "fbcon: Disable accelerated scrolling"

Hello Daniel,

On 1/20/22 15:30, Daniel Vetter wrote:

On Wed, Jan 19, 2022 at 12:08:39PM +0100, Helge Deller wrote:

...

This reverts commit 39aead8373b3c20bb5965c024dfb51a94e526151.

Revert this patch. This patch started to introduce the regression that all hardware acceleration of more than 35 existing fbdev drivers were bypassed and thus fbcon console output for those was dramatically slowed down by factor of 10 and more.

Reverting this commit has no impact on DRM, since none of the DRM drivers are tagged with the acceleration flags FBINFO_HWACCEL_COPYAREA, FBINFO_HWACCEL_FILLRECT or others.

Signed-off-by: Helge Deller deller@gmx.de Cc: stable@vger.kernel.org # v5.16

So if this really has to come back then I think the pragmatic approach is to do it behind a CONFIG_FBCON_ACCEL, default n, and with a huge warning that enabling that shouldn't be done for any distro which only enables firmware and drm fbdev drivers.

Thanks for coming back on this, but quite frankly I don't understand that request. How should that warning look like, something along: "BE WARNED: The framebuffer text console on your non-DRM supported graphic card will then run faster and smoother if you enable this option." That doesn't make sense. People and distros would want to enable that.

And if a distro *just* has firmware and drm fbdev drivers enabled, none of the non-DRM graphic cards would be loaded anyway and this code wouldn't be executed anyway.

I think what you want is that DRM drivers are preferred over standard fbdev drivers, esp. if there is a driver for both available. But that's completely independed of fbdev-drivers console hardware acceleration.

Plus adjusting the todo to limit it to drm drivers. Maybe also #ifdef out the code that's then dead from fbcon.

Sorry, I don't understand that either. I assume you mean to put code of fbcon which is only used by fbdev-drivers into and #ifdef CONFIG_FB .. #endif (CONFIG_FB may be wrong in this example). That's probably possible, but I don't see a big win. If there is no fbdev driver compiled-in or as module, none of this fbdev-drivers will be loaded and that code path wouldn't be executed anyway. In that case you will win a few bytes of code, but probably not much.

Also in that case I guess it's ok to cc: stable, and really if you cc: stable it needs to go down to 5.11, not 5.16.

Yes, I missed that in my patch request. Will fix.

And if we do that, I think that should go in through a -next cycle, or at least quite some soaking before it's cherry-picked over. Enough to give syzbot a chance to discover any path we've missed at least.

Sure. We don't need to hurry.

Thanks! Helge

-Daniel

...

---

Documentation/gpu/todo.rst | 21 --------------- drivers/video/fbdev/core/fbcon.c | 45 ++++++++++++++++++++++++++------ 2 files changed, 37 insertions(+), 29 deletions(-)

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst index 29506815d24a..a1212b5b3026 100644 --- a/Documentation/gpu/todo.rst +++ b/Documentation/gpu/todo.rst @@ -300,27 +300,6 @@ Contact: Daniel Vetter, Noralf Tronnes

Level: Advanced

-Garbage collect fbdev scrolling acceleration

-Scroll acceleration is disabled in fbcon by hard-wiring p->scrollmode = -SCROLL_REDRAW. There's a ton of code this will allow us to remove:

-- lots of code in fbcon.c

-- a bunch of the hooks in fbcon_ops, maybe the remaining hooks could be called

• directly instead of the function table (with a switch on p->rotate)

-- fb_copyarea is unused after this, and can be deleted from all drivers

-Note that not all acceleration code can be deleted, since clearing and cursor -support is still accelerated, which might be good candidates for further -deletion projects.

-Contact: Daniel Vetter

-Level: Intermediate

idr_init_base()

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c index 22bb3892f6bd..b813985f1403 100644 --- a/drivers/video/fbdev/core/fbcon.c +++ b/drivers/video/fbdev/core/fbcon.c @@ -1025,7 +1025,7 @@ static void fbcon_init(struct vc_data *vc, int init) struct vc_data *svc = *default_mode; struct fbcon_display *t, *p = &fb_display[vc->vc_num]; int logo = 1, new_rows, new_cols, rows, cols;

• int ret;

• int cap, ret;

  if (WARN_ON(info_idx == -1)) return;

@@ -1034,6 +1034,7 @@ static void fbcon_init(struct vc_data *vc, int init) con2fb_map[vc->vc_num] = info_idx;

info = registered_fb[con2fb_map[vc->vc_num]];

• cap = info->flags;

  if (logo_shown < 0 && console_loglevel <= CONSOLE_LOGLEVEL_QUIET) logo_shown = FBCON_LOGO_DONTSHOW;

@@ -1135,13 +1136,11 @@ static void fbcon_init(struct vc_data *vc, int init)

ops->graphics = 0;

• /*

• * No more hw acceleration for fbcon.
  

• *
  

• * FIXME: Garbage collect all the now dead code after sufficient time
  

• * has passed.
  

• */
  

• p->scrollmode = SCROLL_REDRAW;

• if ((cap & FBINFO_HWACCEL_COPYAREA) &&

•    !(cap & FBINFO_HWACCEL_DISABLED))
  

• p->scrollmode = SCROLL_MOVE;
  

• else /* default to something safe */

• p->scrollmode = SCROLL_REDRAW;
  
  

  /*

  • ++guenther: console.c:vc_allocate() relies on initializing

@@ -1953,15 +1952,45 @@ static void updatescrollmode(struct fbcon_display *p, { struct fbcon_ops *ops = info->fbcon_par; int fh = vc->vc_font.height;

• int cap = info->flags;

• u16 t = 0;

• int ypan = FBCON_SWAP(ops->rotate, info->fix.ypanstep,

• 		  info->fix.xpanstep);
  

• int ywrap = FBCON_SWAP(ops->rotate, info->fix.ywrapstep, t); int yres = FBCON_SWAP(ops->rotate, info->var.yres, info->var.xres); int vyres = FBCON_SWAP(ops->rotate, info->var.yres_virtual, info->var.xres_virtual);

• int good_pan = (cap & FBINFO_HWACCEL_YPAN) &&

• divides(ypan, vc->vc_font.height) && vyres > yres;
  

• int good_wrap = (cap & FBINFO_HWACCEL_YWRAP) &&

• divides(ywrap, vc->vc_font.height) &&
  

• divides(vc->vc_font.height, vyres) &&
  

• divides(vc->vc_font.height, yres);
  

• int reading_fast = cap & FBINFO_READS_FAST;

• int fast_copyarea = (cap & FBINFO_HWACCEL_COPYAREA) &&

• !(cap & FBINFO_HWACCEL_DISABLED);
  

• int fast_imageblit = (cap & FBINFO_HWACCEL_IMAGEBLIT) &&

• !(cap & FBINFO_HWACCEL_DISABLED);
  
  

  p->vrows = vyres/fh; if (yres > (fh * (vc->vc_rows + 1))) p->vrows -= (yres - (fh * vc->vc_rows)) / fh; if ((yres % fh) && (vyres % fh < yres % fh)) p->vrows--;

• if (good_wrap || good_pan) {

• if (reading_fast || fast_copyarea)
  

• 	p->scrollmode = good_wrap ?
  

• 		SCROLL_WRAP_MOVE : SCROLL_PAN_MOVE;
  

• else
  

• 	p->scrollmode = good_wrap ? SCROLL_REDRAW :
  

• 		SCROLL_PAN_REDRAW;
  

• } else {

• if (reading_fast || (fast_copyarea && !fast_imageblit))
  

• 	p->scrollmode = SCROLL_MOVE;
  

• else
  

• 	p->scrollmode = SCROLL_REDRAW;
  

• }

}

#define PITCH(w) (((w) + 7) >> 3)

2.31.1