From: Jan Stary Subject: Re: wscons 256 colour support To: tech@openbsd.org Date: Tue, 17 Jun 2025 10:19:26 +0200 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; > >