-
-
usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1) 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?
-
usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1) Probably should use ARRAY_SIZE(solaris_color_to_pc_color), in case we ever change the types here.
-
usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1) Does some other part of the stack guarantee that index will not go beyond the array size?
-
usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1) Do we not care about conversion errors from the strtol or cases where the string is not a valid string like '34asdf'?
-
usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1) Should we worry about the case where the string name is neither 'tem.inverse' or 'tem.inverse-screen'?
-
usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1) 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.
-
usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1) Should there by a default case here which results in an error?
-
usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1) These should probably be unsigned to match the places that they come from. With pitch we're truncating the uint32_t to an int.
-
usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1) 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?
-
usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1) 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.
-
-
usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1) I guess there's nothing we can really do if this or the next call fail, is there?
-
usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1) 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.
-
usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1) Minor note: inconsistent function declaration style, missing return after type.
-
usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1) This gets the case where we have no fb and we're outputing only to serial or similar?
-
usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1) Do we need to free fp->vf_map[n] before returning here?
-
usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1) Can we expand this comment to say why we should just replace the name?
-
usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1) 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.
-
usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1) Is it OK that insert_font() will take ownership of np->n_name?
-
usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1) 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.
-
usr/src/boot/sys/boot/common/help.common (Diff revision 1) 'the default resolution will be set to 800x600.'
-
usr/src/boot/sys/boot/common/module.c (Diff revision 1) 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?
-
usr/src/boot/sys/boot/common/multiboot2.c (Diff revision 1) Is it possible to add a #define to i386/libi386/vbe.h to cover the different mode arguments to bios_set_text_mode?
-
usr/src/boot/sys/boot/common/multiboot2.c (Diff revision 1) 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.
-
-
usr/src/boot/sys/boot/common/multiboot2.c (Diff revision 1) Is EFI Guaranteed to never have the indexed type framebuffer? Also, don't we need to check the result of the mb_alloc()?
-
usr/src/boot/sys/boot/common/multiboot2.c (Diff revision 1) Don't we need to check the result of the mb_alloc?
-
usr/src/boot/sys/boot/common/tem.c (Diff revision 1) 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.
-
usr/src/boot/sys/boot/efi/include/eficon.h (Diff revision 1) I noticed this is missing a number of scan codes from the spec. Should we add them or wait for a later date?
-
usr/src/boot/sys/boot/efi/libefi/efi_console.c (Diff revision 1) I gues we don't care if this fails since there isn't much we can do, right?
-
usr/src/boot/sys/boot/efi/libefi/efi_console.c (Diff revision 1) 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.
-
usr/src/boot/sys/boot/efi/libefi/efi_console.c (Diff revision 1) 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()?
-
usr/src/boot/sys/boot/efi/libefi/efi_console.c (Diff revision 1) Is the only used at startup why this doesn't have a framebuffer case like the others?
-
usr/src/boot/sys/boot/efi/libefi/efi_console.c (Diff revision 1) 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?
-
-
usr/src/boot/sys/boot/efi/libefi/efi_console.c (Diff revision 1) Is it OK to continue if it failed? Does this happen when mirroring or another case? Probably worth an extra comment.
-
-
usr/src/boot/sys/boot/efi/loader/framebuffer.c (Diff revision 1) Space between function and open paren?
-
usr/src/boot/sys/boot/efi/loader/framebuffer.c (Diff revision 1) 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?
-
usr/src/boot/sys/boot/efi/loader/framebuffer.c (Diff revision 1) Same thing about strchr versus strtoul end pointer.
-
usr/src/boot/sys/boot/efi/loader/framebuffer.c (Diff revision 1) efifb_color_depth returns an unsigned integer, but we're assigning it to a signed integer. Maybe i and d should be unsigned?
-
usr/src/boot/sys/boot/forth/frames.4th (Diff revision 1) Can you briefly explain why all the starting constants changed here?
-
usr/src/boot/sys/boot/i386/libi386/spinconsole.c (Diff revision 1) 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?
-
usr/src/boot/sys/boot/i386/libi386/vbe.h (Diff revision 1) Maybe we can use this in the place where I pointed out the confusion with the >= 0x100 in multiboot.
-
usr/src/boot/sys/boot/i386/libi386/vbe.c (Diff revision 1) This code looks rather familiar at this point. Maybe we can commonize it?
-
usr/src/boot/sys/boot/i386/libi386/vbe.c (Diff revision 1) Is there a reason we're converting unsigned values to signed ones?
-
usr/src/boot/sys/boot/i386/libi386/vbe.c (Diff revision 1) Probably should use snprintf for these command error buffer prints so we don't clobber global memory.
-
usr/src/boot/sys/boot/i386/libi386/vgasubr.c (Diff revision 1) 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.
-
-
-
-
-
usr/src/boot/sys/sys/list_impl.h (Diff revision 1) Is there no way to avoid duplicating this header?
-
-
usr/src/boot/sys/sys/tem_impl.h (Diff revision 1) 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.
-
usr/src/boot/sys/sys/vgareg.h (Diff revision 1) 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?
-
usr/src/boot/sys/sys/vgasubr.h (Diff revision 1) 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?
-
usr/src/boot/sys/sys/visual_io.h (Diff revision 1) 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/?
-
usr/src/common/ficl/loader.c (Diff revision 1) Just to make sure in this case to ficl, ret = -1 is success?
-
usr/src/common/ficl/loader.c (Diff revision 1) Mind putting the closing #endif comment ala: #endif /* STANDALONE */
-
usr/src/common/font/font.c (Diff revision 1) Should we remove the kernel copy of this file for the time being and use its copy?
-
usr/src/common/font/font.c (Diff revision 1) Worth throwing in the enum pc_colors like the other users used to do?
-
usr/src/common/font/font.c (Diff revision 1) Shouldn't these be unsigned values, since the font_map works in unsigned values and the arguments are unsigned?
-
usr/src/common/font/font.c (Diff revision 1) 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?
-
usr/src/common/font/font.c (Diff revision 1) 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?
-
usr/src/common/pnglite/pnglite.h (Diff revision 1) 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.
-
usr/src/common/pnglite/pnglite.c (Diff revision 1) 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.
-
usr/src/common/pnglite/pnglite.c (Diff revision 1) 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.
-
usr/src/common/pnglite/pnglite.c (Diff revision 1) 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.
-
usr/src/common/pnglite/pnglite.c (Diff revision 1) Minor style nit, this function is different from the prior ones with respect to cstyle in the return after the return type.
-
usr/src/common/pnglite/pnglite.c (Diff revision 1) Minor style nit, this function is different from the prior ones with respect to cstyle in the return after the return type.
-
usr/src/common/pnglite/pnglite.c (Diff revision 1) Shouldn't this only be called if w're in the result == PNG_NO_ERROR case. Perhaps at +188.
-
usr/src/common/pnglite/pnglite.c (Diff revision 1) Do we care about someone being able to overflow here?
-
usr/src/common/pnglite/pnglite.c (Diff revision 1) 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?
-
-
usr/src/common/pnglite/pnglite.c (Diff revision 1) 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.
-
usr/src/common/pnglite/pnglite.c (Diff revision 1) All of the filter length / stride values could probably be unsigned. This applies to the others as well.
-
usr/src/common/pnglite/pnglite.c (Diff revision 1) 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.
-
usr/src/common/pnglite/pnglite.c (Diff revision 1) This converts an unsigned data to a signed data. It won't overflow, but seems odd to change the sign.
-
usr/src/man/man5/loader.5 (Diff revision 1) 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.
10028 loader: implement framebuffer console
Review Request #1313 — Created Dec. 3, 2018 and submitted
Information | |
---|---|
tsoome | |
illumos-gate | |
8918, 10028, 10029, 10030, 10031, 10032, 10033, 10034, 10035, 10036, 10037, 10038, 10039, 10040 | |
2c92777... | |
Reviewers | |
general | |
10029 common/font: create shared font.c 10030 import pnglite into usr/src/common/pnglite 8918 loader.efi: add vesa edid support 10031 loader: import tem for loader console 10032 loader: implement tem utf-8 support 10033 ficl: add simple gfx words 10034 loader: use term-drawrect for menu frame 10035 loader: add alpha blending for gfx_fb 10036 ficl: add fb-putimage 10037 loader: add illumos.png logo 10038 loader: replace gop and vesa with framebuffer 10039 loader: build rules for new font setup 10040 loader: gfx use GOP Blt() function in visual_io callbacks
Change Summary:
missing underline support in depths less then 32.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+10616 -1238) |
-
Re-defined common macros.
-
usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 2) Is it worth to add an
ABS()
macro into <sys/param.h>?
(Not for this issue.) -
usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 2) - This macro already exist in <sys/param.h>, but that's name is capitalized.
- And it's not used, so delete it.
-
-
usr/src/common/pnglite/pnglite.c (Diff revision 2) #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.)
-
Change Summary:
pnglite cleanup from reviews.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+10669 -1238) |
Change Summary:
pnglite cstyle
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+10669 -1238) |
Change Summary:
ficl updates based on reviews
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+10670 -1238) |
Change Summary:
help.common review
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+10670 -1238) |
-
-
usr/src/common/ficl/loader.c (Diff revision 6) names
should be an unsigned value.... ficlUnsigned names; ... names = ficlStackPopUnsigned(ficlVmGetDataStack(pVM));
Change Summary:
use ficlUnsigned
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+10670 -1238) |
Change Summary:
multiboot(2) updates, add names for constants
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+10674 -1240) |
Change Summary:
white space nits
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 9 (+10827 -1391) |
Change Summary:
unused parameters
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 10 (+10829 -1393) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 11 (+10835 -1393) |
Change Summary:
cleanup for gfx_fb.c
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 12 (+10864 -1393) |
Change Summary:
update for efi_console.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 13 (+10893 -1415) |
Change Summary:
more framebuffer updates.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 14 (+10875 -1418) |
Change Summary:
cleaning up duplicated headers/sources
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 15 (+9718 -1470) |
Change Summary:
update for font.c
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 16 (+9752 -1470) |
Change Summary:
whitespace nit
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 17 (+9752 -1470) |
Change Summary:
improve feedback
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 18 (+9755 -1470) |
Change Summary:
whitespace nits from pbchk
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 19 (+9758 -1474) |
-
Duplicated macro.
-
usr/src/boot/sys/boot/uboot/common/main.c (Diff revision 19) <sys/param.h>
already contains thisnitems
macro.
-
-
usr/src/boot/sys/boot/common/gfx_fb.c (Diff revisions 1 - 19) Should we be using CMD_OK or should we actually be using CMD_WARN/CMD_ERROR in cases where the user input something incorrect?
-
usr/src/boot/sys/boot/common/gfx_fb.c (Diff revisions 1 - 19) I think this is always safe, but I'll be honest, seeing the cast was a bit weird at first glance.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 20 (+9769 -1474) |
-
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.
Change Summary:
simple scrolling optimization skip the line tails if the content is the same. For images, we always copy the content. For ultra wide screens, we even may want to check the other segments of the line (not done in this patch).
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 21 (+9911 -1474) |
-
-
usr/src/boot/sys/boot/common/tem.c (Diff revisions 20 - 21) Unchecked return value? Since this returns void, I guess we should panic on failure?
-
usr/src/boot/sys/boot/common/tem.c (Diff revisions 20 - 21) Unchecked return value? Since this returns void, I guess we should panic on failure?
-
usr/src/boot/sys/boot/common/tem.c (Diff revisions 20 - 21) 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.
-
usr/src/boot/sys/boot/common/tem.c (Diff revisions 20 - 21) It might be worth a comment explaining why these two cases (this one and the one a few lines later) are here.
-
usr/src/boot/sys/boot/common/tem.c (Diff revisions 20 - 21) Should we maybe use the constants for these?
-
-
usr/src/boot/sys/boot/common/tem.c (Diff revisions 20 - 21) Maybe unsigned count or change the loop to be > 0?
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 22 (+9911 -1474) |