Nouveau - Oct 2020 - [PATCH] fbcon: Disable accelerated scrolling

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.

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

Hi Daniel et al.

On Wed, Oct 28, 2020 at 05:06:00PM +0100, Daniel Vetter
wrote:
> 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.
So far I follow you - and agree.
Acked-by: Sam Ravnborg <sam at ravnborg.org>
> 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 one year? We deleted the scrollback code without any prior
warning - which was fine. And acceleration support has less users
so there should be no reason to wait.

So unless there are good arguments that I miss then we should just
delete the acceleration code outright.

	Sam

On Wed, Oct 28, 2020 at 5:45 PM Sam Ravnborg <sam at ravnborg.org>
wrote:
>
> Hi Daniel et al.
>
> On Wed, Oct 28, 2020 at 05:06:00PM +0100, Daniel Vetter wrote:
> > 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.
>
> So far I follow you - and agree.
> Acked-by: Sam Ravnborg <sam at ravnborg.org>
>
> > 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 one year? We deleted the scrollback code without any prior
> warning - which was fine. And acceleration support has less users
> so there should be no reason to wait.
>
> So unless there are good arguments that I miss then we should just
> delete the acceleration code outright.
If you grep for FBINFO_HWACCEL and related stuff, we could delete like
half the driver code, plus a ton of the related support code in fbcon
and fbdev core. It's going to be a lot of work, and I don't want to do
that and then have to back it out again. Compared to this the
softscrollback removal was nothing.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

On Wed, Oct 28, 2020 at 05:06:00PM +0100, Daniel Vetter
wrote:
> 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.
> 
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie at samsung.com>
> Cc: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
> Cc: Linus Torvalds <torvalds at linux-foundation.org>
> Cc: Ben Skeggs <bskeggs at redhat.com>
> Cc: nouveau at lists.freedesktop.org
> Cc: Tomi Valkeinen <tomi.valkeinen at ti.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: Jiri Slaby <jirislaby at kernel.org>
> Cc: "Gustavo A. R. Silva" <gustavoars at kernel.org>
> Cc: Tetsuo Handa <penguin-kernel at I-love.SAKURA.ne.jp>
> Cc: Peilin Ye <yepeilin.cs at gmail.com>
> Cc: George Kennedy <george.kennedy at oracle.com>
> Cc: Nathan Chancellor <natechancellor at gmail.com>
> Cc: Peter Rosin <peda at axentia.se>
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> ---
>  drivers/video/fbdev/core/fbcon.c | 38 ++++++--------------------------
>  1 file changed, 7 insertions(+), 31 deletions(-)
Nice!

But I'm with Sam, delete early :)

Reviewed-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>

Hi Daniel.

On Wed, Oct 28, 2020 at 05:06:00PM +0100, Daniel Vetter
wrote:
> 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.
See below for a warning fix.

Some figures from trying to toss accel code out from a few fbdev drivers:

 drivers/video/fbdev/cirrusfb.c | 300 +----------------------------------------
 1 file changed, 4 insertions(+), 296 deletions(-)

 drivers/video/fbdev/aty/radeon_accel.c | 174 ---------------------------------
 drivers/video/fbdev/aty/radeon_base.c  |  43 ++------
 drivers/video/fbdev/aty/radeon_pm.c    |   7 --
 drivers/video/fbdev/aty/radeonfb.h     |   3 -
 4 files changed, 7 insertions(+), 220 deletions(-)

This may open up the discussion if the right course of action would be
to drop the drivers in favour of drm counterparts - but thats another
story.

	Sam
> @@ -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);
Some bot will likely tell you that this causes warnings.
At least it did in my sparc64 build.

Fix:

diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 398914e035e9..e8b009c621d8 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -2150,10 +2150,6 @@ static void updatescrollmode(struct fbcon_display *p,
 {
 	struct fbcon_ops *ops = info->fbcon_par;
 	int fh = vc->vc_font.height;
-	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);

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

In any case

Reviewed-by: Thomas Zimmermann <tzimmermann at 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

On Wed, Oct 28, 2020 at 8:02 PM Thomas Zimmermann <tzimmermann at 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 at samsung.com>
> > Cc: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
> > Cc: Linus Torvalds <torvalds at linux-foundation.org>
> > Cc: Ben Skeggs <bskeggs at redhat.com>
> > Cc: nouveau at lists.freedesktop.org
> > Cc: Tomi Valkeinen <tomi.valkeinen at ti.com>
> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > Cc: Jiri Slaby <jirislaby at kernel.org>
> > Cc: "Gustavo A. R. Silva" <gustavoars at kernel.org>
> > Cc: Tetsuo Handa <penguin-kernel at I-love.SAKURA.ne.jp>
> > Cc: Peilin Ye <yepeilin.cs at gmail.com>
> > Cc: George Kennedy <george.kennedy at oracle.com>
> > Cc: Nathan Chancellor <natechancellor at gmail.com>
> > Cc: Peter Rosin <peda at axentia.se>
> > Signed-off-by: Daniel Vetter <daniel.vetter at 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.
-Daniel
>
> In any case
>
> Reviewed-by: Thomas Zimmermann <tzimmermann at 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


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

On Wed, Oct 28, 2020 at 7:50 PM Sam Ravnborg <sam at ravnborg.org>
wrote:
>
> Hi Daniel.
>
> On Wed, Oct 28, 2020 at 05:06:00PM +0100, Daniel Vetter wrote:
> > 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.
>
> See below for a warning fix.
>
> Some figures from trying to toss accel code out from a few fbdev drivers:
>
>  drivers/video/fbdev/cirrusfb.c | 300
+----------------------------------------
>  1 file changed, 4 insertions(+), 296 deletions(-)
>
>  drivers/video/fbdev/aty/radeon_accel.c | 174
---------------------------------
>  drivers/video/fbdev/aty/radeon_base.c  |  43 ++------
>  drivers/video/fbdev/aty/radeon_pm.c    |   7 --
>  drivers/video/fbdev/aty/radeonfb.h     |   3 -
>  4 files changed, 7 insertions(+), 220 deletions(-)
>
> This may open up the discussion if the right course of action would be
> to drop the drivers in favour of drm counterparts - but thats another
> story.
Yeah I think we can start deleting drivers for which we have drm
drivers which are mostly feature parity and see whether anyone pipes
up. There's always going to be the odd corner case (like apparently
the fbdev ati driver works better on some ppc machines than the drm
one).

The thing is, we can't delete the entire accel code with this, I think
only fb_copyarea goes away. The other hooks I think still have some
users.
-Daniel
>
>         Sam
>
> > @@ -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);
>
> Some bot will likely tell you that this causes warnings.
> At least it did in my sparc64 build.
>
> Fix:
>
> diff --git a/drivers/video/fbdev/core/fbcon.c
b/drivers/video/fbdev/core/fbcon.c
> index 398914e035e9..e8b009c621d8 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -2150,10 +2150,6 @@ static void updatescrollmode(struct fbcon_display
*p,
>  {
>         struct fbcon_ops *ops = info->fbcon_par;
>         int fh = vc->vc_font.height;
> -       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);
>
>
>

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

On 28. 10. 20, 17:06, Daniel Vetter wrote:
> 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").
...
> --- 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
If you go this non-invasive path, then only a nit here: "Garbage"

thanks,
-- 
js
suse labs