| Message ID | 20220110005638.21599-2-andre.przywara@arm.com |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | video: improve UEFI experience on DM_VIDEO | expand |
On 1/10/22 01:56, Andre Przywara wrote: > So far the DM_VIDEO console is completely lacking any cursor, which makes > typing and correcting quite irritating. > > Add a simple cursor display by writing a SPACE glyph in the background > colour to the next character position on the screen. Any typed character > will naturally overwrite it, so we need to only explicitly clear it if > the next character will appear somewhere else (newline, backspace). Does this work with (non-monospaced) Truetype fonts? Moving backwards in Truetype is awkward. Best regards Heinrich > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > drivers/video/console_normal.c | 1 + > drivers/video/vidconsole-uclass.c | 42 +++++++++++++++++++++++++++++++ > include/video_console.h | 1 + > 3 files changed, 44 insertions(+) > > diff --git a/drivers/video/console_normal.c b/drivers/video/console_normal.c > index 04f022491e5..bfd3aab8d24 100644 > --- a/drivers/video/console_normal.c > +++ b/drivers/video/console_normal.c > @@ -160,6 +160,7 @@ static int console_normal_probe(struct udevice *dev) > vc_priv->y_charsize = VIDEO_FONT_HEIGHT; > vc_priv->cols = vid_priv->xsize / VIDEO_FONT_WIDTH; > vc_priv->rows = vid_priv->ysize / VIDEO_FONT_HEIGHT; > + vc_priv->cursor_visible = true; > > return 0; > } > diff --git a/drivers/video/vidconsole-uclass.c b/drivers/video/vidconsole-uclass.c > index f42db40d4cd..420fd86f9ac 100644 > --- a/drivers/video/vidconsole-uclass.c > +++ b/drivers/video/vidconsole-uclass.c > @@ -70,6 +70,26 @@ static int vidconsole_entry_start(struct udevice *dev) > return ops->entry_start(dev); > } > > +static void draw_cursor(struct udevice *dev, bool state) > +{ > + struct vidconsole_priv *priv = dev_get_uclass_priv(dev); > + struct video_priv *vid_priv = dev_get_uclass_priv(dev->parent); > + u32 tmp; > + > + if (!priv->cursor_visible) > + return; > + > + if (state) { > + tmp = vid_priv->colour_bg; > + vid_priv->colour_bg = vid_priv->colour_fg; > + } > + > + vidconsole_putc_xy(dev, priv->xcur_frac, priv->ycur, ' '); > + > + if (state) > + vid_priv->colour_bg = tmp; > +} > + > /* Move backwards one space */ > static int vidconsole_back(struct udevice *dev) > { > @@ -77,6 +97,8 @@ static int vidconsole_back(struct udevice *dev) > struct vidconsole_ops *ops = vidconsole_get_ops(dev); > int ret; > > + draw_cursor(dev, false); > + > if (ops->backspace) { > ret = ops->backspace(dev); > if (ret != -ENOSYS) > @@ -103,6 +125,8 @@ static void vidconsole_newline(struct udevice *dev) > const int rows = CONFIG_CONSOLE_SCROLL_LINES; > int i, ret; > > + draw_cursor(dev, false); > + > priv->xcur_frac = priv->xstart_frac; > priv->ycur += priv->y_charsize; > > @@ -342,6 +366,14 @@ static void vidconsole_escape_char(struct udevice *dev, char ch) > > break; > } > + case 'l': > + draw_cursor(dev, false); > + priv->cursor_visible = 0; > + break; > + case 'h': > + priv->cursor_visible = 1; > + draw_cursor(dev, true); > + break; > case 'J': { > int mode; > > @@ -516,6 +548,11 @@ int vidconsole_put_char(struct udevice *dev, char ch) > struct vidconsole_priv *priv = dev_get_uclass_priv(dev); > int ret; > > + /* > + * We don't need to clear the cursor since we are going to overwrite > + * that character anyway. > + */ > + > if (priv->escape) { > vidconsole_escape_char(dev, ch); > return 0; > @@ -530,6 +567,7 @@ int vidconsole_put_char(struct udevice *dev, char ch) > /* beep */ > break; > case '\r': > + draw_cursor(dev, false); > priv->xcur_frac = priv->xstart_frac; > break; > case '\n': > @@ -537,6 +575,7 @@ int vidconsole_put_char(struct udevice *dev, char ch) > vidconsole_entry_start(dev); > break; > case '\t': /* Tab (8 chars alignment) */ > + draw_cursor(dev, false); > priv->xcur_frac = ((priv->xcur_frac / priv->tab_width_frac) > + 1) * priv->tab_width_frac; > > @@ -554,6 +593,8 @@ int vidconsole_put_char(struct udevice *dev, char ch) > break; > } > > + draw_cursor(dev, true); > + > return 0; > } > > @@ -620,6 +661,7 @@ static int vidconsole_pre_probe(struct udevice *dev) > struct video_priv *vid_priv = dev_get_uclass_priv(vid); > > priv->xsize_frac = VID_TO_POS(vid_priv->xsize); > + priv->cursor_visible = false; > > return 0; > } > diff --git a/include/video_console.h b/include/video_console.h > index 06b798ef10c..a908f1412e8 100644 > --- a/include/video_console.h > +++ b/include/video_console.h > @@ -83,6 +83,7 @@ struct vidconsole_priv { > int escape_len; > int row_saved; > int col_saved; > + bool cursor_visible; > char escape_buf[32]; > }; >
On Mon, 10 Jan 2022 10:41:14 +0100 Heinrich Schuchardt <heinrich.schuchardt@canonical.com> wrote: Hi, > On 1/10/22 01:56, Andre Przywara wrote: > > So far the DM_VIDEO console is completely lacking any cursor, which makes > > typing and correcting quite irritating. > > > > Add a simple cursor display by writing a SPACE glyph in the background > > colour to the next character position on the screen. Any typed character > > will naturally overwrite it, so we need to only explicitly clear it if > > the next character will appear somewhere else (newline, backspace). > > Does this work with (non-monospaced) Truetype fonts? > Moving backwards in Truetype is awkward. I don't know, I need to try this again. I think I tried TrueType (before adding wide character bitmap support), but ran into some issues, might well be due to the proportional fonts only. In general I think *proper* cursor support can be tricky and tedious, so this version here is deliberately made simple, to have at least *some* cursor. Shall we enable this code only for monospace fonts (both TT and bitmap)? Cheers, Andre > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > --- > > drivers/video/console_normal.c | 1 + > > drivers/video/vidconsole-uclass.c | 42 +++++++++++++++++++++++++++++++ > > include/video_console.h | 1 + > > 3 files changed, 44 insertions(+) > > > > diff --git a/drivers/video/console_normal.c b/drivers/video/console_normal.c > > index 04f022491e5..bfd3aab8d24 100644 > > --- a/drivers/video/console_normal.c > > +++ b/drivers/video/console_normal.c > > @@ -160,6 +160,7 @@ static int console_normal_probe(struct udevice *dev) > > vc_priv->y_charsize = VIDEO_FONT_HEIGHT; > > vc_priv->cols = vid_priv->xsize / VIDEO_FONT_WIDTH; > > vc_priv->rows = vid_priv->ysize / VIDEO_FONT_HEIGHT; > > + vc_priv->cursor_visible = true; > > > > return 0; > > } > > diff --git a/drivers/video/vidconsole-uclass.c b/drivers/video/vidconsole-uclass.c > > index f42db40d4cd..420fd86f9ac 100644 > > --- a/drivers/video/vidconsole-uclass.c > > +++ b/drivers/video/vidconsole-uclass.c > > @@ -70,6 +70,26 @@ static int vidconsole_entry_start(struct udevice *dev) > > return ops->entry_start(dev); > > } > > > > +static void draw_cursor(struct udevice *dev, bool state) > > +{ > > + struct vidconsole_priv *priv = dev_get_uclass_priv(dev); > > + struct video_priv *vid_priv = dev_get_uclass_priv(dev->parent); > > + u32 tmp; > > + > > + if (!priv->cursor_visible) > > + return; > > + > > + if (state) { > > + tmp = vid_priv->colour_bg; > > + vid_priv->colour_bg = vid_priv->colour_fg; > > + } > > + > > + vidconsole_putc_xy(dev, priv->xcur_frac, priv->ycur, ' '); > > + > > + if (state) > > + vid_priv->colour_bg = tmp; > > +} > > + > > /* Move backwards one space */ > > static int vidconsole_back(struct udevice *dev) > > { > > @@ -77,6 +97,8 @@ static int vidconsole_back(struct udevice *dev) > > struct vidconsole_ops *ops = vidconsole_get_ops(dev); > > int ret; > > > > + draw_cursor(dev, false); > > + > > if (ops->backspace) { > > ret = ops->backspace(dev); > > if (ret != -ENOSYS) > > @@ -103,6 +125,8 @@ static void vidconsole_newline(struct udevice *dev) > > const int rows = CONFIG_CONSOLE_SCROLL_LINES; > > int i, ret; > > > > + draw_cursor(dev, false); > > + > > priv->xcur_frac = priv->xstart_frac; > > priv->ycur += priv->y_charsize; > > > > @@ -342,6 +366,14 @@ static void vidconsole_escape_char(struct udevice *dev, char ch) > > > > break; > > } > > + case 'l': > > + draw_cursor(dev, false); > > + priv->cursor_visible = 0; > > + break; > > + case 'h': > > + priv->cursor_visible = 1; > > + draw_cursor(dev, true); > > + break; > > case 'J': { > > int mode; > > > > @@ -516,6 +548,11 @@ int vidconsole_put_char(struct udevice *dev, char ch) > > struct vidconsole_priv *priv = dev_get_uclass_priv(dev); > > int ret; > > > > + /* > > + * We don't need to clear the cursor since we are going to overwrite > > + * that character anyway. > > + */ > > + > > if (priv->escape) { > > vidconsole_escape_char(dev, ch); > > return 0; > > @@ -530,6 +567,7 @@ int vidconsole_put_char(struct udevice *dev, char ch) > > /* beep */ > > break; > > case '\r': > > + draw_cursor(dev, false); > > priv->xcur_frac = priv->xstart_frac; > > break; > > case '\n': > > @@ -537,6 +575,7 @@ int vidconsole_put_char(struct udevice *dev, char ch) > > vidconsole_entry_start(dev); > > break; > > case '\t': /* Tab (8 chars alignment) */ > > + draw_cursor(dev, false); > > priv->xcur_frac = ((priv->xcur_frac / priv->tab_width_frac) > > + 1) * priv->tab_width_frac; > > > > @@ -554,6 +593,8 @@ int vidconsole_put_char(struct udevice *dev, char ch) > > break; > > } > > > > + draw_cursor(dev, true); > > + > > return 0; > > } > > > > @@ -620,6 +661,7 @@ static int vidconsole_pre_probe(struct udevice *dev) > > struct video_priv *vid_priv = dev_get_uclass_priv(vid); > > > > priv->xsize_frac = VID_TO_POS(vid_priv->xsize); > > + priv->cursor_visible = false; > > > > return 0; > > } > > diff --git a/include/video_console.h b/include/video_console.h > > index 06b798ef10c..a908f1412e8 100644 > > --- a/include/video_console.h > > +++ b/include/video_console.h > > @@ -83,6 +83,7 @@ struct vidconsole_priv { > > int escape_len; > > int row_saved; > > int col_saved; > > + bool cursor_visible; > > char escape_buf[32]; > > }; > > >
Hi Andre, On Sun, 9 Jan 2022 at 17:57, Andre Przywara <andre.przywara@arm.com> wrote: > > So far the DM_VIDEO console is completely lacking any cursor, which makes > typing and correcting quite irritating. > > Add a simple cursor display by writing a SPACE glyph in the background > colour to the next character position on the screen. Any typed character > will naturally overwrite it, so we need to only explicitly clear it if > the next character will appear somewhere else (newline, backspace). > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > drivers/video/console_normal.c | 1 + > drivers/video/vidconsole-uclass.c | 42 +++++++++++++++++++++++++++++++ > include/video_console.h | 1 + > 3 files changed, 44 insertions(+) See console_truetype_backspace() for how backspace works with truetype fonts. For the normal console: Reviewed-by: Simon Glass <sjg@chromium.org> Tested on: sandbox Tested-by: Simon Glass <sjg@chromium.org> Please see nit below > > diff --git a/drivers/video/console_normal.c b/drivers/video/console_normal.c > index 04f022491e5..bfd3aab8d24 100644 > --- a/drivers/video/console_normal.c > +++ b/drivers/video/console_normal.c > @@ -160,6 +160,7 @@ static int console_normal_probe(struct udevice *dev) > vc_priv->y_charsize = VIDEO_FONT_HEIGHT; > vc_priv->cols = vid_priv->xsize / VIDEO_FONT_WIDTH; > vc_priv->rows = vid_priv->ysize / VIDEO_FONT_HEIGHT; > + vc_priv->cursor_visible = true; > > return 0; > } > diff --git a/drivers/video/vidconsole-uclass.c b/drivers/video/vidconsole-uclass.c > index f42db40d4cd..420fd86f9ac 100644 > --- a/drivers/video/vidconsole-uclass.c > +++ b/drivers/video/vidconsole-uclass.c > @@ -70,6 +70,26 @@ static int vidconsole_entry_start(struct udevice *dev) > return ops->entry_start(dev); > } > > +static void draw_cursor(struct udevice *dev, bool state) Please add function comment > +{ > + struct vidconsole_priv *priv = dev_get_uclass_priv(dev); > + struct video_priv *vid_priv = dev_get_uclass_priv(dev->parent); > + u32 tmp; > + > + if (!priv->cursor_visible) > + return; > + > + if (state) { > + tmp = vid_priv->colour_bg; > + vid_priv->colour_bg = vid_priv->colour_fg; > + } > + > + vidconsole_putc_xy(dev, priv->xcur_frac, priv->ycur, ' '); > + > + if (state) > + vid_priv->colour_bg = tmp; > +} > + > /* Move backwards one space */ > static int vidconsole_back(struct udevice *dev) > { > @@ -77,6 +97,8 @@ static int vidconsole_back(struct udevice *dev) > struct vidconsole_ops *ops = vidconsole_get_ops(dev); > int ret; > > + draw_cursor(dev, false); > + > if (ops->backspace) { > ret = ops->backspace(dev); > if (ret != -ENOSYS) > @@ -103,6 +125,8 @@ static void vidconsole_newline(struct udevice *dev) > const int rows = CONFIG_CONSOLE_SCROLL_LINES; > int i, ret; > > + draw_cursor(dev, false); > + > priv->xcur_frac = priv->xstart_frac; > priv->ycur += priv->y_charsize; > > @@ -342,6 +366,14 @@ static void vidconsole_escape_char(struct udevice *dev, char ch) > > break; > } > + case 'l': > + draw_cursor(dev, false); > + priv->cursor_visible = 0; > + break; > + case 'h': > + priv->cursor_visible = 1; > + draw_cursor(dev, true); > + break; > case 'J': { > int mode; > > @@ -516,6 +548,11 @@ int vidconsole_put_char(struct udevice *dev, char ch) > struct vidconsole_priv *priv = dev_get_uclass_priv(dev); > int ret; > > + /* > + * We don't need to clear the cursor since we are going to overwrite > + * that character anyway. > + */ > + > if (priv->escape) { > vidconsole_escape_char(dev, ch); > return 0; > @@ -530,6 +567,7 @@ int vidconsole_put_char(struct udevice *dev, char ch) > /* beep */ > break; > case '\r': > + draw_cursor(dev, false); > priv->xcur_frac = priv->xstart_frac; > break; > case '\n': > @@ -537,6 +575,7 @@ int vidconsole_put_char(struct udevice *dev, char ch) > vidconsole_entry_start(dev); > break; > case '\t': /* Tab (8 chars alignment) */ > + draw_cursor(dev, false); > priv->xcur_frac = ((priv->xcur_frac / priv->tab_width_frac) > + 1) * priv->tab_width_frac; > > @@ -554,6 +593,8 @@ int vidconsole_put_char(struct udevice *dev, char ch) > break; > } > > + draw_cursor(dev, true); > + > return 0; > } > > @@ -620,6 +661,7 @@ static int vidconsole_pre_probe(struct udevice *dev) > struct video_priv *vid_priv = dev_get_uclass_priv(vid); > > priv->xsize_frac = VID_TO_POS(vid_priv->xsize); > + priv->cursor_visible = false; > > return 0; > } > diff --git a/include/video_console.h b/include/video_console.h > index 06b798ef10c..a908f1412e8 100644 > --- a/include/video_console.h > +++ b/include/video_console.h > @@ -83,6 +83,7 @@ struct vidconsole_priv { > int escape_len; > int row_saved; > int col_saved; > + bool cursor_visible; Please add to struct comment > char escape_buf[32]; > }; > > -- > 2.17.6 > Regards, Simon
diff --git a/drivers/video/console_normal.c b/drivers/video/console_normal.c index 04f022491e5..bfd3aab8d24 100644 --- a/drivers/video/console_normal.c +++ b/drivers/video/console_normal.c @@ -160,6 +160,7 @@ static int console_normal_probe(struct udevice *dev) vc_priv->y_charsize = VIDEO_FONT_HEIGHT; vc_priv->cols = vid_priv->xsize / VIDEO_FONT_WIDTH; vc_priv->rows = vid_priv->ysize / VIDEO_FONT_HEIGHT; + vc_priv->cursor_visible = true; return 0; } diff --git a/drivers/video/vidconsole-uclass.c b/drivers/video/vidconsole-uclass.c index f42db40d4cd..420fd86f9ac 100644 --- a/drivers/video/vidconsole-uclass.c +++ b/drivers/video/vidconsole-uclass.c @@ -70,6 +70,26 @@ static int vidconsole_entry_start(struct udevice *dev) return ops->entry_start(dev); } +static void draw_cursor(struct udevice *dev, bool state) +{ + struct vidconsole_priv *priv = dev_get_uclass_priv(dev); + struct video_priv *vid_priv = dev_get_uclass_priv(dev->parent); + u32 tmp; + + if (!priv->cursor_visible) + return; + + if (state) { + tmp = vid_priv->colour_bg; + vid_priv->colour_bg = vid_priv->colour_fg; + } + + vidconsole_putc_xy(dev, priv->xcur_frac, priv->ycur, ' '); + + if (state) + vid_priv->colour_bg = tmp; +} + /* Move backwards one space */ static int vidconsole_back(struct udevice *dev) { @@ -77,6 +97,8 @@ static int vidconsole_back(struct udevice *dev) struct vidconsole_ops *ops = vidconsole_get_ops(dev); int ret; + draw_cursor(dev, false); + if (ops->backspace) { ret = ops->backspace(dev); if (ret != -ENOSYS) @@ -103,6 +125,8 @@ static void vidconsole_newline(struct udevice *dev) const int rows = CONFIG_CONSOLE_SCROLL_LINES; int i, ret; + draw_cursor(dev, false); + priv->xcur_frac = priv->xstart_frac; priv->ycur += priv->y_charsize; @@ -342,6 +366,14 @@ static void vidconsole_escape_char(struct udevice *dev, char ch) break; } + case 'l': + draw_cursor(dev, false); + priv->cursor_visible = 0; + break; + case 'h': + priv->cursor_visible = 1; + draw_cursor(dev, true); + break; case 'J': { int mode; @@ -516,6 +548,11 @@ int vidconsole_put_char(struct udevice *dev, char ch) struct vidconsole_priv *priv = dev_get_uclass_priv(dev); int ret; + /* + * We don't need to clear the cursor since we are going to overwrite + * that character anyway. + */ + if (priv->escape) { vidconsole_escape_char(dev, ch); return 0; @@ -530,6 +567,7 @@ int vidconsole_put_char(struct udevice *dev, char ch) /* beep */ break; case '\r': + draw_cursor(dev, false); priv->xcur_frac = priv->xstart_frac; break; case '\n': @@ -537,6 +575,7 @@ int vidconsole_put_char(struct udevice *dev, char ch) vidconsole_entry_start(dev); break; case '\t': /* Tab (8 chars alignment) */ + draw_cursor(dev, false); priv->xcur_frac = ((priv->xcur_frac / priv->tab_width_frac) + 1) * priv->tab_width_frac; @@ -554,6 +593,8 @@ int vidconsole_put_char(struct udevice *dev, char ch) break; } + draw_cursor(dev, true); + return 0; } @@ -620,6 +661,7 @@ static int vidconsole_pre_probe(struct udevice *dev) struct video_priv *vid_priv = dev_get_uclass_priv(vid); priv->xsize_frac = VID_TO_POS(vid_priv->xsize); + priv->cursor_visible = false; return 0; } diff --git a/include/video_console.h b/include/video_console.h index 06b798ef10c..a908f1412e8 100644 --- a/include/video_console.h +++ b/include/video_console.h @@ -83,6 +83,7 @@ struct vidconsole_priv { int escape_len; int row_saved; int col_saved; + bool cursor_visible; char escape_buf[32]; };
So far the DM_VIDEO console is completely lacking any cursor, which makes typing and correcting quite irritating. Add a simple cursor display by writing a SPACE glyph in the background colour to the next character position on the screen. Any typed character will naturally overwrite it, so we need to only explicitly clear it if the next character will appear somewhere else (newline, backspace). Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- drivers/video/console_normal.c | 1 + drivers/video/vidconsole-uclass.c | 42 +++++++++++++++++++++++++++++++ include/video_console.h | 1 + 3 files changed, 44 insertions(+)