[PATCH] dmaengine: imx-sdma: advertise all supported slave bus widths

[PATCH] dmaengine: imx-sdma: advertise all supported slave bus widths

[Lucas Stach]

The commit [1] which changed imx-pcm-dma to derive the pcm_config from the
attached dmaengine introduced a regression on the supported sample formats,
as imx-sdma doesn't properly advertise all the supported slave bus widths.

Fix this to restore full functionality of the imx sound subsystem.

[1] 70d435ba1cd6 ("ASoC: imx-pcm-dma: simplify pcm_config")
Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/dma/imx-sdma.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index a67ec1bdc4e0..e374183f19d0 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -1851,8 +1851,12 @@ static int sdma_probe(struct platform_device *pdev)
 	sdma->dma_device.device_prep_dma_cyclic = sdma_prep_dma_cyclic;
 	sdma->dma_device.device_config = sdma_config;
 	sdma->dma_device.device_terminate_all = sdma_disable_channel_with_delay;
-	sdma->dma_device.src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
-	sdma->dma_device.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
+	sdma->dma_device.src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
+					   BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
+					   BIT(DMA_SLAVE_BUSWIDTH_1_BYTE);
+	sdma->dma_device.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
+					   BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
+					   BIT(DMA_SLAVE_BUSWIDTH_1_BYTE);
 	sdma->dma_device.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
 	sdma->dma_device.residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
 	sdma->dma_device.device_issue_pending = sdma_issue_pending;
-- 
2.11.0

Re: [PATCH] dmaengine: imx-sdma: advertise all supported slave bus widths

[Lothar Waßmann]

Hi,

On Fri, 13 Oct 2017 12:43:46 +0200 Lucas Stach wrote:
> The commit [1] which changed imx-pcm-dma to derive the pcm_config from the
> attached dmaengine introduced a regression on the supported sample formats,
> as imx-sdma doesn't properly advertise all the supported slave bus widths.
> 
> Fix this to restore full functionality of the imx sound subsystem.
> 
> [1] 70d435ba1cd6 ("ASoC: imx-pcm-dma: simplify pcm_config")
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/dma/imx-sdma.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index a67ec1bdc4e0..e374183f19d0 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -1851,8 +1851,12 @@ static int sdma_probe(struct platform_device *pdev)
>  	sdma->dma_device.device_prep_dma_cyclic = sdma_prep_dma_cyclic;
>  	sdma->dma_device.device_config = sdma_config;
>  	sdma->dma_device.device_terminate_all = sdma_disable_channel_with_delay;
> -	sdma->dma_device.src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
> -	sdma->dma_device.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
> +	sdma->dma_device.src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
> +					   BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> +					   BIT(DMA_SLAVE_BUSWIDTH_1_BYTE);
> +	sdma->dma_device.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
> +					   BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> +					   BIT(DMA_SLAVE_BUSWIDTH_1_BYTE);
>  	sdma->dma_device.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
>  	sdma->dma_device.residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
>  	sdma->dma_device.device_issue_pending = sdma_issue_pending;
>
Thank you, this fixes the regression I observed.

Tested-by: Lothar Waßmann <LW@KARO-electronics.de>


Lothar Waßmann
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] dmaengine: imx-sdma: advertise all supported slave bus widths

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 466 bytes --]

On Fri, Oct 13, 2017 at 12:43:46PM +0200, Lucas Stach wrote:

> The commit [1] which changed imx-pcm-dma to derive the pcm_config from the
> attached dmaengine introduced a regression on the supported sample formats,
> as imx-sdma doesn't properly advertise all the supported slave bus widths.

How did this work previously - shouldn't we have failed to use the
unadvertised bus widths?  I'd expect the framework to be providing error
checking for the clients here.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] dmaengine: imx-sdma: advertise all supported slave bus widths

[Lucas Stach]

Am Freitag, den 13.10.2017, 12:25 +0100 schrieb Mark Brown:
> On Fri, Oct 13, 2017 at 12:43:46PM +0200, Lucas Stach wrote:
> 
> > The commit [1] which changed imx-pcm-dma to derive the pcm_config from the
> > attached dmaengine introduced a regression on the supported sample formats,
> > as imx-sdma doesn't properly advertise all the supported slave bus widths.
> 
> How did this work previously - shouldn't we have failed to use the
> unadvertised bus widths?  I'd expect the framework to be providing error
> checking for the clients here.

No, if the sound PCM implementation explicitly provides a pcm_config
with the hw.formats set to 0, the core only looks at the DAI supported
formats. Now that we derive the pcm_config from the dmaengine
hw.formats gets filled with formats supported by the dmaengine, which
is then used as an additional constraint by the core.

Regards,
Lucas

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] dmaengine: imx-sdma: advertise all supported slave bus widths

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1172 bytes --]

On Fri, Oct 13, 2017 at 01:35:43PM +0200, Lucas Stach wrote:
> Am Freitag, den 13.10.2017, 12:25 +0100 schrieb Mark Brown:
> > On Fri, Oct 13, 2017 at 12:43:46PM +0200, Lucas Stach wrote:

> > > The commit [1] which changed imx-pcm-dma to derive the pcm_config from the
> > > attached dmaengine introduced a regression on the supported sample formats,
> > > as imx-sdma doesn't properly advertise all the supported slave bus widths.

> > How did this work previously - shouldn't we have failed to use the
> > unadvertised bus widths?  I'd expect the framework to be providing error
> > checking for the clients here.

> No, if the sound PCM implementation explicitly provides a pcm_config
> with the hw.formats set to 0, the core only looks at the DAI supported
> formats. Now that we derive the pcm_config from the dmaengine

I'm not talking about the sound side here...

> hw.formats gets filled with formats supported by the dmaengine, which
> is then used as an additional constraint by the core.

...I'm talking about the constraints supported on the DMA side.  I would
not expect the DMA side to be accepting things it said it didn't support.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] dmaengine: imx-sdma: advertise all supported slave bus widths

[Lucas Stach]

Am Freitag, den 13.10.2017, 15:39 +0100 schrieb Mark Brown:
> On Fri, Oct 13, 2017 at 01:35:43PM +0200, Lucas Stach wrote:
> > Am Freitag, den 13.10.2017, 12:25 +0100 schrieb Mark Brown:
> > > On Fri, Oct 13, 2017 at 12:43:46PM +0200, Lucas Stach wrote:
> > > > The commit [1] which changed imx-pcm-dma to derive the pcm_config from the
> > > > attached dmaengine introduced a regression on the supported sample formats,
> > > > as imx-sdma doesn't properly advertise all the supported slave bus widths.
> > > How did this work previously - shouldn't we have failed to use the
> > > unadvertised bus widths?  I'd expect the framework to be providing error
> > > checking for the clients here.
> > No, if the sound PCM implementation explicitly provides a pcm_config
> > with the hw.formats set to 0, the core only looks at the DAI supported
> > formats. Now that we derive the pcm_config from the dmaengine
> 
> I'm not talking about the sound side here...
> 
> > hw.formats gets filled with formats supported by the dmaengine, which
> > is then used as an additional constraint by the core.
> 
> ...I'm talking about the constraints supported on the DMA side.  I would
> not expect the DMA side to be accepting things it said it didn't support.

AFAIR the caps for the supported slave bus width were introduced later
than other caps and not all drivers did implement them properly, so
validation at the core level at this time would probably have broken a
lot of existing users.

That said the dmaengine core warns since kernel 4.0 if a driver doesn't
set those caps, so it's probably fine if we cook up a patch to do the
validation now.

Regards,
Lucas
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] dmaengine: imx-sdma: advertise all supported slave bus widths

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 913 bytes --]

On Fri, Oct 13, 2017 at 04:50:01PM +0200, Lucas Stach wrote:
> Am Freitag, den 13.10.2017, 15:39 +0100 schrieb Mark Brown:

> > ...I'm talking about the constraints supported on the DMA side.  I would
> > not expect the DMA side to be accepting things it said it didn't support.

> AFAIR the caps for the supported slave bus width were introduced later
> than other caps and not all drivers did implement them properly, so
> validation at the core level at this time would probably have broken a
> lot of existing users.

> That said the dmaengine core warns since kernel 4.0 if a driver doesn't
> set those caps, so it's probably fine if we cook up a patch to do the
> validation now.

That can should be able to be handled cleanly by enforcing any caps that
are advertised but if the driver doesn't declare anything at all then
just accepting anything and leaving it up to the driver to cope.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] dmaengine: imx-sdma: advertise all supported slave bus widths

[Vinod Koul]

On Fri, Oct 13, 2017 at 06:13:03PM +0100, Mark Brown wrote:
> On Fri, Oct 13, 2017 at 04:50:01PM +0200, Lucas Stach wrote:
> > Am Freitag, den 13.10.2017, 15:39 +0100 schrieb Mark Brown:
> 
> > > ...I'm talking about the constraints supported on the DMA side.  I would
> > > not expect the DMA side to be accepting things it said it didn't support.
> 
> > AFAIR the caps for the supported slave bus width were introduced later
> > than other caps and not all drivers did implement them properly, so
> > validation at the core level at this time would probably have broken a
> > lot of existing users.
> 
> > That said the dmaengine core warns since kernel 4.0 if a driver doesn't
> > set those caps, so it's probably fine if we cook up a patch to do the
> > validation now.
> 
> That can should be able to be handled cleanly by enforcing any caps that
> are advertised but if the driver doesn't declare anything at all then
> just accepting anything and leaving it up to the driver to cope.

well DMAengine core cannot guess what hardware supports, but if not provided
we may want to go back to a saner default values which people might support.

But then, not doing this helps people find issues and fix them :)

So which is a better deal?

-- 
~Vinod

Re: [PATCH] dmaengine: imx-sdma: advertise all supported slave bus widths

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 612 bytes --]

On Mon, Oct 16, 2017 at 11:58:36AM +0530, Vinod Koul wrote:
> On Fri, Oct 13, 2017 at 06:13:03PM +0100, Mark Brown wrote:

> > That can should be able to be handled cleanly by enforcing any caps that
> > are advertised but if the driver doesn't declare anything at all then
> > just accepting anything and leaving it up to the driver to cope.

> well DMAengine core cannot guess what hardware supports, but if not provided
> we may want to go back to a saner default values which people might support.

That's why I say "any caps that are advertised" - if the driver said
something I'd expect it to be enforced.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] dmaengine: imx-sdma: advertise all supported slave bus widths

[Vinod Koul]

On Fri, Oct 13, 2017 at 12:43:46PM +0200, Lucas Stach wrote:
> The commit [1] which changed imx-pcm-dma to derive the pcm_config from the

Commit 70d435ba1cd6 (...) changed... please

> attached dmaengine introduced a regression on the supported sample formats,
> as imx-sdma doesn't properly advertise all the supported slave bus widths.
> 
> Fix this to restore full functionality of the imx sound subsystem.
> 
> [1] 70d435ba1cd6 ("ASoC: imx-pcm-dma: simplify pcm_config")

Fixes: 70d435ba1cd6 ... please

> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/dma/imx-sdma.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index a67ec1bdc4e0..e374183f19d0 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -1851,8 +1851,12 @@ static int sdma_probe(struct platform_device *pdev)
>  	sdma->dma_device.device_prep_dma_cyclic = sdma_prep_dma_cyclic;
>  	sdma->dma_device.device_config = sdma_config;
>  	sdma->dma_device.device_terminate_all = sdma_disable_channel_with_delay;
> -	sdma->dma_device.src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
> -	sdma->dma_device.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
> +	sdma->dma_device.src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
> +					   BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> +					   BIT(DMA_SLAVE_BUSWIDTH_1_BYTE);
> +	sdma->dma_device.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
> +					   BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> +					   BIT(DMA_SLAVE_BUSWIDTH_1_BYTE);
>  	sdma->dma_device.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
>  	sdma->dma_device.residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
>  	sdma->dma_device.device_issue_pending = sdma_issue_pending;
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe dmaengine" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
~Vinod

Re: [PATCH] dmaengine: imx-sdma: advertise all supported slave bus widths

[Lucas Stach]

Am Montag, den 16.10.2017, 12:21 +0530 schrieb Vinod Koul:
> On Fri, Oct 13, 2017 at 12:43:46PM +0200, Lucas Stach wrote:
> > The commit [1] which changed imx-pcm-dma to derive the pcm_config
> > from the
> 
> Commit 70d435ba1cd6 (...) changed... please
> 
> > attached dmaengine introduced a regression on the supported sample
> > formats,
> > as imx-sdma doesn't properly advertise all the supported slave bus
> > widths.
> > 
> > Fix this to restore full functionality of the imx sound subsystem.
> > 
> > [1] 70d435ba1cd6 ("ASoC: imx-pcm-dma: simplify pcm_config")
> 
> Fixes: 70d435ba1cd6 ... please

Nope, it isn't strictly a fix for this commit, which is why I didn't
mark it as such. It's a separate fix, which fixes a bad interaction
between dmaengine and sound, but it may also affect other users.

Regards,
Lucas

> 
> > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> >  drivers/dma/imx-sdma.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> > index a67ec1bdc4e0..e374183f19d0 100644
> > --- a/drivers/dma/imx-sdma.c
> > +++ b/drivers/dma/imx-sdma.c
> > @@ -1851,8 +1851,12 @@ static int sdma_probe(struct platform_device *pdev)
> > > >  	sdma->dma_device.device_prep_dma_cyclic = sdma_prep_dma_cyclic;
> > > >  	sdma->dma_device.device_config = sdma_config;
> > > >  	sdma->dma_device.device_terminate_all = sdma_disable_channel_with_delay;
> > > > -	sdma->dma_device.src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
> > > > -	sdma->dma_device.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
> > > > +	sdma->dma_device.src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
> > > > +					   BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> > > > +					   BIT(DMA_SLAVE_BUSWIDTH_1_BYTE);
> > > > +	sdma->dma_device.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
> > > > +					   BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> > > > +					   BIT(DMA_SLAVE_BUSWIDTH_1_BYTE);
> > > >  	sdma->dma_device.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
> > > >  	sdma->dma_device.residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
> > > >  	sdma->dma_device.device_issue_pending = sdma_issue_pending;
> > -- 
> > 2.11.0
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe dmaengine" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] dmaengine: imx-sdma: advertise all supported slave bus widths

[Vinod Koul]

On Fri, Oct 13, 2017 at 12:43:46PM +0200, Lucas Stach wrote:
> The commit [1] which changed imx-pcm-dma to derive the pcm_config from the
> attached dmaengine introduced a regression on the supported sample formats,
> as imx-sdma doesn't properly advertise all the supported slave bus widths.
> 
> Fix this to restore full functionality of the imx sound subsystem.
> 
> [1] 70d435ba1cd6 ("ASoC: imx-pcm-dma: simplify pcm_config")
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

This doesnt apply for me, please rebase and resend. While at it please fix
the stuff flagged by checkpatch

-- 
~Vinod