Patchwork [1/2] dma: axi-dmac: don't check the number of frames for alignment

login
register
mail settings
Submitter Alexandru Ardelean
Date Feb. 15, 2019, 9:17 a.m.
Message ID <20190215091750.28035-1-alexandru.ardelean@analog.com>
Download mbox | patch
Permalink /patch/727297/
State New
Headers show

Comments

Alexandru Ardelean - Feb. 15, 2019, 9:17 a.m.
Fixes commit 0e3b67b348b8 ("dmaengine: Add support for the Analog Devices
AXI-DMAC DMA controller")

For 2D transfers, there is no requirement for Y_LENGTH to be aligned
to the bus-width (or anything). X_LENGTH is required to be aligned
though.

So, we shouldn't check that the number of frames is aligned.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/dma/dma-axi-dmac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Vinod Koul - Feb. 25, 2019, 6:52 a.m.
On 15-02-19, 11:17, Alexandru Ardelean wrote:
> Fixes commit 0e3b67b348b8 ("dmaengine: Add support for the Analog Devices
> AXI-DMAC DMA controller")

Do you mean to add a Fixes tag?

> For 2D transfers, there is no requirement for Y_LENGTH to be aligned
> to the bus-width (or anything). X_LENGTH is required to be aligned
> though.
> 
> So, we shouldn't check that the number of frames is aligned.

Does this fix a bug as indicated by Fixes tag?

Lastly, it is dmaengine: xxx not dma: xxx Please fix that.

> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  drivers/dma/dma-axi-dmac.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/dma-axi-dmac.c b/drivers/dma/dma-axi-dmac.c
> index ffc0adc2f6ce..2c999113b989 100644
> --- a/drivers/dma/dma-axi-dmac.c
> +++ b/drivers/dma/dma-axi-dmac.c
> @@ -485,7 +485,7 @@ static struct dma_async_tx_descriptor *axi_dmac_prep_interleaved(
>  
>  	if (chan->hw_2d) {
>  		if (!axi_dmac_check_len(chan, xt->sgl[0].size) ||
> -		    !axi_dmac_check_len(chan, xt->numf))
> +		    xt->numf == 0)
>  			return NULL;
>  		if (xt->sgl[0].size + dst_icg > chan->max_length ||
>  		    xt->sgl[0].size + src_icg > chan->max_length)
> -- 
> 2.17.1
Ardelean, Alexandru - Feb. 26, 2019, 7:14 a.m.
On Mon, 2019-02-25 at 12:22 +0530, Vinod Koul wrote:
> [External]

> 

> 

> On 15-02-19, 11:17, Alexandru Ardelean wrote:

> > Fixes commit 0e3b67b348b8 ("dmaengine: Add support for the Analog

> > Devices

> > AXI-DMAC DMA controller")

> 

> Do you mean to add a Fixes tag?


Yep.
I'm terrible at this apparently. I'll read about how to do that properly.
Other maintainers have complained about this as well [i.e. the fact that I
didn't add proper Fixes tags ].

> 

> > For 2D transfers, there is no requirement for Y_LENGTH to be aligned

> > to the bus-width (or anything). X_LENGTH is required to be aligned

> > though.

> > 

> > So, we shouldn't check that the number of frames is aligned.

> 

> Does this fix a bug as indicated by Fixes tag?


Yes

> 

> Lastly, it is dmaengine: xxx not dma: xxx Please fix that.


Ack
Will fix

> 

> > 

> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

> > ---

> >  drivers/dma/dma-axi-dmac.c | 2 +-

> >  1 file changed, 1 insertion(+), 1 deletion(-)

> > 

> > diff --git a/drivers/dma/dma-axi-dmac.c b/drivers/dma/dma-axi-dmac.c

> > index ffc0adc2f6ce..2c999113b989 100644

> > --- a/drivers/dma/dma-axi-dmac.c

> > +++ b/drivers/dma/dma-axi-dmac.c

> > @@ -485,7 +485,7 @@ static struct dma_async_tx_descriptor

> > *axi_dmac_prep_interleaved(

> > 

> >       if (chan->hw_2d) {

> >               if (!axi_dmac_check_len(chan, xt->sgl[0].size) ||

> > -                 !axi_dmac_check_len(chan, xt->numf))

> > +                 xt->numf == 0)

> >                       return NULL;

> >               if (xt->sgl[0].size + dst_icg > chan->max_length ||

> >                   xt->sgl[0].size + src_icg > chan->max_length)

> > --

> > 2.17.1

> 

> --

> ~Vinod
Vinod Koul - Feb. 26, 2019, 8:01 a.m.
On 26-02-19, 07:14, Ardelean, Alexandru wrote:
> On Mon, 2019-02-25 at 12:22 +0530, Vinod Koul wrote:
> > [External]
> > 
> > 
> > On 15-02-19, 11:17, Alexandru Ardelean wrote:
> > > Fixes commit 0e3b67b348b8 ("dmaengine: Add support for the Analog
> > > Devices
> > > AXI-DMAC DMA controller")
> > 
> > Do you mean to add a Fixes tag?
> 
> Yep.
> I'm terrible at this apparently. I'll read about how to do that properly.
> Other maintainers have complained about this as well [i.e. the fact that I
> didn't add proper Fixes tags ].

So if it fixes a bug in original commit and should go immediately to
next rc-X then fixes is mostly appropriate.. (also backporting to
stable). In this case it doesnt seem so.

Also canonical form is

Fixes: abcdef : ("buggy patch")

and this line should be before the s-o-b line..

> > 
> > > For 2D transfers, there is no requirement for Y_LENGTH to be aligned
> > > to the bus-width (or anything). X_LENGTH is required to be aligned
> > > though.
> > > 
> > > So, we shouldn't check that the number of frames is aligned.
> > 
> > Does this fix a bug as indicated by Fixes tag?
> 
> Yes

well if it is aligned it shouldn't cause break right. Yes not having
aligned helps the driver but seems to be correct the wrong
interpretation/implementation.. right?
Ardelean, Alexandru - Feb. 26, 2019, 8:36 a.m.
On Tue, 2019-02-26 at 13:31 +0530, Vinod Koul wrote:
> [External]

> 

> 

> On 26-02-19, 07:14, Ardelean, Alexandru wrote:

> > On Mon, 2019-02-25 at 12:22 +0530, Vinod Koul wrote:

> > > [External]

> > > 

> > > 

> > > On 15-02-19, 11:17, Alexandru Ardelean wrote:

> > > > Fixes commit 0e3b67b348b8 ("dmaengine: Add support for the Analog

> > > > Devices

> > > > AXI-DMAC DMA controller")

> > > 

> > > Do you mean to add a Fixes tag?

> > 

> > Yep.

> > I'm terrible at this apparently. I'll read about how to do that

> > properly.

> > Other maintainers have complained about this as well [i.e. the fact

> > that I

> > didn't add proper Fixes tags ].

> 

> So if it fixes a bug in original commit and should go immediately to

> next rc-X then fixes is mostly appropriate.. (also backporting to

> stable). In this case it doesnt seem so.

> 

> Also canonical form is

> 

> Fixes: abcdef : ("buggy patch")

> 

> and this line should be before the s-o-b line..


I just found this in the submitting-patch-docs.

> 

> > > 

> > > > For 2D transfers, there is no requirement for Y_LENGTH to be

> > > > aligned

> > > > to the bus-width (or anything). X_LENGTH is required to be aligned

> > > > though.

> > > > 

> > > > So, we shouldn't check that the number of frames is aligned.

> > > 

> > > Does this fix a bug as indicated by Fixes tag?

> > 

> > Yes

> 

> well if it is aligned it shouldn't cause break right. Yes not having

> aligned helps the driver but seems to be correct the wrong

> interpretation/implementation.. right?


I'm preparing a V2 here.
2D transfers are typically used when DMA talks to video, where Y_LENGTH is
the number of rows, which don't need to be aligned.
I'll try to make the comment more helpful.

> 

> --

> ~Vinod

Patch

diff --git a/drivers/dma/dma-axi-dmac.c b/drivers/dma/dma-axi-dmac.c
index ffc0adc2f6ce..2c999113b989 100644
--- a/drivers/dma/dma-axi-dmac.c
+++ b/drivers/dma/dma-axi-dmac.c
@@ -485,7 +485,7 @@  static struct dma_async_tx_descriptor *axi_dmac_prep_interleaved(
 
 	if (chan->hw_2d) {
 		if (!axi_dmac_check_len(chan, xt->sgl[0].size) ||
-		    !axi_dmac_check_len(chan, xt->numf))
+		    xt->numf == 0)
 			return NULL;
 		if (xt->sgl[0].size + dst_icg > chan->max_length ||
 		    xt->sgl[0].size + src_icg > chan->max_length)