Repository navigation
kpb: disable IRQs before calculation of read pointer - #2329
Conversation
120f2b0 to
6ab20fa
Compare
mrajwa
changed the title
lgirdwood
left a comment
lgirdwood
left a comment
There was a problem hiding this comment.
Good find ! But we need to make the impact local only and check if this applies to others. If this is generic buffer update then we should make it a generic API with the locking built in.
There was a problem hiding this comment.
Do we really want to disable all IRQs on all coes ? A local spinlock_irq() would be better.. Do we also need this locking on other components too ?
There was a problem hiding this comment.
@lgirdwood, this call arch_interrupt_global_enable/disable disables IRQs on single core only. Spinlock inside IRQ is not an option as it will stall us forever. Also, we will probably need a mechanism to detect on what core our pipeline is present and disable IRQs for this particular core. As for now my solution is sufficient I believe.
There was a problem hiding this comment.
@mrajwa arch_interrupt_global_enable() may disable all IRQs on all core on non Intel arch. Btw, why does the spinlock_irq() fail and stall forever.? The spinlock should protect other updates to these values and should prevent all other IRQs on that core. i.e. it should never spin forever if properly used (that means it must also be used in any irq code that touches these pointers)
There was a problem hiding this comment.
@mrajwa arch_interrupt_global_enable() may disable all IRQs on all core on non Intel arch.
Currently it won't happen as this interface only support Xtensa core
Btw, why does the spinlock_irq() fail and stall forever.?
Maybe I misunderstood your idea but if what you are saying boils down to waiting inside IRQ then two thing concerns me:
- Since init_draining is passive level we may get stuck in that IRQ
- Even if we do it in a clever way by postponing that IRQ job it will probably be more expensive than sigle lock before these few instructions aimed for pointer calculation.
There was a problem hiding this comment.
I still see this as a locking issue, I think you should look at all the asynchronous flows within kbp and see where they can race. You are probably missing protection around some of these calculations that can either be trigger via pipeline, voice detect, DMA, IPC or other sources.
The spinlock_irq() wont wait in the IRQ handler. If we are spinning on acquiring the lock in IRQ context then our locking in KBP is broken. Generally you should be protecting (with the spinlock) anything state that the IRQ handler will change so that code running at normal context will see the state atomically.
There was a problem hiding this comment.
I think you should look at all the asynchronous flows within kbp and see where they can race. You are probably missing protection around some of these calculations that can either be trigger via pipeline, voice detect, DMA, IPC or other sources.
Yes this is what I have done and this PR is the result of such analysis. In any other place, including draining we are fine when the IRQ happens. But at the init draining time it is dangerous.
As for the spinlock_irq(), the very first thing it does in current implementation is exactly what I did in the first commit - disable all IRQ below level 5. Then it keeps the lock in shared memory context so other cores should not tinker with our data. The later seems to be premature now as we only utilize one core now.
There was a problem hiding this comment.
please use the spinlock. it's client level API and arch_ are system level APIs not intended for audio processing components.
There was a problem hiding this comment.
Okay I buy this argument now. I have updated, is it OK for you now?
c1f5f52 to
189be3b
Compare
189be3b to
0dd21ba
Compare
There was a problem hiding this comment.
minor, it's always good practice to name/comment the usage here. i.e. /* pointer update locking */ Btw, wont we need the same for W ptr ?
There was a problem hiding this comment.
@lgirdwood, I have updated the comment to describe current use in more details. However in the future we may use this lock in other places too.
Btw, wont we need the same for W ptr ?
Honestly I don't see any chance for a race condition there at the moment. There is only one task which writes to internal history buffer so there is nothing else that can interrupt or overwrite it. Even with multiple customers of KPB there will always be just one buffering task.
dc56757 to
b788a5a
Compare
This patch adds critical section around calculations of draining starting point (buff->r_ptr). Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>
This patch fixes the problem with format switch. The problem was when we allocated history buffer for smaller format like S16_LE and then dynamically changed format to some wider one like S24_LE. In this case KPB was detecting that buffer has already been allocated and used it what limits the amount of data we can store for new format. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>
b788a5a to
3141107
Compare
|
SOFCI TEST |
|
@lgirdwood only two tests on that APL_UP2_NOCODEC failed. Looks like platform issue? |
|
@mmaka1 are you OK with the PR now? If so lets merge it. |
This patch adds critical section around calculations of
draining starting point (buff->r_ptr).
Signed-off-by: Marcin Rajwa marcin.rajwa@linux.intel.com