| Message ID | 20171013104346.16818-1-l.stach@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
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.
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
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.
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?
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
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 > >
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.
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
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;
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(-)