From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.100, mailfrom: michael.d.kinney@intel.com) Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by groups.io with SMTP; Wed, 14 Aug 2019 08:23:03 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 14 Aug 2019 08:23:02 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,385,1559545200"; d="scan'208,217";a="176603636" Received: from orsmsx103.amr.corp.intel.com ([10.22.225.130]) by fmsmga008.fm.intel.com with ESMTP; 14 Aug 2019 08:23:02 -0700 Received: from orsmsx158.amr.corp.intel.com (10.22.240.20) by ORSMSX103.amr.corp.intel.com (10.22.225.130) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 14 Aug 2019 08:22:58 -0700 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.177]) by ORSMSX158.amr.corp.intel.com ([169.254.10.29]) with mapi id 14.03.0439.000; Wed, 14 Aug 2019 08:22:58 -0700 From: "Michael D Kinney" To: Andy Hayes , "devel@edk2.groups.io" , "Ni, Ray" , "Kinney, Michael D" CC: Leif Lindholm Subject: Re: [PATCH v1 0/1] Added GOP driver for DisplayLink-based Universal USB Docks to edk2-platforms Thread-Topic: [PATCH v1 0/1] Added GOP driver for DisplayLink-based Universal USB Docks to edk2-platforms Thread-Index: AdVSrd2PFMdqDEOiTxq+nuVLobGX2QABK/7w Date: Wed, 14 Aug 2019 15:22:57 +0000 Message-ID: References: In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.22.254.140] MIME-Version: 1.0 Return-Path: michael.d.kinney@intel.com Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_E92EE9817A31E24EB0585FDF735412F5B9D88D8CORSMSX113amrcor_" --_000_E92EE9817A31E24EB0585FDF735412F5B9D88D8CORSMSX113amrcor_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Andy, Thanks for the contribution. Is this patch for the edk2-platform repo? Th= e 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 wik= is. https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Pr= ocess 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 inconsistencie= s 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-standar= ds-specification I see a few #if 0 and commented out lines of code and TODO comments. Can y= ou please clean these up and if needed add Tianocore Bugzillas for future enhancements/work items fo= r this driver. https://bugzilla.tianocore.org/ I also noticed that you are using named fields to initialize C structures: STATIC CONST struct VideoMode ModeData[] =3D { + { + // 640 x 480 @ 60Hz + .Reserved2 =3D 2, + .PixelClock =3D 2517, + .HActive =3D 640, + .HBlanking =3D 160, + .HSyncOffset =3D 16, + .HSyncWidth =3D 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 th= e 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 b= e 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 hav= e 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@displaylink.com] Sent: Wednesday, August 14, 2019 7:44 AM To: devel@edk2.groups.io Cc: Leif Lindholm ; Kinney, Michael D Subject: [PATCH v1 0/1] Added GOP driver for DisplayLink-based Universal US= B Docks to edk2-platforms >>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 Uni= versal 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/Displa= yLinkGopDxe.inf create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/Edid.h create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/UsbDes= criptors.h create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/UsbDis= playLink.h create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/Capabi= lityDescriptor.c create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/Compon= entName.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/UsbDes= criptors.c create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/UsbDis= playLink.c create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/UsbTra= nsfer.c create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/VideoM= odes.c create mode 100644 Drivers/DisplayLink/DisplayLinkPkg/ReadMe.md -- 2.17.1 --_000_E92EE9817A31E24EB0585FDF735412F5B9D88D8CORSMSX113amrcor_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable

Hi Andy,

 

Thanks for the contribution.&n= bsp; 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’ feat= ure 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.githu= b.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers<= /a>

 

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-sta= ndards-specification

 

I see a few #if 0 and commented out lines of code an= d 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 in= itialize C structures:

 

STATIC CONST struct VideoMode ModeData[] =3D {

+  {

+     // 640 x 480 @ 60Hz

+    .Reserved2 =3D 2,

+    .PixelClock =3D 2517,

+    .HActive =3D 640,

+    .HBlanking =3D 160,

+    .HSyncOffset =3D 16,

+    .HSyncWidth =3D 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 th= is 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 indivi= dual GOP drivers. I have

added Ray to this thread who may have ideas on how t= o handle these hot

add events.

 

Thanks,

 

Mike

 

From: Andy Hayes [mailto:andy.hayes@displaylink.com]
Sent: Wednesday, August 14, 2019 7:44 AM
To: devel@edk2.groups.io
Cc: Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael = D <michael.d.kinney@intel.com>
Subject: [PATCH v1 0/1] Added GOP driver for DisplayLink-based Unive= rsal USB Docks to edk2-platforms

 

From 4a42eb997aeb3699217b40bf3bc47dec56458847 Mon Se= p 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 fo= r DisplayLink-based Universal USB Docks to edk2-platforms

 

andy.h= ayes@displaylink.com (1):

  Added GOP driver for USB Docks which use Disp= layLink chips.

 

.../DisplayLinkPkg/DisplayLinkPkg.dsc  &nb= sp;      |   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  &= nbsp;    |  587 ++++++++= ;+

.../DisplayLinkGop/UsbDescriptors.c   = ;        |  144 +++

.../DisplayLinkGop/UsbDisplayLink.c   = ;        | 1109 +++++= ;++++++++++++

.../DisplayLinkGop/UsbTransfer.c   &n= bsp;          |  180 += ;++

.../DisplayLinkGop/VideoModes.c   &nb= sp;           |  254= ++++

Drivers/DisplayLink/DisplayLinkPkg/ReadMe.md  |=    77 ++

Maintainers.txt      &= nbsp;           &nbs= p;            | = ;   5 +

15 files changed, 3972 insertions(+)<= /p>

create mode 100644 Drivers/DisplayLink/DisplayLinkPk= g/DisplayLinkPkg.dsc

create mode 100644 Drivers/DisplayLink/DisplayLinkPk= g/DisplayLinkGop/DisplayLinkGopDxe.inf

create mode 100644 Drivers/DisplayLink/DisplayLinkPk= g/DisplayLinkGop/Edid.h

create mode 100644 Drivers/DisplayLink/DisplayLinkPk= g/DisplayLinkGop/UsbDescriptors.h

create mode 100644 Drivers/DisplayLink/DisplayLinkPk= g/DisplayLinkGop/UsbDisplayLink.h

create mode 100644 Drivers/DisplayLink/DisplayLinkPk= g/DisplayLinkGop/CapabilityDescriptor.c

create mode 100644 Drivers/DisplayLink/DisplayLinkPk= g/DisplayLinkGop/ComponentName.c

create mode 100644 Drivers/DisplayLink/DisplayLinkPk= g/DisplayLinkGop/Edid.c

create mode 100644 Drivers/DisplayLink/DisplayLinkPk= g/DisplayLinkGop/Gop.c

create mode 100644 Drivers/DisplayLink/DisplayLinkPk= g/DisplayLinkGop/UsbDescriptors.c

create mode 100644 Drivers/DisplayLink/DisplayLinkPk= g/DisplayLinkGop/UsbDisplayLink.c

create mode 100644 Drivers/DisplayLink/DisplayLinkPk= g/DisplayLinkGop/UsbTransfer.c

create mode 100644 Drivers/DisplayLink/DisplayLinkPk= g/DisplayLinkGop/VideoModes.c

create mode 100644 Drivers/DisplayLink/DisplayLinkPk= g/ReadMe.md

 

--

2.17.1

--_000_E92EE9817A31E24EB0585FDF735412F5B9D88D8CORSMSX113amrcor_--