On 2015-10-20 20:28:53, Ni, Ruiyu wrote: > Jordan, > The new library only exposes two APIs: FrameBufferBltConfigure and > FrameBufferBlt. I like the new design very much! It's very clean > now! > > I also like the FrameBufferBltConfigure() to require caller supply > the buffer so that we do not need to have an additional API like > FrameBufferFreeConfigure().
I also like that a runtime application, like maybe an OS loader could still use the library at runtime if it allocates the memory as RuntimeData. > I also like the idea to merge all the Blt operation to a single > FrameBufferBlt(). My main reason for not wanting to do that with BltLib is that I was hoping BltLib could make the Blt functions easier to use by providing extra functions with less parameters. > I also like the idea to remove the GOP protocol version of BltLib. Well, I don't think we need to remove BltLib or the GopBltLib from under OptionRomPkg. I still hope maybe we could design an easy to use BltLib in the future. I agree, that the current design under OptionRomPkg could be improved. > Two small comments: > Can you change the parameter name for FrameBufferBltConfigure() from > Buffer/BufferSize to Configure/ConfigureSize? > And change the parameter name for FrameBufferBlt() from Config to > Configure? So that we have consistent name for the frame buffer > configuration block. I prefer Config, or Configuration for the configuration buffer. I think Configure is the action of setting up the configuration, so it sounds good for the function name, but not for the buffer. I decided to check how often we use Config, Configure and Configuration in MdePkg and MdeModulePkg. $ git grep -P 'Config(?!u)' origin/master -- MdePkg MdeModulePkg | wc -l 3818 $ git grep -P 'Configure' origin/master -- MdePkg MdeModulePkg | wc -l 535 $ git grep -P 'Configuration' origin/master -- MdePkg MdeModulePkg | wc -l 1047 So, Config (not followed by a 'u' character) is used more often. But, it seems like all are okay. (By the way, 'git grep' is so useful! :) > Can you remove the EFIAPI for internal functions? such as > FrameBufferBltVideoFill. Ah, good catch. I made an updated branch with your suggestions, except I kept the Config name as I mention above: https://github.com/jljusten/edk2/commits/framebuffer-blt-lib-v2 -Jordan > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Jordan > Justen > Sent: Wednesday, October 21, 2015 8:57 AM > To: Ni, Ruiyu <ruiyu...@intel.com>; edk2-devel@lists.01.org > Subject: Re: [edk2] [Patch 6/8] OptionRomPkg/OvmfPkg: Remove > BltLib::BltConfigure API > > Ray, > > I've been thinking about this a bit. For the BltLib, I think you > mainly like the FrameBufferBltLib library instance. So, how about just > design the library interface just for this purpose, and call the > interface FrameBufferBltLib rather than BltLib? > > What do you think of these 3 patches? > > https://github.com/jljusten/edk2/commits/framebuffer-blt-lib > > -Jordan > > On 2015-08-23 23:15:34, Ni, Ruiyu wrote: > > Jordan, > > The mail is too long so let me summarize in below: (I changed all > > API name to remove "Lib") > > 1. Have four or six BLT APIs? > > four: BltVideoFill BltVideoToVideo BltVideoToBuffer BltBufferToVideo > > six: BltVideoFill BltVideoToVideo BltVideoToBuffer BltBufferToVideo > > BltVideoToBufferEx BltBufferToVideoEx > > I prefer 4 because it aligns to the GOP protocol 4 BLT operations > > and the parameters are also aligned. > > You prefer 6 for easy calling. > > > > Laszlo and Mike, > > any ideas? > > > > > > > > 2. BltConfigure/BltFreeConfiguration > > 2.a. I agree to use the handle to hold the shift/mask. But I need to > > see whether there is any concerns from Mike. > > 2.b. I suggest to use VOID * instead of EFI_HANDLE. Let's wait for > > Andrew/Mike's input. > > > > 3. VideoModeSetLib for more abstraction > > I agree to either change the driver name as you suggested, either in > > the other way to have a more generic GOP driver. Do you know any > > graphics driver developers? Can you ask them if the more generic GOP > > driver will satisfy their needs? > > > > Thanks, > > Ray > > > > _______________________________________________ > > 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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
