public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH V2 0/4] Remove unnecessary Map/Unmap in XhciDxe/EhciDxe for AsyncInterruptTransfer
@ 2018-10-26  5:42 Star Zeng
  2018-10-26  5:42 ` [PATCH V2 1/4] MdeModulePkg XhciDxe: Extract new XhciInsertAsyncIntTransfer function Star Zeng
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Star Zeng @ 2018-10-26  5:42 UTC (permalink / raw)
  To: edk2-devel; +Cc: Star Zeng, Ruiyu Ni, Hao Wu, Jian J Wang, Jiewen Yao

V2: Thanks for Ray's feedback.
Add the missing "FreePool (Data);".
Remove the unnecessary indentation change.

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1274

Please refer to the log message of each commit for more details.

Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Hao Wu <hao.a.wu@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>

Star Zeng (4):
  MdeModulePkg XhciDxe: Extract new XhciInsertAsyncIntTransfer function
  MdeModulePkg EhciDxe: Extract new EhciInsertAsyncIntTransfer function
  MdeModulePkg XhciDxe: Use common buffer for AsyncInterruptTransfer
  MdeModulePkg EhciDxe: Use common buffer for AsyncInterruptTransfer

 MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c      |  28 +----
 MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c | 115 ++++++++++---------
 MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.h |  36 +++++-
 MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.c   |  38 ++++++-
 MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.h   |  33 +++---
 MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c      |  19 +---
 MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 188 ++++++++++++++++++-------------
 MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h |  55 +++++++--
 8 files changed, 302 insertions(+), 210 deletions(-)

-- 
2.7.0.windows.1



^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH V2 1/4] MdeModulePkg XhciDxe: Extract new XhciInsertAsyncIntTransfer function
  2018-10-26  5:42 [PATCH V2 0/4] Remove unnecessary Map/Unmap in XhciDxe/EhciDxe for AsyncInterruptTransfer Star Zeng
@ 2018-10-26  5:42 ` Star Zeng
  2018-10-26  7:33   ` Wu, Hao A
  2018-10-26  5:42 ` [PATCH V2 2/4] MdeModulePkg EhciDxe: Extract new EhciInsertAsyncIntTransfer function Star Zeng
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 12+ messages in thread
From: Star Zeng @ 2018-10-26  5:42 UTC (permalink / raw)
  To: edk2-devel; +Cc: Star Zeng, Ruiyu Ni, Hao Wu, Jian J Wang, Jiewen Yao

V2:
Add the missing "FreePool (Data);".
Remove the unnecessary indentation change.

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1274

Extract new XhciInsertAsyncIntTransfer function from
XhcAsyncInterruptTransfer.

It is code preparation for following patch,
no essential functional change.

Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Hao Wu <hao.a.wu@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng <star.zeng@intel.com>
---
 MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c      | 18 +--------
 MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 65 ++++++++++++++++++++++++++++++++
 MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h | 28 ++++++++++++++
 3 files changed, 94 insertions(+), 17 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
index f1c60bef01c0..7f64f9c7c982 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
@@ -1346,7 +1346,6 @@ XhcAsyncInterruptTransfer (
   EFI_STATUS              Status;
   UINT8                   SlotId;
   UINT8                   Index;
-  UINT8                   *Data;
   EFI_TPL                 OldTpl;
 
   //
@@ -1413,36 +1412,21 @@ XhcAsyncInterruptTransfer (
     goto ON_EXIT;
   }
 
-  Data = AllocateZeroPool (DataLength);
-
-  if (Data == NULL) {
-    DEBUG ((EFI_D_ERROR, "XhcAsyncInterruptTransfer: failed to allocate buffer\n"));
-    Status = EFI_OUT_OF_RESOURCES;
-    goto ON_EXIT;
-  }
-
-  Urb = XhcCreateUrb (
+  Urb = XhciInsertAsyncIntTransfer (
           Xhc,
           DeviceAddress,
           EndPointAddress,
           DeviceSpeed,
           MaximumPacketLength,
-          XHC_INT_TRANSFER_ASYNC,
-          NULL,
-          Data,
           DataLength,
           CallBackFunction,
           Context
           );
-
   if (Urb == NULL) {
-    DEBUG ((EFI_D_ERROR, "XhcAsyncInterruptTransfer: failed to create URB\n"));
-    FreePool (Data);
     Status = EFI_OUT_OF_RESOURCES;
     goto ON_EXIT;
   }
 
-  InsertHeadList (&Xhc->AsyncIntTransfers, &Urb->UrbList);
   //
   // Ring the doorbell
   //
diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
index 166c44bf5e66..75959ae08363 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
@@ -1411,6 +1411,71 @@ XhciDelAllAsyncIntTransfers (
 }
 
 /**
+  Insert a single asynchronous interrupt transfer for
+  the device and endpoint.
+
+  @param Xhc            The XHCI Instance
+  @param BusAddr        The logical device address assigned by UsbBus driver
+  @param EpAddr         Endpoint addrress
+  @param DevSpeed       The device speed
+  @param MaxPacket      The max packet length of the endpoint
+  @param DataLen        The length of data buffer
+  @param Callback       The function to call when data is transferred
+  @param Context        The context to the callback
+
+  @return Created URB or NULL
+
+**/
+URB *
+XhciInsertAsyncIntTransfer (
+  IN USB_XHCI_INSTANCE                  *Xhc,
+  IN UINT8                              BusAddr,
+  IN UINT8                              EpAddr,
+  IN UINT8                              DevSpeed,
+  IN UINTN                              MaxPacket,
+  IN UINTN                              DataLen,
+  IN EFI_ASYNC_USB_TRANSFER_CALLBACK    Callback,
+  IN VOID                               *Context
+  )
+{
+  VOID      *Data;
+  URB       *Urb;
+
+  Data = AllocateZeroPool (DataLen);
+  if (Data == NULL) {
+    DEBUG ((DEBUG_ERROR, "%a: failed to allocate buffer\n", __FUNCTION__));
+    return NULL;
+  }
+
+  Urb = XhcCreateUrb (
+          Xhc,
+          BusAddr,
+          EpAddr,
+          DevSpeed,
+          MaxPacket,
+          XHC_INT_TRANSFER_ASYNC,
+          NULL,
+          Data,
+          DataLen,
+          Callback,
+          Context
+          );
+  if (Urb == NULL) {
+    DEBUG ((DEBUG_ERROR, "%a: failed to create URB\n", __FUNCTION__));
+    FreePool (Data);
+    return NULL;
+  }
+
+  //
+  // New asynchronous transfer must inserted to the head.
+  // Check the comments in XhcMoniteAsyncRequests
+  //
+  InsertHeadList (&Xhc->AsyncIntTransfers, &Urb->UrbList);
+
+  return Urb;
+}
+
+/**
   Update the queue head for next round of asynchronous transfer
 
   @param  Xhc     The XHCI Instance.
diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
index 097408828a1f..cd1403f2842a 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
@@ -853,6 +853,34 @@ XhciDelAllAsyncIntTransfers (
   );
 
 /**
+  Insert a single asynchronous interrupt transfer for
+  the device and endpoint.
+
+  @param Xhc            The XHCI Instance
+  @param BusAddr        The logical device address assigned by UsbBus driver
+  @param EpAddr         Endpoint addrress
+  @param DevSpeed       The device speed
+  @param MaxPacket      The max packet length of the endpoint
+  @param DataLen        The length of data buffer
+  @param Callback       The function to call when data is transferred
+  @param Context        The context to the callback
+
+  @return Created URB or NULL
+
+**/
+URB *
+XhciInsertAsyncIntTransfer (
+  IN USB_XHCI_INSTANCE                  *Xhc,
+  IN UINT8                              DeviceAddress,
+  IN UINT8                              EndPointAddress,
+  IN UINT8                              DeviceSpeed,
+  IN UINTN                              MaximumPacketLength,
+  IN UINTN                              DataLength,
+  IN EFI_ASYNC_USB_TRANSFER_CALLBACK    CallBackFunction,
+  IN VOID                               *Context
+  );
+
+/**
   Set Bios Ownership
 
   @param  Xhc          The XHCI Instance.
-- 
2.7.0.windows.1



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH V2 2/4] MdeModulePkg EhciDxe: Extract new EhciInsertAsyncIntTransfer function
  2018-10-26  5:42 [PATCH V2 0/4] Remove unnecessary Map/Unmap in XhciDxe/EhciDxe for AsyncInterruptTransfer Star Zeng
  2018-10-26  5:42 ` [PATCH V2 1/4] MdeModulePkg XhciDxe: Extract new XhciInsertAsyncIntTransfer function Star Zeng
@ 2018-10-26  5:42 ` Star Zeng
  2018-10-26  7:54   ` Wu, Hao A
  2018-10-26  5:42 ` [PATCH V2 3/4] MdeModulePkg XhciDxe: Use common buffer for AsyncInterruptTransfer Star Zeng
  2018-10-26  5:42 ` [PATCH V2 4/4] MdeModulePkg EhciDxe: " Star Zeng
  3 siblings, 1 reply; 12+ messages in thread
From: Star Zeng @ 2018-10-26  5:42 UTC (permalink / raw)
  To: edk2-devel; +Cc: Star Zeng, Ruiyu Ni, Hao Wu, Jian J Wang, Jiewen Yao

V2:
Add the missing "gBS->FreePool (Data);".

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1274

Extract new EhciInsertAsyncIntTransfer function from
EhcAsyncInterruptTransfer.

It is code preparation for following patch,
no essential functional change.

Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Hao Wu <hao.a.wu@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng <star.zeng@intel.com>
---
 MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c      | 25 +----------
 MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c | 77 ++++++++++++++++++++++++++++++++
 MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.h | 36 ++++++++++++++-
 3 files changed, 113 insertions(+), 25 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c b/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
index 50b5598df4fb..5569f4f9618b 100644
--- a/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
+++ b/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
@@ -997,7 +997,6 @@ EhcAsyncInterruptTransfer (
   URB                     *Urb;
   EFI_TPL                 OldTpl;
   EFI_STATUS              Status;
-  UINT8                   *Data;
 
   //
   // Validate parameters
@@ -1046,16 +1045,7 @@ EhcAsyncInterruptTransfer (
 
   EhcAckAllInterrupt (Ehc);
 
-  Data = AllocatePool (DataLength);
-
-  if (Data == NULL) {
-    DEBUG ((EFI_D_ERROR, "EhcAsyncInterruptTransfer: failed to allocate buffer\n"));
-
-    Status = EFI_OUT_OF_RESOURCES;
-    goto ON_EXIT;
-  }
-
-  Urb = EhcCreateUrb (
+  Urb = EhciInsertAsyncIntTransfer (
           Ehc,
           DeviceAddress,
           EndPointAddress,
@@ -1063,9 +1053,6 @@ EhcAsyncInterruptTransfer (
           *DataToggle,
           MaximumPacketLength,
           Translator,
-          EHC_INT_TRANSFER_ASYNC,
-          NULL,
-          Data,
           DataLength,
           CallBackFunction,
           Context,
@@ -1073,20 +1060,10 @@ EhcAsyncInterruptTransfer (
           );
 
   if (Urb == NULL) {
-    DEBUG ((EFI_D_ERROR, "EhcAsyncInterruptTransfer: failed to create URB\n"));
-
-    gBS->FreePool (Data);
     Status = EFI_OUT_OF_RESOURCES;
     goto ON_EXIT;
   }
 
-  //
-  // New asynchronous transfer must inserted to the head.
-  // Check the comments in EhcMoniteAsyncRequests
-  //
-  EhcLinkQhToPeriod (Ehc, Urb->Qh);
-  InsertHeadList (&Ehc->AsyncIntTransfers, &Urb->UrbList);
-
 ON_EXIT:
   Ehc->PciIo->Flush (Ehc->PciIo);
   gBS->RestoreTPL (OldTpl);
diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c b/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c
index 168280be81d7..2d202d439d1c 100644
--- a/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c
+++ b/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c
@@ -814,6 +814,83 @@ EhciDelAllAsyncIntTransfers (
   }
 }
 
+/**
+  Insert a single asynchronous interrupt transfer for
+  the device and endpoint.
+
+  @param  Ehc               The EHCI device.
+  @param  DevAddr           The device address.
+  @param  EpAddr            Endpoint addrress & its direction.
+  @param  DevSpeed          The device speed.
+  @param  Toggle            Initial data toggle to use.
+  @param  MaxPacket         The max packet length of the endpoint.
+  @param  Hub               The transaction translator to use.
+  @param  Data              The user data to transfer.
+  @param  DataLen           The length of data buffer.
+  @param  Callback          The function to call when data is transferred.
+  @param  Context           The context to the callback.
+  @param  Interval          The interval for interrupt transfer.
+
+  @return Created URB or NULL.
+
+**/
+URB *
+EhciInsertAsyncIntTransfer (
+  IN USB2_HC_DEV                        *Ehc,
+  IN UINT8                              DevAddr,
+  IN UINT8                              EpAddr,
+  IN UINT8                              DevSpeed,
+  IN UINT8                              Toggle,
+  IN UINTN                              MaxPacket,
+  IN EFI_USB2_HC_TRANSACTION_TRANSLATOR *Hub,
+  IN UINTN                              DataLen,
+  IN EFI_ASYNC_USB_TRANSFER_CALLBACK    Callback,
+  IN VOID                               *Context,
+  IN UINTN                              Interval
+  )
+{
+  VOID      *Data;
+  URB       *Urb;
+
+  Data = AllocatePool (DataLen);
+
+  if (Data == NULL) {
+    DEBUG ((DEBUG_ERROR, "%a: failed to allocate buffer\n", __FUNCTION__));
+    return NULL;
+  }
+
+  Urb = EhcCreateUrb (
+          Ehc,
+          DevAddr,
+          EpAddr,
+          DevSpeed,
+          Toggle,
+          MaxPacket,
+          Hub,
+          EHC_INT_TRANSFER_ASYNC,
+          NULL,
+          Data,
+          DataLen,
+          Callback,
+          Context,
+          Interval
+          );
+
+  if (Urb == NULL) {
+    DEBUG ((DEBUG_ERROR, "%a: failed to create URB\n", __FUNCTION__));
+    gBS->FreePool (Data);
+    return NULL;
+  }
+
+  //
+  // New asynchronous transfer must inserted to the head.
+  // Check the comments in EhcMoniteAsyncRequests
+  //
+  EhcLinkQhToPeriod (Ehc, Urb->Qh);
+  InsertHeadList (&Ehc->AsyncIntTransfers, &Urb->UrbList);
+
+  return Urb;
+}
 
 /**
   Flush data from PCI controller specific address to mapped system
diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.h b/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.h
index c03bd619d7e8..b852e6327f37 100644
--- a/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.h
+++ b/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.h
@@ -2,7 +2,7 @@
 
   This file contains the definination for host controller schedule routines.
 
-Copyright (c) 2007 - 2009, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.<BR>
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD License
 which accompanies this distribution.  The full text of the license may be found at
@@ -162,6 +162,40 @@ EhciDelAllAsyncIntTransfers (
   IN USB2_HC_DEV          *Ehc
   );
 
+/**
+  Insert a single asynchronous interrupt transfer for
+  the device and endpoint.
+
+  @param  Ehc               The EHCI device.
+  @param  DevAddr           The device address.
+  @param  EpAddr            Endpoint addrress & its direction.
+  @param  DevSpeed          The device speed.
+  @param  Toggle            Initial data toggle to use.
+  @param  MaxPacket         The max packet length of the endpoint.
+  @param  Hub               The transaction translator to use.
+  @param  Data              The user data to transfer.
+  @param  DataLen           The length of data buffer.
+  @param  Callback          The function to call when data is transferred.
+  @param  Context           The context to the callback.
+  @param  Interval          The interval for interrupt transfer.
+
+  @return Created URB or NULL.
+
+**/
+URB *
+EhciInsertAsyncIntTransfer (
+  IN USB2_HC_DEV                        *Ehc,
+  IN UINT8                              BusAddr,
+  IN UINT8                              EpAddr,
+  IN UINT8                              DevSpeed,
+  IN UINT8                              Toggle,
+  IN UINTN                              MaxPacket,
+  IN EFI_USB2_HC_TRANSACTION_TRANSLATOR *Hub,
+  IN UINTN                              DataLen,
+  IN EFI_ASYNC_USB_TRANSFER_CALLBACK    Callback,
+  IN VOID                               *Context,
+  IN UINTN                              Interval
+  );
 
 /**
   Interrupt transfer periodic check handler.
-- 
2.7.0.windows.1



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH V2 3/4] MdeModulePkg XhciDxe: Use common buffer for AsyncInterruptTransfer
  2018-10-26  5:42 [PATCH V2 0/4] Remove unnecessary Map/Unmap in XhciDxe/EhciDxe for AsyncInterruptTransfer Star Zeng
  2018-10-26  5:42 ` [PATCH V2 1/4] MdeModulePkg XhciDxe: Extract new XhciInsertAsyncIntTransfer function Star Zeng
  2018-10-26  5:42 ` [PATCH V2 2/4] MdeModulePkg EhciDxe: Extract new EhciInsertAsyncIntTransfer function Star Zeng
@ 2018-10-26  5:42 ` Star Zeng
  2018-10-26  7:38   ` Wu, Hao A
  2018-10-26  5:42 ` [PATCH V2 4/4] MdeModulePkg EhciDxe: " Star Zeng
  3 siblings, 1 reply; 12+ messages in thread
From: Star Zeng @ 2018-10-26  5:42 UTC (permalink / raw)
  To: edk2-devel; +Cc: Star Zeng, Ruiyu Ni, Hao Wu, Jian J Wang, Jiewen Yao

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1274

In current code, XhcMonitorAsyncRequests (timer handler) will do
unmap and map operations for AsyncIntTransfers to "Flush data from
PCI controller specific address to mapped system memory address".
XhcMonitorAsyncRequests
  XhcFlushAsyncIntMap
    PciIo->Unmap
      IoMmu->SetAttribute
    PciIo->Map
      IoMmu->SetAttribute

This may impact the boot performance.

Since the data buffer for XhcMonitorAsyncRequests is internal
buffer, we can allocate common buffer by PciIo->AllocateBuffer
and map the buffer with EfiPciIoOperationBusMasterCommonBuffer,
then the unmap and map operations can be removed.

///
/// Provides both read and write access to system memory by
/// both the processor and a bus master. The buffer is coherent
/// from both the processor's and the bus master's point of view.
///
EfiPciIoOperationBusMasterCommonBuffer,

Test done:
USB KB works normally.
USB disk read/write works normally.

Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Hao Wu <hao.a.wu@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng <star.zeng@intel.com>
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
---
 MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c      |   1 +
 MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 141 +++++++++++--------------------
 MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h |  27 +++---
 3 files changed, 67 insertions(+), 102 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
index 7f64f9c7c982..64855a4c158c 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
@@ -769,6 +769,7 @@ XhcTransfer (
           MaximumPacketLength,
           Type,
           Request,
+          FALSE,
           Data,
           *DataLength,
           NULL,
diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
index 75959ae08363..d03a6681ce0d 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
@@ -118,17 +118,18 @@ ON_EXIT:
 /**
   Create a new URB for a new transaction.
 
-  @param  Xhc       The XHCI Instance
-  @param  BusAddr   The logical device address assigned by UsbBus driver
-  @param  EpAddr    Endpoint addrress
-  @param  DevSpeed  The device speed
-  @param  MaxPacket The max packet length of the endpoint
-  @param  Type      The transaction type
-  @param  Request   The standard USB request for control transfer
-  @param  Data      The user data to transfer
-  @param  DataLen   The length of data buffer
-  @param  Callback  The function to call when data is transferred
-  @param  Context   The context to the callback
+  @param  Xhc                   The XHCI Instance
+  @param  BusAddr               The logical device address assigned by UsbBus driver
+  @param  EpAddr                Endpoint addrress
+  @param  DevSpeed              The device speed
+  @param  MaxPacket             The max packet length of the endpoint
+  @param  Type                  The transaction type
+  @param  Request               The standard USB request for control transfer
+  @param  AllocateCommonBuffer  Indicate whether need to allocate common buffer for data transfer
+  @param  Data                  The user data to transfer, NULL if AllocateCommonBuffer is TRUE
+  @param  DataLen               The length of data buffer
+  @param  Callback              The function to call when data is transferred
+  @param  Context               The context to the callback
 
   @return Created URB or NULL
 
@@ -142,6 +143,7 @@ XhcCreateUrb (
   IN UINTN                              MaxPacket,
   IN UINTN                              Type,
   IN EFI_USB_DEVICE_REQUEST             *Request,
+  IN BOOLEAN                            AllocateCommonBuffer,
   IN VOID                               *Data,
   IN UINTN                              DataLen,
   IN EFI_ASYNC_USB_TRANSFER_CALLBACK    Callback,
@@ -169,8 +171,24 @@ XhcCreateUrb (
   Ep->Type      = Type;
 
   Urb->Request  = Request;
+  if (AllocateCommonBuffer) {
+    ASSERT (Data == NULL);
+    Status = Xhc->PciIo->AllocateBuffer (
+                           Xhc->PciIo,
+                           AllocateAnyPages,
+                           EfiBootServicesData,
+                           EFI_SIZE_TO_PAGES (DataLen),
+                           &Data,
+                           0
+                           );
+    if (EFI_ERROR (Status) || (Data == NULL)) {
+      FreePool (Urb);
+      return NULL;
+    }
+  }
   Urb->Data     = Data;
   Urb->DataLen  = DataLen;
+  Urb->AllocateCommonBuffer = AllocateCommonBuffer;
   Urb->Callback = Callback;
   Urb->Context  = Context;
 
@@ -178,6 +196,11 @@ XhcCreateUrb (
   ASSERT_EFI_ERROR (Status);
   if (EFI_ERROR (Status)) {
     DEBUG ((EFI_D_ERROR, "XhcCreateUrb: XhcCreateTransferTrb Failed, Status = %r\n", Status));
+    Xhc->PciIo->FreeBuffer (
+                  Xhc->PciIo,
+                  EFI_SIZE_TO_PAGES (Urb->DataLen),
+                  Urb->Data
+                  );
     FreePool (Urb);
     Urb = NULL;
   }
@@ -206,6 +229,14 @@ XhcFreeUrb (
     Xhc->PciIo->Unmap (Xhc->PciIo, Urb->DataMap);
   }
 
+  if (Urb->AllocateCommonBuffer) {
+    Xhc->PciIo->FreeBuffer (
+                  Xhc->PciIo,
+                  EFI_SIZE_TO_PAGES (Urb->DataLen),
+                  Urb->Data
+                  );
+  }
+
   FreePool (Urb);
 }
 
@@ -264,10 +295,14 @@ XhcCreateTransferTrb (
   // No need to remap.
   //
   if ((Urb->Data != NULL) && (Urb->DataMap == NULL)) {
-    if (((UINT8) (Urb->Ep.Direction)) == EfiUsbDataIn) {
-      MapOp = EfiPciIoOperationBusMasterWrite;
+    if (Urb->AllocateCommonBuffer) {
+      MapOp = EfiPciIoOperationBusMasterCommonBuffer;
     } else {
-      MapOp = EfiPciIoOperationBusMasterRead;
+      if (((UINT8) (Urb->Ep.Direction)) == EfiUsbDataIn) {
+        MapOp = EfiPciIoOperationBusMasterWrite;
+      } else {
+        MapOp = EfiPciIoOperationBusMasterRead;
+      }
     }
 
     Len = Urb->DataLen;
@@ -1367,7 +1402,6 @@ XhciDelAsyncIntTransfer (
       }
 
       RemoveEntryList (&Urb->UrbList);
-      FreePool (Urb->Data);
       XhcFreeUrb (Xhc, Urb);
       return EFI_SUCCESS;
     }
@@ -1405,7 +1439,6 @@ XhciDelAllAsyncIntTransfers (
     }
 
     RemoveEntryList (&Urb->UrbList);
-    FreePool (Urb->Data);
     XhcFreeUrb (Xhc, Urb);
   }
 }
@@ -1438,15 +1471,8 @@ XhciInsertAsyncIntTransfer (
   IN VOID                               *Context
   )
 {
-  VOID      *Data;
   URB       *Urb;
 
-  Data = AllocateZeroPool (DataLen);
-  if (Data == NULL) {
-    DEBUG ((DEBUG_ERROR, "%a: failed to allocate buffer\n", __FUNCTION__));
-    return NULL;
-  }
-
   Urb = XhcCreateUrb (
           Xhc,
           BusAddr,
@@ -1455,14 +1481,14 @@ XhciInsertAsyncIntTransfer (
           MaxPacket,
           XHC_INT_TRANSFER_ASYNC,
           NULL,
-          Data,
+          TRUE,
+          NULL,
           DataLen,
           Callback,
           Context
           );
   if (Urb == NULL) {
     DEBUG ((DEBUG_ERROR, "%a: failed to create URB\n", __FUNCTION__));
-    FreePool (Data);
     return NULL;
   }
 
@@ -1503,61 +1529,6 @@ XhcUpdateAsyncRequest (
 }
 
 /**
-  Flush data from PCI controller specific address to mapped system
-  memory address.
-
-  @param  Xhc                The XHCI device.
-  @param  Urb                The URB to unmap.
-
-  @retval EFI_SUCCESS        Success to flush data to mapped system memory.
-  @retval EFI_DEVICE_ERROR   Fail to flush data to mapped system memory.
-
-**/
-EFI_STATUS
-XhcFlushAsyncIntMap (
-  IN  USB_XHCI_INSTANCE   *Xhc,
-  IN  URB                 *Urb
-  )
-{
-  EFI_STATUS                    Status;
-  EFI_PHYSICAL_ADDRESS          PhyAddr;
-  EFI_PCI_IO_PROTOCOL_OPERATION MapOp;
-  EFI_PCI_IO_PROTOCOL           *PciIo;
-  UINTN                         Len;
-  VOID                          *Map;
-
-  PciIo = Xhc->PciIo;
-  Len   = Urb->DataLen;
-
-  if (Urb->Ep.Direction == EfiUsbDataIn) {
-    MapOp = EfiPciIoOperationBusMasterWrite;
-  } else {
-    MapOp = EfiPciIoOperationBusMasterRead;
-  }
-
-  if (Urb->DataMap != NULL) {
-    Status = PciIo->Unmap (PciIo, Urb->DataMap);
-    if (EFI_ERROR (Status)) {
-      goto ON_ERROR;
-    }
-  }
-
-  Urb->DataMap = NULL;
-
-  Status = PciIo->Map (PciIo, MapOp, Urb->Data, &Len, &PhyAddr, &Map);
-  if (EFI_ERROR (Status) || (Len != Urb->DataLen)) {
-    goto ON_ERROR;
-  }
-
-  Urb->DataPhy  = (VOID *) ((UINTN) PhyAddr);
-  Urb->DataMap  = Map;
-  return EFI_SUCCESS;
-
-ON_ERROR:
-  return EFI_DEVICE_ERROR;
-}
-
-/**
   Interrupt transfer periodic check handler.
 
   @param  Event                 Interrupt event.
@@ -1577,7 +1548,6 @@ XhcMonitorAsyncRequests (
   UINT8                   *ProcBuf;
   URB                     *Urb;
   UINT8                   SlotId;
-  EFI_STATUS              Status;
   EFI_TPL                 OldTpl;
 
   OldTpl = gBS->RaiseTPL (XHC_TPL);
@@ -1606,15 +1576,6 @@ XhcMonitorAsyncRequests (
     }
 
     //
-    // Flush any PCI posted write transactions from a PCI host
-    // bridge to system memory.
-    //
-    Status = XhcFlushAsyncIntMap (Xhc, Urb);
-    if (EFI_ERROR (Status)) {
-      DEBUG ((EFI_D_ERROR, "XhcMonitorAsyncRequests: Fail to Flush AsyncInt Mapped Memeory\n"));
-    }
-
-    //
     // Allocate a buffer then copy the transferred data for user.
     // If failed to allocate the buffer, update the URB for next
     // round of transfer. Ignore the data of this round.
diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
index cd1403f2842a..2b5b95b7fb60 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
@@ -172,6 +172,7 @@ typedef struct _URB {
   //
   USB_ENDPOINT                    Ep;
   EFI_USB_DEVICE_REQUEST          *Request;
+  BOOLEAN                         AllocateCommonBuffer;
   VOID                            *Data;
   UINTN                           DataLen;
   VOID                            *DataPhy;
@@ -1432,17 +1433,18 @@ XhcSetTrDequeuePointer (
 /**
   Create a new URB for a new transaction.
 
-  @param  Xhc       The XHCI Instance
-  @param  DevAddr   The device address
-  @param  EpAddr    Endpoint addrress
-  @param  DevSpeed  The device speed
-  @param  MaxPacket The max packet length of the endpoint
-  @param  Type      The transaction type
-  @param  Request   The standard USB request for control transfer
-  @param  Data      The user data to transfer
-  @param  DataLen   The length of data buffer
-  @param  Callback  The function to call when data is transferred
-  @param  Context   The context to the callback
+  @param  Xhc                   The XHCI Instance
+  @param  BusAddr               The logical device address assigned by UsbBus driver
+  @param  EpAddr                Endpoint addrress
+  @param  DevSpeed              The device speed
+  @param  MaxPacket             The max packet length of the endpoint
+  @param  Type                  The transaction type
+  @param  Request               The standard USB request for control transfer
+  @param  AllocateCommonBuffer  Indicate whether need to allocate common buffer for data transfer
+  @param  Data                  The user data to transfer, NULL if AllocateCommonBuffer is TRUE
+  @param  DataLen               The length of data buffer
+  @param  Callback              The function to call when data is transferred
+  @param  Context               The context to the callback
 
   @return Created URB or NULL
 
@@ -1450,12 +1452,13 @@ XhcSetTrDequeuePointer (
 URB*
 XhcCreateUrb (
   IN USB_XHCI_INSTANCE                  *Xhc,
-  IN UINT8                              DevAddr,
+  IN UINT8                              BusAddr,
   IN UINT8                              EpAddr,
   IN UINT8                              DevSpeed,
   IN UINTN                              MaxPacket,
   IN UINTN                              Type,
   IN EFI_USB_DEVICE_REQUEST             *Request,
+  IN BOOLEAN                            AllocateCommonBuffer,
   IN VOID                               *Data,
   IN UINTN                              DataLen,
   IN EFI_ASYNC_USB_TRANSFER_CALLBACK    Callback,
-- 
2.7.0.windows.1



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH V2 4/4] MdeModulePkg EhciDxe: Use common buffer for AsyncInterruptTransfer
  2018-10-26  5:42 [PATCH V2 0/4] Remove unnecessary Map/Unmap in XhciDxe/EhciDxe for AsyncInterruptTransfer Star Zeng
                   ` (2 preceding siblings ...)
  2018-10-26  5:42 ` [PATCH V2 3/4] MdeModulePkg XhciDxe: Use common buffer for AsyncInterruptTransfer Star Zeng
@ 2018-10-26  5:42 ` Star Zeng
  2018-10-26  8:12   ` Wu, Hao A
  3 siblings, 1 reply; 12+ messages in thread
From: Star Zeng @ 2018-10-26  5:42 UTC (permalink / raw)
  To: edk2-devel; +Cc: Star Zeng, Ruiyu Ni, Hao Wu, Jian J Wang, Jiewen Yao

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1274

In current code, EhcMonitorAsyncRequests (timer handler) will do
unmap and map operations for AsyncIntTransfers to "Flush data from
PCI controller specific address to mapped system memory address".
EhcMonitorAsyncRequests
  EhcFlushAsyncIntMap
    PciIo->Unmap
      IoMmu->SetAttribute
    PciIo->Map
      IoMmu->SetAttribute

This may impact the boot performance.

Since the data buffer for EhcMonitorAsyncRequests is internal
buffer, we can allocate common buffer by PciIo->AllocateBuffer
and map the buffer with EfiPciIoOperationBusMasterCommonBuffer,
then the unmap and map operations can be removed.

///
/// Provides both read and write access to system memory by
/// both the processor and a bus master. The buffer is coherent
/// from both the processor's and the bus master's point of view.
///
EfiPciIoOperationBusMasterCommonBuffer,

Test done:
USB KB works normally.
USB disk read/write works normally.

Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Hao Wu <hao.a.wu@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng <star.zeng@intel.com>
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
---
 MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c      |  3 ++
 MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c | 78 +-------------------------------
 MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.c   | 38 ++++++++++++++--
 MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.h   | 33 ++++++++------
 4 files changed, 57 insertions(+), 95 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c b/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
index 5569f4f9618b..764eeda58ba1 100644
--- a/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
+++ b/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
@@ -763,6 +763,7 @@ EhcControlTransfer (
           Translator,
           EHC_CTRL_TRANSFER,
           Request,
+          FALSE,
           Data,
           *DataLength,
           NULL,
@@ -906,6 +907,7 @@ EhcBulkTransfer (
           Translator,
           EHC_BULK_TRANSFER,
           NULL,
+          FALSE,
           Data[0],
           *DataLength,
           NULL,
@@ -1163,6 +1165,7 @@ EhcSyncInterruptTransfer (
           Translator,
           EHC_INT_TRANSFER_SYNC,
           NULL,
+          FALSE,
           Data,
           *DataLength,
           NULL,
diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c b/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c
index 2d202d439d1c..f1edcf20e342 100644
--- a/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c
+++ b/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c
@@ -778,7 +778,6 @@ EhciDelAsyncIntTransfer (
       EhcUnlinkQhFromPeriod (Ehc, Urb->Qh);
       RemoveEntryList (&Urb->UrbList);
 
-      gBS->FreePool (Urb->Data);
       EhcFreeUrb (Ehc, Urb);
       return EFI_SUCCESS;
     }
@@ -809,7 +808,6 @@ EhciDelAllAsyncIntTransfers (
     EhcUnlinkQhFromPeriod (Ehc, Urb->Qh);
     RemoveEntryList (&Urb->UrbList);
 
-    gBS->FreePool (Urb->Data);
     EhcFreeUrb (Ehc, Urb);
   }
 }
@@ -849,16 +847,8 @@ EhciInsertAsyncIntTransfer (
   IN UINTN                              Interval
   )
 {
-  VOID      *Data;
   URB       *Urb;
 
-  Data = AllocatePool (DataLen);
-
-  if (Data == NULL) {
-    DEBUG ((DEBUG_ERROR, "%a: failed to allocate buffer\n", __FUNCTION__));
-    return NULL;
-  }
-
   Urb = EhcCreateUrb (
           Ehc,
           DevAddr,
@@ -869,7 +859,8 @@ EhciInsertAsyncIntTransfer (
           Hub,
           EHC_INT_TRANSFER_ASYNC,
           NULL,
-          Data,
+          TRUE,
+          NULL,
           DataLen,
           Callback,
           Context,
@@ -878,7 +869,6 @@ EhciInsertAsyncIntTransfer (
 
   if (Urb == NULL) {
     DEBUG ((DEBUG_ERROR, "%a: failed to create URB\n", __FUNCTION__));
-    gBS->FreePool (Data);
     return NULL;
   }
 
@@ -893,60 +883,6 @@ EhciInsertAsyncIntTransfer (
 }
 
 /**
-  Flush data from PCI controller specific address to mapped system
-  memory address.
-
-  @param  Ehc                The EHCI device.
-  @param  Urb                The URB to unmap.
-
-  @retval EFI_SUCCESS        Success to flush data to mapped system memory.
-  @retval EFI_DEVICE_ERROR   Fail to flush data to mapped system memory.
-
-**/
-EFI_STATUS
-EhcFlushAsyncIntMap (
-  IN  USB2_HC_DEV         *Ehc,
-  IN  URB                 *Urb
-  )
-{
-  EFI_STATUS                    Status;
-  EFI_PHYSICAL_ADDRESS          PhyAddr;
-  EFI_PCI_IO_PROTOCOL_OPERATION MapOp;
-  EFI_PCI_IO_PROTOCOL           *PciIo;
-  UINTN                         Len;
-  VOID                          *Map;
-
-  PciIo = Ehc->PciIo;
-  Len   = Urb->DataLen;
-
-  if (Urb->Ep.Direction == EfiUsbDataIn) {
-    MapOp = EfiPciIoOperationBusMasterWrite;
-  } else {
-    MapOp = EfiPciIoOperationBusMasterRead;
-  }
-
-  Status = PciIo->Unmap (PciIo, Urb->DataMap);
-  if (EFI_ERROR (Status)) {
-    goto ON_ERROR;
-  }
-
-  Urb->DataMap = NULL;
-
-  Status = PciIo->Map (PciIo, MapOp, Urb->Data, &Len, &PhyAddr, &Map);
-  if (EFI_ERROR (Status) || (Len != Urb->DataLen)) {
-    goto ON_ERROR;
-  }
-
-  Urb->DataPhy  = (VOID *) ((UINTN) PhyAddr);
-  Urb->DataMap  = Map;
-  return EFI_SUCCESS;
-
-ON_ERROR:
-  return EFI_DEVICE_ERROR;
-}
-
-
-/**
   Update the queue head for next round of asynchronous transfer.
 
   @param  Ehc                   The EHCI device.
@@ -1051,7 +987,6 @@ EhcMonitorAsyncRequests (
   BOOLEAN                 Finished;
   UINT8                   *ProcBuf;
   URB                     *Urb;
-  EFI_STATUS              Status;
 
   OldTpl  = gBS->RaiseTPL (EHC_TPL);
   Ehc     = (USB2_HC_DEV *) Context;
@@ -1070,15 +1005,6 @@ EhcMonitorAsyncRequests (
     }
 
     //
-    // Flush any PCI posted write transactions from a PCI host
-    // bridge to system memory.
-    //
-    Status = EhcFlushAsyncIntMap (Ehc, Urb);
-    if (EFI_ERROR (Status)) {
-      DEBUG ((EFI_D_ERROR, "EhcMonitorAsyncRequests: Fail to Flush AsyncInt Mapped Memeory\n"));
-    }
-
-    //
     // Allocate a buffer then copy the transferred data for user.
     // If failed to allocate the buffer, update the URB for next
     // round of transfer. Ignore the data of this round.
diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.c b/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.c
index 6afb327df165..09c12776ecdb 100644
--- a/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.c
+++ b/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.c
@@ -339,6 +339,14 @@ EhcFreeUrb (
     PciIo->Unmap (PciIo, Urb->DataMap);
   }
 
+  if (Urb->AllocateCommonBuffer) {
+    PciIo->FreeBuffer (
+             PciIo,
+             EFI_SIZE_TO_PAGES (Urb->DataLen),
+             Urb->Data
+             );
+  }
+
   if (Urb->Qh != NULL) {
     //
     // Ensure that this queue head has been unlinked from the
@@ -529,7 +537,8 @@ ON_ERROR:
   @param  Hub                   The transaction translator to use.
   @param  Type                  The transaction type.
   @param  Request               The standard USB request for control transfer.
-  @param  Data                  The user data to transfer.
+  @param  AllocateCommonBuffer  Indicate whether need to allocate common buffer for data transfer.
+  @param  Data                  The user data to transfer, NULL if AllocateCommonBuffer is TRUE.
   @param  DataLen               The length of data buffer.
   @param  Callback              The function to call when data is transferred.
   @param  Context               The context to the callback.
@@ -549,6 +558,7 @@ EhcCreateUrb (
   IN EFI_USB2_HC_TRANSACTION_TRANSLATOR *Hub,
   IN UINTN                              Type,
   IN EFI_USB_DEVICE_REQUEST             *Request,
+  IN BOOLEAN                            AllocateCommonBuffer,
   IN VOID                               *Data,
   IN UINTN                              DataLen,
   IN EFI_ASYNC_USB_TRANSFER_CALLBACK    Callback,
@@ -596,8 +606,24 @@ EhcCreateUrb (
   Ep->PollRate    = EhcConvertPollRate (Interval);
 
   Urb->Request    = Request;
+  if (AllocateCommonBuffer) {
+    ASSERT (Data == NULL);
+    Status = Ehc->PciIo->AllocateBuffer (
+                           Ehc->PciIo,
+                           AllocateAnyPages,
+                           EfiBootServicesData,
+                           EFI_SIZE_TO_PAGES (DataLen),
+                           &Data,
+                           0
+                           );
+    if (EFI_ERROR (Status) || (Data == NULL)) {
+      FreePool (Urb);
+      return NULL;
+    }
+  }
   Urb->Data       = Data;
   Urb->DataLen    = DataLen;
+  Urb->AllocateCommonBuffer = AllocateCommonBuffer;
   Urb->Callback   = Callback;
   Urb->Context    = Context;
 
@@ -627,10 +653,14 @@ EhcCreateUrb (
   if (Data != NULL) {
     Len     = DataLen;
 
-    if (Ep->Direction == EfiUsbDataIn) {
-      MapOp = EfiPciIoOperationBusMasterWrite;
+    if (Urb->AllocateCommonBuffer) {
+      MapOp = EfiPciIoOperationBusMasterCommonBuffer;
     } else {
-      MapOp = EfiPciIoOperationBusMasterRead;
+      if (Ep->Direction == EfiUsbDataIn) {
+        MapOp = EfiPciIoOperationBusMasterWrite;
+      } else {
+        MapOp = EfiPciIoOperationBusMasterRead;
+      }
     }
 
     Status  = PciIo->Map (PciIo, MapOp, Data, &Len, &PhyAddr, &Map);
diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.h b/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.h
index 02e9af81be39..cb3599f9d361 100644
--- a/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.h
+++ b/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.h
@@ -3,7 +3,7 @@
     This file contains URB request, each request is warpped in a
     URB (Usb Request Block).
 
-Copyright (c) 2007 - 2010, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.<BR>
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD License
 which accompanies this distribution.  The full text of the license may be found at
@@ -216,6 +216,7 @@ struct _URB {
   EFI_USB_DEVICE_REQUEST          *Request;     // Control transfer only
   VOID                            *RequestPhy;  // Address of the mapped request
   VOID                            *RequestMap;
+  BOOLEAN                         AllocateCommonBuffer;
   VOID                            *Data;
   UINTN                           DataLen;
   VOID                            *DataPhy;     // Address of the mapped user data
@@ -298,20 +299,21 @@ EhcFreeUrb (
 /**
   Create a new URB and its associated QTD.
 
-  @param  Ehc        The EHCI device.
-  @param  DevAddr    The device address.
-  @param  EpAddr     Endpoint addrress & its direction.
-  @param  DevSpeed   The device speed.
-  @param  Toggle     Initial data toggle to use.
-  @param  MaxPacket  The max packet length of the endpoint.
-  @param  Hub        The transaction translator to use.
-  @param  Type       The transaction type.
-  @param  Request    The standard USB request for control transfer.
-  @param  Data       The user data to transfer.
-  @param  DataLen    The length of data buffer.
-  @param  Callback   The function to call when data is transferred.
-  @param  Context    The context to the callback.
-  @param  Interval   The interval for interrupt transfer.
+  @param  Ehc                   The EHCI device.
+  @param  DevAddr               The device address.
+  @param  EpAddr                Endpoint addrress & its direction.
+  @param  DevSpeed              The device speed.
+  @param  Toggle                Initial data toggle to use.
+  @param  MaxPacket             The max packet length of the endpoint.
+  @param  Hub                   The transaction translator to use.
+  @param  Type                  The transaction type.
+  @param  Request               The standard USB request for control transfer.
+  @param  AllocateCommonBuffer  Indicate whether need to allocate common buffer for data transfer.
+  @param  Data                  The user data to transfer, NULL if AllocateCommonBuffer is TRUE.
+  @param  DataLen               The length of data buffer.
+  @param  Callback              The function to call when data is transferred.
+  @param  Context               The context to the callback.
+  @param  Interval              The interval for interrupt transfer.
 
   @return Created URB or NULL.
 
@@ -327,6 +329,7 @@ EhcCreateUrb (
   IN EFI_USB2_HC_TRANSACTION_TRANSLATOR *Hub,
   IN UINTN                              Type,
   IN EFI_USB_DEVICE_REQUEST             *Request,
+  IN BOOLEAN                            AllocateCommonBuffer,
   IN VOID                               *Data,
   IN UINTN                              DataLen,
   IN EFI_ASYNC_USB_TRANSFER_CALLBACK    Callback,
-- 
2.7.0.windows.1



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH V2 1/4] MdeModulePkg XhciDxe: Extract new XhciInsertAsyncIntTransfer function
  2018-10-26  5:42 ` [PATCH V2 1/4] MdeModulePkg XhciDxe: Extract new XhciInsertAsyncIntTransfer function Star Zeng
@ 2018-10-26  7:33   ` Wu, Hao A
  2018-10-26 13:42     ` Zeng, Star
  0 siblings, 1 reply; 12+ messages in thread
From: Wu, Hao A @ 2018-10-26  7:33 UTC (permalink / raw)
  To: Zeng, Star, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu, Wang, Jian J, Yao, Jiewen

Hi Star,

The interface for function XhciInsertAsyncIntTransfer() does not match between
XhciSched.c and XhciSched.h.

Other than that:
Reviewed-by: Hao Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu


> -----Original Message-----
> From: Zeng, Star
> Sent: Friday, October 26, 2018 1:42 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star; Ni, Ruiyu; Wu, Hao A; Wang, Jian J; Yao, Jiewen
> Subject: [PATCH V2 1/4] MdeModulePkg XhciDxe: Extract new
> XhciInsertAsyncIntTransfer function
> 
> V2:
> Add the missing "FreePool (Data);".
> Remove the unnecessary indentation change.
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1274
> 
> Extract new XhciInsertAsyncIntTransfer function from
> XhcAsyncInterruptTransfer.
> 
> It is code preparation for following patch,
> no essential functional change.
> 
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Hao Wu <hao.a.wu@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
>  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c      | 18 +--------
>  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 65
> ++++++++++++++++++++++++++++++++
>  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h | 28 ++++++++++++++
>  3 files changed, 94 insertions(+), 17 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> index f1c60bef01c0..7f64f9c7c982 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> @@ -1346,7 +1346,6 @@ XhcAsyncInterruptTransfer (
>    EFI_STATUS              Status;
>    UINT8                   SlotId;
>    UINT8                   Index;
> -  UINT8                   *Data;
>    EFI_TPL                 OldTpl;
> 
>    //
> @@ -1413,36 +1412,21 @@ XhcAsyncInterruptTransfer (
>      goto ON_EXIT;
>    }
> 
> -  Data = AllocateZeroPool (DataLength);
> -
> -  if (Data == NULL) {
> -    DEBUG ((EFI_D_ERROR, "XhcAsyncInterruptTransfer: failed to allocate
> buffer\n"));
> -    Status = EFI_OUT_OF_RESOURCES;
> -    goto ON_EXIT;
> -  }
> -
> -  Urb = XhcCreateUrb (
> +  Urb = XhciInsertAsyncIntTransfer (
>            Xhc,
>            DeviceAddress,
>            EndPointAddress,
>            DeviceSpeed,
>            MaximumPacketLength,
> -          XHC_INT_TRANSFER_ASYNC,
> -          NULL,
> -          Data,
>            DataLength,
>            CallBackFunction,
>            Context
>            );
> -
>    if (Urb == NULL) {
> -    DEBUG ((EFI_D_ERROR, "XhcAsyncInterruptTransfer: failed to create
> URB\n"));
> -    FreePool (Data);
>      Status = EFI_OUT_OF_RESOURCES;
>      goto ON_EXIT;
>    }
> 
> -  InsertHeadList (&Xhc->AsyncIntTransfers, &Urb->UrbList);
>    //
>    // Ring the doorbell
>    //
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> index 166c44bf5e66..75959ae08363 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> @@ -1411,6 +1411,71 @@ XhciDelAllAsyncIntTransfers (
>  }
> 
>  /**
> +  Insert a single asynchronous interrupt transfer for
> +  the device and endpoint.
> +
> +  @param Xhc            The XHCI Instance
> +  @param BusAddr        The logical device address assigned by UsbBus driver
> +  @param EpAddr         Endpoint addrress
> +  @param DevSpeed       The device speed
> +  @param MaxPacket      The max packet length of the endpoint
> +  @param DataLen        The length of data buffer
> +  @param Callback       The function to call when data is transferred
> +  @param Context        The context to the callback
> +
> +  @return Created URB or NULL
> +
> +**/
> +URB *
> +XhciInsertAsyncIntTransfer (
> +  IN USB_XHCI_INSTANCE                  *Xhc,
> +  IN UINT8                              BusAddr,
> +  IN UINT8                              EpAddr,
> +  IN UINT8                              DevSpeed,
> +  IN UINTN                              MaxPacket,
> +  IN UINTN                              DataLen,
> +  IN EFI_ASYNC_USB_TRANSFER_CALLBACK    Callback,
> +  IN VOID                               *Context
> +  )
> +{
> +  VOID      *Data;
> +  URB       *Urb;
> +
> +  Data = AllocateZeroPool (DataLen);
> +  if (Data == NULL) {
> +    DEBUG ((DEBUG_ERROR, "%a: failed to allocate buffer\n",
> __FUNCTION__));
> +    return NULL;
> +  }
> +
> +  Urb = XhcCreateUrb (
> +          Xhc,
> +          BusAddr,
> +          EpAddr,
> +          DevSpeed,
> +          MaxPacket,
> +          XHC_INT_TRANSFER_ASYNC,
> +          NULL,
> +          Data,
> +          DataLen,
> +          Callback,
> +          Context
> +          );
> +  if (Urb == NULL) {
> +    DEBUG ((DEBUG_ERROR, "%a: failed to create URB\n", __FUNCTION__));
> +    FreePool (Data);
> +    return NULL;
> +  }
> +
> +  //
> +  // New asynchronous transfer must inserted to the head.
> +  // Check the comments in XhcMoniteAsyncRequests
> +  //
> +  InsertHeadList (&Xhc->AsyncIntTransfers, &Urb->UrbList);
> +
> +  return Urb;
> +}
> +
> +/**
>    Update the queue head for next round of asynchronous transfer
> 
>    @param  Xhc     The XHCI Instance.
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
> index 097408828a1f..cd1403f2842a 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
> @@ -853,6 +853,34 @@ XhciDelAllAsyncIntTransfers (
>    );
> 
>  /**
> +  Insert a single asynchronous interrupt transfer for
> +  the device and endpoint.
> +
> +  @param Xhc            The XHCI Instance
> +  @param BusAddr        The logical device address assigned by UsbBus driver
> +  @param EpAddr         Endpoint addrress
> +  @param DevSpeed       The device speed
> +  @param MaxPacket      The max packet length of the endpoint
> +  @param DataLen        The length of data buffer
> +  @param Callback       The function to call when data is transferred
> +  @param Context        The context to the callback
> +
> +  @return Created URB or NULL
> +
> +**/
> +URB *
> +XhciInsertAsyncIntTransfer (
> +  IN USB_XHCI_INSTANCE                  *Xhc,
> +  IN UINT8                              DeviceAddress,
> +  IN UINT8                              EndPointAddress,
> +  IN UINT8                              DeviceSpeed,
> +  IN UINTN                              MaximumPacketLength,
> +  IN UINTN                              DataLength,
> +  IN EFI_ASYNC_USB_TRANSFER_CALLBACK    CallBackFunction,
> +  IN VOID                               *Context
> +  );
> +
> +/**
>    Set Bios Ownership
> 
>    @param  Xhc          The XHCI Instance.
> --
> 2.7.0.windows.1



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH V2 3/4] MdeModulePkg XhciDxe: Use common buffer for AsyncInterruptTransfer
  2018-10-26  5:42 ` [PATCH V2 3/4] MdeModulePkg XhciDxe: Use common buffer for AsyncInterruptTransfer Star Zeng
@ 2018-10-26  7:38   ` Wu, Hao A
  2018-10-26  8:14     ` Wu, Hao A
  0 siblings, 1 reply; 12+ messages in thread
From: Wu, Hao A @ 2018-10-26  7:38 UTC (permalink / raw)
  To: Zeng, Star, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu, Yao, Jiewen, Zeng, Star

Reviewed-by: Hao Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Star Zeng
> Sent: Friday, October 26, 2018 1:42 PM
> To: edk2-devel@lists.01.org
> Cc: Ni, Ruiyu; Wu, Hao A; Yao, Jiewen; Zeng, Star
> Subject: [edk2] [PATCH V2 3/4] MdeModulePkg XhciDxe: Use common buffer
> for AsyncInterruptTransfer
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1274
> 
> In current code, XhcMonitorAsyncRequests (timer handler) will do
> unmap and map operations for AsyncIntTransfers to "Flush data from
> PCI controller specific address to mapped system memory address".
> XhcMonitorAsyncRequests
>   XhcFlushAsyncIntMap
>     PciIo->Unmap
>       IoMmu->SetAttribute
>     PciIo->Map
>       IoMmu->SetAttribute
> 
> This may impact the boot performance.
> 
> Since the data buffer for XhcMonitorAsyncRequests is internal
> buffer, we can allocate common buffer by PciIo->AllocateBuffer
> and map the buffer with EfiPciIoOperationBusMasterCommonBuffer,
> then the unmap and map operations can be removed.
> 
> ///
> /// Provides both read and write access to system memory by
> /// both the processor and a bus master. The buffer is coherent
> /// from both the processor's and the bus master's point of view.
> ///
> EfiPciIoOperationBusMasterCommonBuffer,
> 
> Test done:
> USB KB works normally.
> USB disk read/write works normally.
> 
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Hao Wu <hao.a.wu@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
> ---
>  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c      |   1 +
>  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 141 +++++++++++-------------
> -------
>  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h |  27 +++---
>  3 files changed, 67 insertions(+), 102 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> index 7f64f9c7c982..64855a4c158c 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> @@ -769,6 +769,7 @@ XhcTransfer (
>            MaximumPacketLength,
>            Type,
>            Request,
> +          FALSE,
>            Data,
>            *DataLength,
>            NULL,
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> index 75959ae08363..d03a6681ce0d 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> @@ -118,17 +118,18 @@ ON_EXIT:
>  /**
>    Create a new URB for a new transaction.
> 
> -  @param  Xhc       The XHCI Instance
> -  @param  BusAddr   The logical device address assigned by UsbBus driver
> -  @param  EpAddr    Endpoint addrress
> -  @param  DevSpeed  The device speed
> -  @param  MaxPacket The max packet length of the endpoint
> -  @param  Type      The transaction type
> -  @param  Request   The standard USB request for control transfer
> -  @param  Data      The user data to transfer
> -  @param  DataLen   The length of data buffer
> -  @param  Callback  The function to call when data is transferred
> -  @param  Context   The context to the callback
> +  @param  Xhc                   The XHCI Instance
> +  @param  BusAddr               The logical device address assigned by UsbBus
> driver
> +  @param  EpAddr                Endpoint addrress
> +  @param  DevSpeed              The device speed
> +  @param  MaxPacket             The max packet length of the endpoint
> +  @param  Type                  The transaction type
> +  @param  Request               The standard USB request for control transfer
> +  @param  AllocateCommonBuffer  Indicate whether need to allocate
> common buffer for data transfer
> +  @param  Data                  The user data to transfer, NULL if
> AllocateCommonBuffer is TRUE
> +  @param  DataLen               The length of data buffer
> +  @param  Callback              The function to call when data is transferred
> +  @param  Context               The context to the callback
> 
>    @return Created URB or NULL
> 
> @@ -142,6 +143,7 @@ XhcCreateUrb (
>    IN UINTN                              MaxPacket,
>    IN UINTN                              Type,
>    IN EFI_USB_DEVICE_REQUEST             *Request,
> +  IN BOOLEAN                            AllocateCommonBuffer,
>    IN VOID                               *Data,
>    IN UINTN                              DataLen,
>    IN EFI_ASYNC_USB_TRANSFER_CALLBACK    Callback,
> @@ -169,8 +171,24 @@ XhcCreateUrb (
>    Ep->Type      = Type;
> 
>    Urb->Request  = Request;
> +  if (AllocateCommonBuffer) {
> +    ASSERT (Data == NULL);
> +    Status = Xhc->PciIo->AllocateBuffer (
> +                           Xhc->PciIo,
> +                           AllocateAnyPages,
> +                           EfiBootServicesData,
> +                           EFI_SIZE_TO_PAGES (DataLen),
> +                           &Data,
> +                           0
> +                           );
> +    if (EFI_ERROR (Status) || (Data == NULL)) {
> +      FreePool (Urb);
> +      return NULL;
> +    }
> +  }
>    Urb->Data     = Data;
>    Urb->DataLen  = DataLen;
> +  Urb->AllocateCommonBuffer = AllocateCommonBuffer;
>    Urb->Callback = Callback;
>    Urb->Context  = Context;
> 
> @@ -178,6 +196,11 @@ XhcCreateUrb (
>    ASSERT_EFI_ERROR (Status);
>    if (EFI_ERROR (Status)) {
>      DEBUG ((EFI_D_ERROR, "XhcCreateUrb: XhcCreateTransferTrb Failed,
> Status = %r\n", Status));
> +    Xhc->PciIo->FreeBuffer (
> +                  Xhc->PciIo,
> +                  EFI_SIZE_TO_PAGES (Urb->DataLen),
> +                  Urb->Data
> +                  );
>      FreePool (Urb);
>      Urb = NULL;
>    }
> @@ -206,6 +229,14 @@ XhcFreeUrb (
>      Xhc->PciIo->Unmap (Xhc->PciIo, Urb->DataMap);
>    }
> 
> +  if (Urb->AllocateCommonBuffer) {
> +    Xhc->PciIo->FreeBuffer (
> +                  Xhc->PciIo,
> +                  EFI_SIZE_TO_PAGES (Urb->DataLen),
> +                  Urb->Data
> +                  );
> +  }
> +
>    FreePool (Urb);
>  }
> 
> @@ -264,10 +295,14 @@ XhcCreateTransferTrb (
>    // No need to remap.
>    //
>    if ((Urb->Data != NULL) && (Urb->DataMap == NULL)) {
> -    if (((UINT8) (Urb->Ep.Direction)) == EfiUsbDataIn) {
> -      MapOp = EfiPciIoOperationBusMasterWrite;
> +    if (Urb->AllocateCommonBuffer) {
> +      MapOp = EfiPciIoOperationBusMasterCommonBuffer;
>      } else {
> -      MapOp = EfiPciIoOperationBusMasterRead;
> +      if (((UINT8) (Urb->Ep.Direction)) == EfiUsbDataIn) {
> +        MapOp = EfiPciIoOperationBusMasterWrite;
> +      } else {
> +        MapOp = EfiPciIoOperationBusMasterRead;
> +      }
>      }
> 
>      Len = Urb->DataLen;
> @@ -1367,7 +1402,6 @@ XhciDelAsyncIntTransfer (
>        }
> 
>        RemoveEntryList (&Urb->UrbList);
> -      FreePool (Urb->Data);
>        XhcFreeUrb (Xhc, Urb);
>        return EFI_SUCCESS;
>      }
> @@ -1405,7 +1439,6 @@ XhciDelAllAsyncIntTransfers (
>      }
> 
>      RemoveEntryList (&Urb->UrbList);
> -    FreePool (Urb->Data);
>      XhcFreeUrb (Xhc, Urb);
>    }
>  }
> @@ -1438,15 +1471,8 @@ XhciInsertAsyncIntTransfer (
>    IN VOID                               *Context
>    )
>  {
> -  VOID      *Data;
>    URB       *Urb;
> 
> -  Data = AllocateZeroPool (DataLen);
> -  if (Data == NULL) {
> -    DEBUG ((DEBUG_ERROR, "%a: failed to allocate buffer\n",
> __FUNCTION__));
> -    return NULL;
> -  }
> -
>    Urb = XhcCreateUrb (
>            Xhc,
>            BusAddr,
> @@ -1455,14 +1481,14 @@ XhciInsertAsyncIntTransfer (
>            MaxPacket,
>            XHC_INT_TRANSFER_ASYNC,
>            NULL,
> -          Data,
> +          TRUE,
> +          NULL,
>            DataLen,
>            Callback,
>            Context
>            );
>    if (Urb == NULL) {
>      DEBUG ((DEBUG_ERROR, "%a: failed to create URB\n", __FUNCTION__));
> -    FreePool (Data);
>      return NULL;
>    }
> 
> @@ -1503,61 +1529,6 @@ XhcUpdateAsyncRequest (
>  }
> 
>  /**
> -  Flush data from PCI controller specific address to mapped system
> -  memory address.
> -
> -  @param  Xhc                The XHCI device.
> -  @param  Urb                The URB to unmap.
> -
> -  @retval EFI_SUCCESS        Success to flush data to mapped system memory.
> -  @retval EFI_DEVICE_ERROR   Fail to flush data to mapped system memory.
> -
> -**/
> -EFI_STATUS
> -XhcFlushAsyncIntMap (
> -  IN  USB_XHCI_INSTANCE   *Xhc,
> -  IN  URB                 *Urb
> -  )
> -{
> -  EFI_STATUS                    Status;
> -  EFI_PHYSICAL_ADDRESS          PhyAddr;
> -  EFI_PCI_IO_PROTOCOL_OPERATION MapOp;
> -  EFI_PCI_IO_PROTOCOL           *PciIo;
> -  UINTN                         Len;
> -  VOID                          *Map;
> -
> -  PciIo = Xhc->PciIo;
> -  Len   = Urb->DataLen;
> -
> -  if (Urb->Ep.Direction == EfiUsbDataIn) {
> -    MapOp = EfiPciIoOperationBusMasterWrite;
> -  } else {
> -    MapOp = EfiPciIoOperationBusMasterRead;
> -  }
> -
> -  if (Urb->DataMap != NULL) {
> -    Status = PciIo->Unmap (PciIo, Urb->DataMap);
> -    if (EFI_ERROR (Status)) {
> -      goto ON_ERROR;
> -    }
> -  }
> -
> -  Urb->DataMap = NULL;
> -
> -  Status = PciIo->Map (PciIo, MapOp, Urb->Data, &Len, &PhyAddr, &Map);
> -  if (EFI_ERROR (Status) || (Len != Urb->DataLen)) {
> -    goto ON_ERROR;
> -  }
> -
> -  Urb->DataPhy  = (VOID *) ((UINTN) PhyAddr);
> -  Urb->DataMap  = Map;
> -  return EFI_SUCCESS;
> -
> -ON_ERROR:
> -  return EFI_DEVICE_ERROR;
> -}
> -
> -/**
>    Interrupt transfer periodic check handler.
> 
>    @param  Event                 Interrupt event.
> @@ -1577,7 +1548,6 @@ XhcMonitorAsyncRequests (
>    UINT8                   *ProcBuf;
>    URB                     *Urb;
>    UINT8                   SlotId;
> -  EFI_STATUS              Status;
>    EFI_TPL                 OldTpl;
> 
>    OldTpl = gBS->RaiseTPL (XHC_TPL);
> @@ -1606,15 +1576,6 @@ XhcMonitorAsyncRequests (
>      }
> 
>      //
> -    // Flush any PCI posted write transactions from a PCI host
> -    // bridge to system memory.
> -    //
> -    Status = XhcFlushAsyncIntMap (Xhc, Urb);
> -    if (EFI_ERROR (Status)) {
> -      DEBUG ((EFI_D_ERROR, "XhcMonitorAsyncRequests: Fail to Flush
> AsyncInt Mapped Memeory\n"));
> -    }
> -
> -    //
>      // Allocate a buffer then copy the transferred data for user.
>      // If failed to allocate the buffer, update the URB for next
>      // round of transfer. Ignore the data of this round.
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
> index cd1403f2842a..2b5b95b7fb60 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
> @@ -172,6 +172,7 @@ typedef struct _URB {
>    //
>    USB_ENDPOINT                    Ep;
>    EFI_USB_DEVICE_REQUEST          *Request;
> +  BOOLEAN                         AllocateCommonBuffer;
>    VOID                            *Data;
>    UINTN                           DataLen;
>    VOID                            *DataPhy;
> @@ -1432,17 +1433,18 @@ XhcSetTrDequeuePointer (
>  /**
>    Create a new URB for a new transaction.
> 
> -  @param  Xhc       The XHCI Instance
> -  @param  DevAddr   The device address
> -  @param  EpAddr    Endpoint addrress
> -  @param  DevSpeed  The device speed
> -  @param  MaxPacket The max packet length of the endpoint
> -  @param  Type      The transaction type
> -  @param  Request   The standard USB request for control transfer
> -  @param  Data      The user data to transfer
> -  @param  DataLen   The length of data buffer
> -  @param  Callback  The function to call when data is transferred
> -  @param  Context   The context to the callback
> +  @param  Xhc                   The XHCI Instance
> +  @param  BusAddr               The logical device address assigned by UsbBus
> driver
> +  @param  EpAddr                Endpoint addrress
> +  @param  DevSpeed              The device speed
> +  @param  MaxPacket             The max packet length of the endpoint
> +  @param  Type                  The transaction type
> +  @param  Request               The standard USB request for control transfer
> +  @param  AllocateCommonBuffer  Indicate whether need to allocate
> common buffer for data transfer
> +  @param  Data                  The user data to transfer, NULL if
> AllocateCommonBuffer is TRUE
> +  @param  DataLen               The length of data buffer
> +  @param  Callback              The function to call when data is transferred
> +  @param  Context               The context to the callback
> 
>    @return Created URB or NULL
> 
> @@ -1450,12 +1452,13 @@ XhcSetTrDequeuePointer (
>  URB*
>  XhcCreateUrb (
>    IN USB_XHCI_INSTANCE                  *Xhc,
> -  IN UINT8                              DevAddr,
> +  IN UINT8                              BusAddr,
>    IN UINT8                              EpAddr,
>    IN UINT8                              DevSpeed,
>    IN UINTN                              MaxPacket,
>    IN UINTN                              Type,
>    IN EFI_USB_DEVICE_REQUEST             *Request,
> +  IN BOOLEAN                            AllocateCommonBuffer,
>    IN VOID                               *Data,
>    IN UINTN                              DataLen,
>    IN EFI_ASYNC_USB_TRANSFER_CALLBACK    Callback,
> --
> 2.7.0.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH V2 2/4] MdeModulePkg EhciDxe: Extract new EhciInsertAsyncIntTransfer function
  2018-10-26  5:42 ` [PATCH V2 2/4] MdeModulePkg EhciDxe: Extract new EhciInsertAsyncIntTransfer function Star Zeng
@ 2018-10-26  7:54   ` Wu, Hao A
  0 siblings, 0 replies; 12+ messages in thread
From: Wu, Hao A @ 2018-10-26  7:54 UTC (permalink / raw)
  To: Zeng, Star, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu, Yao, Jiewen, Zeng, Star

Reviewed-by: Hao Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Star Zeng
> Sent: Friday, October 26, 2018 1:42 PM
> To: edk2-devel@lists.01.org
> Cc: Ni, Ruiyu; Wu, Hao A; Yao, Jiewen; Zeng, Star
> Subject: [edk2] [PATCH V2 2/4] MdeModulePkg EhciDxe: Extract new
> EhciInsertAsyncIntTransfer function
> 
> V2:
> Add the missing "gBS->FreePool (Data);".
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1274
> 
> Extract new EhciInsertAsyncIntTransfer function from
> EhcAsyncInterruptTransfer.
> 
> It is code preparation for following patch,
> no essential functional change.
> 
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Hao Wu <hao.a.wu@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
>  MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c      | 25 +----------
>  MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c | 77
> ++++++++++++++++++++++++++++++++
>  MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.h | 36 ++++++++++++++-
>  3 files changed, 113 insertions(+), 25 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
> b/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
> index 50b5598df4fb..5569f4f9618b 100644
> --- a/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
> +++ b/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
> @@ -997,7 +997,6 @@ EhcAsyncInterruptTransfer (
>    URB                     *Urb;
>    EFI_TPL                 OldTpl;
>    EFI_STATUS              Status;
> -  UINT8                   *Data;
> 
>    //
>    // Validate parameters
> @@ -1046,16 +1045,7 @@ EhcAsyncInterruptTransfer (
> 
>    EhcAckAllInterrupt (Ehc);
> 
> -  Data = AllocatePool (DataLength);
> -
> -  if (Data == NULL) {
> -    DEBUG ((EFI_D_ERROR, "EhcAsyncInterruptTransfer: failed to allocate
> buffer\n"));
> -
> -    Status = EFI_OUT_OF_RESOURCES;
> -    goto ON_EXIT;
> -  }
> -
> -  Urb = EhcCreateUrb (
> +  Urb = EhciInsertAsyncIntTransfer (
>            Ehc,
>            DeviceAddress,
>            EndPointAddress,
> @@ -1063,9 +1053,6 @@ EhcAsyncInterruptTransfer (
>            *DataToggle,
>            MaximumPacketLength,
>            Translator,
> -          EHC_INT_TRANSFER_ASYNC,
> -          NULL,
> -          Data,
>            DataLength,
>            CallBackFunction,
>            Context,
> @@ -1073,20 +1060,10 @@ EhcAsyncInterruptTransfer (
>            );
> 
>    if (Urb == NULL) {
> -    DEBUG ((EFI_D_ERROR, "EhcAsyncInterruptTransfer: failed to create
> URB\n"));
> -
> -    gBS->FreePool (Data);
>      Status = EFI_OUT_OF_RESOURCES;
>      goto ON_EXIT;
>    }
> 
> -  //
> -  // New asynchronous transfer must inserted to the head.
> -  // Check the comments in EhcMoniteAsyncRequests
> -  //
> -  EhcLinkQhToPeriod (Ehc, Urb->Qh);
> -  InsertHeadList (&Ehc->AsyncIntTransfers, &Urb->UrbList);
> -
>  ON_EXIT:
>    Ehc->PciIo->Flush (Ehc->PciIo);
>    gBS->RestoreTPL (OldTpl);
> diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c
> b/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c
> index 168280be81d7..2d202d439d1c 100644
> --- a/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c
> +++ b/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c
> @@ -814,6 +814,83 @@ EhciDelAllAsyncIntTransfers (
>    }
>  }
> 
> +/**
> +  Insert a single asynchronous interrupt transfer for
> +  the device and endpoint.
> +
> +  @param  Ehc               The EHCI device.
> +  @param  DevAddr           The device address.
> +  @param  EpAddr            Endpoint addrress & its direction.
> +  @param  DevSpeed          The device speed.
> +  @param  Toggle            Initial data toggle to use.
> +  @param  MaxPacket         The max packet length of the endpoint.
> +  @param  Hub               The transaction translator to use.
> +  @param  Data              The user data to transfer.
> +  @param  DataLen           The length of data buffer.
> +  @param  Callback          The function to call when data is transferred.
> +  @param  Context           The context to the callback.
> +  @param  Interval          The interval for interrupt transfer.
> +
> +  @return Created URB or NULL.
> +
> +**/
> +URB *
> +EhciInsertAsyncIntTransfer (
> +  IN USB2_HC_DEV                        *Ehc,
> +  IN UINT8                              DevAddr,
> +  IN UINT8                              EpAddr,
> +  IN UINT8                              DevSpeed,
> +  IN UINT8                              Toggle,
> +  IN UINTN                              MaxPacket,
> +  IN EFI_USB2_HC_TRANSACTION_TRANSLATOR *Hub,
> +  IN UINTN                              DataLen,
> +  IN EFI_ASYNC_USB_TRANSFER_CALLBACK    Callback,
> +  IN VOID                               *Context,
> +  IN UINTN                              Interval
> +  )
> +{
> +  VOID      *Data;
> +  URB       *Urb;
> +
> +  Data = AllocatePool (DataLen);
> +
> +  if (Data == NULL) {
> +    DEBUG ((DEBUG_ERROR, "%a: failed to allocate buffer\n",
> __FUNCTION__));
> +    return NULL;
> +  }
> +
> +  Urb = EhcCreateUrb (
> +          Ehc,
> +          DevAddr,
> +          EpAddr,
> +          DevSpeed,
> +          Toggle,
> +          MaxPacket,
> +          Hub,
> +          EHC_INT_TRANSFER_ASYNC,
> +          NULL,
> +          Data,
> +          DataLen,
> +          Callback,
> +          Context,
> +          Interval
> +          );
> +
> +  if (Urb == NULL) {
> +    DEBUG ((DEBUG_ERROR, "%a: failed to create URB\n", __FUNCTION__));
> +    gBS->FreePool (Data);
> +    return NULL;
> +  }
> +
> +  //
> +  // New asynchronous transfer must inserted to the head.
> +  // Check the comments in EhcMoniteAsyncRequests
> +  //
> +  EhcLinkQhToPeriod (Ehc, Urb->Qh);
> +  InsertHeadList (&Ehc->AsyncIntTransfers, &Urb->UrbList);
> +
> +  return Urb;
> +}
> 
>  /**
>    Flush data from PCI controller specific address to mapped system
> diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.h
> b/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.h
> index c03bd619d7e8..b852e6327f37 100644
> --- a/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.h
> +++ b/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.h
> @@ -2,7 +2,7 @@
> 
>    This file contains the definination for host controller schedule routines.
> 
> -Copyright (c) 2007 - 2009, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.<BR>
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD
> License
>  which accompanies this distribution.  The full text of the license may be
> found at
> @@ -162,6 +162,40 @@ EhciDelAllAsyncIntTransfers (
>    IN USB2_HC_DEV          *Ehc
>    );
> 
> +/**
> +  Insert a single asynchronous interrupt transfer for
> +  the device and endpoint.
> +
> +  @param  Ehc               The EHCI device.
> +  @param  DevAddr           The device address.
> +  @param  EpAddr            Endpoint addrress & its direction.
> +  @param  DevSpeed          The device speed.
> +  @param  Toggle            Initial data toggle to use.
> +  @param  MaxPacket         The max packet length of the endpoint.
> +  @param  Hub               The transaction translator to use.
> +  @param  Data              The user data to transfer.
> +  @param  DataLen           The length of data buffer.
> +  @param  Callback          The function to call when data is transferred.
> +  @param  Context           The context to the callback.
> +  @param  Interval          The interval for interrupt transfer.
> +
> +  @return Created URB or NULL.
> +
> +**/
> +URB *
> +EhciInsertAsyncIntTransfer (
> +  IN USB2_HC_DEV                        *Ehc,
> +  IN UINT8                              BusAddr,
> +  IN UINT8                              EpAddr,
> +  IN UINT8                              DevSpeed,
> +  IN UINT8                              Toggle,
> +  IN UINTN                              MaxPacket,
> +  IN EFI_USB2_HC_TRANSACTION_TRANSLATOR *Hub,
> +  IN UINTN                              DataLen,
> +  IN EFI_ASYNC_USB_TRANSFER_CALLBACK    Callback,
> +  IN VOID                               *Context,
> +  IN UINTN                              Interval
> +  );
> 
>  /**
>    Interrupt transfer periodic check handler.
> --
> 2.7.0.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH V2 4/4] MdeModulePkg EhciDxe: Use common buffer for AsyncInterruptTransfer
  2018-10-26  5:42 ` [PATCH V2 4/4] MdeModulePkg EhciDxe: " Star Zeng
@ 2018-10-26  8:12   ` Wu, Hao A
  0 siblings, 0 replies; 12+ messages in thread
From: Wu, Hao A @ 2018-10-26  8:12 UTC (permalink / raw)
  To: Zeng, Star, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu, Yao, Jiewen, Zeng, Star

Reviewed-by: Hao Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Star Zeng
> Sent: Friday, October 26, 2018 1:42 PM
> To: edk2-devel@lists.01.org
> Cc: Ni, Ruiyu; Wu, Hao A; Yao, Jiewen; Zeng, Star
> Subject: [edk2] [PATCH V2 4/4] MdeModulePkg EhciDxe: Use common buffer
> for AsyncInterruptTransfer
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1274
> 
> In current code, EhcMonitorAsyncRequests (timer handler) will do
> unmap and map operations for AsyncIntTransfers to "Flush data from
> PCI controller specific address to mapped system memory address".
> EhcMonitorAsyncRequests
>   EhcFlushAsyncIntMap
>     PciIo->Unmap
>       IoMmu->SetAttribute
>     PciIo->Map
>       IoMmu->SetAttribute
> 
> This may impact the boot performance.
> 
> Since the data buffer for EhcMonitorAsyncRequests is internal
> buffer, we can allocate common buffer by PciIo->AllocateBuffer
> and map the buffer with EfiPciIoOperationBusMasterCommonBuffer,
> then the unmap and map operations can be removed.
> 
> ///
> /// Provides both read and write access to system memory by
> /// both the processor and a bus master. The buffer is coherent
> /// from both the processor's and the bus master's point of view.
> ///
> EfiPciIoOperationBusMasterCommonBuffer,
> 
> Test done:
> USB KB works normally.
> USB disk read/write works normally.
> 
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Hao Wu <hao.a.wu@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
> ---
>  MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c      |  3 ++
>  MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c | 78 +-----------------------------
> --
>  MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.c   | 38 ++++++++++++++--
>  MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.h   | 33 ++++++++------
>  4 files changed, 57 insertions(+), 95 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
> b/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
> index 5569f4f9618b..764eeda58ba1 100644
> --- a/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
> +++ b/MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c
> @@ -763,6 +763,7 @@ EhcControlTransfer (
>            Translator,
>            EHC_CTRL_TRANSFER,
>            Request,
> +          FALSE,
>            Data,
>            *DataLength,
>            NULL,
> @@ -906,6 +907,7 @@ EhcBulkTransfer (
>            Translator,
>            EHC_BULK_TRANSFER,
>            NULL,
> +          FALSE,
>            Data[0],
>            *DataLength,
>            NULL,
> @@ -1163,6 +1165,7 @@ EhcSyncInterruptTransfer (
>            Translator,
>            EHC_INT_TRANSFER_SYNC,
>            NULL,
> +          FALSE,
>            Data,
>            *DataLength,
>            NULL,
> diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c
> b/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c
> index 2d202d439d1c..f1edcf20e342 100644
> --- a/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c
> +++ b/MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c
> @@ -778,7 +778,6 @@ EhciDelAsyncIntTransfer (
>        EhcUnlinkQhFromPeriod (Ehc, Urb->Qh);
>        RemoveEntryList (&Urb->UrbList);
> 
> -      gBS->FreePool (Urb->Data);
>        EhcFreeUrb (Ehc, Urb);
>        return EFI_SUCCESS;
>      }
> @@ -809,7 +808,6 @@ EhciDelAllAsyncIntTransfers (
>      EhcUnlinkQhFromPeriod (Ehc, Urb->Qh);
>      RemoveEntryList (&Urb->UrbList);
> 
> -    gBS->FreePool (Urb->Data);
>      EhcFreeUrb (Ehc, Urb);
>    }
>  }
> @@ -849,16 +847,8 @@ EhciInsertAsyncIntTransfer (
>    IN UINTN                              Interval
>    )
>  {
> -  VOID      *Data;
>    URB       *Urb;
> 
> -  Data = AllocatePool (DataLen);
> -
> -  if (Data == NULL) {
> -    DEBUG ((DEBUG_ERROR, "%a: failed to allocate buffer\n",
> __FUNCTION__));
> -    return NULL;
> -  }
> -
>    Urb = EhcCreateUrb (
>            Ehc,
>            DevAddr,
> @@ -869,7 +859,8 @@ EhciInsertAsyncIntTransfer (
>            Hub,
>            EHC_INT_TRANSFER_ASYNC,
>            NULL,
> -          Data,
> +          TRUE,
> +          NULL,
>            DataLen,
>            Callback,
>            Context,
> @@ -878,7 +869,6 @@ EhciInsertAsyncIntTransfer (
> 
>    if (Urb == NULL) {
>      DEBUG ((DEBUG_ERROR, "%a: failed to create URB\n", __FUNCTION__));
> -    gBS->FreePool (Data);
>      return NULL;
>    }
> 
> @@ -893,60 +883,6 @@ EhciInsertAsyncIntTransfer (
>  }
> 
>  /**
> -  Flush data from PCI controller specific address to mapped system
> -  memory address.
> -
> -  @param  Ehc                The EHCI device.
> -  @param  Urb                The URB to unmap.
> -
> -  @retval EFI_SUCCESS        Success to flush data to mapped system memory.
> -  @retval EFI_DEVICE_ERROR   Fail to flush data to mapped system memory.
> -
> -**/
> -EFI_STATUS
> -EhcFlushAsyncIntMap (
> -  IN  USB2_HC_DEV         *Ehc,
> -  IN  URB                 *Urb
> -  )
> -{
> -  EFI_STATUS                    Status;
> -  EFI_PHYSICAL_ADDRESS          PhyAddr;
> -  EFI_PCI_IO_PROTOCOL_OPERATION MapOp;
> -  EFI_PCI_IO_PROTOCOL           *PciIo;
> -  UINTN                         Len;
> -  VOID                          *Map;
> -
> -  PciIo = Ehc->PciIo;
> -  Len   = Urb->DataLen;
> -
> -  if (Urb->Ep.Direction == EfiUsbDataIn) {
> -    MapOp = EfiPciIoOperationBusMasterWrite;
> -  } else {
> -    MapOp = EfiPciIoOperationBusMasterRead;
> -  }
> -
> -  Status = PciIo->Unmap (PciIo, Urb->DataMap);
> -  if (EFI_ERROR (Status)) {
> -    goto ON_ERROR;
> -  }
> -
> -  Urb->DataMap = NULL;
> -
> -  Status = PciIo->Map (PciIo, MapOp, Urb->Data, &Len, &PhyAddr, &Map);
> -  if (EFI_ERROR (Status) || (Len != Urb->DataLen)) {
> -    goto ON_ERROR;
> -  }
> -
> -  Urb->DataPhy  = (VOID *) ((UINTN) PhyAddr);
> -  Urb->DataMap  = Map;
> -  return EFI_SUCCESS;
> -
> -ON_ERROR:
> -  return EFI_DEVICE_ERROR;
> -}
> -
> -
> -/**
>    Update the queue head for next round of asynchronous transfer.
> 
>    @param  Ehc                   The EHCI device.
> @@ -1051,7 +987,6 @@ EhcMonitorAsyncRequests (
>    BOOLEAN                 Finished;
>    UINT8                   *ProcBuf;
>    URB                     *Urb;
> -  EFI_STATUS              Status;
> 
>    OldTpl  = gBS->RaiseTPL (EHC_TPL);
>    Ehc     = (USB2_HC_DEV *) Context;
> @@ -1070,15 +1005,6 @@ EhcMonitorAsyncRequests (
>      }
> 
>      //
> -    // Flush any PCI posted write transactions from a PCI host
> -    // bridge to system memory.
> -    //
> -    Status = EhcFlushAsyncIntMap (Ehc, Urb);
> -    if (EFI_ERROR (Status)) {
> -      DEBUG ((EFI_D_ERROR, "EhcMonitorAsyncRequests: Fail to Flush
> AsyncInt Mapped Memeory\n"));
> -    }
> -
> -    //
>      // Allocate a buffer then copy the transferred data for user.
>      // If failed to allocate the buffer, update the URB for next
>      // round of transfer. Ignore the data of this round.
> diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.c
> b/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.c
> index 6afb327df165..09c12776ecdb 100644
> --- a/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.c
> +++ b/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.c
> @@ -339,6 +339,14 @@ EhcFreeUrb (
>      PciIo->Unmap (PciIo, Urb->DataMap);
>    }
> 
> +  if (Urb->AllocateCommonBuffer) {
> +    PciIo->FreeBuffer (
> +             PciIo,
> +             EFI_SIZE_TO_PAGES (Urb->DataLen),
> +             Urb->Data
> +             );
> +  }
> +
>    if (Urb->Qh != NULL) {
>      //
>      // Ensure that this queue head has been unlinked from the
> @@ -529,7 +537,8 @@ ON_ERROR:
>    @param  Hub                   The transaction translator to use.
>    @param  Type                  The transaction type.
>    @param  Request               The standard USB request for control transfer.
> -  @param  Data                  The user data to transfer.
> +  @param  AllocateCommonBuffer  Indicate whether need to allocate
> common buffer for data transfer.
> +  @param  Data                  The user data to transfer, NULL if
> AllocateCommonBuffer is TRUE.
>    @param  DataLen               The length of data buffer.
>    @param  Callback              The function to call when data is transferred.
>    @param  Context               The context to the callback.
> @@ -549,6 +558,7 @@ EhcCreateUrb (
>    IN EFI_USB2_HC_TRANSACTION_TRANSLATOR *Hub,
>    IN UINTN                              Type,
>    IN EFI_USB_DEVICE_REQUEST             *Request,
> +  IN BOOLEAN                            AllocateCommonBuffer,
>    IN VOID                               *Data,
>    IN UINTN                              DataLen,
>    IN EFI_ASYNC_USB_TRANSFER_CALLBACK    Callback,
> @@ -596,8 +606,24 @@ EhcCreateUrb (
>    Ep->PollRate    = EhcConvertPollRate (Interval);
> 
>    Urb->Request    = Request;
> +  if (AllocateCommonBuffer) {
> +    ASSERT (Data == NULL);
> +    Status = Ehc->PciIo->AllocateBuffer (
> +                           Ehc->PciIo,
> +                           AllocateAnyPages,
> +                           EfiBootServicesData,
> +                           EFI_SIZE_TO_PAGES (DataLen),
> +                           &Data,
> +                           0
> +                           );
> +    if (EFI_ERROR (Status) || (Data == NULL)) {
> +      FreePool (Urb);
> +      return NULL;
> +    }
> +  }
>    Urb->Data       = Data;
>    Urb->DataLen    = DataLen;
> +  Urb->AllocateCommonBuffer = AllocateCommonBuffer;
>    Urb->Callback   = Callback;
>    Urb->Context    = Context;
> 
> @@ -627,10 +653,14 @@ EhcCreateUrb (
>    if (Data != NULL) {
>      Len     = DataLen;
> 
> -    if (Ep->Direction == EfiUsbDataIn) {
> -      MapOp = EfiPciIoOperationBusMasterWrite;
> +    if (Urb->AllocateCommonBuffer) {
> +      MapOp = EfiPciIoOperationBusMasterCommonBuffer;
>      } else {
> -      MapOp = EfiPciIoOperationBusMasterRead;
> +      if (Ep->Direction == EfiUsbDataIn) {
> +        MapOp = EfiPciIoOperationBusMasterWrite;
> +      } else {
> +        MapOp = EfiPciIoOperationBusMasterRead;
> +      }
>      }
> 
>      Status  = PciIo->Map (PciIo, MapOp, Data, &Len, &PhyAddr, &Map);
> diff --git a/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.h
> b/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.h
> index 02e9af81be39..cb3599f9d361 100644
> --- a/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.h
> +++ b/MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.h
> @@ -3,7 +3,7 @@
>      This file contains URB request, each request is warpped in a
>      URB (Usb Request Block).
> 
> -Copyright (c) 2007 - 2010, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.<BR>
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD
> License
>  which accompanies this distribution.  The full text of the license may be
> found at
> @@ -216,6 +216,7 @@ struct _URB {
>    EFI_USB_DEVICE_REQUEST          *Request;     // Control transfer only
>    VOID                            *RequestPhy;  // Address of the mapped request
>    VOID                            *RequestMap;
> +  BOOLEAN                         AllocateCommonBuffer;
>    VOID                            *Data;
>    UINTN                           DataLen;
>    VOID                            *DataPhy;     // Address of the mapped user data
> @@ -298,20 +299,21 @@ EhcFreeUrb (
>  /**
>    Create a new URB and its associated QTD.
> 
> -  @param  Ehc        The EHCI device.
> -  @param  DevAddr    The device address.
> -  @param  EpAddr     Endpoint addrress & its direction.
> -  @param  DevSpeed   The device speed.
> -  @param  Toggle     Initial data toggle to use.
> -  @param  MaxPacket  The max packet length of the endpoint.
> -  @param  Hub        The transaction translator to use.
> -  @param  Type       The transaction type.
> -  @param  Request    The standard USB request for control transfer.
> -  @param  Data       The user data to transfer.
> -  @param  DataLen    The length of data buffer.
> -  @param  Callback   The function to call when data is transferred.
> -  @param  Context    The context to the callback.
> -  @param  Interval   The interval for interrupt transfer.
> +  @param  Ehc                   The EHCI device.
> +  @param  DevAddr               The device address.
> +  @param  EpAddr                Endpoint addrress & its direction.
> +  @param  DevSpeed              The device speed.
> +  @param  Toggle                Initial data toggle to use.
> +  @param  MaxPacket             The max packet length of the endpoint.
> +  @param  Hub                   The transaction translator to use.
> +  @param  Type                  The transaction type.
> +  @param  Request               The standard USB request for control transfer.
> +  @param  AllocateCommonBuffer  Indicate whether need to allocate
> common buffer for data transfer.
> +  @param  Data                  The user data to transfer, NULL if
> AllocateCommonBuffer is TRUE.
> +  @param  DataLen               The length of data buffer.
> +  @param  Callback              The function to call when data is transferred.
> +  @param  Context               The context to the callback.
> +  @param  Interval              The interval for interrupt transfer.
> 
>    @return Created URB or NULL.
> 
> @@ -327,6 +329,7 @@ EhcCreateUrb (
>    IN EFI_USB2_HC_TRANSACTION_TRANSLATOR *Hub,
>    IN UINTN                              Type,
>    IN EFI_USB_DEVICE_REQUEST             *Request,
> +  IN BOOLEAN                            AllocateCommonBuffer,
>    IN VOID                               *Data,
>    IN UINTN                              DataLen,
>    IN EFI_ASYNC_USB_TRANSFER_CALLBACK    Callback,
> --
> 2.7.0.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH V2 3/4] MdeModulePkg XhciDxe: Use common buffer for AsyncInterruptTransfer
  2018-10-26  7:38   ` Wu, Hao A
@ 2018-10-26  8:14     ` Wu, Hao A
  2018-10-26 13:43       ` Zeng, Star
  0 siblings, 1 reply; 12+ messages in thread
From: Wu, Hao A @ 2018-10-26  8:14 UTC (permalink / raw)
  To: Wu, Hao A, Zeng, Star, edk2-devel@lists.01.org
  Cc: Ni, Ruiyu, Yao, Jiewen, Zeng, Star

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Wu, Hao A
> Sent: Friday, October 26, 2018 3:38 PM
> To: Zeng, Star; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu; Yao, Jiewen; Zeng, Star
> Subject: Re: [edk2] [PATCH V2 3/4] MdeModulePkg XhciDxe: Use common
> buffer for AsyncInterruptTransfer
> 
> Reviewed-by: Hao Wu <hao.a.wu@intel.com>

Sorry, missed one question for this patch.
Please see below for more detail:

> 
> Best Regards,
> Hao Wu
> 
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > Star Zeng
> > Sent: Friday, October 26, 2018 1:42 PM
> > To: edk2-devel@lists.01.org
> > Cc: Ni, Ruiyu; Wu, Hao A; Yao, Jiewen; Zeng, Star
> > Subject: [edk2] [PATCH V2 3/4] MdeModulePkg XhciDxe: Use common
> buffer
> > for AsyncInterruptTransfer
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1274
> >
> > In current code, XhcMonitorAsyncRequests (timer handler) will do
> > unmap and map operations for AsyncIntTransfers to "Flush data from
> > PCI controller specific address to mapped system memory address".
> > XhcMonitorAsyncRequests
> >   XhcFlushAsyncIntMap
> >     PciIo->Unmap
> >       IoMmu->SetAttribute
> >     PciIo->Map
> >       IoMmu->SetAttribute
> >
> > This may impact the boot performance.
> >
> > Since the data buffer for XhcMonitorAsyncRequests is internal
> > buffer, we can allocate common buffer by PciIo->AllocateBuffer
> > and map the buffer with EfiPciIoOperationBusMasterCommonBuffer,
> > then the unmap and map operations can be removed.
> >
> > ///
> > /// Provides both read and write access to system memory by
> > /// both the processor and a bus master. The buffer is coherent
> > /// from both the processor's and the bus master's point of view.
> > ///
> > EfiPciIoOperationBusMasterCommonBuffer,
> >
> > Test done:
> > USB KB works normally.
> > USB disk read/write works normally.
> >
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Cc: Hao Wu <hao.a.wu@intel.com>
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Star Zeng <star.zeng@intel.com>
> > Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
> > ---
> >  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c      |   1 +
> >  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 141 +++++++++++-----------
> --
> > -------
> >  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h |  27 +++---
> >  3 files changed, 67 insertions(+), 102 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> > b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> > index 7f64f9c7c982..64855a4c158c 100644
> > --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> > @@ -769,6 +769,7 @@ XhcTransfer (
> >            MaximumPacketLength,
> >            Type,
> >            Request,
> > +          FALSE,
> >            Data,
> >            *DataLength,
> >            NULL,
> > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > index 75959ae08363..d03a6681ce0d 100644
> > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > @@ -118,17 +118,18 @@ ON_EXIT:
> >  /**
> >    Create a new URB for a new transaction.
> >
> > -  @param  Xhc       The XHCI Instance
> > -  @param  BusAddr   The logical device address assigned by UsbBus driver
> > -  @param  EpAddr    Endpoint addrress
> > -  @param  DevSpeed  The device speed
> > -  @param  MaxPacket The max packet length of the endpoint
> > -  @param  Type      The transaction type
> > -  @param  Request   The standard USB request for control transfer
> > -  @param  Data      The user data to transfer
> > -  @param  DataLen   The length of data buffer
> > -  @param  Callback  The function to call when data is transferred
> > -  @param  Context   The context to the callback
> > +  @param  Xhc                   The XHCI Instance
> > +  @param  BusAddr               The logical device address assigned by UsbBus
> > driver
> > +  @param  EpAddr                Endpoint addrress
> > +  @param  DevSpeed              The device speed
> > +  @param  MaxPacket             The max packet length of the endpoint
> > +  @param  Type                  The transaction type
> > +  @param  Request               The standard USB request for control transfer
> > +  @param  AllocateCommonBuffer  Indicate whether need to allocate
> > common buffer for data transfer
> > +  @param  Data                  The user data to transfer, NULL if
> > AllocateCommonBuffer is TRUE
> > +  @param  DataLen               The length of data buffer
> > +  @param  Callback              The function to call when data is transferred
> > +  @param  Context               The context to the callback
> >
> >    @return Created URB or NULL
> >
> > @@ -142,6 +143,7 @@ XhcCreateUrb (
> >    IN UINTN                              MaxPacket,
> >    IN UINTN                              Type,
> >    IN EFI_USB_DEVICE_REQUEST             *Request,
> > +  IN BOOLEAN                            AllocateCommonBuffer,
> >    IN VOID                               *Data,
> >    IN UINTN                              DataLen,
> >    IN EFI_ASYNC_USB_TRANSFER_CALLBACK    Callback,
> > @@ -169,8 +171,24 @@ XhcCreateUrb (
> >    Ep->Type      = Type;
> >
> >    Urb->Request  = Request;
> > +  if (AllocateCommonBuffer) {
> > +    ASSERT (Data == NULL);
> > +    Status = Xhc->PciIo->AllocateBuffer (
> > +                           Xhc->PciIo,
> > +                           AllocateAnyPages,
> > +                           EfiBootServicesData,
> > +                           EFI_SIZE_TO_PAGES (DataLen),
> > +                           &Data,
> > +                           0
> > +                           );
> > +    if (EFI_ERROR (Status) || (Data == NULL)) {
> > +      FreePool (Urb);
> > +      return NULL;
> > +    }
> > +  }
> >    Urb->Data     = Data;
> >    Urb->DataLen  = DataLen;
> > +  Urb->AllocateCommonBuffer = AllocateCommonBuffer;
> >    Urb->Callback = Callback;
> >    Urb->Context  = Context;
> >
> > @@ -178,6 +196,11 @@ XhcCreateUrb (
> >    ASSERT_EFI_ERROR (Status);
> >    if (EFI_ERROR (Status)) {
> >      DEBUG ((EFI_D_ERROR, "XhcCreateUrb: XhcCreateTransferTrb Failed,
> > Status = %r\n", Status));
> > +    Xhc->PciIo->FreeBuffer (
> > +                  Xhc->PciIo,
> > +                  EFI_SIZE_TO_PAGES (Urb->DataLen),
> > +                  Urb->Data
> > +                  );

Does this "Xhc->PciIo->FreeBuffer()" call here need Urb->AllocateCommonBuffer
check?

Best Regards,
Hao Wu

> >      FreePool (Urb);
> >      Urb = NULL;
> >    }
> > @@ -206,6 +229,14 @@ XhcFreeUrb (
> >      Xhc->PciIo->Unmap (Xhc->PciIo, Urb->DataMap);
> >    }
> >
> > +  if (Urb->AllocateCommonBuffer) {
> > +    Xhc->PciIo->FreeBuffer (
> > +                  Xhc->PciIo,
> > +                  EFI_SIZE_TO_PAGES (Urb->DataLen),
> > +                  Urb->Data
> > +                  );
> > +  }
> > +
> >    FreePool (Urb);
> >  }
> >
> > @@ -264,10 +295,14 @@ XhcCreateTransferTrb (
> >    // No need to remap.
> >    //
> >    if ((Urb->Data != NULL) && (Urb->DataMap == NULL)) {
> > -    if (((UINT8) (Urb->Ep.Direction)) == EfiUsbDataIn) {
> > -      MapOp = EfiPciIoOperationBusMasterWrite;
> > +    if (Urb->AllocateCommonBuffer) {
> > +      MapOp = EfiPciIoOperationBusMasterCommonBuffer;
> >      } else {
> > -      MapOp = EfiPciIoOperationBusMasterRead;
> > +      if (((UINT8) (Urb->Ep.Direction)) == EfiUsbDataIn) {
> > +        MapOp = EfiPciIoOperationBusMasterWrite;
> > +      } else {
> > +        MapOp = EfiPciIoOperationBusMasterRead;
> > +      }
> >      }
> >
> >      Len = Urb->DataLen;
> > @@ -1367,7 +1402,6 @@ XhciDelAsyncIntTransfer (
> >        }
> >
> >        RemoveEntryList (&Urb->UrbList);
> > -      FreePool (Urb->Data);
> >        XhcFreeUrb (Xhc, Urb);
> >        return EFI_SUCCESS;
> >      }
> > @@ -1405,7 +1439,6 @@ XhciDelAllAsyncIntTransfers (
> >      }
> >
> >      RemoveEntryList (&Urb->UrbList);
> > -    FreePool (Urb->Data);
> >      XhcFreeUrb (Xhc, Urb);
> >    }
> >  }
> > @@ -1438,15 +1471,8 @@ XhciInsertAsyncIntTransfer (
> >    IN VOID                               *Context
> >    )
> >  {
> > -  VOID      *Data;
> >    URB       *Urb;
> >
> > -  Data = AllocateZeroPool (DataLen);
> > -  if (Data == NULL) {
> > -    DEBUG ((DEBUG_ERROR, "%a: failed to allocate buffer\n",
> > __FUNCTION__));
> > -    return NULL;
> > -  }
> > -
> >    Urb = XhcCreateUrb (
> >            Xhc,
> >            BusAddr,
> > @@ -1455,14 +1481,14 @@ XhciInsertAsyncIntTransfer (
> >            MaxPacket,
> >            XHC_INT_TRANSFER_ASYNC,
> >            NULL,
> > -          Data,
> > +          TRUE,
> > +          NULL,
> >            DataLen,
> >            Callback,
> >            Context
> >            );
> >    if (Urb == NULL) {
> >      DEBUG ((DEBUG_ERROR, "%a: failed to create URB\n",
> __FUNCTION__));
> > -    FreePool (Data);
> >      return NULL;
> >    }
> >
> > @@ -1503,61 +1529,6 @@ XhcUpdateAsyncRequest (
> >  }
> >
> >  /**
> > -  Flush data from PCI controller specific address to mapped system
> > -  memory address.
> > -
> > -  @param  Xhc                The XHCI device.
> > -  @param  Urb                The URB to unmap.
> > -
> > -  @retval EFI_SUCCESS        Success to flush data to mapped system
> memory.
> > -  @retval EFI_DEVICE_ERROR   Fail to flush data to mapped system
> memory.
> > -
> > -**/
> > -EFI_STATUS
> > -XhcFlushAsyncIntMap (
> > -  IN  USB_XHCI_INSTANCE   *Xhc,
> > -  IN  URB                 *Urb
> > -  )
> > -{
> > -  EFI_STATUS                    Status;
> > -  EFI_PHYSICAL_ADDRESS          PhyAddr;
> > -  EFI_PCI_IO_PROTOCOL_OPERATION MapOp;
> > -  EFI_PCI_IO_PROTOCOL           *PciIo;
> > -  UINTN                         Len;
> > -  VOID                          *Map;
> > -
> > -  PciIo = Xhc->PciIo;
> > -  Len   = Urb->DataLen;
> > -
> > -  if (Urb->Ep.Direction == EfiUsbDataIn) {
> > -    MapOp = EfiPciIoOperationBusMasterWrite;
> > -  } else {
> > -    MapOp = EfiPciIoOperationBusMasterRead;
> > -  }
> > -
> > -  if (Urb->DataMap != NULL) {
> > -    Status = PciIo->Unmap (PciIo, Urb->DataMap);
> > -    if (EFI_ERROR (Status)) {
> > -      goto ON_ERROR;
> > -    }
> > -  }
> > -
> > -  Urb->DataMap = NULL;
> > -
> > -  Status = PciIo->Map (PciIo, MapOp, Urb->Data, &Len, &PhyAddr, &Map);
> > -  if (EFI_ERROR (Status) || (Len != Urb->DataLen)) {
> > -    goto ON_ERROR;
> > -  }
> > -
> > -  Urb->DataPhy  = (VOID *) ((UINTN) PhyAddr);
> > -  Urb->DataMap  = Map;
> > -  return EFI_SUCCESS;
> > -
> > -ON_ERROR:
> > -  return EFI_DEVICE_ERROR;
> > -}
> > -
> > -/**
> >    Interrupt transfer periodic check handler.
> >
> >    @param  Event                 Interrupt event.
> > @@ -1577,7 +1548,6 @@ XhcMonitorAsyncRequests (
> >    UINT8                   *ProcBuf;
> >    URB                     *Urb;
> >    UINT8                   SlotId;
> > -  EFI_STATUS              Status;
> >    EFI_TPL                 OldTpl;
> >
> >    OldTpl = gBS->RaiseTPL (XHC_TPL);
> > @@ -1606,15 +1576,6 @@ XhcMonitorAsyncRequests (
> >      }
> >
> >      //
> > -    // Flush any PCI posted write transactions from a PCI host
> > -    // bridge to system memory.
> > -    //
> > -    Status = XhcFlushAsyncIntMap (Xhc, Urb);
> > -    if (EFI_ERROR (Status)) {
> > -      DEBUG ((EFI_D_ERROR, "XhcMonitorAsyncRequests: Fail to Flush
> > AsyncInt Mapped Memeory\n"));
> > -    }
> > -
> > -    //
> >      // Allocate a buffer then copy the transferred data for user.
> >      // If failed to allocate the buffer, update the URB for next
> >      // round of transfer. Ignore the data of this round.
> > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
> > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
> > index cd1403f2842a..2b5b95b7fb60 100644
> > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
> > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
> > @@ -172,6 +172,7 @@ typedef struct _URB {
> >    //
> >    USB_ENDPOINT                    Ep;
> >    EFI_USB_DEVICE_REQUEST          *Request;
> > +  BOOLEAN                         AllocateCommonBuffer;
> >    VOID                            *Data;
> >    UINTN                           DataLen;
> >    VOID                            *DataPhy;
> > @@ -1432,17 +1433,18 @@ XhcSetTrDequeuePointer (
> >  /**
> >    Create a new URB for a new transaction.
> >
> > -  @param  Xhc       The XHCI Instance
> > -  @param  DevAddr   The device address
> > -  @param  EpAddr    Endpoint addrress
> > -  @param  DevSpeed  The device speed
> > -  @param  MaxPacket The max packet length of the endpoint
> > -  @param  Type      The transaction type
> > -  @param  Request   The standard USB request for control transfer
> > -  @param  Data      The user data to transfer
> > -  @param  DataLen   The length of data buffer
> > -  @param  Callback  The function to call when data is transferred
> > -  @param  Context   The context to the callback
> > +  @param  Xhc                   The XHCI Instance
> > +  @param  BusAddr               The logical device address assigned by UsbBus
> > driver
> > +  @param  EpAddr                Endpoint addrress
> > +  @param  DevSpeed              The device speed
> > +  @param  MaxPacket             The max packet length of the endpoint
> > +  @param  Type                  The transaction type
> > +  @param  Request               The standard USB request for control transfer
> > +  @param  AllocateCommonBuffer  Indicate whether need to allocate
> > common buffer for data transfer
> > +  @param  Data                  The user data to transfer, NULL if
> > AllocateCommonBuffer is TRUE
> > +  @param  DataLen               The length of data buffer
> > +  @param  Callback              The function to call when data is transferred
> > +  @param  Context               The context to the callback
> >
> >    @return Created URB or NULL
> >
> > @@ -1450,12 +1452,13 @@ XhcSetTrDequeuePointer (
> >  URB*
> >  XhcCreateUrb (
> >    IN USB_XHCI_INSTANCE                  *Xhc,
> > -  IN UINT8                              DevAddr,
> > +  IN UINT8                              BusAddr,
> >    IN UINT8                              EpAddr,
> >    IN UINT8                              DevSpeed,
> >    IN UINTN                              MaxPacket,
> >    IN UINTN                              Type,
> >    IN EFI_USB_DEVICE_REQUEST             *Request,
> > +  IN BOOLEAN                            AllocateCommonBuffer,
> >    IN VOID                               *Data,
> >    IN UINTN                              DataLen,
> >    IN EFI_ASYNC_USB_TRANSFER_CALLBACK    Callback,
> > --
> > 2.7.0.windows.1
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH V2 1/4] MdeModulePkg XhciDxe: Extract new XhciInsertAsyncIntTransfer function
  2018-10-26  7:33   ` Wu, Hao A
@ 2018-10-26 13:42     ` Zeng, Star
  0 siblings, 0 replies; 12+ messages in thread
From: Zeng, Star @ 2018-10-26 13:42 UTC (permalink / raw)
  To: Wu, Hao A, edk2-devel@lists.01.org
  Cc: Ni, Ruiyu, Wang, Jian J, Yao, Jiewen, Zeng, Star

Good catch. I just posted V3 patches to cover it.

Thanks,
Star
-----Original Message-----
From: Wu, Hao A 
Sent: Friday, October 26, 2018 3:33 PM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
Subject: RE: [PATCH V2 1/4] MdeModulePkg XhciDxe: Extract new XhciInsertAsyncIntTransfer function

Hi Star,

The interface for function XhciInsertAsyncIntTransfer() does not match between XhciSched.c and XhciSched.h.

Other than that:
Reviewed-by: Hao Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu


> -----Original Message-----
> From: Zeng, Star
> Sent: Friday, October 26, 2018 1:42 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star; Ni, Ruiyu; Wu, Hao A; Wang, Jian J; Yao, Jiewen
> Subject: [PATCH V2 1/4] MdeModulePkg XhciDxe: Extract new 
> XhciInsertAsyncIntTransfer function
> 
> V2:
> Add the missing "FreePool (Data);".
> Remove the unnecessary indentation change.
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1274
> 
> Extract new XhciInsertAsyncIntTransfer function from 
> XhcAsyncInterruptTransfer.
> 
> It is code preparation for following patch, no essential functional 
> change.
> 
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Hao Wu <hao.a.wu@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
>  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c      | 18 +--------
>  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 65
> ++++++++++++++++++++++++++++++++
>  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h | 28 ++++++++++++++
>  3 files changed, 94 insertions(+), 17 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> index f1c60bef01c0..7f64f9c7c982 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> @@ -1346,7 +1346,6 @@ XhcAsyncInterruptTransfer (
>    EFI_STATUS              Status;
>    UINT8                   SlotId;
>    UINT8                   Index;
> -  UINT8                   *Data;
>    EFI_TPL                 OldTpl;
> 
>    //
> @@ -1413,36 +1412,21 @@ XhcAsyncInterruptTransfer (
>      goto ON_EXIT;
>    }
> 
> -  Data = AllocateZeroPool (DataLength);
> -
> -  if (Data == NULL) {
> -    DEBUG ((EFI_D_ERROR, "XhcAsyncInterruptTransfer: failed to allocate
> buffer\n"));
> -    Status = EFI_OUT_OF_RESOURCES;
> -    goto ON_EXIT;
> -  }
> -
> -  Urb = XhcCreateUrb (
> +  Urb = XhciInsertAsyncIntTransfer (
>            Xhc,
>            DeviceAddress,
>            EndPointAddress,
>            DeviceSpeed,
>            MaximumPacketLength,
> -          XHC_INT_TRANSFER_ASYNC,
> -          NULL,
> -          Data,
>            DataLength,
>            CallBackFunction,
>            Context
>            );
> -
>    if (Urb == NULL) {
> -    DEBUG ((EFI_D_ERROR, "XhcAsyncInterruptTransfer: failed to create
> URB\n"));
> -    FreePool (Data);
>      Status = EFI_OUT_OF_RESOURCES;
>      goto ON_EXIT;
>    }
> 
> -  InsertHeadList (&Xhc->AsyncIntTransfers, &Urb->UrbList);
>    //
>    // Ring the doorbell
>    //
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> index 166c44bf5e66..75959ae08363 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> @@ -1411,6 +1411,71 @@ XhciDelAllAsyncIntTransfers (  }
> 
>  /**
> +  Insert a single asynchronous interrupt transfer for  the device and 
> + endpoint.
> +
> +  @param Xhc            The XHCI Instance
> +  @param BusAddr        The logical device address assigned by UsbBus driver
> +  @param EpAddr         Endpoint addrress
> +  @param DevSpeed       The device speed
> +  @param MaxPacket      The max packet length of the endpoint
> +  @param DataLen        The length of data buffer
> +  @param Callback       The function to call when data is transferred
> +  @param Context        The context to the callback
> +
> +  @return Created URB or NULL
> +
> +**/
> +URB *
> +XhciInsertAsyncIntTransfer (
> +  IN USB_XHCI_INSTANCE                  *Xhc,
> +  IN UINT8                              BusAddr,
> +  IN UINT8                              EpAddr,
> +  IN UINT8                              DevSpeed,
> +  IN UINTN                              MaxPacket,
> +  IN UINTN                              DataLen,
> +  IN EFI_ASYNC_USB_TRANSFER_CALLBACK    Callback,
> +  IN VOID                               *Context
> +  )
> +{
> +  VOID      *Data;
> +  URB       *Urb;
> +
> +  Data = AllocateZeroPool (DataLen);
> +  if (Data == NULL) {
> +    DEBUG ((DEBUG_ERROR, "%a: failed to allocate buffer\n",
> __FUNCTION__));
> +    return NULL;
> +  }
> +
> +  Urb = XhcCreateUrb (
> +          Xhc,
> +          BusAddr,
> +          EpAddr,
> +          DevSpeed,
> +          MaxPacket,
> +          XHC_INT_TRANSFER_ASYNC,
> +          NULL,
> +          Data,
> +          DataLen,
> +          Callback,
> +          Context
> +          );
> +  if (Urb == NULL) {
> +    DEBUG ((DEBUG_ERROR, "%a: failed to create URB\n", __FUNCTION__));
> +    FreePool (Data);
> +    return NULL;
> +  }
> +
> +  //
> +  // New asynchronous transfer must inserted to the head.
> +  // Check the comments in XhcMoniteAsyncRequests  //  InsertHeadList 
> + (&Xhc->AsyncIntTransfers, &Urb->UrbList);
> +
> +  return Urb;
> +}
> +
> +/**
>    Update the queue head for next round of asynchronous transfer
> 
>    @param  Xhc     The XHCI Instance.
> diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
> b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
> index 097408828a1f..cd1403f2842a 100644
> --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
> +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
> @@ -853,6 +853,34 @@ XhciDelAllAsyncIntTransfers (
>    );
> 
>  /**
> +  Insert a single asynchronous interrupt transfer for  the device and 
> + endpoint.
> +
> +  @param Xhc            The XHCI Instance
> +  @param BusAddr        The logical device address assigned by UsbBus driver
> +  @param EpAddr         Endpoint addrress
> +  @param DevSpeed       The device speed
> +  @param MaxPacket      The max packet length of the endpoint
> +  @param DataLen        The length of data buffer
> +  @param Callback       The function to call when data is transferred
> +  @param Context        The context to the callback
> +
> +  @return Created URB or NULL
> +
> +**/
> +URB *
> +XhciInsertAsyncIntTransfer (
> +  IN USB_XHCI_INSTANCE                  *Xhc,
> +  IN UINT8                              DeviceAddress,
> +  IN UINT8                              EndPointAddress,
> +  IN UINT8                              DeviceSpeed,
> +  IN UINTN                              MaximumPacketLength,
> +  IN UINTN                              DataLength,
> +  IN EFI_ASYNC_USB_TRANSFER_CALLBACK    CallBackFunction,
> +  IN VOID                               *Context
> +  );
> +
> +/**
>    Set Bios Ownership
> 
>    @param  Xhc          The XHCI Instance.
> --
> 2.7.0.windows.1



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH V2 3/4] MdeModulePkg XhciDxe: Use common buffer for AsyncInterruptTransfer
  2018-10-26  8:14     ` Wu, Hao A
@ 2018-10-26 13:43       ` Zeng, Star
  0 siblings, 0 replies; 12+ messages in thread
From: Zeng, Star @ 2018-10-26 13:43 UTC (permalink / raw)
  To: Wu, Hao A, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu, Yao, Jiewen, Zeng, Star

Good catch. We can use XhcFreeUrb directly. I just posted V3 patches to cover it.

Thanks,
Star
-----Original Message-----
From: Wu, Hao A 
Sent: Friday, October 26, 2018 4:15 PM
To: Wu, Hao A <hao.a.wu@intel.com>; Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: RE: [edk2] [PATCH V2 3/4] MdeModulePkg XhciDxe: Use common buffer for AsyncInterruptTransfer

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 
> Wu, Hao A
> Sent: Friday, October 26, 2018 3:38 PM
> To: Zeng, Star; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu; Yao, Jiewen; Zeng, Star
> Subject: Re: [edk2] [PATCH V2 3/4] MdeModulePkg XhciDxe: Use common 
> buffer for AsyncInterruptTransfer
> 
> Reviewed-by: Hao Wu <hao.a.wu@intel.com>

Sorry, missed one question for this patch.
Please see below for more detail:

> 
> Best Regards,
> Hao Wu
> 
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf 
> > Of Star Zeng
> > Sent: Friday, October 26, 2018 1:42 PM
> > To: edk2-devel@lists.01.org
> > Cc: Ni, Ruiyu; Wu, Hao A; Yao, Jiewen; Zeng, Star
> > Subject: [edk2] [PATCH V2 3/4] MdeModulePkg XhciDxe: Use common
> buffer
> > for AsyncInterruptTransfer
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1274
> >
> > In current code, XhcMonitorAsyncRequests (timer handler) will do 
> > unmap and map operations for AsyncIntTransfers to "Flush data from 
> > PCI controller specific address to mapped system memory address".
> > XhcMonitorAsyncRequests
> >   XhcFlushAsyncIntMap
> >     PciIo->Unmap
> >       IoMmu->SetAttribute
> >     PciIo->Map
> >       IoMmu->SetAttribute
> >
> > This may impact the boot performance.
> >
> > Since the data buffer for XhcMonitorAsyncRequests is internal 
> > buffer, we can allocate common buffer by PciIo->AllocateBuffer and 
> > map the buffer with EfiPciIoOperationBusMasterCommonBuffer,
> > then the unmap and map operations can be removed.
> >
> > ///
> > /// Provides both read and write access to system memory by /// both 
> > the processor and a bus master. The buffer is coherent /// from both 
> > the processor's and the bus master's point of view.
> > ///
> > EfiPciIoOperationBusMasterCommonBuffer,
> >
> > Test done:
> > USB KB works normally.
> > USB disk read/write works normally.
> >
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Cc: Hao Wu <hao.a.wu@intel.com>
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Star Zeng <star.zeng@intel.com>
> > Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
> > ---
> >  MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c      |   1 +
> >  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 141 
> > +++++++++++-----------
> --
> > -------
> >  MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h |  27 +++---
> >  3 files changed, 67 insertions(+), 102 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> > b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> > index 7f64f9c7c982..64855a4c158c 100644
> > --- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
> > @@ -769,6 +769,7 @@ XhcTransfer (
> >            MaximumPacketLength,
> >            Type,
> >            Request,
> > +          FALSE,
> >            Data,
> >            *DataLength,
> >            NULL,
> > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > index 75959ae08363..d03a6681ce0d 100644
> > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
> > @@ -118,17 +118,18 @@ ON_EXIT:
> >  /**
> >    Create a new URB for a new transaction.
> >
> > -  @param  Xhc       The XHCI Instance
> > -  @param  BusAddr   The logical device address assigned by UsbBus driver
> > -  @param  EpAddr    Endpoint addrress
> > -  @param  DevSpeed  The device speed
> > -  @param  MaxPacket The max packet length of the endpoint
> > -  @param  Type      The transaction type
> > -  @param  Request   The standard USB request for control transfer
> > -  @param  Data      The user data to transfer
> > -  @param  DataLen   The length of data buffer
> > -  @param  Callback  The function to call when data is transferred
> > -  @param  Context   The context to the callback
> > +  @param  Xhc                   The XHCI Instance
> > +  @param  BusAddr               The logical device address assigned by UsbBus
> > driver
> > +  @param  EpAddr                Endpoint addrress
> > +  @param  DevSpeed              The device speed
> > +  @param  MaxPacket             The max packet length of the endpoint
> > +  @param  Type                  The transaction type
> > +  @param  Request               The standard USB request for control transfer
> > +  @param  AllocateCommonBuffer  Indicate whether need to allocate
> > common buffer for data transfer
> > +  @param  Data                  The user data to transfer, NULL if
> > AllocateCommonBuffer is TRUE
> > +  @param  DataLen               The length of data buffer
> > +  @param  Callback              The function to call when data is transferred
> > +  @param  Context               The context to the callback
> >
> >    @return Created URB or NULL
> >
> > @@ -142,6 +143,7 @@ XhcCreateUrb (
> >    IN UINTN                              MaxPacket,
> >    IN UINTN                              Type,
> >    IN EFI_USB_DEVICE_REQUEST             *Request,
> > +  IN BOOLEAN                            AllocateCommonBuffer,
> >    IN VOID                               *Data,
> >    IN UINTN                              DataLen,
> >    IN EFI_ASYNC_USB_TRANSFER_CALLBACK    Callback,
> > @@ -169,8 +171,24 @@ XhcCreateUrb (
> >    Ep->Type      = Type;
> >
> >    Urb->Request  = Request;
> > +  if (AllocateCommonBuffer) {
> > +    ASSERT (Data == NULL);
> > +    Status = Xhc->PciIo->AllocateBuffer (
> > +                           Xhc->PciIo,
> > +                           AllocateAnyPages,
> > +                           EfiBootServicesData,
> > +                           EFI_SIZE_TO_PAGES (DataLen),
> > +                           &Data,
> > +                           0
> > +                           );
> > +    if (EFI_ERROR (Status) || (Data == NULL)) {
> > +      FreePool (Urb);
> > +      return NULL;
> > +    }
> > +  }
> >    Urb->Data     = Data;
> >    Urb->DataLen  = DataLen;
> > +  Urb->AllocateCommonBuffer = AllocateCommonBuffer;
> >    Urb->Callback = Callback;
> >    Urb->Context  = Context;
> >
> > @@ -178,6 +196,11 @@ XhcCreateUrb (
> >    ASSERT_EFI_ERROR (Status);
> >    if (EFI_ERROR (Status)) {
> >      DEBUG ((EFI_D_ERROR, "XhcCreateUrb: XhcCreateTransferTrb 
> > Failed, Status = %r\n", Status));
> > +    Xhc->PciIo->FreeBuffer (
> > +                  Xhc->PciIo,
> > +                  EFI_SIZE_TO_PAGES (Urb->DataLen),
> > +                  Urb->Data
> > +                  );

Does this "Xhc->PciIo->FreeBuffer()" call here need Urb->AllocateCommonBuffer check?

Best Regards,
Hao Wu

> >      FreePool (Urb);
> >      Urb = NULL;
> >    }
> > @@ -206,6 +229,14 @@ XhcFreeUrb (
> >      Xhc->PciIo->Unmap (Xhc->PciIo, Urb->DataMap);
> >    }
> >
> > +  if (Urb->AllocateCommonBuffer) {
> > +    Xhc->PciIo->FreeBuffer (
> > +                  Xhc->PciIo,
> > +                  EFI_SIZE_TO_PAGES (Urb->DataLen),
> > +                  Urb->Data
> > +                  );
> > +  }
> > +
> >    FreePool (Urb);
> >  }
> >
> > @@ -264,10 +295,14 @@ XhcCreateTransferTrb (
> >    // No need to remap.
> >    //
> >    if ((Urb->Data != NULL) && (Urb->DataMap == NULL)) {
> > -    if (((UINT8) (Urb->Ep.Direction)) == EfiUsbDataIn) {
> > -      MapOp = EfiPciIoOperationBusMasterWrite;
> > +    if (Urb->AllocateCommonBuffer) {
> > +      MapOp = EfiPciIoOperationBusMasterCommonBuffer;
> >      } else {
> > -      MapOp = EfiPciIoOperationBusMasterRead;
> > +      if (((UINT8) (Urb->Ep.Direction)) == EfiUsbDataIn) {
> > +        MapOp = EfiPciIoOperationBusMasterWrite;
> > +      } else {
> > +        MapOp = EfiPciIoOperationBusMasterRead;
> > +      }
> >      }
> >
> >      Len = Urb->DataLen;
> > @@ -1367,7 +1402,6 @@ XhciDelAsyncIntTransfer (
> >        }
> >
> >        RemoveEntryList (&Urb->UrbList);
> > -      FreePool (Urb->Data);
> >        XhcFreeUrb (Xhc, Urb);
> >        return EFI_SUCCESS;
> >      }
> > @@ -1405,7 +1439,6 @@ XhciDelAllAsyncIntTransfers (
> >      }
> >
> >      RemoveEntryList (&Urb->UrbList);
> > -    FreePool (Urb->Data);
> >      XhcFreeUrb (Xhc, Urb);
> >    }
> >  }
> > @@ -1438,15 +1471,8 @@ XhciInsertAsyncIntTransfer (
> >    IN VOID                               *Context
> >    )
> >  {
> > -  VOID      *Data;
> >    URB       *Urb;
> >
> > -  Data = AllocateZeroPool (DataLen);
> > -  if (Data == NULL) {
> > -    DEBUG ((DEBUG_ERROR, "%a: failed to allocate buffer\n",
> > __FUNCTION__));
> > -    return NULL;
> > -  }
> > -
> >    Urb = XhcCreateUrb (
> >            Xhc,
> >            BusAddr,
> > @@ -1455,14 +1481,14 @@ XhciInsertAsyncIntTransfer (
> >            MaxPacket,
> >            XHC_INT_TRANSFER_ASYNC,
> >            NULL,
> > -          Data,
> > +          TRUE,
> > +          NULL,
> >            DataLen,
> >            Callback,
> >            Context
> >            );
> >    if (Urb == NULL) {
> >      DEBUG ((DEBUG_ERROR, "%a: failed to create URB\n",
> __FUNCTION__));
> > -    FreePool (Data);
> >      return NULL;
> >    }
> >
> > @@ -1503,61 +1529,6 @@ XhcUpdateAsyncRequest (  }
> >
> >  /**
> > -  Flush data from PCI controller specific address to mapped system
> > -  memory address.
> > -
> > -  @param  Xhc                The XHCI device.
> > -  @param  Urb                The URB to unmap.
> > -
> > -  @retval EFI_SUCCESS        Success to flush data to mapped system
> memory.
> > -  @retval EFI_DEVICE_ERROR   Fail to flush data to mapped system
> memory.
> > -
> > -**/
> > -EFI_STATUS
> > -XhcFlushAsyncIntMap (
> > -  IN  USB_XHCI_INSTANCE   *Xhc,
> > -  IN  URB                 *Urb
> > -  )
> > -{
> > -  EFI_STATUS                    Status;
> > -  EFI_PHYSICAL_ADDRESS          PhyAddr;
> > -  EFI_PCI_IO_PROTOCOL_OPERATION MapOp;
> > -  EFI_PCI_IO_PROTOCOL           *PciIo;
> > -  UINTN                         Len;
> > -  VOID                          *Map;
> > -
> > -  PciIo = Xhc->PciIo;
> > -  Len   = Urb->DataLen;
> > -
> > -  if (Urb->Ep.Direction == EfiUsbDataIn) {
> > -    MapOp = EfiPciIoOperationBusMasterWrite;
> > -  } else {
> > -    MapOp = EfiPciIoOperationBusMasterRead;
> > -  }
> > -
> > -  if (Urb->DataMap != NULL) {
> > -    Status = PciIo->Unmap (PciIo, Urb->DataMap);
> > -    if (EFI_ERROR (Status)) {
> > -      goto ON_ERROR;
> > -    }
> > -  }
> > -
> > -  Urb->DataMap = NULL;
> > -
> > -  Status = PciIo->Map (PciIo, MapOp, Urb->Data, &Len, &PhyAddr, 
> > &Map);
> > -  if (EFI_ERROR (Status) || (Len != Urb->DataLen)) {
> > -    goto ON_ERROR;
> > -  }
> > -
> > -  Urb->DataPhy  = (VOID *) ((UINTN) PhyAddr);
> > -  Urb->DataMap  = Map;
> > -  return EFI_SUCCESS;
> > -
> > -ON_ERROR:
> > -  return EFI_DEVICE_ERROR;
> > -}
> > -
> > -/**
> >    Interrupt transfer periodic check handler.
> >
> >    @param  Event                 Interrupt event.
> > @@ -1577,7 +1548,6 @@ XhcMonitorAsyncRequests (
> >    UINT8                   *ProcBuf;
> >    URB                     *Urb;
> >    UINT8                   SlotId;
> > -  EFI_STATUS              Status;
> >    EFI_TPL                 OldTpl;
> >
> >    OldTpl = gBS->RaiseTPL (XHC_TPL); @@ -1606,15 +1576,6 @@ 
> > XhcMonitorAsyncRequests (
> >      }
> >
> >      //
> > -    // Flush any PCI posted write transactions from a PCI host
> > -    // bridge to system memory.
> > -    //
> > -    Status = XhcFlushAsyncIntMap (Xhc, Urb);
> > -    if (EFI_ERROR (Status)) {
> > -      DEBUG ((EFI_D_ERROR, "XhcMonitorAsyncRequests: Fail to Flush
> > AsyncInt Mapped Memeory\n"));
> > -    }
> > -
> > -    //
> >      // Allocate a buffer then copy the transferred data for user.
> >      // If failed to allocate the buffer, update the URB for next
> >      // round of transfer. Ignore the data of this round.
> > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
> > b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
> > index cd1403f2842a..2b5b95b7fb60 100644
> > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
> > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h
> > @@ -172,6 +172,7 @@ typedef struct _URB {
> >    //
> >    USB_ENDPOINT                    Ep;
> >    EFI_USB_DEVICE_REQUEST          *Request;
> > +  BOOLEAN                         AllocateCommonBuffer;
> >    VOID                            *Data;
> >    UINTN                           DataLen;
> >    VOID                            *DataPhy;
> > @@ -1432,17 +1433,18 @@ XhcSetTrDequeuePointer (
> >  /**
> >    Create a new URB for a new transaction.
> >
> > -  @param  Xhc       The XHCI Instance
> > -  @param  DevAddr   The device address
> > -  @param  EpAddr    Endpoint addrress
> > -  @param  DevSpeed  The device speed
> > -  @param  MaxPacket The max packet length of the endpoint
> > -  @param  Type      The transaction type
> > -  @param  Request   The standard USB request for control transfer
> > -  @param  Data      The user data to transfer
> > -  @param  DataLen   The length of data buffer
> > -  @param  Callback  The function to call when data is transferred
> > -  @param  Context   The context to the callback
> > +  @param  Xhc                   The XHCI Instance
> > +  @param  BusAddr               The logical device address assigned by UsbBus
> > driver
> > +  @param  EpAddr                Endpoint addrress
> > +  @param  DevSpeed              The device speed
> > +  @param  MaxPacket             The max packet length of the endpoint
> > +  @param  Type                  The transaction type
> > +  @param  Request               The standard USB request for control transfer
> > +  @param  AllocateCommonBuffer  Indicate whether need to allocate
> > common buffer for data transfer
> > +  @param  Data                  The user data to transfer, NULL if
> > AllocateCommonBuffer is TRUE
> > +  @param  DataLen               The length of data buffer
> > +  @param  Callback              The function to call when data is transferred
> > +  @param  Context               The context to the callback
> >
> >    @return Created URB or NULL
> >
> > @@ -1450,12 +1452,13 @@ XhcSetTrDequeuePointer (
> >  URB*
> >  XhcCreateUrb (
> >    IN USB_XHCI_INSTANCE                  *Xhc,
> > -  IN UINT8                              DevAddr,
> > +  IN UINT8                              BusAddr,
> >    IN UINT8                              EpAddr,
> >    IN UINT8                              DevSpeed,
> >    IN UINTN                              MaxPacket,
> >    IN UINTN                              Type,
> >    IN EFI_USB_DEVICE_REQUEST             *Request,
> > +  IN BOOLEAN                            AllocateCommonBuffer,
> >    IN VOID                               *Data,
> >    IN UINTN                              DataLen,
> >    IN EFI_ASYNC_USB_TRANSFER_CALLBACK    Callback,
> > --
> > 2.7.0.windows.1
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2018-10-26 13:43 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-10-26  5:42 [PATCH V2 0/4] Remove unnecessary Map/Unmap in XhciDxe/EhciDxe for AsyncInterruptTransfer Star Zeng
2018-10-26  5:42 ` [PATCH V2 1/4] MdeModulePkg XhciDxe: Extract new XhciInsertAsyncIntTransfer function Star Zeng
2018-10-26  7:33   ` Wu, Hao A
2018-10-26 13:42     ` Zeng, Star
2018-10-26  5:42 ` [PATCH V2 2/4] MdeModulePkg EhciDxe: Extract new EhciInsertAsyncIntTransfer function Star Zeng
2018-10-26  7:54   ` Wu, Hao A
2018-10-26  5:42 ` [PATCH V2 3/4] MdeModulePkg XhciDxe: Use common buffer for AsyncInterruptTransfer Star Zeng
2018-10-26  7:38   ` Wu, Hao A
2018-10-26  8:14     ` Wu, Hao A
2018-10-26 13:43       ` Zeng, Star
2018-10-26  5:42 ` [PATCH V2 4/4] MdeModulePkg EhciDxe: " Star Zeng
2018-10-26  8:12   ` Wu, Hao A

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox