All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Môshe van der Sterre" <me@moshe.nl>
To: Hans de Goede <hdegoede@redhat.com>
Cc: linux-efi@vger.kernel.org, linux-fbdev@vger.kernel.org,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
	dri-devel@lists.freedesktop.org,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: Re: [PATCH 2/2] efifb: Copy the ACPI BGRT boot graphics to the framebuffer
Date: Mon, 18 Jun 2018 10:53:30 +0200	[thread overview]
Message-ID: <c8d28a61-46ed-c066-5a23-e74e9327380c@moshe.nl> (raw)
In-Reply-To: <20180617153235.16219-3-hdegoede@redhat.com>

Hi Hans,

On 06/17/2018 05:32 PM, Hans de Goede wrote:
> On systems where fbcon is configured for deferred console takeover, the
> intend is for the framebuffer to show the boot graphics (e.g a vendor
> logo) until some message (e.g. an error) is printed or a graphical
> session takes over.
> 
> Some firmware however relies on the OS to show the boot graphics
> (indicated by bgrt_tab.status being 0) and the boot graphics may have
> been destroyed by e.g. the grub boot menu.

It may be clearer to just say that the boot graphics may have been destroyed. The reference to the status field and firmware expectations only confuses the intention of this patch, imho.
(This ties in to what I say below)

> This patch adds support to efifb to show the boot graphics and
> automatically enables this when fbcon is configured for deferred
> console takeover.
> 
> +	/*
> +	 * We do not check bgrt_tab.status here because this seems to only
> +	 * reflect if the firmware has shown the boot graphics at all, if it
> +	 * later got destroyed by something status will still be 1.
> +	 * Since we draw the exact same graphic at the exact same place this
> +	 * will not lead to any tearing if the boot graphic is already there.
> +	 */

I agree that ignoring bgrt_tab.status is the absolute best option.

The status (valid-bit) can, in the real world, be any value with any meaning.
I checked this on a few machines as part of commit 66dbe99cfe30.
 - My workstation always has 0.
 - An old server that I checked always has 1.
 - My laptop has 1 if the boot is uninterrupted, 0 if I request the UEFI boot menu.

So, I have the same reservation about this comment as I have about the commit message. Imho, simply mentioning that the status field cannot be relied upon (in any case), would get the point across.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

WARNING: multiple messages have this Message-ID (diff)
From: "Môshe van der Sterre" <me@moshe.nl>
To: Hans de Goede <hdegoede@redhat.com>
Cc: linux-efi@vger.kernel.org, linux-fbdev@vger.kernel.org,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
	dri-devel@lists.freedesktop.org,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: Re: [PATCH 2/2] efifb: Copy the ACPI BGRT boot graphics to the framebuffer
Date: Mon, 18 Jun 2018 08:53:30 +0000	[thread overview]
Message-ID: <c8d28a61-46ed-c066-5a23-e74e9327380c@moshe.nl> (raw)
In-Reply-To: <20180617153235.16219-3-hdegoede@redhat.com>

Hi Hans,

On 06/17/2018 05:32 PM, Hans de Goede wrote:
> On systems where fbcon is configured for deferred console takeover, the
> intend is for the framebuffer to show the boot graphics (e.g a vendor
> logo) until some message (e.g. an error) is printed or a graphical
> session takes over.
> 
> Some firmware however relies on the OS to show the boot graphics
> (indicated by bgrt_tab.status being 0) and the boot graphics may have
> been destroyed by e.g. the grub boot menu.

It may be clearer to just say that the boot graphics may have been destroyed. The reference to the status field and firmware expectations only confuses the intention of this patch, imho.
(This ties in to what I say below)

> This patch adds support to efifb to show the boot graphics and
> automatically enables this when fbcon is configured for deferred
> console takeover.
> 
> +	/*
> +	 * We do not check bgrt_tab.status here because this seems to only
> +	 * reflect if the firmware has shown the boot graphics at all, if it
> +	 * later got destroyed by something status will still be 1.
> +	 * Since we draw the exact same graphic at the exact same place this
> +	 * will not lead to any tearing if the boot graphic is already there.
> +	 */

I agree that ignoring bgrt_tab.status is the absolute best option.

The status (valid-bit) can, in the real world, be any value with any meaning.
I checked this on a few machines as part of commit 66dbe99cfe30.
 - My workstation always has 0.
 - An old server that I checked always has 1.
 - My laptop has 1 if the boot is uninterrupted, 0 if I request the UEFI boot menu.

So, I have the same reservation about this comment as I have about the commit message. Imho, simply mentioning that the status field cannot be relied upon (in any case), would get the point across.

  parent reply	other threads:[~2018-06-18  8:53 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-17 15:32 [PATCH 0/2] efifb: Copy the ACPI BGRT boot graphics to the Hans de Goede
2018-06-17 15:32 ` Hans de Goede
2018-06-17 15:32 ` [PATCH 1/2] efi/bgrt: Drop __initdata from bgrt_image_size Hans de Goede
2018-06-17 15:32   ` Hans de Goede
2018-06-18  6:42   ` Ard Biesheuvel
2018-06-18  6:42     ` Ard Biesheuvel
2018-06-17 15:32 ` [PATCH 2/2] efifb: Copy the ACPI BGRT boot graphics to the framebuffer Hans de Goede
2018-06-17 15:32   ` Hans de Goede
2018-06-18  7:36   ` Ard Biesheuvel
2018-06-18  7:36     ` Ard Biesheuvel
2018-06-18  8:30     ` Hans de Goede
2018-06-18  8:30       ` Hans de Goede
2018-06-18  8:43       ` Ard Biesheuvel
2018-06-18  8:43         ` Ard Biesheuvel
2018-06-18  9:13         ` Hans de Goede
2018-06-18  9:13           ` Hans de Goede
2018-06-18 10:43           ` Ard Biesheuvel
2018-06-18 10:43             ` Ard Biesheuvel
2018-06-18  8:53   ` Môshe van der Sterre [this message]
2018-06-18  8:53     ` Môshe van der Sterre
2018-06-18  9:08     ` Hans de Goede
2018-06-18  9:08       ` Hans de Goede
2018-06-18  9:23 ` [PATCH 0/2] efifb: Copy the ACPI BGRT boot graphics to the Daniel Vetter
2018-06-18  9:23   ` Daniel Vetter
2018-06-18  9:30   ` Hans de Goede
2018-06-18  9:30     ` Hans de Goede

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c8d28a61-46ed-c066-5a23-e74e9327380c@moshe.nl \
    --to=me@moshe.nl \
    --cc=ard.biesheuvel@linaro.org \
    --cc=b.zolnierkie@samsung.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=hdegoede@redhat.com \
    --cc=linux-efi@vger.kernel.org \
    --cc=linux-fbdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.