1. Patchew
  2. EDK2
  3. View series

[edk2] [PATCH] OvmfPkg/Gop: clear the screen to black in SetMode()

Ruiyu Ni posted 1 patch 6 years, 9 months ago
Failed in applying to current master (apply log)
OvmfPkg/QemuVideoDxe/Gop.c | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
[edk2] [PATCH] OvmfPkg/Gop: clear the screen to black in SetMode()
Posted by Ruiyu Ni 6 years, 9 months ago 
Today's implementation forgot to clear the screen to black in
SetMode(). It causes SCT SetMode() test fails.

The patch adds the clear screen operation in SetMode() to fix
the SCT failure.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 OvmfPkg/QemuVideoDxe/Gop.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/OvmfPkg/QemuVideoDxe/Gop.c b/OvmfPkg/QemuVideoDxe/Gop.c
index 512fd27acb..f573f7011a 100644
--- a/OvmfPkg/QemuVideoDxe/Gop.c
+++ b/OvmfPkg/QemuVideoDxe/Gop.c
@@ -1,7 +1,7 @@
 /** @file
   Graphics Output Protocol functions for the QEMU video controller.
 
-  Copyright (c) 2007 - 2017, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.<BR>
 
   This program and the accompanying materials
   are licensed and made available under the terms and conditions of the BSD License
@@ -196,9 +196,10 @@ Routine Description:
 
 --*/
 {
-  QEMU_VIDEO_PRIVATE_DATA    *Private;
-  QEMU_VIDEO_MODE_DATA       *ModeData;
-  RETURN_STATUS              Status;
+  QEMU_VIDEO_PRIVATE_DATA       *Private;
+  QEMU_VIDEO_MODE_DATA          *ModeData;
+  RETURN_STATUS                 Status;
+  EFI_GRAPHICS_OUTPUT_BLT_PIXEL Black;
 
   Private = QEMU_VIDEO_PRIVATE_DATA_FROM_GRAPHICS_OUTPUT_THIS (This);
 
@@ -271,7 +272,19 @@ Routine Description:
   }
   ASSERT (Status == RETURN_SUCCESS);
 
-  return EFI_SUCCESS;
+  //
+  // Per UEFI Spec, need to clear the visible portions of the output display to black.
+  //
+  ZeroMem (&Black, sizeof (Black));
+  return FrameBufferBlt (
+           Private->FrameBufferBltConfigure,
+           &Black,
+           EfiBltVideoFill,
+           0, 0,
+           0, 0,
+           This->Mode->Info->HorizontalResolution, This->Mode->Info->VerticalResolution,
+           0
+           );
 }
 
 EFI_STATUS
-- 
2.16.1.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] OvmfPkg/Gop: clear the screen to black in SetMode()
Posted by Laszlo Ersek 6 years, 9 months ago 
Hi Ray,

On 03/13/18 11:05, Ruiyu Ni wrote:
> Today's implementation forgot to clear the screen to black in
> SetMode(). It causes SCT SetMode() test fails.
> 
> The patch adds the clear screen operation in SetMode() to fix
> the SCT failure.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  OvmfPkg/QemuVideoDxe/Gop.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/OvmfPkg/QemuVideoDxe/Gop.c b/OvmfPkg/QemuVideoDxe/Gop.c
> index 512fd27acb..f573f7011a 100644
> --- a/OvmfPkg/QemuVideoDxe/Gop.c
> +++ b/OvmfPkg/QemuVideoDxe/Gop.c
> @@ -1,7 +1,7 @@
>  /** @file
>    Graphics Output Protocol functions for the QEMU video controller.
>  
> -  Copyright (c) 2007 - 2017, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.<BR>
>  
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of the BSD License
> @@ -196,9 +196,10 @@ Routine Description:
>  
>  --*/
>  {
> -  QEMU_VIDEO_PRIVATE_DATA    *Private;
> -  QEMU_VIDEO_MODE_DATA       *ModeData;
> -  RETURN_STATUS              Status;
> +  QEMU_VIDEO_PRIVATE_DATA       *Private;
> +  QEMU_VIDEO_MODE_DATA          *ModeData;
> +  RETURN_STATUS                 Status;
> +  EFI_GRAPHICS_OUTPUT_BLT_PIXEL Black;
>  
>    Private = QEMU_VIDEO_PRIVATE_DATA_FROM_GRAPHICS_OUTPUT_THIS (This);
>  
> @@ -271,7 +272,19 @@ Routine Description:
>    }
>    ASSERT (Status == RETURN_SUCCESS);
>  
> -  return EFI_SUCCESS;
> +  //
> +  // Per UEFI Spec, need to clear the visible portions of the output display to black.
> +  //
> +  ZeroMem (&Black, sizeof (Black));
> +  return FrameBufferBlt (
> +           Private->FrameBufferBltConfigure,
> +           &Black,
> +           EfiBltVideoFill,
> +           0, 0,
> +           0, 0,
> +           This->Mode->Info->HorizontalResolution, This->Mode->Info->VerticalResolution,
> +           0
> +           );
>  }
>  
>  EFI_STATUS
> 

Thanks for the patch!

What would you think of the following update: FrameBufferBlt() is
documented to return RETURN_INVALID_PARAMETER for "Invalid parameter
were passed in", and RETURN_SUCCESS otherwise ("The Blt operation was
performed successfully"). I suggest that we keep the "return
EFI_SUCCESS" in the end, just ASSERT() that FrameBufferBlt() succeeds.

(Even if there was a third possibility, i.e. for FrameBufferBlt() to
fail for a different reason, I think that should still not report that
SetMode() failed.)

If you agree with the idea, please update the patch before pushing it.
If you disagree with it, I don't insist.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] OvmfPkg/Gop: clear the screen to black in SetMode()
Posted by Ni, Ruiyu 6 years, 9 months ago 

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo
> Ersek
> Sent: Tuesday, March 13, 2018 9:10 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Subject: Re: [edk2] [PATCH] OvmfPkg/Gop: clear the screen to black in SetMode()
> 
> Hi Ray,
> 
> On 03/13/18 11:05, Ruiyu Ni wrote:
> > Today's implementation forgot to clear the screen to black in
> > SetMode(). It causes SCT SetMode() test fails.
> >
> > The patch adds the clear screen operation in SetMode() to fix the SCT
> > failure.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  OvmfPkg/QemuVideoDxe/Gop.c | 23 ++++++++++++++++++-----
> >  1 file changed, 18 insertions(+), 5 deletions(-)
> >
> > diff --git a/OvmfPkg/QemuVideoDxe/Gop.c b/OvmfPkg/QemuVideoDxe/Gop.c
> > index 512fd27acb..f573f7011a 100644
> > --- a/OvmfPkg/QemuVideoDxe/Gop.c
> > +++ b/OvmfPkg/QemuVideoDxe/Gop.c
> > @@ -1,7 +1,7 @@
> >  /** @file
> >    Graphics Output Protocol functions for the QEMU video controller.
> >
> > -  Copyright (c) 2007 - 2017, Intel Corporation. All rights
> > reserved.<BR>
> > +  Copyright (c) 2007 - 2018, Intel Corporation. All rights
> > + reserved.<BR>
> >
> >    This program and the accompanying materials
> >    are licensed and made available under the terms and conditions of
> > the BSD License @@ -196,9 +196,10 @@ Routine Description:
> >
> >  --*/
> >  {
> > -  QEMU_VIDEO_PRIVATE_DATA    *Private;
> > -  QEMU_VIDEO_MODE_DATA       *ModeData;
> > -  RETURN_STATUS              Status;
> > +  QEMU_VIDEO_PRIVATE_DATA       *Private;
> > +  QEMU_VIDEO_MODE_DATA          *ModeData;
> > +  RETURN_STATUS                 Status;
> > +  EFI_GRAPHICS_OUTPUT_BLT_PIXEL Black;
> >
> >    Private = QEMU_VIDEO_PRIVATE_DATA_FROM_GRAPHICS_OUTPUT_THIS
> (This);
> >
> > @@ -271,7 +272,19 @@ Routine Description:
> >    }
> >    ASSERT (Status == RETURN_SUCCESS);
> >
> > -  return EFI_SUCCESS;
> > +  //
> > +  // Per UEFI Spec, need to clear the visible portions of the output display to
> black.
> > +  //
> > +  ZeroMem (&Black, sizeof (Black));
> > +  return FrameBufferBlt (
> > +           Private->FrameBufferBltConfigure,
> > +           &Black,
> > +           EfiBltVideoFill,
> > +           0, 0,
> > +           0, 0,
> > +           This->Mode->Info->HorizontalResolution, This->Mode->Info-
> >VerticalResolution,
> > +           0
> > +           );
> >  }
> >
> >  EFI_STATUS
> >
> 
> Thanks for the patch!
> 
> What would you think of the following update: FrameBufferBlt() is documented
> to return RETURN_INVALID_PARAMETER for "Invalid parameter were passed in",
> and RETURN_SUCCESS otherwise ("The Blt operation was performed
> successfully"). I suggest that we keep the "return EFI_SUCCESS" in the end, just
> ASSERT() that FrameBufferBlt() succeeds.
> 
> (Even if there was a third possibility, i.e. for FrameBufferBlt() to fail for a
> different reason, I think that should still not report that
> SetMode() failed.)
I agree!

> 
> If you agree with the idea, please update the patch before pushing it.
> If you disagree with it, I don't insist.
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patchew 2.1-devel-8c64711

© 2016 - 2024 Red Hat, Inc.