Re: [PATCH 19/19] ArmPlatformPkg: New DP500/DP550/DP650 GOP driver.

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

On 5 December 2017 at 20:03, Evan Lloyd <Evan.Lloyd@arm.com> wrote:
>
>
>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: 01 December 2017 17:19
>> To: Evan Lloyd <Evan.Lloyd@arm.com>
>> Cc: edk2-devel@lists.01.org; Matteo Carlini <Matteo.Carlini@arm.com>;
>> Leif Lindholm <leif.lindholm@linaro.org>; Girish Pathak
>> <Girish.Pathak@arm.com>
>> Subject: Re: [PATCH 19/19] ArmPlatformPkg: New DP500/DP550/DP650
>> GOP driver.
>>
>> On 1 December 2017 at 13:12, Evan Lloyd <Evan.Lloyd@arm.com> wrote:
>> >
>> >
>> >> -----Original Message-----
>> >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> >> Sent: 28 November 2017 18:18
>> >> To: Evan Lloyd <Evan.Lloyd@arm.com>
>> >> Cc: edk2-devel@lists.01.org; Matteo Carlini <Matteo.Carlini@arm.com>;
>> >> Leif Lindholm <leif.lindholm@linaro.org>; Girish Pathak
>> >> <Girish.Pathak@arm.com>
>> >> Subject: Re: [PATCH 19/19] ArmPlatformPkg: New DP500/DP550/DP650
>> GOP
>> >> driver.
>> >>
>> >> On 26 September 2017 at 21:15,  <evan.lloyd@arm.com> wrote:
>> >> > From: Girish Pathak <girish.pathak@arm.com>
>> >> >
>> >> > This change adds support for the ARM Mali DP500/DP500/DP650
>> display
>> >> > processors using the GOP protocol. It has been tested on FVP base
>> >> > models + DP550 support.
>> >> >
>> >> > This change does not modify functionality provided by PL111 or
>> >> > HDLCD. The driver should be suitable for those platforms that
>> >> > implement ARM Mali DP500/DP550/DP650 replacing PL111/HDLCD.
>> >> >
>> >> > Only "Layer Graphics" of the ARM Mali DP is configured for
>> >> > rendering the RGB/BGR format frame buffer to satisfy the UEFI GOP
>> >> > requirements Other layers e.g. video layers are not configured.
>> >> >
>> >> > NOTE: This change implements the Mali DP on models. Versions for
>> >> > actual hardware are liable to require extra handling for clock
>> >> > input changes, etc.
>> >> >
>> >> > 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>
>> >>
>> >> Hello Girish, Evan,
>> >>
>> >> (replying to 19/19 because I cannot find the cover letter in my
>> >> edk2-devel archive but this really applies to the whole series)
>> >>
>> >> I have been looking at these patches again now that I am trying to
>> >> clean up ArmPlatformPkg, which is currently a dumping ground for all
>> >> things vaguely ARM related, and is also structured quite differently
>> >> from other packages.
>> >>
>> >> Ideally, ArmPlatformPkg should only contain the following:
>> >> - library class interfaces under Include/Library; header files kept
>> >> here should only contain elements that define API
>> >> - driver specific include files Include/IndustryStandard but *only*
>> >> if they cannot be kept locally with the driver in question
>> >> - libraries under Library/
>> >> - drivers under Drivers/
>> >>
>> >> This aligns with the common arrangement adopted by most EDK2
>> packages.
>> >>
>> >> This series does many different things, and does not distinguish at
>> >> all between common code and code living under ArmVExpressPkg. Given
>> >
>> >  [[Evan Lloyd]] All of the commits in the series are in ArmPlatformPkg.
>> > You may be in the process of disentangling the VE specific bits, but
>> > hitherto that has not been a consideration.  (Note: I'm not arguing against
>> the disentangling, only pointing out that it was not a factor at the point we
>> submitted the patches) The reason there are so many commits is only that
>> we have been asked to break things up into "bite sized" chunks for the
>> convenience of maintainers.
>> > The aim was to make each a coherent item with a simple justification.
>> >
>> >> that I am trying to move ArmVExpressPkg out of EDK2 into
>> >> edk2-platforms (where it arguably belongs) having this series in
>> >> limbo for two months is basically blocking my work, and so I would
>> >> like to explore ways to proceed with this without interfering with
>> >> each other's work too much. At the same time, the way the code is
>> >> structured is a continuation of the pattern I am trying to get rid
>> >> of, so they will need some rework anyway in order to be upstreamable
>> IMHO.
>> >
>> >  [[Evan Lloyd]] Not being psychic, we had not made allowance for your
>> plans.
>> > However, if you take the trouble to look at the changes, they achieve
>> exactly the split you aim for.
>> > The display type code (PL011 and HDLCD) is unravelled from the VE code.
>> > All that remains would be to move the VE specific part into edk2-
>> platforms.
>> >
>> >>
>> >> So could we split it up please? Assuming the intention is the ability
>> >> to reuse the Mali code on non-VExpress platforms, I would like to see
>> >> that code proposed separately, without any mention of VExpressPkg.dec
>> >
>> >  [[Evan Lloyd]] Given that the original code was unfortunate, I'm not sure
>> that it makes much difference what order the changes get made.
>> > Going West then North gets you to the same place as North then West
>> (except near a pole).
>> > If you accept that there was a logical progression in the changes made,
>> then it might be better to not rejig things pointlessly.
>> >
>> >> whatsoever. If you introduce any library classes that abstract away
>> >> the differences between platforms, you can include a Null version of
>> >> such a library that simply does ASSERT (FALSE) in every function:
>> >> this
>> >
>> >  [[Evan Lloyd]] One could, indeed, do that.  We, however, would be very
>> reluctant to incur the overhead of rework in response to spurious cavils
>> from a maintainer when it is of no direct relevance to us.
>> >
>>
>> I don't think the suggestion that we evil maintainers are nothing but an
>> impediment to the likes of you and your team members doing the actual
>> work is justified.
>>
>> We are all on the same team here, and the goal is to make UEFI code
>> reusable for the customers of /your/ employer. Throwing stuff over the
>> fence != upstreaming, and it is my job as a maintainer to ensure that code is
>> still maintainable long after the original authors have moved on to
>> something else.
>>
>> ArmPlatformPkg is a perfect example where code reuse is much more
>> difficult than it needs to be, and we as maintainers need to deal with
>> contributors from other companies that have used it as 'guidance' for how
>> to architect their UEFI firmware, which is usually filled with vexpress-isms
>> that date back to before anyone currently involved with UEFI can remember.
>>
>> This is why I have taken the time to sit down, go through all the crap code,
>> clean it up, refactor it and propose it on the list as improvements. I even
>> went so far as taking the preparatory Mali work of your team and rebase it
>> so that we can keep the bits that we may share, and move the bits out that
>> should not be kept in main EDK2 because they are being taken as gospel by
>> engineers that are new to
>> ARM+UEFI.
>>
>> If this is too much to deal with for you, then fine, don't upstream your code.
>> But if you do, you are going to have to play nice with the others, including
>> the maintainers.
>>
> [[Evan Lloyd]] Hi Ard.  Firstly, I apologize, I assumed from your name that you were Dutch and would therefore probably have a lively sense of humour.  Of course, if I have touched a nerve, that is unfortunate and I'm sorry.

No, the apparently blatantly obvious tongue-in-cheek nature of your
response was completely lost on me. But I know a person who does have
a lively sense of humour, so next time I will ask him for help.

> I agree that cleaning up the code is important, worthwhile, and an objective for us all.  What can be a difficulty is our very different conceptions of what clean means.
>

Fair enough. But as maintainers, we take ownership of your code, with
the implied promise to keep it in a working state. I don't think it is
unreasonable that we get to dictate some of the terms under which that
occurs.

> You should be aware though that a certain amount of whingeing is to be expected when dealing with Brits. (Ask Leif - he knows! Or any Australian.)
> I do not disagree with your intent - but I do sometimes feel that the criteria applied do not take into account the cost/benefit aspects, and seek to air that point.  I shall endeavour to make such points in a less bantering way in future.
>

Thanks.

I think one of the misconceptions may be that upstreaming is something
one does once the code is completely finished. Instead, please involve
us much sooner if you intend to upstream your code (or just Leif for
confidential stuff). That way, any effort invested in the code
benefits your product as well as the open source, rather than shipping
one version, and having to go back and change stuff for the version
that ends up upstream.