From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::22f; helo=mail-it0-x22f.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x22f.google.com (mail-it0-x22f.google.com [IPv6:2607:f8b0:4001:c0b::22f]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 11A6F20352A84 for ; Fri, 1 Dec 2017 09:14:09 -0800 (PST) Received: by mail-it0-x22f.google.com with SMTP id t1so3221071ite.5 for ; Fri, 01 Dec 2017 09:18:36 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=+TxaVVL3FQdc9u0I552iJCZILH3scFpY3rKZUR4s30Q=; b=B56ijarBi9EQp89D55ViU+eep1fZcgt8ivAb56gqY8MiL9Aa3PEPEs6GTfbgEE+LlP d12glJaRvVraUqlOZm/EZDQb9Gn0/7s8clQsjA0K95Z0YF58Wmn9ul/OD57YulECWibs XZkXhpWXrNPJPA8xuEwJXypZ5daEJfbgEoMGk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=+TxaVVL3FQdc9u0I552iJCZILH3scFpY3rKZUR4s30Q=; b=sZILzCI3keE06t1nxALICbEfVNFmfHAnw8ZkANHSIyABrLyTPwY8ZgqlP0j+sXWmTS qP635WOILZF0pLuJjtiNvTGZIlnCIl7aY+9wc2tpm19L7XBirhY7hzU8kEvD44SU20aH ppVpLbVsiZcwPJbbwMJueH1lzk7/4v+WnGTMt5eoX61x3fHKiDfVy8HNncozcx6hW8D/ XdVYGg9f8V1MsAVAr80niAIPT46H7dLgZ4FAi2TGtZJevnjwdogRnXtse6GInVYKFuPW 2cmDCpfnLgS6kmweQyKRHwUDYIc5vVu4Wb1AlgDEw5MuQT+Cbc31ZzE2Cp7iV9IvjHNq bUWQ== X-Gm-Message-State: AKGB3mJjDlQV95O7gwuGhN8dJpxzLleTaZ/VsXWz0Hxw2hMlRiUU+yGp W2T8Oh5+K5e65a4aJ7RIIbvfO/HtrKgEz67YVtoztQ== X-Google-Smtp-Source: AGs4zMa1T4lIZbUiN/Ose7Fw6WaVOqNsTa11r1qTZkWsc9UlPhY80cJLWYnGVuQ2ovSJSw6L2PoirxmjzCrNex5FHSg= X-Received: by 10.36.31.212 with SMTP id d203mr2698987itd.48.1512148715638; Fri, 01 Dec 2017 09:18:35 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.104.16 with HTTP; Fri, 1 Dec 2017 09:18:35 -0800 (PST) In-Reply-To: References: <20170926201529.11644-1-evan.lloyd@arm.com> <20170926201529.11644-20-evan.lloyd@arm.com> From: Ard Biesheuvel Date: Fri, 1 Dec 2017 17:18:35 +0000 Message-ID: To: Evan Lloyd Cc: "edk2-devel@lists.01.org" , Matteo Carlini , Leif Lindholm , Girish Pathak Subject: Re: [PATCH 19/19] ArmPlatformPkg: New DP500/DP550/DP650 GOP driver. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 01 Dec 2017 17:14:10 -0000 Content-Type: text/plain; charset="UTF-8" On 1 December 2017 at 13:12, Evan Lloyd wrote: > > >> -----Original Message----- >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] >> Sent: 28 November 2017 18:18 >> To: Evan Lloyd >> Cc: edk2-devel@lists.01.org; Matteo Carlini ; >> Leif Lindholm ; Girish Pathak >> >> Subject: Re: [PATCH 19/19] ArmPlatformPkg: New DP500/DP550/DP650 >> GOP driver. >> >> On 26 September 2017 at 21:15, wrote: >> > From: Girish Pathak >> > >> > 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 >> > Signed-off-by: Evan Lloyd >> >> 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. -- Ard.