public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Ni, Ruiyu" <ruiyu.ni@Intel.com>
To: Star Zeng <star.zeng@intel.com>, edk2-devel@lists.01.org
Cc: Hao Wu <hao.a.wu@intel.com>, Jian J Wang <jian.j.wang@intel.com>,
	Jiewen Yao <jiewen.yao@intel.com>
Subject: Re: [PATCH 3/4] MdeModulePkg XhciDxe: Use common buffer for AsyncInterruptTransfer
Date: Fri, 26 Oct 2018 13:36:35 +0800	[thread overview]
Message-ID: <c34c7ff2-d46b-b84f-9e36-ff1c364a8f29@Intel.com> (raw)
In-Reply-To: <1540465113-103964-4-git-send-email-star.zeng@intel.com>

On 10/25/2018 6:58 PM, Star Zeng wrote:
> 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>
> ---
>   MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c      |   1 +
>   MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 134 +++++++++++--------------------
>   MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h |  27 ++++---
>   3 files changed, 64 insertions(+), 98 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 2d7c08dc5bfa..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,11 +295,15 @@ XhcCreateTransferTrb (
>     // No need to remap.
>     //
>     if ((Urb->Data != NULL) && (Urb->DataMap == NULL)) {
> +    if (Urb->AllocateCommonBuffer) {
> +      MapOp = EfiPciIoOperationBusMasterCommonBuffer;
> +    } else {
>         if (((UINT8) (Urb->Ep.Direction)) == EfiUsbDataIn) {
>           MapOp = EfiPciIoOperationBusMasterWrite;
>         } else {
>           MapOp = EfiPciIoOperationBusMasterRead;
>         }
> +    }
>   
>       Len = Urb->DataLen;
>       Status  = Xhc->PciIo->Map (Xhc->PciIo, MapOp, Urb->Data, &Len, &PhyAddr, &Map);
> @@ -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,7 +1481,8 @@ XhciInsertAsyncIntTransfer (
>             MaxPacket,
>             XHC_INT_TRANSFER_ASYNC,
>             NULL,
> -          Data,
> +          TRUE,
> +          NULL,
>             DataLen,
>             Callback,
>             Context
> @@ -1502,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.
> @@ -1576,7 +1548,6 @@ XhcMonitorAsyncRequests (
>     UINT8                   *ProcBuf;
>     URB                     *Urb;
>     UINT8                   SlotId;
> -  EFI_STATUS              Status;
>     EFI_TPL                 OldTpl;
>   
>     OldTpl = gBS->RaiseTPL (XHC_TPL);
> @@ -1605,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,
> 
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>

-- 
Thanks,
Ray


  reply	other threads:[~2018-10-26  5:35 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-25 10:58 [PATCH 0/4] Remove unnecessary Map/Unmap in XhciDxe/EhciDxe for AsyncInterruptTransfer Star Zeng
2018-10-25 10:58 ` [PATCH 1/4] MdeModulePkg XhciDxe: Extract new XhciInsertAsyncIntTransfer function Star Zeng
2018-10-26  4:57   ` Ni, Ruiyu
2018-10-26  5:01     ` Zeng, Star
2018-10-26  5:05     ` Zeng, Star
2018-10-25 10:58 ` [PATCH 2/4] MdeModulePkg EhciDxe: Extract new EhciInsertAsyncIntTransfer function Star Zeng
2018-10-26  4:59   ` Ni, Ruiyu
2018-10-25 10:58 ` [PATCH 3/4] MdeModulePkg XhciDxe: Use common buffer for AsyncInterruptTransfer Star Zeng
2018-10-26  5:36   ` Ni, Ruiyu [this message]
2018-10-25 10:58 ` [PATCH 4/4] MdeModulePkg EhciDxe: " Star Zeng
2018-10-26  5:36   ` Ni, Ruiyu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c34c7ff2-d46b-b84f-9e36-ff1c364a8f29@Intel.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox