Re: [PATCH 3/3] OvmfPkg/QemuVideoDxe: Frame buffer config size may change in new mode

[Laszlo Ersek <lersek@redhat.com>]

On 01/23/17 07:11, Ruiyu Ni wrote:
> https://bugzilla.tianocore.org/show_bug.cgi?id=339
> 
> The patch removes the assumption in QemuVideoDxe driver that it
> wrongly assumes the frame buffer configure size is the same in
> different video modes.
> The assumption is true in old FrameBufferBltLib but is false in
> new implementation.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
>  OvmfPkg/QemuVideoDxe/Gop.c | 47 +++++++++++++++++++++++++---------------------
>  1 file changed, 26 insertions(+), 21 deletions(-)

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

Thanks!
Laszlo

> diff --git a/OvmfPkg/QemuVideoDxe/Gop.c b/OvmfPkg/QemuVideoDxe/Gop.c
> index 5485ba3..359e921 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 - 2016, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2007 - 2017, 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
> @@ -189,30 +189,35 @@ Routine Description:
>    QemuVideoCompleteModeData (Private, This->Mode);
>  
>    //
> -  // Allocate when using first time.
> +  // Re-initialize the frame buffer configure when mode changes.
>    //
> -  if (Private->FrameBufferBltConfigure == NULL) {
> -    Status = FrameBufferBltConfigure (
> -               (VOID*) (UINTN) This->Mode->FrameBufferBase,
> -               This->Mode->Info,
> -               Private->FrameBufferBltConfigure,
> -               &Private->FrameBufferBltConfigureSize
> -               );
> -    ASSERT (Status == RETURN_BUFFER_TOO_SMALL);
> +  Status = FrameBufferBltConfigure (
> +             (VOID*) (UINTN) This->Mode->FrameBufferBase,
> +             This->Mode->Info,
> +             Private->FrameBufferBltConfigure,
> +             &Private->FrameBufferBltConfigureSize
> +             );
> +  if (Status == RETURN_BUFFER_TOO_SMALL) {
> +    //
> +    // Frame buffer configure may be larger in new mode.
> +    //
> +    if (Private->FrameBufferBltConfigure != NULL) {
> +      FreePool (Private->FrameBufferBltConfigure);
> +    }
>      Private->FrameBufferBltConfigure =
>        AllocatePool (Private->FrameBufferBltConfigureSize);
> -  }
> +    ASSERT (Private->FrameBufferBltConfigure != NULL);
>  
> -  //
> -  // Create the configuration for FrameBufferBltLib
> -  //
> -  ASSERT (Private->FrameBufferBltConfigure != NULL);
> -  Status = FrameBufferBltConfigure (
> -              (VOID*) (UINTN) This->Mode->FrameBufferBase,
> -              This->Mode->Info,
> -              Private->FrameBufferBltConfigure,
> -              &Private->FrameBufferBltConfigureSize
> -              );
> +    //
> +    // Create the configuration for FrameBufferBltLib
> +    //
> +    Status = FrameBufferBltConfigure (
> +                (VOID*) (UINTN) This->Mode->FrameBufferBase,
> +                This->Mode->Info,
> +                Private->FrameBufferBltConfigure,
> +                &Private->FrameBufferBltConfigureSize
> +                );
> +  }
>    ASSERT (Status == RETURN_SUCCESS);
>  
>    return EFI_SUCCESS;
>