[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 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/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 EpAddrEndpoint 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 EpAddrEndpoint 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 BOOLEANAllocateCommonBuffer, IN VOID *Data, IN UINTN DataLen, IN EFI_ASYNC_USB_TRANSFER_CALLBACKCallback, @@ -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), +
Re: [edk2] [PATCH V2 3/4] MdeModulePkg XhciDxe: Use common buffer for AsyncInterruptTransfer
Reviewed-by: Hao Wu Best Regards, Hao Wu > -Original Message- > From: edk2-devel [mailto:edk2-devel-boun...@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 > 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/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 EpAddrEndpoint 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 EpAddrEndpoint 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 BOOLEANAllocateCommonBuffer, >IN VOID *Data, >IN UINTN DataLen, >IN EFI_ASYNC_USB_TRANSFER_CALLBACKCallback, > @@ -169,8 +171,24 @@ XhcCreateUrb ( >Ep->Type = Type; > >Urb->Request
Re: [edk2] [PATCH V2 3/4] MdeModulePkg XhciDxe: Use common buffer for AsyncInterruptTransfer
> -Original Message- > From: edk2-devel [mailto:edk2-devel-boun...@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 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-boun...@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 > > 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/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 EpAddrEndpoint 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 EpAddrEndpoint 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 Alloca
Re: [edk2] [PATCH V2 3/4] MdeModulePkg XhciDxe: Use common buffer for AsyncInterruptTransfer
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 ; 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 > -Original Message- > From: edk2-devel [mailto:edk2-devel-boun...@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 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-boun...@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 > > 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/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 EpAddrEndpoint 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 > > +