From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 66E901A1E31 for ; Mon, 10 Oct 2016 07:56:14 -0700 (PDT) Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D86FAC04D292; Mon, 10 Oct 2016 14:56:13 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-14.phx2.redhat.com [10.3.116.14]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u9AEuCSG020165; Mon, 10 Oct 2016 10:56:12 -0400 To: Ruiyu Ni , edk2-devel@ml01.01.org References: <20161009084953.58512-1-ruiyu.ni@intel.com> <20161009084953.58512-6-ruiyu.ni@intel.com> Cc: Jordan Justen , Ard Biesheuvel From: Laszlo Ersek Message-ID: <0f514b21-0a22-d7a2-2433-1cd32224b3c5@redhat.com> Date: Mon, 10 Oct 2016 16:56:11 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: <20161009084953.58512-6-ruiyu.ni@intel.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Mon, 10 Oct 2016 14:56:13 +0000 (UTC) Subject: Re: [PATCH v2 5/5] OvmfPkg: QemuVideoDxe uses MdeModulePkg/FrameBufferLib X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 10 Oct 2016 14:56:14 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 10/09/16 10:49, Ruiyu Ni wrote: > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ruiyu Ni > Cc: Laszlo Ersek > Cc: Jordan Justen > --- > OvmfPkg/OvmfPkgIa32.dsc | 6 ++--- > OvmfPkg/OvmfPkgIa32X64.dsc | 6 ++--- > OvmfPkg/OvmfPkgX64.dsc | 5 +--- > OvmfPkg/QemuVideoDxe/Gop.c | 47 ++++++++++++++++++++++++++++------- > OvmfPkg/QemuVideoDxe/Qemu.h | 17 ++++++++----- > OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 5 ++-- > 6 files changed, 57 insertions(+), 29 deletions(-) (1) This patch will break ArmVirtPkg, because the following two additional DSC files include QemuVideoDxe.inf: ArmVirtPkg/ArmVirtQemu.dsc ArmVirtPkg/ArmVirtQemuKernel.dsc So please split up this patch as follows: - patch 5 -- FrameBufferBltLib class resolution for ArmVirtPkg (2 DSCs), - patch 6 -- FrameBufferBltLib class resolution for OvmfPkg (3 DSCs), - patch 7 -- QemuVideoDxe changes. > diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc > index 7213197..13c996f 100644 > --- a/OvmfPkg/OvmfPkgIa32.dsc > +++ b/OvmfPkg/OvmfPkgIa32.dsc > @@ -115,6 +115,7 @@ [LibraryClasses] > LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf > !endif > CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf > + FrameBufferBltLib|MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLib.inf > > !ifdef $(SOURCE_DEBUG_ENABLE) > PeCoffExtraActionLib|SourceLevelDebugPkg/Library/PeCoffExtraActionLibDebug/PeCoffExtraActionLibDebug.inf > @@ -633,10 +634,7 @@ [Components] > MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf > MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf > > - OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf { > - > - BltLib|OptionRomPkg/Library/FrameBufferBltLib/FrameBufferBltLib.inf > - } > + OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > OvmfPkg/VirtioGpuDxe/VirtioGpu.inf > > # > diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc > index c27024a..3a90f6f 100644 > --- a/OvmfPkg/OvmfPkgIa32X64.dsc > +++ b/OvmfPkg/OvmfPkgIa32X64.dsc > @@ -120,6 +120,7 @@ [LibraryClasses] > LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf > !endif > CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf > + FrameBufferBltLib|MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLib.inf > > !ifdef $(SOURCE_DEBUG_ENABLE) > PeCoffExtraActionLib|SourceLevelDebugPkg/Library/PeCoffExtraActionLibDebug/PeCoffExtraActionLibDebug.inf > @@ -642,10 +643,7 @@ [Components.X64] > MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf > MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf > > - OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf { > - > - BltLib|OptionRomPkg/Library/FrameBufferBltLib/FrameBufferBltLib.inf > - } > + OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > OvmfPkg/VirtioGpuDxe/VirtioGpu.inf > > # > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc > index c34b266..ea8ed7f 100644 > --- a/OvmfPkg/OvmfPkgX64.dsc > +++ b/OvmfPkg/OvmfPkgX64.dsc > @@ -640,10 +640,7 @@ [Components] > MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf > MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf > > - OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf { > - > - BltLib|OptionRomPkg/Library/FrameBufferBltLib/FrameBufferBltLib.inf > - } > + OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > OvmfPkg/VirtioGpuDxe/VirtioGpu.inf > > # (2) I think you missed the FrameBufferBltLib resolution in "OvmfPkgX64.dsc". > diff --git a/OvmfPkg/QemuVideoDxe/Gop.c b/OvmfPkg/QemuVideoDxe/Gop.c > index 18d0779..532f20e 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 - 2010, Intel Corporation. All rights reserved.
> + Copyright (c) 2007 - 2016, Intel Corporation. All rights reserved.
> > This program and the accompanying materials > are licensed and made available under the terms and conditions of the BSD License > @@ -14,8 +14,6 @@ > **/ > > #include "Qemu.h" > -#include > -#include > > STATIC > VOID > @@ -159,7 +157,7 @@ Routine Description: > { > QEMU_VIDEO_PRIVATE_DATA *Private; > QEMU_VIDEO_MODE_DATA *ModeData; > -// UINTN Count; > + RETURN_STATUS Status; > > Private = QEMU_VIDEO_PRIVATE_DATA_FROM_GRAPHICS_OUTPUT_THIS (This); > > @@ -201,10 +199,32 @@ Routine Description: > > QemuVideoCompleteModeData (Private, This->Mode); > > - BltLibConfigure ( > - (VOID*)(UINTN) This->Mode->FrameBufferBase, > - This->Mode->Info > - ); > + // > + // Allocate when using first time. > + // > + if (Private->FrameBufferBltConfigure == NULL) { > + Status = FrameBufferBltConfigure ( > + (VOID*) (UINTN) This->Mode->FrameBufferBase, > + This->Mode->Info, > + Private->FrameBufferBltConfigure, > + &Private->FrameBufferBltConfigureSize > + ); > + ASSERT (Status == RETURN_BUFFER_TOO_SMALL); > + Private->FrameBufferBltConfigure = > + AllocatePool (Private->FrameBufferBltConfigureSize); > + } > + > + // > + // Create the configuration for FrameBufferBltLib > + // > + ASSERT (Private->FrameBufferBltConfigure != NULL); > + Status = FrameBufferBltConfigure ( > + (VOID*) (UINTN) This->Mode->FrameBufferBase, > + This->Mode->Info, > + Private->FrameBufferBltConfigure, > + &Private->FrameBufferBltConfigureSize > + ); > + ASSERT (Status == RETURN_SUCCESS); > > return EFI_SUCCESS; > } > @@ -254,7 +274,9 @@ Returns: > { > EFI_STATUS Status; > EFI_TPL OriginalTPL; > + QEMU_VIDEO_PRIVATE_DATA *Private; > > + Private = QEMU_VIDEO_PRIVATE_DATA_FROM_GRAPHICS_OUTPUT_THIS (This); > // > // We have to raise to TPL Notify, so we make an atomic write the frame buffer. > // We would not want a timer based event (Cursor, ...) to come in while we are > @@ -267,7 +289,8 @@ Returns: > case EfiBltBufferToVideo: > case EfiBltVideoFill: > case EfiBltVideoToVideo: > - Status = BltLibGopBlt ( > + Status = FrameBufferBlt ( > + Private->FrameBufferBltConfigure, > BltBuffer, > BltOperation, > SourceX, > @@ -327,6 +350,8 @@ QemuVideoGraphicsOutputConstructor ( > Private->GraphicsOutput.Mode->MaxMode = (UINT32) Private->MaxMode; > Private->GraphicsOutput.Mode->Mode = GRAPHICS_OUTPUT_INVALIDE_MODE_NUMBER; > Private->LineBuffer = NULL; > + Private->FrameBufferBltConfigure = NULL; > + Private->FrameBufferBltConfigureSize = 0; > > // > // Initialize the hardware > @@ -374,6 +399,10 @@ Returns: > FreePool (Private->LineBuffer); > } > > + if (Private->FrameBufferBltConfigure != NULL) { > + FreePool (Private->FrameBufferBltConfigure); > + } > + > if (Private->GraphicsOutput.Mode != NULL) { > if (Private->GraphicsOutput.Mode->Info != NULL) { > gBS->FreePool (Private->GraphicsOutput.Mode->Info); > diff --git a/OvmfPkg/QemuVideoDxe/Qemu.h b/OvmfPkg/QemuVideoDxe/Qemu.h > index 52ee20d..8d92ebc 100644 > --- a/OvmfPkg/QemuVideoDxe/Qemu.h > +++ b/OvmfPkg/QemuVideoDxe/Qemu.h > @@ -1,7 +1,7 @@ > /** @file > QEMU Video Controller Driver > > - Copyright (c) 2006 - 2010, Intel Corporation. All rights reserved.
> + Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.
> This program and the accompanying materials > are licensed and made available under the terms and conditions of the BSD License > which accompanies this distribution. The full text of the license may be found at > @@ -35,8 +35,10 @@ > #include > #include > #include > +#include > > #include > +#include > > // > // QEMU Video PCI Configuration Header values > @@ -50,11 +52,12 @@ > // QEMU Vide Graphical Mode Data > // > typedef struct { > - UINT32 InternalModeIndex; // points into card-specific mode table > - UINT32 HorizontalResolution; > - UINT32 VerticalResolution; > - UINT32 ColorDepth; > - UINT32 RefreshRate; > + UINT32 InternalModeIndex; // points into card-specific mode table > + UINT32 HorizontalResolution; > + UINT32 VerticalResolution; > + UINT32 ColorDepth; > + UINT32 RefreshRate; > + FRAME_BUFFER_CONFIGURE *FrameBufferConfigure; > } QEMU_VIDEO_MODE_DATA; > > #define PIXEL_RED_SHIFT 0 (3) This hunk is unnecessary; you never reference the new field QEMU_VIDEO_MODE_DATA.FrameBufferConfigure. The rest of this patch looks okay to me. Thanks Laszlo > @@ -119,6 +122,8 @@ typedef struct { > > UINT8 *LineBuffer; > QEMU_VIDEO_VARIANT Variant; > + FRAME_BUFFER_CONFIGURE *FrameBufferBltConfigure; > + UINTN FrameBufferBltConfigureSize; > } QEMU_VIDEO_PRIVATE_DATA; > > /// > diff --git a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > index ce1ff93..affb6ff 100644 > --- a/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > +++ b/OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > @@ -2,7 +2,7 @@ > # This driver is a sample implementation of the Graphics Output Protocol for > # the QEMU (Cirrus Logic 5446) video controller. > # > -# Copyright (c) 2006 - 2011, Intel Corporation. All rights reserved.
> +# Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.
> # > # This program and the accompanying materials > # are licensed and made available under the terms and conditions of the BSD License > @@ -44,12 +44,13 @@ [Sources.Ia32, Sources.X64] > > [Packages] > MdePkg/MdePkg.dec > + MdeModulePkg/MdeModulePkg.dec > OptionRomPkg/OptionRomPkg.dec > OvmfPkg/OvmfPkg.dec > > [LibraryClasses] > BaseMemoryLib > - BltLib > + FrameBufferBltLib > DebugLib > DevicePathLib > MemoryAllocationLib >