Re: ArmPlatformPkg: LcdGraphicsOutputDxe, PL111, and HdLcd rejig

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

Hi Girish,

On Wed, May 03, 2017 at 04:58:33PM +0000, Girish Pathak wrote:
> With a view to an upcoming change, we have been examining the current Graphics
> Output Protocol implementation. Currently for, ARM platforms, the UEFI Graphics
> Output Protocol is implemented using a platform specific Library
> (PL111LcdArmVExpressLib/HdLcdArmVExpressLib) and a DXE driver
> (PL111LcdGraphicsOutputDxe/HdLcdGraphicsOutputDxe). The platform specific
> library handles platform specific variations such as platform supported display
> modes, memory management of the frame buffer, and clock/mux setting. The DXE
> driver implements the GOP protocol and manages the respective display IP.
> Although this implementation works fine for current platforms, we think the way
> the current DXE driver sources are linked is sub-optimal and can be improved to
> meet recommendation of the EDKII Module Writer's Guide.

The real improvement is the reduction in code duplication.

> The DXE driver source contains three source files per driver. The files
> LcdGraphicsOutputDxe.c and LcdGraphicsOutputBlt.c implement common
> functionality whereas HdLcd.c/PL111Lcd.c  implement display IP specific part of
> the DXE. The problem is, there are two .inf files for HdLcdGraphicsOutputDxe
> and PL111LcdGraphicsOutputDxe and both link common code LcdGraphicsOutputDxe.c
> and LcdGraphicsOutputBlt.c with display IP source instead of a library instance
> , which seems incorrect and can be improved. We propose to separate HdLcd.c
> and PL111Lcd.c and create independent libraries managing only respective
> display IP which can then be instantiated as LcdHwLib and linked in a common
> LcdGraphicsOutputDxe DXE driver. This will help to clearly partition
> implementation of the Graphics Output Protocol into three separate components,
> a platform specific component for the display IP, a display IP specific
> component and GOP common code.

This all sounds imminently sensible. One request for clarifiation below.

> So instead of the current structure in ArmPlatformPkg for display DXE:
> 
>   ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcdGraphicsOutputDxe.inf    (HDLCD GOP DXE)
>   ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111LcdGraphicsOutputDxe.inf    (PL111 GOP DXE)
> 
> We propose a structure like:
> 
>   ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/LcdGraphicsOutputDxe.inf    (GOP DXE independent of hardware/platform)

Are we expecting this one to be truly independent, or simply common
between most/all ARM Ltd. display controllers?

>   ArmPlatformPkg/Drivers/HdLcd/HdLcd.inf    (Library code managing HDLCD HW IP)
>   ArmPlatformPkg/Drivers/PL111/PL11Lcd.inf    (Library code managing PL111 HW IP)
> 
> LcdGraphicsOutputDxe.inf will link to LcdPlatformLib and LcdHwLib which can be
> selected for the platform in the platform specific .dsc file.
> 
> e.g.
> 
> Under LibraryClasses we might have:
>     LcdPlatformLib|ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpressLib.inf
>     LcdHwLib|ArmPlatformPkg/Drivers/HdLcd/HdLcd.inf
> 
> And Under Components:
>     ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/LcdGraphicsOutputDxe.inf  (Common GOP DXE code for ARM Platforms)
> 
> This is a significant change and we would like to invite viewpoints before we
> proceed with implementing these. Since the change would only be with respect to
> display aspects of ArmPlatformPkg we don't foresee any impact on any other
> functionality.
> 
> Please reply if you feel this intended change might impact you and why ?
> Unless objections are raised, we will soon submit the patches for review.

Sounds good to me.

Regards,

Leif