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
>>
>
