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
next 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 [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