On 06/29/16 15:24, Ruiyu Ni wrote:
> The patch serials refine the FrameBufferBltLib in OptionRomPkg and move it to
> MdeModulePkg. Based on the library, a generic GOP driver GraphicsOutputDxe
> is developed and added to MdeModulePkg.
>
> Ruiyu Ni (9):
> OptionRomPkg: Refine FrameBufferBltLib to use UINT8* instead of VOID*
> OptionRomPkg: Add video move test case to BltLibSample application
> OptionRomPkg/BltLib: Fix a bug in BltVideoToVideo operation
> OptionRomPkg: Remove GopBltLib instance
> OptionRomPkg: Refine BltLib to FrameBufferBltLib
> MdeModulePkg: Move OptionRomPkg/FrameBufferBltLib to MdeModulePkg
> OvmfPkg: QemuVideoDxe uses MdeModulePkg/FrameBufferLib
> MdePkg/GraphicsInfoHob: Add GraphicsDeviceInfo HOB GUID and structure
> MdeModulePkg: Add GraphicsOutputDxe driver.
I don't have much of an opinion about (nor expertise in)
FrameBufferBltLib. I remember however that you guys (Jordan and Ray)
discussed it in-depth a few months (years? :)) ago -- is that right?
So, assuming the new library interface is acceptable to everyone, I'm
happy to review patch 7/9 (the OvmfPkg adaptation). But perhaps the
interface will be revised due to Jordan's feedback, for v2, so it might
not be best for me to rush reviewing 7/9.
Jordan, can you review patch 5/9 first, for the interface changes?
Hmmm... Actually, I do have some comments already.
(1) First, for patch #5: I think the "Configure" parameter of
FrameBufferBltConfigure should not have type VOID*. Instead, the
FRAMEBUFFER_CONFIGURE type name (not the actual definition of the type!)
should be exposed in a public header. This is called "incomplete type"
in C. It's also called "opaque struct" more commonly.
The point is that clients of the library will be able to use pointers to
FRAMEBUFFER_CONFIGURE, but they won't know the fields and the size of
FRAMEBUFFER_CONFIGURE. This is a very big improvement above VOID,
because for VOID*, the compiler can do no type-checking. For
FRAMEBUFFER_CONFIGURE*, it can.
An example can be seen in
"MdePkg/Include/Library/OrderedCollectionLib.h": the type is called
ORDERED_COLLECTION.
(2) Second, in patch #7 I don't think Configure and ConfigureSize are
good names for the new fields in QEMU_VIDEO_PRIVATE_DATA.
"BltLibConfiguration" sounds better to me.
(3) For patch #6, I think FrameBufferBltLib.inf should get a new FILE_GUID.
(4) Again for patch #7: only "OvmfPkgIa32X64.dsc" was modified. I think
the other two DSC files were not added with "git add" before commit?
(5) In patch #7: the way "Private->Configure" is allocated in
QemuVideoGraphicsOutputSetMode() is not correct. It is correct for the
very first call, but for every further call, it has potential to leak
memory. Namely, if RETURN_BUFFER_TOO_SMALL is returned, that doesn't
guarantee that Private->Configure is currently NULL -- maybe a buffer
exists there (from an earlier allocation), it's just too small. So
instead of AllocatePool(), the area should be reallocated.
Admittedly, this doesn't make a lot of sense if the config buffer has a
fixed (opaque) type, FRAMEBUFFER_CONFIGURE -- the client cannot know the
actual size of that struct, but whatever the size, it never changes.
So, ultimately, I think one of the following two should be done:
- either consolidate on a public, opaque (but fixed size)
FRAMEBUFFER_CONFIGURE configuration buffer type, and then preallocate it
once, *outside* of QemuVideoGraphicsOutputSetMode(),
- or shoot for maximum flexibility, where the config buffer size *can*
change from mode to mode. In this case stick with the (VOID*) indeed,
but then also call FreePool() -- conditionally -- before AllocatePool().
(6) As much as I dislike saying this, I'll say it: if we want to remain
bisection-clean, then the series should be reworked a bit. First, the
new library class and instance should be created independently, as a
customized copy of the current library. Then all clients should be
flipped over to the new library. Finally the old (now unused) library
(class and instance) should be removed.
As-is, this series breaks the OVMF build when checking out the tree at
patch #5 or patch #6, I think.
Furthermore, the patch that creates the customized copy of the library
should be formatted with "--find-copies-harder", so that the new code is
expressed in terms of copy commands + diffs, not as brand new code.
(OTOH, this is not strictly necessary if you push the series to github
-- we can then fetch it and pass "--find-copies-harder" locally.)
Thanks
Laszlo
> MdeModulePkg/Include/Library/FrameBufferBltLib.h | 90 +++
> .../Library/FrameBufferBltLib/FrameBufferBltLib.c | 709 +++++++++++++++++++
> .../FrameBufferBltLib/FrameBufferBltLib.inf | 5 +-
> MdeModulePkg/MdeModulePkg.dec | 4 +
> MdeModulePkg/MdeModulePkg.dsc | 3 +
> .../Console/GraphicsOutputDxe/ComponentName.c | 190 ++++++
> .../Console/GraphicsOutputDxe/GraphicsOutput.c | 732 ++++++++++++++++++++
> .../Console/GraphicsOutputDxe/GraphicsOutput.h | 59 ++
> .../GraphicsOutputDxe/GraphicsOutputDxe.inf | 55 +-
> MdePkg/Include/Guid/GraphicsInfoHob.h | 17 +-
> MdePkg/MdePkg.dec | 1 +
> .../Application/BltLibSample/BltLibSample.c | 145 ++--
> .../Application/BltLibSample/BltLibSample.inf | 5 +-
> OptionRomPkg/Include/Library/BltLib.h | 259 -------
> .../Library/FrameBufferBltLib/FrameBufferBltLib.c | 750
> ---------------------
> OptionRomPkg/Library/GopBltLib/GopBltLib.c | 455 -------------
> OptionRomPkg/Library/GopBltLib/GopBltLib.inf | 37 -
> OptionRomPkg/OptionRomPkg.dec | 11 +-
> OptionRomPkg/OptionRomPkg.dsc | 7 +-
> OvmfPkg/OvmfPkgIa32X64.dsc | 6 +-
> OvmfPkg/QemuVideoDxe/Gop.c | 38 +-
> OvmfPkg/QemuVideoDxe/Qemu.h | 4 +-
> OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 5 +-
> 23 files changed, 1969 insertions(+), 1618 deletions(-)
> create mode 100644 MdeModulePkg/Include/Library/FrameBufferBltLib.h
> create mode 100644 MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLib.c
> rename {OptionRomPkg =>
> MdeModulePkg}/Library/FrameBufferBltLib/FrameBufferBltLib.inf (87%)
> create mode 100644
> MdeModulePkg/Universal/Console/GraphicsOutputDxe/ComponentName.c
> create mode 100644
> MdeModulePkg/Universal/Console/GraphicsOutputDxe/GraphicsOutput.c
> create mode 100644
> MdeModulePkg/Universal/Console/GraphicsOutputDxe/GraphicsOutput.h
> copy OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf =>
> MdeModulePkg/Universal/Console/GraphicsOutputDxe/GraphicsOutputDxe.inf (53%)
> delete mode 100644 OptionRomPkg/Include/Library/BltLib.h
> delete mode 100644 OptionRomPkg/Library/FrameBufferBltLib/FrameBufferBltLib.c
> delete mode 100644 OptionRomPkg/Library/GopBltLib/GopBltLib.c
> delete mode 100644 OptionRomPkg/Library/GopBltLib/GopBltLib.inf
>
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
