| Message ID | 1394808168-32608-4-git-send-email-peter.ujfalusi@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/14/2014 03:42 PM, Peter Ujfalusi wrote: > We need to place constraint on the period and buffer size if the read > or write AFIFO is enabled and it is configured for more than one word > otherwise the DMA will fail in buffer configuration where the sizes > are not aligned with the requested FIFO configuration. > > Some application (like mplayer) needs the constraint placed on the > buffer size as well. If only period size is constrained they might > fail to figure out the allowed buffer configuration. Hm... the sound like there is still a bug somewhere. We constrain the buffer size to be a multiple of the period size. If the period size is constraint to be a multiple of a constant then the buffer size should automatically be constrained to be a multiple of constant * period count. And just constraining the buffer size to be a multiple of the burst size still allows buffer sizes that are not a multiple of burst size * period count. > > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > --- > sound/soc/davinci/davinci-mcasp.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c > index a01ae97c90aa..3ca6e8c4568a 100644 > --- a/sound/soc/davinci/davinci-mcasp.c > +++ b/sound/soc/davinci/davinci-mcasp.c > @@ -720,6 +720,7 @@ static int davinci_mcasp_startup(struct snd_pcm_substream *substream, > struct snd_soc_dai *dai) > { > struct davinci_mcasp *mcasp = snd_soc_dai_get_drvdata(dai); > + int afifo_numevt; > > if (mcasp->version == MCASP_VERSION_4) > snd_soc_dai_set_dma_data(dai, substream, > @@ -727,6 +728,21 @@ static int davinci_mcasp_startup(struct snd_pcm_substream *substream, > else > snd_soc_dai_set_dma_data(dai, substream, mcasp->dma_params); > > + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) > + afifo_numevt = mcasp->txnumevt; > + else > + afifo_numevt = mcasp->rxnumevt; Shouldn't this use the same calculation that's used to set dma_data->maxburst? Also we should add this to the dmaengine PCM driver, since there will most likely be more DMA controllers with this restriction, doesn't necessarily need to be done in this patch series though. > + > + if (afifo_numevt > 1) { > + snd_pcm_hw_constraint_step(substream->runtime, 0, > + SNDRV_PCM_HW_PARAM_PERIOD_SIZE, > + afifo_numevt); > + > + snd_pcm_hw_constraint_step(substream->runtime, 0, > + SNDRV_PCM_HW_PARAM_BUFFER_SIZE, > + afifo_numevt); > + } > + > return 0; > } > >
On 03/16/2014 01:18 PM, Lars-Peter Clausen wrote: > On 03/14/2014 03:42 PM, Peter Ujfalusi wrote: >> We need to place constraint on the period and buffer size if the read >> or write AFIFO is enabled and it is configured for more than one word >> otherwise the DMA will fail in buffer configuration where the sizes >> are not aligned with the requested FIFO configuration. >> >> Some application (like mplayer) needs the constraint placed on the >> buffer size as well. If only period size is constrained they might >> fail to figure out the allowed buffer configuration. > > Hm... the sound like there is still a bug somewhere. We constrain the buffer > size to be a multiple of the period size. If the period size is constraint to > be a multiple of a constant then the buffer size should automatically be > constrained to be a multiple of constant * period count. And just constraining > the buffer size to be a multiple of the burst size still allows buffer sizes > that are not a multiple of burst size * period count. Yeah, aplay, PA is fine. mplayer however does things in a different manner. I use mplayer mainly to test PA (with -ao pulse) but sometimes I use it through ALSA and this is where I noticed the issue. If I constrain only period_size mplayer will fail to set hw_params. If I only constrain the buffer_size than we can have period_size which does not allign with the FIFO settings. I also tried to have snd_pcm_hw_rule_add() and using the snd_interval_step() - which need to be exported but mplayer fails with that also. It fails even if I add the rule for period and buffer size. Also the constraint as it is currently is not optimal since it does not take into account the number of channels as you pointed out. >> >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> >> --- >> sound/soc/davinci/davinci-mcasp.c | 16 ++++++++++++++++ >> 1 file changed, 16 insertions(+) >> >> diff --git a/sound/soc/davinci/davinci-mcasp.c >> b/sound/soc/davinci/davinci-mcasp.c >> index a01ae97c90aa..3ca6e8c4568a 100644 >> --- a/sound/soc/davinci/davinci-mcasp.c >> +++ b/sound/soc/davinci/davinci-mcasp.c >> @@ -720,6 +720,7 @@ static int davinci_mcasp_startup(struct >> snd_pcm_substream *substream, >> struct snd_soc_dai *dai) >> { >> struct davinci_mcasp *mcasp = snd_soc_dai_get_drvdata(dai); >> + int afifo_numevt; >> >> if (mcasp->version == MCASP_VERSION_4) >> snd_soc_dai_set_dma_data(dai, substream, >> @@ -727,6 +728,21 @@ static int davinci_mcasp_startup(struct >> snd_pcm_substream *substream, >> else >> snd_soc_dai_set_dma_data(dai, substream, mcasp->dma_params); >> >> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) >> + afifo_numevt = mcasp->txnumevt; >> + else >> + afifo_numevt = mcasp->rxnumevt; > > Shouldn't this use the same calculation that's used to set dma_data->maxburst? > Also we should add this to the dmaengine PCM driver, since there will most > likely be more DMA controllers with this restriction, doesn't necessarily need > to be done in this patch series though. At startup time we do not know the number of channels going to be used. I'm planning to improve this constraint handling in a coming series. Using the afifo_numevt as step is not optimal since in case of stereo stream the step should be (afifo_numevt / 2). afifo_numevt is the sample count and not the frame count. > >> + >> + if (afifo_numevt > 1) { >> + snd_pcm_hw_constraint_step(substream->runtime, 0, >> + SNDRV_PCM_HW_PARAM_PERIOD_SIZE, >> + afifo_numevt); >> + >> + snd_pcm_hw_constraint_step(substream->runtime, 0, >> + SNDRV_PCM_HW_PARAM_BUFFER_SIZE, >> + afifo_numevt); >> + } >> + >> return 0; >> } >> >> >
On Mon, Mar 17, 2014 at 03:28:49PM +0200, Peter Ujfalusi wrote: > On 03/16/2014 01:18 PM, Lars-Peter Clausen wrote: > > Hm... the sound like there is still a bug somewhere. We constrain the buffer > > size to be a multiple of the period size. If the period size is constraint to > > be a multiple of a constant then the buffer size should automatically be > > constrained to be a multiple of constant * period count. And just constraining > > the buffer size to be a multiple of the burst size still allows buffer sizes > > that are not a multiple of burst size * period count. > Yeah, aplay, PA is fine. mplayer however does things in a different manner. I > use mplayer mainly to test PA (with -ao pulse) but sometimes I use it through > ALSA and this is where I noticed the issue. > If I constrain only period_size mplayer will fail to set hw_params. If I only > constrain the buffer_size than we can have period_size which does not allign > with the FIFO settings. > I also tried to have snd_pcm_hw_rule_add() and using the snd_interval_step() - > which need to be exported but mplayer fails with that also. It fails even if I > add the rule for period and buffer size. > Also the constraint as it is currently is not optimal since it does not take > into account the number of channels as you pointed out. This is all sounding like the thing that needs to be looked at here is mplayer so we understand what's going wrong with regard to the buffer sizes. It's sounding like if we should be doing it this is a general thing which we should be constraining presumably it'd apply to all drivers, not just this one, and so shouldn't be being fixed in the driver but it's not obvious to me why the period constraint isn't sufficient.
On 03/17/2014 06:52 PM, Mark Brown wrote: > This is all sounding like the thing that needs to be looked at here is > mplayer so we understand what's going wrong with regard to the buffer > sizes. The error which was printed by mplayer was the snd_pcm_hw_params() failure. Prior to this call it calls number of snd_pcm_hw_params_set_* including: snd_pcm_hw_params_set_buffer_time_near() snd_pcm_hw_params_set_periods_near() all without error. So I recompiled alsa-lib with debug and compiled mplayer on the board as well. I only kept the constraint for the period size from this patch (no constraint on buffer size). The compiled and original mplayer binary is working fine, no errors :o Then I recompiled alsa-lib without debug (as it was before): same thing, moth mplayer binary works fine and it picks correct buffer configuration. I'll resend the last two patch and place the constraint only to the period size. > It's sounding like if we should be doing it this is a general > thing which we should be constraining presumably it'd apply to all > drivers, not just this one, and so shouldn't be being fixed in the > driver but it's not obvious to me why the period constraint isn't > sufficient. It can only be done if we have fixed FIFO for the dai. Right now this is kind of true for McASP. In HW we actually have 64 32bit word FIFO and currently you can set the desired FIFO depth via DT/pdata. I'm not really happy about this since it creates this constraint on the buffer allocation when the SoC is using eDMA (when sDMA is in used with McASP we do not have such issue). McBSP also have FIFO on OMAPs, but there I do not lock the FIFO depth, it is adaptive based on the period/buffer size. If we have more device with fixed FIFO depth then it make sense to handle the constraint in the core. However in case of McASP it is still not that straight forward. The DMA burst size depends on the number of channels, number of used serializers and the AFIFO's depth configuration. At the end the period size need to be constraint to be steps of DMA burst size for eDMA. Not sure if this is applicable for other SoCs.
On Tue, Mar 18, 2014 at 02:35:48PM +0200, Peter Ujfalusi wrote: > On 03/17/2014 06:52 PM, Mark Brown wrote: > > It's sounding like if we should be doing it this is a general > > thing which we should be constraining presumably it'd apply to all > > drivers, not just this one, and so shouldn't be being fixed in the > > driver but it's not obvious to me why the period constraint isn't > > sufficient. > It can only be done if we have fixed FIFO for the dai. Right now this is kind > of true for McASP. In HW we actually have 64 32bit word FIFO and currently you > can set the desired FIFO depth via DT/pdata. I'm not really happy about this Sorry, what I meant was more the bit where you were setting the constraint on both the buffer and the period than the constraint itself - that seemed somehow redundant.
diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c index a01ae97c90aa..3ca6e8c4568a 100644 --- a/sound/soc/davinci/davinci-mcasp.c +++ b/sound/soc/davinci/davinci-mcasp.c @@ -720,6 +720,7 @@ static int davinci_mcasp_startup(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { struct davinci_mcasp *mcasp = snd_soc_dai_get_drvdata(dai); + int afifo_numevt; if (mcasp->version == MCASP_VERSION_4) snd_soc_dai_set_dma_data(dai, substream, @@ -727,6 +728,21 @@ static int davinci_mcasp_startup(struct snd_pcm_substream *substream, else snd_soc_dai_set_dma_data(dai, substream, mcasp->dma_params); + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + afifo_numevt = mcasp->txnumevt; + else + afifo_numevt = mcasp->rxnumevt; + + if (afifo_numevt > 1) { + snd_pcm_hw_constraint_step(substream->runtime, 0, + SNDRV_PCM_HW_PARAM_PERIOD_SIZE, + afifo_numevt); + + snd_pcm_hw_constraint_step(substream->runtime, 0, + SNDRV_PCM_HW_PARAM_BUFFER_SIZE, + afifo_numevt); + } + return 0; }
We need to place constraint on the period and buffer size if the read or write AFIFO is enabled and it is configured for more than one word otherwise the DMA will fail in buffer configuration where the sizes are not aligned with the requested FIFO configuration. Some application (like mplayer) needs the constraint placed on the buffer size as well. If only period size is constrained they might fail to figure out the allowed buffer configuration. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> --- sound/soc/davinci/davinci-mcasp.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)