Re: [PATCH] fbcon: Disable accelerated scrolling

Hi

Am 28.10.20 um 20:55 schrieb Daniel Vetter:

On Wed, Oct 28, 2020 at 8:02 PM Thomas Zimmermann tzimmermann@suse.de wrote:

...

Hi

Am 28.10.20 um 17:06 schrieb Daniel Vetter:

...

So ever since syzbot discovered fbcon, we have solid proof that it's full of bugs. And often the solution is to just delete code and remove features, e.g. 50145474f6ef ("fbcon: remove soft scrollback code").

Now the problem is that most modern-ish drivers really only treat fbcon as an dumb kernel console until userspace takes over, and Oops printer for some emergencies. Looking at drm drivers and the basic vesa/efi fbdev drivers shows that only 3 drivers support any kind of acceleration:

• nouveau, seems to be enabled by default
• omapdrm, when a DMM remapper exists using remapper rewriting for y/xpanning
• gma500, but that is getting deleted now for the GTT remapper trick, and the accelerated copyarea never set the FBINFO_HWACCEL_COPYAREA flag, so unused (and could be deleted already I think).

No other driver supportes accelerated fbcon. And fbcon is the only user of this accel code (it's not exposed as uapi through ioctls), which means we could garbage collect fairly enormous amounts of code if we kill this.

Plus because syzbot only runs on virtual hardware, and none of the drivers for that have acceleration, we'd remove a huge gap in testing. And there's no other even remotely comprehensive testing aside from syzbot.

This patch here just disables the acceleration code by always redrawing when scrolling. The plan is that once this has been merged for well over a year in released kernels, we can start to go around and delete a lot of code.

Why wait a year? I'd say delete early, delete often. ;)

...

Cc: Bartlomiej Zolnierkiewicz b.zolnierkie@samsung.com Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: Linus Torvalds torvalds@linux-foundation.org Cc: Ben Skeggs bskeggs@redhat.com Cc: nouveau@lists.freedesktop.org Cc: Tomi Valkeinen tomi.valkeinen@ti.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Jiri Slaby jirislaby@kernel.org Cc: "Gustavo A. R. Silva" gustavoars@kernel.org Cc: Tetsuo Handa penguin-kernel@I-love.SAKURA.ne.jp Cc: Peilin Ye yepeilin.cs@gmail.com Cc: George Kennedy george.kennedy@oracle.com Cc: Nathan Chancellor natechancellor@gmail.com Cc: Peter Rosin peda@axentia.se Signed-off-by: Daniel Vetter daniel.vetter@intel.com

---

drivers/video/fbdev/core/fbcon.c | 38 ++++++-------------------------- 1 file changed, 7 insertions(+), 31 deletions(-)

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c index cef437817b0d..d74ccbbb29bb 100644 --- a/drivers/video/fbdev/core/fbcon.c +++ b/drivers/video/fbdev/core/fbcon.c @@ -1147,11 +1147,13 @@ static void fbcon_init(struct vc_data *vc, int init)

  ops->graphics = 0;

• if ((cap & FBINFO_HWACCEL_COPYAREA) &&
  

•     !(cap & FBINFO_HWACCEL_DISABLED))
  

•         p->scrollmode = SCROLL_MOVE;
  

• else /* default to something safe */
  

•         p->scrollmode = SCROLL_REDRAW;
  

• /*
  

•  * No more hw acceleration for fbcon.
  

•  *
  

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

•  * has passed.
  

•  */
  

• p->scrollmode = SCROLL_REDRAW;
  

I just grepped for scrollmode and there aren't many places that use it. Could you remove it as well?

Removing scrollmode will start the delete feast. In fbcon alone I think we can drop half the code.

That really deserves a TODO item then IMHO.

Best regards Thomas

-Daniel

...

In any case

Reviewed-by: Thomas Zimmermann tzimmermann@suse.de

Best regards Thomas

...

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

@@ -1961,7 +1963,6 @@ 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);
  

@@ -1969,37 +1970,12 @@ static void updatescrollmode(struct fbcon_display *p, 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)

-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer