On 2015-08-23 20:09:23, Ni, Ruiyu wrote:
> Jordan,
> reply embedded in the mail.
>
> Mike,
> Jordan proposed to add a new parameter Configuration in BltConfigure,
> instead of removing this API, to eliminate the global variables in
> library implementation. I think it's also a good solution and can
> benefit the library performance. What's your thoughts?
> EFI_STATUS
> EFIAPI
> BltConfigure (
> IN VOID *FrameBuffer,
> IN EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *FrameBufferInfo,
> OUT EFI_HANDLE *Configuration
> );
I also had considered:
EFI_STATUS
EFIAPI
BltLibConfigureGop (
IN EFI_GRAPHICS_OUTPUT_PROTOCOL *Gop,
OUT EFI_HANDLE *Configuration
);
Which would either configure based on the current GOP settings, or in
the case of GopBltLib, it could just save the pointer to the GOP
protocol and call the protocol Blt function.
> EFI_STATUS
> EFIAPI
> BltFreeConfiguration (
> IN EFI_HANDLE Configuration
> );
>
>
> On 2015/8/24 6:28, Jordan Justen wrote:
> > Previously you said to Laszlo in regard to keeping the BltLibVideoFill
> > function: "The reason I wanted to use the four APIs instead of only
> > one BltLibGopBlt is if user just wants to fill video, he can supply
> > less parameters."
> >
> > This is exactly why I think the current BltLibVideoToBltBuffer and
> > BltLibBufferToVideo are useful. They have less parameters, and should
> > be useful in a case where the buffer is sized exactly to the blit
> > operation.
> >
> > It is also very easy to implement in the library by just calling the
> > other 'Ex' functions.
> >
> > If you just want to avoid overlapping functions, then why not just
> > keep *only* the BltLibGopBlt function?
>
> True we can choose *only" keep the BltLibGopBlt function, instead of
> keeping the current four functions. We can assume the interfaces of
> GOP protocol defined in spec were carefully reviewed, then, why not
> just align to these interfaces. It's a little bit confusing when the
> protocol implementation of BltVideoToBltBuffer operation calls a
> BltVideoToBltBuffer*Ex* library API. After all for the additional
> parameters, caller can simply pass 0.
I was not serious about the suggestion to just have the single blit
function. My preference would be to leave all of the current
functions.
One of my goals for BltLib was that it would make it easier for code
to use the blit operations. Think of it similar to HobLib. The Build*
functions in that library make it easier to create HOBs.
But, I thought the BltLibGopBlt function was nice for a GOP driver to
use, so I added it to the library interface.
I do agree that BltLibConfigure was not the greatest design. I wanted
to make it easier to wrapper a GOP protocol as I mentioned above.
If we only have a single blit function, then it makes GopBltLib
pointless, whereas I was hoping that the BltLib interface could make
calling the GOP protocol a little easier and with more readable code.
> >> Q2: If removing BltConfigure, does that mean the library will have
> >> to check to see if the mode is different? what's the benefit?
> >>
> >> After removing BltConfigure(), the library will calculate the
> >> shift/mask every time when doing the BLT operations. The benefit is
> >> to make the library stateless so it doesn't contain any state
> >> information so it can be called from different threads/CPUs.
> >
> > I don't think the GOP protocol blit is reentrant, so I'm still not
> > sure about where the requirement comes from.
>
> A GOP implementation can choose to utilize all the processors to fill
> the frame buffer simultaneously for good performance. I don't know
> whether there is any such GOP MP implementation but we'd better not to
> restrict it from the API interface level.
Hmm. I'm not convinced it is necessary. Do you know who originally
proposed that this would be a requirement?
I guess it would be nice for a multi-screen/framebuffer situation to
not have to call BltLibConfigure to blit to all of the screens.
If we add the hidden 'Configuration' struct mentioned above, I guess
it seems reasonable.
> > But, I did have another idea. What if BltLibConfigure has an output
> > EFI_HANDLE, and each BltLib function has this handle as it's first
> > parameter. Then we only need 1 parameter to each function, and it can
> > encapsulate any data needed for that configuration. For example, it
> > can still calculate the shifts and masks just once.
> >
> > Since the returned handle would likely be an allocated structure, I
> > think we should also then add a BltLibFreeConfiguration.
> >
> > In other words:
> >
> > EFI_STATUS
> > EFIAPI
> > BltLibConfigure (
> > IN VOID *FrameBuffer,
> > IN EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *FrameBufferInfo,
> > OUT EFI_HANDLE *Configuration
> > );
> >
> > EFI_STATUS
> > EFIAPI
> > BltLibFreeConfiguration (
> > IN EFI_HANDLE Configuration
> > );
>
> I agree with your idea, with just a little modification. Do not use
> EFI_HANDLE but directly use VOID * because EFI_HANDLE is a concept
> used in DXE Core protocol/handle database.
Could we get some confirmation from Mike or maybe Andrew on this? The
EFI spec defines EFI_HANDLE as VOID*, so why can't we use it instead
of VOID* for an opaque structure?
-Jordan
> See below
> SimpleTextInEx::RegisterKeystrokeNotify() also uses VOID * as
> NotifyHandle.
>
> typedef
> EFI_STATUS
> (EFIAPI *EFI_REGISTER_KEYSTROKE_NOTIFY) (
> IN EFI_SIMPLE_TEXT_INPUT_EX_PROTOCOL *This,
> IN EFI_KEY_DATA *KeyData,
> IN EFI_KEY_NOTIFY_FUNCTION KeyNotificationFunction,
> OUT VOID **NotifyHandle
> );
>
> The benefit of using a handle is it can avoid the shift/mask being
> re-calculated every time someone calling the library BLT functions.
>
> >
> >> For example strtok_r() is the stateless and reentrant version of strtok().
> >> char *strtok(char *str, const char *delim);
> >> char *strtok_r(char *str, const char *delim, char **saveptr);
> >>
> >> Q3: What if we design a VideoModeSetLib?
> >>
> >> GraphicsOutputDxe + null_VideoModeSetLib + BltLib -> current
> >> GraphicsOutputDxe
> >> GraphicsOutputDxe + qemu_VideoModeSetLib + BltLib -> current QemuVideoDxe
> >>
> >> Null_VideoModeSetLib only allows the video in the fixed mode
> >> supplied from PEI HOB Qemu_VideoModeSetLib allows the video in
> >> several modes for QEMU.
> >>
> >> Is my understanding correct?
> >> Do you have more usage scenarios about the VideoModeSetLib? For now
> >> I only see two and only two, considering the design complexity
> >> introduced by VideoModeSetLib, leave the QemuVideoDxe driver as is
> >> make people easier to understand.
> >
> > Maybe a better name for your new driver is PeiHobGopDxe?
> >
> > My point was that with the video mode setting lib, we could probably
> > produce a GOP driver for most video devices just by customizing the
> > two libraries. In that case, I think the GraphicsOutputDxe name makes
> > sense.
> 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?
> >
> > -Jordan
> >
> >> -----Original Message-----
> >> From: Justen, Jordan L
> >> Sent: Tuesday, August 18, 2015 1:18 AM
> >> To: Ni, Ruiyu <ruiyu...@intel.com>; edk2-devel@lists.01.org
> >> Cc: Ni, Ruiyu <ruiyu...@intel.com>; Laszlo Ersek <ler...@redhat.com>
> >> Subject: Re: [Patch 6/8] OptionRomPkg/OvmfPkg: Remove BltLib::BltConfigure
> >> API
> >>
> >> On 2015-08-17 06:45:27, Ruiyu Ni wrote:
> >>> The BltConfigure() API caches the video frame buffer meta data which
> >>> forbids the library to be implemented to support re-entry.
> >>
> >> How does this help? GraphicsOutputDxe will set a mode, and then use
> >> BltLib with that mode.
> >>
> >> I already asked this, and I had some other questions:
> >>
> >> http://article.gmane.org/gmane.comp.bios.edk2.devel/1209
> >>
> >> -Jordan
> >>
> >>> Contributed-under: TianoCore Contribution Agreement 1.0
> >>> Signed-off-by: Ruiyu Ni <ruiyu...@intel.com>
> >>> Cc: Laszlo Ersek <ler...@redhat.com>
> >>> Cc: Jordan Justen <jordan.l.jus...@intel.com>
>
> _______________________________________________
> 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
