lkml.org 
[lkml]   [2013]   [Oct]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] x86/efi: Add EFI framebuffer earlyprintk support

    * Matt Fleming <matt@console-pimps.org> wrote:

    > On Thu, 10 Oct, at 07:28:44PM, Ingo Molnar wrote:
    > > Btw., could we perhaps remap the whole framebuffer at init time, or is it
    > > too large? If early_ioremap() fails for whatever reason then that will
    > > emit a WARN_ON(), which will recurse in a fairly nasty way ...
    >
    > The framebuffer memory will be quite large, so I don't think it makes
    > sense to map it all this early, because it's likely we'll run out of
    > fixmap entries.

    Fair enough.

    > > > +static __init void early_efi_clear_screen(void)
    > > > +{
    > > > + struct screen_info *si;
    > > > + int y;
    > > > +
    > > > + si = &boot_params.screen_info;
    > > > + for (y = 0; y < si->lfb_height; y++)
    > > > + early_efi_clear_line(y);
    > >
    > > Looks a bit superfluous to introduce 'si' just for that single use?
    >
    > I did this to reduce the length of the "for (y = 0..." line.

    But that line looks fine with that included. If it goes slightly above 80
    chars that's still OK IMHO.

    > > > +static void early_efi_write_char(void *dst, char c, int h)
    > > > +{
    > > > + const u8 *src;
    > > > + u32 fgcolor = 0xaaaaaa;
    > >
    > > That's RGB grey, right? Why not 0xffffff for a very visible white?
    >
    > The VGA earlyprintk code uses the equivalent grey, AFAIK, which is why I
    > picked this value.

    The VGA code should be changed to white too I suspect ;-)

    > > > + if (efi_y + font->height >= si->lfb_height) {
    > > > + early_efi_clear_screen();
    > > > + efi_y = 0;
    > >
    > > So, if I understand it correctly this clears the screen and starts at
    > > the top when we scroll off the bottom, right?
    > >
    > > That might make the recovery of oopses hard when the number of log
    > > lines is unlucky.
    > >
    > > Would scrolling a few lines up instead, via a well-calculated memcpy
    > > and memset be doable?
    >
    > Yeah we can do that. I thought about this originally but decided against
    > it because I figured it would complicate the code unnecessarily. But it
    > turned out to be fairly trivial.

    Cool!

    > > > + if (!font)
    > > > + return -ENODEV;
    > > > +
    > > > + early_efi_clear_screen();
    > >
    > > Assuming we implement scrolling above, here too it might make sense to
    > > scroll up the framebuffer - if the crash is early enough then some
    > > firmware and boot stub info might still be present in the framebuffer?
    >
    > It's possible that some info will be in the framebuffer, but we can't
    > begin writing immediately after the boot stub info because we don't know
    > the last xy coordinates the firmware wrote to.
    >
    > But yeah, leaving it intact and beginning our output from the last line
    > of the framebuffer makes more sense than clearing the screen entirely.

    Especially with scrolling it should not matter where the previous info is
    on the screen: if we start with a scroll event then we can make some space
    at the bottom and start our printout there, or so.

    Thanks,

    Ingo


    \
     
     \ /
      Last update: 2013-10-12 20:01    [W:7.798 / U:0.236 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site