* [PATCH v3 0/3] Fix regression introduced by disabling accelerated scrolling in fbcon
@ 2022-02-01 18:59 Helge Deller
2022-02-01 18:59 ` [PATCH v3 1/3] Revert "fbdev: Garbage collect fbdev scrolling acceleration, part 1 (from TODO list)" Helge Deller
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Helge Deller @ 2022-02-01 18:59 UTC (permalink / raw)
To: DRI Development, linux-fbdev, Daniel Vetter
Cc: James Bottomley, John David Anglin
This series reverts two patches which disabled scrolling acceleration in
fbcon/fbdev. Those patches introduced a regression for fbdev-supported graphic
cards because of the performance penalty by doing screen scrolling by software
instead of using existing 2D hardware acceleration.
The third patch introduces a new config option
CONFIG_FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION, which -if enabled- switches
fbcon to utilize the bitblt and fillrect hardware acceleration in the
framebuffer console. If disabled, such acceleration will not be used, even if
it is supported by the graphics hardware driver.
This series is being discussed on linux-fbdev and dri-devel mailing lists.
Helge
Helge Deller (3):
Revert "fbdev: Garbage collect fbdev scrolling acceleration, part 1
(from TODO list)"
Revert "fbcon: Disable accelerated scrolling"
fbcon: Add option to enable legacy hardware acceleration
Documentation/gpu/todo.rst | 24 -
drivers/video/console/Kconfig | 11 +
drivers/video/fbdev/core/bitblit.c | 16 +
drivers/video/fbdev/core/fbcon.c | 557 +++++++++++++++++++++++-
drivers/video/fbdev/core/fbcon.h | 72 +++
drivers/video/fbdev/core/fbcon_ccw.c | 28 +-
drivers/video/fbdev/core/fbcon_cw.c | 28 +-
drivers/video/fbdev/core/fbcon_rotate.h | 9 +
drivers/video/fbdev/core/fbcon_ud.c | 37 +-
drivers/video/fbdev/core/tileblit.c | 16 +
drivers/video/fbdev/skeletonfb.c | 12 +-
include/linux/fb.h | 2 +-
12 files changed, 744 insertions(+), 68 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 1/3] Revert "fbdev: Garbage collect fbdev scrolling acceleration, part 1 (from TODO list)"
2022-02-01 18:59 [PATCH v3 0/3] Fix regression introduced by disabling accelerated scrolling in fbcon Helge Deller
@ 2022-02-01 18:59 ` Helge Deller
2022-02-01 18:59 ` [PATCH v3 2/3] Revert "fbcon: Disable accelerated scrolling" Helge Deller
2022-02-01 18:59 ` [PATCH v3 3/3] fbcon: Add option to enable legacy hardware acceleration Helge Deller
2 siblings, 0 replies; 12+ messages in thread
From: Helge Deller @ 2022-02-01 18:59 UTC (permalink / raw)
To: DRI Development, linux-fbdev, Daniel Vetter
Cc: James Bottomley, John David Anglin
This reverts commit b3ec8cdf457e5e63d396fe1346cc788cf7c1b578.
Revert the second (of 2) commits which disabled scrolling acceleration
in fbcon/fbdev. It introduced a regression for fbdev-supported graphic
cards because of the performance penalty by doing screen scrolling by
software instead of using the existing graphic card 2D hardware
acceleration.
Console scrolling acceleration was disabled by dropping code which
checked at runtime the driver hardware capabilities for the
BINFO_HWACCEL_COPYAREA or FBINFO_HWACCEL_FILLRECT flags and if set, it
enabled scrollmode SCROLL_MOVE which uses hardware acceleration to move
screen contents. After dropping those checks scrollmode was hard-wired
to SCROLL_REDRAW instead, which forces all graphic cards to redraw every
character at the new screen position when scrolling.
This change effectively disabled all hardware-based scrolling acceleration for
ALL drivers, because now all kind of 2D hardware acceleration (bitblt,
fillrect) in the drivers isn't used any longer.
The original commit message mentions that only 3 DRM drivers (nouveau, omapdrm
and gma500) used hardware acceleration in the past and thus code for checking
and using scrolling acceleration is obsolete.
This statement is NOT TRUE, because beside the DRM drivers there are around 35
other fbdev drivers which depend on fbdev/fbcon and still provide hardware
acceleration for fbdev/fbcon.
The original commit message also states that syzbot found lots of bugs in fbcon
and thus it's "often the solution to just delete code and remove features".
This is true, and the bugs - which actually affected all users of fbcon,
including DRM - were fixed, or code was dropped like e.g. the support for
software scrollback in vgacon (commit 973c096f6a85).
So to further analyze which bugs were found by syzbot, I've looked through all
patches in drivers/video which were tagged with syzbot or syzkaller back to
year 2005. The vast majority fixed the reported issues on a higher level, e.g.
when screen is to be resized, or when font size is to be changed. The few ones
which touched driver code fixed a real driver bug, e.g. by adding a check.
But NONE of those patches touched code of either the SCROLL_MOVE or the
SCROLL_REDRAW case.
That means, there was no real reason why SCROLL_MOVE had to be ripped-out and
just SCROLL_REDRAW had to be used instead. The only reason I can imagine so far
was that SCROLL_MOVE wasn't used by DRM and as such it was assumed that it
could go away. That argument completely missed the fact that SCROLL_MOVE is
still heavily used by fbdev (non-DRM) drivers.
Some people mention that using memcpy() instead of the hardware acceleration is
pretty much the same speed. But that's not true, at least not for older graphic
cards and machines where we see speed decreases by factor 10 and more and thus
this change leads to console responsiveness way worse than before.
That's why the original commit is to be reverted. By reverting we
reintroduce hardware-based scrolling acceleration and fix the
performance regression for fbdev drivers.
There isn't any impact on DRM when reverting those patches.
Signed-off-by: Helge Deller <deller@gmx.de>
Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
Acked-by: Sven Schnelle <svens@stackframe.org>
Cc: stable@vger.kernel.org # v5.16+
Signed-off-by: Helge Deller <deller@gmx.de>
---
Documentation/gpu/todo.rst | 13 +-
drivers/video/fbdev/core/bitblit.c | 16 +
drivers/video/fbdev/core/fbcon.c | 509 +++++++++++++++++++++++-
drivers/video/fbdev/core/fbcon.h | 59 +++
drivers/video/fbdev/core/fbcon_ccw.c | 28 +-
drivers/video/fbdev/core/fbcon_cw.c | 28 +-
drivers/video/fbdev/core/fbcon_rotate.h | 9 +
drivers/video/fbdev/core/fbcon_ud.c | 37 +-
drivers/video/fbdev/core/tileblit.c | 16 +
drivers/video/fbdev/skeletonfb.c | 12 +-
include/linux/fb.h | 2 +-
11 files changed, 678 insertions(+), 51 deletions(-)
diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index da138dd39883..29506815d24a 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -303,19 +303,16 @@ Level: Advanced
Garbage collect fbdev scrolling acceleration
--------------------------------------------
-Scroll acceleration has been disabled in fbcon. Now it works as the old
-SCROLL_REDRAW mode. A ton of code was removed in fbcon.c and the hook bmove was
-removed from fbcon_ops.
-Remaining tasks:
+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:
-- a bunch of the hooks in fbcon_ops could be removed or simplified by calling
+- 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
-- after that, fb_copyarea can be deleted from fb_ops in include/linux/fb.h as
- well as cfb_copyarea
-
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.
diff --git a/drivers/video/fbdev/core/bitblit.c b/drivers/video/fbdev/core/bitblit.c
index 01fae2c96965..f98e8f298bc1 100644
--- a/drivers/video/fbdev/core/bitblit.c
+++ b/drivers/video/fbdev/core/bitblit.c
@@ -43,6 +43,21 @@ static void update_attr(u8 *dst, u8 *src, int attribute,
}
}
+static void bit_bmove(struct vc_data *vc, struct fb_info *info, int sy,
+ int sx, int dy, int dx, int height, int width)
+{
+ struct fb_copyarea area;
+
+ area.sx = sx * vc->vc_font.width;
+ area.sy = sy * vc->vc_font.height;
+ area.dx = dx * vc->vc_font.width;
+ area.dy = dy * vc->vc_font.height;
+ area.height = height * vc->vc_font.height;
+ area.width = width * vc->vc_font.width;
+
+ info->fbops->fb_copyarea(info, &area);
+}
+
static void bit_clear(struct vc_data *vc, struct fb_info *info, int sy,
int sx, int height, int width)
{
@@ -378,6 +393,7 @@ static int bit_update_start(struct fb_info *info)
void fbcon_set_bitops(struct fbcon_ops *ops)
{
+ ops->bmove = bit_bmove;
ops->clear = bit_clear;
ops->putcs = bit_putcs;
ops->clear_margins = bit_clear_margins;
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 99ecd9a6d844..22bb3892f6bd 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -173,6 +173,8 @@ static void fbcon_putcs(struct vc_data *vc, const unsigned short *s,
int count, int ypos, int xpos);
static void fbcon_clear_margins(struct vc_data *vc, int bottom_only);
static void fbcon_cursor(struct vc_data *vc, int mode);
+static void fbcon_bmove(struct vc_data *vc, int sy, int sx, int dy, int dx,
+ int height, int width);
static int fbcon_switch(struct vc_data *vc);
static int fbcon_blank(struct vc_data *vc, int blank, int mode_switch);
static void fbcon_set_palette(struct vc_data *vc, const unsigned char *table);
@@ -180,8 +182,16 @@ static void fbcon_set_palette(struct vc_data *vc, const unsigned char *table);
/*
* Internal routines
*/
+static __inline__ void ywrap_up(struct vc_data *vc, int count);
+static __inline__ void ywrap_down(struct vc_data *vc, int count);
+static __inline__ void ypan_up(struct vc_data *vc, int count);
+static __inline__ void ypan_down(struct vc_data *vc, int count);
+static void fbcon_bmove_rec(struct vc_data *vc, struct fbcon_display *p, int sy, int sx,
+ int dy, int dx, int height, int width, u_int y_break);
static void fbcon_set_disp(struct fb_info *info, struct fb_var_screeninfo *var,
int unit);
+static void fbcon_redraw_move(struct vc_data *vc, struct fbcon_display *p,
+ int line, int count, int dy);
static void fbcon_modechanged(struct fb_info *info);
static void fbcon_set_all_vcs(struct fb_info *info);
static void fbcon_start(void);
@@ -1125,6 +1135,14 @@ 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;
+
/*
* ++guenther: console.c:vc_allocate() relies on initializing
* vc_{cols,rows}, but we must not set those if we are only
@@ -1211,13 +1229,14 @@ static void fbcon_deinit(struct vc_data *vc)
* This system is now divided into two levels because of complications
* caused by hardware scrolling. Top level functions:
*
- * fbcon_clear(), fbcon_putc(), fbcon_clear_margins()
+ * fbcon_bmove(), fbcon_clear(), fbcon_putc(), fbcon_clear_margins()
*
* handles y values in range [0, scr_height-1] that correspond to real
* screen positions. y_wrap shift means that first line of bitmap may be
* anywhere on this display. These functions convert lineoffsets to
* bitmap offsets and deal with the wrap-around case by splitting blits.
*
+ * fbcon_bmove_physical_8() -- These functions fast implementations
* fbcon_clear_physical_8() -- of original fbcon_XXX fns.
* fbcon_putc_physical_8() -- (font width != 8) may be added later
*
@@ -1390,6 +1409,224 @@ static void fbcon_set_disp(struct fb_info *info, struct fb_var_screeninfo *var,
}
}
+static __inline__ void ywrap_up(struct vc_data *vc, int count)
+{
+ struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
+ struct fbcon_ops *ops = info->fbcon_par;
+ struct fbcon_display *p = &fb_display[vc->vc_num];
+
+ p->yscroll += count;
+ if (p->yscroll >= p->vrows) /* Deal with wrap */
+ p->yscroll -= p->vrows;
+ ops->var.xoffset = 0;
+ ops->var.yoffset = p->yscroll * vc->vc_font.height;
+ ops->var.vmode |= FB_VMODE_YWRAP;
+ ops->update_start(info);
+ scrollback_max += count;
+ if (scrollback_max > scrollback_phys_max)
+ scrollback_max = scrollback_phys_max;
+ scrollback_current = 0;
+}
+
+static __inline__ void ywrap_down(struct vc_data *vc, int count)
+{
+ struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
+ struct fbcon_ops *ops = info->fbcon_par;
+ struct fbcon_display *p = &fb_display[vc->vc_num];
+
+ p->yscroll -= count;
+ if (p->yscroll < 0) /* Deal with wrap */
+ p->yscroll += p->vrows;
+ ops->var.xoffset = 0;
+ ops->var.yoffset = p->yscroll * vc->vc_font.height;
+ ops->var.vmode |= FB_VMODE_YWRAP;
+ ops->update_start(info);
+ scrollback_max -= count;
+ if (scrollback_max < 0)
+ scrollback_max = 0;
+ scrollback_current = 0;
+}
+
+static __inline__ void ypan_up(struct vc_data *vc, int count)
+{
+ struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
+ struct fbcon_display *p = &fb_display[vc->vc_num];
+ struct fbcon_ops *ops = info->fbcon_par;
+
+ p->yscroll += count;
+ if (p->yscroll > p->vrows - vc->vc_rows) {
+ ops->bmove(vc, info, p->vrows - vc->vc_rows,
+ 0, 0, 0, vc->vc_rows, vc->vc_cols);
+ p->yscroll -= p->vrows - vc->vc_rows;
+ }
+
+ ops->var.xoffset = 0;
+ ops->var.yoffset = p->yscroll * vc->vc_font.height;
+ ops->var.vmode &= ~FB_VMODE_YWRAP;
+ ops->update_start(info);
+ fbcon_clear_margins(vc, 1);
+ scrollback_max += count;
+ if (scrollback_max > scrollback_phys_max)
+ scrollback_max = scrollback_phys_max;
+ scrollback_current = 0;
+}
+
+static __inline__ void ypan_up_redraw(struct vc_data *vc, int t, int count)
+{
+ struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
+ struct fbcon_ops *ops = info->fbcon_par;
+ struct fbcon_display *p = &fb_display[vc->vc_num];
+
+ p->yscroll += count;
+
+ if (p->yscroll > p->vrows - vc->vc_rows) {
+ p->yscroll -= p->vrows - vc->vc_rows;
+ fbcon_redraw_move(vc, p, t + count, vc->vc_rows - count, t);
+ }
+
+ ops->var.xoffset = 0;
+ ops->var.yoffset = p->yscroll * vc->vc_font.height;
+ ops->var.vmode &= ~FB_VMODE_YWRAP;
+ ops->update_start(info);
+ fbcon_clear_margins(vc, 1);
+ scrollback_max += count;
+ if (scrollback_max > scrollback_phys_max)
+ scrollback_max = scrollback_phys_max;
+ scrollback_current = 0;
+}
+
+static __inline__ void ypan_down(struct vc_data *vc, int count)
+{
+ struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
+ struct fbcon_display *p = &fb_display[vc->vc_num];
+ struct fbcon_ops *ops = info->fbcon_par;
+
+ p->yscroll -= count;
+ if (p->yscroll < 0) {
+ ops->bmove(vc, info, 0, 0, p->vrows - vc->vc_rows,
+ 0, vc->vc_rows, vc->vc_cols);
+ p->yscroll += p->vrows - vc->vc_rows;
+ }
+
+ ops->var.xoffset = 0;
+ ops->var.yoffset = p->yscroll * vc->vc_font.height;
+ ops->var.vmode &= ~FB_VMODE_YWRAP;
+ ops->update_start(info);
+ fbcon_clear_margins(vc, 1);
+ scrollback_max -= count;
+ if (scrollback_max < 0)
+ scrollback_max = 0;
+ scrollback_current = 0;
+}
+
+static __inline__ void ypan_down_redraw(struct vc_data *vc, int t, int count)
+{
+ struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
+ struct fbcon_ops *ops = info->fbcon_par;
+ struct fbcon_display *p = &fb_display[vc->vc_num];
+
+ p->yscroll -= count;
+
+ if (p->yscroll < 0) {
+ p->yscroll += p->vrows - vc->vc_rows;
+ fbcon_redraw_move(vc, p, t, vc->vc_rows - count, t + count);
+ }
+
+ ops->var.xoffset = 0;
+ ops->var.yoffset = p->yscroll * vc->vc_font.height;
+ ops->var.vmode &= ~FB_VMODE_YWRAP;
+ ops->update_start(info);
+ fbcon_clear_margins(vc, 1);
+ scrollback_max -= count;
+ if (scrollback_max < 0)
+ scrollback_max = 0;
+ scrollback_current = 0;
+}
+
+static void fbcon_redraw_move(struct vc_data *vc, struct fbcon_display *p,
+ int line, int count, int dy)
+{
+ unsigned short *s = (unsigned short *)
+ (vc->vc_origin + vc->vc_size_row * line);
+
+ while (count--) {
+ unsigned short *start = s;
+ unsigned short *le = advance_row(s, 1);
+ unsigned short c;
+ int x = 0;
+ unsigned short attr = 1;
+
+ do {
+ c = scr_readw(s);
+ if (attr != (c & 0xff00)) {
+ attr = c & 0xff00;
+ if (s > start) {
+ fbcon_putcs(vc, start, s - start,
+ dy, x);
+ x += s - start;
+ start = s;
+ }
+ }
+ console_conditional_schedule();
+ s++;
+ } while (s < le);
+ if (s > start)
+ fbcon_putcs(vc, start, s - start, dy, x);
+ console_conditional_schedule();
+ dy++;
+ }
+}
+
+static void fbcon_redraw_blit(struct vc_data *vc, struct fb_info *info,
+ struct fbcon_display *p, int line, int count, int ycount)
+{
+ int offset = ycount * vc->vc_cols;
+ unsigned short *d = (unsigned short *)
+ (vc->vc_origin + vc->vc_size_row * line);
+ unsigned short *s = d + offset;
+ struct fbcon_ops *ops = info->fbcon_par;
+
+ while (count--) {
+ unsigned short *start = s;
+ unsigned short *le = advance_row(s, 1);
+ unsigned short c;
+ int x = 0;
+
+ do {
+ c = scr_readw(s);
+
+ if (c == scr_readw(d)) {
+ if (s > start) {
+ ops->bmove(vc, info, line + ycount, x,
+ line, x, 1, s-start);
+ x += s - start + 1;
+ start = s + 1;
+ } else {
+ x++;
+ start++;
+ }
+ }
+
+ scr_writew(c, d);
+ console_conditional_schedule();
+ s++;
+ d++;
+ } while (s < le);
+ if (s > start)
+ ops->bmove(vc, info, line + ycount, x, line, x, 1,
+ s-start);
+ console_conditional_schedule();
+ if (ycount > 0)
+ line++;
+ else {
+ line--;
+ /* NOTE: We subtract two lines from these pointers */
+ s -= vc->vc_size_row;
+ d -= vc->vc_size_row;
+ }
+ }
+}
+
static void fbcon_redraw(struct vc_data *vc, struct fbcon_display *p,
int line, int count, int offset)
{
@@ -1450,6 +1687,7 @@ static bool fbcon_scroll(struct vc_data *vc, unsigned int t, unsigned int b,
{
struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
struct fbcon_display *p = &fb_display[vc->vc_num];
+ int scroll_partial = info->flags & FBINFO_PARTIAL_PAN_OK;
if (fbcon_is_inactive(vc, info))
return true;
@@ -1466,32 +1704,249 @@ static bool fbcon_scroll(struct vc_data *vc, unsigned int t, unsigned int b,
case SM_UP:
if (count > vc->vc_rows) /* Maximum realistic size */
count = vc->vc_rows;
- fbcon_redraw(vc, p, t, b - t - count,
- count * vc->vc_cols);
- fbcon_clear(vc, b - count, 0, count, vc->vc_cols);
- scr_memsetw((unsigned short *) (vc->vc_origin +
- vc->vc_size_row *
- (b - count)),
- vc->vc_video_erase_char,
- vc->vc_size_row * count);
- return true;
+ if (logo_shown >= 0)
+ goto redraw_up;
+ switch (p->scrollmode) {
+ case SCROLL_MOVE:
+ fbcon_redraw_blit(vc, info, p, t, b - t - count,
+ count);
+ fbcon_clear(vc, b - count, 0, count, vc->vc_cols);
+ scr_memsetw((unsigned short *) (vc->vc_origin +
+ vc->vc_size_row *
+ (b - count)),
+ vc->vc_video_erase_char,
+ vc->vc_size_row * count);
+ return true;
+
+ case SCROLL_WRAP_MOVE:
+ if (b - t - count > 3 * vc->vc_rows >> 2) {
+ if (t > 0)
+ fbcon_bmove(vc, 0, 0, count, 0, t,
+ vc->vc_cols);
+ ywrap_up(vc, count);
+ if (vc->vc_rows - b > 0)
+ fbcon_bmove(vc, b - count, 0, b, 0,
+ vc->vc_rows - b,
+ vc->vc_cols);
+ } else if (info->flags & FBINFO_READS_FAST)
+ fbcon_bmove(vc, t + count, 0, t, 0,
+ b - t - count, vc->vc_cols);
+ else
+ goto redraw_up;
+ fbcon_clear(vc, b - count, 0, count, vc->vc_cols);
+ break;
+
+ case SCROLL_PAN_REDRAW:
+ if ((p->yscroll + count <=
+ 2 * (p->vrows - vc->vc_rows))
+ && ((!scroll_partial && (b - t == vc->vc_rows))
+ || (scroll_partial
+ && (b - t - count >
+ 3 * vc->vc_rows >> 2)))) {
+ if (t > 0)
+ fbcon_redraw_move(vc, p, 0, t, count);
+ ypan_up_redraw(vc, t, count);
+ if (vc->vc_rows - b > 0)
+ fbcon_redraw_move(vc, p, b,
+ vc->vc_rows - b, b);
+ } else
+ fbcon_redraw_move(vc, p, t + count, b - t - count, t);
+ fbcon_clear(vc, b - count, 0, count, vc->vc_cols);
+ break;
+
+ case SCROLL_PAN_MOVE:
+ if ((p->yscroll + count <=
+ 2 * (p->vrows - vc->vc_rows))
+ && ((!scroll_partial && (b - t == vc->vc_rows))
+ || (scroll_partial
+ && (b - t - count >
+ 3 * vc->vc_rows >> 2)))) {
+ if (t > 0)
+ fbcon_bmove(vc, 0, 0, count, 0, t,
+ vc->vc_cols);
+ ypan_up(vc, count);
+ if (vc->vc_rows - b > 0)
+ fbcon_bmove(vc, b - count, 0, b, 0,
+ vc->vc_rows - b,
+ vc->vc_cols);
+ } else if (info->flags & FBINFO_READS_FAST)
+ fbcon_bmove(vc, t + count, 0, t, 0,
+ b - t - count, vc->vc_cols);
+ else
+ goto redraw_up;
+ fbcon_clear(vc, b - count, 0, count, vc->vc_cols);
+ break;
+
+ case SCROLL_REDRAW:
+ redraw_up:
+ fbcon_redraw(vc, p, t, b - t - count,
+ count * vc->vc_cols);
+ fbcon_clear(vc, b - count, 0, count, vc->vc_cols);
+ scr_memsetw((unsigned short *) (vc->vc_origin +
+ vc->vc_size_row *
+ (b - count)),
+ vc->vc_video_erase_char,
+ vc->vc_size_row * count);
+ return true;
+ }
+ break;
case SM_DOWN:
if (count > vc->vc_rows) /* Maximum realistic size */
count = vc->vc_rows;
- fbcon_redraw(vc, p, b - 1, b - t - count,
- -count * vc->vc_cols);
- fbcon_clear(vc, t, 0, count, vc->vc_cols);
- scr_memsetw((unsigned short *) (vc->vc_origin +
- vc->vc_size_row *
- t),
- vc->vc_video_erase_char,
- vc->vc_size_row * count);
- return true;
+ if (logo_shown >= 0)
+ goto redraw_down;
+ switch (p->scrollmode) {
+ case SCROLL_MOVE:
+ fbcon_redraw_blit(vc, info, p, b - 1, b - t - count,
+ -count);
+ fbcon_clear(vc, t, 0, count, vc->vc_cols);
+ scr_memsetw((unsigned short *) (vc->vc_origin +
+ vc->vc_size_row *
+ t),
+ vc->vc_video_erase_char,
+ vc->vc_size_row * count);
+ return true;
+
+ case SCROLL_WRAP_MOVE:
+ if (b - t - count > 3 * vc->vc_rows >> 2) {
+ if (vc->vc_rows - b > 0)
+ fbcon_bmove(vc, b, 0, b - count, 0,
+ vc->vc_rows - b,
+ vc->vc_cols);
+ ywrap_down(vc, count);
+ if (t > 0)
+ fbcon_bmove(vc, count, 0, 0, 0, t,
+ vc->vc_cols);
+ } else if (info->flags & FBINFO_READS_FAST)
+ fbcon_bmove(vc, t, 0, t + count, 0,
+ b - t - count, vc->vc_cols);
+ else
+ goto redraw_down;
+ fbcon_clear(vc, t, 0, count, vc->vc_cols);
+ break;
+
+ case SCROLL_PAN_MOVE:
+ if ((count - p->yscroll <= p->vrows - vc->vc_rows)
+ && ((!scroll_partial && (b - t == vc->vc_rows))
+ || (scroll_partial
+ && (b - t - count >
+ 3 * vc->vc_rows >> 2)))) {
+ if (vc->vc_rows - b > 0)
+ fbcon_bmove(vc, b, 0, b - count, 0,
+ vc->vc_rows - b,
+ vc->vc_cols);
+ ypan_down(vc, count);
+ if (t > 0)
+ fbcon_bmove(vc, count, 0, 0, 0, t,
+ vc->vc_cols);
+ } else if (info->flags & FBINFO_READS_FAST)
+ fbcon_bmove(vc, t, 0, t + count, 0,
+ b - t - count, vc->vc_cols);
+ else
+ goto redraw_down;
+ fbcon_clear(vc, t, 0, count, vc->vc_cols);
+ break;
+
+ case SCROLL_PAN_REDRAW:
+ if ((count - p->yscroll <= p->vrows - vc->vc_rows)
+ && ((!scroll_partial && (b - t == vc->vc_rows))
+ || (scroll_partial
+ && (b - t - count >
+ 3 * vc->vc_rows >> 2)))) {
+ if (vc->vc_rows - b > 0)
+ fbcon_redraw_move(vc, p, b, vc->vc_rows - b,
+ b - count);
+ ypan_down_redraw(vc, t, count);
+ if (t > 0)
+ fbcon_redraw_move(vc, p, count, t, 0);
+ } else
+ fbcon_redraw_move(vc, p, t, b - t - count, t + count);
+ fbcon_clear(vc, t, 0, count, vc->vc_cols);
+ break;
+
+ case SCROLL_REDRAW:
+ redraw_down:
+ fbcon_redraw(vc, p, b - 1, b - t - count,
+ -count * vc->vc_cols);
+ fbcon_clear(vc, t, 0, count, vc->vc_cols);
+ scr_memsetw((unsigned short *) (vc->vc_origin +
+ vc->vc_size_row *
+ t),
+ vc->vc_video_erase_char,
+ vc->vc_size_row * count);
+ return true;
+ }
}
return false;
}
+
+static void fbcon_bmove(struct vc_data *vc, int sy, int sx, int dy, int dx,
+ int height, int width)
+{
+ struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
+ struct fbcon_display *p = &fb_display[vc->vc_num];
+
+ if (fbcon_is_inactive(vc, info))
+ return;
+
+ if (!width || !height)
+ return;
+
+ /* Split blits that cross physical y_wrap case.
+ * Pathological case involves 4 blits, better to use recursive
+ * code rather than unrolled case
+ *
+ * Recursive invocations don't need to erase the cursor over and
+ * over again, so we use fbcon_bmove_rec()
+ */
+ fbcon_bmove_rec(vc, p, sy, sx, dy, dx, height, width,
+ p->vrows - p->yscroll);
+}
+
+static void fbcon_bmove_rec(struct vc_data *vc, struct fbcon_display *p, int sy, int sx,
+ int dy, int dx, int height, int width, u_int y_break)
+{
+ struct fb_info *info = registered_fb[con2fb_map[vc->vc_num]];
+ struct fbcon_ops *ops = info->fbcon_par;
+ u_int b;
+
+ if (sy < y_break && sy + height > y_break) {
+ b = y_break - sy;
+ if (dy < sy) { /* Avoid trashing self */
+ fbcon_bmove_rec(vc, p, sy, sx, dy, dx, b, width,
+ y_break);
+ fbcon_bmove_rec(vc, p, sy + b, sx, dy + b, dx,
+ height - b, width, y_break);
+ } else {
+ fbcon_bmove_rec(vc, p, sy + b, sx, dy + b, dx,
+ height - b, width, y_break);
+ fbcon_bmove_rec(vc, p, sy, sx, dy, dx, b, width,
+ y_break);
+ }
+ return;
+ }
+
+ if (dy < y_break && dy + height > y_break) {
+ b = y_break - dy;
+ if (dy < sy) { /* Avoid trashing self */
+ fbcon_bmove_rec(vc, p, sy, sx, dy, dx, b, width,
+ y_break);
+ fbcon_bmove_rec(vc, p, sy + b, sx, dy + b, dx,
+ height - b, width, y_break);
+ } else {
+ fbcon_bmove_rec(vc, p, sy + b, sx, dy + b, dx,
+ height - b, width, y_break);
+ fbcon_bmove_rec(vc, p, sy, sx, dy, dx, b, width,
+ y_break);
+ }
+ return;
+ }
+ ops->bmove(vc, info, real_y(p, sy), sx, real_y(p, dy), dx,
+ height, width);
+}
+
static void updatescrollmode(struct fbcon_display *p,
struct fb_info *info,
struct vc_data *vc)
@@ -1664,7 +2119,21 @@ static int fbcon_switch(struct vc_data *vc)
updatescrollmode(p, info, vc);
- scrollback_phys_max = 0;
+ switch (p->scrollmode) {
+ case SCROLL_WRAP_MOVE:
+ scrollback_phys_max = p->vrows - vc->vc_rows;
+ break;
+ case SCROLL_PAN_MOVE:
+ case SCROLL_PAN_REDRAW:
+ scrollback_phys_max = p->vrows - 2 * vc->vc_rows;
+ if (scrollback_phys_max < 0)
+ scrollback_phys_max = 0;
+ break;
+ default:
+ scrollback_phys_max = 0;
+ break;
+ }
+
scrollback_max = 0;
scrollback_current = 0;
diff --git a/drivers/video/fbdev/core/fbcon.h b/drivers/video/fbdev/core/fbcon.h
index a00603b4451a..9315b360c898 100644
--- a/drivers/video/fbdev/core/fbcon.h
+++ b/drivers/video/fbdev/core/fbcon.h
@@ -29,6 +29,7 @@ struct fbcon_display {
/* Filled in by the low-level console driver */
const u_char *fontdata;
int userfont; /* != 0 if fontdata kmalloc()ed */
+ u_short scrollmode; /* Scroll Method */
u_short inverse; /* != 0 text black on white as default */
short yscroll; /* Hardware scrolling */
int vrows; /* number of virtual rows */
@@ -51,6 +52,8 @@ struct fbcon_display {
};
struct fbcon_ops {
+ void (*bmove)(struct vc_data *vc, struct fb_info *info, int sy,
+ int sx, int dy, int dx, int height, int width);
void (*clear)(struct vc_data *vc, struct fb_info *info, int sy,
int sx, int height, int width);
void (*putcs)(struct vc_data *vc, struct fb_info *info,
@@ -149,6 +152,62 @@ static inline int attr_col_ec(int shift, struct vc_data *vc,
#define attr_bgcol_ec(bgshift, vc, info) attr_col_ec(bgshift, vc, info, 0)
#define attr_fgcol_ec(fgshift, vc, info) attr_col_ec(fgshift, vc, info, 1)
+ /*
+ * Scroll Method
+ */
+
+/* There are several methods fbcon can use to move text around the screen:
+ *
+ * Operation Pan Wrap
+ *---------------------------------------------
+ * SCROLL_MOVE copyarea No No
+ * SCROLL_PAN_MOVE copyarea Yes No
+ * SCROLL_WRAP_MOVE copyarea No Yes
+ * SCROLL_REDRAW imageblit No No
+ * SCROLL_PAN_REDRAW imageblit Yes No
+ * SCROLL_WRAP_REDRAW imageblit No Yes
+ *
+ * (SCROLL_WRAP_REDRAW is not implemented yet)
+ *
+ * In general, fbcon will choose the best scrolling
+ * method based on the rule below:
+ *
+ * Pan/Wrap > accel imageblit > accel copyarea >
+ * soft imageblit > (soft copyarea)
+ *
+ * Exception to the rule: Pan + accel copyarea is
+ * preferred over Pan + accel imageblit.
+ *
+ * The above is typical for PCI/AGP cards. Unless
+ * overridden, fbcon will never use soft copyarea.
+ *
+ * If you need to override the above rule, set the
+ * appropriate flags in fb_info->flags. For example,
+ * to prefer copyarea over imageblit, set
+ * FBINFO_READS_FAST.
+ *
+ * Other notes:
+ * + use the hardware engine to move the text
+ * (hw-accelerated copyarea() and fillrect())
+ * + use hardware-supported panning on a large virtual screen
+ * + amifb can not only pan, but also wrap the display by N lines
+ * (i.e. visible line i = physical line (i+N) % yres).
+ * + read what's already rendered on the screen and
+ * write it in a different place (this is cfb_copyarea())
+ * + re-render the text to the screen
+ *
+ * Whether to use wrapping or panning can only be figured out at
+ * runtime (when we know whether our font height is a multiple
+ * of the pan/wrap step)
+ *
+ */
+
+#define SCROLL_MOVE 0x001
+#define SCROLL_PAN_MOVE 0x002
+#define SCROLL_WRAP_MOVE 0x003
+#define SCROLL_REDRAW 0x004
+#define SCROLL_PAN_REDRAW 0x005
+
#ifdef CONFIG_FB_TILEBLITTING
extern void fbcon_set_tileops(struct vc_data *vc, struct fb_info *info);
#endif
diff --git a/drivers/video/fbdev/core/fbcon_ccw.c b/drivers/video/fbdev/core/fbcon_ccw.c
index ffa78936eaab..9cd2c4b05c32 100644
--- a/drivers/video/fbdev/core/fbcon_ccw.c
+++ b/drivers/video/fbdev/core/fbcon_ccw.c
@@ -59,12 +59,31 @@ static void ccw_update_attr(u8 *dst, u8 *src, int attribute,
}
}
+
+static void ccw_bmove(struct vc_data *vc, struct fb_info *info, int sy,
+ int sx, int dy, int dx, int height, int width)
+{
+ struct fbcon_ops *ops = info->fbcon_par;
+ struct fb_copyarea area;
+ u32 vyres = GETVYRES(ops->p->scrollmode, info);
+
+ area.sx = sy * vc->vc_font.height;
+ area.sy = vyres - ((sx + width) * vc->vc_font.width);
+ area.dx = dy * vc->vc_font.height;
+ area.dy = vyres - ((dx + width) * vc->vc_font.width);
+ area.width = height * vc->vc_font.height;
+ area.height = width * vc->vc_font.width;
+
+ info->fbops->fb_copyarea(info, &area);
+}
+
static void ccw_clear(struct vc_data *vc, struct fb_info *info, int sy,
int sx, int height, int width)
{
+ struct fbcon_ops *ops = info->fbcon_par;
struct fb_fillrect region;
int bgshift = (vc->vc_hi_font_mask) ? 13 : 12;
- u32 vyres = info->var.yres;
+ u32 vyres = GETVYRES(ops->p->scrollmode, info);
region.color = attr_bgcol_ec(bgshift,vc,info);
region.dx = sy * vc->vc_font.height;
@@ -121,7 +140,7 @@ static void ccw_putcs(struct vc_data *vc, struct fb_info *info,
u32 cnt, pitch, size;
u32 attribute = get_attribute(info, scr_readw(s));
u8 *dst, *buf = NULL;
- u32 vyres = info->var.yres;
+ u32 vyres = GETVYRES(ops->p->scrollmode, info);
if (!ops->fontbuffer)
return;
@@ -210,7 +229,7 @@ static void ccw_cursor(struct vc_data *vc, struct fb_info *info, int mode,
int attribute, use_sw = vc->vc_cursor_type & CUR_SW;
int err = 1, dx, dy;
char *src;
- u32 vyres = info->var.yres;
+ u32 vyres = GETVYRES(ops->p->scrollmode, info);
if (!ops->fontbuffer)
return;
@@ -368,7 +387,7 @@ static int ccw_update_start(struct fb_info *info)
{
struct fbcon_ops *ops = info->fbcon_par;
u32 yoffset;
- u32 vyres = info->var.yres;
+ u32 vyres = GETVYRES(ops->p->scrollmode, info);
int err;
yoffset = (vyres - info->var.yres) - ops->var.xoffset;
@@ -383,6 +402,7 @@ static int ccw_update_start(struct fb_info *info)
void fbcon_rotate_ccw(struct fbcon_ops *ops)
{
+ ops->bmove = ccw_bmove;
ops->clear = ccw_clear;
ops->putcs = ccw_putcs;
ops->clear_margins = ccw_clear_margins;
diff --git a/drivers/video/fbdev/core/fbcon_cw.c b/drivers/video/fbdev/core/fbcon_cw.c
index 92e5b7fb51ee..88d89fad3f05 100644
--- a/drivers/video/fbdev/core/fbcon_cw.c
+++ b/drivers/video/fbdev/core/fbcon_cw.c
@@ -44,12 +44,31 @@ static void cw_update_attr(u8 *dst, u8 *src, int attribute,
}
}
+
+static void cw_bmove(struct vc_data *vc, struct fb_info *info, int sy,
+ int sx, int dy, int dx, int height, int width)
+{
+ struct fbcon_ops *ops = info->fbcon_par;
+ struct fb_copyarea area;
+ u32 vxres = GETVXRES(ops->p->scrollmode, info);
+
+ area.sx = vxres - ((sy + height) * vc->vc_font.height);
+ area.sy = sx * vc->vc_font.width;
+ area.dx = vxres - ((dy + height) * vc->vc_font.height);
+ area.dy = dx * vc->vc_font.width;
+ area.width = height * vc->vc_font.height;
+ area.height = width * vc->vc_font.width;
+
+ info->fbops->fb_copyarea(info, &area);
+}
+
static void cw_clear(struct vc_data *vc, struct fb_info *info, int sy,
int sx, int height, int width)
{
+ struct fbcon_ops *ops = info->fbcon_par;
struct fb_fillrect region;
int bgshift = (vc->vc_hi_font_mask) ? 13 : 12;
- u32 vxres = info->var.xres;
+ u32 vxres = GETVXRES(ops->p->scrollmode, info);
region.color = attr_bgcol_ec(bgshift,vc,info);
region.dx = vxres - ((sy + height) * vc->vc_font.height);
@@ -106,7 +125,7 @@ static void cw_putcs(struct vc_data *vc, struct fb_info *info,
u32 cnt, pitch, size;
u32 attribute = get_attribute(info, scr_readw(s));
u8 *dst, *buf = NULL;
- u32 vxres = info->var.xres;
+ u32 vxres = GETVXRES(ops->p->scrollmode, info);
if (!ops->fontbuffer)
return;
@@ -193,7 +212,7 @@ static void cw_cursor(struct vc_data *vc, struct fb_info *info, int mode,
int attribute, use_sw = vc->vc_cursor_type & CUR_SW;
int err = 1, dx, dy;
char *src;
- u32 vxres = info->var.xres;
+ u32 vxres = GETVXRES(ops->p->scrollmode, info);
if (!ops->fontbuffer)
return;
@@ -350,7 +369,7 @@ static void cw_cursor(struct vc_data *vc, struct fb_info *info, int mode,
static int cw_update_start(struct fb_info *info)
{
struct fbcon_ops *ops = info->fbcon_par;
- u32 vxres = info->var.xres;
+ u32 vxres = GETVXRES(ops->p->scrollmode, info);
u32 xoffset;
int err;
@@ -366,6 +385,7 @@ static int cw_update_start(struct fb_info *info)
void fbcon_rotate_cw(struct fbcon_ops *ops)
{
+ ops->bmove = cw_bmove;
ops->clear = cw_clear;
ops->putcs = cw_putcs;
ops->clear_margins = cw_clear_margins;
diff --git a/drivers/video/fbdev/core/fbcon_rotate.h b/drivers/video/fbdev/core/fbcon_rotate.h
index b528b2e54283..e233444cda66 100644
--- a/drivers/video/fbdev/core/fbcon_rotate.h
+++ b/drivers/video/fbdev/core/fbcon_rotate.h
@@ -11,6 +11,15 @@
#ifndef _FBCON_ROTATE_H
#define _FBCON_ROTATE_H
+#define GETVYRES(s,i) ({ \
+ (s == SCROLL_REDRAW || s == SCROLL_MOVE) ? \
+ (i)->var.yres : (i)->var.yres_virtual; })
+
+#define GETVXRES(s,i) ({ \
+ (s == SCROLL_REDRAW || s == SCROLL_MOVE || !(i)->fix.xpanstep) ? \
+ (i)->var.xres : (i)->var.xres_virtual; })
+
+
static inline int pattern_test_bit(u32 x, u32 y, u32 pitch, const char *pat)
{
u32 tmp = (y * pitch) + x, index = tmp / 8, bit = tmp % 8;
diff --git a/drivers/video/fbdev/core/fbcon_ud.c b/drivers/video/fbdev/core/fbcon_ud.c
index 09619bd8e021..8d5e66b1bdfb 100644
--- a/drivers/video/fbdev/core/fbcon_ud.c
+++ b/drivers/video/fbdev/core/fbcon_ud.c
@@ -44,13 +44,33 @@ static void ud_update_attr(u8 *dst, u8 *src, int attribute,
}
}
+
+static void ud_bmove(struct vc_data *vc, struct fb_info *info, int sy,
+ int sx, int dy, int dx, int height, int width)
+{
+ struct fbcon_ops *ops = info->fbcon_par;
+ struct fb_copyarea area;
+ u32 vyres = GETVYRES(ops->p->scrollmode, info);
+ u32 vxres = GETVXRES(ops->p->scrollmode, info);
+
+ area.sy = vyres - ((sy + height) * vc->vc_font.height);
+ area.sx = vxres - ((sx + width) * vc->vc_font.width);
+ area.dy = vyres - ((dy + height) * vc->vc_font.height);
+ area.dx = vxres - ((dx + width) * vc->vc_font.width);
+ area.height = height * vc->vc_font.height;
+ area.width = width * vc->vc_font.width;
+
+ info->fbops->fb_copyarea(info, &area);
+}
+
static void ud_clear(struct vc_data *vc, struct fb_info *info, int sy,
int sx, int height, int width)
{
+ struct fbcon_ops *ops = info->fbcon_par;
struct fb_fillrect region;
int bgshift = (vc->vc_hi_font_mask) ? 13 : 12;
- u32 vyres = info->var.yres;
- u32 vxres = info->var.xres;
+ u32 vyres = GETVYRES(ops->p->scrollmode, info);
+ u32 vxres = GETVXRES(ops->p->scrollmode, info);
region.color = attr_bgcol_ec(bgshift,vc,info);
region.dy = vyres - ((sy + height) * vc->vc_font.height);
@@ -142,8 +162,8 @@ static void ud_putcs(struct vc_data *vc, struct fb_info *info,
u32 mod = vc->vc_font.width % 8, cnt, pitch, size;
u32 attribute = get_attribute(info, scr_readw(s));
u8 *dst, *buf = NULL;
- u32 vyres = info->var.yres;
- u32 vxres = info->var.xres;
+ u32 vyres = GETVYRES(ops->p->scrollmode, info);
+ u32 vxres = GETVXRES(ops->p->scrollmode, info);
if (!ops->fontbuffer)
return;
@@ -239,8 +259,8 @@ static void ud_cursor(struct vc_data *vc, struct fb_info *info, int mode,
int attribute, use_sw = vc->vc_cursor_type & CUR_SW;
int err = 1, dx, dy;
char *src;
- u32 vyres = info->var.yres;
- u32 vxres = info->var.xres;
+ u32 vyres = GETVYRES(ops->p->scrollmode, info);
+ u32 vxres = GETVXRES(ops->p->scrollmode, info);
if (!ops->fontbuffer)
return;
@@ -390,8 +410,8 @@ static int ud_update_start(struct fb_info *info)
{
struct fbcon_ops *ops = info->fbcon_par;
int xoffset, yoffset;
- u32 vyres = info->var.yres;
- u32 vxres = info->var.xres;
+ u32 vyres = GETVYRES(ops->p->scrollmode, info);
+ u32 vxres = GETVXRES(ops->p->scrollmode, info);
int err;
xoffset = vxres - info->var.xres - ops->var.xoffset;
@@ -409,6 +429,7 @@ static int ud_update_start(struct fb_info *info)
void fbcon_rotate_ud(struct fbcon_ops *ops)
{
+ ops->bmove = ud_bmove;
ops->clear = ud_clear;
ops->putcs = ud_putcs;
ops->clear_margins = ud_clear_margins;
diff --git a/drivers/video/fbdev/core/tileblit.c b/drivers/video/fbdev/core/tileblit.c
index 72af95053bcb..2768eff247ba 100644
--- a/drivers/video/fbdev/core/tileblit.c
+++ b/drivers/video/fbdev/core/tileblit.c
@@ -16,6 +16,21 @@
#include <asm/types.h>
#include "fbcon.h"
+static void tile_bmove(struct vc_data *vc, struct fb_info *info, int sy,
+ int sx, int dy, int dx, int height, int width)
+{
+ struct fb_tilearea area;
+
+ area.sx = sx;
+ area.sy = sy;
+ area.dx = dx;
+ area.dy = dy;
+ area.height = height;
+ area.width = width;
+
+ info->tileops->fb_tilecopy(info, &area);
+}
+
static void tile_clear(struct vc_data *vc, struct fb_info *info, int sy,
int sx, int height, int width)
{
@@ -118,6 +133,7 @@ void fbcon_set_tileops(struct vc_data *vc, struct fb_info *info)
struct fb_tilemap map;
struct fbcon_ops *ops = info->fbcon_par;
+ ops->bmove = tile_bmove;
ops->clear = tile_clear;
ops->putcs = tile_putcs;
ops->clear_margins = tile_clear_margins;
diff --git a/drivers/video/fbdev/skeletonfb.c b/drivers/video/fbdev/skeletonfb.c
index 0fe922f726e9..bcacfb6934fa 100644
--- a/drivers/video/fbdev/skeletonfb.c
+++ b/drivers/video/fbdev/skeletonfb.c
@@ -505,15 +505,15 @@ void xxxfb_fillrect(struct fb_info *p, const struct fb_fillrect *region)
}
/**
- * xxxfb_copyarea - OBSOLETE function.
+ * xxxfb_copyarea - REQUIRED function. Can use generic routines if
+ * non acclerated hardware and packed pixel based.
* Copies one area of the screen to another area.
- * Will be deleted in a future version
*
* @info: frame buffer structure that represents a single frame buffer
* @area: Structure providing the data to copy the framebuffer contents
* from one region to another.
*
- * This drawing operation copied a rectangular area from one area of the
+ * This drawing operation copies a rectangular area from one area of the
* screen to another area.
*/
void xxxfb_copyarea(struct fb_info *p, const struct fb_copyarea *area)
@@ -645,9 +645,9 @@ static const struct fb_ops xxxfb_ops = {
.fb_setcolreg = xxxfb_setcolreg,
.fb_blank = xxxfb_blank,
.fb_pan_display = xxxfb_pan_display,
- .fb_fillrect = xxxfb_fillrect, /* Needed !!! */
- .fb_copyarea = xxxfb_copyarea, /* Obsolete */
- .fb_imageblit = xxxfb_imageblit, /* Needed !!! */
+ .fb_fillrect = xxxfb_fillrect, /* Needed !!! */
+ .fb_copyarea = xxxfb_copyarea, /* Needed !!! */
+ .fb_imageblit = xxxfb_imageblit, /* Needed !!! */
.fb_cursor = xxxfb_cursor, /* Optional !!! */
.fb_sync = xxxfb_sync,
.fb_ioctl = xxxfb_ioctl,
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 3da95842b207..02f362c661c8 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -262,7 +262,7 @@ struct fb_ops {
/* Draws a rectangle */
void (*fb_fillrect) (struct fb_info *info, const struct fb_fillrect *rect);
- /* Copy data from area to another. Obsolete. */
+ /* Copy data from area to another */
void (*fb_copyarea) (struct fb_info *info, const struct fb_copyarea *region);
/* Draws a image to the display */
void (*fb_imageblit) (struct fb_info *info, const struct fb_image *image);
--
2.34.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 2/3] Revert "fbcon: Disable accelerated scrolling"
2022-02-01 18:59 [PATCH v3 0/3] Fix regression introduced by disabling accelerated scrolling in fbcon Helge Deller
2022-02-01 18:59 ` [PATCH v3 1/3] Revert "fbdev: Garbage collect fbdev scrolling acceleration, part 1 (from TODO list)" Helge Deller
@ 2022-02-01 18:59 ` Helge Deller
2022-02-01 18:59 ` [PATCH v3 3/3] fbcon: Add option to enable legacy hardware acceleration Helge Deller
2 siblings, 0 replies; 12+ messages in thread
From: Helge Deller @ 2022-02-01 18:59 UTC (permalink / raw)
To: DRI Development, linux-fbdev, Daniel Vetter
Cc: James Bottomley, John David Anglin
This reverts commit 39aead8373b3c20bb5965c024dfb51a94e526151.
Revert the first (of 2) commits which disabled scrolling acceleration in
fbcon/fbdev. It introduced a regression for fbdev-supported graphic cards
because of the performance penalty by doing screen scrolling by software
instead of using the existing graphic card 2D hardware acceleration.
Console scrolling acceleration was disabled by dropping code which
checked at runtime the driver hardware capabilities for the
BINFO_HWACCEL_COPYAREA or FBINFO_HWACCEL_FILLRECT flags and if set, it
enabled scrollmode SCROLL_MOVE which uses hardware acceleration to move
screen contents. After dropping those checks scrollmode was hard-wired
to SCROLL_REDRAW instead, which forces all graphic cards to redraw every
character at the new screen position when scrolling.
This change effectively disabled all hardware-based scrolling acceleration for
ALL drivers, because now all kind of 2D hardware acceleration (bitblt,
fillrect) in the drivers isn't used any longer.
The original commit message mentions that only 3 DRM drivers (nouveau, omapdrm
and gma500) used hardware acceleration in the past and thus code for checking
and using scrolling acceleration is obsolete.
This statement is NOT TRUE, because beside the DRM drivers there are around 35
other fbdev drivers which depend on fbdev/fbcon and still provide hardware
acceleration for fbdev/fbcon.
The original commit message also states that syzbot found lots of bugs in fbcon
and thus it's "often the solution to just delete code and remove features".
This is true, and the bugs - which actually affected all users of fbcon,
including DRM - were fixed, or code was dropped like e.g. the support for
software scrollback in vgacon (commit 973c096f6a85).
So to further analyze which bugs were found by syzbot, I've looked through all
patches in drivers/video which were tagged with syzbot or syzkaller back to
year 2005. The vast majority fixed the reported issues on a higher level, e.g.
when screen is to be resized, or when font size is to be changed. The few ones
which touched driver code fixed a real driver bug, e.g. by adding a check.
But NONE of those patches touched code of either the SCROLL_MOVE or the
SCROLL_REDRAW case.
That means, there was no real reason why SCROLL_MOVE had to be ripped-out and
just SCROLL_REDRAW had to be used instead. The only reason I can imagine so far
was that SCROLL_MOVE wasn't used by DRM and as such it was assumed that it
could go away. That argument completely missed the fact that SCROLL_MOVE is
still heavily used by fbdev (non-DRM) drivers.
Some people mention that using memcpy() instead of the hardware acceleration is
pretty much the same speed. But that's not true, at least not for older graphic
cards and machines where we see speed decreases by factor 10 and more and thus
this change leads to console responsiveness way worse than before.
That's why the original commit is to be reverted. By reverting we
reintroduce hardware-based scrolling acceleration and fix the
performance regression for fbdev drivers.
There isn't any impact on DRM when reverting those patches.
Signed-off-by: Helge Deller <deller@gmx.de>
Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
Acked-by: Sven Schnelle <svens@stackframe.org>
Cc: stable@vger.kernel.org # v5.10+
Signed-off-by: Helge Deller <deller@gmx.de>
---
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.34.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 3/3] fbcon: Add option to enable legacy hardware acceleration
2022-02-01 18:59 [PATCH v3 0/3] Fix regression introduced by disabling accelerated scrolling in fbcon Helge Deller
2022-02-01 18:59 ` [PATCH v3 1/3] Revert "fbdev: Garbage collect fbdev scrolling acceleration, part 1 (from TODO list)" Helge Deller
2022-02-01 18:59 ` [PATCH v3 2/3] Revert "fbcon: Disable accelerated scrolling" Helge Deller
@ 2022-02-01 18:59 ` Helge Deller
2022-02-01 20:11 ` Daniel Vetter
2 siblings, 1 reply; 12+ messages in thread
From: Helge Deller @ 2022-02-01 18:59 UTC (permalink / raw)
To: DRI Development, linux-fbdev, Daniel Vetter
Cc: James Bottomley, John David Anglin
Add a config option CONFIG_FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION to
enable bitblt and fillrect hardware acceleration in the framebuffer
console. If disabled, such acceleration will not be used, even if it is
supported by the graphics hardware driver.
If you plan to use DRM as your main graphics output system, you should
disable this option since it will prevent compiling in code which isn't
used later on when DRM takes over.
For all other configurations, e.g. if none of your graphic cards support
DRM (yet), DRM isn't available for your architecture, or you can't be
sure that the graphic card in the target system will support DRM, you
most likely want to enable this option.
In the non-accelerated case (e.g. when DRM is used), the inlined
fb_scrollmode() function is hardcoded to return SCROLL_REDRAW and as such the
compiler is able to optimize much unneccesary code away.
In this v3 patch version I additionally changed the GETVYRES() and GETVXRES()
macros to take a pointer to the fbcon_display struct. This fixes the build when
console rotation is enabled and helps the compiler again to optimize out code.
Signed-off-by: Helge Deller <deller@gmx.de>
Cc: stable@vger.kernel.org # v5.10+
Signed-off-by: Helge Deller <deller@gmx.de>
---
drivers/video/console/Kconfig | 11 +++++++
drivers/video/fbdev/core/fbcon.c | 39 ++++++++++++++++++-------
drivers/video/fbdev/core/fbcon.h | 15 +++++++++-
drivers/video/fbdev/core/fbcon_ccw.c | 10 +++----
drivers/video/fbdev/core/fbcon_cw.c | 10 +++----
drivers/video/fbdev/core/fbcon_rotate.h | 4 +--
drivers/video/fbdev/core/fbcon_ud.c | 20 ++++++-------
7 files changed, 75 insertions(+), 34 deletions(-)
diff --git a/drivers/video/console/Kconfig b/drivers/video/console/Kconfig
index 840d9813b0bc..6029fd41d7d0 100644
--- a/drivers/video/console/Kconfig
+++ b/drivers/video/console/Kconfig
@@ -78,6 +78,17 @@ config FRAMEBUFFER_CONSOLE
help
Low-level framebuffer-based console driver.
+config FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION
+ bool "Framebuffer Console hardware acceleration support"
+ depends on FRAMEBUFFER_CONSOLE
+ default n if DRM
+ default y
+ help
+ If you use a system on which DRM is fully supported you usually want to say N,
+ otherwise you probably want to enable this option.
+ If enabled the framebuffer console will utilize the hardware acceleration
+ of your graphics card by using hardware bitblt and fillrect features.
+
config FRAMEBUFFER_CONSOLE_DETECT_PRIMARY
bool "Map the console to the primary display device"
depends on FRAMEBUFFER_CONSOLE
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index b813985f1403..f7b7d35953e8 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -1136,11 +1136,13 @@ static void fbcon_init(struct vc_data *vc, int init)
ops->graphics = 0;
+#ifdef CONFIG_FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION
if ((cap & FBINFO_HWACCEL_COPYAREA) &&
!(cap & FBINFO_HWACCEL_DISABLED))
p->scrollmode = SCROLL_MOVE;
else /* default to something safe */
p->scrollmode = SCROLL_REDRAW;
+#endif
/*
* ++guenther: console.c:vc_allocate() relies on initializing
@@ -1705,7 +1707,7 @@ static bool fbcon_scroll(struct vc_data *vc, unsigned int t, unsigned int b,
count = vc->vc_rows;
if (logo_shown >= 0)
goto redraw_up;
- switch (p->scrollmode) {
+ switch (fb_scrollmode(p)) {
case SCROLL_MOVE:
fbcon_redraw_blit(vc, info, p, t, b - t - count,
count);
@@ -1795,7 +1797,7 @@ static bool fbcon_scroll(struct vc_data *vc, unsigned int t, unsigned int b,
count = vc->vc_rows;
if (logo_shown >= 0)
goto redraw_down;
- switch (p->scrollmode) {
+ switch (fb_scrollmode(p)) {
case SCROLL_MOVE:
fbcon_redraw_blit(vc, info, p, b - 1, b - t - count,
-count);
@@ -1946,12 +1948,12 @@ static void fbcon_bmove_rec(struct vc_data *vc, struct fbcon_display *p, int sy,
height, width);
}
-static void updatescrollmode(struct fbcon_display *p,
+static void updatescrollmode_accel(struct fbcon_display *p,
struct fb_info *info,
struct vc_data *vc)
{
+#ifdef CONFIG_FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION
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,
@@ -1972,12 +1974,6 @@ static void updatescrollmode(struct fbcon_display *p,
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 ?
@@ -1991,6 +1987,27 @@ static void updatescrollmode(struct fbcon_display *p,
else
p->scrollmode = SCROLL_REDRAW;
}
+#endif
+}
+
+static void updatescrollmode(struct fbcon_display *p,
+ struct fb_info *info,
+ struct vc_data *vc)
+{
+ struct fbcon_ops *ops = info->fbcon_par;
+ int fh = vc->vc_font.height;
+ 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);
+
+ 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--;
+
+ /* update scrollmode in case hardware acceleration is used */
+ updatescrollmode_accel(p, info, vc);
}
#define PITCH(w) (((w) + 7) >> 3)
@@ -2148,7 +2165,7 @@ static int fbcon_switch(struct vc_data *vc)
updatescrollmode(p, info, vc);
- switch (p->scrollmode) {
+ switch (fb_scrollmode(p)) {
case SCROLL_WRAP_MOVE:
scrollback_phys_max = p->vrows - vc->vc_rows;
break;
diff --git a/drivers/video/fbdev/core/fbcon.h b/drivers/video/fbdev/core/fbcon.h
index 9315b360c898..0f16cbc99e6a 100644
--- a/drivers/video/fbdev/core/fbcon.h
+++ b/drivers/video/fbdev/core/fbcon.h
@@ -29,7 +29,9 @@ struct fbcon_display {
/* Filled in by the low-level console driver */
const u_char *fontdata;
int userfont; /* != 0 if fontdata kmalloc()ed */
- u_short scrollmode; /* Scroll Method */
+#ifdef CONFIG_FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION
+ u_short scrollmode; /* Scroll Method, use fb_scrollmode() */
+#endif
u_short inverse; /* != 0 text black on white as default */
short yscroll; /* Hardware scrolling */
int vrows; /* number of virtual rows */
@@ -208,6 +210,17 @@ static inline int attr_col_ec(int shift, struct vc_data *vc,
#define SCROLL_REDRAW 0x004
#define SCROLL_PAN_REDRAW 0x005
+static inline u_short fb_scrollmode(struct fbcon_display *fb)
+{
+#ifdef CONFIG_FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION
+ return fb->scrollmode;
+#else
+ /* hardcoded to SCROLL_REDRAW if acceleration was disabled. */
+ return SCROLL_REDRAW;
+#endif
+}
+
+
#ifdef CONFIG_FB_TILEBLITTING
extern void fbcon_set_tileops(struct vc_data *vc, struct fb_info *info);
#endif
diff --git a/drivers/video/fbdev/core/fbcon_ccw.c b/drivers/video/fbdev/core/fbcon_ccw.c
index 9cd2c4b05c32..2789ace79634 100644
--- a/drivers/video/fbdev/core/fbcon_ccw.c
+++ b/drivers/video/fbdev/core/fbcon_ccw.c
@@ -65,7 +65,7 @@ static void ccw_bmove(struct vc_data *vc, struct fb_info *info, int sy,
{
struct fbcon_ops *ops = info->fbcon_par;
struct fb_copyarea area;
- u32 vyres = GETVYRES(ops->p->scrollmode, info);
+ u32 vyres = GETVYRES(ops->p, info);
area.sx = sy * vc->vc_font.height;
area.sy = vyres - ((sx + width) * vc->vc_font.width);
@@ -83,7 +83,7 @@ static void ccw_clear(struct vc_data *vc, struct fb_info *info, int sy,
struct fbcon_ops *ops = info->fbcon_par;
struct fb_fillrect region;
int bgshift = (vc->vc_hi_font_mask) ? 13 : 12;
- u32 vyres = GETVYRES(ops->p->scrollmode, info);
+ u32 vyres = GETVYRES(ops->p, info);
region.color = attr_bgcol_ec(bgshift,vc,info);
region.dx = sy * vc->vc_font.height;
@@ -140,7 +140,7 @@ static void ccw_putcs(struct vc_data *vc, struct fb_info *info,
u32 cnt, pitch, size;
u32 attribute = get_attribute(info, scr_readw(s));
u8 *dst, *buf = NULL;
- u32 vyres = GETVYRES(ops->p->scrollmode, info);
+ u32 vyres = GETVYRES(ops->p, info);
if (!ops->fontbuffer)
return;
@@ -229,7 +229,7 @@ static void ccw_cursor(struct vc_data *vc, struct fb_info *info, int mode,
int attribute, use_sw = vc->vc_cursor_type & CUR_SW;
int err = 1, dx, dy;
char *src;
- u32 vyres = GETVYRES(ops->p->scrollmode, info);
+ u32 vyres = GETVYRES(ops->p, info);
if (!ops->fontbuffer)
return;
@@ -387,7 +387,7 @@ static int ccw_update_start(struct fb_info *info)
{
struct fbcon_ops *ops = info->fbcon_par;
u32 yoffset;
- u32 vyres = GETVYRES(ops->p->scrollmode, info);
+ u32 vyres = GETVYRES(ops->p, info);
int err;
yoffset = (vyres - info->var.yres) - ops->var.xoffset;
diff --git a/drivers/video/fbdev/core/fbcon_cw.c b/drivers/video/fbdev/core/fbcon_cw.c
index 88d89fad3f05..86a254c1b2b7 100644
--- a/drivers/video/fbdev/core/fbcon_cw.c
+++ b/drivers/video/fbdev/core/fbcon_cw.c
@@ -50,7 +50,7 @@ static void cw_bmove(struct vc_data *vc, struct fb_info *info, int sy,
{
struct fbcon_ops *ops = info->fbcon_par;
struct fb_copyarea area;
- u32 vxres = GETVXRES(ops->p->scrollmode, info);
+ u32 vxres = GETVXRES(ops->p, info);
area.sx = vxres - ((sy + height) * vc->vc_font.height);
area.sy = sx * vc->vc_font.width;
@@ -68,7 +68,7 @@ static void cw_clear(struct vc_data *vc, struct fb_info *info, int sy,
struct fbcon_ops *ops = info->fbcon_par;
struct fb_fillrect region;
int bgshift = (vc->vc_hi_font_mask) ? 13 : 12;
- u32 vxres = GETVXRES(ops->p->scrollmode, info);
+ u32 vxres = GETVXRES(ops->p, info);
region.color = attr_bgcol_ec(bgshift,vc,info);
region.dx = vxres - ((sy + height) * vc->vc_font.height);
@@ -125,7 +125,7 @@ static void cw_putcs(struct vc_data *vc, struct fb_info *info,
u32 cnt, pitch, size;
u32 attribute = get_attribute(info, scr_readw(s));
u8 *dst, *buf = NULL;
- u32 vxres = GETVXRES(ops->p->scrollmode, info);
+ u32 vxres = GETVXRES(ops->p, info);
if (!ops->fontbuffer)
return;
@@ -212,7 +212,7 @@ static void cw_cursor(struct vc_data *vc, struct fb_info *info, int mode,
int attribute, use_sw = vc->vc_cursor_type & CUR_SW;
int err = 1, dx, dy;
char *src;
- u32 vxres = GETVXRES(ops->p->scrollmode, info);
+ u32 vxres = GETVXRES(ops->p, info);
if (!ops->fontbuffer)
return;
@@ -369,7 +369,7 @@ static void cw_cursor(struct vc_data *vc, struct fb_info *info, int mode,
static int cw_update_start(struct fb_info *info)
{
struct fbcon_ops *ops = info->fbcon_par;
- u32 vxres = GETVXRES(ops->p->scrollmode, info);
+ u32 vxres = GETVXRES(ops->p, info);
u32 xoffset;
int err;
diff --git a/drivers/video/fbdev/core/fbcon_rotate.h b/drivers/video/fbdev/core/fbcon_rotate.h
index e233444cda66..01cbe303b8a2 100644
--- a/drivers/video/fbdev/core/fbcon_rotate.h
+++ b/drivers/video/fbdev/core/fbcon_rotate.h
@@ -12,11 +12,11 @@
#define _FBCON_ROTATE_H
#define GETVYRES(s,i) ({ \
- (s == SCROLL_REDRAW || s == SCROLL_MOVE) ? \
+ (fb_scrollmode(s) == SCROLL_REDRAW || fb_scrollmode(s) == SCROLL_MOVE) ? \
(i)->var.yres : (i)->var.yres_virtual; })
#define GETVXRES(s,i) ({ \
- (s == SCROLL_REDRAW || s == SCROLL_MOVE || !(i)->fix.xpanstep) ? \
+ (fb_scrollmode(s) == SCROLL_REDRAW || fb_scrollmode(s) == SCROLL_MOVE || !(i)->fix.xpanstep) ? \
(i)->var.xres : (i)->var.xres_virtual; })
diff --git a/drivers/video/fbdev/core/fbcon_ud.c b/drivers/video/fbdev/core/fbcon_ud.c
index 8d5e66b1bdfb..23bc045769d0 100644
--- a/drivers/video/fbdev/core/fbcon_ud.c
+++ b/drivers/video/fbdev/core/fbcon_ud.c
@@ -50,8 +50,8 @@ static void ud_bmove(struct vc_data *vc, struct fb_info *info, int sy,
{
struct fbcon_ops *ops = info->fbcon_par;
struct fb_copyarea area;
- u32 vyres = GETVYRES(ops->p->scrollmode, info);
- u32 vxres = GETVXRES(ops->p->scrollmode, info);
+ u32 vyres = GETVYRES(ops->p, info);
+ u32 vxres = GETVXRES(ops->p, info);
area.sy = vyres - ((sy + height) * vc->vc_font.height);
area.sx = vxres - ((sx + width) * vc->vc_font.width);
@@ -69,8 +69,8 @@ static void ud_clear(struct vc_data *vc, struct fb_info *info, int sy,
struct fbcon_ops *ops = info->fbcon_par;
struct fb_fillrect region;
int bgshift = (vc->vc_hi_font_mask) ? 13 : 12;
- u32 vyres = GETVYRES(ops->p->scrollmode, info);
- u32 vxres = GETVXRES(ops->p->scrollmode, info);
+ u32 vyres = GETVYRES(ops->p, info);
+ u32 vxres = GETVXRES(ops->p, info);
region.color = attr_bgcol_ec(bgshift,vc,info);
region.dy = vyres - ((sy + height) * vc->vc_font.height);
@@ -162,8 +162,8 @@ static void ud_putcs(struct vc_data *vc, struct fb_info *info,
u32 mod = vc->vc_font.width % 8, cnt, pitch, size;
u32 attribute = get_attribute(info, scr_readw(s));
u8 *dst, *buf = NULL;
- u32 vyres = GETVYRES(ops->p->scrollmode, info);
- u32 vxres = GETVXRES(ops->p->scrollmode, info);
+ u32 vyres = GETVYRES(ops->p, info);
+ u32 vxres = GETVXRES(ops->p, info);
if (!ops->fontbuffer)
return;
@@ -259,8 +259,8 @@ static void ud_cursor(struct vc_data *vc, struct fb_info *info, int mode,
int attribute, use_sw = vc->vc_cursor_type & CUR_SW;
int err = 1, dx, dy;
char *src;
- u32 vyres = GETVYRES(ops->p->scrollmode, info);
- u32 vxres = GETVXRES(ops->p->scrollmode, info);
+ u32 vyres = GETVYRES(ops->p, info);
+ u32 vxres = GETVXRES(ops->p, info);
if (!ops->fontbuffer)
return;
@@ -410,8 +410,8 @@ static int ud_update_start(struct fb_info *info)
{
struct fbcon_ops *ops = info->fbcon_par;
int xoffset, yoffset;
- u32 vyres = GETVYRES(ops->p->scrollmode, info);
- u32 vxres = GETVXRES(ops->p->scrollmode, info);
+ u32 vyres = GETVYRES(ops->p, info);
+ u32 vxres = GETVXRES(ops->p, info);
int err;
xoffset = vxres - info->var.xres - ops->var.xoffset;
--
2.34.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3 3/3] fbcon: Add option to enable legacy hardware acceleration
2022-02-01 18:59 ` [PATCH v3 3/3] fbcon: Add option to enable legacy hardware acceleration Helge Deller
@ 2022-02-01 20:11 ` Daniel Vetter
0 siblings, 0 replies; 12+ messages in thread
From: Daniel Vetter @ 2022-02-01 20:11 UTC (permalink / raw)
To: Helge Deller
Cc: DRI Development, linux-fbdev, James Bottomley, John David Anglin
On Tue, Feb 1, 2022 at 7:59 PM Helge Deller <deller@gmx.de> wrote:
>
> Add a config option CONFIG_FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION to
> enable bitblt and fillrect hardware acceleration in the framebuffer
> console. If disabled, such acceleration will not be used, even if it is
> supported by the graphics hardware driver.
>
> If you plan to use DRM as your main graphics output system, you should
> disable this option since it will prevent compiling in code which isn't
> used later on when DRM takes over.
>
> For all other configurations, e.g. if none of your graphic cards support
> DRM (yet), DRM isn't available for your architecture, or you can't be
> sure that the graphic card in the target system will support DRM, you
> most likely want to enable this option.
>
> In the non-accelerated case (e.g. when DRM is used), the inlined
> fb_scrollmode() function is hardcoded to return SCROLL_REDRAW and as such the
> compiler is able to optimize much unneccesary code away.
>
> In this v3 patch version I additionally changed the GETVYRES() and GETVXRES()
> macros to take a pointer to the fbcon_display struct. This fixes the build when
> console rotation is enabled and helps the compiler again to optimize out code.
>
> Signed-off-by: Helge Deller <deller@gmx.de>
> Cc: stable@vger.kernel.org # v5.10+
> Signed-off-by: Helge Deller <deller@gmx.de>
> ---
> drivers/video/console/Kconfig | 11 +++++++
> drivers/video/fbdev/core/fbcon.c | 39 ++++++++++++++++++-------
> drivers/video/fbdev/core/fbcon.h | 15 +++++++++-
> drivers/video/fbdev/core/fbcon_ccw.c | 10 +++----
> drivers/video/fbdev/core/fbcon_cw.c | 10 +++----
> drivers/video/fbdev/core/fbcon_rotate.h | 4 +--
> drivers/video/fbdev/core/fbcon_ud.c | 20 ++++++-------
> 7 files changed, 75 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/video/console/Kconfig b/drivers/video/console/Kconfig
> index 840d9813b0bc..6029fd41d7d0 100644
> --- a/drivers/video/console/Kconfig
> +++ b/drivers/video/console/Kconfig
> @@ -78,6 +78,17 @@ config FRAMEBUFFER_CONSOLE
> help
> Low-level framebuffer-based console driver.
>
> +config FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION
> + bool "Framebuffer Console hardware acceleration support"
> + depends on FRAMEBUFFER_CONSOLE
> + default n if DRM
> + default y
> + help
> + If you use a system on which DRM is fully supported you usually want to say N,
> + otherwise you probably want to enable this option.
> + If enabled the framebuffer console will utilize the hardware acceleration
> + of your graphics card by using hardware bitblt and fillrect features.
This really doesn't have much to do with DRM but more about running
questionable code. That's why I went with the generalized stern
warning and default n, and explained when it's ok to do this (single
user and you care more about fbcon performance than potential issues
because you only run trusted stuff with access to your vt and fbdev
/dev node). Also you didn't keep any todo entry for removing DRM accel
code, and iirc some nouveau folks also complained that they at least
once had some kind of accel, so that's another reason to not tie this
to DRM.
Anyway aside from this looks reasonable, can you pls respin with a
more cautious Kconfig text?
Thanks, Daniel
> +
> config FRAMEBUFFER_CONSOLE_DETECT_PRIMARY
> bool "Map the console to the primary display device"
> depends on FRAMEBUFFER_CONSOLE
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index b813985f1403..f7b7d35953e8 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -1136,11 +1136,13 @@ static void fbcon_init(struct vc_data *vc, int init)
>
> ops->graphics = 0;
>
> +#ifdef CONFIG_FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION
> if ((cap & FBINFO_HWACCEL_COPYAREA) &&
> !(cap & FBINFO_HWACCEL_DISABLED))
> p->scrollmode = SCROLL_MOVE;
> else /* default to something safe */
> p->scrollmode = SCROLL_REDRAW;
> +#endif
>
> /*
> * ++guenther: console.c:vc_allocate() relies on initializing
> @@ -1705,7 +1707,7 @@ static bool fbcon_scroll(struct vc_data *vc, unsigned int t, unsigned int b,
> count = vc->vc_rows;
> if (logo_shown >= 0)
> goto redraw_up;
> - switch (p->scrollmode) {
> + switch (fb_scrollmode(p)) {
> case SCROLL_MOVE:
> fbcon_redraw_blit(vc, info, p, t, b - t - count,
> count);
> @@ -1795,7 +1797,7 @@ static bool fbcon_scroll(struct vc_data *vc, unsigned int t, unsigned int b,
> count = vc->vc_rows;
> if (logo_shown >= 0)
> goto redraw_down;
> - switch (p->scrollmode) {
> + switch (fb_scrollmode(p)) {
> case SCROLL_MOVE:
> fbcon_redraw_blit(vc, info, p, b - 1, b - t - count,
> -count);
> @@ -1946,12 +1948,12 @@ static void fbcon_bmove_rec(struct vc_data *vc, struct fbcon_display *p, int sy,
> height, width);
> }
>
> -static void updatescrollmode(struct fbcon_display *p,
> +static void updatescrollmode_accel(struct fbcon_display *p,
> struct fb_info *info,
> struct vc_data *vc)
> {
> +#ifdef CONFIG_FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION
> 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,
> @@ -1972,12 +1974,6 @@ static void updatescrollmode(struct fbcon_display *p,
> 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 ?
> @@ -1991,6 +1987,27 @@ static void updatescrollmode(struct fbcon_display *p,
> else
> p->scrollmode = SCROLL_REDRAW;
> }
> +#endif
> +}
> +
> +static void updatescrollmode(struct fbcon_display *p,
> + struct fb_info *info,
> + struct vc_data *vc)
> +{
> + struct fbcon_ops *ops = info->fbcon_par;
> + int fh = vc->vc_font.height;
> + 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);
> +
> + 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--;
> +
> + /* update scrollmode in case hardware acceleration is used */
> + updatescrollmode_accel(p, info, vc);
> }
>
> #define PITCH(w) (((w) + 7) >> 3)
> @@ -2148,7 +2165,7 @@ static int fbcon_switch(struct vc_data *vc)
>
> updatescrollmode(p, info, vc);
>
> - switch (p->scrollmode) {
> + switch (fb_scrollmode(p)) {
> case SCROLL_WRAP_MOVE:
> scrollback_phys_max = p->vrows - vc->vc_rows;
> break;
> diff --git a/drivers/video/fbdev/core/fbcon.h b/drivers/video/fbdev/core/fbcon.h
> index 9315b360c898..0f16cbc99e6a 100644
> --- a/drivers/video/fbdev/core/fbcon.h
> +++ b/drivers/video/fbdev/core/fbcon.h
> @@ -29,7 +29,9 @@ struct fbcon_display {
> /* Filled in by the low-level console driver */
> const u_char *fontdata;
> int userfont; /* != 0 if fontdata kmalloc()ed */
> - u_short scrollmode; /* Scroll Method */
> +#ifdef CONFIG_FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION
> + u_short scrollmode; /* Scroll Method, use fb_scrollmode() */
> +#endif
> u_short inverse; /* != 0 text black on white as default */
> short yscroll; /* Hardware scrolling */
> int vrows; /* number of virtual rows */
> @@ -208,6 +210,17 @@ static inline int attr_col_ec(int shift, struct vc_data *vc,
> #define SCROLL_REDRAW 0x004
> #define SCROLL_PAN_REDRAW 0x005
>
> +static inline u_short fb_scrollmode(struct fbcon_display *fb)
> +{
> +#ifdef CONFIG_FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION
> + return fb->scrollmode;
> +#else
> + /* hardcoded to SCROLL_REDRAW if acceleration was disabled. */
> + return SCROLL_REDRAW;
> +#endif
> +}
> +
> +
> #ifdef CONFIG_FB_TILEBLITTING
> extern void fbcon_set_tileops(struct vc_data *vc, struct fb_info *info);
> #endif
> diff --git a/drivers/video/fbdev/core/fbcon_ccw.c b/drivers/video/fbdev/core/fbcon_ccw.c
> index 9cd2c4b05c32..2789ace79634 100644
> --- a/drivers/video/fbdev/core/fbcon_ccw.c
> +++ b/drivers/video/fbdev/core/fbcon_ccw.c
> @@ -65,7 +65,7 @@ static void ccw_bmove(struct vc_data *vc, struct fb_info *info, int sy,
> {
> struct fbcon_ops *ops = info->fbcon_par;
> struct fb_copyarea area;
> - u32 vyres = GETVYRES(ops->p->scrollmode, info);
> + u32 vyres = GETVYRES(ops->p, info);
>
> area.sx = sy * vc->vc_font.height;
> area.sy = vyres - ((sx + width) * vc->vc_font.width);
> @@ -83,7 +83,7 @@ static void ccw_clear(struct vc_data *vc, struct fb_info *info, int sy,
> struct fbcon_ops *ops = info->fbcon_par;
> struct fb_fillrect region;
> int bgshift = (vc->vc_hi_font_mask) ? 13 : 12;
> - u32 vyres = GETVYRES(ops->p->scrollmode, info);
> + u32 vyres = GETVYRES(ops->p, info);
>
> region.color = attr_bgcol_ec(bgshift,vc,info);
> region.dx = sy * vc->vc_font.height;
> @@ -140,7 +140,7 @@ static void ccw_putcs(struct vc_data *vc, struct fb_info *info,
> u32 cnt, pitch, size;
> u32 attribute = get_attribute(info, scr_readw(s));
> u8 *dst, *buf = NULL;
> - u32 vyres = GETVYRES(ops->p->scrollmode, info);
> + u32 vyres = GETVYRES(ops->p, info);
>
> if (!ops->fontbuffer)
> return;
> @@ -229,7 +229,7 @@ static void ccw_cursor(struct vc_data *vc, struct fb_info *info, int mode,
> int attribute, use_sw = vc->vc_cursor_type & CUR_SW;
> int err = 1, dx, dy;
> char *src;
> - u32 vyres = GETVYRES(ops->p->scrollmode, info);
> + u32 vyres = GETVYRES(ops->p, info);
>
> if (!ops->fontbuffer)
> return;
> @@ -387,7 +387,7 @@ static int ccw_update_start(struct fb_info *info)
> {
> struct fbcon_ops *ops = info->fbcon_par;
> u32 yoffset;
> - u32 vyres = GETVYRES(ops->p->scrollmode, info);
> + u32 vyres = GETVYRES(ops->p, info);
> int err;
>
> yoffset = (vyres - info->var.yres) - ops->var.xoffset;
> diff --git a/drivers/video/fbdev/core/fbcon_cw.c b/drivers/video/fbdev/core/fbcon_cw.c
> index 88d89fad3f05..86a254c1b2b7 100644
> --- a/drivers/video/fbdev/core/fbcon_cw.c
> +++ b/drivers/video/fbdev/core/fbcon_cw.c
> @@ -50,7 +50,7 @@ static void cw_bmove(struct vc_data *vc, struct fb_info *info, int sy,
> {
> struct fbcon_ops *ops = info->fbcon_par;
> struct fb_copyarea area;
> - u32 vxres = GETVXRES(ops->p->scrollmode, info);
> + u32 vxres = GETVXRES(ops->p, info);
>
> area.sx = vxres - ((sy + height) * vc->vc_font.height);
> area.sy = sx * vc->vc_font.width;
> @@ -68,7 +68,7 @@ static void cw_clear(struct vc_data *vc, struct fb_info *info, int sy,
> struct fbcon_ops *ops = info->fbcon_par;
> struct fb_fillrect region;
> int bgshift = (vc->vc_hi_font_mask) ? 13 : 12;
> - u32 vxres = GETVXRES(ops->p->scrollmode, info);
> + u32 vxres = GETVXRES(ops->p, info);
>
> region.color = attr_bgcol_ec(bgshift,vc,info);
> region.dx = vxres - ((sy + height) * vc->vc_font.height);
> @@ -125,7 +125,7 @@ static void cw_putcs(struct vc_data *vc, struct fb_info *info,
> u32 cnt, pitch, size;
> u32 attribute = get_attribute(info, scr_readw(s));
> u8 *dst, *buf = NULL;
> - u32 vxres = GETVXRES(ops->p->scrollmode, info);
> + u32 vxres = GETVXRES(ops->p, info);
>
> if (!ops->fontbuffer)
> return;
> @@ -212,7 +212,7 @@ static void cw_cursor(struct vc_data *vc, struct fb_info *info, int mode,
> int attribute, use_sw = vc->vc_cursor_type & CUR_SW;
> int err = 1, dx, dy;
> char *src;
> - u32 vxres = GETVXRES(ops->p->scrollmode, info);
> + u32 vxres = GETVXRES(ops->p, info);
>
> if (!ops->fontbuffer)
> return;
> @@ -369,7 +369,7 @@ static void cw_cursor(struct vc_data *vc, struct fb_info *info, int mode,
> static int cw_update_start(struct fb_info *info)
> {
> struct fbcon_ops *ops = info->fbcon_par;
> - u32 vxres = GETVXRES(ops->p->scrollmode, info);
> + u32 vxres = GETVXRES(ops->p, info);
> u32 xoffset;
> int err;
>
> diff --git a/drivers/video/fbdev/core/fbcon_rotate.h b/drivers/video/fbdev/core/fbcon_rotate.h
> index e233444cda66..01cbe303b8a2 100644
> --- a/drivers/video/fbdev/core/fbcon_rotate.h
> +++ b/drivers/video/fbdev/core/fbcon_rotate.h
> @@ -12,11 +12,11 @@
> #define _FBCON_ROTATE_H
>
> #define GETVYRES(s,i) ({ \
> - (s == SCROLL_REDRAW || s == SCROLL_MOVE) ? \
> + (fb_scrollmode(s) == SCROLL_REDRAW || fb_scrollmode(s) == SCROLL_MOVE) ? \
> (i)->var.yres : (i)->var.yres_virtual; })
>
> #define GETVXRES(s,i) ({ \
> - (s == SCROLL_REDRAW || s == SCROLL_MOVE || !(i)->fix.xpanstep) ? \
> + (fb_scrollmode(s) == SCROLL_REDRAW || fb_scrollmode(s) == SCROLL_MOVE || !(i)->fix.xpanstep) ? \
> (i)->var.xres : (i)->var.xres_virtual; })
>
>
> diff --git a/drivers/video/fbdev/core/fbcon_ud.c b/drivers/video/fbdev/core/fbcon_ud.c
> index 8d5e66b1bdfb..23bc045769d0 100644
> --- a/drivers/video/fbdev/core/fbcon_ud.c
> +++ b/drivers/video/fbdev/core/fbcon_ud.c
> @@ -50,8 +50,8 @@ static void ud_bmove(struct vc_data *vc, struct fb_info *info, int sy,
> {
> struct fbcon_ops *ops = info->fbcon_par;
> struct fb_copyarea area;
> - u32 vyres = GETVYRES(ops->p->scrollmode, info);
> - u32 vxres = GETVXRES(ops->p->scrollmode, info);
> + u32 vyres = GETVYRES(ops->p, info);
> + u32 vxres = GETVXRES(ops->p, info);
>
> area.sy = vyres - ((sy + height) * vc->vc_font.height);
> area.sx = vxres - ((sx + width) * vc->vc_font.width);
> @@ -69,8 +69,8 @@ static void ud_clear(struct vc_data *vc, struct fb_info *info, int sy,
> struct fbcon_ops *ops = info->fbcon_par;
> struct fb_fillrect region;
> int bgshift = (vc->vc_hi_font_mask) ? 13 : 12;
> - u32 vyres = GETVYRES(ops->p->scrollmode, info);
> - u32 vxres = GETVXRES(ops->p->scrollmode, info);
> + u32 vyres = GETVYRES(ops->p, info);
> + u32 vxres = GETVXRES(ops->p, info);
>
> region.color = attr_bgcol_ec(bgshift,vc,info);
> region.dy = vyres - ((sy + height) * vc->vc_font.height);
> @@ -162,8 +162,8 @@ static void ud_putcs(struct vc_data *vc, struct fb_info *info,
> u32 mod = vc->vc_font.width % 8, cnt, pitch, size;
> u32 attribute = get_attribute(info, scr_readw(s));
> u8 *dst, *buf = NULL;
> - u32 vyres = GETVYRES(ops->p->scrollmode, info);
> - u32 vxres = GETVXRES(ops->p->scrollmode, info);
> + u32 vyres = GETVYRES(ops->p, info);
> + u32 vxres = GETVXRES(ops->p, info);
>
> if (!ops->fontbuffer)
> return;
> @@ -259,8 +259,8 @@ static void ud_cursor(struct vc_data *vc, struct fb_info *info, int mode,
> int attribute, use_sw = vc->vc_cursor_type & CUR_SW;
> int err = 1, dx, dy;
> char *src;
> - u32 vyres = GETVYRES(ops->p->scrollmode, info);
> - u32 vxres = GETVXRES(ops->p->scrollmode, info);
> + u32 vyres = GETVYRES(ops->p, info);
> + u32 vxres = GETVXRES(ops->p, info);
>
> if (!ops->fontbuffer)
> return;
> @@ -410,8 +410,8 @@ static int ud_update_start(struct fb_info *info)
> {
> struct fbcon_ops *ops = info->fbcon_par;
> int xoffset, yoffset;
> - u32 vyres = GETVYRES(ops->p->scrollmode, info);
> - u32 vxres = GETVXRES(ops->p->scrollmode, info);
> + u32 vyres = GETVYRES(ops->p, info);
> + u32 vxres = GETVXRES(ops->p, info);
> int err;
>
> xoffset = vxres - info->var.xres - ops->var.xoffset;
> --
> 2.34.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 3/3] fbcon: Add option to enable legacy hardware acceleration
@ 2022-02-01 20:11 ` Daniel Vetter
0 siblings, 0 replies; 12+ messages in thread
From: Daniel Vetter @ 2022-02-01 20:11 UTC (permalink / raw)
To: Helge Deller
Cc: James Bottomley, linux-fbdev, John David Anglin, DRI Development
On Tue, Feb 1, 2022 at 7:59 PM Helge Deller <deller@gmx.de> wrote:
>
> Add a config option CONFIG_FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION to
> enable bitblt and fillrect hardware acceleration in the framebuffer
> console. If disabled, such acceleration will not be used, even if it is
> supported by the graphics hardware driver.
>
> If you plan to use DRM as your main graphics output system, you should
> disable this option since it will prevent compiling in code which isn't
> used later on when DRM takes over.
>
> For all other configurations, e.g. if none of your graphic cards support
> DRM (yet), DRM isn't available for your architecture, or you can't be
> sure that the graphic card in the target system will support DRM, you
> most likely want to enable this option.
>
> In the non-accelerated case (e.g. when DRM is used), the inlined
> fb_scrollmode() function is hardcoded to return SCROLL_REDRAW and as such the
> compiler is able to optimize much unneccesary code away.
>
> In this v3 patch version I additionally changed the GETVYRES() and GETVXRES()
> macros to take a pointer to the fbcon_display struct. This fixes the build when
> console rotation is enabled and helps the compiler again to optimize out code.
>
> Signed-off-by: Helge Deller <deller@gmx.de>
> Cc: stable@vger.kernel.org # v5.10+
> Signed-off-by: Helge Deller <deller@gmx.de>
> ---
> drivers/video/console/Kconfig | 11 +++++++
> drivers/video/fbdev/core/fbcon.c | 39 ++++++++++++++++++-------
> drivers/video/fbdev/core/fbcon.h | 15 +++++++++-
> drivers/video/fbdev/core/fbcon_ccw.c | 10 +++----
> drivers/video/fbdev/core/fbcon_cw.c | 10 +++----
> drivers/video/fbdev/core/fbcon_rotate.h | 4 +--
> drivers/video/fbdev/core/fbcon_ud.c | 20 ++++++-------
> 7 files changed, 75 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/video/console/Kconfig b/drivers/video/console/Kconfig
> index 840d9813b0bc..6029fd41d7d0 100644
> --- a/drivers/video/console/Kconfig
> +++ b/drivers/video/console/Kconfig
> @@ -78,6 +78,17 @@ config FRAMEBUFFER_CONSOLE
> help
> Low-level framebuffer-based console driver.
>
> +config FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION
> + bool "Framebuffer Console hardware acceleration support"
> + depends on FRAMEBUFFER_CONSOLE
> + default n if DRM
> + default y
> + help
> + If you use a system on which DRM is fully supported you usually want to say N,
> + otherwise you probably want to enable this option.
> + If enabled the framebuffer console will utilize the hardware acceleration
> + of your graphics card by using hardware bitblt and fillrect features.
This really doesn't have much to do with DRM but more about running
questionable code. That's why I went with the generalized stern
warning and default n, and explained when it's ok to do this (single
user and you care more about fbcon performance than potential issues
because you only run trusted stuff with access to your vt and fbdev
/dev node). Also you didn't keep any todo entry for removing DRM accel
code, and iirc some nouveau folks also complained that they at least
once had some kind of accel, so that's another reason to not tie this
to DRM.
Anyway aside from this looks reasonable, can you pls respin with a
more cautious Kconfig text?
Thanks, Daniel
> +
> config FRAMEBUFFER_CONSOLE_DETECT_PRIMARY
> bool "Map the console to the primary display device"
> depends on FRAMEBUFFER_CONSOLE
> diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
> index b813985f1403..f7b7d35953e8 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -1136,11 +1136,13 @@ static void fbcon_init(struct vc_data *vc, int init)
>
> ops->graphics = 0;
>
> +#ifdef CONFIG_FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION
> if ((cap & FBINFO_HWACCEL_COPYAREA) &&
> !(cap & FBINFO_HWACCEL_DISABLED))
> p->scrollmode = SCROLL_MOVE;
> else /* default to something safe */
> p->scrollmode = SCROLL_REDRAW;
> +#endif
>
> /*
> * ++guenther: console.c:vc_allocate() relies on initializing
> @@ -1705,7 +1707,7 @@ static bool fbcon_scroll(struct vc_data *vc, unsigned int t, unsigned int b,
> count = vc->vc_rows;
> if (logo_shown >= 0)
> goto redraw_up;
> - switch (p->scrollmode) {
> + switch (fb_scrollmode(p)) {
> case SCROLL_MOVE:
> fbcon_redraw_blit(vc, info, p, t, b - t - count,
> count);
> @@ -1795,7 +1797,7 @@ static bool fbcon_scroll(struct vc_data *vc, unsigned int t, unsigned int b,
> count = vc->vc_rows;
> if (logo_shown >= 0)
> goto redraw_down;
> - switch (p->scrollmode) {
> + switch (fb_scrollmode(p)) {
> case SCROLL_MOVE:
> fbcon_redraw_blit(vc, info, p, b - 1, b - t - count,
> -count);
> @@ -1946,12 +1948,12 @@ static void fbcon_bmove_rec(struct vc_data *vc, struct fbcon_display *p, int sy,
> height, width);
> }
>
> -static void updatescrollmode(struct fbcon_display *p,
> +static void updatescrollmode_accel(struct fbcon_display *p,
> struct fb_info *info,
> struct vc_data *vc)
> {
> +#ifdef CONFIG_FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION
> 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,
> @@ -1972,12 +1974,6 @@ static void updatescrollmode(struct fbcon_display *p,
> 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 ?
> @@ -1991,6 +1987,27 @@ static void updatescrollmode(struct fbcon_display *p,
> else
> p->scrollmode = SCROLL_REDRAW;
> }
> +#endif
> +}
> +
> +static void updatescrollmode(struct fbcon_display *p,
> + struct fb_info *info,
> + struct vc_data *vc)
> +{
> + struct fbcon_ops *ops = info->fbcon_par;
> + int fh = vc->vc_font.height;
> + 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);
> +
> + 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--;
> +
> + /* update scrollmode in case hardware acceleration is used */
> + updatescrollmode_accel(p, info, vc);
> }
>
> #define PITCH(w) (((w) + 7) >> 3)
> @@ -2148,7 +2165,7 @@ static int fbcon_switch(struct vc_data *vc)
>
> updatescrollmode(p, info, vc);
>
> - switch (p->scrollmode) {
> + switch (fb_scrollmode(p)) {
> case SCROLL_WRAP_MOVE:
> scrollback_phys_max = p->vrows - vc->vc_rows;
> break;
> diff --git a/drivers/video/fbdev/core/fbcon.h b/drivers/video/fbdev/core/fbcon.h
> index 9315b360c898..0f16cbc99e6a 100644
> --- a/drivers/video/fbdev/core/fbcon.h
> +++ b/drivers/video/fbdev/core/fbcon.h
> @@ -29,7 +29,9 @@ struct fbcon_display {
> /* Filled in by the low-level console driver */
> const u_char *fontdata;
> int userfont; /* != 0 if fontdata kmalloc()ed */
> - u_short scrollmode; /* Scroll Method */
> +#ifdef CONFIG_FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION
> + u_short scrollmode; /* Scroll Method, use fb_scrollmode() */
> +#endif
> u_short inverse; /* != 0 text black on white as default */
> short yscroll; /* Hardware scrolling */
> int vrows; /* number of virtual rows */
> @@ -208,6 +210,17 @@ static inline int attr_col_ec(int shift, struct vc_data *vc,
> #define SCROLL_REDRAW 0x004
> #define SCROLL_PAN_REDRAW 0x005
>
> +static inline u_short fb_scrollmode(struct fbcon_display *fb)
> +{
> +#ifdef CONFIG_FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION
> + return fb->scrollmode;
> +#else
> + /* hardcoded to SCROLL_REDRAW if acceleration was disabled. */
> + return SCROLL_REDRAW;
> +#endif
> +}
> +
> +
> #ifdef CONFIG_FB_TILEBLITTING
> extern void fbcon_set_tileops(struct vc_data *vc, struct fb_info *info);
> #endif
> diff --git a/drivers/video/fbdev/core/fbcon_ccw.c b/drivers/video/fbdev/core/fbcon_ccw.c
> index 9cd2c4b05c32..2789ace79634 100644
> --- a/drivers/video/fbdev/core/fbcon_ccw.c
> +++ b/drivers/video/fbdev/core/fbcon_ccw.c
> @@ -65,7 +65,7 @@ static void ccw_bmove(struct vc_data *vc, struct fb_info *info, int sy,
> {
> struct fbcon_ops *ops = info->fbcon_par;
> struct fb_copyarea area;
> - u32 vyres = GETVYRES(ops->p->scrollmode, info);
> + u32 vyres = GETVYRES(ops->p, info);
>
> area.sx = sy * vc->vc_font.height;
> area.sy = vyres - ((sx + width) * vc->vc_font.width);
> @@ -83,7 +83,7 @@ static void ccw_clear(struct vc_data *vc, struct fb_info *info, int sy,
> struct fbcon_ops *ops = info->fbcon_par;
> struct fb_fillrect region;
> int bgshift = (vc->vc_hi_font_mask) ? 13 : 12;
> - u32 vyres = GETVYRES(ops->p->scrollmode, info);
> + u32 vyres = GETVYRES(ops->p, info);
>
> region.color = attr_bgcol_ec(bgshift,vc,info);
> region.dx = sy * vc->vc_font.height;
> @@ -140,7 +140,7 @@ static void ccw_putcs(struct vc_data *vc, struct fb_info *info,
> u32 cnt, pitch, size;
> u32 attribute = get_attribute(info, scr_readw(s));
> u8 *dst, *buf = NULL;
> - u32 vyres = GETVYRES(ops->p->scrollmode, info);
> + u32 vyres = GETVYRES(ops->p, info);
>
> if (!ops->fontbuffer)
> return;
> @@ -229,7 +229,7 @@ static void ccw_cursor(struct vc_data *vc, struct fb_info *info, int mode,
> int attribute, use_sw = vc->vc_cursor_type & CUR_SW;
> int err = 1, dx, dy;
> char *src;
> - u32 vyres = GETVYRES(ops->p->scrollmode, info);
> + u32 vyres = GETVYRES(ops->p, info);
>
> if (!ops->fontbuffer)
> return;
> @@ -387,7 +387,7 @@ static int ccw_update_start(struct fb_info *info)
> {
> struct fbcon_ops *ops = info->fbcon_par;
> u32 yoffset;
> - u32 vyres = GETVYRES(ops->p->scrollmode, info);
> + u32 vyres = GETVYRES(ops->p, info);
> int err;
>
> yoffset = (vyres - info->var.yres) - ops->var.xoffset;
> diff --git a/drivers/video/fbdev/core/fbcon_cw.c b/drivers/video/fbdev/core/fbcon_cw.c
> index 88d89fad3f05..86a254c1b2b7 100644
> --- a/drivers/video/fbdev/core/fbcon_cw.c
> +++ b/drivers/video/fbdev/core/fbcon_cw.c
> @@ -50,7 +50,7 @@ static void cw_bmove(struct vc_data *vc, struct fb_info *info, int sy,
> {
> struct fbcon_ops *ops = info->fbcon_par;
> struct fb_copyarea area;
> - u32 vxres = GETVXRES(ops->p->scrollmode, info);
> + u32 vxres = GETVXRES(ops->p, info);
>
> area.sx = vxres - ((sy + height) * vc->vc_font.height);
> area.sy = sx * vc->vc_font.width;
> @@ -68,7 +68,7 @@ static void cw_clear(struct vc_data *vc, struct fb_info *info, int sy,
> struct fbcon_ops *ops = info->fbcon_par;
> struct fb_fillrect region;
> int bgshift = (vc->vc_hi_font_mask) ? 13 : 12;
> - u32 vxres = GETVXRES(ops->p->scrollmode, info);
> + u32 vxres = GETVXRES(ops->p, info);
>
> region.color = attr_bgcol_ec(bgshift,vc,info);
> region.dx = vxres - ((sy + height) * vc->vc_font.height);
> @@ -125,7 +125,7 @@ static void cw_putcs(struct vc_data *vc, struct fb_info *info,
> u32 cnt, pitch, size;
> u32 attribute = get_attribute(info, scr_readw(s));
> u8 *dst, *buf = NULL;
> - u32 vxres = GETVXRES(ops->p->scrollmode, info);
> + u32 vxres = GETVXRES(ops->p, info);
>
> if (!ops->fontbuffer)
> return;
> @@ -212,7 +212,7 @@ static void cw_cursor(struct vc_data *vc, struct fb_info *info, int mode,
> int attribute, use_sw = vc->vc_cursor_type & CUR_SW;
> int err = 1, dx, dy;
> char *src;
> - u32 vxres = GETVXRES(ops->p->scrollmode, info);
> + u32 vxres = GETVXRES(ops->p, info);
>
> if (!ops->fontbuffer)
> return;
> @@ -369,7 +369,7 @@ static void cw_cursor(struct vc_data *vc, struct fb_info *info, int mode,
> static int cw_update_start(struct fb_info *info)
> {
> struct fbcon_ops *ops = info->fbcon_par;
> - u32 vxres = GETVXRES(ops->p->scrollmode, info);
> + u32 vxres = GETVXRES(ops->p, info);
> u32 xoffset;
> int err;
>
> diff --git a/drivers/video/fbdev/core/fbcon_rotate.h b/drivers/video/fbdev/core/fbcon_rotate.h
> index e233444cda66..01cbe303b8a2 100644
> --- a/drivers/video/fbdev/core/fbcon_rotate.h
> +++ b/drivers/video/fbdev/core/fbcon_rotate.h
> @@ -12,11 +12,11 @@
> #define _FBCON_ROTATE_H
>
> #define GETVYRES(s,i) ({ \
> - (s == SCROLL_REDRAW || s == SCROLL_MOVE) ? \
> + (fb_scrollmode(s) == SCROLL_REDRAW || fb_scrollmode(s) == SCROLL_MOVE) ? \
> (i)->var.yres : (i)->var.yres_virtual; })
>
> #define GETVXRES(s,i) ({ \
> - (s == SCROLL_REDRAW || s == SCROLL_MOVE || !(i)->fix.xpanstep) ? \
> + (fb_scrollmode(s) == SCROLL_REDRAW || fb_scrollmode(s) == SCROLL_MOVE || !(i)->fix.xpanstep) ? \
> (i)->var.xres : (i)->var.xres_virtual; })
>
>
> diff --git a/drivers/video/fbdev/core/fbcon_ud.c b/drivers/video/fbdev/core/fbcon_ud.c
> index 8d5e66b1bdfb..23bc045769d0 100644
> --- a/drivers/video/fbdev/core/fbcon_ud.c
> +++ b/drivers/video/fbdev/core/fbcon_ud.c
> @@ -50,8 +50,8 @@ static void ud_bmove(struct vc_data *vc, struct fb_info *info, int sy,
> {
> struct fbcon_ops *ops = info->fbcon_par;
> struct fb_copyarea area;
> - u32 vyres = GETVYRES(ops->p->scrollmode, info);
> - u32 vxres = GETVXRES(ops->p->scrollmode, info);
> + u32 vyres = GETVYRES(ops->p, info);
> + u32 vxres = GETVXRES(ops->p, info);
>
> area.sy = vyres - ((sy + height) * vc->vc_font.height);
> area.sx = vxres - ((sx + width) * vc->vc_font.width);
> @@ -69,8 +69,8 @@ static void ud_clear(struct vc_data *vc, struct fb_info *info, int sy,
> struct fbcon_ops *ops = info->fbcon_par;
> struct fb_fillrect region;
> int bgshift = (vc->vc_hi_font_mask) ? 13 : 12;
> - u32 vyres = GETVYRES(ops->p->scrollmode, info);
> - u32 vxres = GETVXRES(ops->p->scrollmode, info);
> + u32 vyres = GETVYRES(ops->p, info);
> + u32 vxres = GETVXRES(ops->p, info);
>
> region.color = attr_bgcol_ec(bgshift,vc,info);
> region.dy = vyres - ((sy + height) * vc->vc_font.height);
> @@ -162,8 +162,8 @@ static void ud_putcs(struct vc_data *vc, struct fb_info *info,
> u32 mod = vc->vc_font.width % 8, cnt, pitch, size;
> u32 attribute = get_attribute(info, scr_readw(s));
> u8 *dst, *buf = NULL;
> - u32 vyres = GETVYRES(ops->p->scrollmode, info);
> - u32 vxres = GETVXRES(ops->p->scrollmode, info);
> + u32 vyres = GETVYRES(ops->p, info);
> + u32 vxres = GETVXRES(ops->p, info);
>
> if (!ops->fontbuffer)
> return;
> @@ -259,8 +259,8 @@ static void ud_cursor(struct vc_data *vc, struct fb_info *info, int mode,
> int attribute, use_sw = vc->vc_cursor_type & CUR_SW;
> int err = 1, dx, dy;
> char *src;
> - u32 vyres = GETVYRES(ops->p->scrollmode, info);
> - u32 vxres = GETVXRES(ops->p->scrollmode, info);
> + u32 vyres = GETVYRES(ops->p, info);
> + u32 vxres = GETVXRES(ops->p, info);
>
> if (!ops->fontbuffer)
> return;
> @@ -410,8 +410,8 @@ static int ud_update_start(struct fb_info *info)
> {
> struct fbcon_ops *ops = info->fbcon_par;
> int xoffset, yoffset;
> - u32 vyres = GETVYRES(ops->p->scrollmode, info);
> - u32 vxres = GETVXRES(ops->p->scrollmode, info);
> + u32 vyres = GETVYRES(ops->p, info);
> + u32 vxres = GETVXRES(ops->p, info);
> int err;
>
> xoffset = vxres - info->var.xres - ops->var.xoffset;
> --
> 2.34.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 3/3] fbcon: Add option to enable legacy hardware acceleration
2022-02-01 20:11 ` Daniel Vetter
@ 2022-02-01 22:52 ` Helge Deller
-1 siblings, 0 replies; 12+ messages in thread
From: Helge Deller @ 2022-02-01 22:52 UTC (permalink / raw)
To: Daniel Vetter; +Cc: DRI Development, linux-fbdev
Hello Daniel,
On 2/1/22 21:11, Daniel Vetter wrote:
> On Tue, Feb 1, 2022 at 7:59 PM Helge Deller <deller@gmx.de> wrote:
>>
>> Add a config option CONFIG_FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION to
>> enable bitblt and fillrect hardware acceleration in the framebuffer
>> console. If disabled, such acceleration will not be used, even if it is
>> supported by the graphics hardware driver.
>>
>> If you plan to use DRM as your main graphics output system, you should
>> disable this option since it will prevent compiling in code which isn't
>> used later on when DRM takes over.
>>
>> For all other configurations, e.g. if none of your graphic cards support
>> DRM (yet), DRM isn't available for your architecture, or you can't be
>> sure that the graphic card in the target system will support DRM, you
>> most likely want to enable this option.
>>
>> In the non-accelerated case (e.g. when DRM is used), the inlined
>> fb_scrollmode() function is hardcoded to return SCROLL_REDRAW and as such the
>> compiler is able to optimize much unneccesary code away.
>>
>> In this v3 patch version I additionally changed the GETVYRES() and GETVXRES()
>> macros to take a pointer to the fbcon_display struct. This fixes the build when
>> console rotation is enabled and helps the compiler again to optimize out code.
>>
>> Signed-off-by: Helge Deller <deller@gmx.de>
>> Cc: stable@vger.kernel.org # v5.10+
>> Signed-off-by: Helge Deller <deller@gmx.de>
>> ---
>> drivers/video/console/Kconfig | 11 +++++++
>> drivers/video/fbdev/core/fbcon.c | 39 ++++++++++++++++++-------
>> drivers/video/fbdev/core/fbcon.h | 15 +++++++++-
>> drivers/video/fbdev/core/fbcon_ccw.c | 10 +++----
>> drivers/video/fbdev/core/fbcon_cw.c | 10 +++----
>> drivers/video/fbdev/core/fbcon_rotate.h | 4 +--
>> drivers/video/fbdev/core/fbcon_ud.c | 20 ++++++-------
>> 7 files changed, 75 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/video/console/Kconfig b/drivers/video/console/Kconfig
>> index 840d9813b0bc..6029fd41d7d0 100644
>> --- a/drivers/video/console/Kconfig
>> +++ b/drivers/video/console/Kconfig
>> @@ -78,6 +78,17 @@ config FRAMEBUFFER_CONSOLE
>> help
>> Low-level framebuffer-based console driver.
>>
>> +config FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION
>> + bool "Framebuffer Console hardware acceleration support"
>> + depends on FRAMEBUFFER_CONSOLE
>> + default n if DRM
>> + default y
>> + help
>> + If you use a system on which DRM is fully supported you usually want to say N,
>> + otherwise you probably want to enable this option.
>> + If enabled the framebuffer console will utilize the hardware acceleration
>> + of your graphics card by using hardware bitblt and fillrect features.
>
> This really doesn't have much to do with DRM but more about running
> questionable code. That's why I went with the generalized stern
> warning and default n, and explained when it's ok to do this (single
> user and you care more about fbcon performance than potential issues
> because you only run trusted stuff with access to your vt and fbdev
> /dev node).
I think this is something we both will keep to have different opinion about :-)
This console acceleration code is not exported or visible to userspace,
e.g. you can't access or trigger it via /dev/fb0.
It's only called internally from inside fbcon, so it's independed if
it's a single- or multi-user system.
The only way to "access" it is via standard stdio, where fbcon then
either scrolls the screen via redrawing characters at new positions
or by using hardware bitblt to move screen contents.
IMHO there is nothing which is critical here.
Even when I analyzed the existing bug reports, there was none which
affected that specific code.
Anyway, let's look at that part in your patch:
+ bool "Enable legacy fbcon code for hw acceleration"
+ depends on FRAMEBUFFER_CONSOLE
+ default n
+ help
+ Only enable this options if you are in full control of machine since
+ the fbcon acceleration code is essentially unmaintained and known
+ problematic.
+
+ If unsure, select n.
Since I'm willing to maintain that scrolling/panning code, I'd like to
drop the "is essentially unmaintained" part.
And the "known problematic" part is up to now just speculative (which would be
valid for other parts of the kernel too, btw).
As this whole disussions showed, there are some few architectures/platforms
which really still need this acceleration, while others don't.
So, why not leave the decision up to the arch maintainers, which may opt-in
or opt-out, while keeping the default on "n"?
With that, here is a new proposal:
+config FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION
+ bool "Enable legacy fbcon hardware acceleration code"
+ depends on FRAMEBUFFER_CONSOLE
+ default y if (PARISC) # a few other arches may want to be listed here too...
+ default n
+ help
+ This option enables the fbcon (framebuffer text-based) hardware acceleration for
+ graphics drivers which were written for the fbdev graphics interface.
+
+ On modern machines, on mainstream machines (like x86-64) or when using a modern
+ Linux distribution those fbdev drivers usually aren't used.
+ So enabling this option wouldn't have any effect, which is why you want
+ to disable this option on such newer machines.
+
+ If you compile this kernel for older machines which still require the fbdev
+ drivers, you may want to say Y.
+
+ If unsure, select n.
Would that be acceptable?
Arch maintainers may then later send (or reply now with) patches, e.g.:
+ default y if (M68K && XYZ)
...
> Also you didn't keep any todo entry for removing DRM accel code,
That wasn't intentional.
I just sent 2 revert-patches and an fbcon-accel-enabling-patch.
I'll look up what you had in your patch series and add it as seperate patch.
> and iirc some nouveau folks also complained that they at least
> once had some kind of accel, so that's another reason to not tie this
> to DRM.
The above proposal to add additional "default y if XYZ" would enable
them to opt-in.
> Anyway aside from this looks reasonable, can you pls respin with a
> more cautious Kconfig text?
Sure, I'll do as soon as we have a decision on the above Kconfig text.
Helge
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 3/3] fbcon: Add option to enable legacy hardware acceleration
@ 2022-02-01 22:52 ` Helge Deller
0 siblings, 0 replies; 12+ messages in thread
From: Helge Deller @ 2022-02-01 22:52 UTC (permalink / raw)
To: Daniel Vetter; +Cc: linux-fbdev, DRI Development
Hello Daniel,
On 2/1/22 21:11, Daniel Vetter wrote:
> On Tue, Feb 1, 2022 at 7:59 PM Helge Deller <deller@gmx.de> wrote:
>>
>> Add a config option CONFIG_FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION to
>> enable bitblt and fillrect hardware acceleration in the framebuffer
>> console. If disabled, such acceleration will not be used, even if it is
>> supported by the graphics hardware driver.
>>
>> If you plan to use DRM as your main graphics output system, you should
>> disable this option since it will prevent compiling in code which isn't
>> used later on when DRM takes over.
>>
>> For all other configurations, e.g. if none of your graphic cards support
>> DRM (yet), DRM isn't available for your architecture, or you can't be
>> sure that the graphic card in the target system will support DRM, you
>> most likely want to enable this option.
>>
>> In the non-accelerated case (e.g. when DRM is used), the inlined
>> fb_scrollmode() function is hardcoded to return SCROLL_REDRAW and as such the
>> compiler is able to optimize much unneccesary code away.
>>
>> In this v3 patch version I additionally changed the GETVYRES() and GETVXRES()
>> macros to take a pointer to the fbcon_display struct. This fixes the build when
>> console rotation is enabled and helps the compiler again to optimize out code.
>>
>> Signed-off-by: Helge Deller <deller@gmx.de>
>> Cc: stable@vger.kernel.org # v5.10+
>> Signed-off-by: Helge Deller <deller@gmx.de>
>> ---
>> drivers/video/console/Kconfig | 11 +++++++
>> drivers/video/fbdev/core/fbcon.c | 39 ++++++++++++++++++-------
>> drivers/video/fbdev/core/fbcon.h | 15 +++++++++-
>> drivers/video/fbdev/core/fbcon_ccw.c | 10 +++----
>> drivers/video/fbdev/core/fbcon_cw.c | 10 +++----
>> drivers/video/fbdev/core/fbcon_rotate.h | 4 +--
>> drivers/video/fbdev/core/fbcon_ud.c | 20 ++++++-------
>> 7 files changed, 75 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/video/console/Kconfig b/drivers/video/console/Kconfig
>> index 840d9813b0bc..6029fd41d7d0 100644
>> --- a/drivers/video/console/Kconfig
>> +++ b/drivers/video/console/Kconfig
>> @@ -78,6 +78,17 @@ config FRAMEBUFFER_CONSOLE
>> help
>> Low-level framebuffer-based console driver.
>>
>> +config FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION
>> + bool "Framebuffer Console hardware acceleration support"
>> + depends on FRAMEBUFFER_CONSOLE
>> + default n if DRM
>> + default y
>> + help
>> + If you use a system on which DRM is fully supported you usually want to say N,
>> + otherwise you probably want to enable this option.
>> + If enabled the framebuffer console will utilize the hardware acceleration
>> + of your graphics card by using hardware bitblt and fillrect features.
>
> This really doesn't have much to do with DRM but more about running
> questionable code. That's why I went with the generalized stern
> warning and default n, and explained when it's ok to do this (single
> user and you care more about fbcon performance than potential issues
> because you only run trusted stuff with access to your vt and fbdev
> /dev node).
I think this is something we both will keep to have different opinion about :-)
This console acceleration code is not exported or visible to userspace,
e.g. you can't access or trigger it via /dev/fb0.
It's only called internally from inside fbcon, so it's independed if
it's a single- or multi-user system.
The only way to "access" it is via standard stdio, where fbcon then
either scrolls the screen via redrawing characters at new positions
or by using hardware bitblt to move screen contents.
IMHO there is nothing which is critical here.
Even when I analyzed the existing bug reports, there was none which
affected that specific code.
Anyway, let's look at that part in your patch:
+ bool "Enable legacy fbcon code for hw acceleration"
+ depends on FRAMEBUFFER_CONSOLE
+ default n
+ help
+ Only enable this options if you are in full control of machine since
+ the fbcon acceleration code is essentially unmaintained and known
+ problematic.
+
+ If unsure, select n.
Since I'm willing to maintain that scrolling/panning code, I'd like to
drop the "is essentially unmaintained" part.
And the "known problematic" part is up to now just speculative (which would be
valid for other parts of the kernel too, btw).
As this whole disussions showed, there are some few architectures/platforms
which really still need this acceleration, while others don't.
So, why not leave the decision up to the arch maintainers, which may opt-in
or opt-out, while keeping the default on "n"?
With that, here is a new proposal:
+config FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION
+ bool "Enable legacy fbcon hardware acceleration code"
+ depends on FRAMEBUFFER_CONSOLE
+ default y if (PARISC) # a few other arches may want to be listed here too...
+ default n
+ help
+ This option enables the fbcon (framebuffer text-based) hardware acceleration for
+ graphics drivers which were written for the fbdev graphics interface.
+
+ On modern machines, on mainstream machines (like x86-64) or when using a modern
+ Linux distribution those fbdev drivers usually aren't used.
+ So enabling this option wouldn't have any effect, which is why you want
+ to disable this option on such newer machines.
+
+ If you compile this kernel for older machines which still require the fbdev
+ drivers, you may want to say Y.
+
+ If unsure, select n.
Would that be acceptable?
Arch maintainers may then later send (or reply now with) patches, e.g.:
+ default y if (M68K && XYZ)
...
> Also you didn't keep any todo entry for removing DRM accel code,
That wasn't intentional.
I just sent 2 revert-patches and an fbcon-accel-enabling-patch.
I'll look up what you had in your patch series and add it as seperate patch.
> and iirc some nouveau folks also complained that they at least
> once had some kind of accel, so that's another reason to not tie this
> to DRM.
The above proposal to add additional "default y if XYZ" would enable
them to opt-in.
> Anyway aside from this looks reasonable, can you pls respin with a
> more cautious Kconfig text?
Sure, I'll do as soon as we have a decision on the above Kconfig text.
Helge
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 3/3] fbcon: Add option to enable legacy hardware acceleration
2022-02-01 22:52 ` Helge Deller
@ 2022-02-02 11:05 ` Daniel Vetter
-1 siblings, 0 replies; 12+ messages in thread
From: Daniel Vetter @ 2022-02-02 11:05 UTC (permalink / raw)
To: Helge Deller; +Cc: DRI Development, linux-fbdev
On Tue, Feb 1, 2022 at 11:52 PM Helge Deller <deller@gmx.de> wrote:
>
> Hello Daniel,
>
> On 2/1/22 21:11, Daniel Vetter wrote:
> > On Tue, Feb 1, 2022 at 7:59 PM Helge Deller <deller@gmx.de> wrote:
> >>
> >> Add a config option CONFIG_FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION to
> >> enable bitblt and fillrect hardware acceleration in the framebuffer
> >> console. If disabled, such acceleration will not be used, even if it is
> >> supported by the graphics hardware driver.
> >>
> >> If you plan to use DRM as your main graphics output system, you should
> >> disable this option since it will prevent compiling in code which isn't
> >> used later on when DRM takes over.
> >>
> >> For all other configurations, e.g. if none of your graphic cards support
> >> DRM (yet), DRM isn't available for your architecture, or you can't be
> >> sure that the graphic card in the target system will support DRM, you
> >> most likely want to enable this option.
> >>
> >> In the non-accelerated case (e.g. when DRM is used), the inlined
> >> fb_scrollmode() function is hardcoded to return SCROLL_REDRAW and as such the
> >> compiler is able to optimize much unneccesary code away.
> >>
> >> In this v3 patch version I additionally changed the GETVYRES() and GETVXRES()
> >> macros to take a pointer to the fbcon_display struct. This fixes the build when
> >> console rotation is enabled and helps the compiler again to optimize out code.
> >>
> >> Signed-off-by: Helge Deller <deller@gmx.de>
> >> Cc: stable@vger.kernel.org # v5.10+
> >> Signed-off-by: Helge Deller <deller@gmx.de>
> >> ---
> >> drivers/video/console/Kconfig | 11 +++++++
> >> drivers/video/fbdev/core/fbcon.c | 39 ++++++++++++++++++-------
> >> drivers/video/fbdev/core/fbcon.h | 15 +++++++++-
> >> drivers/video/fbdev/core/fbcon_ccw.c | 10 +++----
> >> drivers/video/fbdev/core/fbcon_cw.c | 10 +++----
> >> drivers/video/fbdev/core/fbcon_rotate.h | 4 +--
> >> drivers/video/fbdev/core/fbcon_ud.c | 20 ++++++-------
> >> 7 files changed, 75 insertions(+), 34 deletions(-)
> >>
> >> diff --git a/drivers/video/console/Kconfig b/drivers/video/console/Kconfig
> >> index 840d9813b0bc..6029fd41d7d0 100644
> >> --- a/drivers/video/console/Kconfig
> >> +++ b/drivers/video/console/Kconfig
> >> @@ -78,6 +78,17 @@ config FRAMEBUFFER_CONSOLE
> >> help
> >> Low-level framebuffer-based console driver.
> >>
> >> +config FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION
> >> + bool "Framebuffer Console hardware acceleration support"
> >> + depends on FRAMEBUFFER_CONSOLE
> >> + default n if DRM
> >> + default y
> >> + help
> >> + If you use a system on which DRM is fully supported you usually want to say N,
> >> + otherwise you probably want to enable this option.
> >> + If enabled the framebuffer console will utilize the hardware acceleration
> >> + of your graphics card by using hardware bitblt and fillrect features.
> >
> > This really doesn't have much to do with DRM but more about running
> > questionable code. That's why I went with the generalized stern
> > warning and default n, and explained when it's ok to do this (single
> > user and you care more about fbcon performance than potential issues
> > because you only run trusted stuff with access to your vt and fbdev
> > /dev node).
>
> I think this is something we both will keep to have different opinion about :-)
>
> This console acceleration code is not exported or visible to userspace,
> e.g. you can't access or trigger it via /dev/fb0.
> It's only called internally from inside fbcon, so it's independed if
> it's a single- or multi-user system.
> The only way to "access" it is via standard stdio, where fbcon then
> either scrolls the screen via redrawing characters at new positions
> or by using hardware bitblt to move screen contents.
> IMHO there is nothing which is critical here.
> Even when I analyzed the existing bug reports, there was none which
> affected that specific code.
Maybe to clarify. The issues that generally result in this code going
boom in syzbot are when you race console activity (which can be
largely controlled through VT ioctls from userspace too, plus
/dev/kmsg) against fbdev resizing on the /dev/fb/0 nodes. The locking
there is kinda wild, and the code is very hard to understand. This is
why we've resorted to just disabling/deleting this code because most
cases we have no idea what's actually going on, aside from something
is clearly not going right.
Also the multi-user here means "you run untrusted code from other
people", not "you run multiple things in parallel" or "multiple people
are logged in at the same time".
> Anyway, let's look at that part in your patch:
>
> + bool "Enable legacy fbcon code for hw acceleration"
> + depends on FRAMEBUFFER_CONSOLE
> + default n
> + help
> + Only enable this options if you are in full control of machine since
> + the fbcon acceleration code is essentially unmaintained and known
> + problematic.
> +
> + If unsure, select n.
>
> Since I'm willing to maintain that scrolling/panning code, I'd like to
> drop the "is essentially unmaintained" part.
> And the "known problematic" part is up to now just speculative (which would be
> valid for other parts of the kernel too, btw).
>
> As this whole disussions showed, there are some few architectures/platforms
> which really still need this acceleration, while others don't.
> So, why not leave the decision up to the arch maintainers, which may opt-in
> or opt-out, while keeping the default on "n"?
>
> With that, here is a new proposal:
>
> +config FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION
> + bool "Enable legacy fbcon hardware acceleration code"
> + depends on FRAMEBUFFER_CONSOLE
> + default y if (PARISC) # a few other arches may want to be listed here too...
> + default n
> + help
> + This option enables the fbcon (framebuffer text-based) hardware acceleration for
> + graphics drivers which were written for the fbdev graphics interface.
> +
> + On modern machines, on mainstream machines (like x86-64) or when using a modern
> + Linux distribution those fbdev drivers usually aren't used.
> + So enabling this option wouldn't have any effect, which is why you want
> + to disable this option on such newer machines.
> +
> + If you compile this kernel for older machines which still require the fbdev
> + drivers, you may want to say Y.
> +
> + If unsure, select n.
>
> Would that be acceptable?
Perfect.
> Arch maintainers may then later send (or reply now with) patches, e.g.:
> + default y if (M68K && XYZ)
> ...
Yeah makes sense.
> > Also you didn't keep any todo entry for removing DRM accel code,
>
> That wasn't intentional.
> I just sent 2 revert-patches and an fbcon-accel-enabling-patch.
> I'll look up what you had in your patch series and add it as seperate patch.
tbh I think it's fine either way. I think it's still rather unclear
which way drm will go, least because there's not many people who can
occasionally squeeze some time away to move things forward. Could be
that we add a new emergency logging console in the kernel for drm, and
distros switch over to kmscon (which is in userspace, and accelerated
iirc if you have gl, so most modern systems). Or whether we just
improve fbcon until it's solid enough. Or some other option.
So given that just dropping the todo sounds ok, it was just a bit
inconsistent with the Kconfig :-)
> > and iirc some nouveau folks also complained that they at least
> > once had some kind of accel, so that's another reason to not tie this
> > to DRM.
>
> The above proposal to add additional "default y if XYZ" would enable
> them to opt-in.
>
> > Anyway aside from this looks reasonable, can you pls respin with a
> > more cautious Kconfig text?
>
> Sure, I'll do as soon as we have a decision on the above Kconfig text.
Ideally if you can send them out today so it's not missing the
drm-misc-fixes train that leaves tomorrow, that would be best.
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 3/3] fbcon: Add option to enable legacy hardware acceleration
@ 2022-02-02 11:05 ` Daniel Vetter
0 siblings, 0 replies; 12+ messages in thread
From: Daniel Vetter @ 2022-02-02 11:05 UTC (permalink / raw)
To: Helge Deller; +Cc: linux-fbdev, DRI Development
On Tue, Feb 1, 2022 at 11:52 PM Helge Deller <deller@gmx.de> wrote:
>
> Hello Daniel,
>
> On 2/1/22 21:11, Daniel Vetter wrote:
> > On Tue, Feb 1, 2022 at 7:59 PM Helge Deller <deller@gmx.de> wrote:
> >>
> >> Add a config option CONFIG_FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION to
> >> enable bitblt and fillrect hardware acceleration in the framebuffer
> >> console. If disabled, such acceleration will not be used, even if it is
> >> supported by the graphics hardware driver.
> >>
> >> If you plan to use DRM as your main graphics output system, you should
> >> disable this option since it will prevent compiling in code which isn't
> >> used later on when DRM takes over.
> >>
> >> For all other configurations, e.g. if none of your graphic cards support
> >> DRM (yet), DRM isn't available for your architecture, or you can't be
> >> sure that the graphic card in the target system will support DRM, you
> >> most likely want to enable this option.
> >>
> >> In the non-accelerated case (e.g. when DRM is used), the inlined
> >> fb_scrollmode() function is hardcoded to return SCROLL_REDRAW and as such the
> >> compiler is able to optimize much unneccesary code away.
> >>
> >> In this v3 patch version I additionally changed the GETVYRES() and GETVXRES()
> >> macros to take a pointer to the fbcon_display struct. This fixes the build when
> >> console rotation is enabled and helps the compiler again to optimize out code.
> >>
> >> Signed-off-by: Helge Deller <deller@gmx.de>
> >> Cc: stable@vger.kernel.org # v5.10+
> >> Signed-off-by: Helge Deller <deller@gmx.de>
> >> ---
> >> drivers/video/console/Kconfig | 11 +++++++
> >> drivers/video/fbdev/core/fbcon.c | 39 ++++++++++++++++++-------
> >> drivers/video/fbdev/core/fbcon.h | 15 +++++++++-
> >> drivers/video/fbdev/core/fbcon_ccw.c | 10 +++----
> >> drivers/video/fbdev/core/fbcon_cw.c | 10 +++----
> >> drivers/video/fbdev/core/fbcon_rotate.h | 4 +--
> >> drivers/video/fbdev/core/fbcon_ud.c | 20 ++++++-------
> >> 7 files changed, 75 insertions(+), 34 deletions(-)
> >>
> >> diff --git a/drivers/video/console/Kconfig b/drivers/video/console/Kconfig
> >> index 840d9813b0bc..6029fd41d7d0 100644
> >> --- a/drivers/video/console/Kconfig
> >> +++ b/drivers/video/console/Kconfig
> >> @@ -78,6 +78,17 @@ config FRAMEBUFFER_CONSOLE
> >> help
> >> Low-level framebuffer-based console driver.
> >>
> >> +config FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION
> >> + bool "Framebuffer Console hardware acceleration support"
> >> + depends on FRAMEBUFFER_CONSOLE
> >> + default n if DRM
> >> + default y
> >> + help
> >> + If you use a system on which DRM is fully supported you usually want to say N,
> >> + otherwise you probably want to enable this option.
> >> + If enabled the framebuffer console will utilize the hardware acceleration
> >> + of your graphics card by using hardware bitblt and fillrect features.
> >
> > This really doesn't have much to do with DRM but more about running
> > questionable code. That's why I went with the generalized stern
> > warning and default n, and explained when it's ok to do this (single
> > user and you care more about fbcon performance than potential issues
> > because you only run trusted stuff with access to your vt and fbdev
> > /dev node).
>
> I think this is something we both will keep to have different opinion about :-)
>
> This console acceleration code is not exported or visible to userspace,
> e.g. you can't access or trigger it via /dev/fb0.
> It's only called internally from inside fbcon, so it's independed if
> it's a single- or multi-user system.
> The only way to "access" it is via standard stdio, where fbcon then
> either scrolls the screen via redrawing characters at new positions
> or by using hardware bitblt to move screen contents.
> IMHO there is nothing which is critical here.
> Even when I analyzed the existing bug reports, there was none which
> affected that specific code.
Maybe to clarify. The issues that generally result in this code going
boom in syzbot are when you race console activity (which can be
largely controlled through VT ioctls from userspace too, plus
/dev/kmsg) against fbdev resizing on the /dev/fb/0 nodes. The locking
there is kinda wild, and the code is very hard to understand. This is
why we've resorted to just disabling/deleting this code because most
cases we have no idea what's actually going on, aside from something
is clearly not going right.
Also the multi-user here means "you run untrusted code from other
people", not "you run multiple things in parallel" or "multiple people
are logged in at the same time".
> Anyway, let's look at that part in your patch:
>
> + bool "Enable legacy fbcon code for hw acceleration"
> + depends on FRAMEBUFFER_CONSOLE
> + default n
> + help
> + Only enable this options if you are in full control of machine since
> + the fbcon acceleration code is essentially unmaintained and known
> + problematic.
> +
> + If unsure, select n.
>
> Since I'm willing to maintain that scrolling/panning code, I'd like to
> drop the "is essentially unmaintained" part.
> And the "known problematic" part is up to now just speculative (which would be
> valid for other parts of the kernel too, btw).
>
> As this whole disussions showed, there are some few architectures/platforms
> which really still need this acceleration, while others don't.
> So, why not leave the decision up to the arch maintainers, which may opt-in
> or opt-out, while keeping the default on "n"?
>
> With that, here is a new proposal:
>
> +config FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION
> + bool "Enable legacy fbcon hardware acceleration code"
> + depends on FRAMEBUFFER_CONSOLE
> + default y if (PARISC) # a few other arches may want to be listed here too...
> + default n
> + help
> + This option enables the fbcon (framebuffer text-based) hardware acceleration for
> + graphics drivers which were written for the fbdev graphics interface.
> +
> + On modern machines, on mainstream machines (like x86-64) or when using a modern
> + Linux distribution those fbdev drivers usually aren't used.
> + So enabling this option wouldn't have any effect, which is why you want
> + to disable this option on such newer machines.
> +
> + If you compile this kernel for older machines which still require the fbdev
> + drivers, you may want to say Y.
> +
> + If unsure, select n.
>
> Would that be acceptable?
Perfect.
> Arch maintainers may then later send (or reply now with) patches, e.g.:
> + default y if (M68K && XYZ)
> ...
Yeah makes sense.
> > Also you didn't keep any todo entry for removing DRM accel code,
>
> That wasn't intentional.
> I just sent 2 revert-patches and an fbcon-accel-enabling-patch.
> I'll look up what you had in your patch series and add it as seperate patch.
tbh I think it's fine either way. I think it's still rather unclear
which way drm will go, least because there's not many people who can
occasionally squeeze some time away to move things forward. Could be
that we add a new emergency logging console in the kernel for drm, and
distros switch over to kmscon (which is in userspace, and accelerated
iirc if you have gl, so most modern systems). Or whether we just
improve fbcon until it's solid enough. Or some other option.
So given that just dropping the todo sounds ok, it was just a bit
inconsistent with the Kconfig :-)
> > and iirc some nouveau folks also complained that they at least
> > once had some kind of accel, so that's another reason to not tie this
> > to DRM.
>
> The above proposal to add additional "default y if XYZ" would enable
> them to opt-in.
>
> > Anyway aside from this looks reasonable, can you pls respin with a
> > more cautious Kconfig text?
>
> Sure, I'll do as soon as we have a decision on the above Kconfig text.
Ideally if you can send them out today so it's not missing the
drm-misc-fixes train that leaves tomorrow, that would be best.
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 3/3] fbcon: Add option to enable legacy hardware acceleration
2022-02-02 11:05 ` Daniel Vetter
@ 2022-02-02 13:41 ` Helge Deller
-1 siblings, 0 replies; 12+ messages in thread
From: Helge Deller @ 2022-02-02 13:41 UTC (permalink / raw)
To: Daniel Vetter; +Cc: DRI Development, linux-fbdev
On 2/2/22 12:05, Daniel Vetter wrote:
> On Tue, Feb 1, 2022 at 11:52 PM Helge Deller <deller@gmx.de> wrote:
>>
>> Hello Daniel,
>>
>> On 2/1/22 21:11, Daniel Vetter wrote:
>>> On Tue, Feb 1, 2022 at 7:59 PM Helge Deller <deller@gmx.de> wrote:
>>>>
>>>> Add a config option CONFIG_FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION to
>>>> enable bitblt and fillrect hardware acceleration in the framebuffer
>>>> console. If disabled, such acceleration will not be used, even if it is
>>>> supported by the graphics hardware driver.
>>>>
>>>> If you plan to use DRM as your main graphics output system, you should
>>>> disable this option since it will prevent compiling in code which isn't
>>>> used later on when DRM takes over.
>>>>
>>>> For all other configurations, e.g. if none of your graphic cards support
>>>> DRM (yet), DRM isn't available for your architecture, or you can't be
>>>> sure that the graphic card in the target system will support DRM, you
>>>> most likely want to enable this option.
>>>>
>>>> In the non-accelerated case (e.g. when DRM is used), the inlined
>>>> fb_scrollmode() function is hardcoded to return SCROLL_REDRAW and as such the
>>>> compiler is able to optimize much unneccesary code away.
>>>>
>>>> In this v3 patch version I additionally changed the GETVYRES() and GETVXRES()
>>>> macros to take a pointer to the fbcon_display struct. This fixes the build when
>>>> console rotation is enabled and helps the compiler again to optimize out code.
>>>>
>>>> Signed-off-by: Helge Deller <deller@gmx.de>
>>>> Cc: stable@vger.kernel.org # v5.10+
>>>> Signed-off-by: Helge Deller <deller@gmx.de>
>>>> ---
>>>> drivers/video/console/Kconfig | 11 +++++++
>>>> drivers/video/fbdev/core/fbcon.c | 39 ++++++++++++++++++-------
>>>> drivers/video/fbdev/core/fbcon.h | 15 +++++++++-
>>>> drivers/video/fbdev/core/fbcon_ccw.c | 10 +++----
>>>> drivers/video/fbdev/core/fbcon_cw.c | 10 +++----
>>>> drivers/video/fbdev/core/fbcon_rotate.h | 4 +--
>>>> drivers/video/fbdev/core/fbcon_ud.c | 20 ++++++-------
>>>> 7 files changed, 75 insertions(+), 34 deletions(-)
>>>>
>>>> diff --git a/drivers/video/console/Kconfig b/drivers/video/console/Kconfig
>>>> index 840d9813b0bc..6029fd41d7d0 100644
>>>> --- a/drivers/video/console/Kconfig
>>>> +++ b/drivers/video/console/Kconfig
>>>> @@ -78,6 +78,17 @@ config FRAMEBUFFER_CONSOLE
>>>> help
>>>> Low-level framebuffer-based console driver.
>>>>
>>>> +config FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION
>>>> + bool "Framebuffer Console hardware acceleration support"
>>>> + depends on FRAMEBUFFER_CONSOLE
>>>> + default n if DRM
>>>> + default y
>>>> + help
>>>> + If you use a system on which DRM is fully supported you usually want to say N,
>>>> + otherwise you probably want to enable this option.
>>>> + If enabled the framebuffer console will utilize the hardware acceleration
>>>> + of your graphics card by using hardware bitblt and fillrect features.
>>>
>>> This really doesn't have much to do with DRM but more about running
>>> questionable code. That's why I went with the generalized stern
>>> warning and default n, and explained when it's ok to do this (single
>>> user and you care more about fbcon performance than potential issues
>>> because you only run trusted stuff with access to your vt and fbdev
>>> /dev node).
>>
>> I think this is something we both will keep to have different opinion about :-)
>>
>> This console acceleration code is not exported or visible to userspace,
>> e.g. you can't access or trigger it via /dev/fb0.
>> It's only called internally from inside fbcon, so it's independed if
>> it's a single- or multi-user system.
>> The only way to "access" it is via standard stdio, where fbcon then
>> either scrolls the screen via redrawing characters at new positions
>> or by using hardware bitblt to move screen contents.
>> IMHO there is nothing which is critical here.
>> Even when I analyzed the existing bug reports, there was none which
>> affected that specific code.
>
> Maybe to clarify. The issues that generally result in this code going
> boom in syzbot are when you race console activity (which can be
> largely controlled through VT ioctls from userspace too, plus
> /dev/kmsg) against fbdev resizing on the /dev/fb/0 nodes. The locking
> there is kinda wild, and the code is very hard to understand. This is
> why we've resorted to just disabling/deleting this code because most
> cases we have no idea what's actually going on, aside from something
> is clearly not going right.
Yes, that's clear. But that (missing) locking needs to be on a higher level
in the call chain, so the same lock fixes would fix scroll-redraw and hw
acceleration.
> Also the multi-user here means "you run untrusted code from other
> people", not "you run multiple things in parallel" or "multiple people
> are logged in at the same time".
>
>> Anyway, let's look at that part in your patch:
>>
>> + bool "Enable legacy fbcon code for hw acceleration"
>> + depends on FRAMEBUFFER_CONSOLE
>> + default n
>> + help
>> + Only enable this options if you are in full control of machine since
>> + the fbcon acceleration code is essentially unmaintained and known
>> + problematic.
>> +
>> + If unsure, select n.
>>
>> Since I'm willing to maintain that scrolling/panning code, I'd like to
>> drop the "is essentially unmaintained" part.
>> And the "known problematic" part is up to now just speculative (which would be
>> valid for other parts of the kernel too, btw).
>>
>> As this whole disussions showed, there are some few architectures/platforms
>> which really still need this acceleration, while others don't.
>> So, why not leave the decision up to the arch maintainers, which may opt-in
>> or opt-out, while keeping the default on "n"?
>>
>> With that, here is a new proposal:
>>
>> +config FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION
>> + bool "Enable legacy fbcon hardware acceleration code"
>> + depends on FRAMEBUFFER_CONSOLE
>> + default y if (PARISC) # a few other arches may want to be listed here too...
>> + default n
>> + help
>> + This option enables the fbcon (framebuffer text-based) hardware acceleration for
>> + graphics drivers which were written for the fbdev graphics interface.
>> +
>> + On modern machines, on mainstream machines (like x86-64) or when using a modern
>> + Linux distribution those fbdev drivers usually aren't used.
>> + So enabling this option wouldn't have any effect, which is why you want
>> + to disable this option on such newer machines.
>> +
>> + If you compile this kernel for older machines which still require the fbdev
>> + drivers, you may want to say Y.
>> +
>> + If unsure, select n.
>>
>> Would that be acceptable?
>
> Perfect.
Great!!
>> Arch maintainers may then later send (or reply now with) patches, e.g.:
>> + default y if (M68K && XYZ)
>> ...
>
> Yeah makes sense.
>
>>> Also you didn't keep any todo entry for removing DRM accel code,
>>
>> That wasn't intentional.
>> I just sent 2 revert-patches and an fbcon-accel-enabling-patch.
>> I'll look up what you had in your patch series and add it as seperate patch.
>
> tbh I think it's fine either way. I think it's still rather unclear
> which way drm will go, least because there's not many people who can
> occasionally squeeze some time away to move things forward. Could be
> that we add a new emergency logging console in the kernel for drm, and
> distros switch over to kmscon (which is in userspace, and accelerated
> iirc if you have gl, so most modern systems). Or whether we just
> improve fbcon until it's solid enough. Or some other option.
>
> So given that just dropping the todo sounds ok, it was just a bit
> inconsistent with the Kconfig :-)
Ok, I leave it out.
>>> and iirc some nouveau folks also complained that they at least
>>> once had some kind of accel, so that's another reason to not tie this
>>> to DRM.
>>
>> The above proposal to add additional "default y if XYZ" would enable
>> them to opt-in.
>>
>>> Anyway aside from this looks reasonable, can you pls respin with a
>>> more cautious Kconfig text?
>>
>> Sure, I'll do as soon as we have a decision on the above Kconfig text.
>
> Ideally if you can send them out today so it's not missing the
> drm-misc-fixes train that leaves tomorrow, that would be best.
Cool. I'll send the new series in a few minutes.
Thanks!
Helge
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 3/3] fbcon: Add option to enable legacy hardware acceleration
@ 2022-02-02 13:41 ` Helge Deller
0 siblings, 0 replies; 12+ messages in thread
From: Helge Deller @ 2022-02-02 13:41 UTC (permalink / raw)
To: Daniel Vetter; +Cc: linux-fbdev, DRI Development
On 2/2/22 12:05, Daniel Vetter wrote:
> On Tue, Feb 1, 2022 at 11:52 PM Helge Deller <deller@gmx.de> wrote:
>>
>> Hello Daniel,
>>
>> On 2/1/22 21:11, Daniel Vetter wrote:
>>> On Tue, Feb 1, 2022 at 7:59 PM Helge Deller <deller@gmx.de> wrote:
>>>>
>>>> Add a config option CONFIG_FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION to
>>>> enable bitblt and fillrect hardware acceleration in the framebuffer
>>>> console. If disabled, such acceleration will not be used, even if it is
>>>> supported by the graphics hardware driver.
>>>>
>>>> If you plan to use DRM as your main graphics output system, you should
>>>> disable this option since it will prevent compiling in code which isn't
>>>> used later on when DRM takes over.
>>>>
>>>> For all other configurations, e.g. if none of your graphic cards support
>>>> DRM (yet), DRM isn't available for your architecture, or you can't be
>>>> sure that the graphic card in the target system will support DRM, you
>>>> most likely want to enable this option.
>>>>
>>>> In the non-accelerated case (e.g. when DRM is used), the inlined
>>>> fb_scrollmode() function is hardcoded to return SCROLL_REDRAW and as such the
>>>> compiler is able to optimize much unneccesary code away.
>>>>
>>>> In this v3 patch version I additionally changed the GETVYRES() and GETVXRES()
>>>> macros to take a pointer to the fbcon_display struct. This fixes the build when
>>>> console rotation is enabled and helps the compiler again to optimize out code.
>>>>
>>>> Signed-off-by: Helge Deller <deller@gmx.de>
>>>> Cc: stable@vger.kernel.org # v5.10+
>>>> Signed-off-by: Helge Deller <deller@gmx.de>
>>>> ---
>>>> drivers/video/console/Kconfig | 11 +++++++
>>>> drivers/video/fbdev/core/fbcon.c | 39 ++++++++++++++++++-------
>>>> drivers/video/fbdev/core/fbcon.h | 15 +++++++++-
>>>> drivers/video/fbdev/core/fbcon_ccw.c | 10 +++----
>>>> drivers/video/fbdev/core/fbcon_cw.c | 10 +++----
>>>> drivers/video/fbdev/core/fbcon_rotate.h | 4 +--
>>>> drivers/video/fbdev/core/fbcon_ud.c | 20 ++++++-------
>>>> 7 files changed, 75 insertions(+), 34 deletions(-)
>>>>
>>>> diff --git a/drivers/video/console/Kconfig b/drivers/video/console/Kconfig
>>>> index 840d9813b0bc..6029fd41d7d0 100644
>>>> --- a/drivers/video/console/Kconfig
>>>> +++ b/drivers/video/console/Kconfig
>>>> @@ -78,6 +78,17 @@ config FRAMEBUFFER_CONSOLE
>>>> help
>>>> Low-level framebuffer-based console driver.
>>>>
>>>> +config FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION
>>>> + bool "Framebuffer Console hardware acceleration support"
>>>> + depends on FRAMEBUFFER_CONSOLE
>>>> + default n if DRM
>>>> + default y
>>>> + help
>>>> + If you use a system on which DRM is fully supported you usually want to say N,
>>>> + otherwise you probably want to enable this option.
>>>> + If enabled the framebuffer console will utilize the hardware acceleration
>>>> + of your graphics card by using hardware bitblt and fillrect features.
>>>
>>> This really doesn't have much to do with DRM but more about running
>>> questionable code. That's why I went with the generalized stern
>>> warning and default n, and explained when it's ok to do this (single
>>> user and you care more about fbcon performance than potential issues
>>> because you only run trusted stuff with access to your vt and fbdev
>>> /dev node).
>>
>> I think this is something we both will keep to have different opinion about :-)
>>
>> This console acceleration code is not exported or visible to userspace,
>> e.g. you can't access or trigger it via /dev/fb0.
>> It's only called internally from inside fbcon, so it's independed if
>> it's a single- or multi-user system.
>> The only way to "access" it is via standard stdio, where fbcon then
>> either scrolls the screen via redrawing characters at new positions
>> or by using hardware bitblt to move screen contents.
>> IMHO there is nothing which is critical here.
>> Even when I analyzed the existing bug reports, there was none which
>> affected that specific code.
>
> Maybe to clarify. The issues that generally result in this code going
> boom in syzbot are when you race console activity (which can be
> largely controlled through VT ioctls from userspace too, plus
> /dev/kmsg) against fbdev resizing on the /dev/fb/0 nodes. The locking
> there is kinda wild, and the code is very hard to understand. This is
> why we've resorted to just disabling/deleting this code because most
> cases we have no idea what's actually going on, aside from something
> is clearly not going right.
Yes, that's clear. But that (missing) locking needs to be on a higher level
in the call chain, so the same lock fixes would fix scroll-redraw and hw
acceleration.
> Also the multi-user here means "you run untrusted code from other
> people", not "you run multiple things in parallel" or "multiple people
> are logged in at the same time".
>
>> Anyway, let's look at that part in your patch:
>>
>> + bool "Enable legacy fbcon code for hw acceleration"
>> + depends on FRAMEBUFFER_CONSOLE
>> + default n
>> + help
>> + Only enable this options if you are in full control of machine since
>> + the fbcon acceleration code is essentially unmaintained and known
>> + problematic.
>> +
>> + If unsure, select n.
>>
>> Since I'm willing to maintain that scrolling/panning code, I'd like to
>> drop the "is essentially unmaintained" part.
>> And the "known problematic" part is up to now just speculative (which would be
>> valid for other parts of the kernel too, btw).
>>
>> As this whole disussions showed, there are some few architectures/platforms
>> which really still need this acceleration, while others don't.
>> So, why not leave the decision up to the arch maintainers, which may opt-in
>> or opt-out, while keeping the default on "n"?
>>
>> With that, here is a new proposal:
>>
>> +config FRAMEBUFFER_CONSOLE_LEGACY_ACCELERATION
>> + bool "Enable legacy fbcon hardware acceleration code"
>> + depends on FRAMEBUFFER_CONSOLE
>> + default y if (PARISC) # a few other arches may want to be listed here too...
>> + default n
>> + help
>> + This option enables the fbcon (framebuffer text-based) hardware acceleration for
>> + graphics drivers which were written for the fbdev graphics interface.
>> +
>> + On modern machines, on mainstream machines (like x86-64) or when using a modern
>> + Linux distribution those fbdev drivers usually aren't used.
>> + So enabling this option wouldn't have any effect, which is why you want
>> + to disable this option on such newer machines.
>> +
>> + If you compile this kernel for older machines which still require the fbdev
>> + drivers, you may want to say Y.
>> +
>> + If unsure, select n.
>>
>> Would that be acceptable?
>
> Perfect.
Great!!
>> Arch maintainers may then later send (or reply now with) patches, e.g.:
>> + default y if (M68K && XYZ)
>> ...
>
> Yeah makes sense.
>
>>> Also you didn't keep any todo entry for removing DRM accel code,
>>
>> That wasn't intentional.
>> I just sent 2 revert-patches and an fbcon-accel-enabling-patch.
>> I'll look up what you had in your patch series and add it as seperate patch.
>
> tbh I think it's fine either way. I think it's still rather unclear
> which way drm will go, least because there's not many people who can
> occasionally squeeze some time away to move things forward. Could be
> that we add a new emergency logging console in the kernel for drm, and
> distros switch over to kmscon (which is in userspace, and accelerated
> iirc if you have gl, so most modern systems). Or whether we just
> improve fbcon until it's solid enough. Or some other option.
>
> So given that just dropping the todo sounds ok, it was just a bit
> inconsistent with the Kconfig :-)
Ok, I leave it out.
>>> and iirc some nouveau folks also complained that they at least
>>> once had some kind of accel, so that's another reason to not tie this
>>> to DRM.
>>
>> The above proposal to add additional "default y if XYZ" would enable
>> them to opt-in.
>>
>>> Anyway aside from this looks reasonable, can you pls respin with a
>>> more cautious Kconfig text?
>>
>> Sure, I'll do as soon as we have a decision on the above Kconfig text.
>
> Ideally if you can send them out today so it's not missing the
> drm-misc-fixes train that leaves tomorrow, that would be best.
Cool. I'll send the new series in a few minutes.
Thanks!
Helge
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-02-02 13:41 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-01 18:59 [PATCH v3 0/3] Fix regression introduced by disabling accelerated scrolling in fbcon Helge Deller
2022-02-01 18:59 ` [PATCH v3 1/3] Revert "fbdev: Garbage collect fbdev scrolling acceleration, part 1 (from TODO list)" Helge Deller
2022-02-01 18:59 ` [PATCH v3 2/3] Revert "fbcon: Disable accelerated scrolling" Helge Deller
2022-02-01 18:59 ` [PATCH v3 3/3] fbcon: Add option to enable legacy hardware acceleration Helge Deller
2022-02-01 20:11 ` Daniel Vetter
2022-02-01 20:11 ` Daniel Vetter
2022-02-01 22:52 ` Helge Deller
2022-02-01 22:52 ` Helge Deller
2022-02-02 11:05 ` Daniel Vetter
2022-02-02 11:05 ` Daniel Vetter
2022-02-02 13:41 ` Helge Deller
2022-02-02 13:41 ` Helge Deller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.