From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.65; helo=mga03.intel.com; envelope-from=hao.a.wu@intel.com; receiver=edk2-devel@lists.01.org Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id CC9F62117D27A for ; Fri, 26 Oct 2018 01:12:13 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 26 Oct 2018 01:12:12 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,427,1534834800"; d="scan'208";a="81072114" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by fmsmga007.fm.intel.com with ESMTP; 26 Oct 2018 01:12:12 -0700 Received: from fmsmsx121.amr.corp.intel.com (10.18.125.36) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.319.2; Fri, 26 Oct 2018 01:12:12 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by fmsmsx121.amr.corp.intel.com (10.18.125.36) with Microsoft SMTP Server (TLS) id 14.3.319.2; Fri, 26 Oct 2018 01:12:12 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.117]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.161]) with mapi id 14.03.0415.000; Fri, 26 Oct 2018 16:12:10 +0800 From: "Wu, Hao A" To: "Zeng, Star" , "edk2-devel@lists.01.org" CC: "Ni, Ruiyu" , "Yao, Jiewen" , "Zeng, Star" Thread-Topic: [edk2] [PATCH V2 4/4] MdeModulePkg EhciDxe: Use common buffer for AsyncInterruptTransfer Thread-Index: AQHUbO65Gq99MvqGQ0eIHTUpu75mA6UxLT/w Date: Fri, 26 Oct 2018 08:12:09 +0000 Message-ID: References: <1540532541-115032-1-git-send-email-star.zeng@intel.com> <1540532541-115032-5-git-send-email-star.zeng@intel.com> In-Reply-To: <1540532541-115032-5-git-send-email-star.zeng@intel.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH V2 4/4] MdeModulePkg EhciDxe: Use common buffer for AsyncInterruptTransfer X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 26 Oct 2018 08:12:14 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Reviewed-by: Hao Wu 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 >=20 > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1274 >=20 > 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 >=20 > This may impact the boot performance. >=20 > 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. >=20 > /// > /// 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, >=20 > Test done: > USB KB works normally. > USB disk read/write works normally. >=20 > Cc: Ruiyu Ni > Cc: Hao Wu > Cc: Jian J Wang > Cc: Jiewen Yao > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Star Zeng > Reviewed-by: Ruiyu Ni > --- > 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(-) >=20 > 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); >=20 > - gBS->FreePool (Urb->Data); > EhcFreeUrb (Ehc, Urb); > return EFI_SUCCESS; > } > @@ -809,7 +808,6 @@ EhciDelAllAsyncIntTransfers ( > EhcUnlinkQhFromPeriod (Ehc, Urb->Qh); > RemoveEntryList (&Urb->UrbList); >=20 > - gBS->FreePool (Urb->Data); > EhcFreeUrb (Ehc, Urb); > } > } > @@ -849,16 +847,8 @@ EhciInsertAsyncIntTransfer ( > IN UINTN Interval > ) > { > - VOID *Data; > URB *Urb; >=20 > - Data =3D AllocatePool (DataLen); > - > - if (Data =3D=3D NULL) { > - DEBUG ((DEBUG_ERROR, "%a: failed to allocate buffer\n", > __FUNCTION__)); > - return NULL; > - } > - > Urb =3D 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 ( >=20 > if (Urb =3D=3D NULL) { > DEBUG ((DEBUG_ERROR, "%a: failed to create URB\n", __FUNCTION__)); > - gBS->FreePool (Data); > return NULL; > } >=20 > @@ -893,60 +883,6 @@ EhciInsertAsyncIntTransfer ( > } >=20 > /** > - 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 memo= ry. > - @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 =3D Ehc->PciIo; > - Len =3D Urb->DataLen; > - > - if (Urb->Ep.Direction =3D=3D EfiUsbDataIn) { > - MapOp =3D EfiPciIoOperationBusMasterWrite; > - } else { > - MapOp =3D EfiPciIoOperationBusMasterRead; > - } > - > - Status =3D PciIo->Unmap (PciIo, Urb->DataMap); > - if (EFI_ERROR (Status)) { > - goto ON_ERROR; > - } > - > - Urb->DataMap =3D NULL; > - > - Status =3D PciIo->Map (PciIo, MapOp, Urb->Data, &Len, &PhyAddr, &Map); > - if (EFI_ERROR (Status) || (Len !=3D Urb->DataLen)) { > - goto ON_ERROR; > - } > - > - Urb->DataPhy =3D (VOID *) ((UINTN) PhyAddr); > - Urb->DataMap =3D Map; > - return EFI_SUCCESS; > - > -ON_ERROR: > - return EFI_DEVICE_ERROR; > -} > - > - > -/** > Update the queue head for next round of asynchronous transfer. >=20 > @param Ehc The EHCI device. > @@ -1051,7 +987,6 @@ EhcMonitorAsyncRequests ( > BOOLEAN Finished; > UINT8 *ProcBuf; > URB *Urb; > - EFI_STATUS Status; >=20 > OldTpl =3D gBS->RaiseTPL (EHC_TPL); > Ehc =3D (USB2_HC_DEV *) Context; > @@ -1070,15 +1005,6 @@ EhcMonitorAsyncRequests ( > } >=20 > // > - // Flush any PCI posted write transactions from a PCI host > - // bridge to system memory. > - // > - Status =3D 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); > } >=20 > + if (Urb->AllocateCommonBuffer) { > + PciIo->FreeBuffer ( > + PciIo, > + EFI_SIZE_TO_PAGES (Urb->DataLen), > + Urb->Data > + ); > + } > + > if (Urb->Qh !=3D 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 tra= nsfer. > - @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 transf= erred. > @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 =3D EhcConvertPollRate (Interval); >=20 > Urb->Request =3D Request; > + if (AllocateCommonBuffer) { > + ASSERT (Data =3D=3D NULL); > + Status =3D Ehc->PciIo->AllocateBuffer ( > + Ehc->PciIo, > + AllocateAnyPages, > + EfiBootServicesData, > + EFI_SIZE_TO_PAGES (DataLen), > + &Data, > + 0 > + ); > + if (EFI_ERROR (Status) || (Data =3D=3D NULL)) { > + FreePool (Urb); > + return NULL; > + } > + } > Urb->Data =3D Data; > Urb->DataLen =3D DataLen; > + Urb->AllocateCommonBuffer =3D AllocateCommonBuffer; > Urb->Callback =3D Callback; > Urb->Context =3D Context; >=20 > @@ -627,10 +653,14 @@ EhcCreateUrb ( > if (Data !=3D NULL) { > Len =3D DataLen; >=20 > - if (Ep->Direction =3D=3D EfiUsbDataIn) { > - MapOp =3D EfiPciIoOperationBusMasterWrite; > + if (Urb->AllocateCommonBuffer) { > + MapOp =3D EfiPciIoOperationBusMasterCommonBuffer; > } else { > - MapOp =3D EfiPciIoOperationBusMasterRead; > + if (Ep->Direction =3D=3D EfiUsbDataIn) { > + MapOp =3D EfiPciIoOperationBusMasterWrite; > + } else { > + MapOp =3D EfiPciIoOperationBusMasterRead; > + } > } >=20 > Status =3D 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). >=20 > -Copyright (c) 2007 - 2010, Intel Corporation. All rights reserved.
> +Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.
> This program and the accompanying materials > are licensed and made available under the terms and conditions of the BS= D > License > which accompanies this distribution. The full text of the license may b= e > 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. >=20 > - @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 tra= nsfer. > + @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 transf= erred. > + @param Context The context to the callback. > + @param Interval The interval for interrupt transfer. >=20 > @return Created URB or NULL. >=20 > @@ -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 >=20 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel