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

[Helge Deller]

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@xxxxxx>
>> Cc: stable@xxxxxxxxxxxxxxx # 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
>>
>