[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 2/2] x86: Set up framebuffer given by Multiboot2
On 09.02.2022 14:09, Tu Dinh Ngoc wrote: > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -551,16 +551,55 @@ struct boot_video_info { > extern struct boot_video_info boot_vid_info; > #endif > > -static void __init parse_video_info(void) > +static void __init parse_video_info(multiboot_info_t *mbi) The parameter can be pointer-to-const afaict. > { > #ifdef CONFIG_VIDEO > struct boot_video_info *bvi = &bootsym(boot_vid_info); > > + /* > + * fb detection will be in this order: > + * - efifb (as efifb probe sets a new GOP mode before parse_video_info > is called, > + * we must use this mode instead of the one given by mbifb) > + * - mbifb > + * - vesafb > + */ This ordering needs justification, first and foremost because this way you risk regressions when VESAFB data is also available. There would be no such risk if you made mbifb lowest priority. Style: Comments should start with an upper case letter. There are very few exceptions to this (e.g. when a comment starts with an identifier referring elsewhere), but here there's no problem with starting the comment "FB detection ...". > /* vga_console_info is filled directly on EFI platform. */ > if ( efi_enabled(EFI_BOOT) ) > return; > > - if ( (bvi->orig_video_isVGA == 1) && (bvi->orig_video_mode == 3) ) > + if ( mbi->flags & MBI_FB ) Even with MBI_FB's value corrected in patch 1 - what about the flag being set from an MB1 bootloader? I'd be hesitant to trust that the data is dependable upon everywhere. > + { > + uint64_t lfb_rgb_bitmap = 0; I don't think you really need this initializer. > + vga_console_info.video_type = XEN_VGATYPE_VESA_LFB; > + vga_console_info.u.vesa_lfb.width = mbi->fb.width; > + vga_console_info.u.vesa_lfb.height = mbi->fb.height; > + vga_console_info.u.vesa_lfb.bytes_per_line = mbi->fb.pitch; > + vga_console_info.u.vesa_lfb.bits_per_pixel = mbi->fb.bpp; > + vga_console_info.u.vesa_lfb.lfb_base = mbi->fb.addr; > + vga_console_info.u.vesa_lfb.lfb_size = (mbi->fb.pitch * > mbi->fb.height + 0xffff) >> 16; > + > + vga_console_info.u.vesa_lfb.red_pos = mbi->fb.red_pos; > + vga_console_info.u.vesa_lfb.red_size = mbi->fb.red_size; > + lfb_rgb_bitmap |= (((uint64_t)1 << mbi->fb.red_size) - 1) << > mbi->fb.red_pos; 1ull is both shorter and avoids using a cast. > + vga_console_info.u.vesa_lfb.green_pos = mbi->fb.green_pos; > + vga_console_info.u.vesa_lfb.green_size = mbi->fb.green_size; > + lfb_rgb_bitmap |= (((uint64_t)1 << mbi->fb.green_size) - 1) << > mbi->fb.green_pos; > + vga_console_info.u.vesa_lfb.blue_pos = mbi->fb.blue_pos; > + vga_console_info.u.vesa_lfb.blue_size = mbi->fb.blue_size; > + lfb_rgb_bitmap |= (((uint64_t)1 << mbi->fb.blue_size) - 1) << > mbi->fb.blue_pos; > + > + /* assume non-weird bit format */ > + vga_console_info.u.vesa_lfb.rsvd_pos = > find_first_zero_bit(&lfb_rgb_bitmap, sizeof(lfb_rgb_bitmap) * __CHAR_BIT__); > + vga_console_info.u.vesa_lfb.rsvd_size = mbi->fb.bpp - > mbi->fb.red_size - mbi->fb.green_size - mbi->fb.blue_size; The comment really is about this 2nd assignment afaict, so it would better move down. I'm not convinced "assume" is enough, though. I think the data should simply not be used if the color placement doesn't match our expectations. Also these are overly long lines again. Finally if lfb_rgb_bitmap was "unsigned long" (and hence still at least 64 bits, as we're on a 64-bit architecture) you could use the simpler (because not requiring the address to be taken) find_first_set_bit(~lfb_rgb_bitmap), provided of course lfb_rgb_bitmap doesn't have all bits set. > + if (vga_console_info.u.vesa_lfb.rsvd_pos >= mbi->fb.bpp || > vga_console_info.u.vesa_lfb.rsvd_size < 0) { Style: Missing blanks, line length, and brace placement. Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |