From: "Wu, Hao A" <hao.a.wu@intel.com>
To: "Zeng, Star" <star.zeng@intel.com>,
"edk2-devel@lists.01.org" <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: [PATCH V2 4/4] MdeModulePkg EhciDxe: Use common buffer for AsyncInterruptTransfer
Date: Fri, 26 Oct 2018 08:12:09 +0000 [thread overview]
Message-ID: <B80AF82E9BFB8E4FBD8C89DA810C6A093C846CE7@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <1540532541-115032-5-git-send-email-star.zeng@intel.com>
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
prev parent reply other threads:[~2018-10-26 8:12 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
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 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=B80AF82E9BFB8E4FBD8C89DA810C6A093C846CE7@SHSMSX104.ccr.corp.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