From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.136; helo=mga12.intel.com; envelope-from=ruiyu.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) (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 E22482117B57A for ; Thu, 25 Oct 2018 22:35:20 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 25 Oct 2018 22:35:20 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,426,1534834800"; d="scan'208";a="84604025" Received: from ray-dev.ccr.corp.intel.com (HELO [10.239.9.11]) ([10.239.9.11]) by orsmga008.jf.intel.com with ESMTP; 25 Oct 2018 22:35:18 -0700 To: Star Zeng , edk2-devel@lists.01.org Cc: Hao Wu , Jian J Wang , Jiewen Yao References: <1540465113-103964-1-git-send-email-star.zeng@intel.com> <1540465113-103964-4-git-send-email-star.zeng@intel.com> From: "Ni, Ruiyu" Message-ID: Date: Fri, 26 Oct 2018 13:36:35 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <1540465113-103964-4-git-send-email-star.zeng@intel.com> Subject: Re: [PATCH 3/4] MdeModulePkg XhciDxe: 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 05:35:22 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Hao Wu > Cc: Jian J Wang > Cc: Jiewen Yao > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Star Zeng > --- > 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 -- Thanks, Ray