ArmPlatformPkg: LcdGraphicsOutputDxe, PL111, and HdLcd rejig

ArmPlatformPkg: LcdGraphicsOutputDxe, PL111, and HdLcd rejig

[Girish Pathak]

Hi,

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

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

Regards,
Girish
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: ArmPlatformPkg: LcdGraphicsOutputDxe, PL111, and HdLcd rejig

[Leif Lindholm]

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

Re: ArmPlatformPkg: LcdGraphicsOutputDxe, PL111, and HdLcd rejig

[Ard Biesheuvel]

> 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

Re: ArmPlatformPkg: LcdGraphicsOutputDxe, PL111, and HdLcd rejig

[Evan Lloyd]

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
...
> >
> > The real improvement is the reduction in code duplication.
> >
>
> Perhaps we shoukd move to the UEFI driver model at the same time?
>
[[Evan Lloyd]] That would certainly be the "proper" way to do it.
The reasons not to are:
    1. So that early output can be seen on the display.
        (I confess that argument would be stronger were ConSplitterDxe not itself UEFI driver model)  :-{
    2. The theoretical case of a boot logo, which, I think, should flash up as early as possible.

However, we don't feel strongly about either.
...
Regards,
Evan

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.