[PATCH 06/10] ALSA: axd: add basic files for sending/receiving axd cmds
These files do the important part of talking with AXD to send and
receive data buffers.
Signed-off-by: Qais Yousef
Cc: Liam Girdwood
Cc: Mark Brown
Cc: Jaroslav Kysela
Cc: Takashi Iwai
Cc: linux-kernel@vger.kernel.org
---
sound/soc/img/axd/axd_cmds.c | 102 +++
sound/soc/img/axd/axd_cmds.h | 532 ++
sound/soc/img/axd/axd_cmds_pipes.c | 1387
3 files changed, 2021 insertions(+)
create mode 100644 sound/soc/img/axd/axd_cmds.c
create mode 100644 sound/soc/img/axd/axd_cmds.h
create mode 100644 sound/soc/img/axd/axd_cmds_pipes.c
diff --git a/sound/soc/img/axd/axd_cmds.c b/sound/soc/img/axd/axd_cmds.c
new file mode 100644
index ..eb160f46489b
--- /dev/null
+++ b/sound/soc/img/axd/axd_cmds.c
@@ -0,0 +1,102 @@
+/*
+ * Copyright (C) 2011-2015 Imagination Technologies Ltd.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version
+ * 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * AXD Commands API - generic setup functions.
+ */
+#include "axd_api.h"
+#include "axd_cmds.h"
+#include "axd_cmds_internal.h"
+#include "axd_module.h"
+
+static unsigned long __io_address;
+static unsigned long __phys_address;
+
+void axd_cmd_init(struct axd_cmd *cmd, unsigned long cmd_address,
+ unsigned long io_address, unsigned long phys_address)
+{
+ int i;
+
+ cmd->message = (struct axd_memory_map __iomem *)cmd_address;
+ mutex_init(&cmd->cm_lock);
+ init_waitqueue_head(&cmd->wait);
+ axd_set_flag(&cmd->response_flg, 0);
+ axd_set_flag(&cmd->fw_stopped_flg, 0);
+ for (i = 0; i < AXD_MAX_PIPES; i++) {
+ axd_cmd_inpipe_init(cmd, i);
+ axd_cmd_outpipe_init(cmd, i);
+ }
+ __io_address = io_address;
+ __phys_address = phys_address;
+ cmd->watchdogenabled = 1;
+ /*
+* By default, always discard any pending buffers if an output device is
+* closed before EOS is reached.
+* This behaviour can be changed through kcontrol. If discard is
disabled,
+* then upon closing an output device before EOS is reached, it'll
+* resume from where it stopped.
+*/
+ axd_set_flag(&cmd->discard_flg, 1);
+ axd_set_flag(&cmd->ctrlbuf_active_flg, 0);
+}
+
+int axd_cmd_set_pc(struct axd_cmd *cmd, unsigned int thread, unsigned long pc)
+{
+ if (thread >= THREAD_COUNT)
+ return -1;
+ iowrite32(pc, &cmd->message->pc[thread]);
+ return 0;
+}
+
+unsigned long axd_cmd_get_datain_address(struct axd_cmd *cmd)
+{
+ struct axd_dev *axd = container_of(cmd, struct axd_dev, cmd);
+
+ return (unsigned long) axd->buf_base_m;
+}
+
+unsigned long axd_cmd_get_datain_size(struct axd_cmd *cmd)
+{
+ struct axd_dev *axd = container_of(cmd, struct axd_dev, cmd);
+
+ return axd->inbuf_size;
+}
+
+unsigned long axd_cmd_get_dataout_address(struct axd_cmd *cmd)
+{
+ struct axd_dev *axd = container_of(cmd, struct axd_dev, cmd);
+
+ return ((unsigned long) axd->buf_base_m) + axd->inbuf_size;
+}
+
+unsigned long axd_cmd_get_dataout_size(struct axd_cmd *cmd)
+{
+ struct axd_dev *axd = container_of(cmd, struct axd_dev, cmd);
+
+ return axd->outbuf_size;
+}
+
+/*
+ * The driver understands IO address, while f/w understands physical addresses.
+ * A couple of helper functions to aid in converting when exchanging buffers.
+ *
+ * NOTE:
+ * buf must NOT be NULL - we want this as fast as possible, so omit the check
+ * for NULLl
+ */
+inline char *axd_io_2_phys(const char *buf)
+{
+ return (char *)(buf - __io_address + __phys_address);
+}
+inline char *axd_phys_2_io(const char *buf)
+{
+ return (char *)(buf - __phys_address + __io_address);
+}
diff --git a/sound/soc/img/axd/axd_cmds.h b/sound/soc/img/axd/axd_cmds.h
new file mode 100644
index ..d8f3db29eea3
--- /dev/null
+++ b/sound/soc/img/axd/axd_cmds.h
@@ -0,0 +1,532 @@
+/*
+ * Copyright (C) 2011-2015 Imagination Technologies Ltd.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version
+ * 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * AXD API commands Helper functions.
+ */
+#ifndef AXD_CMDS_H_
+#define AXD_CMDS_H_
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include "linux
Re: [PATCH 06/10] ALSA: axd: add basic files for sending/receiving axd cmds
On Mon, Aug 24, 2015 at 01:39:15PM +0100, Qais Yousef wrote:
> +int axd_cmd_set_pc(struct axd_cmd *cmd, unsigned int thread, unsigned long
> pc)
> +{
> + if (thread >= THREAD_COUNT)
> + return -1;
Return sensible error codes please.
> +unsigned long axd_cmd_get_datain_address(struct axd_cmd *cmd)
> +{
> + struct axd_dev *axd = container_of(cmd, struct axd_dev, cmd);
> +
> + return (unsigned long) axd->buf_base_m;
> +}
What's going on with these casts?
> +static inline void axd_set_flag(unsigned int *flag, unsigned int value)
> +{
> + *flag = value;
> + smp_wmb(); /* guarantee smp ordering */
> +}
> +
> +static inline unsigned int axd_get_flag(unsigned int *flag)
> +{
> + smp_rmb(); /* guarantee smp ordering */
> + return *flag;
> +}
Please use a normal locking construct rather than hand rolling
something, or alternatively introduce new generic operations. The fact
that you're hand rolling these things that have no driver specific
content is really worrying in terms of their safety.
> +/*
> + * axd_pipe->enabled_flg for output pipes is overloaded to mean two things:
> + *
> + * - PIPE_STARTED: indicates that pipe was opened but no buffers were passed.
> + * When stopping the pipes, we know that we don't need to discard anything
> if
> + * the discard_flg is set in cmd struct. Which allows us to terminate
> easily
> + * and quickly.
> + *
> + * - PIPE_RUNNING: indicates that pipe has processed some buffers, so we
> should
> + * discard if user terminates early (and discard_flg is set in cmd struct).
> + */
> +#define PIPE_STARTED 1
> +#define PIPE_RUNNING 2
Why is the case with in place buffers not a simple zero iteration loop?
> +#ifdef AXD_DEBUG_DIAG
> +static unsigned int inSentCount[AXD_MAX_PIPES];
> +static unsigned int inRecvCount[AXD_MAX_PIPES];
> +static unsigned int outSentCount[AXD_MAX_PIPES];
> +static unsigned int outRecvCount[AXD_MAX_PIPES];
> +static unsigned int primeupCount[AXD_MAX_PIPES];
> +static unsigned int read_size[AXD_MAX_PIPES];
> +static unsigned int write_size[AXD_MAX_PIPES];
> +static unsigned int recv_size[AXD_MAX_PIPES];
No static globals and please follow the kernel coding style.
> +static inline void axd_datain_kick(struct axd_pipe *axd_pipe)
> +{
> + unsigned long flags;
> + struct axd_memory_map __iomem *message = axd_pipe->cmd->message;
> + unsigned int pipe = axd_pipe->id;
> + unsigned int temp;
> +
> +#ifdef AXD_DEBUG_DIAG
> + inSentCount[pipe]++;
> +#endif
Define accessor macros for these and then define them to noops when not
debugging rather than having #defines in the code.
> +static irqreturn_t axd_irq(int irq, void *data)
> +{
> + struct axd_cmd *cmd = data;
> + unsigned int int_status;
> + unsigned long flags;
> + int i, ret;
> +
> + /*
> + * int_status is ioremapped() which means it could page fault. When axd
> + * is running on the same core as the host, holding lock2 would disable
> + * exception handling in that core which means a page fault would stuff
> + * host thread executing the driver. We do a double read here to ensure
> + * that we stall until the memory access is done before lock2 is
> + * acquired, hence ensuring that any page fault is handled outside lock2
> + * region.
> + */
> + int_status = ioread32(&cmd->message->int_status);
> + int_status = ioread32(&cmd->message->int_status);
Eew.
> +
> + axd_platform_irq_ack();
When would this ever be called anywhere else? Just inline it (and it's
better practice to only ack things we handle...).
> + flags = axd_platform_lock();
> + int_status = ioread32(&cmd->message->int_status);
> + iowrite32(0, &cmd->message->int_status);
> +
> + if (!int_status)
> + goto out;
This should cause us to return IRQ_NONE.
> + if (int_status & AXD_INT_ERROR) {
> + struct axd_dev *axd = container_of(cmd, struct axd_dev, cmd);
> + int error = ioread32(&cmd->message->error);
> +
> + pr_debug("< Received error interrupt\n");
> + switch (error) {
> + default:
> + case 0:
> + break;
We just ignore these?
> + case 2:
> + dev_warn(axd->dev, "Failed to set last configuration
> command\n");
> + break;
Does the configuration command notice?
> + /*
> + * if we could lock the semaphore, then we're guaranteed that the
> + * current rd_idx is valid and ready to be used. So no need to verify
> + * that the status of the descriptor at rd_idx is valid.
> + */
> + spin_lock(&desc_ctrl->rd_lock);
It really feels like this locking is all complicated and fragile. I'm
not entirely sure the optimisation is worth it - are we really sending
compressed audio at such a high rate that it's worth having concurrency
handling that's hard to think about?
> +void axd_cmd_free_
Re: [PATCH 06/10] ALSA: axd: add basic files for sending/receiving axd cmds
On 08/26/2015 08:16 PM, Mark Brown wrote:
On Mon, Aug 24, 2015 at 01:39:15PM +0100, Qais Yousef wrote:
+int axd_cmd_set_pc(struct axd_cmd *cmd, unsigned int thread, unsigned long pc)
+{
+ if (thread >= THREAD_COUNT)
+ return -1;
Return sensible error codes please.
OK.
+unsigned long axd_cmd_get_datain_address(struct axd_cmd *cmd)
+{
+ struct axd_dev *axd = container_of(cmd, struct axd_dev, cmd);
+
+ return (unsigned long) axd->buf_base_m;
+}
What's going on with these casts?
As with the other cases. buf_base_m is void * __iomem but we want to do
some arithmatic to help AXD start up and understand where it needs to
run. I agree they don't look nice and if I can avoid them I'd be happy
to do so.
+static inline void axd_set_flag(unsigned int *flag, unsigned int value)
+{
+ *flag = value;
+ smp_wmb(); /* guarantee smp ordering */
+}
+
+static inline unsigned int axd_get_flag(unsigned int *flag)
+{
+ smp_rmb(); /* guarantee smp ordering */
+ return *flag;
+}
Please use a normal locking construct rather than hand rolling
something, or alternatively introduce new generic operations. The fact
that you're hand rolling these things that have no driver specific
content is really worrying in terms of their safety.
I need to check atomic_ops.txt again but I think atomic_t is not always
smb safe. I definitely was running on a version of Meta archicture in
the past where atomic_t wasn't always smp safe.
I'll check if the rules have changed or something new was introduced to
deal with this.
+/*
+ * axd_pipe->enabled_flg for output pipes is overloaded to mean two things:
+ *
+ * - PIPE_STARTED: indicates that pipe was opened but no buffers were passed.
+ * When stopping the pipes, we know that we don't need to discard anything if
+ * the discard_flg is set in cmd struct. Which allows us to terminate easily
+ * and quickly.
+ *
+ * - PIPE_RUNNING: indicates that pipe has processed some buffers, so we should
+ * discard if user terminates early (and discard_flg is set in cmd struct).
+ */
+#define PIPE_STARTED 1
+#define PIPE_RUNNING 2
Why is the case with in place buffers not a simple zero iteration loop?
This is important when AXD is not consuming the data through I2S and
returning them to Linux. What we're trying to deal with here is the
firmware processed some data and expects Linux to consume whatever it
has sent back to it. We want to ensure that if the user suddenly stopped
consuming this data by closing the pipe to drop anything we receive back
from AXD otherwise the workqueue would block indefinitely waiting for
the user that disappeared to consume it causing a deadlock.
+#ifdef AXD_DEBUG_DIAG
+static unsigned int inSentCount[AXD_MAX_PIPES];
+static unsigned int inRecvCount[AXD_MAX_PIPES];
+static unsigned int outSentCount[AXD_MAX_PIPES];
+static unsigned int outRecvCount[AXD_MAX_PIPES];
+static unsigned int primeupCount[AXD_MAX_PIPES];
+static unsigned int read_size[AXD_MAX_PIPES];
+static unsigned int write_size[AXD_MAX_PIPES];
+static unsigned int recv_size[AXD_MAX_PIPES];
No static globals and please follow the kernel coding style.
OK I'll fix.
+static inline void axd_datain_kick(struct axd_pipe *axd_pipe)
+{
+ unsigned long flags;
+ struct axd_memory_map __iomem *message = axd_pipe->cmd->message;
+ unsigned int pipe = axd_pipe->id;
+ unsigned int temp;
+
+#ifdef AXD_DEBUG_DIAG
+ inSentCount[pipe]++;
+#endif
Define accessor macros for these and then define them to noops when not
debugging rather than having #defines in the code.
Yep sounds a better way to do it.
+static irqreturn_t axd_irq(int irq, void *data)
+{
+ struct axd_cmd *cmd = data;
+ unsigned int int_status;
+ unsigned long flags;
+ int i, ret;
+
+ /*
+* int_status is ioremapped() which means it could page fault. When axd
+* is running on the same core as the host, holding lock2 would disable
+* exception handling in that core which means a page fault would stuff
+* host thread executing the driver. We do a double read here to ensure
+* that we stall until the memory access is done before lock2 is
+* acquired, hence ensuring that any page fault is handled outside lock2
+* region.
+ */
+ int_status = ioread32(&cmd->message->int_status);
+ int_status = ioread32(&cmd->message->int_status);
Eew.
Luckily this is not a problem anymore. This must have slipped back in
while preparing the patches for submission. I'll audit the code again to
make sure this didn't happen somewhere else.
+
+ axd_platform_irq_ack();
When would this ever be called anywhere else? Just inline it (and it's
better practice to only ack things we handle...).
It wouldn't be called anywhere else but its implementation could be
platform specific that's why it's abstracted. At the moment it does
noth
Re: [PATCH 06/10] ALSA: axd: add basic files for sending/receiving axd cmds
On Thu, Aug 27, 2015 at 04:40:09PM +0100, Qais Yousef wrote:
> On 08/26/2015 08:16 PM, Mark Brown wrote:
> >On Mon, Aug 24, 2015 at 01:39:15PM +0100, Qais Yousef wrote:
> >>+unsigned long axd_cmd_get_datain_address(struct axd_cmd *cmd)
> >>+{
> >>+ struct axd_dev *axd = container_of(cmd, struct axd_dev, cmd);
> >>+
> >>+ return (unsigned long) axd->buf_base_m;
> >>+}
> >What's going on with these casts?
> As with the other cases. buf_base_m is void * __iomem but we want to do some
> arithmatic to help AXD start up and understand where it needs to run. I
> agree they don't look nice and if I can avoid them I'd be happy to do so.
C supports poinnter arithmetic...
> >>+static inline void axd_set_flag(unsigned int *flag, unsigned int value)
> >>+{
> >>+ *flag = value;
> >>+ smp_wmb(); /* guarantee smp ordering */
> >>+}
> >>+static inline unsigned int axd_get_flag(unsigned int *flag)
> >>+{
> >>+ smp_rmb(); /* guarantee smp ordering */
> >>+ return *flag;
> >>+}
> >Please use a normal locking construct rather than hand rolling
> >something, or alternatively introduce new generic operations. The fact
> >that you're hand rolling these things that have no driver specific
> >content is really worrying in terms of their safety.
> I need to check atomic_ops.txt again but I think atomic_t is not always smb
> safe. I definitely was running on a version of Meta archicture in the past
> where atomic_t wasn't always smp safe.
> I'll check if the rules have changed or something new was introduced to deal
> with this.
It is true that when using atomic_t with multiprocessor you still need
memory barriers but that doesn't mean atomics bring no benefit. But
that's not really the point here - the point is the more general one
that the whole idea of open coding memory barrier concurrency constructs
doesn't look great. It makes the code much more complex and error prone
compared to using normal locking and other concurrency constructs (which
Linux has a rich set of).
If we really need performance then it can make sense but I'm not seeing
anything here that appears to motivate this. In general all the
concurrency code looks much more complex than I would expect.
> >>+#define PIPE_STARTED 1
> >>+#define PIPE_RUNNING 2
> >Why is the case with in place buffers not a simple zero iteration loop?
> This is important when AXD is not consuming the data through I2S and
> returning them to Linux. What we're trying to deal with here is the firmware
> processed some data and expects Linux to consume whatever it has sent back
> to it. We want to ensure that if the user suddenly stopped consuming this
> data by closing the pipe to drop anything we receive back from AXD otherwise
> the workqueue would block indefinitely waiting for the user that disappeared
> to consume it causing a deadlock.
That doesn't seem to address the question...
> >>+ axd_platform_irq_ack();
> >When would this ever be called anywhere else? Just inline it (and it's
> >better practice to only ack things we handle...).
> It wouldn't be called anywhere else but its implementation could be platform
> specific that's why it's abstracted. At the moment it does nothing now we're
> using MIPS but we shouldn't assume that this will always be the case.
> The main purpose of this function is to deassert the interrupt line if the
> way interrrupts are wired for that platform required so. In the past we were
> running in hardware where interrupts are sent through special slave port and
> the interrupt required to be acked or deasserted.
This sounds like something that should be in the interrupt controller
implementation not the leaf driver, just remove this unless you're
actually abstracting something.
> >>+ flags = axd_platform_lock();
> >>+ int_status = ioread32(&cmd->message->int_status);
> >>+ iowrite32(0, &cmd->message->int_status);
> >>+
> >>+ if (!int_status)
> >>+ goto out;
> >This should cause us to return IRQ_NONE.
> I don't think it's necessary. It could happen that AXD sent a DATAIN
> interrupt and shortly after sent DATAOUT interrupt but the handler was
> running before the DATAOUT case is handled causing both interrupts to be
> handled in one go but the handler could be called again to find out that
> there's nothing to do.
Please implement your interrupt handler properly so that the genirq
error handling code can work and it is robust against things going
wrong in future. It's not like it's a huge amount of complex code.
> >>+ if (int_status & AXD_INT_ERROR) {
> >>+ struct axd_dev *axd = container_of(cmd, struct axd_dev, cmd);
> >>+ int error = ioread32(&cmd->message->error);
> >>+
> >>+ pr_debug("< Received error interrupt\n");
> >>+ switch (error) {
> >>+ default:
> >>+ case 0:
> >>+ break;
> >We just ignore these?
> Case 0 doesn't indicate anything anymore. I can print a warning about
> unexpected e
Re: [PATCH 06/10] ALSA: axd: add basic files for sending/receiving axd cmds
On 08/29/2015 11:18 AM, Mark Brown wrote:
On Thu, Aug 27, 2015 at 04:40:09PM +0100, Qais Yousef wrote:
On 08/26/2015 08:16 PM, Mark Brown wrote:
On Mon, Aug 24, 2015 at 01:39:15PM +0100, Qais Yousef wrote:
+unsigned long axd_cmd_get_datain_address(struct axd_cmd *cmd)
+{
+ struct axd_dev *axd = container_of(cmd, struct axd_dev, cmd);
+
+ return (unsigned long) axd->buf_base_m;
+}
What's going on with these casts?
As with the other cases. buf_base_m is void * __iomem but we want to do some
arithmatic to help AXD start up and understand where it needs to run. I
agree they don't look nice and if I can avoid them I'd be happy to do so.
C supports poinnter arithmetic...
+static inline void axd_set_flag(unsigned int *flag, unsigned int value)
+{
+ *flag = value;
+ smp_wmb(); /* guarantee smp ordering */
+}
+static inline unsigned int axd_get_flag(unsigned int *flag)
+{
+ smp_rmb(); /* guarantee smp ordering */
+ return *flag;
+}
Please use a normal locking construct rather than hand rolling
something, or alternatively introduce new generic operations. The fact
that you're hand rolling these things that have no driver specific
content is really worrying in terms of their safety.
I need to check atomic_ops.txt again but I think atomic_t is not always smb
safe. I definitely was running on a version of Meta archicture in the past
where atomic_t wasn't always smp safe.
I'll check if the rules have changed or something new was introduced to deal
with this.
It is true that when using atomic_t with multiprocessor you still need
memory barriers but that doesn't mean atomics bring no benefit. But
that's not really the point here - the point is the more general one
that the whole idea of open coding memory barrier concurrency constructs
doesn't look great. It makes the code much more complex and error prone
compared to using normal locking and other concurrency constructs (which
Linux has a rich set of).
If we really need performance then it can make sense but I'm not seeing
anything here that appears to motivate this. In general all the
concurrency code looks much more complex than I would expect.
OK I'll improve on this.
+#define PIPE_STARTED 1
+#define PIPE_RUNNING 2
Why is the case with in place buffers not a simple zero iteration loop?
This is important when AXD is not consuming the data through I2S and
returning them to Linux. What we're trying to deal with here is the firmware
processed some data and expects Linux to consume whatever it has sent back
to it. We want to ensure that if the user suddenly stopped consuming this
data by closing the pipe to drop anything we receive back from AXD otherwise
the workqueue would block indefinitely waiting for the user that disappeared
to consume it causing a deadlock.
That doesn't seem to address the question...
I'm sorry I don't understand your question then. Can you rephrase it please?
+ axd_platform_irq_ack();
When would this ever be called anywhere else? Just inline it (and it's
better practice to only ack things we handle...).
It wouldn't be called anywhere else but its implementation could be platform
specific that's why it's abstracted. At the moment it does nothing now we're
using MIPS but we shouldn't assume that this will always be the case.
The main purpose of this function is to deassert the interrupt line if the
way interrrupts are wired for that platform required so. In the past we were
running in hardware where interrupts are sent through special slave port and
the interrupt required to be acked or deasserted.
This sounds like something that should be in the interrupt controller
implementation not the leaf driver, just remove this unless you're
actually abstracting something.
We're actually abstracting something. This mechanism might not be part
of an interrupt controller that is understood by Linux. At least I had
this case in the past where the interrupt generated by AXD must be acked
by writing to a special memory address.
We don't have a current user for it now though so it makes sense to
remove it and if a similar user comes up in the future we can sort it
out then.
+ flags = axd_platform_lock();
+ int_status = ioread32(&cmd->message->int_status);
+ iowrite32(0, &cmd->message->int_status);
+
+ if (!int_status)
+ goto out;
This should cause us to return IRQ_NONE.
I don't think it's necessary. It could happen that AXD sent a DATAIN
interrupt and shortly after sent DATAOUT interrupt but the handler was
running before the DATAOUT case is handled causing both interrupts to be
handled in one go but the handler could be called again to find out that
there's nothing to do.
Please implement your interrupt handler properly so that the genirq
error handling code can work and it is robust against things going
wrong in future. It's not like it's a huge amount of complex code.
OK. I thought that since the situation of int_satus
Re: [PATCH 06/10] ALSA: axd: add basic files for sending/receiving axd cmds
On Tue, Sep 01, 2015 at 11:46:19AM +0100, Qais Yousef wrote: > On 08/29/2015 11:18 AM, Mark Brown wrote: > >On Thu, Aug 27, 2015 at 04:40:09PM +0100, Qais Yousef wrote: Again, please delete unneeded contexts and leave blanks between paragraphs (I notice you've even been removing the blank lines from quoted material). > +#define PIPE_STARTED 1 > +#define PIPE_RUNNING 2 > >>>Why is the case with in place buffers not a simple zero iteration loop? > >>This is important when AXD is not consuming the data through I2S and > >>returning them to Linux. What we're trying to deal with here is the firmware > >>processed some data and expects Linux to consume whatever it has sent back > >>to it. We want to ensure that if the user suddenly stopped consuming this > >>data by closing the pipe to drop anything we receive back from AXD otherwise > >>the workqueue would block indefinitely waiting for the user that disappeared > >>to consume it causing a deadlock. > >That doesn't seem to address the question... > I'm sorry I don't understand your question then. Can you rephrase it please? Why don't we just always try to consume any buffers that are in flight? > >Why not just reuse the bufferq implementation if that's what you want to > >use? More generally most audio ring buffers just keep track of the last > >place read or written and don't bother with semaphores (why do we even > >need to block?). It's not just the semaphore you're using here but also > >some non-atomic variables accessed with memory barriers and mutexes all > >scattered over a very large block of code. It is far too much effort to > >reason about what the locking scheme is supposed to be here to determine > >if it is safe, and that's not going to get any easier when reviewing > >future changes. > We need to block because in the past at least the driver could work on > blocking mode and it would return the buffers back to Linux and there was no > guarantee that the reader and writer would be on the same rate. The worst > case assumption was that the writer and the reader could be 2 different > apps. For example an app getting data from network to AXD to be encoded and > another app reading the encoded data to store it on disk. And AXD supports > multi-pipeline, so more one of these operations could be happening at the > same time. I can't really connect the above with a need to block, sorry... I'm going to assume this was something to do with old non-standard external interfaces. signature.asc Description: Digital signature
