From: "Zeng, Star" <star.zeng@intel.com>
To: "Wu, Hao A" <hao.a.wu@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 3/4] MdeModulePkg XhciDxe: Use common buffer for AsyncInterruptTransfer
Date: Fri, 26 Oct 2018 13:43:41 +0000 [thread overview]
Message-ID: <0C09AFA07DD0434D9E2A0C6AEB0483103BC5BF16@shsmsx102.ccr.corp.intel.com> (raw)
In-Reply-To: <B80AF82E9BFB8E4FBD8C89DA810C6A093C846D01@SHSMSX104.ccr.corp.intel.com>
Good catch. We can use XhcFreeUrb directly. I just posted V3 patches to cover it.
Thanks,
Star
-----Original Message-----
From: Wu, Hao A
Sent: Friday, October 26, 2018 4:15 PM
To: Wu, Hao A <hao.a.wu@intel.com>; Zeng, Star <star.zeng@intel.com>; 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: [edk2] [PATCH V2 3/4] MdeModulePkg XhciDxe: Use common buffer for AsyncInterruptTransfer
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Wu, Hao A
> Sent: Friday, October 26, 2018 3:38 PM
> To: Zeng, Star; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu; Yao, Jiewen; Zeng, Star
> Subject: Re: [edk2] [PATCH V2 3/4] MdeModulePkg XhciDxe: Use common
> buffer for AsyncInterruptTransfer
>
> Reviewed-by: Hao Wu <hao.a.wu@intel.com>
Sorry, missed one question for this patch.
Please see below for more detail:
>
> 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 3/4] MdeModulePkg XhciDxe: Use common
> buffer
> > for AsyncInterruptTransfer
> >
> > 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>
> > Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
> > ---
> > MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c | 1 +
> > MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 141
> > +++++++++++-----------
> --
> > -------
> > MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.h | 27 +++---
> > 3 files changed, 67 insertions(+), 102 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 75959ae08363..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
> > + );
Does this "Xhc->PciIo->FreeBuffer()" call here need Urb->AllocateCommonBuffer check?
Best Regards,
Hao Wu
> > 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,10 +295,14 @@ XhcCreateTransferTrb (
> > // No need to remap.
> > //
> > if ((Urb->Data != NULL) && (Urb->DataMap == NULL)) {
> > - if (((UINT8) (Urb->Ep.Direction)) == EfiUsbDataIn) {
> > - MapOp = EfiPciIoOperationBusMasterWrite;
> > + if (Urb->AllocateCommonBuffer) {
> > + MapOp = EfiPciIoOperationBusMasterCommonBuffer;
> > } else {
> > - MapOp = EfiPciIoOperationBusMasterRead;
> > + if (((UINT8) (Urb->Ep.Direction)) == EfiUsbDataIn) {
> > + MapOp = EfiPciIoOperationBusMasterWrite;
> > + } else {
> > + MapOp = EfiPciIoOperationBusMasterRead;
> > + }
> > }
> >
> > Len = Urb->DataLen;
> > @@ -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,14 +1481,14 @@ XhciInsertAsyncIntTransfer (
> > MaxPacket,
> > XHC_INT_TRANSFER_ASYNC,
> > NULL,
> > - Data,
> > + TRUE,
> > + NULL,
> > DataLen,
> > Callback,
> > Context
> > );
> > if (Urb == NULL) {
> > DEBUG ((DEBUG_ERROR, "%a: failed to create URB\n",
> __FUNCTION__));
> > - FreePool (Data);
> > return NULL;
> > }
> >
> > @@ -1503,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.
> > @@ -1577,7 +1548,6 @@ XhcMonitorAsyncRequests (
> > UINT8 *ProcBuf;
> > URB *Urb;
> > UINT8 SlotId;
> > - EFI_STATUS Status;
> > EFI_TPL OldTpl;
> >
> > OldTpl = gBS->RaiseTPL (XHC_TPL); @@ -1606,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,
> > --
> > 2.7.0.windows.1
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
next prev parent reply other threads:[~2018-10-26 13:43 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 [this message]
2018-10-26 5:42 ` [PATCH V2 4/4] MdeModulePkg EhciDxe: " Star Zeng
2018-10-26 8:12 ` Wu, Hao A
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=0C09AFA07DD0434D9E2A0C6AEB0483103BC5BF16@shsmsx102.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