[PATCH] dmaengine: cookie bypass for out of order completion

[PATCH] dmaengine: cookie bypass for out of order completion

[Dave Jiang]

The cookie tracking in dmaengine expects all submissions completed in
order. Some DMA devices like Intel DSA can complete submissions out of
order, especially if configured with a work queue sharing multiple DMA
engines. Add a status DMA_OUT_OF_ORDER that tx_status can be returned for
those DMA devices. The user should use callbacks to track the completion
rather than the DMA cookie. This would address the issue of dmatest
complaining that descriptors are "busy" when the cookie count goes
backwards due to out of order completion.

Reported-by: Swathi Kovvuri <swathi.kovvuri@intel.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Tested-by: Swathi Kovvuri <swathi.kovvuri@intel.com>
---
 drivers/dma/dmatest.c     |    3 ++-
 drivers/dma/idxd/dma.c    |    2 +-
 include/linux/dmaengine.h |    1 +
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
index a2cadfa2e6d7..60a4a9cec3c8 100644
--- a/drivers/dma/dmatest.c
+++ b/drivers/dma/dmatest.c
@@ -821,7 +821,8 @@ static int dmatest_func(void *data)
 			result("test timed out", total_tests, src->off, dst->off,
 			       len, 0);
 			goto error_unmap_continue;
-		} else if (status != DMA_COMPLETE) {
+		} else if (status != DMA_COMPLETE &&
+			   status != DMA_OUT_OF_ORDER) {
 			result(status == DMA_ERROR ?
 			       "completion error status" :
 			       "completion busy status", total_tests, src->off,
diff --git a/drivers/dma/idxd/dma.c b/drivers/dma/idxd/dma.c
index c64c1429d160..3f54826abc12 100644
--- a/drivers/dma/idxd/dma.c
+++ b/drivers/dma/idxd/dma.c
@@ -133,7 +133,7 @@ static enum dma_status idxd_dma_tx_status(struct dma_chan *dma_chan,
 					  dma_cookie_t cookie,
 					  struct dma_tx_state *txstate)
 {
-	return dma_cookie_status(dma_chan, cookie, txstate);
+	return DMA_OUT_OF_ORDER;
 }
 
 /*
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 21065c04c4ac..a0c130131e45 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -39,6 +39,7 @@ enum dma_status {
 	DMA_IN_PROGRESS,
 	DMA_PAUSED,
 	DMA_ERROR,
+	DMA_OUT_OF_ORDER,
 };
 
 /**

Re: [PATCH] dmaengine: cookie bypass for out of order completion

[Vinod Koul]

Hi Dave,

On 30-04-20, 11:35, Dave Jiang wrote:
> The cookie tracking in dmaengine expects all submissions completed in

Correct and that is a *very* fundamental assumption of the cookie
management. Modifying this will cause impact to other as well..

> order. Some DMA devices like Intel DSA can complete submissions out of
> order, especially if configured with a work queue sharing multiple DMA
> engines. Add a status DMA_OUT_OF_ORDER that tx_status can be returned for

We should add this as a capability in dmaengine. How else would users
know if they can expect out of order completion..

> those DMA devices. The user should use callbacks to track the completion
> rather than the DMA cookie. This would address the issue of dmatest
> complaining that descriptors are "busy" when the cookie count goes
> backwards due to out of order completion.

Can we add some documentation for this behaviour as well

> 
> Reported-by: Swathi Kovvuri <swathi.kovvuri@intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> Tested-by: Swathi Kovvuri <swathi.kovvuri@intel.com>
> ---
>  drivers/dma/dmatest.c     |    3 ++-
>  drivers/dma/idxd/dma.c    |    2 +-
>  include/linux/dmaengine.h |    1 +
>  3 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
> index a2cadfa2e6d7..60a4a9cec3c8 100644
> --- a/drivers/dma/dmatest.c
> +++ b/drivers/dma/dmatest.c
> @@ -821,7 +821,8 @@ static int dmatest_func(void *data)
>  			result("test timed out", total_tests, src->off, dst->off,
>  			       len, 0);
>  			goto error_unmap_continue;
> -		} else if (status != DMA_COMPLETE) {
> +		} else if (status != DMA_COMPLETE &&
> +			   status != DMA_OUT_OF_ORDER) {
>  			result(status == DMA_ERROR ?
>  			       "completion error status" :
>  			       "completion busy status", total_tests, src->off,
> diff --git a/drivers/dma/idxd/dma.c b/drivers/dma/idxd/dma.c
> index c64c1429d160..3f54826abc12 100644
> --- a/drivers/dma/idxd/dma.c
> +++ b/drivers/dma/idxd/dma.c
> @@ -133,7 +133,7 @@ static enum dma_status idxd_dma_tx_status(struct dma_chan *dma_chan,
>  					  dma_cookie_t cookie,
>  					  struct dma_tx_state *txstate)
>  {
> -	return dma_cookie_status(dma_chan, cookie, txstate);
> +	return DMA_OUT_OF_ORDER;

So you are returning out of order always?

>  }
>  
>  /*
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index 21065c04c4ac..a0c130131e45 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -39,6 +39,7 @@ enum dma_status {
>  	DMA_IN_PROGRESS,
>  	DMA_PAUSED,
>  	DMA_ERROR,
> +	DMA_OUT_OF_ORDER,
>  };
>  
>  /**

-- 
~Vinod

Re: [PATCH] dmaengine: cookie bypass for out of order completion

[Dave Jiang]

On 5/3/2020 10:39 PM, Vinod Koul wrote:
> Hi Dave,
> 
> On 30-04-20, 11:35, Dave Jiang wrote:
>> The cookie tracking in dmaengine expects all submissions completed in
> 
> Correct and that is a *very* fundamental assumption of the cookie
> management. Modifying this will cause impact to other as well..

The current modification only impacts drivers that opt out of this. So 
all existing or future driver does not return the out of order flag 
should remain unimpacted.

> 
>> order. Some DMA devices like Intel DSA can complete submissions out of
>> order, especially if configured with a work queue sharing multiple DMA
>> engines. Add a status DMA_OUT_OF_ORDER that tx_status can be returned for
> 
> We should add this as a capability in dmaengine. How else would users
> know if they can expect out of order completion..

Hmmm...this is more an attribute of the hardware rather than a 
capability that a user would request right? Should we add a new function 
that would provide an avenue for users to query the device on such 
attributes and others like channel depth or SGL max size?

> 
>> those DMA devices. The user should use callbacks to track the completion
>> rather than the DMA cookie. This would address the issue of dmatest
>> complaining that descriptors are "busy" when the cookie count goes
>> backwards due to out of order completion.
> 
> Can we add some documentation for this behaviour as well

sure thing.

> 
>>
>> Reported-by: Swathi Kovvuri <swathi.kovvuri@intel.com>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> Tested-by: Swathi Kovvuri <swathi.kovvuri@intel.com>
>> ---
>>   drivers/dma/dmatest.c     |    3 ++-
>>   drivers/dma/idxd/dma.c    |    2 +-
>>   include/linux/dmaengine.h |    1 +
>>   3 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
>> index a2cadfa2e6d7..60a4a9cec3c8 100644
>> --- a/drivers/dma/dmatest.c
>> +++ b/drivers/dma/dmatest.c
>> @@ -821,7 +821,8 @@ static int dmatest_func(void *data)
>>   			result("test timed out", total_tests, src->off, dst->off,
>>   			       len, 0);
>>   			goto error_unmap_continue;
>> -		} else if (status != DMA_COMPLETE) {
>> +		} else if (status != DMA_COMPLETE &&
>> +			   status != DMA_OUT_OF_ORDER) {
>>   			result(status == DMA_ERROR ?
>>   			       "completion error status" :
>>   			       "completion busy status", total_tests, src->off,
>> diff --git a/drivers/dma/idxd/dma.c b/drivers/dma/idxd/dma.c
>> index c64c1429d160..3f54826abc12 100644
>> --- a/drivers/dma/idxd/dma.c
>> +++ b/drivers/dma/idxd/dma.c
>> @@ -133,7 +133,7 @@ static enum dma_status idxd_dma_tx_status(struct dma_chan *dma_chan,
>>   					  dma_cookie_t cookie,
>>   					  struct dma_tx_state *txstate)
>>   {
>> -	return dma_cookie_status(dma_chan, cookie, txstate);
>> +	return DMA_OUT_OF_ORDER;
> 
> So you are returning out of order always?

Yes. The hardware does not gaurantee in order processing at all. The 
only way to do so is to submit a batched operation with fence set on 
every descriptor in the batch.

> 
>>   }
>>   
>>   /*
>> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
>> index 21065c04c4ac..a0c130131e45 100644
>> --- a/include/linux/dmaengine.h
>> +++ b/include/linux/dmaengine.h
>> @@ -39,6 +39,7 @@ enum dma_status {
>>   	DMA_IN_PROGRESS,
>>   	DMA_PAUSED,
>>   	DMA_ERROR,
>> +	DMA_OUT_OF_ORDER,
>>   };
>>   
>>   /**
> 

Re: [PATCH] dmaengine: cookie bypass for out of order completion

[Vinod Koul]

Hi Dave,

On 04-05-20, 07:59, Dave Jiang wrote:
> On 5/3/2020 10:39 PM, Vinod Koul wrote:
> > On 30-04-20, 11:35, Dave Jiang wrote:
> > > The cookie tracking in dmaengine expects all submissions completed in
> > 
> > Correct and that is a *very* fundamental assumption of the cookie
> > management. Modifying this will cause impact to other as well..
> 
> The current modification only impacts drivers that opt out of this. So all
> existing or future driver does not return the out of order flag should
> remain unimpacted.
> 
> > 
> > > order. Some DMA devices like Intel DSA can complete submissions out of
> > > order, especially if configured with a work queue sharing multiple DMA
> > > engines. Add a status DMA_OUT_OF_ORDER that tx_status can be returned for
> > 
> > We should add this as a capability in dmaengine. How else would users
> > know if they can expect out of order completion..
> 
> Hmmm...this is more an attribute of the hardware rather than a capability
> that a user would request right? Should we add a new function that would
> provide an avenue for users to query the device on such attributes and
> others like channel depth or SGL max size?

Well we have capabilities of hw reported so that users knows. This
should also be a capability reported

> > > those DMA devices. The user should use callbacks to track the completion
> > > rather than the DMA cookie. This would address the issue of dmatest
> > > complaining that descriptors are "busy" when the cookie count goes
> > > backwards due to out of order completion.
> > 
> > Can we add some documentation for this behaviour as well
> 
> sure thing.
> 
> > 
> > > 
> > > Reported-by: Swathi Kovvuri <swathi.kovvuri@intel.com>
> > > Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> > > Tested-by: Swathi Kovvuri <swathi.kovvuri@intel.com>
> > > ---
> > >   drivers/dma/dmatest.c     |    3 ++-
> > >   drivers/dma/idxd/dma.c    |    2 +-
> > >   include/linux/dmaengine.h |    1 +
> > >   3 files changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
> > > index a2cadfa2e6d7..60a4a9cec3c8 100644
> > > --- a/drivers/dma/dmatest.c
> > > +++ b/drivers/dma/dmatest.c
> > > @@ -821,7 +821,8 @@ static int dmatest_func(void *data)
> > >   			result("test timed out", total_tests, src->off, dst->off,
> > >   			       len, 0);
> > >   			goto error_unmap_continue;
> > > -		} else if (status != DMA_COMPLETE) {
> > > +		} else if (status != DMA_COMPLETE &&
> > > +			   status != DMA_OUT_OF_ORDER) {
> > >   			result(status == DMA_ERROR ?
> > >   			       "completion error status" :
> > >   			       "completion busy status", total_tests, src->off,
> > > diff --git a/drivers/dma/idxd/dma.c b/drivers/dma/idxd/dma.c
> > > index c64c1429d160..3f54826abc12 100644
> > > --- a/drivers/dma/idxd/dma.c
> > > +++ b/drivers/dma/idxd/dma.c
> > > @@ -133,7 +133,7 @@ static enum dma_status idxd_dma_tx_status(struct dma_chan *dma_chan,
> > >   					  dma_cookie_t cookie,
> > >   					  struct dma_tx_state *txstate)
> > >   {
> > > -	return dma_cookie_status(dma_chan, cookie, txstate);
> > > +	return DMA_OUT_OF_ORDER;
> > 
> > So you are returning out of order always?
> 
> Yes. The hardware does not gaurantee in order processing at all. The only
> way to do so is to submit a batched operation with fence set on every
> descriptor in the batch.
> 
> > 
> > >   }
> > >   /*
> > > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> > > index 21065c04c4ac..a0c130131e45 100644
> > > --- a/include/linux/dmaengine.h
> > > +++ b/include/linux/dmaengine.h
> > > @@ -39,6 +39,7 @@ enum dma_status {
> > >   	DMA_IN_PROGRESS,
> > >   	DMA_PAUSED,
> > >   	DMA_ERROR,
> > > +	DMA_OUT_OF_ORDER,
> > >   };
> > >   /**
> > 

-- 
~Vinod