On Thu, Aug 15, 2019 at 10:15:48AM +0000, Andy Hayes wrote: > Thanks very much for the very quick feedback from both of you - I > really appreciate it. I'll fix the issues that you pointed out and > re-submit the patch.
Thanks! > I've been testing the build with VisualStudio 2015 and GCC 7.3. (VS > for X64, GCC5 for IA32/X64/ARM/AARCH64) I had a bit of trouble > getting the clang build to work but I'll try it again to make sure > that I don't introduce any other errors. Yeah, the compilers (especially GCC/clang) are getting pickier My baseline today is Clang 7 and GCC 8.3. Testing with Clang 8 and GCC9 is definitely becoming relevant, but could in most cases replace testing with GCC 8. > The way that the C structures are initialized - what other compilers > need to be tested to make sure there is sufficient support for this > syntax? (I believe the syntax is a C99 thing). Well, in this case, the "problem" is that the compiler generates calls to "known to be there" libc helper functions. Which aren't for us. On the IA32/X64 side, these rules have been put in place. On the ARM/AARCH64 side, we have ArmPkg/Library/CompilerIntrinsicsLib, which on (32-bit) ARM was a fundamental necessity for other reasons. So testing with a particular compiler version may or may not trigger the issue, and seemingly unrelated future code changes may nudge the compiler into doing something else instead. So it's not exactly foolproof. > I know that there is a big list in tools.def, but realistically > which ones do you have to be sure work correctly, to cover the > majority of users? It's easy enough to cover all of the versions of > GCC and CLANG for, say, X86/X64/ARM/AARCH64, but how far back with > Visual Studio versions do I need to go? I don't expect everyone to test everything - only to test more than one option and fix things as reported. Unless it's core code. It *would* be perfetcly valid (though less than ideal) to have external device drivers not supported by the full set of toolchains. Of course, making your code build with multiple toolchains frequently flush out latent bugs, so... In this case, the changes were all pretty trivial, so I would prefer to see them implemented. We have some cleanup work to do on the cross-compilation aspect in general for !VS. While it can be nudged into working, it works differently on ARM and X64. Personally, I worry mostly about compatibility with the toolchain versions included in the latest stable version of Debian (since that's the distribution people tend to complain about lagging behind :). From Debian Buster, we finally have cross-compilation symmetry AARCH64<->X64. For something like UDK, we *should* probably worry about all versions included in all LTS Linux distro releases, but... > > I am curious how this driver interacts with the ConSplitter when > > displays are hot added and hot removed. I see a reference to a > > feature that appears to sync the GOP content between frame buffers > > when a display is hot added. I would be better if the common code in > > the ConSplitter handled these types of operations instead of putting > > that code in individual GOP drivers. I have added Ray to this thread > > who may have ideas on how to handle these hot add events. > > I think the code that you are looking at that syncs GOP content > might be a debug feature, rather than a hotplug handler. We found > that some platforms only render to the first GOP device that they > find, so I added the ability to build a version of the driver (using > the build flag COPY_PIXELS_FROM_PRIMARY_GOP_DEVICE) that > "steals" the pixels from another GOP framebuffer and displays them > on our output, to check that our driver fundamentally works on that > platform. This code is excluded in a normal build. Most platforms > are well-behaved and render to all GOP devices correctly. Best Regards, Leif > In general we've found that hotplug of our device works fine, > without us having to do anything special in the driver. Note that > the system will initially see this as a USB hotplug rather than a > display hotplug. For various reasons we decided not to support > display hotplug (i.e. plugging and unplugging displays into out > device once our device is plugged in to USB and initialized) in the > driver, mainly to keep things simple. > > Thanks again for your help, > > Andy Hayes > > From: Kinney, Michael D > <michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com>> > Sent: 14 August 2019 16:23 > To: Andy Hayes > <andy.ha...@displaylink.com<mailto:andy.ha...@displaylink.com>>; > devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ni, Ray > <ray...@intel.com<mailto:ray...@intel.com>>; Kinney, Michael D > <michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com>> > Cc: Leif Lindholm <leif.lindh...@linaro.org<mailto:leif.lindh...@linaro.org>> > Subject: [External] RE: [PATCH v1 0/1] Added GOP driver for DisplayLink-based > Universal USB Docks to edk2-platforms > > Hi Andy, > > Thanks for the contribution. Is this patch for the edk2-platform repo? The > email subject can help clarify the intended repo. > > Also, we prefer the patches to be provided inline instead of an attachment. > You can use the > 'git send-email' feature to do this. Here are a couple links to useful wikis. > > https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process<https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process><https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process<https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process>> > https://github.com/tianocore/tianocore.github.io/wiki/Laszlo<https://github.com/tianocore/tianocore.github.io/wiki/Laszlo>'s-unkempt-git-guide-for-edk2-contributors-and-maintainers<https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers<https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers>> > > I also noticed looking at the patch file that there are some inconsistencies > with the > EDK II C Coding Standards. Most are around function headers and indents. Here > is a link > to the latest version: > > https://www.gitbook.com/download/pdf/book/edk2-docs/edk-ii-c-coding-standards-specification<https://www.gitbook.com/download/pdf/book/edk2-docs/edk-ii-c-coding-standards-specification><https://www.gitbook.com/download/pdf/book/edk2-docs/edk-ii-c-coding-standards-specification<https://www.gitbook.com/download/pdf/book/edk2-docs/edk-ii-c-coding-standards-specification>> > > I see a few #if 0 and commented out lines of code and TODO comments. Can you > please clean these up > and if needed add Tianocore Bugzillas for future enhancements/work items for > this driver. > > https://bugzilla.tianocore.org/<https://bugzilla.tianocore.org/><https://bugzilla.tianocore.org/<https://bugzilla.tianocore.org/>> > > I also noticed that you are using named fields to initialize C structures: > > STATIC CONST struct VideoMode ModeData[] = { > + { > + // 640 x 480 @ 60Hz > + .Reserved2 = 2, > + .PixelClock = 2517, > + .HActive = 640, > + .HBlanking = 160, > + .HSyncOffset = 16, > + .HSyncWidth = 96, > > I do like this style because it has better self documentation of the field > being > initialized to a value. However, I do not see this style in the rest of the > EDK II > sources, so I am concerned that we may have required compilers for the EDK II > that may not support this syntax. > > If we find all the supported compilers do support this syntax, this would be > a great addition to the EDK II C Coding Standards. > > I am curious how this driver interacts with the ConSplitter when displays > are hot added and hot removed. I see a reference to a feature that appears > to sync the GOP content between frame buffers when a display is hot added. > I would be better if the common code in the ConSplitter handled these types > of operations instead of putting that code in individual GOP drivers. I have > added Ray to this thread who may have ideas on how to handle these hot > add events. > > Thanks, > > Mike > > From: Andy Hayes [mailto:andy.ha...@displaylink.com] > Sent: Wednesday, August 14, 2019 7:44 AM > To: > devel@edk2.groups.io<mailto:devel@edk2.groups.io<mailto:devel@edk2.groups.io%3cmailto:devel@edk2.groups.io>> > Cc: Leif Lindholm > <leif.lindh...@linaro.org<mailto:leif.lindh...@linaro.org<mailto:leif.lindh...@linaro.org%3cmailto:leif.lindh...@linaro.org>>>; > Kinney, Michael D > <michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com%3cmailto:michael.d.kin...@intel.com>>> > Subject: [PATCH v1 0/1] Added GOP driver for DisplayLink-based Universal USB > Docks to edk2-platforms > > From 4a42eb997aeb3699217b40bf3bc47dec56458847 Mon Sep 17 00:00:00 2001 > From: "Andy Hayes" < > andy.ha...@displaylink.com<mailto:andy.ha...@displaylink.com<mailto:andy.ha...@displaylink.com%3cmailto:andy.ha...@displaylink.com>> > > > Date: Wed, 14 Aug 2019 15:30:20 +0100 > Subject: [PATCH v1 0/1] Added GOP graphics driver for DisplayLink-based > Universal USB Docks to edk2-platforms > > andy.ha...@displaylink.com<mailto:andy.ha...@displaylink.com<mailto:andy.ha...@displaylink.com%3cmailto:andy.ha...@displaylink.com>> > (1): > Added GOP driver for USB Docks which use DisplayLink chips. > > .../DisplayLinkPkg/DisplayLinkPkg.dsc | 61 + > .../DisplayLinkGop/DisplayLinkGopDxe.inf | 63 + > .../DisplayLinkPkg/DisplayLinkGop/Edid.h | 129 ++ > .../DisplayLinkGop/UsbDescriptors.h | 109 ++ > .../DisplayLinkGop/UsbDisplayLink.h | 284 +++++ > .../DisplayLinkGop/CapabilityDescriptor.c | 137 ++ > .../DisplayLinkGop/ComponentName.c | 235 ++++ > .../DisplayLinkPkg/DisplayLinkGop/Edid.c | 598 +++++++++ > .../DisplayLinkPkg/DisplayLinkGop/Gop.c | 587 +++++++++ > .../DisplayLinkGop/UsbDescriptors.c | 144 +++ > .../DisplayLinkGop/UsbDisplayLink.c | 1109 +++++++++++++++++ > .../DisplayLinkGop/UsbTransfer.c | 180 +++ > .../DisplayLinkGop/VideoModes.c | 254 ++++ > Drivers/DisplayLink/DisplayLinkPkg/ReadMe.md | 77 ++ > Maintainers.txt | 5 + > 15 files changed, 3972 insertions(+) > create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkPkg.dsc > create mode 100644 > Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/DisplayLinkGopDxe.inf > create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/Edid.h > create mode 100644 > Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/UsbDescriptors.h > create mode 100644 > Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/UsbDisplayLink.h > create mode 100644 > Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/CapabilityDescriptor.c > create mode 100644 > Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/ComponentName.c > create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/Edid.c > create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/Gop.c > create mode 100644 > Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/UsbDescriptors.c > create mode 100644 > Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/UsbDisplayLink.c > create mode 100644 > Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/UsbTransfer.c > create mode 100644 > Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/VideoModes.c > create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/ReadMe.md > > -- > 2.17.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#45720): https://edk2.groups.io/g/devel/message/45720 Mute This Topic: https://groups.io/mt/32864415/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
