Index | Thread | Search

From:
Jan Stary <hans@stare.cz>
Subject:
Re: wscons 256 colour support
To:
tech@openbsd.org
Date:
Tue, 17 Jun 2025 10:19:26 +0200

Download raw body.

Thread
  • Jan Stary:

    wscons 256 colour support

  • Thius just to say that it works fine on my
    MacBook Air M1 using current/arm64.
    
    	Jan
    
    On Jun 13 03:20:20, kolipe.c@exoticsilicon.com wrote:
    > On Thu, Jun 12, 2025 at 06:56:37PM -0400, Daniel Dickman wrote:
    > > I would like to see support for this if possible.
    > 
    > Great!
    > 
    > > 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?
    > 
    > Good catch, it should be x.
    > 
    > (The existing code only calls the macro with x == i, so no functional change,
    >  but I've fixed it in the updated diff below.)
    > 
    > > >  	/* 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?
    > 
    > It's still correct.
    > 
    > That function is performing underlining for rotated displays, (obviously the
    > underline has to be rotated as well and the regular code doesn't handle that),
    > and the comment is referring to the fact that the underline painting code is
    > hardcoded to only support a 16bpp framebuffer layout.
    > 
    > I would be very surprised if anybody was actually using display rotation on
    > wscons.  The rotation code should probably be removed altogether.
    > 
    > > > +	#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) >> ...
    > 
    > These macros are only intended for use in generating the colour cube, so I
    > didn't think it was necessary, but if there is a risk that someone will call
    > them elsewhere with an expression for x then it might be a good idea.
    > 
    > Updated diff below...
    > 
    > --- sys/dev/rasops/rasops.c	Thu Jun 12 19:39:02 2025
    > +++ sys/dev/rasops/rasops.c	Thu Jun 13 04:14:23 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 */
    > +	#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+((x-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;
    > 
    > 
    
    
  • Jan Stary:

    wscons 256 colour support