[RFC v2] usb: xhci: add Immediate Data Transfer support

[RFC v2] usb: xhci: add Immediate Data Transfer support

[Nicolas Saenz Julienne]

Immediate data transfers (IDT) allow the HCD to copy small chunks of
data (up to 8bytes) directly into its output transfer TRBs. This avoids
the somewhat expensive DMA mappings that are performed by default on
most URBs submissions.

In the case an URB was suitable for IDT. The data is directly copied
into the "Data Buffer Pointer" region of the TRB and the IDT flag is
set. Instead of triggering memory accesses the HC will use the data
directly.

The implementation covers all kind of output endpoints. All have been
tested successfully with multiple devices except for isochronous ones. I
can't seem to find a device that'll perform transfers with such small
packet sizes.

The implementation takes into account that the 8 byte buffers provided
by the URB will never cross a 64KB boundary.

Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
---
Chages since first RFC:
  - Got rid of URB flag
  - Implement feature for Control transfers
  - Implement feature for Isoc transfers

 drivers/usb/host/xhci-ring.c | 20 ++++++++++++++++++++
 drivers/usb/host/xhci.c      | 16 ++++++++++++++++
 drivers/usb/host/xhci.h      | 12 ++++++++++++
 3 files changed, 48 insertions(+)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 40fa25c4d041..a4efbe62a1a3 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3272,8 +3272,15 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
 			field |= TRB_IOC;
 			more_trbs_coming = false;
 			td->last_trb = ring->enqueue;
+
+			if (xhci_urb_suitable_for_idt(urb)) {
+				memcpy(&send_addr, urb->transfer_buffer,
+				       trb_buff_len);
+				field |= TRB_IDT;
+			}
 		}
 
+
 		/* Only set interrupt on short packet for IN endpoints */
 		if (usb_urb_dir_in(urb))
 			field |= TRB_ISP;
@@ -3411,6 +3418,12 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
 	if (urb->transfer_buffer_length > 0) {
 		u32 length_field, remainder;
 
+		if (xhci_urb_suitable_for_idt(urb)) {
+			memcpy(&urb->transfer_dma, urb->transfer_buffer,
+			       urb->transfer_buffer_length);
+			field |= TRB_IDT;
+		}
+
 		remainder = xhci_td_remainder(xhci, 0,
 				urb->transfer_buffer_length,
 				urb->transfer_buffer_length,
@@ -3420,6 +3433,7 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
 				TRB_INTR_TARGET(0);
 		if (setup->bRequestType & USB_DIR_IN)
 			field |= TRB_DIR_IN;
+
 		queue_trb(xhci, ep_ring, true,
 				lower_32_bits(urb->transfer_dma),
 				upper_32_bits(urb->transfer_dma),
@@ -3710,6 +3724,12 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
 			if (trb_buff_len > td_remain_len)
 				trb_buff_len = td_remain_len;
 
+			if (xhci_urb_suitable_for_idt(urb)) {
+				memcpy(&addr, urb->transfer_buffer,
+				       trb_buff_len);
+				field |= TRB_IDT;
+			}
+
 			/* Set the TRB length, TD size, & interrupter fields. */
 			remainder = xhci_td_remainder(xhci, running_total,
 						   trb_buff_len, td_len,
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 005e65922608..dec62f7f5dc8 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1238,6 +1238,21 @@ EXPORT_SYMBOL_GPL(xhci_resume);
 
 /*-------------------------------------------------------------------------*/
 
+/*
+ * Bypass the DMA mapping if URB is suitable for Immediate Transfer (IDT),
+ * we'll copy the actual data into the TRB address register. This is limited to
+ * transfers up to 8 bytes on output endpoints of any kind with wMaxPacketSize
+ * >= 8 bytes.
+ */
+static int xhci_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
+				gfp_t mem_flags)
+{
+	if (xhci_urb_suitable_for_idt(urb))
+		return 0;
+
+	return usb_hcd_map_urb_for_dma(hcd, urb, mem_flags);
+}
+
 /**
  * xhci_get_endpoint_index - Used for passing endpoint bitmasks between the core and
  * HCDs.  Find the index for an endpoint given its descriptor.  Use the return
@@ -5155,6 +5170,7 @@ static const struct hc_driver xhci_hc_driver = {
 	/*
 	 * managing i/o requests and associated device resources
 	 */
+	.map_urb_for_dma =      xhci_map_urb_for_dma,
 	.urb_enqueue =		xhci_urb_enqueue,
 	.urb_dequeue =		xhci_urb_dequeue,
 	.alloc_dev =		xhci_alloc_dev,
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 652dc36e3012..9d77b0901ab7 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1295,6 +1295,8 @@ enum xhci_setup_dev {
 #define TRB_IOC			(1<<5)
 /* The buffer pointer contains immediate data */
 #define TRB_IDT			(1<<6)
+/* TDs smaller than this might use IDT */
+#define TRB_IDT_MAX_SIZE	8
 
 /* Block Event Interrupt */
 #define	TRB_BEI			(1<<9)
@@ -2141,6 +2143,16 @@ static inline struct xhci_ring *xhci_urb_to_transfer_ring(struct xhci_hcd *xhci,
 					urb->stream_id);
 }
 
+static inline bool xhci_urb_suitable_for_idt(struct urb *urb)
+{
+	if (usb_endpoint_maxp(&urb->ep->desc) >= TRB_IDT_MAX_SIZE &&
+	    urb->transfer_buffer_length <= TRB_IDT_MAX_SIZE &&
+	    usb_urb_dir_out(urb))
+		return true;
+
+	return false;
+}
+
 static inline char *xhci_slot_state_string(u32 state)
 {
 	switch (state) {
-- 
2.20.1

Re: [RFC v2] usb: xhci: add Immediate Data Transfer support

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2696 bytes --]


Hi,

Nicolas Saenz Julienne <nsaenzjulienne@suse.de> writes:
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 40fa25c4d041..a4efbe62a1a3 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -3272,8 +3272,15 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
>  			field |= TRB_IOC;
>  			more_trbs_coming = false;
>  			td->last_trb = ring->enqueue;
> +
> +			if (xhci_urb_suitable_for_idt(urb)) {
> +				memcpy(&send_addr, urb->transfer_buffer,
> +				       trb_buff_len);
> +				field |= TRB_IDT;
> +			}
>  		}
>  
> +

trailing change

> @@ -3411,6 +3418,12 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
>  	if (urb->transfer_buffer_length > 0) {
>  		u32 length_field, remainder;
>  
> +		if (xhci_urb_suitable_for_idt(urb)) {
> +			memcpy(&urb->transfer_dma, urb->transfer_buffer,
> +			       urb->transfer_buffer_length);
> +			field |= TRB_IDT;
> +		}
> +
>  		remainder = xhci_td_remainder(xhci, 0,
>  				urb->transfer_buffer_length,
>  				urb->transfer_buffer_length,
> @@ -3420,6 +3433,7 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
>  				TRB_INTR_TARGET(0);
>  		if (setup->bRequestType & USB_DIR_IN)
>  			field |= TRB_DIR_IN;
> +

trailing change

> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 005e65922608..dec62f7f5dc8 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -1238,6 +1238,21 @@ EXPORT_SYMBOL_GPL(xhci_resume);
>  
>  /*-------------------------------------------------------------------------*/
>  
> +/*
> + * Bypass the DMA mapping if URB is suitable for Immediate Transfer (IDT),
> + * we'll copy the actual data into the TRB address register. This is limited to
> + * transfers up to 8 bytes on output endpoints of any kind with wMaxPacketSize
> + * >= 8 bytes.
> + */
> +static int xhci_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
> +				gfp_t mem_flags)
> +{
> +	if (xhci_urb_suitable_for_idt(urb))
> +		return 0;
> +
> +	return usb_hcd_map_urb_for_dma(hcd, urb, mem_flags);
> +}

don't you need a matching unmap_urb_for_dma()??

> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 652dc36e3012..9d77b0901ab7 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1295,6 +1295,8 @@ enum xhci_setup_dev {
>  #define TRB_IOC			(1<<5)
>  /* The buffer pointer contains immediate data */
>  #define TRB_IDT			(1<<6)
> +/* TDs smaller than this might use IDT */

Technically, "TDs at most this" since you're 8 itself is an allowed
size.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [RFC v2] usb: xhci: add Immediate Data Transfer support

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 3498 bytes --]

Hi Felipe, thanks for the review!

On Wed, 2019-02-06 at 08:35 +0200, Felipe Balbi wrote:
> Hi,
> 
> Nicolas Saenz Julienne <nsaenzjulienne@suse.de> writes:
> > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> > index 40fa25c4d041..a4efbe62a1a3 100644
> > --- a/drivers/usb/host/xhci-ring.c
> > +++ b/drivers/usb/host/xhci-ring.c
> > @@ -3272,8 +3272,15 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t
> > mem_flags,
> >  			field |= TRB_IOC;
> >  			more_trbs_coming = false;
> >  			td->last_trb = ring->enqueue;
> > +
> > +			if (xhci_urb_suitable_for_idt(urb)) {
> > +				memcpy(&send_addr, urb->transfer_buffer,
> > +				       trb_buff_len);
> > +				field |= TRB_IDT;
> > +			}
> >  		}
> >  
> > +
> 
> trailing change

Noted

> 
> > @@ -3411,6 +3418,12 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t
> > mem_flags,
> >  	if (urb->transfer_buffer_length > 0) {
> >  		u32 length_field, remainder;
> >  
> > +		if (xhci_urb_suitable_for_idt(urb)) {
> > +			memcpy(&urb->transfer_dma, urb->transfer_buffer,
> > +			       urb->transfer_buffer_length);
> > +			field |= TRB_IDT;
> > +		}
> > +
> >  		remainder = xhci_td_remainder(xhci, 0,
> >  				urb->transfer_buffer_length,
> >  				urb->transfer_buffer_length,
> > @@ -3420,6 +3433,7 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t
> > mem_flags,
> >  				TRB_INTR_TARGET(0);
> >  		if (setup->bRequestType & USB_DIR_IN)
> >  			field |= TRB_DIR_IN;
> > +
> 
> trailing change

Noted

> 
> > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > index 005e65922608..dec62f7f5dc8 100644
> > --- a/drivers/usb/host/xhci.c
> > +++ b/drivers/usb/host/xhci.c
> > @@ -1238,6 +1238,21 @@ EXPORT_SYMBOL_GPL(xhci_resume);
> >  
> >  /*-------------------------------------------------------------------------
> > */
> >  
> > +/*
> > + * Bypass the DMA mapping if URB is suitable for Immediate Transfer (IDT),
> > + * we'll copy the actual data into the TRB address register. This is
> > limited to
> > + * transfers up to 8 bytes on output endpoints of any kind with
> > wMaxPacketSize
> > + * >= 8 bytes.
> > + */
> > +static int xhci_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
> > +				gfp_t mem_flags)
> > +{
> > +	if (xhci_urb_suitable_for_idt(urb))
> > +		return 0;
> > +
> > +	return usb_hcd_map_urb_for_dma(hcd, urb, mem_flags);
> > +}
> 
> don't you need a matching unmap_urb_for_dma()??

Not really as every DMA mapping sets a matching URB flag to track it. For
example when usb_hcd_map_urb_for_dma() uses dma_map_single() it will set
URB_DMA_MAP_SINGLE in urb->transfer_flags, later on unmap_urb_for_dma() will
catch it and unmap it. As I bypass the mapping altogether there are no
flags set, so unmap_urb_for_dma() won't have any effect.

I could still add it for clarity, and well, I guess it'll save some
instructions on the IDT suitable side.

> 
> > diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> > index 652dc36e3012..9d77b0901ab7 100644
> > --- a/drivers/usb/host/xhci.h
> > +++ b/drivers/usb/host/xhci.h
> > @@ -1295,6 +1295,8 @@ enum xhci_setup_dev {
> >  #define TRB_IOC			(1<<5)
> >  /* The buffer pointer contains immediate data */
> >  #define TRB_IDT			(1<<6)
> > +/* TDs smaller than this might use IDT */
> 
> Technically, "TDs at most this" since you're 8 itself is an allowed
> size.
> 

Noted

Regards,
Nicolas



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC v2] usb: xhci: add Immediate Data Transfer support

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2148 bytes --]


Hi,

Nicolas Saenz Julienne <nsaenzjulienne@suse.de> writes:
>> > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> > index 005e65922608..dec62f7f5dc8 100644
>> > --- a/drivers/usb/host/xhci.c
>> > +++ b/drivers/usb/host/xhci.c
>> > @@ -1238,6 +1238,21 @@ EXPORT_SYMBOL_GPL(xhci_resume);
>> >  
>> >  /*-------------------------------------------------------------------------
>> > */
>> >  
>> > +/*
>> > + * Bypass the DMA mapping if URB is suitable for Immediate Transfer (IDT),
>> > + * we'll copy the actual data into the TRB address register. This is
>> > limited to
>> > + * transfers up to 8 bytes on output endpoints of any kind with
>> > wMaxPacketSize
>> > + * >= 8 bytes.
>> > + */
>> > +static int xhci_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
>> > +				gfp_t mem_flags)
>> > +{
>> > +	if (xhci_urb_suitable_for_idt(urb))
>> > +		return 0;
>> > +
>> > +	return usb_hcd_map_urb_for_dma(hcd, urb, mem_flags);
>> > +}
>> 
>> don't you need a matching unmap_urb_for_dma()??
>
> Not really as every DMA mapping sets a matching URB flag to track it. For
> example when usb_hcd_map_urb_for_dma() uses dma_map_single() it will set
> URB_DMA_MAP_SINGLE in urb->transfer_flags, later on unmap_urb_for_dma() will
> catch it and unmap it. As I bypass the mapping altogether there are no
> flags set, so unmap_urb_for_dma() won't have any effect.
>
> I could still add it for clarity, and well, I guess it'll save some
> instructions on the IDT suitable side.

thanks for the clarification.

>> > diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>> > index 652dc36e3012..9d77b0901ab7 100644
>> > --- a/drivers/usb/host/xhci.h
>> > +++ b/drivers/usb/host/xhci.h
>> > @@ -1295,6 +1295,8 @@ enum xhci_setup_dev {
>> >  #define TRB_IOC			(1<<5)
>> >  /* The buffer pointer contains immediate data */
>> >  #define TRB_IDT			(1<<6)
>> > +/* TDs smaller than this might use IDT */
>> 
>> Technically, "TDs at most this" since you're 8 itself is an allowed

heh, I made a mess on this sentence, but I guess you got the gist of it.

cheers

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]