[PATCH] efifb: BGRT: Add check for new BGRT status field rotation bits

[PATCH] efifb: BGRT: Add check for new BGRT status field rotation bits

[Hans de Goede]

Starting with ACPI 6.2 bits 1 and 2 of the BGRT status field are no longer
reserved. These bits are now used to indicate if the image needs to be
rotated before being displayed.

The efifb code does not support rotating the image before copying it to
the screen.

This commit adds a check for these new bits and if they are set leaves the
fb contents as is instead of trying to use the un-rotated BGRT image.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/video/fbdev/efifb.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index 9f39f0c360e0..dfa8dd47d19d 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -169,6 +169,11 @@ static void efifb_show_boot_graphics(struct fb_info *info)
 		return;
 	}
 
+	if (bgrt_tab.status & 0x06) {
+		pr_info("efifb: BGRT rotation bits set, not showing boot graphics\n");
+		return;
+	}
+
 	/* Avoid flashing the logo if we're going to print std probe messages */
 	if (console_loglevel > CONSOLE_LOGLEVEL_QUIET)
 		return;
-- 
2.21.0

Re: [PATCH] efifb: BGRT: Add check for new BGRT status field rotation bits

[Ard Biesheuvel]

On Wed, 29 May 2019 at 17:46, Hans de Goede <hdegoede@redhat.com> wrote:
>
> Starting with ACPI 6.2 bits 1 and 2 of the BGRT status field are no longer
> reserved. These bits are now used to indicate if the image needs to be
> rotated before being displayed.
>
> The efifb code does not support rotating the image before copying it to
> the screen.
>
> This commit adds a check for these new bits and if they are set leaves the
> fb contents as is instead of trying to use the un-rotated BGRT image.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>  drivers/video/fbdev/efifb.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index 9f39f0c360e0..dfa8dd47d19d 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -169,6 +169,11 @@ static void efifb_show_boot_graphics(struct fb_info *info)
>                 return;
>         }
>
> +       if (bgrt_tab.status & 0x06) {
> +               pr_info("efifb: BGRT rotation bits set, not showing boot graphics\n");
> +               return;
> +       }
> +
>         /* Avoid flashing the logo if we're going to print std probe messages */
>         if (console_loglevel > CONSOLE_LOGLEVEL_QUIET)
>                 return;
> --
> 2.21.0
>

Re: [PATCH] efifb: BGRT: Add check for new BGRT status field rotation bits

[Ard Biesheuvel]

On Mon, 10 Jun 2019 at 17:12, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Wed, 29 May 2019 at 17:46, Hans de Goede <hdegoede@redhat.com> wrote:
> >
> > Starting with ACPI 6.2 bits 1 and 2 of the BGRT status field are no longer
> > reserved. These bits are now used to indicate if the image needs to be
> > rotated before being displayed.
> >
> > The efifb code does not support rotating the image before copying it to
> > the screen.
> >
> > This commit adds a check for these new bits and if they are set leaves the
> > fb contents as is instead of trying to use the un-rotated BGRT image.
> >
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>

BTW should we make sure that this patch and the efi-bgrt patch get
merged at the same time? I guess the net result is just that we get
rid of some error in the log, but a rotated BMP will be ignored
otherwise. Or is it relevant for userland in some other way?

Re: [PATCH] efifb: BGRT: Add check for new BGRT status field rotation bits

[Hans de Goede]

Hi,

On 11-06-19 16:04, Ard Biesheuvel wrote:
> On Mon, 10 Jun 2019 at 17:12, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>
>> On Wed, 29 May 2019 at 17:46, Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> Starting with ACPI 6.2 bits 1 and 2 of the BGRT status field are no longer
>>> reserved. These bits are now used to indicate if the image needs to be
>>> rotated before being displayed.
>>>
>>> The efifb code does not support rotating the image before copying it to
>>> the screen.
>>>
>>> This commit adds a check for these new bits and if they are set leaves the
>>> fb contents as is instead of trying to use the un-rotated BGRT image.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>
>> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>
> 
> BTW should we make sure that this patch and the efi-bgrt patch get
> merged at the same time?

The 2 patches are related but merging them at the same time is not
necessary.

> I guess the net result is just that we get
> rid of some error in the log, but a rotated BMP will be ignored
> otherwise.

Right, worse case (if the bmp fits pre-rotation) it will be displayed
rotated. Note on the one machine I'm aware of which uses these bits
the bmp does not fit pre-rotation, so we end up triggering:

error:
         memunmap(bgrt_image);
         pr_warn("efifb: Ignoring BGRT: unexpected or invalid BMP data\n");
}

Which this patch replaces with hitting:

         if (bgrt_tab.status & 0x06) {
                 pr_info("efifb: BGRT rotation bits set, not showing boot graphics\n");
                 return;
         }

Instead. So at least on the one machine I know of this is 99% cosmetic.

> Or is it relevant for userland in some other way?

No.

Regards,

Hans

Re: [PATCH] efifb: BGRT: Add check for new BGRT status field rotation bits

[Ard Biesheuvel]

On Tue, 11 Jun 2019 at 16:24, Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 11-06-19 16:04, Ard Biesheuvel wrote:
> > On Mon, 10 Jun 2019 at 17:12, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >>
> >> On Wed, 29 May 2019 at 17:46, Hans de Goede <hdegoede@redhat.com> wrote:
> >>>
> >>> Starting with ACPI 6.2 bits 1 and 2 of the BGRT status field are no longer
> >>> reserved. These bits are now used to indicate if the image needs to be
> >>> rotated before being displayed.
> >>>
> >>> The efifb code does not support rotating the image before copying it to
> >>> the screen.
> >>>
> >>> This commit adds a check for these new bits and if they are set leaves the
> >>> fb contents as is instead of trying to use the un-rotated BGRT image.
> >>>
> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>
> >> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>
> >
> > BTW should we make sure that this patch and the efi-bgrt patch get
> > merged at the same time?
>
> The 2 patches are related but merging them at the same time is not
> necessary.
>
> > I guess the net result is just that we get
> > rid of some error in the log, but a rotated BMP will be ignored
> > otherwise.
>
> Right, worse case (if the bmp fits pre-rotation) it will be displayed
> rotated. Note on the one machine I'm aware of which uses these bits
> the bmp does not fit pre-rotation, so we end up triggering:
>
> error:
>          memunmap(bgrt_image);
>          pr_warn("efifb: Ignoring BGRT: unexpected or invalid BMP data\n");
> }
>

Doesn't that mean we may now end up breaking 'quiet', by exchanging a
pr_notice() in the efi-bgrt driver for a pr_warn() in this one?

> Which this patch replaces with hitting:
>
>          if (bgrt_tab.status & 0x06) {
>                  pr_info("efifb: BGRT rotation bits set, not showing boot graphics\n");
>                  return;
>          }
>
> Instead. So at least on the one machine I know of this is 99% cosmetic.
>
> > Or is it relevant for userland in some other way?
>
> No.
>
> Regards,
>
> Hans

Re: [PATCH] efifb: BGRT: Add check for new BGRT status field rotation bits

[Hans de Goede]

Hi,

On 11-06-19 16:37, Ard Biesheuvel wrote:
> On Tue, 11 Jun 2019 at 16:24, Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 11-06-19 16:04, Ard Biesheuvel wrote:
>>> On Mon, 10 Jun 2019 at 17:12, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>>
>>>> On Wed, 29 May 2019 at 17:46, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>
>>>>> Starting with ACPI 6.2 bits 1 and 2 of the BGRT status field are no longer
>>>>> reserved. These bits are now used to indicate if the image needs to be
>>>>> rotated before being displayed.
>>>>>
>>>>> The efifb code does not support rotating the image before copying it to
>>>>> the screen.
>>>>>
>>>>> This commit adds a check for these new bits and if they are set leaves the
>>>>> fb contents as is instead of trying to use the un-rotated BGRT image.
>>>>>
>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>
>>>> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>>
>>>
>>> BTW should we make sure that this patch and the efi-bgrt patch get
>>> merged at the same time?
>>
>> The 2 patches are related but merging them at the same time is not
>> necessary.
>>
>>> I guess the net result is just that we get
>>> rid of some error in the log, but a rotated BMP will be ignored
>>> otherwise.
>>
>> Right, worse case (if the bmp fits pre-rotation) it will be displayed
>> rotated. Note on the one machine I'm aware of which uses these bits
>> the bmp does not fit pre-rotation, so we end up triggering:
>>
>> error:
>>           memunmap(bgrt_image);
>>           pr_warn("efifb: Ignoring BGRT: unexpected or invalid BMP data\n");
>> }
>>
> 
> Doesn't that mean we may now end up breaking 'quiet', by exchanging a
> pr_notice() in the efi-bgrt driver for a pr_warn() in this one?

quiet has only logged pr_err and more severe for as long as I can
remember, so notice / warn does not matter for quiet.

Also for flickerfree boot I've made the quiet cut-off configurable
(CONFIG_CONSOLE_LOGLEVEL_QUIET) and in Fedora at least we set it to only
show messages at KERN_CRIT and more severe levels, since there are
simply too many false-positive pr_err messages in the kernel and
I quickly got tired of the whack-a-mole game.

Regards,

Hans

Re: [PATCH] efifb: BGRT: Add check for new BGRT status field rotation bits

[Bartlomiej Zolnierkiewicz]

On 6/11/19 4:24 PM, Hans de Goede wrote:
> Hi,
> 
> On 11-06-19 16:04, Ard Biesheuvel wrote:
>> On Mon, 10 Jun 2019 at 17:12, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>
>>> On Wed, 29 May 2019 at 17:46, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> Starting with ACPI 6.2 bits 1 and 2 of the BGRT status field are no longer
>>>> reserved. These bits are now used to indicate if the image needs to be
>>>> rotated before being displayed.
>>>>
>>>> The efifb code does not support rotating the image before copying it to
>>>> the screen.
>>>>
>>>> This commit adds a check for these new bits and if they are set leaves the
>>>> fb contents as is instead of trying to use the un-rotated BGRT image.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>
>>> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>
>>
>> BTW should we make sure that this patch and the efi-bgrt patch get
>> merged at the same time?
> 
> The 2 patches are related but merging them at the same time is not
> necessary.
> 
>> I guess the net result is just that we get
>> rid of some error in the log, but a rotated BMP will be ignored
>> otherwise.
> 
> Right, worse case (if the bmp fits pre-rotation) it will be displayed
> rotated. Note on the one machine I'm aware of which uses these bits
> the bmp does not fit pre-rotation, so we end up triggering:
> 
> error:
>         memunmap(bgrt_image);
>         pr_warn("efifb: Ignoring BGRT: unexpected or invalid BMP data\n");
> }
> 
> Which this patch replaces with hitting:
> 
>         if (bgrt_tab.status & 0x06) {
>                 pr_info("efifb: BGRT rotation bits set, not showing boot graphics\n");
>                 return;
>         }
> 
> Instead. So at least on the one machine I know of this is 99% cosmetic.
> 
>> Or is it relevant for userland in some other way?
> 
> No.
> 
> Regards,
> 
> Hans

Patch queued for v5.3, thanks.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics