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 4/4] MdeModulePkg EhciDxe: Use common buffer for AsyncInterruptTransfer
Date: Fri, 26 Oct 2018 13:36:56 +0800 [thread overview]
Message-ID: <1f40a7d2-363d-71bc-4914-a929b8c5b3b0@Intel.com> (raw)
In-Reply-To: <1540465113-103964-5-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, 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>
> ---
> MdeModulePkg/Bus/Pci/EhciDxe/Ehci.c | 3 ++
> MdeModulePkg/Bus/Pci/EhciDxe/EhciSched.c | 77 +-------------------------------
> MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.c | 38 ++++++++++++++--
> MdeModulePkg/Bus/Pci/EhciDxe/EhciUrb.h | 33 +++++++-------
> 4 files changed, 57 insertions(+), 94 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 d38340e49c8d..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,
> @@ -892,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.
> @@ -1050,7 +987,6 @@ EhcMonitorAsyncRequests (
> BOOLEAN Finished;
> UINT8 *ProcBuf;
> URB *Urb;
> - EFI_STATUS Status;
>
> OldTpl = gBS->RaiseTPL (EHC_TPL);
> Ehc = (USB2_HC_DEV *) Context;
> @@ -1069,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,
>
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
--
Thanks,
Ray
prev parent 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
2018-10-25 10:58 ` [PATCH 4/4] MdeModulePkg EhciDxe: " Star Zeng
2018-10-26 5:36 ` Ni, Ruiyu [this message]
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=1f40a7d2-363d-71bc-4914-a929b8c5b3b0@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