10028 loader: implement framebuffer console

Review Request #1313 — Created Dec. 3, 2018 and submitted

tsoome
illumos-gate
8918, 10028, 10029, 10030, 10031, 10032, 10033, 10034, 10035, 10036, 10037, 10038, 10039, 10040
2c92777...
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


  • 0
  • 0
  • 69
  • 22
  • 91
Description From Last Updated
rm
  1. 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!

    1. The note about sharing the code/headers with kernel - since I did decide to push loader change first and kernel change(s) later, I do not really want to mess with kernel bits. So most of the sharing setup will have to happen later, when we will build up the kernel part.

      This is especially true about tem, because the tem in kernel is more complex - it has to support threads and has to support safe calls from kmdb. Also we would like to keep the loader tem relatively simple (because of space constraints), while we would like to have kernel tem to evolve to support scroll back buffer and things like that.

    2. OK, that seems like a reasonable trade off.

  2. 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?
  3. 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.
    1. in loader, it is nitems()

  4. 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?
    1. indirectly; we start up with default foreground and background colors and only way to change those is via esc sequences sent to tem, and there we do have fixed list of possible values. If we allow to set color directly to lit the pixel or we do implement interface to allow custom RGB values to be loaded, then this should be reviewed.

  5. 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'?
    1. it is nice to care, so I did update..

    2. Thanks.

  6. 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'?
    1. no, this callback is set in gfx_framework_init() and bond to those two variables, we will only be called when user is about to set one of those.

  7. 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.
    1. Yes, I would wait for actual case here.

    2. It seems fine to avoid writing the endian conversion code, but in it's place we should probably have a compile-time check? e.g., https://github.com/joyent/illumos-joyent/blob/8a15db18cc817a39246b143e7ccd35a0898ed743/usr/src/uts/common/sys/scsi/adapters/smrt/smrt.h#L38-L44

    3. I'm fine if we want to punt on it, let's just make it explicitly fail at compile time.

  8. usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1)
     
     

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

    1. tem actually does not do anything about the error...

  9. 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.
    1. Yes, I have bunch of those to get unsigned, the extra warnings will trigger otherwise...

  10. 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?
    1. We can not get blend to 0xffff. The maximum we can get is fg = 0xff, bg = 0xff, and alpha = 0xfe (0xff is corner case). in such setup, we will have blend 0xfe01 and that will result with h = 0xff.

  11. 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.
    1. ay, this one is confusing because the bpp here is bytes per pixel. comment added.

  12. usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1)
     
     
    s/baack/back
  13. 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?
    1. Yep. There are two error cases:
      BltOperation is not valid. -- we should not fail this ever.
      The device had an error and could not complete the request. -- this one might happen, but if so, we can not really do anything about it at this point.

  14. 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.

  15. usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1)
     
     

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

  16. 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?
    1. if we have BIOS loader and the console is set to text mode. Note the "direct" drawing functions like setpixel() are still flawed for UEFI - in case we actually do not have physical frame buffer and can only use Blt() to draw. But this flaw should be dealt later (especially because I have nowhere to test, and our kernel FB console would be dead too).

  17. usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 1)
     
     
     
     
    Do we need to free fp->vf_map[n] before returning here?
    1. no, but I'll add it anyhow for consistency. The load_font() will clean up anyhow.

  18. 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?
  19. 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.
  20. 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?
    1. we better allocate own instance..

  21. 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.
  22. 'the default resolution will be set to 800x600.'
  23. 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?
    1. Yes, the idea is that:
      1. we have compiled in (limited) font - 8x16, to be able to have basic screen output on FB. 8x16 was selected because it can be shared with VGA text mode too. The alternative implementation could be to keep text mode/SimpleTextOutputProtocol till we have font loaded.
      2. if we have font loaded (manual or autoload), it is full font and used by loader, and if FB is supported by the kernel, we will pass this font down to kernel as multiboot module (type=console-font). This module has small header with pointers to data and it is in memory representation of the loaded font.
      3. kernel also will have small built in font for case we get framebuffer, or to load vga text mode font, but if the console-module font was passed, it will be used instead - in early boot by using the module memory directly, and later the memory will be allocated and the module will be released.
      4. At some point in future, we can implement font loading to kernel - the base mechanism to switch the font is already present, but the tooling and the data transfer to kernel is to be implemented. However, I would assume that for most cases the boot loader mechanism will be preferred anyhow.

    2. OK, thanks for the additional details here. This makes sense. I like it.

  24. Is it possible to add a #define to i386/libi386/vbe.h
    to cover the different mode arguments to bios_set_text_mode?
  25. 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.
  26. Maybe comment this EFI endif?
  27. 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()?
    1. never is such an long time, but I think this is quite unlikely - at least for now we only do get 32-bit depths. mb_malloc is explained below.

  28. Don't we need to check the result of the mb_alloc?
    1. no, we count the size for mb info, then allocate it, and mb_malloc() will get chunks from this area.

  29. 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.
    1. As noted in opening comment - I am afraid this one will be quite impossible to share; with full rewrite, if we would separate out the terminal emulator state automata, we could get the common code. But I am not sure if this is worth the hassle.

  30. I noticed this is missing a number of scan codes from the spec. Should we add them or wait for a later date?
    1. Those are all there but seplit bad. rearranged.

  31. I gues we don't care if this fails since there isn't much we can do, right?
    1. Yes. in worst case, if the menu (where we really need the terminal emulation) is distorted, we can always just drop to cli (ok) prompt and boot manually. And in any case, for 99.999% cases the boot screen is visible for countdown time:D

  32. 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.
  33. 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()?
    1. the function name only is hinting it was created for tem; since the kernel tem does require its presence, I did kept the same for loader. In kernel it is only meaningful for sparc, to switch the firmware cursor off.

      but for feature patity, I did factor out those bits now.

  34. Is the only used at startup why this doesn't have a framebuffer case like the others?
    1. Yes, this is also originating from sparc prom, when tem is starting up, we fetch the prom cursor position to continue where from we were left to. So we only get the cursor position in terms of terminal coordinates and the tem will do the math. Note that here it does not really matter if we can preserve the cursor position because switching the font will clear the whole screen anyhow.

  35. 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?
  36. 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.
  37. Clean up whitespace?

  38. Space between function and open paren?
  39. 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?
  40. usr/src/boot/sys/boot/efi/loader/framebuffer.c (Diff revision 1)
     
     
     
     
     
    Same thing about strchr versus strtoul end pointer.
  41. efifb_color_depth returns an unsigned integer, but we're assigning it to a signed integer. Maybe i and d should be unsigned?
  42. usr/src/boot/sys/boot/forth/frames.4th (Diff revision 1)
     
     
    Can you briefly explain why all the starting constants changed here?
    1. To avoid translation pain. the UEFI text console is unicode, our console fonts are unicode, so we can just use xemit to emit unicode box chars and let the low level console code to sort the final output - bitmaps are drawn, UEFI simple text output will pring unicode and vga text mode will translate unicode to its own box chars.

  43. 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?
    1. The consoles (and block devices and file systems) are built on static arrays and "text" is always present, so yea, we can take it granted. Nevertheless it is good idea to have some guard...

  44. Maybe we can use this in the place where I pointed out the confusion with the >= 0x100 in multiboot.
  45. 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?
  46. Is there a reason we're converting unsigned values to signed ones?
  47. Probably should use snprintf for these command error buffer prints so we don't clobber global memory.
  48. 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.
  49. usr/src/boot/sys/sys/ascii.h (Diff revision 1)
     
     

    See other notes on shared headers on later files.

  50. usr/src/boot/sys/sys/consplat.h (Diff revision 1)
     
     
    See other notes on shared headers on later files.
  51. usr/src/boot/sys/sys/kd.h (Diff revision 1)
     
     
    Is there no way to share this header?
  52. usr/src/boot/sys/sys/list.h (Diff revision 1)
     
     
    Is there no way to avoid duplicating this header?
  53. usr/src/boot/sys/sys/list_impl.h (Diff revision 1)
     
     
    Is there no way to avoid duplicating this header?
  54. usr/src/boot/sys/sys/tem.h (Diff revision 1)
     
     
    See later notes on header files.
    1. yea, I'd leave tem headers duplicated for now. To build the unicode support, the kernel tem will need similar changes and once done, we can see how we can deal with those.

  55. 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.
  56. 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?
  57. 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?
  58. 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/?
    1. this is a bit fishy, but well...

  59. usr/src/common/ficl/loader.c (Diff revision 1)
     
     
    Just to make sure in this case to ficl, ret = -1 is success?
    1. Yes, in forth, the -1 is traditional notation for true or success.

      1. common/ficl/ficl.h#590 already contains definitions for FICL_TRUE and FICL_FALSE. Maybe we could use these macro constants or create the next two and use those to lessen the code ambiguity (in general, not just here).
      #define FICL_SUCCESS FICL_TRUE
      #define FICL_FAILURE FICL_FALSE
      
      1. Type of ret variable should be ficlInteger, not int.
    2. good point, fixed.

  60. usr/src/common/ficl/loader.c (Diff revision 1)
     
     
    Mind putting the closing #endif comment ala:
    
    #endif /* STANDALONE */
  61. 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?
    1. Yes, but later, once we have the kernel updates in place. We also will drop the current fonts built into the kernel etc and to avoid "prermaturely" touching the kernel, I did create this patch dedicated only for the loader part.

  62. usr/src/common/font/font.c (Diff revision 1)
     
     
    Worth throwing in the enum pc_colors like the other users used to do?
  63. 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?
    1. I think it should be safe, as we first do perform the linear search so all values should get well above 0:)

  64. 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?
    1. Yes. The code is built to process font data where we can have both normal and bold glyphs, with fallback to normal. For example, Gallant does only provide normal (its actually semibold). But also it may be that some glyphs are missing from bold set. If dst is still 0, we have special corner case where font very first glyph is one used to denote the missing symbol (? or empty rectangle or like).

  65. 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?
  66. 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.
  67. 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.
  68. 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.
  69. 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.
  70. 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.
  71. 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.

  72. 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.
  73. usr/src/common/pnglite/pnglite.c (Diff revision 1)
     
     
    Do we care about someone being able to overflow here?
  74. 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?
    1. The normal approach should be that we do not end up with partial setup. So we will always free in png_end_inflate() and clean up in case of error in png_init_inflate()

  75. usr/src/common/pnglite/pnglite.c (Diff revision 1)
     
     
    I assume we don't want to use realloc?
  76. 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.
  77. 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.
  78. 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.
    1. I would believe, they were intending to achieve (in[i] + sum/2) & 0xff here.

  79. 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.
  80. 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.
    1. I did decide to go with screen-* because we have those properties in our kernel (wscons.c): screen-#cols, screen-#rows, screen-width, screen-height. We build up the console properties in loader and pass the values down to kernel via environment module and the kernel will inherit and use the values (down to cursor position). This will allow us to get consistent console and will save us from translating loader properties to kernel properties in kernel fakebop.c.

  81. 
      
tsoome
domag02
  1. Re-defined common macros.
  2. 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.)

  3. 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.
  4. usr/src/boot/sys/boot/common/gfx_fb.c (Diff revision 2)
     
     

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

  5. 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.)

    1. abs() is tricky because it is function in userland (man abs).

  6. usr/src/common/pnglite/pnglite.c (Diff revision 2)
     
     

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

  7. 
      
tsoome
tsoome
tsoome
tsoome
domag02
  1. 
      
  2. usr/src/common/ficl/loader.c (Diff revision 6)
     
     

    names should be an unsigned value.

    ...
    ficlUnsigned names;
    ...
    names = ficlStackPopUnsigned(ficlVmGetDataStack(pVM));
    
  3. 
      
tsoome
tsoome
tsoome
tsoome
tsoome
tsoome
tsoome
tsoome
tsoome
tsoome
tsoome
tsoome
tsoome
domag02
  1. Duplicated macro.
  2. <sys/param.h> already contains this nitems macro.

    1. yes, but not related to this work:)

  3. 
      
rm
  1. 
      
  2. 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?
    1. There is a problem with those codes - when we get the error while interpreting startup scripts, the interpeter will stop. So we have one hand with lets set an error and other hand with should we break the boot process. I guess with colors we should rather keep boot going:)

    2. OK, this makes sense. Yeah, we'd rather try to boot, I think. Seems like in the future we should separate the fatalness of the two operations somehow, but that's another day's problem.

  3. 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.
  4. 
      
tsoome
rm
  1. 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.

  2. 
      
tsoome
rm
  1. 
      
  2. 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?

    1. Yep. I can not get any other good option.

  3. 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?

    1. tvs_screen_buf is simple - if we fail to allocate, we will not use it (the code has checks in place). We fall back on full line copy in copy area.

    2. OK, can I recommend a comment there that mentions that?

    3. yes, good point.

  4. 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.

    1. yea, I do not think using unsigned value will serve any good purpose there except we will complicate things.

  5. 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.

  6. usr/src/boot/sys/boot/common/tem.c (Diff revisions 20 - 21)
     
     
     
    Should we maybe use the constants for these?
    1. ya, well, and those actually only matter for debug purposes (to paint the image area).

    2. removed them.

  7. usr/src/boot/sys/boot/common/tem.c (Diff revisions 20 - 21)
     
     
     
     
    Why is this bit in an ifdef DEBUG?
    1. Literally for debug - so the developer can see the area:)

    2. removed it.

  8. usr/src/boot/sys/boot/common/tem.c (Diff revisions 20 - 21)
     
     
    Maybe unsigned count or change the loop to be > 0?
  9. 
      
tsoome
rm
  1. Ship It!
  2. 
      
domag02
  1. Ship It!
  2. 
      
tsoome
Review request changed

Status: Closed (submitted)

Loading...