Toomas, thanks for putting this all together. I know that I have a lot of comments, but I think that the vast majority of this is good and I know represents a lot of hard work. Thanks!

Maybe add a comment that this is initialized to the BIOS based version and that in the gfx_framework_init() function will change it to use UEFI methods if appropriate?

Probably should use ARRAY_SIZE(solaris_color_to_pc_color), in case we ever change the types here.

Does some other part of the stack guarantee that index will not go beyond the array size?

Do we not care about conversion errors from the strtol or cases where the string is not a valid string like '34asdf'?

Should we worry about the case where the string name is neither 'tem.inverse' or 'tem.inverse-screen'?

Is the framebuffer endianness well defined to be little endian or does it match the host endianness? If the first case, we probably need to adjust the cases here to do conversion.

Should there by a default case here which results in an error?

These should probably be unsigned to match the places that they come from. With pitch we're truncating the uint32_t to an int.

I'm not sure if the calculation of blend guarantees this property or not, but if somehow blend = 0xffff, then we'll have h = 0xff, l = 0xff, which will cause h to be set to 0x100, which we will truncate to zero at the return. Is that the right behavior? Should values greater than a uint8_t in h be truncated versus round up to UINT8_MAX?

Why do we only do alpha blending in the 4 bits per pixel case? Maybe worth a comment explaining that. It's not clear to me why this case is special and the rest default into a memcpy.

s/baack/back

I guess there's nothing we can really do if this or the next call fail, is there?

Can we add a comment somewhere in the file about when we know we have to deal with the TPL? As if I had to modify this or add something new, I'd never know.

Minor note: inconsistent function declaration style, missing return after type.

This gets the case where we have no fb and we're outputing only to serial or similar?

Do we need to free fp->vf_map[n] before returning here?

Can we expand this comment to say why we should just replace the name?

I was going through other callers a bit and was confused when the name argument was never freed. I think that the caller here should do the strdup of name. That might make it clearer to others what's going on.

Is it OK that insert_font() will take ownership of np->n_name?

If we don't change anything about insert_font(), then this should have a comment that when insert_font is successful, it takes ownership over name, so it's OK that we never free it after this point. That confused me for a bit.

'the default resolution will be set to 800x600.'

Does this need to live in a shared header?

Unrelated, but I like the strategy. It seems to make sense. This shows up as a module in the multiboot2 sense right? We won't create a boot time module from it, will we?

Is it possible to add a #define to i386/libi386/vbe.h
to cover the different mode arguments to bios_set_text_mode?

It'd be nice to have a constant for the value 0x100 to make it a little less magical. It's not clear to me exactly why it's 0x100, but I guess that's the first VBE mode value from looking at the VBE 3.0 spec.

Maybe comment this EFI endif?

Is EFI Guaranteed to never have the indexed type framebuffer?

Also, don't we need to check the result of the mb_alloc()?

Don't we need to check the result of the mb_alloc?

How different is this from the kernel version? I assume there's no good way to share the two and it's worth just having a separate copy? I wasn't sure if this was more of a straight up copy with the kernel module bits removed or not, so I haven't looked at this file in great detail.

I noticed this is missing a number of scan codes from the spec. Should we add them or wait for a later date?

I gues we don't care if this fails since there isn't much we can do, right?

I guess we don't care if this fails? Or is it more of the case that there's nothing we can do? There are a bunch of other cases like this in this file, but I'm not going to call them each out as I imagine it'll be the same thing.

Why does this case use tem to hide the cursor, but the other case to show it doesn't not when we're in the !plat_stdout_is_framebuffer()?

Is the only used at startup why this doesn't have a framebuffer case like the others?

Can we say that it doesn't have enough memory while probing the console so we have a sense of where in the code base we got to?

snprintf?

Is it OK to continue if it failed? Does this happen when mirroring or another case? Probably worth an extra comment.

Clean up whitespace?

Space between function and open paren?

Is there a reason we're not checking for x being the character after the strtoul finished? Or are there fonts that are of a different form from %ux%u?

Same thing about strchr versus strtoul end pointer.

efifb_color_depth returns an unsigned integer, but we're assigning it to a signed integer. Maybe i and d should be unsigned?

Can you briefly explain why all the starting constants changed here?

What if we don't find a text console here? Is that possible? If we don't, won't we write over the end of the array?

Maybe we can use this in the place where I pointed out the confusion with the >= 0x100 in multiboot.

This code looks rather familiar at this point. Maybe we can commonize it?

Is there a reason we're converting unsigned values to signed ones?

Probably should use snprintf for these command error buffer prints so we don't clobber global memory.

This looks fairly similar to the existing vgasubr.c. Is it possible to share it in usr/src/common and ifdef STANDALONE versus KERNEL? Or is it impossible because of the use of the struct vgaregmap there? It just seems unfortunate to have almost two identical copies.

See other notes on shared headers on later files.

See other notes on shared headers on later files.

Is there no way to share this header?

Is there no way to avoid duplicating this header?

Is there no way to avoid duplicating this header?

See later notes on header files.

See other header files for notes on sharing (sorry these were reviewed in reverse order on this page). I know this one is a bit more different than others.

Aside from these definitions, this looks the same as the existing kernel header. See other comments about these, but maybe we can share a definition in usr/src/common/ so that way we can have the two stay roughly in sync rather than duplicating them?

Is there a reason we're using u_int rather than uint_t? I guess because of the difference in arguments there's no good way to share this with the pre-existing copy? Maybe we could do something like typedef the first argument and ifdef KERNEL it's the struct otherwise ifdef STANDALONE it's the uint_t?

It looks like this header is the exact same as uts/common/sys/visual_io.h with the exception of adding the following:

\* struct visual_ops
\* 16-bit data to the color array
\* VIS_CONSCLEAR

Is there a reason we can't actually combine this back with the one in uts and share the header between the two rather than just duplicating it in its entirety?

If we're worried about all the intersections between the kernel due to putting it on the loader include file search path, maybe we can share them in common/?

Just to make sure in this case to ficl, ret = -1 is success?

Mind putting the closing #endif comment ala:

#endif /* STANDALONE */

Should we remove the kernel copy of this file for the time being and use its copy?

Worth throwing in the enum pc_colors like the other users used to do?

Shouldn't these be unsigned values, since the font_map works in unsigned values and the arguments are unsigned?

Do we care about the special case where we don't find something and get to dst == 0. Is the first entry in vf_bytes below going to be correct?

Why is this the only one that checks the TEM_CHAR_ATTR andd the others do not. If that's how it should be, can you add a comment explaining why please?

I think we need ot make sure a THIRDPARTYLICENSE and THIRDPARTYLICENSE.descrip are added to cover this and that they are in the corresponding manifest.

I have a bunch of comments on this code. I'm not sure how much modification we want to do to the third party code, but I wanted to mention it. Though I imagine that we may have had to modify it for this.

lseek and read return ssize_t values, but this is returning a size_t. This means that we're definitely transforming that in a way that would make it look like we read a lot of data on failure.

The (off_t)(size \* numel) is taking two unsigned values, multiplying them, and casting them to a signed value. We should probably check for overflow.

Minor style nit, this function is different from the prior ones with respect to cstyle in the return after the return type.

Minor style nit, this function is different from the prior ones with respect to cstyle in the return after the return type.

Shouldn't this only be called if w're in the result == PNG_NO_ERROR case. Perhaps at +188.

Do we care about someone being able to overflow here?

If we hit this case, then we will never free png->zs. I'm not sure where the right place to try and free this up is, but certainly looking at the way callers work today, this'll be leaked. I could see us doing one of two things. One is to always free it in png_end_inflate when we have a zlib error. The other is to try and keep this around and free it on close. However, that has its own risks because we would make sure that we freed it before another call to png_start_inflate.

Maybe we should do both? Try and free it in png_end_inflate, but also try and catch someone not freeing it in png_close() and someone doing the same in png_start_inflate()?

What do you think makes sense?

I assume we don't want to use realloc?

Are there endian concerns here? My read is that this data is in Big Endian, but we're just taking an array of bytes and casting it to a uint32_t, which I think means we'll read that in an endian aware way.

All of the filter length / stride values could probably be unsigned. This applies to the others as well.

Why are we casting this to a char versus a uint8_t? Unfortunately a default 'char' can either be signed or unsigned based on the processor / platform. ARM for example, historically has had the opposite of x86.

This converts an unsigned data to a signed data. It won't overflow, but seems odd to change the sign.

Are COLUMNS and LINES no longer honored? Or is it only no longer honored in the context of using the VGA frame buffer? Just trying to understand why this changed.

Re-defined common macros.

Is it worth to add an ABS() macro into <sys/param.h>?

(Not for this issue.)

This macro already exist in <sys/param.h>, but that's name is capitalized.
And it's not used, so delete it.

abs() macro used here + in the next line.

#ifdef _STANDALONE
#include <sys/param.h>

#ifndef ABS
/* should be in <sys/param.h> */
#define ABS(x)      ((x) < 0 ? -(x) : (x))
#endif

#else /* !_STANDALONE */
#include <sys/sysmacros.h>
#endif

(The preferred macro name should be the capitalized name.)

abs() used here + in the next 2 lines.

names should be an unsigned value.
...
ficlUnsigned names;
...
names = ficlStackPopUnsigned(ficlVmGetDataStack(pVM));

Duplicated macro.

<sys/param.h> already contains this nitems macro.

Should we be using CMD_OK or should we actually be using CMD_WARN/CMD_ERROR in cases where the user input something incorrect?

I think this is always safe, but I'll be honest, seeing the cast was a bit weird at first glance.

In general, I think this is good, though if you don't mind, I'd still like the explicit build failure if we're building big endian that jclulow described.

Unchecked return value? Since this returns void, I guess we should panic on failure?

Unchecked return value? Since this returns void, I guess we should panic on failure?

Should width and cols be unsigned here since we're using them as indexes into an array? Though that change would require redoing the way the while loop works due to the comparison / wraparound.

It might be worth a comment explaining why these two cases (this one and the one a few lines later) are here.

Should we maybe use the constants for these?

Why is this bit in an ifdef DEBUG?

Maybe unsigned count or change the loop to be > 0?

Ship It!

Ship It!