Index | Thread | Search

From:
Daniel Dickman <didickman@gmail.com>
Subject:
Re: wscons 256 colour support
To:
tech@openbsd.org
Date:
Thu, 12 Jun 2025 18:56:37 -0400

Download raw body.

Thread
I would like to see support for this if possible.

Before I try the diff though:

> +	#define EBGREY_CHANNEL(x) (int)(1+((i-232)*11))

What does the "i" refer to in this define? We have a parameter "x" but no 
reference to "x" in the macro expansion itself, so not sure if this is 
expected or not?

>  	/* XXX this assumes 16-bit color depth */

Does this comment need to be changed? Or is it still correct even after 
the updates below are applied?

> +	#define EBCOL_BLUE(x) ((48*((x-16)%6) >> (8 - ri->ri_bnum)) << ri->ri_bpos)

Could the defines benefit from additional parentheses around parameters 
for safety? e.g :

#define EBCOL_BLUE(x) ((48*((    (x)    -16)%6) >> ...


On Thu, 12 Jun 2025, Crystal Kolipe wrote:

> This patch adds support for 256 colours to wscons on 32-bit displays.
> 
> I originally posted a version of this back in 2023, as part of a larger
> console enhancement diff, but the 256 colour part is usable completely
> independently of the other enhancements.
> 
> Since we now have enough support already in for the other control sequences
> necessary for TERM=xterm-256color to display output on wscons, it seems like a
> good time to revisit this 256 colour code.
> 
> It's been in use here for over two years without problems.
> 
> --- sys/dev/rasops/rasops.c	Thu Jun 12 19:39:02 2025
> +++ sys/dev/rasops/rasops.c	Thu Jun 12 19:41:57 2025
> @@ -446,6 +446,10 @@
>  		    WSSCREEN_WSCOLORS | WSSCREEN_REVERSE;
>  	}
>  
> +	if (ri->ri_depth == 32) {
> +		ri->ri_caps |= WSSCREEN_256COL;
> +		}
> +
>  	switch (ri->ri_depth) {
>  #if NRASOPS1 > 0
>  	case 1:
> @@ -557,7 +561,7 @@
>  	if ((flg & WSATTR_HILIT) != 0 && fg < 8)
>  		fg += 8;
>  
> -	*attr = (bg << 16) | (fg << 24) | (flg & WSATTR_UNDERLINE);
> +	*attr = (bg << 16) | (fg << 24) | flg;
>  	return (0);
>  }
>  
> @@ -581,7 +585,7 @@
>  		bg = swap;
>  	}
>  
> -	*attr = (bg << 16) | (fg << 24) | (flg & WSATTR_UNDERLINE);
> +	*attr = (bg << 16) | (fg << 24) | flg;
>  	return (0);
>  }
>  
> @@ -863,6 +867,24 @@
>  		ri->ri_devcmap[i] = c;
>  #endif
>  	}
> +	/* Define colours 16-255 if we are running in 32bpp or 16bpp */
> +	#define EBCOL_BLUE(x) ((48*((x-16)%6) >> (8 - ri->ri_bnum)) << ri->ri_bpos)
> +	#define EBCOL_GREEN(x) (((48*(((x-16)/6)%6)) >> (8 - ri->ri_gnum)) << ri->ri_gpos)
> +	#define EBCOL_RED(x) (((48*(((x-16)/36)%6)) >> (8 - ri->ri_rnum)) << ri->ri_rpos)
> +	#define EBCOL(x) EBCOL_RED(x) | EBCOL_GREEN(x) | EBCOL_BLUE(x)
> +	#define EBGREY_CHANNEL(x) (int)(1+((i-232)*11))
> +	#define EBGREY_R(x)(( EBGREY_CHANNEL(x) >> (8 - ri->ri_rnum)) << ri->ri_rpos)
> +	#define EBGREY_G(x)(( EBGREY_CHANNEL(x) >> (8 - ri->ri_gnum)) << ri->ri_gpos)
> +	#define EBGREY_B(x)(( EBGREY_CHANNEL(x) >> (8 - ri->ri_bnum)) << ri->ri_bpos)
> +	#define EBGREY(x) EBGREY_R(x) | EBGREY_G(x) | EBGREY_B(x)
> +	if (ri->ri_depth == 32) {
> +		for (i=16 ; i<256; i++) {
> +			c = (i < 232 ? EBCOL(i) : EBGREY(i));
> +			if (ri->ri_flg & RI_BSWAP)
> +				c = swap32(c);
> +			ri->ri_devcmap[i] = c;
> +		}
> +	}
>  #endif
>  }
>  
> @@ -872,8 +894,8 @@
>  void
>  rasops_unpack_attr(void *cookie, uint32_t attr, int *fg, int *bg, int *underline)
>  {
> -	*fg = ((u_int)attr >> 24) & 0xf;
> -	*bg = ((u_int)attr >> 16) & 0xf;
> +	*fg = ((u_int)attr >> 24) & 0xff;
> +	*bg = ((u_int)attr >> 16) & 0xff;
>  	if (underline != NULL)
>  		*underline = (u_int)attr & WSATTR_UNDERLINE;
>  }
> @@ -903,7 +925,7 @@
>  		return 0;
>  #endif
>  
> -	clr = ri->ri_devcmap[(attr >> 16) & 0xf];
> +	clr = ri->ri_devcmap[(attr >> 16) & 0xff];
>  
>  	/*
>  	 * XXX The wsdisplay_emulops interface seems a little deficient in
> @@ -1257,7 +1279,7 @@
>  
>  	/* XXX this assumes 16-bit color depth */
>  	if ((attr & WSATTR_UNDERLINE) != 0) {
> -		int16_t c = (int16_t)ri->ri_devcmap[((u_int)attr >> 24) & 0xf];
> +		int16_t c = (int16_t)ri->ri_devcmap[((u_int)attr >> 24) & 0xff];
>  
>  		while (height--) {
>  			*(int16_t *)rp = c;
> @@ -1788,8 +1810,8 @@
>  	attr = ri->ri_bs[off].attr;
>  
>  	if ((ri->ri_flg & RI_CURSOR) == 0) {
> -		fg = ((u_int)attr >> 24) & 0xf;
> -		bg = ((u_int)attr >> 16) & 0xf;
> +		fg = ((u_int)attr >> 24) & 0xff;
> +		bg = ((u_int)attr >> 16) & 0xff;
>  		attr &= ~0x0ffff0000;
>  		attr |= (fg << 16) | (bg << 24);
>  	}
> --- sys/dev/rasops/rasops.h	Mon Apr  7 04:25:22 2025
> +++ sys/dev/rasops/rasops.h	Thu Jun 12 19:41:57 2025
> @@ -106,7 +106,7 @@
>  	u_char  *ri_origbits;	/* where screen bits actually start */
>  	int	ri_xorigin;	/* where ri_bits begins (x) */
>  	int	ri_yorigin;	/* where ri_bits begins (y) */
> -	int32_t	ri_devcmap[16]; /* color -> framebuffer data */
> +	int32_t	ri_devcmap[256]; /* color -> framebuffer data */
>  
>  	/* The emulops you need to use, and the screen caps for wscons */
>  	struct	wsdisplay_emulops ri_ops;
> --- sys/dev/rasops/rasops32.c	Mon Apr  7 04:25:22 2025
> +++ sys/dev/rasops/rasops32.c	Thu Jun 12 19:41:57 2025
> @@ -91,8 +91,8 @@
>  	width = ri->ri_font->fontwidth;
>  	step = ri->ri_stride >> 3;
>  
> -	b = ri->ri_devcmap[(attr >> 16) & 0xf];
> -	f = ri->ri_devcmap[(attr >> 24) & 0xf];
> +	b = ri->ri_devcmap[(attr >> 16) & 0xff];
> +	f = ri->ri_devcmap[(attr >> 24) & 0xff];
>  	u.d[0][0] = b; u.d[0][1] = b;
>  	u.d[1][0] = b; u.d[1][1] = f;
>  	u.d[2][0] = f; u.d[2][1] = b;
> --- sys/dev/wscons/wsdisplayvar.h	Mon Apr  7 04:25:23 2025
> +++ sys/dev/wscons/wsdisplayvar.h	Thu Jun 12 19:41:57 2025
> @@ -114,6 +114,7 @@
>  #define WSSCREEN_HILIT		4	/* can highlight (however) */
>  #define WSSCREEN_BLINK		8	/* can blink */
>  #define WSSCREEN_UNDERLINE	16	/* can underline */
> +#define WSSCREEN_256COL		32	/* supports 256 colours */
>  };
>  
>  /*
> --- sys/dev/wscons/wsemul_vt100_subr.c	Mon Apr  7 04:42:48 2025
> +++ sys/dev/wscons/wsemul_vt100_subr.c	Thu Jun 12 19:41:57 2025
> @@ -630,6 +630,64 @@
>  				flags |= WSATTR_WSCOLORS;
>  				fgcol = ARG(n) - 30;
>  				break;
> +			/*
> +			 * Sequences starting CSI 38 escape to a larger
> +			 * colourspace, typically either 256 colours or 24-bit.
> +			 *
> +			 * We support CSI 38;5;X;m to set colour X from a
> +			 * palette of 256.
> +			 */
> +#define EXIST_ARG2(i) ((edp->nargs-n)>=3)
> +#define ARG2_OR_DEF(i) (EXIST_ARG2(i) ? ARG(i+2) : 0)
> +			case 38:
> +				/*
> +				 * 38 followed by zero arguments is meaningless.
> +				 */
> +				if (edp->nargs == n+1) {
> +					break ;
> +				}
> +				/*
> +				 * 5 should normally be followed by a single
> +				 * argument, but zero arguments is also valid to
> +				 * set colour zero.
> +				 */
> +				if (ARG(n+1)==5) {
> +					flags |= WSATTR_WSCOLORS;
> +					if (edp->scrcapabilities &
> +					    WSSCREEN_256COL) {
> +						fgcol = ARG2_OR_DEF(n);
> +						} else {
> +						fgcol = (ARG2_OR_DEF(n) < 8 ?
> +						    ARG2_OR_DEF(n) : fgcol );
> +						}
> +					n+=(EXIST_ARG2(n) ? 2 : 1);
> +					break;
> +				}
> +				/*
> +				 * 2 should introduce a sequence of three
> +				 * arguments, specifying RGB.
> +				 *
> +				 * We don't, (yet!), support setting colours by
> +				 * 24-bit RGB arguments and don't want to
> +				 * interpret these as regular SGR codes.
> +				 *
> +				 * If there are more then three, skip just
> +				 * three, otherwise skip all of them.
> +				 */
> +				if (ARG(n+1)==2) {
> +					n=(edp->nargs-n > 5 ? n+4 :
> +					    edp->nargs);
> +					break;
> +				}
> +				/*
> +				 * Invalid code, I.E. not 2 or 5.
> +				 *
> +				 * We do what xterm does and just skip the
> +				 * single unrecognised argument, then allow any
> +				 * following arguments to be interpreted as SGR.
> +				 */
> +				n++;
> +				break;
>  			case 39:
>  				/* reset fg color */
>  				fgcol = WSCOL_WHITE;
> @@ -642,6 +700,29 @@
>  				flags |= WSATTR_WSCOLORS;
>  				bgcol = ARG(n) - 40;
>  				break;
> +			case 48: /* set 8-bit background colour */
> +				if (edp->nargs == n+1) {
> +					break ;
> +				}
> +				if (ARG(n+1)==5) {
> +					flags |= WSATTR_WSCOLORS;
> +					if (edp->scrcapabilities &
> +					    WSSCREEN_256COL) {
> +						bgcol = ARG2_OR_DEF(n);
> +						} else {
> +						bgcol = (ARG2_OR_DEF(n) < 8 ?
> +						    ARG2_OR_DEF(n) : bgcol );
> +						}
> +					n+=(EXIST_ARG2(n) ? 2 : 1);
> +					break;
> +				}
> +				if (ARG(n+1)==2) {
> +					n=(edp->nargs-n > 5 ? n+4 :
> +					    edp->nargs);
> +					break;
> +				}
> +				n++;
> + 				break;
>  			case 49:
>  				/* reset bg color */
>  				bgcol = WSCOL_BLACK;
> 
>