On Mon, Apr 27, 2015 at 03:58:56PM +0200, Arnaud Pouliquen wrote:
On 04/24/2015 08:15 PM, Mark Brown wrote:
On Tue, Apr 14, 2015 at 03:35:27PM +0200, Arnaud Pouliquen wrote:
Please delete unneeded context from your mails and fix your mailer to format things normally. I'm not entirely sure what it's doing, it seems to be be that it's not leaving blanks between paragraphs and wrapping at odd and inconsistent places. It is very hard to read your messages as a result.
There's a lot of very coarse grained spinlock usage in this driver which I'm having a hard time understanding, at the very least the decisions about locking need to be documented much more clearly and I suspect that either things need to be finer grained, we should be using mutexes more or both.
This part is linked to the standby mode described in binding ( patch[1/7]) it manage a runtime suspend, because ASOC runtime suspend is dedicated to DAPM. As you recommend i will try to change it by a DAPM linked to CPU_DAI. Just need to find a wait to retrieve CPU_DAI context in DAPM.. Concerning spinlock I use it to protect context ( called by IRQ, on suspend, by user...). As some code is called in atomic i can not use mutex. I will review it and document it.
Please note my comments about these locks being very coarse grained - I suspect that the spinlock is covering too much.
- /* Get interrupt status & clear them immediately */
- preempt_disable();
- status = GET_UNIPERIF_ITS(player);
- SET_UNIPERIF_ITS_BCLR(player, status);
- preempt_enable();
preempt_disable()? Why is this being done, if you're doing unusual stuff like this the code needs to be very clear about what the locking rules are?
This is used to be sure to not miss an interrupt. If preempted between both instruction i can clear an interruption flag before treat it. In this case i will receive a second interrupt with all flag to 0.
That doesn't sound right, the interrupt appears to be write to clear so if we get a second interrupt between the read and write surely we'd not get another spurious interrupt? It would look like we were acknowleding the second raise. It sounds like something else is going on here.
In any case the spurious interrupt doesn't seem like it should happen a lot, nor like it should be especially serious if we can handle it.
- snd_pcm_stream_lock(player->substream);
Again please explain the locking if we're doing something unusual.
This protect from a race condition, if application request a stop while we receive Error from IP. I call it here to avoid toprotect all snd_pcm_stop calls... but i can protect only the snd_pcm_stop calls if more clear
Please do something to make it clear what you're doing.
- /* Check for fifo error (under run) */
- if (unlikely(status & UNIPERIF_ITS_FIFO_ERROR_MASK(player))) {
dev_err(player->dev, "FIFO underflow error detected");/* Interrupt is just for information when underflow recovery */if (player->info->underflow_enabled) {/* Update state to underflow */player->state = UNIPERIF_STATE_UNDERFLOW;
Why would underflow recovery be optional?
To propose 2 strategies:
- stop on underrun ( because plop will occurs)
- try to recover it: time is recovered when new data is available ( sample
dropping)
Well, the standard behaviour is to halt on error (so that's what the rest of the stack will expect) and it doesn't sound like the recovery is really that great anyway.