All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH] dm: video: Add basic ANSI escape sequence support
@ 2017-09-07 20:28 Rob Clark
  2017-09-07 20:28 ` [U-Boot] [PATCH] dm: video: Fix cache flushes Rob Clark
  2017-09-11  6:18 ` [U-Boot] [PATCH] dm: video: Add basic ANSI escape sequence support Simon Glass
  0 siblings, 2 replies; 8+ messages in thread
From: Rob Clark @ 2017-09-07 20:28 UTC (permalink / raw)
  To: u-boot

Really just the subset that is needed by efi_console.  Perhaps more will
be added later, for example color support would be useful to implement
efi_cout_set_attribute().

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/video/vidconsole-uclass.c | 112 ++++++++++++++++++++++++++++++++++++++
 drivers/video/video-uclass.c      |   4 +-
 include/video.h                   |   7 +++
 include/video_console.h           |  11 ++++
 4 files changed, 131 insertions(+), 3 deletions(-)

diff --git a/drivers/video/vidconsole-uclass.c b/drivers/video/vidconsole-uclass.c
index e081d5a0ee..7998b4cf5f 100644
--- a/drivers/video/vidconsole-uclass.c
+++ b/drivers/video/vidconsole-uclass.c
@@ -9,6 +9,7 @@
  */
 
 #include <common.h>
+#include <linux/ctype.h>
 #include <dm.h>
 #include <video.h>
 #include <video_console.h>
@@ -107,12 +108,123 @@ static void vidconsole_newline(struct udevice *dev)
 	video_sync(dev->parent);
 }
 
+/*
+ * Parse a number from string that ends in a non-numeric character..
+ * sscanf() would be nice.  This is just enough for parsing ANSI escape
+ * sequences.
+ */
+static char *parsenum(char *s, int *num)
+{
+	int n = 0;
+
+	while (isdigit(*s)) {
+		n = (10 * n) + (*s - '0');
+		s++;
+	}
+
+	*num = n;
+	return s;
+}
+
+static void vidconsole_escape_char(struct udevice *dev, char ch)
+{
+	struct vidconsole_priv *priv = dev_get_uclass_priv(dev);
+
+	/* Sanity checking for bogus ESC sequences: */
+	if (priv->escape_len >= sizeof(priv->escape_buf))
+		goto error;
+	if (priv->escape_len == 0 && ch != '[')
+		goto error;
+
+	priv->escape_buf[priv->escape_len++] = ch;
+
+	/*
+	 * Escape sequences are terminated by a letter, so keep
+	 * accumulating until we get one:
+	 */
+	if (!isalpha(ch))
+		return;
+
+	/*
+	 * clear escape mode first, otherwise things will get highly
+	 * surprising if you hit any debug prints that come back to
+	 * this console.
+	 */
+	priv->escape = 0;
+
+	switch (ch) {
+	case 'H':
+	case 'f': {
+		int row, col;
+		char *s = priv->escape_buf;
+
+		/*
+		 * Set cursor position: [%d;%df or [%d;%dH
+		 */
+		s++;    /* [ */
+		s = parsenum(s, &row);
+		s++;    /* ; */
+		s = parsenum(s, &col);
+
+		priv->ycur = row * priv->y_charsize;
+		priv->xcur_frac = priv->xstart_frac +
+			VID_TO_POS(col * priv->x_charsize);
+
+		break;
+	}
+	case 'J': {
+		int mode;
+
+		/*
+		 * Clear part/all screen:
+		 *   [J or [0J - clear screen from cursor down
+		 *   [1J       - clear screen from cursor up
+		 *   [2J       - clear entire screen
+		 *
+		 * TODO we really only handle entire-screen case, others
+		 * probably require some additions to video-uclass (and
+		 * are not really needed yet by efi_console)
+		 */
+		parsenum(priv->escape_buf + 1, &mode);
+
+		if (mode == 2) {
+			video_clear(dev->parent);
+			video_sync(dev->parent);
+			priv->ycur = 0;
+			priv->xcur_frac = priv->xstart_frac;
+		} else {
+			debug("unsupported clear mode: %d\n", mode);
+		}
+		break;
+	}
+	default:
+		debug("unrecognized escape sequence: %*s\n",
+		      priv->escape_len, priv->escape_buf);
+	}
+
+	return;
+
+error:
+	/* something went wrong, just revert to normal mode: */
+	priv->escape = 0;
+	return;
+}
+
 int vidconsole_put_char(struct udevice *dev, char ch)
 {
 	struct vidconsole_priv *priv = dev_get_uclass_priv(dev);
 	int ret;
 
+	if (priv->escape) {
+		vidconsole_escape_char(dev, ch);
+		return 0;
+	}
+
 	switch (ch) {
+	case '\x1b':
+		priv->escape_len = 0;
+		priv->escape = 1;
+		break;
 	case '\a':
 		/* beep */
 		break;
diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
index dfa39b0d1b..dcaceed42c 100644
--- a/drivers/video/video-uclass.c
+++ b/drivers/video/video-uclass.c
@@ -87,7 +87,7 @@ int video_reserve(ulong *addrp)
 	return 0;
 }
 
-static int video_clear(struct udevice *dev)
+void video_clear(struct udevice *dev)
 {
 	struct video_priv *priv = dev_get_uclass_priv(dev);
 
@@ -100,8 +100,6 @@ static int video_clear(struct udevice *dev)
 	} else {
 		memset(priv->fb, priv->colour_bg, priv->fb_size);
 	}
-
-	return 0;
 }
 
 /* Flush video activity to the caches */
diff --git a/include/video.h b/include/video.h
index 5b4e78b182..61ff653121 100644
--- a/include/video.h
+++ b/include/video.h
@@ -115,6 +115,13 @@ struct video_ops {
 int video_reserve(ulong *addrp);
 
 /**
+ * video_clear() - Clear a device's frame buffer to background color.
+ *
+ * @dev:	Device to clear
+ */
+void video_clear(struct udevice *dev);
+
+/**
  * video_sync() - Sync a device's frame buffer with its hardware
  *
  * Some frame buffers are cached or have a secondary frame buffer. This
diff --git a/include/video_console.h b/include/video_console.h
index 26047934da..9dce234bd9 100644
--- a/include/video_console.h
+++ b/include/video_console.h
@@ -29,6 +29,9 @@
  * @xsize_frac:	Width of the display in fractional units
  * @xstart_frac:	Left margin for the text console in fractional units
  * @last_ch:	Last character written to the text console on this line
+ * @escape:	TRUE if currently accumulating an ANSI escape sequence
+ * @escape_len:	Length of accumulated escape sequence so far
+ * @escape_buf:	Buffer to accumulate escape sequence
  */
 struct vidconsole_priv {
 	struct stdio_dev sdev;
@@ -42,6 +45,14 @@ struct vidconsole_priv {
 	int xsize_frac;
 	int xstart_frac;
 	int last_ch;
+	/*
+	 * ANSI escape sequences are accumulated character by character,
+	 * starting after the ESC char (0x1b) until the entire sequence
+	 * is consumed at which point it is acted upon.
+	 */
+	int escape;
+	int escape_len;
+	char escape_buf[32];
 };
 
 /**
-- 
2.13.5

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [U-Boot] [PATCH] dm: video: Fix cache flushes
  2017-09-07 20:28 [U-Boot] [PATCH] dm: video: Add basic ANSI escape sequence support Rob Clark
@ 2017-09-07 20:28 ` Rob Clark
  2017-09-11  6:18 ` [U-Boot] [PATCH] dm: video: Add basic ANSI escape sequence support Simon Glass
  1 sibling, 0 replies; 8+ messages in thread
From: Rob Clark @ 2017-09-07 20:28 UTC (permalink / raw)
  To: u-boot

Content can come to screen via putc() and we cannot always rely on
updates ending with a puts().  This is the case with efi_console output
to vidconsole.  Fixes corruption with Shell.efi.

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/video/vidconsole-uclass.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/video/vidconsole-uclass.c b/drivers/video/vidconsole-uclass.c
index b5afd72227..e081d5a0ee 100644
--- a/drivers/video/vidconsole-uclass.c
+++ b/drivers/video/vidconsole-uclass.c
@@ -163,6 +163,7 @@ static void vidconsole_putc(struct stdio_dev *sdev, const char ch)
 	struct udevice *dev = sdev->priv;
 
 	vidconsole_put_char(dev, ch);
+	video_sync(dev->parent);
 }
 
 static void vidconsole_puts(struct stdio_dev *sdev, const char *s)
@@ -260,6 +261,8 @@ static int do_video_puts(cmd_tbl_t *cmdtp, int flag, int argc,
 	for (s = argv[1]; *s; s++)
 		vidconsole_put_char(dev, *s);
 
+	video_sync(dev->parent);
+
 	return 0;
 }
 
-- 
2.13.5

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [U-Boot] [PATCH] dm: video: Add basic ANSI escape sequence support
  2017-09-07 20:28 [U-Boot] [PATCH] dm: video: Add basic ANSI escape sequence support Rob Clark
  2017-09-07 20:28 ` [U-Boot] [PATCH] dm: video: Fix cache flushes Rob Clark
@ 2017-09-11  6:18 ` Simon Glass
  2017-09-11  9:42   ` Rob Clark
  1 sibling, 1 reply; 8+ messages in thread
From: Simon Glass @ 2017-09-11  6:18 UTC (permalink / raw)
  To: u-boot

On 7 September 2017 at 14:28, Rob Clark <robdclark@gmail.com> wrote:
> Really just the subset that is needed by efi_console.  Perhaps more will
> be added later, for example color support would be useful to implement
> efi_cout_set_attribute().
>
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
>  drivers/video/vidconsole-uclass.c | 112 ++++++++++++++++++++++++++++++++++++++
>  drivers/video/video-uclass.c      |   4 +-
>  include/video.h                   |   7 +++
>  include/video_console.h           |  11 ++++
>  4 files changed, 131 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/video/vidconsole-uclass.c b/drivers/video/vidconsole-uclass.c
> index e081d5a0ee..7998b4cf5f 100644
> --- a/drivers/video/vidconsole-uclass.c
> +++ b/drivers/video/vidconsole-uclass.c
> @@ -9,6 +9,7 @@
>   */
>
>  #include <common.h>
> +#include <linux/ctype.h>
>  #include <dm.h>
>  #include <video.h>
>  #include <video_console.h>
> @@ -107,12 +108,123 @@ static void vidconsole_newline(struct udevice *dev)
>         video_sync(dev->parent);
>  }
>
> +/*
> + * Parse a number from string that ends in a non-numeric character..
> + * sscanf() would be nice.  This is just enough for parsing ANSI escape
> + * sequences.
> + */
> +static char *parsenum(char *s, int *num)

Can you use simple_strtoul() or similar?

> +{
> +       int n = 0;
> +
> +       while (isdigit(*s)) {
> +               n = (10 * n) + (*s - '0');
> +               s++;
> +       }
> +
> +       *num = n;
> +       return s;
> +}
> +
> +static void vidconsole_escape_char(struct udevice *dev, char ch)

Please add a function comment

> +{
> +       struct vidconsole_priv *priv = dev_get_uclass_priv(dev);
> +
> +       /* Sanity checking for bogus ESC sequences: */
> +       if (priv->escape_len >= sizeof(priv->escape_buf))
> +               goto error;
> +       if (priv->escape_len == 0 && ch != '[')
> +               goto error;
> +
> +       priv->escape_buf[priv->escape_len++] = ch;
> +
> +       /*
> +        * Escape sequences are terminated by a letter, so keep
> +        * accumulating until we get one:
> +        */
> +       if (!isalpha(ch))
> +               return;
> +
> +       /*
> +        * clear escape mode first, otherwise things will get highly
> +        * surprising if you hit any debug prints that come back to
> +        * this console.
> +        */
> +       priv->escape = 0;
> +
> +       switch (ch) {
> +       case 'H':
> +       case 'f': {
> +               int row, col;
> +               char *s = priv->escape_buf;
> +
> +               /*
> +                * Set cursor position: [%d;%df or [%d;%dH
> +                */
> +               s++;    /* [ */
> +               s = parsenum(s, &row);
> +               s++;    /* ; */
> +               s = parsenum(s, &col);
> +
> +               priv->ycur = row * priv->y_charsize;
> +               priv->xcur_frac = priv->xstart_frac +
> +                       VID_TO_POS(col * priv->x_charsize);
> +
> +               break;
> +       }
> +       case 'J': {
> +               int mode;
> +
> +               /*
> +                * Clear part/all screen:
> +                *   [J or [0J - clear screen from cursor down
> +                *   [1J       - clear screen from cursor up
> +                *   [2J       - clear entire screen
> +                *
> +                * TODO we really only handle entire-screen case, others
> +                * probably require some additions to video-uclass (and
> +                * are not really needed yet by efi_console)
> +                */
> +               parsenum(priv->escape_buf + 1, &mode);
> +
> +               if (mode == 2) {
> +                       video_clear(dev->parent);
> +                       video_sync(dev->parent);
> +                       priv->ycur = 0;
> +                       priv->xcur_frac = priv->xstart_frac;
> +               } else {
> +                       debug("unsupported clear mode: %d\n", mode);
> +               }
> +               break;
> +       }
> +       default:
> +               debug("unrecognized escape sequence: %*s\n",
> +                     priv->escape_len, priv->escape_buf);
> +       }
> +
> +       return;
> +
> +error:
> +       /* something went wrong, just revert to normal mode: */
> +       priv->escape = 0;
> +       return;
> +}
> +
>  int vidconsole_put_char(struct udevice *dev, char ch)
>  {
>         struct vidconsole_priv *priv = dev_get_uclass_priv(dev);
>         int ret;
>
> +       if (priv->escape) {
> +               vidconsole_escape_char(dev, ch);
> +               return 0;
> +       }

Is it possible to add a CONFIG_VIDEO_ANSI option to enable this?
Perhaps it could be on by default. Then:

if (CONFIG_IS_ENABLED(VIDEO_ANSI) && priv-escape) {
...

This helps to reduce base code size.

> +
>         switch (ch) {
> +       case '\x1b':
> +               priv->escape_len = 0;
> +               priv->escape = 1;
> +               break;
>         case '\a':
>                 /* beep */
>                 break;
> diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
> index dfa39b0d1b..dcaceed42c 100644
> --- a/drivers/video/video-uclass.c
> +++ b/drivers/video/video-uclass.c
> @@ -87,7 +87,7 @@ int video_reserve(ulong *addrp)
>         return 0;
>  }
>
> -static int video_clear(struct udevice *dev)
> +void video_clear(struct udevice *dev)
>  {
>         struct video_priv *priv = dev_get_uclass_priv(dev);
>
> @@ -100,8 +100,6 @@ static int video_clear(struct udevice *dev)
>         } else {
>                 memset(priv->fb, priv->colour_bg, priv->fb_size);
>         }
> -
> -       return 0;
>  }
>
>  /* Flush video activity to the caches */
> diff --git a/include/video.h b/include/video.h
> index 5b4e78b182..61ff653121 100644
> --- a/include/video.h
> +++ b/include/video.h
> @@ -115,6 +115,13 @@ struct video_ops {
>  int video_reserve(ulong *addrp);
>
>  /**
> + * video_clear() - Clear a device's frame buffer to background color.
> + *
> + * @dev:       Device to clear
> + */
> +void video_clear(struct udevice *dev);
> +
> +/**
>   * video_sync() - Sync a device's frame buffer with its hardware
>   *
>   * Some frame buffers are cached or have a secondary frame buffer. This
> diff --git a/include/video_console.h b/include/video_console.h
> index 26047934da..9dce234bd9 100644
> --- a/include/video_console.h
> +++ b/include/video_console.h
> @@ -29,6 +29,9 @@
>   * @xsize_frac:        Width of the display in fractional units
>   * @xstart_frac:       Left margin for the text console in fractional units
>   * @last_ch:   Last character written to the text console on this line
> + * @escape:    TRUE if currently accumulating an ANSI escape sequence
> + * @escape_len:        Length of accumulated escape sequence so far
> + * @escape_buf:        Buffer to accumulate escape sequence
>   */
>  struct vidconsole_priv {
>         struct stdio_dev sdev;
> @@ -42,6 +45,14 @@ struct vidconsole_priv {
>         int xsize_frac;
>         int xstart_frac;
>         int last_ch;
> +       /*
> +        * ANSI escape sequences are accumulated character by character,
> +        * starting after the ESC char (0x1b) until the entire sequence
> +        * is consumed at which point it is acted upon.
> +        */
> +       int escape;
> +       int escape_len;
> +       char escape_buf[32];
>  };
>
>  /**
> --
> 2.13.5
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [U-Boot] [PATCH] dm: video: Add basic ANSI escape sequence support
  2017-09-11  6:18 ` [U-Boot] [PATCH] dm: video: Add basic ANSI escape sequence support Simon Glass
@ 2017-09-11  9:42   ` Rob Clark
  2017-09-11 11:50     ` Lothar Waßmann
  0 siblings, 1 reply; 8+ messages in thread
From: Rob Clark @ 2017-09-11  9:42 UTC (permalink / raw)
  To: u-boot

On Mon, Sep 11, 2017 at 2:18 AM, Simon Glass <sjg@chromium.org> wrote:
> On 7 September 2017 at 14:28, Rob Clark <robdclark@gmail.com> wrote:
>> Really just the subset that is needed by efi_console.  Perhaps more will
>> be added later, for example color support would be useful to implement
>> efi_cout_set_attribute().
>>
>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>> ---
>>  drivers/video/vidconsole-uclass.c | 112 ++++++++++++++++++++++++++++++++++++++
>>  drivers/video/video-uclass.c      |   4 +-
>>  include/video.h                   |   7 +++
>>  include/video_console.h           |  11 ++++
>>  4 files changed, 131 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/video/vidconsole-uclass.c b/drivers/video/vidconsole-uclass.c
>> index e081d5a0ee..7998b4cf5f 100644
>> --- a/drivers/video/vidconsole-uclass.c
>> +++ b/drivers/video/vidconsole-uclass.c
>> @@ -9,6 +9,7 @@
>>   */
>>
>>  #include <common.h>
>> +#include <linux/ctype.h>
>>  #include <dm.h>
>>  #include <video.h>
>>  #include <video_console.h>
>> @@ -107,12 +108,123 @@ static void vidconsole_newline(struct udevice *dev)
>>         video_sync(dev->parent);
>>  }
>>
>> +/*
>> + * Parse a number from string that ends in a non-numeric character..
>> + * sscanf() would be nice.  This is just enough for parsing ANSI escape
>> + * sequences.
>> + */
>> +static char *parsenum(char *s, int *num)
>
> Can you use simple_strtoul() or similar?

Possibly, but I'm not sure it is a good idea.. I don't think escape
sequences are meant to be encoded with hex or octal number strings.
From a quick look, I don't see any escape code terminated with 'x', so
maybe it would end up working ok.. but something like ESC[0x1234m
should be an escape sequence terminated with x followed by normal
chars 1234m and strtoul would get that wrong..

BR,
-R

>> +{
>> +       int n = 0;
>> +
>> +       while (isdigit(*s)) {
>> +               n = (10 * n) + (*s - '0');
>> +               s++;
>> +       }
>> +
>> +       *num = n;
>> +       return s;
>> +}
>> +
>> +static void vidconsole_escape_char(struct udevice *dev, char ch)
>
> Please add a function comment
>
>> +{
>> +       struct vidconsole_priv *priv = dev_get_uclass_priv(dev);
>> +
>> +       /* Sanity checking for bogus ESC sequences: */
>> +       if (priv->escape_len >= sizeof(priv->escape_buf))
>> +               goto error;
>> +       if (priv->escape_len == 0 && ch != '[')
>> +               goto error;
>> +
>> +       priv->escape_buf[priv->escape_len++] = ch;
>> +
>> +       /*
>> +        * Escape sequences are terminated by a letter, so keep
>> +        * accumulating until we get one:
>> +        */
>> +       if (!isalpha(ch))
>> +               return;
>> +
>> +       /*
>> +        * clear escape mode first, otherwise things will get highly
>> +        * surprising if you hit any debug prints that come back to
>> +        * this console.
>> +        */
>> +       priv->escape = 0;
>> +
>> +       switch (ch) {
>> +       case 'H':
>> +       case 'f': {
>> +               int row, col;
>> +               char *s = priv->escape_buf;
>> +
>> +               /*
>> +                * Set cursor position: [%d;%df or [%d;%dH
>> +                */
>> +               s++;    /* [ */
>> +               s = parsenum(s, &row);
>> +               s++;    /* ; */
>> +               s = parsenum(s, &col);
>> +
>> +               priv->ycur = row * priv->y_charsize;
>> +               priv->xcur_frac = priv->xstart_frac +
>> +                       VID_TO_POS(col * priv->x_charsize);
>> +
>> +               break;
>> +       }
>> +       case 'J': {
>> +               int mode;
>> +
>> +               /*
>> +                * Clear part/all screen:
>> +                *   [J or [0J - clear screen from cursor down
>> +                *   [1J       - clear screen from cursor up
>> +                *   [2J       - clear entire screen
>> +                *
>> +                * TODO we really only handle entire-screen case, others
>> +                * probably require some additions to video-uclass (and
>> +                * are not really needed yet by efi_console)
>> +                */
>> +               parsenum(priv->escape_buf + 1, &mode);
>> +
>> +               if (mode == 2) {
>> +                       video_clear(dev->parent);
>> +                       video_sync(dev->parent);
>> +                       priv->ycur = 0;
>> +                       priv->xcur_frac = priv->xstart_frac;
>> +               } else {
>> +                       debug("unsupported clear mode: %d\n", mode);
>> +               }
>> +               break;
>> +       }
>> +       default:
>> +               debug("unrecognized escape sequence: %*s\n",
>> +                     priv->escape_len, priv->escape_buf);
>> +       }
>> +
>> +       return;
>> +
>> +error:
>> +       /* something went wrong, just revert to normal mode: */
>> +       priv->escape = 0;
>> +       return;
>> +}
>> +
>>  int vidconsole_put_char(struct udevice *dev, char ch)
>>  {
>>         struct vidconsole_priv *priv = dev_get_uclass_priv(dev);
>>         int ret;
>>
>> +       if (priv->escape) {
>> +               vidconsole_escape_char(dev, ch);
>> +               return 0;
>> +       }
>
> Is it possible to add a CONFIG_VIDEO_ANSI option to enable this?
> Perhaps it could be on by default. Then:
>
> if (CONFIG_IS_ENABLED(VIDEO_ANSI) && priv-escape) {
> ...
>
> This helps to reduce base code size.
>
>> +
>>         switch (ch) {
>> +       case '\x1b':
>> +               priv->escape_len = 0;
>> +               priv->escape = 1;
>> +               break;
>>         case '\a':
>>                 /* beep */
>>                 break;
>> diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
>> index dfa39b0d1b..dcaceed42c 100644
>> --- a/drivers/video/video-uclass.c
>> +++ b/drivers/video/video-uclass.c
>> @@ -87,7 +87,7 @@ int video_reserve(ulong *addrp)
>>         return 0;
>>  }
>>
>> -static int video_clear(struct udevice *dev)
>> +void video_clear(struct udevice *dev)
>>  {
>>         struct video_priv *priv = dev_get_uclass_priv(dev);
>>
>> @@ -100,8 +100,6 @@ static int video_clear(struct udevice *dev)
>>         } else {
>>                 memset(priv->fb, priv->colour_bg, priv->fb_size);
>>         }
>> -
>> -       return 0;
>>  }
>>
>>  /* Flush video activity to the caches */
>> diff --git a/include/video.h b/include/video.h
>> index 5b4e78b182..61ff653121 100644
>> --- a/include/video.h
>> +++ b/include/video.h
>> @@ -115,6 +115,13 @@ struct video_ops {
>>  int video_reserve(ulong *addrp);
>>
>>  /**
>> + * video_clear() - Clear a device's frame buffer to background color.
>> + *
>> + * @dev:       Device to clear
>> + */
>> +void video_clear(struct udevice *dev);
>> +
>> +/**
>>   * video_sync() - Sync a device's frame buffer with its hardware
>>   *
>>   * Some frame buffers are cached or have a secondary frame buffer. This
>> diff --git a/include/video_console.h b/include/video_console.h
>> index 26047934da..9dce234bd9 100644
>> --- a/include/video_console.h
>> +++ b/include/video_console.h
>> @@ -29,6 +29,9 @@
>>   * @xsize_frac:        Width of the display in fractional units
>>   * @xstart_frac:       Left margin for the text console in fractional units
>>   * @last_ch:   Last character written to the text console on this line
>> + * @escape:    TRUE if currently accumulating an ANSI escape sequence
>> + * @escape_len:        Length of accumulated escape sequence so far
>> + * @escape_buf:        Buffer to accumulate escape sequence
>>   */
>>  struct vidconsole_priv {
>>         struct stdio_dev sdev;
>> @@ -42,6 +45,14 @@ struct vidconsole_priv {
>>         int xsize_frac;
>>         int xstart_frac;
>>         int last_ch;
>> +       /*
>> +        * ANSI escape sequences are accumulated character by character,
>> +        * starting after the ESC char (0x1b) until the entire sequence
>> +        * is consumed at which point it is acted upon.
>> +        */
>> +       int escape;
>> +       int escape_len;
>> +       char escape_buf[32];
>>  };
>>
>>  /**
>> --
>> 2.13.5
>>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [U-Boot] [PATCH] dm: video: Add basic ANSI escape sequence support
  2017-09-11  9:42   ` Rob Clark
@ 2017-09-11 11:50     ` Lothar Waßmann
  2017-09-11 12:31       ` Rob Clark
  0 siblings, 1 reply; 8+ messages in thread
From: Lothar Waßmann @ 2017-09-11 11:50 UTC (permalink / raw)
  To: u-boot

Hi,

On Mon, 11 Sep 2017 05:42:01 -0400 Rob Clark wrote:
> On Mon, Sep 11, 2017 at 2:18 AM, Simon Glass <sjg@chromium.org> wrote:
> > On 7 September 2017 at 14:28, Rob Clark <robdclark@gmail.com> wrote:
> >> Really just the subset that is needed by efi_console.  Perhaps more will
> >> be added later, for example color support would be useful to implement
> >> efi_cout_set_attribute().
> >>
> >> Signed-off-by: Rob Clark <robdclark@gmail.com>
> >> ---
> >>  drivers/video/vidconsole-uclass.c | 112 ++++++++++++++++++++++++++++++++++++++
> >>  drivers/video/video-uclass.c      |   4 +-
> >>  include/video.h                   |   7 +++
> >>  include/video_console.h           |  11 ++++
> >>  4 files changed, 131 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/video/vidconsole-uclass.c b/drivers/video/vidconsole-uclass.c
> >> index e081d5a0ee..7998b4cf5f 100644
> >> --- a/drivers/video/vidconsole-uclass.c
> >> +++ b/drivers/video/vidconsole-uclass.c
> >> @@ -9,6 +9,7 @@
> >>   */
> >>
> >>  #include <common.h>
> >> +#include <linux/ctype.h>
> >>  #include <dm.h>
> >>  #include <video.h>
> >>  #include <video_console.h>
> >> @@ -107,12 +108,123 @@ static void vidconsole_newline(struct udevice *dev)
> >>         video_sync(dev->parent);
> >>  }
> >>
> >> +/*
> >> + * Parse a number from string that ends in a non-numeric character..
> >> + * sscanf() would be nice.  This is just enough for parsing ANSI escape
> >> + * sequences.
> >> + */
> >> +static char *parsenum(char *s, int *num)
> >
> > Can you use simple_strtoul() or similar?
> 
> Possibly, but I'm not sure it is a good idea.. I don't think escape
> sequences are meant to be encoded with hex or octal number strings.
> From a quick look, I don't see any escape code terminated with 'x', so
> maybe it would end up working ok.. but something like ESC[0x1234m
> should be an escape sequence terminated with x followed by normal
> chars 1234m and strtoul would get that wrong..
> 
stroul(s, NULL, 10) will only parse decimal numbers and stop at
non-decimal digits.


Lothar Waßmann

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [U-Boot] [PATCH] dm: video: Add basic ANSI escape sequence support
  2017-09-11 11:50     ` Lothar Waßmann
@ 2017-09-11 12:31       ` Rob Clark
  2017-09-11 20:42         ` Rob Clark
  2017-09-12  2:44         ` Simon Glass
  0 siblings, 2 replies; 8+ messages in thread
From: Rob Clark @ 2017-09-11 12:31 UTC (permalink / raw)
  To: u-boot

On Mon, Sep 11, 2017 at 7:50 AM, Lothar Waßmann <LW@karo-electronics.de> wrote:
> Hi,
>
> On Mon, 11 Sep 2017 05:42:01 -0400 Rob Clark wrote:
>> On Mon, Sep 11, 2017 at 2:18 AM, Simon Glass <sjg@chromium.org> wrote:
>> > On 7 September 2017 at 14:28, Rob Clark <robdclark@gmail.com> wrote:
>> >> Really just the subset that is needed by efi_console.  Perhaps more will
>> >> be added later, for example color support would be useful to implement
>> >> efi_cout_set_attribute().
>> >>
>> >> Signed-off-by: Rob Clark <robdclark@gmail.com>
>> >> ---
>> >>  drivers/video/vidconsole-uclass.c | 112 ++++++++++++++++++++++++++++++++++++++
>> >>  drivers/video/video-uclass.c      |   4 +-
>> >>  include/video.h                   |   7 +++
>> >>  include/video_console.h           |  11 ++++
>> >>  4 files changed, 131 insertions(+), 3 deletions(-)
>> >>
>> >> diff --git a/drivers/video/vidconsole-uclass.c b/drivers/video/vidconsole-uclass.c
>> >> index e081d5a0ee..7998b4cf5f 100644
>> >> --- a/drivers/video/vidconsole-uclass.c
>> >> +++ b/drivers/video/vidconsole-uclass.c
>> >> @@ -9,6 +9,7 @@
>> >>   */
>> >>
>> >>  #include <common.h>
>> >> +#include <linux/ctype.h>
>> >>  #include <dm.h>
>> >>  #include <video.h>
>> >>  #include <video_console.h>
>> >> @@ -107,12 +108,123 @@ static void vidconsole_newline(struct udevice *dev)
>> >>         video_sync(dev->parent);
>> >>  }
>> >>
>> >> +/*
>> >> + * Parse a number from string that ends in a non-numeric character..
>> >> + * sscanf() would be nice.  This is just enough for parsing ANSI escape
>> >> + * sequences.
>> >> + */
>> >> +static char *parsenum(char *s, int *num)
>> >
>> > Can you use simple_strtoul() or similar?
>>
>> Possibly, but I'm not sure it is a good idea.. I don't think escape
>> sequences are meant to be encoded with hex or octal number strings.
>> From a quick look, I don't see any escape code terminated with 'x', so
>> maybe it would end up working ok.. but something like ESC[0x1234m
>> should be an escape sequence terminated with x followed by normal
>> chars 1234m and strtoul would get that wrong..
>>
> stroul(s, NULL, 10) will only parse decimal numbers and stop at
> non-decimal digits.
>

And you'd expect simple_strtoul() would too.. but that does not appear
to be the case.  Not sure if that is intentional.

BR,
-R

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [U-Boot] [PATCH] dm: video: Add basic ANSI escape sequence support
  2017-09-11 12:31       ` Rob Clark
@ 2017-09-11 20:42         ` Rob Clark
  2017-09-12  2:44         ` Simon Glass
  1 sibling, 0 replies; 8+ messages in thread
From: Rob Clark @ 2017-09-11 20:42 UTC (permalink / raw)
  To: u-boot

On Mon, Sep 11, 2017 at 8:31 AM, Rob Clark <robdclark@gmail.com> wrote:
> On Mon, Sep 11, 2017 at 7:50 AM, Lothar Waßmann <LW@karo-electronics.de> wrote:
>> Hi,
>>
>> On Mon, 11 Sep 2017 05:42:01 -0400 Rob Clark wrote:
>>> On Mon, Sep 11, 2017 at 2:18 AM, Simon Glass <sjg@chromium.org> wrote:
>>> > On 7 September 2017 at 14:28, Rob Clark <robdclark@gmail.com> wrote:
>>> >> Really just the subset that is needed by efi_console.  Perhaps more will
>>> >> be added later, for example color support would be useful to implement
>>> >> efi_cout_set_attribute().
>>> >>
>>> >> Signed-off-by: Rob Clark <robdclark@gmail.com>
>>> >> ---
>>> >>  drivers/video/vidconsole-uclass.c | 112 ++++++++++++++++++++++++++++++++++++++
>>> >>  drivers/video/video-uclass.c      |   4 +-
>>> >>  include/video.h                   |   7 +++
>>> >>  include/video_console.h           |  11 ++++
>>> >>  4 files changed, 131 insertions(+), 3 deletions(-)
>>> >>
>>> >> diff --git a/drivers/video/vidconsole-uclass.c b/drivers/video/vidconsole-uclass.c
>>> >> index e081d5a0ee..7998b4cf5f 100644
>>> >> --- a/drivers/video/vidconsole-uclass.c
>>> >> +++ b/drivers/video/vidconsole-uclass.c
>>> >> @@ -9,6 +9,7 @@
>>> >>   */
>>> >>
>>> >>  #include <common.h>
>>> >> +#include <linux/ctype.h>
>>> >>  #include <dm.h>
>>> >>  #include <video.h>
>>> >>  #include <video_console.h>
>>> >> @@ -107,12 +108,123 @@ static void vidconsole_newline(struct udevice *dev)
>>> >>         video_sync(dev->parent);
>>> >>  }
>>> >>
>>> >> +/*
>>> >> + * Parse a number from string that ends in a non-numeric character..
>>> >> + * sscanf() would be nice.  This is just enough for parsing ANSI escape
>>> >> + * sequences.
>>> >> + */
>>> >> +static char *parsenum(char *s, int *num)
>>> >
>>> > Can you use simple_strtoul() or similar?
>>>
>>> Possibly, but I'm not sure it is a good idea.. I don't think escape
>>> sequences are meant to be encoded with hex or octal number strings.
>>> From a quick look, I don't see any escape code terminated with 'x', so
>>> maybe it would end up working ok.. but something like ESC[0x1234m
>>> should be an escape sequence terminated with x followed by normal
>>> chars 1234m and strtoul would get that wrong..
>>>
>> stroul(s, NULL, 10) will only parse decimal numbers and stop at
>> non-decimal digits.
>>
>
> And you'd expect simple_strtoul() would too.. but that does not appear
> to be the case.  Not sure if that is intentional.
>

So I double checked simple_strtoul() in upstream kernel, and (apart
from being re-written) it seems to have fixed this bug.  So I'm
guessing the u-boot copied a buggy version from the linux kernel src
and never got a fix.  I'll send a patch for that.  It will be easy
enough to drop parsenum() once the fix is merged.

BR,
-R

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [U-Boot] [PATCH] dm: video: Add basic ANSI escape sequence support
  2017-09-11 12:31       ` Rob Clark
  2017-09-11 20:42         ` Rob Clark
@ 2017-09-12  2:44         ` Simon Glass
  1 sibling, 0 replies; 8+ messages in thread
From: Simon Glass @ 2017-09-12  2:44 UTC (permalink / raw)
  To: u-boot

Hi Rob,

On 11 September 2017 at 06:31, Rob Clark <robdclark@gmail.com> wrote:
> On Mon, Sep 11, 2017 at 7:50 AM, Lothar Waßmann <LW@karo-electronics.de> wrote:
>> Hi,
>>
>> On Mon, 11 Sep 2017 05:42:01 -0400 Rob Clark wrote:
>>> On Mon, Sep 11, 2017 at 2:18 AM, Simon Glass <sjg@chromium.org> wrote:
>>> > On 7 September 2017 at 14:28, Rob Clark <robdclark@gmail.com> wrote:
>>> >> Really just the subset that is needed by efi_console.  Perhaps more will
>>> >> be added later, for example color support would be useful to implement
>>> >> efi_cout_set_attribute().
>>> >>
>>> >> Signed-off-by: Rob Clark <robdclark@gmail.com>
>>> >> ---
>>> >>  drivers/video/vidconsole-uclass.c | 112 ++++++++++++++++++++++++++++++++++++++
>>> >>  drivers/video/video-uclass.c      |   4 +-
>>> >>  include/video.h                   |   7 +++
>>> >>  include/video_console.h           |  11 ++++
>>> >>  4 files changed, 131 insertions(+), 3 deletions(-)
>>> >>
>>> >> diff --git a/drivers/video/vidconsole-uclass.c b/drivers/video/vidconsole-uclass.c
>>> >> index e081d5a0ee..7998b4cf5f 100644
>>> >> --- a/drivers/video/vidconsole-uclass.c
>>> >> +++ b/drivers/video/vidconsole-uclass.c
>>> >> @@ -9,6 +9,7 @@
>>> >>   */
>>> >>
>>> >>  #include <common.h>
>>> >> +#include <linux/ctype.h>
>>> >>  #include <dm.h>
>>> >>  #include <video.h>
>>> >>  #include <video_console.h>
>>> >> @@ -107,12 +108,123 @@ static void vidconsole_newline(struct udevice *dev)
>>> >>         video_sync(dev->parent);
>>> >>  }
>>> >>
>>> >> +/*
>>> >> + * Parse a number from string that ends in a non-numeric character..
>>> >> + * sscanf() would be nice.  This is just enough for parsing ANSI escape
>>> >> + * sequences.
>>> >> + */
>>> >> +static char *parsenum(char *s, int *num)
>>> >
>>> > Can you use simple_strtoul() or similar?
>>>
>>> Possibly, but I'm not sure it is a good idea.. I don't think escape
>>> sequences are meant to be encoded with hex or octal number strings.
>>> From a quick look, I don't see any escape code terminated with 'x', so
>>> maybe it would end up working ok.. but something like ESC[0x1234m
>>> should be an escape sequence terminated with x followed by normal
>>> chars 1234m and strtoul would get that wrong..
>>>
>> stroul(s, NULL, 10) will only parse decimal numbers and stop at
>> non-decimal digits.
>>
>
> And you'd expect simple_strtoul() would too.. but that does not appear
> to be the case.  Not sure if that is intentional.

That looks like a bug to me although there is no documentation or
tests for that function. The while loop should stop at any character
beyond base I think.

>
> BR,
> -R

Regards,
Simon

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2017-09-12  2:44 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-07 20:28 [U-Boot] [PATCH] dm: video: Add basic ANSI escape sequence support Rob Clark
2017-09-07 20:28 ` [U-Boot] [PATCH] dm: video: Fix cache flushes Rob Clark
2017-09-11  6:18 ` [U-Boot] [PATCH] dm: video: Add basic ANSI escape sequence support Simon Glass
2017-09-11  9:42   ` Rob Clark
2017-09-11 11:50     ` Lothar Waßmann
2017-09-11 12:31       ` Rob Clark
2017-09-11 20:42         ` Rob Clark
2017-09-12  2:44         ` Simon Glass

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.