Re: [Patch 0/9] Add DisplayUpdateProgressLib for capsules

["Kinney, Michael D" <michael.d.kinney@intel.com>]

Jiewen,

Responses below.

Mike

> -----Original Message-----
> From: Yao, Jiewen
> Sent: Wednesday, April 4, 2018 5:03 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> edk2-devel@lists.01.org
> Cc: Sean Brogan <sean.brogan@microsoft.com>; Zeng, Star
> <star.zeng@intel.com>; Dong, Eric
> <eric.dong@intel.com>; Wei, David
> <david.wei@intel.com>; Guo, Mang <mang.guo@intel.com>;
> Steele, Kelly <kelly.steele@intel.com>
> Subject: RE: [Patch 0/9] Add DisplayUpdateProgressLib
> for capsules
> 
> Thanks Mike.
> It is a good feature to add progress support.
> 
> Some thought below:
> 1) for EDKII_FIRMWARE_MANAGEMENT_PROGRESS_PROTOCOL
> Do you think if we need add full support for
> WatchdogTimer services?
> Such as WatchdogCode, WatchdogData?
> 
> Or should we add a version field for the protocol for
> future extension?
> 

A version field is a good idea.  Let's keep it as simple
as possible right now and only add new features based on
real use cases.

> 2) For EDKII_FIRMWARE_MANAGEMENT_PROGRESS_PROTOCOL and
> DisplayUpdateProgress(), we only add
> EFI_GRAPHICS_OUTPUT_BLT_PIXEL_UNION  *Color.
> 
> Do the color means the foreground color or background
> color?
> I found you treat as background in TextLib. But it
> seems complicated.
> Can we add both to avoid calculation?
> 

It is foreground color.  The text lib should use grey
as background like the graphics lib.

> That might be another reason to add version field for
> EDKII_FIRMWARE_MANAGEMENT_PROGRESS_PROTOCOL.
> 
> 3) According to lib naming conversion, we should use
> DisplayUpdateProgressLibGraphic, and
> DisplayUpdateProgressLibText. :-)
> 

Yes.  We should make that change.

> 4) For PerformFlashWriteWithProgress(), I found the
> caller (system firmware update) handles Progress
> (StartPercentage) and Progress (EndPercentage).
> 
> And the callee (Quark flash access lib) handles
> Progress (StartPercentage) but not Progress
> (EndPercentage).
> 
> That is a little weird to me. Can we add more detail
> description in the PerformFlashWriteWithProgress()
> function header to clarify who should handle
> StartPercentage and EndPercentage ?

The Progress() API passed into functions is OPTIONAL.
Which means it may never get called by the next layer
down.  As a result, if the top level always wants the
progress bar to be displayed correctly, it must always
call for start before and end after.

The Quark and Vlv2 versions of the lib should do the
start and the end.  That is a bug that I can fix.

> 
> 
> Thank you
> Yao Jiewen
> 
> > -----Original Message-----
> > From: Kinney, Michael D
> > Sent: Thursday, April 5, 2018 4:26 AM
> > To: edk2-devel@lists.01.org
> > Cc: Sean Brogan <sean.brogan@microsoft.com>; Zeng,
> Star
> > <star.zeng@intel.com>; Dong, Eric
> <eric.dong@intel.com>; Yao, Jiewen
> > <jiewen.yao@intel.com>; Wei, David
> <david.wei@intel.com>; Guo, Mang
> > <mang.guo@intel.com>; Steele, Kelly
> <kelly.steele@intel.com>
> > Subject: [Patch 0/9] Add DisplayUpdateProgressLib for
> capsules
> >
> > https://bugzilla.tianocore.org/show_bug.cgi?id=801
> >
> > Based on content from:
> >
> >
> https://github.com/Microsoft/MS_UEFI/blob/share/MsCapsu
> leSupport/MsCaps
> >
> uleUpdatePkg/Include/Library/DisplayUpdateProgressLib.h
> >
> https://github.com/Microsoft/MS_UEFI/tree/share/MsCapsu
> leSupport/MsCapsu
> > leUpdatePkg/Library/DisplayUpdateProgressGraphicsLib
> >
> https://github.com/Microsoft/MS_UEFI/tree/share/MsCapsu
> leSupport/MsCapsu
> > leUpdatePkg/Library/DisplayUpdateProgressTextLib
> >
> > Add DisplayUpdateProgressLib class along
> implementations for both graphical
> > (Graphics Output Protocol based) and text (Simple
> Text Output Protocol based)
> > consoles.  Also add the EDK II Firmware Management
> Progress Protocol that is
> > an
> > optional protocol that provides the progress bar
> color and a watchdog timeout
> > value thaty can be used when a firmware image is
> updated in a firmware device.
> >
> > * Add progress support to DxeCapsuleLibFmp
> > * Add progress support to SystemFirmwareUpdateDxe
> > * Add progress support to PlatformFlashAccessLib
> class and instances.
> > * Reduce Print() calls during a firmware update.
> >
> > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: David Wei <david.wei@intel.com>
> > Cc: Mang Guo <mang.guo@intel.com>
> > Cc: Kelly Steele <kelly.steele@intel.com>
> >
> > Signed-off-by: Michael D Kinney
> <michael.d.kinney@intel.com>
> > Contributed-under: TianoCore Contribution Agreement
> 1.1
> >
> > Kinney, Michael D (3):
> >   QuarkPlatformPkg: Add DisplayUpdateProgressLib
> mapping
> >   MdeModulePkg/DxeCapsuleLibFmp: Add progress bar
> support
> >   SignedCapsulePkg/SystemFirmwareUpdateDxe: Use
> progress API
> >
> > Michael D Kinney (6):
> >   MdeModulePkg: Add DisplayUpdateProgressLib class
> >   MdeModulePkg: Add DisplayUpdateProgressLib
> instances
> >   Vlv2Tbl2DevicePkg: Add DisplayUpdateProgressLib
> mapping
> >   SignedCapsulePkg/PlatformFlashAccessLib: Add
> progress API
> >   Vlv2TbltDevicePkg/PlatformFlashAccessLib: Add
> progress API
> >   QuarkPlatformPkg/PlatformFlashAccessLib: Add
> progress API
> >
> >  .../Include/Library/DisplayUpdateProgressLib.h     |
> 65 +++
> >  .../Include/Protocol/FirmwareManagementProgress.h  |
> 50 +++
> >  .../DisplayUpdateProgressGraphicsLib.c             |
> 475
> > +++++++++++++++++++++
> >  .../DisplayUpdateProgressGraphicsLib.inf           |
> 60 +++
> >  .../DisplayUpdateProgressGraphicsLib.uni           |
> 18 +
> >  .../DisplayUpdateProgressTextLib.c                 |
> 142 ++++++
> >  .../DisplayUpdateProgressTextLib.inf               |
> 53 +++
> >  .../DisplayUpdateProgressTextLib.uni               |
> 18 +
> >  .../Library/DxeCapsuleLibFmp/DxeCapsuleLib.c       |
> 47 +-
> >  .../Library/DxeCapsuleLibFmp/DxeCapsuleLib.inf     |
> 8 +-
> >  .../DxeCapsuleLibFmp/DxeCapsuleProcessLib.c        |
> 84 +++-
> >  .../DxeCapsuleLibFmp/DxeCapsuleProcessLibNull.c    |
> 21 +-
> >  .../DxeCapsuleLibFmp/DxeRuntimeCapsuleLib.inf      |
> 7 +-
> >  MdeModulePkg/MdeModulePkg.dec                      |
> 11 +
> >  MdeModulePkg/MdeModulePkg.dsc                      |
> 3 +
> >  .../PlatformFlashAccessLibDxe.c                    |
> 59 ++-
> >  QuarkPlatformPkg/Quark.dsc                         |
> 1 +
> >  .../Include/Library/PlatformFlashAccessLib.h       |
> 33 +-
> >  .../PlatformFlashAccessLibNull.c                   |
> 54 ++-
> >  .../SystemFirmwareUpdate/SystemFirmwareUpdateDxe.c |
> 92 ++--
> >  .../PlatformFlashAccessLib.c                       |
> 84 ++--
> >  .../PlatformFlashAccessLib.inf                     |
> 3 +-
> >  Vlv2TbltDevicePkg/PlatformPkgGccX64.dsc            |
> 1 +
> >  Vlv2TbltDevicePkg/PlatformPkgIA32.dsc              |
> 1 +
> >  Vlv2TbltDevicePkg/PlatformPkgX64.dsc               |
> 1 +
> >  25 files changed, 1285 insertions(+), 106
> deletions(-)
> >  create mode 100644
> >
> MdeModulePkg/Include/Library/DisplayUpdateProgressLib.h
> >  create mode 100644
> >
> MdeModulePkg/Include/Protocol/FirmwareManagementProgres
> s.h
> >  create mode 100644
> >
> MdeModulePkg/Library/DisplayUpdateProgressGraphicsLib/D
> isplayUpdateProg
> > ressGraphicsLib.c
> >  create mode 100644
> >
> MdeModulePkg/Library/DisplayUpdateProgressGraphicsLib/D
> isplayUpdateProg
> > ressGraphicsLib.inf
> >  create mode 100644
> >
> MdeModulePkg/Library/DisplayUpdateProgressGraphicsLib/D
> isplayUpdateProg
> > ressGraphicsLib.uni
> >  create mode 100644
> >
> MdeModulePkg/Library/DisplayUpdateProgressTextLib/Displ
> ayUpdateProgressT
> > extLib.c
> >  create mode 100644
> >
> MdeModulePkg/Library/DisplayUpdateProgressTextLib/Displ
> ayUpdateProgressT
> > extLib.inf
> >  create mode 100644
> >
> MdeModulePkg/Library/DisplayUpdateProgressTextLib/Displ
> ayUpdateProgressT
> > extLib.uni
> >
> > --
> > 2.14.2.windows.3