Re: ArmPlatformPkg: LcdGraphicsOutputDxe, PL111, and HdLcd rejig

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

> On 4 May 2017, at 12:14, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> 
> 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.
> 

Perhaps we shoukd move to the UEFI driver model at the same time?

>> 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