[PATCH v1 0/1] Added GOP driver for DisplayLink-based Universal USB Docks to edk2-platforms


Andy Hayes
 

From 4a42eb997aeb3699217b40bf3bc47dec56458847 Mon Sep 17 00:00:00 2001

From: "Andy Hayes" < andy.hayes@... >

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.hayes@... (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


Michael D Kinney
 

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

 

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/

 

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.hayes@...]
Sent: Wednesday, August 14, 2019 7:44 AM
To: devel@edk2.groups.io
Cc: Leif Lindholm <leif.lindholm@...>; Kinney, Michael D <michael.d.kinney@...>
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.hayes@... >

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.hayes@... (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


Leif Lindholm
 

Hi Andy,

Many thanks for this submission.

I am finding a few issues when building with gcc/clang (I expect you
use Visual Studio).

The RELEASE target barfs with
.../edk2-platforms/Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/Gop.c:363:1:
error: unused function 'CalculateRefreshRate'
since the function is only called from a DEBUG statement.
Could be resolved by putting the function inside #ifndef MDEPKG_NDEBUG

Clang triggers an error on the
USB_DISPLAYLINK_DEV_FROM_GRAPHICS_OUTPUT_PROTOCOL macro which seems to
go away if the USB_DISPLAYLINK_DEV_SIGNATURE macro is moved below the
USB_DISPLAYLINK_DEV definition. (I haven't bothered to figure out why
this helps.)

The NOOPT build fails for IA32/X64 on clang with
.../edk2-platforms/Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/UsbDescriptors.c:35:
undefined reference to `memset'
You could get rid of this by doing the assignment separate from the
definition. (Indeed, I believe this is one of the reasons for the
rule.)

On X64/clang, the build fails with
.../edk2-platforms/Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/UsbDisplayLink.c:410:3:
error: '__builtin_ms_va_start' used in System V ABI function
Adding EFIAPI to the definition/declaration of DlGopPrintTextToScreen
makes this one go away.

Best Regards,

Leif

On Wed, Aug 14, 2019 at 02:43:43PM +0000, Andy Hayes wrote:
From 4a42eb997aeb3699217b40bf3bc47dec56458847 Mon Sep 17 00:00:00 2001
From: "Andy Hayes" < andy.hayes@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.hayes@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


Leif Lindholm
 

Hi Andy,

A follow-up - a few more things need changing:

UsbDisplayLink.h (well, any .h file) should only contain the headers
it itself requires.
Moreover, the inclusion of wchar.h must disappear completely.
Instead, use CHAR16 * in the two locations in UsbDisplayLink.c.

Removing wchar.h will then trip you up in Edid.h, at
#define EDID_HEADER_SIZE ((size_t)8)
Please change the size_t to UINTN.

Finally, you need to (and now can after the above changes) get rid of
#undef _DEBUG

#undef NULL
#undef _ASSERT
in UsbDisplayLink.h.

With these additional changes, the driver builds successfully across
GCC 8.3 and CLANG 7.0 for AARCH64/ARM/IA32/X64.

Best Regards,

Leif

On Wed, Aug 14, 2019 at 06:03:51PM +0100, Leif Lindholm wrote:
Hi Andy,

Many thanks for this submission.

I am finding a few issues when building with gcc/clang (I expect you
use Visual Studio).

The RELEASE target barfs with
.../edk2-platforms/Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/Gop.c:363:1:
error: unused function 'CalculateRefreshRate'
since the function is only called from a DEBUG statement.
Could be resolved by putting the function inside #ifndef MDEPKG_NDEBUG

Clang triggers an error on the
USB_DISPLAYLINK_DEV_FROM_GRAPHICS_OUTPUT_PROTOCOL macro which seems to
go away if the USB_DISPLAYLINK_DEV_SIGNATURE macro is moved below the
USB_DISPLAYLINK_DEV definition. (I haven't bothered to figure out why
this helps.)

The NOOPT build fails for IA32/X64 on clang with
.../edk2-platforms/Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/UsbDescriptors.c:35:
undefined reference to `memset'
You could get rid of this by doing the assignment separate from the
definition. (Indeed, I believe this is one of the reasons for the
rule.)

On X64/clang, the build fails with
.../edk2-platforms/Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/UsbDisplayLink.c:410:3:
error: '__builtin_ms_va_start' used in System V ABI function
Adding EFIAPI to the definition/declaration of DlGopPrintTextToScreen
makes this one go away.

Best Regards,

Leif

On Wed, Aug 14, 2019 at 02:43:43PM +0000, Andy Hayes wrote:
From 4a42eb997aeb3699217b40bf3bc47dec56458847 Mon Sep 17 00:00:00 2001
From: "Andy Hayes" < andy.hayes@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.hayes@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


Andy Hayes
 

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.kinney@...>
> Sent: 14 August 2019 16:23
> To: Andy Hayes <andy.hayes@...>; devel@edk2.groups.io; Ni, Ray <ray.ni@...>; Kinney, Michael D <michael.d.kinney@...>
> Cc: Leif Lindholm <leif.lindholm@...>
> 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/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>
>
> 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/>
>
> 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.hayes@...]
> Sent: Wednesday, August 14, 2019 7:44 AM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> Cc: Leif Lindholm <leif.lindholm@...<mailto:leif.lindholm@...>>; Kinney, Michael D <michael.d.kinney@...<mailto:michael.d.kinney@...>>
> 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.hayes@...<mailto:andy.hayes@...> >
> 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.hayes@...<mailto:andy.hayes@...> (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


sebastian.olney@...
 

Reviewed-by: Seb Olney <sebastian.olney@...>