Re: [PATCH v2 1/5] ArmPlatformPkg: introduce LcdHwLib library class

[Leif Lindholm <leif.lindholm@linaro.org>]

On Mon, Dec 11, 2017 at 05:56:37PM +0000, Ard Biesheuvel wrote:
> On 11 December 2017 at 17:32, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Fri, Dec 08, 2017 at 05:31:24PM +0000, Ard Biesheuvel wrote:
> >> Add the declaration and include file for the new LcdHwLib library class,
> >> which will allow us to abstract away from platform  variations in the
> >> LCD graphics output driver.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Girish Pathak <girish.pathak@arm.com>
> >> Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
> >> [ardb: add NULL implementation as well and add it to ArmPlatformPkg.dsc]
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >>  ArmPlatformPkg/ArmPlatformPkg.dec                    |  1 +
> >>  ArmPlatformPkg/ArmPlatformPkg.dsc                    |  1 +
> >>  ArmPlatformPkg/Include/Library/LcdHwLib.h            | 68 ++++++++++++++++++
> >>  ArmPlatformPkg/Library/LcdHwNullLib/LcdHwNullLib.c   | 75 ++++++++++++++++++++
> >>  ArmPlatformPkg/Library/LcdHwNullLib/LcdHwNullLib.inf | 28 ++++++++
> >>  5 files changed, 173 insertions(+)
> >>
> >> diff --git a/ArmPlatformPkg/ArmPlatformPkg.dec b/ArmPlatformPkg/ArmPlatformPkg.dec
> >> index 6a7bbc02d011..b33b6e630d85 100644
> >> --- a/ArmPlatformPkg/ArmPlatformPkg.dec
> >> +++ b/ArmPlatformPkg/ArmPlatformPkg.dec
> >> @@ -33,6 +33,7 @@ [Includes.common]
> >>
> >>  [LibraryClasses]
> >>    ArmPlatformLib|Include/Library/ArmPlatformLib.h
> >> +  LcdHwLib|Include/Library/LcdHwLib.h
> >>    LcdPlatformLib|Include/Library/LcdPlatformLib.h
> >>    NorFlashPlatformLib|Include/Library/NorFlashPlatformLib.h
> >>    PL011UartLib|Include/Library/PL011UartLib.h
> >> diff --git a/ArmPlatformPkg/ArmPlatformPkg.dsc b/ArmPlatformPkg/ArmPlatformPkg.dsc
> >> index c3a8c257cb02..9dd64b472acf 100644
> >> --- a/ArmPlatformPkg/ArmPlatformPkg.dsc
> >> +++ b/ArmPlatformPkg/ArmPlatformPkg.dsc
> >> @@ -101,6 +101,7 @@ [Components.common]
> >>
> >>    ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNull.inf
> >>    ArmPlatformPkg/Library/ArmPlatformStackLib/ArmPlatformStackLib.inf
> >> +  ArmPlatformPkg/Library/LcdHwNullLib/LcdHwNullLib.inf
> >>    ArmPlatformPkg/Library/LcdPlatformNullLib/LcdPlatformNullLib.inf
> >>    ArmPlatformPkg/Library/NorFlashPlatformNullLib/NorFlashPlatformNullLib.inf
> >>    ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.inf
> >> diff --git a/ArmPlatformPkg/Include/Library/LcdHwLib.h b/ArmPlatformPkg/Include/Library/LcdHwLib.h
> >> new file mode 100644
> >> index 000000000000..0c0480862aea
> >> --- /dev/null
> >> +++ b/ArmPlatformPkg/Include/Library/LcdHwLib.h
> >> @@ -0,0 +1,68 @@
> >> +/** @file LcdHwLib.h
> >> +
> >> +  This file contains interface functions for LcdHwLib of ArmPlatformPkg
> >> +
> >> +  Copyright (c) 2017, ARM Ltd. All rights reserved.<BR>
> >> +
> >> +  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
> >> +  http://opensource.org/licenses/bsd-license.php
> >> +
> >> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> >> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> >> +
> >> +**/
> >> +
> >> +#ifndef LCD_HW_LIB_H_
> >> +#define LCD_HW_LIB_H_
> >> +
> >> +#include <Uefi/UefiBaseType.h>
> >> +
> >> +/**
> >> +  Check for presence of display
> >> +
> >> +  @retval EFI_SUCCESS            Platform implements display.
> >> +  @retval EFI_NOT_FOUND          Display not found on the platform.
> >> +
> >> +**/
> >> +EFI_STATUS
> >> +LcdIdentify (
> >> +  VOID
> >> +  );
> >> +
> >> +/**
> >> +  Initialize display.
> >> +
> >> +  @param  FrameBaseAddress       Address of the frame buffer.
> >> +  @retval EFI_SUCCESS            Display initialization success.
> >> +  @retval !(EFI_SUCCESS)         Display initialization failure.
> >> +
> >> +**/
> >> +EFI_STATUS
> >> +LcdInitialize (
> >> +  EFI_PHYSICAL_ADDRESS  FrameBaseAddress
> >> +  );
> >> +
> >> +/**
> >> +  Set requested mode of the display.
> >> +
> >> +  @param  ModeNumber             Display mode number.
> >> +  @retval EFI_SUCCESS            Display set mode success.
> >> +  @retval EFI_DEVICE_ERROR       If mode not found/supported.
> >> +
> >> +**/
> >> +EFI_STATUS
> >> +LcdSetMode (
> >> +  IN UINT32  ModeNumber
> >> +  );
> >> +
> >> +/**
> >> +  De-initializes the display.
> >> +**/
> >> +VOID
> >> +LcdShutdown (
> >> +  VOID
> >> +  );
> >> +
> >> +#endif /* LCD_HW_LIB_H_ */
> >> diff --git a/ArmPlatformPkg/Library/LcdHwNullLib/LcdHwNullLib.c b/ArmPlatformPkg/Library/LcdHwNullLib/LcdHwNullLib.c
> >> new file mode 100644
> >> index 000000000000..2d31b5183c95
> >> --- /dev/null
> >> +++ b/ArmPlatformPkg/Library/LcdHwNullLib/LcdHwNullLib.c
> >> @@ -0,0 +1,75 @@
> >> +/** @file
> >> +
> >> +  Copyright (c) 2017, Linaro, Ltd. 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
> >> +  http://opensource.org/licenses/bsd-license.php
> >> +
> >> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> >> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> >> +
> >> +**/
> >> +
> >> +#include <Base.h>
> >> +#include <Uefi/UefiBaseType.h>
> >> +#include <Library/DebugLib.h>
> >> +#include <Library/LcdPlatformLib.h>
> >> +
> >> +/**
> >> +  Check for presence of display
> >> +
> >> +  @retval EFI_SUCCESS            Platform implements display.
> >> +  @retval EFI_NOT_FOUND          Display not found on the platform.
> >> +
> >> +**/
> >> +EFI_STATUS
> >> +LcdIdentify (
> >> +  VOID
> >> +  )
> >> +{
> >
> > Bikeshedding:
> > Since this Null implementation can never fill any functionality other
> > than letting build complete without platform-specific glue - would it
> > not be more expected for it to ASSERT and/or return failure
> > everywhere?
> 
> I think it makes sense to ASSERT on Null library member that take OUT
> parameters, because you can never return anything meaningful. In this
> case, we can just pretend that the Null LcdHwLib does not require
> initialization, is always present and only supports a single mode, and
> the driver could theoretically still work. Highly unlikely to be
> useful for anything but still.

I don't necessarily agree, but I also don't consider this very
important.

I guess my internal model of LibNulls is that if they can be
paractically used as do-nothing variants, they return OK on anything,
whereas if practical use requires further implementation, they ASSERT
and/or return error. This is not necessarily a correct world view.

For the ArmPlatformPkg resolution, you don't care about
runtime behaviour - it will never be executed.

> On ArmVirtQemu, we switched to the Null ArmPlatformLib implementation
> because we need to fulfil the dependency to be able to use PrePeiCore,
> but we no longer have any code to put in there.

Sure - but in this case you would only use this library if you
actually had a display driver to link against.

Anyway:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

/
    Leif