Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1212130 > unrolled thread

[PATCH 00/10] Add support for img AXD audio hardware decoder

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/10] Add support for img AXD audio hardware decoder Qais Yousef <qais.yousef@imgtec.com> - 2015-08-24 14:40 +0200
    [PATCH 04/10] ALSA: axd: add fw binary header manipulation files Qais Yousef <qais.yousef@imgtec.com> - 2015-08-24 14:40 +0200
    [PATCH 09/10] ALSA: axd: add alsa compress offload operations Qais Yousef <qais.yousef@imgtec.com> - 2015-08-24 14:50 +0200
    [PATCH 05/10] ALSA: axd: add buffers manipulation files Qais Yousef <qais.yousef@imgtec.com> - 2015-08-24 14:50 +0200
      Re: [PATCH 05/10] ALSA: axd: add buffers manipulation files Mark Brown <broonie@kernel.org> - 2015-08-26 20:50 +0200
        Re: [PATCH 05/10] ALSA: axd: add buffers manipulation files Qais Yousef <qais.yousef@imgtec.com> - 2015-08-27 16:30 +0200
          Re: [PATCH 05/10] ALSA: axd: add buffers manipulation files Mark Brown <broonie@kernel.org> - 2015-08-29 11:50 +0200
            Re: [PATCH 05/10] ALSA: axd: add buffers manipulation files Qais Yousef <qais.yousef@imgtec.com> - 2015-09-01 12:10 +0200
              Re: [PATCH 05/10] ALSA: axd: add buffers manipulation files Mark Brown <broonie@kernel.org> - 2015-09-03 14:40 +0200
    [PATCH 08/10] ALSA: axd: add low level AXD platform setup files Qais Yousef <qais.yousef@imgtec.com> - 2015-08-24 14:50 +0200
    [PATCH 10/10] ALSA: axd: add Makefile Qais Yousef <qais.yousef@imgtec.com> - 2015-08-24 14:50 +0200
    Re: [PATCH 00/10] Add support for img AXD audio hardware decoder Mark Brown <broonie@kernel.org> - 2015-08-26 20:10 +0200
      Re: [PATCH 00/10] Add support for img AXD audio hardware decoder Qais Yousef <qais.yousef@imgtec.com> - 2015-08-27 11:10 +0200
    Re: [PATCH 03/10] ALSA: add AXD Audio Processing IP alsa driver Mark Brown <broonie@kernel.org> - 2015-08-26 20:40 +0200
      Re: [PATCH 03/10] ALSA: add AXD Audio Processing IP alsa driver Qais Yousef <qais.yousef@imgtec.com> - 2015-08-27 14:20 +0200
        Re: [PATCH 03/10] ALSA: add AXD Audio Processing IP alsa driver Mark Brown <broonie@kernel.org> - 2015-08-27 17:40 +0200
          Re: [PATCH 03/10] ALSA: add AXD Audio Processing IP alsa driver Qais Yousef <qais.yousef@imgtec.com> - 2015-08-28 11:30 +0200
            Re: [PATCH 03/10] ALSA: add AXD Audio Processing IP alsa driver Mark Brown <broonie@kernel.org> - 2015-09-03 14:50 +0200
    Re: [PATCH 06/10] ALSA: axd: add basic files for sending/receiving  axd cmds Mark Brown <broonie@kernel.org> - 2015-08-26 21:20 +0200
      Re: [PATCH 06/10] ALSA: axd: add basic files for sending/receiving  axd cmds Qais Yousef <qais.yousef@imgtec.com> - 2015-08-27 17:50 +0200
        Re: [PATCH 06/10] ALSA: axd: add basic files for sending/receiving  axd cmds Mark Brown <broonie@kernel.org> - 2015-08-29 12:20 +0200
          Re: [PATCH 06/10] ALSA: axd: add basic files for sending/receiving  axd cmds Qais Yousef <qais.yousef@imgtec.com> - 2015-09-01 12:50 +0200
            Re: [PATCH 06/10] ALSA: axd: add basic files for sending/receiving  axd cmds Mark Brown <broonie@kernel.org> - 2015-09-03 14:50 +0200

Page 1 of 2  [1] 2  Next page →

#1212130 — [PATCH 00/10] Add support for img AXD audio hardware decoder

This patch series adds AXD Alsa Compress Offload SoC driver.

AXD is an audio hardware based on MIPS architecture that supports decoding,
encoding, GEQ, resampling, mixing and synchronisation. At the moment only
decoding support is added in hope to add the rest of the functionality on
top of that once this is accepted.

I divided the files into separate patches by functionality in hope it'll
make the reviewing process easier. Worth noting that a lot of the cmd interface
helper funtions in patch 7 are not used yet but will be as support for more
functionality is added later.

At the moment this code has been tested on Pistachio SoC using gstreamer patched
with the code in this link

	https://bugzilla.gnome.org/show_bug.cgi?id=743192

Qais Yousef (10):
  irqchip: irq-mips-gic: export gic_send_ipi
  dt: add img,axd.txt device tree binding document
  ALSA: add AXD Audio Processing IP alsa driver
  ALSA: axd: add fw binary header manipulation files
  ALSA: axd: add buffers manipulation files
  ALSA: axd: add basic files for sending/receiving axd cmds
  ALSA: axd: add cmd interface helper functions
  ALSA: axd: add low level AXD platform setup files
  ALSA: axd: add alsa compress offload operations
  ALSA: axd: add Makefile

 .../devicetree/bindings/sound/img,axd.txt          |   34 +
 drivers/irqchip/irq-mips-gic.c                     |    1 +
 sound/soc/Kconfig                                  |    1 +
 sound/soc/Makefile                                 |    1 +
 sound/soc/img/Kconfig                              |   11 +
 sound/soc/img/Makefile                             |    1 +
 sound/soc/img/axd/Makefile                         |   13 +
 sound/soc/img/axd/axd_alsa_ops.c                   |  211 ++
 sound/soc/img/axd/axd_api.h                        |  649 ++++
 sound/soc/img/axd/axd_buffers.c                    |  243 ++
 sound/soc/img/axd/axd_buffers.h                    |   74 +
 sound/soc/img/axd/axd_cmds.c                       |  102 +
 sound/soc/img/axd/axd_cmds.h                       |  532 ++++
 sound/soc/img/axd/axd_cmds_config.c                | 1235 ++++++++
 sound/soc/img/axd/axd_cmds_decoder_config.c        |  422 +++
 sound/soc/img/axd/axd_cmds_info.c                  | 1249 ++++++++
 sound/soc/img/axd/axd_cmds_internal.c              | 3264 ++++++++++++++++++++
 sound/soc/img/axd/axd_cmds_internal.h              |  317 ++
 sound/soc/img/axd/axd_cmds_pipes.c                 | 1387 +++++++++
 sound/soc/img/axd/axd_hdr.c                        |   64 +
 sound/soc/img/axd/axd_hdr.h                        |   24 +
 sound/soc/img/axd/axd_module.c                     |  742 +++++
 sound/soc/img/axd/axd_module.h                     |   83 +
 sound/soc/img/axd/axd_platform.h                   |   35 +
 sound/soc/img/axd/axd_platform_mips.c              |  416 +++
 25 files changed, 11111 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/img,axd.txt
 create mode 100644 sound/soc/img/Kconfig
 create mode 100644 sound/soc/img/Makefile
 create mode 100644 sound/soc/img/axd/Makefile
 create mode 100644 sound/soc/img/axd/axd_alsa_ops.c
 create mode 100644 sound/soc/img/axd/axd_api.h
 create mode 100644 sound/soc/img/axd/axd_buffers.c
 create mode 100644 sound/soc/img/axd/axd_buffers.h
 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_config.c
 create mode 100644 sound/soc/img/axd/axd_cmds_decoder_config.c
 create mode 100644 sound/soc/img/axd/axd_cmds_info.c
 create mode 100644 sound/soc/img/axd/axd_cmds_internal.c
 create mode 100644 sound/soc/img/axd/axd_cmds_internal.h
 create mode 100644 sound/soc/img/axd/axd_cmds_pipes.c
 create mode 100644 sound/soc/img/axd/axd_hdr.c
 create mode 100644 sound/soc/img/axd/axd_hdr.h
 create mode 100644 sound/soc/img/axd/axd_module.c
 create mode 100644 sound/soc/img/axd/axd_module.h
 create mode 100644 sound/soc/img/axd/axd_platform.h
 create mode 100644 sound/soc/img/axd/axd_platform_mips.c

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: linux-kernel@vger.kernel.org
Cc: linux-mips@linux-mips.org
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: Kumar Gala <galak@codeaurora.org>
Cc: devicetree@vger.kernel.org
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1212131 — [PATCH 04/10] ALSA: axd: add fw binary header manipulation files

These files provide functions to get information from the fw binary
header.

Signed-off-by: Qais Yousef <qais.yousef@imgtec.com>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: linux-kernel@vger.kernel.org
---
 sound/soc/img/axd/axd_hdr.c | 64 +++++++++++++++++++++++++++++++++++++++++++++
 sound/soc/img/axd/axd_hdr.h | 24 +++++++++++++++++
 2 files changed, 88 insertions(+)
 create mode 100644 sound/soc/img/axd/axd_hdr.c
 create mode 100644 sound/soc/img/axd/axd_hdr.h

diff --git a/sound/soc/img/axd/axd_hdr.c b/sound/soc/img/axd/axd_hdr.c
new file mode 100644
index 000000000000..7be3d11df120
--- /dev/null
+++ b/sound/soc/img/axd/axd_hdr.c
@@ -0,0 +1,64 @@
+/*
+ * 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.
+ *
+ * Helper functions to parse AXD Header in the firmware binary.
+ */
+#include <linux/kernel.h>
+
+#include "axd_api.h"
+#include "axd_hdr.h"
+
+static struct axd_hdr *hdr;
+
+static void dump_hdr(void)
+{
+	unsigned int offset = 0;
+	unsigned long address = (unsigned long)hdr;
+
+	pr_debug("header <0x%08lX>:\n", address);
+	while (offset <= sizeof(*hdr)) {
+		pr_debug("0x%08X\t", *(unsigned int *)(address+offset));
+		offset += 4;
+		if ((offset % (4*4)) == 0)
+			pr_debug("\n");
+	}
+	pr_debug("\n");
+}
+
+void axd_hdr_init(unsigned long address)
+{
+	hdr = (struct axd_hdr *)address;
+	dump_hdr();
+}
+
+unsigned long axd_hdr_get_pc(unsigned int thread)
+{
+	if (thread >= THREAD_COUNT)
+		return -1;
+	return hdr->thread_pc[thread];
+}
+
+unsigned long axd_hdr_get_cmdblock_offset(void)
+{
+	pr_debug("cmdblock_offset = 0x%08X\n", hdr->cmd_block_offset);
+	return hdr->cmd_block_offset;
+}
+
+char *axd_hdr_get_build_str(void)
+{
+	return hdr->build_str;
+}
+
+unsigned long axd_hdr_get_log_offset(void)
+{
+	return hdr->log_offset;
+}
diff --git a/sound/soc/img/axd/axd_hdr.h b/sound/soc/img/axd/axd_hdr.h
new file mode 100644
index 000000000000..dc0b1e3be5a2
--- /dev/null
+++ b/sound/soc/img/axd/axd_hdr.h
@@ -0,0 +1,24 @@
+/*
+ * 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.
+ *
+ * Helper functions to parse AXD Header in the firmware binary
+ */
+#ifndef AXD_HDR_H_
+#define AXD_HDR_H_
+
+void axd_hdr_init(unsigned long address);
+unsigned long axd_hdr_get_pc(unsigned int thread);
+unsigned long axd_hdr_get_cmdblock_offset(void);
+char *axd_hdr_get_build_str(void);
+unsigned long axd_hdr_get_log_offset(void);
+
+#endif /* AXD_HDR_H_ */
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1212132 — [PATCH 09/10] ALSA: axd: add alsa compress offload operations

Add implementation of alsa compress offload operations.
At the moment we only support playback only.

Signed-off-by: Qais Yousef <qais.yousef@imgtec.com>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: linux-kernel@vger.kernel.org
---
 sound/soc/img/axd/axd_alsa_ops.c | 211 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 211 insertions(+)
 create mode 100644 sound/soc/img/axd/axd_alsa_ops.c

diff --git a/sound/soc/img/axd/axd_alsa_ops.c b/sound/soc/img/axd/axd_alsa_ops.c
new file mode 100644
index 000000000000..91e17119b306
--- /dev/null
+++ b/sound/soc/img/axd/axd_alsa_ops.c
@@ -0,0 +1,211 @@
+/*
+ * Copyright (C) 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 ALSA Compressed ops
+ */
+#include <sound/compress_driver.h>
+#include <sound/soc.h>
+
+#include "axd_cmds.h"
+#include "axd_module.h"
+
+static struct axd_dev *get_axd_from_cstream(struct snd_compr_stream *cstream)
+{
+	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
+	return snd_soc_platform_get_drvdata(rtd->platform);
+}
+
+static int copied_total;
+
+static int axd_compr_open(struct snd_compr_stream *cstream)
+{
+	struct axd_dev *axd = get_axd_from_cstream(cstream);
+
+	axd_cmd_output_set_sink(&axd->cmd, 0, 1);
+	return axd_cmd_inpipe_start(&axd->cmd, 0);
+}
+
+static int axd_compr_free(struct snd_compr_stream *cstream)
+{
+	struct axd_dev *axd = get_axd_from_cstream(cstream);
+
+	axd_cmd_inpipe_stop(&axd->cmd, 0);
+	copied_total = 0;
+
+	return 0;
+}
+
+static int axd_compr_set_params(struct snd_compr_stream *cstream,
+				struct snd_compr_params *params)
+{
+	int ret;
+	struct axd_dev *axd = get_axd_from_cstream(cstream);
+
+	ret = axd_cmd_input_set_decoder_params(&axd->cmd, 0, &params->codec);
+	if (ret)
+		return -EINVAL;
+	return 0;
+}
+
+static int axd_compr_get_params(struct snd_compr_stream *cstream,
+				struct snd_codec *params)
+{
+	int ret;
+	struct axd_dev *axd = get_axd_from_cstream(cstream);
+
+	ret = axd_cmd_input_get_decoder_params(&axd->cmd, 0, params);
+	if (ret)
+		return -EIO;
+	return 0;
+}
+
+static int axd_compr_trigger(struct snd_compr_stream *cstream, int cmd)
+{
+	struct axd_dev *axd = get_axd_from_cstream(cstream);
+
+	if (cmd == SND_COMPR_TRIGGER_PARTIAL_DRAIN ||
+		cmd == SND_COMPR_TRIGGER_DRAIN) {
+		/* stop to send EOS which will cause the stream to be drained */
+		axd_cmd_inpipe_stop(&axd->cmd, 0);
+
+		/*
+		 * start again, repeating if EAGAIN is returned meaning we're
+		 * being drained
+		 */
+		while (axd_cmd_inpipe_start(&axd->cmd, 0) == -EAGAIN)
+			cpu_relax();
+
+		copied_total = 0;
+	}
+	return 0;
+}
+
+static int axd_compr_pointer(struct snd_compr_stream *cstream,
+			     struct snd_compr_tstamp *tstamp)
+{
+	tstamp->copied_total = copied_total;
+	return 0;
+}
+
+static int axd_compr_copy(struct snd_compr_stream *cstream, char __user *buf,
+			  size_t count)
+{
+	struct axd_dev *axd = get_axd_from_cstream(cstream);
+	int ret;
+
+	ret = axd_cmd_send_buffer(&axd->cmd, 0, buf, count);
+	if (ret < 0) {
+		dev_err(axd->dev, "failed to write buffer %d\n", ret);
+		return ret;
+	}
+	copied_total += ret;
+
+	return ret;
+}
+
+static int axd_compr_get_caps(struct snd_compr_stream *cstream,
+			      struct snd_compr_caps *caps)
+{
+	struct axd_dev *axd = get_axd_from_cstream(cstream);
+
+	caps->min_fragment_size = 1024*2;
+	caps->max_fragment_size = 1024*2;
+	caps->min_fragments= 1;
+	caps->max_fragments= 5;
+
+	axd_cmd_get_decoders(&axd->cmd, caps);
+
+	return 0;
+}
+
+static int axd_compr_get_codec_caps(struct snd_compr_stream *cstream,
+				    struct snd_compr_codec_caps *codec)
+{
+	switch (codec->codec) {
+	case SND_AUDIOCODEC_PCM:
+		codec->num_descriptors = 1;
+		codec->descriptor[0].max_ch = 2;
+		codec->descriptor[0].sample_rates[0] = 96000;
+		codec->descriptor[0].sample_rates[1] = 64000;
+		codec->descriptor[0].sample_rates[2] = 48000;
+		codec->descriptor[0].sample_rates[3] = 44100;
+		codec->descriptor[0].sample_rates[4] = 32000;
+		codec->descriptor[0].sample_rates[5] = 16000;
+		codec->descriptor[0].sample_rates[6] = 8000;
+		codec->descriptor[0].num_sample_rates = 7;
+		codec->descriptor[0].num_bitrates = 0;
+		codec->descriptor[0].profiles = 0;
+		codec->descriptor[0].modes = 0;
+		codec->descriptor[0].formats = 0;
+		break;
+	case SND_AUDIOCODEC_MP3:
+		codec->num_descriptors = 1;
+		codec->descriptor[0].max_ch = 2;
+		codec->descriptor[0].num_sample_rates = 0;
+		codec->descriptor[0].num_bitrates = 0;
+		codec->descriptor[0].profiles = 0;
+		codec->descriptor[0].modes = 0;
+		codec->descriptor[0].formats = 0;
+		break;
+	case SND_AUDIOCODEC_AAC:
+		codec->num_descriptors = 1;
+		codec->descriptor[0].max_ch = 6;
+		codec->descriptor[0].num_sample_rates = 0;
+		codec->descriptor[0].num_bitrates = 0;
+		codec->descriptor[0].profiles = 0;
+		codec->descriptor[0].modes = SND_AUDIOMODE_AAC_MAIN |
+			SND_AUDIOMODE_AAC_LC | SND_AUDIOMODE_AAC_SSR;
+		codec->descriptor[0].formats = SND_AUDIOSTREAMFORMAT_MP2ADTS |
+			SND_AUDIOSTREAMFORMAT_MP4ADTS | SND_AUDIOSTREAMFORMAT_ADIF |
+			SND_AUDIOSTREAMFORMAT_RAW;
+		break;
+	case SND_AUDIOCODEC_VORBIS:
+		codec->num_descriptors = 0;
+		break;
+	case SND_AUDIOCODEC_FLAC:
+		codec->num_descriptors = 1;
+		codec->descriptor[0].max_ch = 6;
+		codec->descriptor[0].num_sample_rates = 0;
+		codec->descriptor[0].num_bitrates = 0;
+		codec->descriptor[0].profiles = 0;
+		codec->descriptor[0].modes = 0;
+		codec->descriptor[0].formats = SND_AUDIOSTREAMFORMAT_FLAC;
+		break;
+	case SND_AUDIOCODEC_WMA:
+		codec->num_descriptors = 1;
+		codec->descriptor[0].max_ch = 6;
+		codec->descriptor[0].num_sample_rates = 0;
+		codec->descriptor[0].num_bitrates = 0;
+		codec->descriptor[0].profiles = SND_AUDIOPROFILE_WMA7 |
+			SND_AUDIOPROFILE_WMA8 | SND_AUDIOPROFILE_WMA9 |
+			SND_AUDIOPROFILE_WMA10;
+		codec->descriptor[0].modes = 0;
+		codec->descriptor[0].formats = SND_AUDIOSTREAMFORMAT_WMA_NOASF_HDR;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+struct snd_compr_ops axd_compr_ops = {
+	.open		= axd_compr_open,
+	.free		= axd_compr_free,
+	.set_params	= axd_compr_set_params,
+	.get_params	= axd_compr_get_params,
+	.trigger	= axd_compr_trigger,
+	.pointer	= axd_compr_pointer,
+	.copy		= axd_compr_copy,
+	.get_caps	= axd_compr_get_caps,
+	.get_codec_caps	= axd_compr_get_codec_caps
+};
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1212138 — [PATCH 05/10] ALSA: axd: add buffers manipulation files

These files support initilising and managing access to the shared
buffers area in memory that is used to exchange data between AXD
and linux.

Signed-off-by: Qais Yousef <qais.yousef@imgtec.com>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: linux-kernel@vger.kernel.org
---
 sound/soc/img/axd/axd_buffers.c | 243 ++++++++++++++++++++++++++++++++++++++++
 sound/soc/img/axd/axd_buffers.h |  74 ++++++++++++
 2 files changed, 317 insertions(+)
 create mode 100644 sound/soc/img/axd/axd_buffers.c
 create mode 100644 sound/soc/img/axd/axd_buffers.h

diff --git a/sound/soc/img/axd/axd_buffers.c b/sound/soc/img/axd/axd_buffers.c
new file mode 100644
index 000000000000..891344a806f6
--- /dev/null
+++ b/sound/soc/img/axd/axd_buffers.c
@@ -0,0 +1,243 @@
+/*
+ * 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 generic buffer management API.
+ */
+#include <linux/err.h>
+#include <linux/slab.h>
+
+#include "axd_buffers.h"
+
+/**
+ * axd_buffer_init - sets up axd buffer as a pool of fixed sized buffers.
+ * @address: starting address of the buffer as set up in the system
+ * @total_size: total size of available buffer
+ * @element_size: size of each buffer element
+ *
+ * axd_buffer_t *buffer is a memory pool of size @element_size and starting at
+ * address @address and of @total_size size.
+ */
+static int bufferq_init(struct axd_bufferq *bufferq, const char *name,
+			char *address, unsigned int num_elements,
+			unsigned int element_size, unsigned int nonblock)
+{
+	int i;
+	char **queue;
+	unsigned int *size;
+
+	strncpy(bufferq->name, name, 16);
+	bufferq->stride = element_size;
+	bufferq->max = num_elements;
+	bufferq->rd_idx = 0;
+	bufferq->wr_idx = 0;
+	bufferq->nonblock = nonblock;
+	queue = kcalloc(num_elements, sizeof(char *), GFP_KERNEL);
+	if (!queue)
+		return -ENOMEM;
+	bufferq->queue = queue;
+	size = kcalloc(num_elements, sizeof(unsigned int), GFP_KERNEL);
+	if (!size) {
+		kfree(queue);
+		bufferq->queue = NULL;
+		return -ENOMEM;
+	}
+	bufferq->size = size;
+	/*
+	 * setup the queue with all available buffer addresses if the base
+	 * address is passed. Set it up as emptry if base address is NULL.
+	 */
+	if (address) {
+		for (i = 0; i < num_elements; i++) {
+			queue[i] = address + (element_size * i);
+			size[i] = element_size;
+		}
+		sema_init(&bufferq->rd_sem, num_elements);
+		sema_init(&bufferq->wr_sem, 0);
+	} else {
+		for (i = 0; i < num_elements; i++) {
+			queue[i] = NULL;
+			size[i] = element_size;
+		}
+		sema_init(&bufferq->rd_sem, 0);
+		sema_init(&bufferq->wr_sem, num_elements);
+	}
+	spin_lock_init(&bufferq->q_rdlock);
+	spin_lock_init(&bufferq->q_wrlock);
+	pr_debug("Initialized %s of %d elements of size %d bytes\n",
+					name, num_elements, element_size);
+	pr_debug("Address of %s: 0x%08X\n", name, (unsigned int)bufferq);
+	return 0;
+}
+
+int axd_bufferq_init(struct axd_bufferq *bufferq, const char *name,
+			char *address, unsigned int num_elements,
+			unsigned int element_size, unsigned int nonblock)
+{
+	return bufferq_init(bufferq,
+			name, address, num_elements, element_size, nonblock);
+}
+
+int axd_bufferq_init_empty(struct axd_bufferq *bufferq, const char *name,
+			unsigned int num_elements, unsigned int element_size,
+			unsigned int nonblock)
+{
+	return bufferq_init(bufferq,
+			name, NULL, num_elements, element_size, nonblock);
+}
+
+void axd_bufferq_clear(struct axd_bufferq *bufferq)
+{
+	kfree(bufferq->queue);
+	kfree(bufferq->size);
+	bufferq->queue = NULL;
+	bufferq->size = NULL;
+}
+
+/**
+ * axd_buffer_take - returns a valid buffer pointer
+ * @buffer: the buffers pool to be accessed
+ *
+ * This function will go into interruptible sleep if the pool is empty.
+ */
+char *axd_bufferq_take(struct axd_bufferq *bufferq, int *buf_size)
+{
+	char *buf;
+	int ret;
+
+	if (!bufferq->queue)
+		return NULL;
+
+	pr_debug("--(%s)-- taking new buffer\n", bufferq->name);
+	if (bufferq->nonblock) {
+		ret = down_trylock(&bufferq->rd_sem);
+		if (ret)
+			return ERR_PTR(-EAGAIN);
+
+	} else {
+		ret = down_interruptible(&bufferq->rd_sem);
+		if (ret)
+			return ERR_PTR(-ERESTARTSYS);
+		if (bufferq->abort_take) {
+			bufferq->abort_take = 0;
+			return ERR_PTR(-ERESTARTSYS);
+		}
+	}
+	/*
+	 * must ensure we have one access at a time to the queue and rd_idx
+	 * to be preemption and SMP safe
+	 * Sempahores will ensure that we will only read after a complete write
+	 * has finished, so we will never read and write from the same location.
+	 */
+	spin_lock(&bufferq->q_rdlock);
+	buf = bufferq->queue[bufferq->rd_idx];
+	if (buf_size)
+		*buf_size = bufferq->size[bufferq->rd_idx];
+	bufferq->rd_idx++;
+	if (bufferq->rd_idx >= bufferq->max)
+		bufferq->rd_idx = 0;
+	spin_unlock(&bufferq->q_rdlock);
+	up(&bufferq->wr_sem);
+	pr_debug("--(%s)-- took buffer <0x%08X>\n", bufferq->name,
+							(unsigned int)buf);
+	return buf;
+}
+
+/**
+ * axd_buffer_put - returns a buffer to the pool.
+ * @buffer: the buffers pool to be accessed
+ * @buf: the buffer to be returned.
+ *
+ * This function will go into interruptible sleep if the pool is full.
+ */
+int axd_bufferq_put(struct axd_bufferq *bufferq, char *buf, int buf_size)
+{
+	int ret;
+
+	if (!bufferq->queue)
+		return 0;
+
+	if (buf_size < 0)
+		buf_size = bufferq->stride;
+
+	pr_debug("++(%s)++ returning buffer\n", bufferq->name);
+	if (bufferq->nonblock) {
+		ret = down_trylock(&bufferq->wr_sem);
+		if (ret)
+			return -EAGAIN;
+
+	} else {
+		ret = down_interruptible(&bufferq->wr_sem);
+		if (ret)
+			return -ERESTARTSYS;
+		if (bufferq->abort_put) {
+			bufferq->abort_put = 0;
+			return -ERESTARTSYS;
+		}
+	}
+	/*
+	 * must ensure we have one access at a time to the queue and wr_idx
+	 * to be preemption and SMP safe.
+	 * Semaphores will ensure that we only write after a complete read has
+	 * finished, so we will never write and read from the same location.
+	 */
+	spin_lock(&bufferq->q_wrlock);
+	bufferq->queue[bufferq->wr_idx] = buf;
+	bufferq->size[bufferq->wr_idx] = buf_size;
+	bufferq->wr_idx++;
+	if (bufferq->wr_idx >= bufferq->max)
+		bufferq->wr_idx = 0;
+	spin_unlock(&bufferq->q_wrlock);
+	up(&bufferq->rd_sem);
+	pr_debug("++(%s)++ returned buffer <0x%08X>\n", bufferq->name,
+							(unsigned int)buf);
+	return 0;
+}
+
+int axd_bufferq_is_full(struct axd_bufferq *bufferq)
+{
+	int ret;
+	/*
+	 * if we can't put a buffer, then we're full.
+	 */
+	ret = down_trylock(&bufferq->wr_sem);
+	if (!ret)
+		up(&bufferq->wr_sem);
+	return ret;
+}
+
+int axd_bufferq_is_empty(struct axd_bufferq *bufferq)
+{
+	int ret;
+	/*
+	 * if we can't take more buffers, then its empty.
+	 */
+	ret = down_trylock(&bufferq->rd_sem);
+	if (!ret)
+		up(&bufferq->rd_sem);
+	return ret;
+}
+
+void axd_bufferq_abort_take(struct axd_bufferq *bufferq)
+{
+	if (axd_bufferq_is_empty(bufferq)) {
+		bufferq->abort_take = 1;
+		up(&bufferq->rd_sem);
+	}
+}
+
+void axd_bufferq_abort_put(struct axd_bufferq *bufferq)
+{
+	if (axd_bufferq_is_full(bufferq)) {
+		bufferq->abort_put = 1;
+		up(&bufferq->wr_sem);
+	}
+}
diff --git a/sound/soc/img/axd/axd_buffers.h b/sound/soc/img/axd/axd_buffers.h
new file mode 100644
index 000000000000..c585044a8f1f
--- /dev/null
+++ b/sound/soc/img/axd/axd_buffers.h
@@ -0,0 +1,74 @@
+/*
+ * 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 generic buffer management API.
+ */
+#ifndef AXD_BUFFERS_H_
+#define AXD_BUFFERS_H_
+
+#include <linux/semaphore.h>
+#include <linux/spinlock.h>
+
+/**
+ * struct axd_bufferq - axd buffer management structure
+ * @name:	name of the buffer queue
+ * @stride:	the space between buffers in memory
+ * @max:	total number of buffers this queue can handle
+ * @rd_idx:	read index of the circular buffer
+ * @wr_idx:	write index of the circular buffer
+ * @rd_sem:	semaphore to block when full
+ * @wr_sem:	semaphore to block when empty
+ * @q_rdlock:	smp critical section protection for reads
+ * @q_wrlock:	smp critical section protection for writes
+ * @queue:	array of pointers to buffer addresses
+ * @size:	array of buffer's actual amount of data it has inside or it can
+ *		store.
+ * @nonblock:	return an error instead of block when empty/full
+ * @abort_take:	abort any pending blocked take operation
+ * @abort_put:	abort any pending blocked put operation
+ *
+ * axd_bufferq takes a contiguous memory region and divides it into smaller
+ * buffers regions of equal size and represents it as a queue. To avoid
+ * excessive locking it's done as a circular buffer queue.
+ */
+struct axd_bufferq {
+	char name[16];
+	unsigned int stride;
+	unsigned int max;
+	unsigned int rd_idx;
+	unsigned int wr_idx;
+	struct semaphore rd_sem;
+	struct semaphore wr_sem;
+	spinlock_t q_rdlock;
+	spinlock_t q_wrlock;
+	char **queue;
+	unsigned int *size;
+	unsigned int nonblock;
+	unsigned int abort_take;
+	unsigned int abort_put;
+};
+
+int axd_bufferq_init(struct axd_bufferq *bufferq, const char *name,
+			char *address, unsigned int num_elements,
+			unsigned int element_size, unsigned int nonblock);
+int axd_bufferq_init_empty(struct axd_bufferq *bufferq, const char *name,
+			unsigned int num_elements, unsigned int element_size,
+			unsigned int nonblock);
+void axd_bufferq_clear(struct axd_bufferq *bufferq);
+char *axd_bufferq_take(struct axd_bufferq *bufferq, int *buf_size);
+int axd_bufferq_put(struct axd_bufferq *bufferq, char *buf, int buf_size);
+int axd_bufferq_is_full(struct axd_bufferq *bufferq);
+int axd_bufferq_is_empty(struct axd_bufferq *bufferq);
+void axd_bufferq_abort_take(struct axd_bufferq *bufferq);
+void axd_bufferq_abort_put(struct axd_bufferq *bufferq);
+
+#endif /* AXD_BUFFERS_H_ */
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1214129 — Re: [PATCH 05/10] ALSA: axd: add buffers manipulation files

[Multipart message — attachments visible in raw view] — view raw

On Mon, Aug 24, 2015 at 01:39:14PM +0100, Qais Yousef wrote:

> +	/*
> +	 * must ensure we have one access at a time to the queue and rd_idx
> +	 * to be preemption and SMP safe
> +	 * Sempahores will ensure that we will only read after a complete write
> +	 * has finished, so we will never read and write from the same location.
> +	 */

In what way will sempahores ensure that we will only read after a
complete write?

> +	buf = bufferq->queue[bufferq->rd_idx];

So buffers are always retired in the same order that they are acquired?

> +int axd_bufferq_put(struct axd_bufferq *bufferq, char *buf, int buf_size)
> +{
> +	int ret;
> +
> +	if (!bufferq->queue)
> +		return 0;
> +
> +	if (buf_size < 0)
> +		buf_size = bufferq->stride;

We've got strides as well?  What is that?

> +void axd_bufferq_abort_take(struct axd_bufferq *bufferq)
> +{
> +	if (axd_bufferq_is_empty(bufferq)) {
> +		bufferq->abort_take = 1;
> +		up(&bufferq->rd_sem);
> +	}
> +}
> +
> +void axd_bufferq_abort_put(struct axd_bufferq *bufferq)
> +{
> +	if (axd_bufferq_is_full(bufferq)) {
> +		bufferq->abort_put = 1;
> +		up(&bufferq->wr_sem);
> +	}
> +}

These look *incredibly* racy.  Why are they here and why are they safe?

[toc] | [prev] | [next] | [standalone]

#1214602 — Re: [PATCH 05/10] ALSA: axd: add buffers manipulation files

On 08/26/2015 07:43 PM, Mark Brown wrote:
> On Mon, Aug 24, 2015 at 01:39:14PM +0100, Qais Yousef wrote:
>
>> +	/*
>> +	 * must ensure we have one access at a time to the queue and rd_idx
>> +	 * to be preemption and SMP safe
>> +	 * Sempahores will ensure that we will only read after a complete write
>> +	 * has finished, so we will never read and write from the same location.
>> +	 */
> In what way will sempahores ensure that we will only read after a
> complete write?

This comment needs fixing. What it is trying to say is that if we 
reached this point of the code then we're certainly allowed to modify 
the buffer queue and {rd, wr}_idx because the semaphore would have gone 
to sleep otherwise if the queue is full/empty.

Should I just remove the reference to Semaphores from the comment or 
worth rephrasing it?

Would it be better to rename {rd, wr}_{idx, sem} to {take, put}_{idx, sem}?

>
>> +	buf = bufferq->queue[bufferq->rd_idx];
> So buffers are always retired in the same order that they are acquired?

I don't think I get you here. axd_bufferq_take() and axd_bufferq_put() 
could be called in any order.

What this code is trying to do is make a contiguous memory area behave 
as a ring buffer. Then this ring buffer behave as a queue. We use 
semaphore counts to control how many are available to take/put. rd_idx 
and wr_idx should always point at the next location to take/put from/to.

Does this help answering your question?

>
>> +int axd_bufferq_put(struct axd_bufferq *bufferq, char *buf, int buf_size)
>> +{
>> +	int ret;
>> +
>> +	if (!bufferq->queue)
>> +		return 0;
>> +
>> +	if (buf_size < 0)
>> +		buf_size = bufferq->stride;
> We've got strides as well?  What is that?

We break the contiguous buffer area allocated for us into smaller 
buffers separated by (or of size) stride.

>
>> +void axd_bufferq_abort_take(struct axd_bufferq *bufferq)
>> +{
>> +	if (axd_bufferq_is_empty(bufferq)) {
>> +		bufferq->abort_take = 1;
>> +		up(&bufferq->rd_sem);
>> +	}
>> +}
>> +
>> +void axd_bufferq_abort_put(struct axd_bufferq *bufferq)
>> +{
>> +	if (axd_bufferq_is_full(bufferq)) {
>> +		bufferq->abort_put = 1;
>> +		up(&bufferq->wr_sem);
>> +	}
>> +}
> These look *incredibly* racy.  Why are they here and why are they safe?

If we want to restart the firmware we will need to abort any blocking 
reads or writes for the user space to react. I also needed that to 
implement nonblocking access in user space when this was a sysfs based 
driver. It was important then to implement omx IL component correctly.

Do I need to support nonblock reads and writes in ALSA? If I use SIGKILL 
as you suggested in the other email when restarting and nonblock is not 
important then I can remove this.

I just looked at the code history and I was in the past sending SIGBUS 
to the user if we needed to restart then I opted to the abort approach 
as it will allow the application to terminate gracefully as it should 
get EOF instead then and hide the need to restart the firmware in a 
better way. What do you think?

Thanks,
Qais
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1215743 — Re: [PATCH 05/10] ALSA: axd: add buffers manipulation files

[Multipart message — attachments visible in raw view] — view raw

On Thu, Aug 27, 2015 at 03:21:17PM +0100, Qais Yousef wrote:
> On 08/26/2015 07:43 PM, Mark Brown wrote:
> >On Mon, Aug 24, 2015 at 01:39:14PM +0100, Qais Yousef wrote:

> >>+	/*
> >>+	 * must ensure we have one access at a time to the queue and rd_idx
> >>+	 * to be preemption and SMP safe
> >>+	 * Sempahores will ensure that we will only read after a complete write
> >>+	 * has finished, so we will never read and write from the same location.
> >>+	 */

> >In what way will sempahores ensure that we will only read after a
> >complete write?

> This comment needs fixing. What it is trying to say is that if we reached
> this point of the code then we're certainly allowed to modify the buffer
> queue and {rd, wr}_idx because the semaphore would have gone to sleep
> otherwise if the queue is full/empty.

> Should I just remove the reference to Semaphores from the comment or worth
> rephrasing it?

Any comments need to be comprehensible.

> Would it be better to rename {rd, wr}_{idx, sem} to {take, put}_{idx, sem}?

I'm not sure that helps to be honest, the main issue is that the scheme
is fairly complex and unexplained.

> >>+	buf = bufferq->queue[bufferq->rd_idx];

> >So buffers are always retired in the same order that they are acquired?

> I don't think I get you here. axd_bufferq_take() and axd_bufferq_put() could
> be called in any order.

Retiring buffers in the order they are acquired means that buffers are
always freed in the same order they are acquired, you can't free one
buffer before another that was acquired first.

> What this code is trying to do is make a contiguous memory area behave as a
> ring buffer. Then this ring buffer behave as a queue. We use semaphore
> counts to control how many are available to take/put. rd_idx and wr_idx
> should always point at the next location to take/put from/to.

> Does this help answering your question?

No.  Why are we doing this?  Essentially all ALSA buffers are ring
buffers handled in blocks, why does this one need this complex locking
scheme?

> >>+void axd_bufferq_abort_put(struct axd_bufferq *bufferq)
> >>+{
> >>+	if (axd_bufferq_is_full(bufferq)) {
> >>+		bufferq->abort_put = 1;
> >>+		up(&bufferq->wr_sem);
> >>+	}
> >>+}

> >These look *incredibly* racy.  Why are they here and why are they safe?

> If we want to restart the firmware we will need to abort any blocking reads
> or writes for the user space to react. I also needed that to implement

I'm not questioning what the functionns are doing, I'm questioning their
implementation - it doesn't look like they are safe or reliable.  They
just set a flag, relying on something else to notice that the flag has
been set and act appropriately before it goes on and corrupts data.
That just screams concurrency issues.

> nonblocking access in user space when this was a sysfs based driver. It was
> important then to implement omx IL component correctly.

Nobody cares about OMX ILs in mainline or sysfs based interfaces.

> Do I need to support nonblock reads and writes in ALSA? If I use SIGKILL as
> you suggested in the other email when restarting and nonblock is not
> important then I can remove this.

It would be better to support non blocking access.

[toc] | [prev] | [next] | [standalone]

#1216740 — Re: [PATCH 05/10] ALSA: axd: add buffers manipulation files

On 08/29/2015 10:47 AM, Mark Brown wrote:
> On Thu, Aug 27, 2015 at 03:21:17PM +0100, Qais Yousef wrote:
>> On 08/26/2015 07:43 PM, Mark Brown wrote:
>>> On Mon, Aug 24, 2015 at 01:39:14PM +0100, Qais Yousef wrote:
>>>> +	/*
>>>> +	 * must ensure we have one access at a time to the queue and rd_idx
>>>> +	 * to be preemption and SMP safe
>>>> +	 * Sempahores will ensure that we will only read after a complete write
>>>> +	 * has finished, so we will never read and write from the same location.
>>>> +	 */
>>> In what way will sempahores ensure that we will only read after a
>>> complete write?
>> This comment needs fixing. What it is trying to say is that if we reached
>> this point of the code then we're certainly allowed to modify the buffer
>> queue and {rd, wr}_idx because the semaphore would have gone to sleep
>> otherwise if the queue is full/empty.
>> Should I just remove the reference to Semaphores from the comment or worth
>> rephrasing it?
> Any comments need to be comprehensible.
>
>> Would it be better to rename {rd, wr}_{idx, sem} to {take, put}_{idx, sem}?
> I'm not sure that helps to be honest, the main issue is that the scheme
> is fairly complex and unexplained.
>
>>>> +	buf = bufferq->queue[bufferq->rd_idx];
>>> So buffers are always retired in the same order that they are acquired?
>> I don't think I get you here. axd_bufferq_take() and axd_bufferq_put() could
>> be called in any order.
> Retiring buffers in the order they are acquired means that buffers are
> always freed in the same order they are acquired, you can't free one
> buffer before another that was acquired first.
>> What this code is trying to do is make a contiguous memory area behave as a
>> ring buffer. Then this ring buffer behave as a queue. We use semaphore
>> counts to control how many are available to take/put. rd_idx and wr_idx
>> should always point at the next location to take/put from/to.
>> Does this help answering your question?
> No.  Why are we doing this?  Essentially all ALSA buffers are ring
> buffers handled in blocks, why does this one need this complex locking
> scheme?

There are 2 sides to this. The ALSA/driver iface and the driver/firmware 
one. The ALSA/driver iface is called from ALSA ops but the 
driver/firmware is handled by the interrupt and workqueues. The code is 
trying to deal with this concurrency. Also once AXD consumed a buffer it 
sends back an interrupt to the driver that it can reuse it, there's no 
guarantee that this returned buffer is in the same order it was sent.

I hear you though. Let me see how I can simplify this :-)

>>>> +void axd_bufferq_abort_put(struct axd_bufferq *bufferq)
>>>> +{
>>>> +	if (axd_bufferq_is_full(bufferq)) {
>>>> +		bufferq->abort_put = 1;
>>>> +		up(&bufferq->wr_sem);
>>>> +	}
>>>> +}
>>> These look *incredibly* racy.  Why are they here and why are they safe?
>> If we want to restart the firmware we will need to abort any blocking reads
>> or writes for the user space to react. I also needed that to implement
> I'm not questioning what the functionns are doing, I'm questioning their
> implementation - it doesn't look like they are safe or reliable.  They
> just set a flag, relying on something else to notice that the flag has
> been set and act appropriately before it goes on and corrupts data.
> That just screams concurrency issues.

OK. I'll see how I can rework the code to address all of your comments.

Thanks,
Qais

>> nonblocking access in user space when this was a sysfs based driver. It was
>> important then to implement omx IL component correctly.
> Nobody cares about OMX ILs in mainline or sysfs based interfaces.
>
>> Do I need to support nonblock reads and writes in ALSA? If I use SIGKILL as
>> you suggested in the other email when restarting and nonblock is not
>> important then I can remove this.
> It would be better to support non blocking access.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1218231 — Re: [PATCH 05/10] ALSA: axd: add buffers manipulation files

[Multipart message — attachments visible in raw view] — view raw

On Tue, Sep 01, 2015 at 11:00:42AM +0100, Qais Yousef wrote:
> On 08/29/2015 10:47 AM, Mark Brown wrote:

Please delete unneeded context from replies, it makes it easier to find
the new content you have added.  Please also leave blank lines between
paragraphs, it makes it much easier to read messages.

> >>What this code is trying to do is make a contiguous memory area behave as a
> >>ring buffer. Then this ring buffer behave as a queue. We use semaphore
> >>counts to control how many are available to take/put. rd_idx and wr_idx
> >>should always point at the next location to take/put from/to.
> >>Does this help answering your question?

> >No.  Why are we doing this?  Essentially all ALSA buffers are ring
> >buffers handled in blocks, why does this one need this complex locking
> >scheme?

> There are 2 sides to this. The ALSA/driver iface and the driver/firmware
> one. The ALSA/driver iface is called from ALSA ops but the driver/firmware
> is handled by the interrupt and workqueues. The code is trying to deal with
> this concurrency. Also once AXD consumed a buffer it sends back an interrupt

This is just the same as any other ALSA device...

> to the driver that it can reuse it, there's no guarantee that this returned
> buffer is in the same order it was sent.

If that's the case I'm not sure the code is correct - it seemed to have
assumptions that the buffers were going to be retired in the order.

[toc] | [prev] | [next] | [standalone]

#1212140 — [PATCH 08/10] ALSA: axd: add low level AXD platform setup files

At the moment AXD runs on MIPS cores only. These files provide
basic functionality to prepare AXD f/w to bootstrap itself and
do low level interrupt/kick when being initialised from a mips
core.

Signed-off-by: Qais Yousef <qais.yousef@imgtec.com>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: linux-kernel@vger.kernel.org
---
 sound/soc/img/axd/axd_platform.h      |  35 +++
 sound/soc/img/axd/axd_platform_mips.c | 416 ++++++++++++++++++++++++++++++++++
 2 files changed, 451 insertions(+)
 create mode 100644 sound/soc/img/axd/axd_platform.h
 create mode 100644 sound/soc/img/axd/axd_platform_mips.c

diff --git a/sound/soc/img/axd/axd_platform.h b/sound/soc/img/axd/axd_platform.h
new file mode 100644
index 000000000000..f9cc3c308a4a
--- /dev/null
+++ b/sound/soc/img/axd/axd_platform.h
@@ -0,0 +1,35 @@
+/*
+ * 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.
+ *
+ * Platform Specific helper functions.
+ */
+#ifndef AXD_PLATFORM_H_
+#define AXD_PLATFORM_H_
+#include "axd_module.h"
+
+void axd_platform_init(struct axd_dev *axd);
+void axd_platform_set_pc(unsigned long pc);
+int axd_platform_start(void);
+void axd_platform_stop(void);
+unsigned int axd_platform_num_threads(void);
+void axd_platform_kick(void);
+void axd_platform_irq_ack(void);
+void axd_platform_print_regs(void);
+
+/*
+ * protect against simultaneous access to shared memory mapped registers area
+ * between axd and the host
+ */
+unsigned long axd_platform_lock(void);
+void axd_platform_unlock(unsigned long flags);
+
+#endif /* AXD_PLATFORM_H_ */
diff --git a/sound/soc/img/axd/axd_platform_mips.c b/sound/soc/img/axd/axd_platform_mips.c
new file mode 100644
index 000000000000..ac1cf5eb8a64
--- /dev/null
+++ b/sound/soc/img/axd/axd_platform_mips.c
@@ -0,0 +1,416 @@
+/*
+ * 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.
+ *
+ * This file implements running AXD as a single VPE along side linux on the same
+ * core.
+ */
+#include <linux/cpu.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/irqchip/mips-gic.h>
+#include <linux/spinlock.h>
+
+#include <asm/cpu-features.h>
+#include <asm/hazards.h>
+#include <asm/mipsregs.h>
+#include <asm/mipsmtregs.h>
+#include <asm/tlbmisc.h>
+
+#include "axd_module.h"
+#include "axd_platform.h"
+
+
+static unsigned int axd_irqnum;
+static unsigned int axd_irq;
+static unsigned int axd_vpe;
+static spinlock_t lock;
+static unsigned long smpirqflags;
+
+
+static void _axd_platform_init(void *info)
+{
+	unsigned int val;
+	unsigned long irqflags;
+	unsigned long mtflags;
+
+	/*
+	 * make sure nothing else on this vpe or another vpe can try to modify
+	 * any of the shared registers below
+	 */
+	local_irq_save(irqflags);
+	mtflags = dvpe();
+
+	/* EVP = 0, VPC = 1 */
+	val = read_c0_mvpcontrol();
+	val &= ~MVPCONTROL_EVP;
+	val |= MVPCONTROL_VPC;
+	write_c0_mvpcontrol(val);
+	instruction_hazard();
+
+	/* prepare TC for setting up */
+	settc(axd_vpe);
+	write_tc_c0_tchalt(1);
+
+	/* make sure no interrupts are pending and exceptions bits are clear */
+	write_vpe_c0_cause(0);
+	write_vpe_c0_status(0);
+
+	/* bind TC to VPE */
+	val = read_tc_c0_tcbind();
+	val |= (axd_vpe << TCBIND_CURTC_SHIFT) | (axd_vpe << TCBIND_CURVPE_SHIFT);
+	write_tc_c0_tcbind(val);
+
+	/* VPA = 1, MVP = 1 */
+	val = read_vpe_c0_vpeconf0();
+	val |= VPECONF0_MVP;
+	val |= VPECONF0_VPA;
+	write_vpe_c0_vpeconf0(val);
+
+	/* A = 1, IXMT = 0 */
+	val = read_tc_c0_tcstatus();
+	val &= ~TCSTATUS_IXMT;
+	val |= TCSTATUS_A;
+	write_tc_c0_tcstatus(val);
+
+	/* TE = 1 */
+	val = read_vpe_c0_vpecontrol();
+	val |= VPECONTROL_TE;
+	write_vpe_c0_vpecontrol(val);
+
+	/* EVP = 1, VPC = 0 */
+	val = read_c0_mvpcontrol();
+	val |= MVPCONTROL_EVP;
+	val &= ~MVPCONTROL_VPC;
+	write_c0_mvpcontrol(val);
+	instruction_hazard();
+
+	evpe(mtflags);
+	local_irq_restore(irqflags);
+}
+
+void axd_platform_init(struct axd_dev *axd)
+{
+	struct cpumask cpumask;
+
+	axd_irqnum = axd->irqnum;
+	axd_irq = axd->axd_irq;
+	axd_vpe = axd->vpe;
+	spin_lock_init(&lock);
+
+	/*
+	 * ensure axd irq runs on cpu 0 only as it's the only one that can use
+	 * MT to communicate with AXD
+	 */
+	cpumask_clear(&cpumask);
+	cpumask_set_cpu(0, &cpumask);
+	irq_set_affinity_hint(axd_irqnum, &cpumask);
+
+#ifdef CONFIG_HOTPLUG_CPU
+	/*
+	 * offline the cpu before we do anything
+	 * it's best effort here since the cpu could already be offline, hence
+	 * we ignore the return value.
+	 */
+	cpu_down(axd_vpe);
+#endif
+
+	if (smp_processor_id() != 0) {
+		/* only cpu 0 can start AXD, so send it a message to do so */
+		smp_call_function_single(0, &_axd_platform_init, NULL, 1);
+		return;
+	}
+
+	_axd_platform_init(NULL);
+}
+
+static void _reset(void *info)
+{
+	unsigned int val;
+	unsigned long irqflags;
+	unsigned long mtflags;
+
+	local_irq_save(irqflags);
+	mtflags = dvpe();
+
+	settc(axd_vpe);
+	/* first stop TC1 */
+	write_tc_c0_tchalt(1);
+
+	/* clear EXL and ERL from TCSTATUS */
+	val = read_c0_tcstatus();
+	val &= ~(ST0_EXL | ST0_ERL);
+	write_c0_tcstatus(val);
+
+	evpe(mtflags);
+	local_irq_restore(irqflags);
+}
+
+static void reset(void)
+{
+	if (smp_processor_id() != 0) {
+		/* only cpu 0 can reset AXD, so send it a message to do so */
+		smp_call_function_single(0, &_reset, NULL, 1);
+		return;
+	}
+
+	_reset(NULL);
+}
+
+static void _axd_platform_set_pc(void *info)
+{
+	unsigned long irqflags;
+	unsigned long mtflags;
+	unsigned long pc = *(unsigned long *)info;
+
+	local_irq_save(irqflags);
+	mtflags = dvpe();
+
+	settc(axd_vpe);
+	write_tc_c0_tcrestart(pc);
+
+	evpe(mtflags);
+	local_irq_restore(irqflags);
+}
+
+void axd_platform_set_pc(unsigned long pc)
+{
+	if (smp_processor_id() != 0) {
+		/* only cpu 0 can set AXD PC, so send it a message to do so */
+		smp_call_function_single(0, &_axd_platform_set_pc, &pc, 1);
+		return;
+	}
+
+	_axd_platform_set_pc(&pc);
+}
+
+static void thread_control(int start)
+{
+	unsigned long irqflags;
+	unsigned long mtflags;
+
+	local_irq_save(irqflags);
+	mtflags = dvpe();
+
+	settc(axd_vpe);
+	/* start/stop the VPE */
+	write_tc_c0_tchalt(!start);
+
+	evpe(mtflags);
+	local_irq_restore(irqflags);
+}
+
+static void _axd_platform_start(void *info)
+{
+	reset();
+	thread_control(1);
+}
+
+int axd_platform_start(void)
+{
+	if (smp_processor_id() != 0) {
+		/* only cpu 0 can start AXD, so send it a message to do so */
+		smp_call_function_single(0, &_axd_platform_start, NULL, 1);
+		return 0;
+	}
+
+	_axd_platform_start(NULL);
+
+	return 0;
+}
+
+static void _axd_platform_stop(void *info)
+{
+	thread_control(0);
+}
+
+void axd_platform_stop(void)
+{
+	if (smp_processor_id() != 0) {
+		/* only cpu 0 can stop AXD, so send it a message to do so */
+		smp_call_function_single(0, &_axd_platform_stop, NULL, 1);
+		return;
+	}
+
+	_axd_platform_stop(NULL);
+}
+
+unsigned int axd_platform_num_threads(void)
+{
+	return 1;
+}
+
+static void _axd_platform_kick_sw1(void *info)
+{
+	unsigned int val;
+	unsigned long irqflags;
+	unsigned long mtflags;
+
+	local_irq_save(irqflags);
+	mtflags = dvpe();
+
+	settc(axd_vpe);
+	val = read_vpe_c0_cause();
+	val |= CAUSEF_IP1;
+	write_vpe_c0_cause(val);
+
+	evpe(mtflags);
+	local_irq_restore(irqflags);
+}
+
+void axd_platform_kick(void)
+{
+	/*
+	 * ensure all writes to shared uncached memory are visible to AXD
+	 * before sending interrupt
+	 */
+	wmb();
+
+	if (axd_irq) {
+		gic_send_ipi(axd_irq);
+		return;
+	}
+
+	/* fallback to sending interrupt at SW1 */
+	if (smp_processor_id() != 0) {
+		/* only cpu 0 can send AXD SW1, so send it a message to do so */
+		smp_call_function_single(0, &_axd_platform_kick_sw1, NULL, 1);
+		return;
+	}
+
+	_axd_platform_kick_sw1(NULL);
+}
+
+static void axd_smp_platform_lock(void *info)
+{
+	unsigned long *flags = info;
+
+	/*
+	 * prevent AXD irq handler from accessing the lock while another
+	 * processor holds it
+	 */
+	disable_irq(axd_irqnum);
+	*flags = dvpe();
+}
+
+inline unsigned long axd_platform_lock(void)
+{
+	unsigned long irqflags;
+
+	if (smp_processor_id() != 0) {
+		/* only cpu 0 can lock AXD out, so send it a message to do so */
+		unsigned long flags;
+
+		spin_lock(&lock); /* serialise other smp cpus to access the lock */
+		smp_call_function_single(0, &axd_smp_platform_lock, &flags, 1);
+		return flags;
+	}
+
+	/*
+	 * When not servicing AXD irq then another task is trying to acquire the
+	 * lock, in this case we need to acquire the spinlock without spinning
+	 * because cpu0 must keep on running to service other cpus requests..
+	 */
+	if (!in_interrupt())
+		while (!spin_trylock(&lock))
+			cpu_relax();
+
+	/* prevent other cpus from acquiring the lock while we hold it */
+	local_irq_save(irqflags);
+	smpirqflags = irqflags;
+	return dvpe();
+}
+
+static void axd_smp_platform_unlock(void *info)
+{
+	unsigned long *flags = info;
+
+	evpe(*flags);
+	enable_irq(axd_irqnum);
+}
+
+inline void axd_platform_unlock(unsigned long flags)
+{
+	if (smp_processor_id() != 0) {
+		smp_call_function_single(0, &axd_smp_platform_unlock, &flags, 1);
+		spin_unlock(&lock);
+		return;
+	}
+	evpe(flags);
+	local_irq_restore(smpirqflags);
+	if (!in_interrupt())
+		spin_unlock(&lock);
+}
+
+inline void axd_platform_irq_ack(void)
+{
+}
+
+static void print_regs(unsigned int thread)
+{
+	unsigned long irqflags;
+	unsigned long mtflags;
+
+	local_irq_save(irqflags);
+	mtflags = dvpe();
+
+	settc(thread);
+	pr_err("PC:\t\t0x%08lX\n", read_tc_c0_tcrestart());
+	pr_err("STATUS:\t\t0x%08lX\n", read_vpe_c0_status());
+	pr_err("CAUSE:\t\t0x%08lX\n", read_vpe_c0_cause());
+	pr_err("EPC:\t\t0x%08lX\n", read_vpe_c0_epc());
+	pr_err("EBASE:\t\t0x%08lX\n", read_vpe_c0_ebase());
+	pr_err("BADVADDR:\t0x%08lX\n", read_vpe_c0_badvaddr());
+	pr_err("CONFIG:\t\t0x%08lX\n", read_vpe_c0_config());
+	pr_err("MVPCONTROL:\t0x%08X\n", read_c0_mvpcontrol());
+	pr_err("VPECONTROL:\t0x%08lX\n", read_vpe_c0_vpecontrol());
+	pr_err("VPECONF0:\t0x%08lX\n", read_vpe_c0_vpeconf0());
+	pr_err("TCBIND:\t\t0x%08lX\n", read_tc_c0_tcbind());
+	pr_err("TCSTATUS:\t0x%08lX\n", read_tc_c0_tcstatus());
+	pr_err("TCHALT:\t\t0x%08lX\n", read_tc_c0_tchalt());
+	pr_err("\n");
+	pr_err("$0: 0x%08lX\tat: 0x%08lX\tv0: 0x%08lX\tv1: 0x%08lX\n",
+				mftgpr(0), mftgpr(1), mftgpr(2), mftgpr(3));
+	pr_err("a0: 0x%08lX\ta1: 0x%08lX\ta2: 0x%08lX\ta3: 0x%08lX\n",
+				mftgpr(4), mftgpr(5), mftgpr(6), mftgpr(7));
+	pr_err("t0: 0x%08lX\tt1: 0x%08lX\tt2: 0x%08lX\tt3: 0x%08lX\n",
+				mftgpr(8), mftgpr(9), mftgpr(10), mftgpr(11));
+	pr_err("t4: 0x%08lX\tt5: 0x%08lX\tt6: 0x%08lX\tt7: 0x%08lX\n",
+				mftgpr(12), mftgpr(13), mftgpr(14), mftgpr(15));
+	pr_err("s0: 0x%08lX\ts1: 0x%08lX\ts2: 0x%08lX\ts3: 0x%08lX\n",
+				mftgpr(16), mftgpr(17), mftgpr(18), mftgpr(19));
+	pr_err("s4: 0x%08lX\ts5: 0x%08lX\ts6: 0x%08lX\ts7: 0x%08lX\n",
+				mftgpr(20), mftgpr(21), mftgpr(22), mftgpr(23));
+	pr_err("t8: 0x%08lX\tt9: 0x%08lX\tk0: 0x%08lX\tk1: 0x%08lX\n",
+				mftgpr(24), mftgpr(25), mftgpr(26), mftgpr(27));
+	pr_err("gp: 0x%08lX\tsp: 0x%08lX\ts8: 0x%08lX\tra: 0x%08lX\n",
+				mftgpr(28), mftgpr(29), mftgpr(30), mftgpr(31));
+
+	evpe(mtflags);
+	local_irq_restore(irqflags);
+}
+
+static void _axd_platform_print_regs(void *info)
+{
+	pr_err("VPE%d regs dump\n", axd_vpe);
+	print_regs(axd_vpe);
+}
+
+void axd_platform_print_regs(void)
+{
+	if (smp_processor_id() != 0) {
+		/* only cpu 0 can read AXD regs, so send it a message to do so */
+		smp_call_function_single(0, &_axd_platform_print_regs, NULL, 1);
+		return;
+	}
+
+	_axd_platform_print_regs(NULL);
+}
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1212143 — [PATCH 10/10] ALSA: axd: add Makefile

Now all necessary files are added, allow axd to be selected through Kconfig and
compiled.

Signed-off-by: Qais Yousef <qais.yousef@imgtec.com>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: linux-kernel@vger.kernel.org
---
 sound/soc/Kconfig          |  1 +
 sound/soc/Makefile         |  1 +
 sound/soc/img/Kconfig      | 11 +++++++++++
 sound/soc/img/Makefile     |  1 +
 sound/soc/img/axd/Makefile | 13 +++++++++++++
 5 files changed, 27 insertions(+)
 create mode 100644 sound/soc/img/Kconfig
 create mode 100644 sound/soc/img/Makefile
 create mode 100644 sound/soc/img/axd/Makefile

diff --git a/sound/soc/Kconfig b/sound/soc/Kconfig
index 2ae9619443d1..8f29af1d397e 100644
--- a/sound/soc/Kconfig
+++ b/sound/soc/Kconfig
@@ -44,6 +44,7 @@ source "sound/soc/jz4740/Kconfig"
 source "sound/soc/nuc900/Kconfig"
 source "sound/soc/omap/Kconfig"
 source "sound/soc/kirkwood/Kconfig"
+source "sound/soc/img/Kconfig"
 source "sound/soc/intel/Kconfig"
 source "sound/soc/mediatek/Kconfig"
 source "sound/soc/mxs/Kconfig"
diff --git a/sound/soc/Makefile b/sound/soc/Makefile
index e189903fabf4..c6a1c04b8e39 100644
--- a/sound/soc/Makefile
+++ b/sound/soc/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_SND_SOC)	+= davinci/
 obj-$(CONFIG_SND_SOC)	+= dwc/
 obj-$(CONFIG_SND_SOC)	+= fsl/
 obj-$(CONFIG_SND_SOC)	+= jz4740/
+obj-$(CONFIG_SND_SOC)	+= img/
 obj-$(CONFIG_SND_SOC)	+= intel/
 obj-$(CONFIG_SND_SOC)	+= mediatek/
 obj-$(CONFIG_SND_SOC)	+= mxs/
diff --git a/sound/soc/img/Kconfig b/sound/soc/img/Kconfig
new file mode 100644
index 000000000000..5a089b7d4929
--- /dev/null
+++ b/sound/soc/img/Kconfig
@@ -0,0 +1,11 @@
+config SND_SOC_IMG_AXD
+	tristate "Imagination AXD Audio Processing IP"
+	depends on MIPS && COMMON_CLK && CMA
+	---help---
+	  Say Y or M here if you to add support for AXD Audio Processing IP.
+
+config SND_SOC_IMG_AXD_DEBUGFS
+	bool "AXD debugfs support"
+	depends on SND_SOC_IMG_AXD && DEBUG_FS
+	---help---
+	  Say Y if you want to create AXD debugfs nodes
diff --git a/sound/soc/img/Makefile b/sound/soc/img/Makefile
new file mode 100644
index 000000000000..189abf5d927c
--- /dev/null
+++ b/sound/soc/img/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_SND_SOC_IMG_AXD) += axd/
diff --git a/sound/soc/img/axd/Makefile b/sound/soc/img/axd/Makefile
new file mode 100644
index 000000000000..cfa1f412bf19
--- /dev/null
+++ b/sound/soc/img/axd/Makefile
@@ -0,0 +1,13 @@
+obj-$(CONFIG_SND_SOC_IMG_AXD) := axd.o
+
+axd-objs = axd_alsa_ops.o \
+	   axd_buffers.o \
+	   axd_cmds.o \
+	   axd_cmds_config.o \
+	   axd_cmds_decoder_config.o \
+	   axd_cmds_info.o \
+	   axd_cmds_internal.o \
+	   axd_cmds_pipes.o \
+	   axd_hdr.o \
+	   axd_module.o \
+	   axd_platform_$(ARCH).o \
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1214090

[Multipart message — attachments visible in raw view] — view raw

On Mon, Aug 24, 2015 at 01:39:09PM +0100, Qais Yousef wrote:

> Qais Yousef (10):
>   irqchip: irq-mips-gic: export gic_send_ipi
>   dt: add img,axd.txt device tree binding document
>   ALSA: add AXD Audio Processing IP alsa driver
>   ALSA: axd: add fw binary header manipulation files
>   ALSA: axd: add buffers manipulation files
>   ALSA: axd: add basic files for sending/receiving axd cmds
>   ALSA: axd: add cmd interface helper functions
>   ALSA: axd: add low level AXD platform setup files
>   ALSA: axd: add alsa compress offload operations
>   ALSA: axd: add Makefile

Please try to use subject lines matching the style for the subsystem, I
very nearly deleted this unread because it looks like an ALSA patch
series, not an ASoC one.

[toc] | [prev] | [next] | [standalone]

#1214403

On 08/26/2015 07:04 PM, Mark Brown wrote:
> On Mon, Aug 24, 2015 at 01:39:09PM +0100, Qais Yousef wrote:
>
>> Qais Yousef (10):
>>    irqchip: irq-mips-gic: export gic_send_ipi
>>    dt: add img,axd.txt device tree binding document
>>    ALSA: add AXD Audio Processing IP alsa driver
>>    ALSA: axd: add fw binary header manipulation files
>>    ALSA: axd: add buffers manipulation files
>>    ALSA: axd: add basic files for sending/receiving axd cmds
>>    ALSA: axd: add cmd interface helper functions
>>    ALSA: axd: add low level AXD platform setup files
>>    ALSA: axd: add alsa compress offload operations
>>    ALSA: axd: add Makefile
> Please try to use subject lines matching the style for the subsystem, I
> very nearly deleted this unread because it looks like an ALSA patch
> series, not an ASoC one.

OK sorry about that. I'll fix this in the next series.

Thanks,
Qais
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1214120 — Re: [PATCH 03/10] ALSA: add AXD Audio Processing IP alsa driver

[Multipart message — attachments visible in raw view] — view raw

On Mon, Aug 24, 2015 at 01:39:12PM +0100, Qais Yousef wrote:

> +#define THREAD_COUNT 4

This is a very generic name that looks likely to collide with something
else, please namespace.

> +#define AXD_INPUT_DESCRIPTORS 10
> +struct axd_input {
> +	struct axd_buffer_desc descriptors[AXD_INPUT_DESCRIPTORS];
> +};

Where do these numbers come from?  Are they hardware limits or something
else?

> +/* this is required by MIPS ioremap_cachable() */
> +#include <asm/pgtable.h>

Don't work around this here, fix it in the relevant header.

> +#define AXD_BASE_VADDR		0xD0000000

This sounds like something that is going to be platform dependant,
should this be supplied from board configuration?

> +extern struct snd_compr_ops axd_compr_ops;

Prototype shared definitions in headers not in C files please so we know
the definition matches.

> +static struct snd_soc_dai_driver axd_dai[] = {
> +	{

Why an array with only one entry?

> +	if (!*offp) {
> +		unsigned int flags = axd_platform_lock();
> +		unsigned int log_offset = ioread32(log_addr);
> +		unsigned int log_wrapped = ioread32(log_addr + 8);
> +		char __iomem *log_buff = (char __iomem *)(log_addr + 12);
> +
> +		/* new read from beginning, fill up our internal buffer */
> +		if (!log_wrapped) {
> +			memcpy_fromio(axd->log_rbuf, log_buff, log_offset);
> +			axd->log_rbuf_rem = log_offset;
> +		} else {
> +			char __iomem *pos = log_buff + log_offset;
> +			unsigned int rem = log_size - log_offset;
> +
> +			memcpy_fromio(axd->log_rbuf, pos, rem);
> +			memcpy_fromio(axd->log_rbuf + rem, log_buff, log_offset);
> +			axd->log_rbuf_rem = log_size;
> +		}
> +		axd_platform_unlock(flags);

I didn't see the lock being taken?

> +static ssize_t axd_write_mask(struct file *filep,
> +				const char __user *buff, size_t count, loff_t *offp)
> +{
> +	struct axd_dev *axd = filep->f_inode->i_private;
> +	unsigned int mask;
> +	char buffer[32] = {};
> +	int ret;
> +
> +	/* ensure we always have null at the end */
> +	ret = copy_from_user(buffer, buff, min(31u, count));
> +	if (ret < 0)
> +		return ret;
> +
> +	if (!kstrtouint(buffer, 0, &mask))
> +		axd_write_reg(&axd->cmd, AXD_REG_DEBUG_MASK, mask);

What are we writing here?  If we're going behind the driver's back on
something that might confuse it it's generally better to taint the
kernel so we know dodgy stuff happened later on.

> +static void axd_debugfs_create(struct axd_dev *axd)
> +{
> +	axd->debugfs = debugfs_create_dir(dev_name(axd->dev), NULL);
> +	if (IS_ERR_OR_NULL(axd->debugfs)) {
> +		dev_err(axd->dev, "failed to create debugfs node\n");
> +		return;
> +	}

It'd be nicer to create this under the relevant ASoC debugfs directory
so it's easier to find.

> +#ifdef CONFIG_CRYPTO_LZO
> +#include <linux/crypto.h>

This include should be with all the other includes, not down here.

> +	size = axd->fw_size;
> +	cached_fw_base = (char *)CAC_ADDR((int)axd->fw_base_m);
> +	ret = crypto_comp_decompress(tfm, fw->data + 8,
> +				fw->size - 8, cached_fw_base, &size);
> +	if (ret)
> +		dev_err(axd->dev, "Failed to decompress the firmware\n");

Print return codes if you get them.

> +
> +	if (size != axd->fw_size) {
> +		dev_err(axd->dev, "Uncompressed file size doesn't match reported file size\n");
> +		ret = -EINVAL;
> +	}

Should we be checking this if the decompression failed?

> +}
> +#else /* !CONFIG_CRYPTO_LZO */
> +static int decompress_fw(struct axd_dev *axd, const struct firmware *fw)

Blank lines between things please.

> +{
> +	dev_err(axd->dev, "The firmware must be lzo decompressed first, compile driver again with CONFIG_CRYPTO_LZO enabled in kernel or do the decompression in user space.\n");

Please split this up into a few prints for wrapping, similarly in
several other places.

> +	return -EIO;

-ENOTSUPP.

> +		return -EIO;
> +	}
> +	/*

More vertical blanks missing.

> +	 * We copy through the cache, fw will do the necessary cache
> +	 * flushes and syncing at startup.
> +	 * Copying from uncached makes it more difficult for the
> +	 * firmware to keep the caches coherent with memory when it sets
> +	 * tlbs and start running.
> +	 */
> +	memcpy_toio((void *)cached_fw_base, fw->data, fw->size);

Why the cast here?  I'm also not seeing where we handled the copying to
I/O in the decompression case?

> +	dev_info(axd->dev, "Loading firmware at 0x%p ...\n", axd->fw_base_m);

This should be _dbg() at most, otherwise it's going to get noisy.

> +	t0_new_pc = (unsigned long) axd->fw_base_m + (t0_new_pc - AXD_BASE_VADDR);

Those casts look fishy...

> +	for (i = 0; i < AXD_LDFW_RETRIES; i++) {
> +		ret = axd_wait_ready(axd_cmd->message);
> +		if (!ret) {
> +			/*
> +			 * Let the firmware know the address of the buffer
> +			 * region
> +			 */
> +			ret = axd_write_reg(axd_cmd,
> +					AXD_REG_BUFFER_BASE, axd->buf_base_p);
> +			if (ret) {
> +				dev_err(axd->dev,
> +					"Failed to setup buffers base address\n");

Again print errors please.

> +				goto out;
> +			}
> +			return 0;
> +
> +		}
> +	}

I'm not seeing any diagnostics if we fall out of the retry loop here?

> +static void axd_reset(struct work_struct *work)
> +{
> +	unsigned int major, minor, patch;
> +	int i;
> +
> +	struct axd_dev *axd = container_of(work, struct axd_dev, watchdogwork);
> +
> +
> +	/* if we got a fatal error, don't reset if watchdog is disabled */
> +	if (unlikely(!axd->cmd.watchdogenabled))
> +		return;

There's generally no need for unlikely() annotations outside of hot
paths.

> +	/* stop the watchdog timer until we restart */
> +	del_timer(&axd->watchdogtimer);

I'd expect del_timer_sync() to make sure that the timer stopped.

> +	if (!axd_get_flag(&axd->cmd.fw_stopped_flg)) {
> +		/* ping the firmware by requesting its version info */
> +		axd_cmd_get_version(&axd->cmd, &major, &minor, &patch);
> +		if (!major && !minor && !patch) {
> +			dev_warn(axd->dev, "Firmware stopped responding...\n");
> +			axd_set_flag(&axd->cmd.fw_stopped_flg, 1);
> +		} else {
> +			goto out;
> +		}
> +	}

It might be useful to display the firmware version we loaded.

> +	axd_platform_print_regs();
> +	dev_warn(axd->dev, "Reloading AXD firmware...\n");

This is going to get noisy and isn't adding much.

> +	/* wake up any task sleeping on command response */
> +	wake_up(&axd->cmd.wait);
> +	/* give chance to user land tasks to react to the crash */
> +	ssleep(2);

This looks horribly racy, I'd expect us to be trashing and/or killing
off any active work and resources here.

> +static void axd_watchdog_timer(unsigned long arg)
> +{
> +	struct axd_dev *axd = (struct axd_dev *)arg;
> +
> +	/* skip if watchdog is not enabled */
> +	if (unlikely(!axd->cmd.watchdogenabled))
> +		goto out;
> +
> +	schedule_work(&axd->watchdogwork);
> +	return;
> +out:
> +	mod_timer(&axd->watchdogtimer, jiffies + WATCHDOG_TIMEOUT);
> +}

So we have a timer that just schedules some work?  Why not just
schedule_delayed_work()?

> +	/*
> +	 * Verify that the firmware is ready. In normal cases the firmware
> +	 * should start immediately, but to be more robust we do this
> +	 * verification and give the firmware a chance of 3 seconds to be ready
> +	 * otherwise we exit in failure.
> +	 */
> +	for (i = 0; i < AXD_LDFW_RETRIES; i++) {
> +		axd_cmd_get_version(&axd->cmd, &major, &minor, &patch);
> +		if (major || minor || patch) {
> +			/* firmware is ready */
> +			break;
> +		}
> +		/* if we couldn't read the version after 3 tries, error */
> +		if (i == AXD_LDFW_RETRIES - 1) {
> +			dev_err(axd->dev, "Failed to communicate with the firmware\n");
> +			ret = -EIO;
> +			goto error;
> +		}
> +		/* wait for 10 ms for the firmware to start */
> +		msleep(10);
> +	}
> +	dev_info(axd->dev, "Running firmware version %u.%u.%u %s\n",
> +				major, minor, patch, axd_hdr_get_build_str());

Why is this code not shared with the restart case?

> +	ret = of_property_read_u32_array(of_node, "gic-irq", val, 2);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +				"'gic-irq' parameter must be set\n");
> +		return ret;
> +	}

This appears to have a DT binding but the binding is not documented.
All new DT bindings must be documented.  I'm concerned that some of the
properties being read from DT may not be ideal here...

[toc] | [prev] | [next] | [standalone]

#1214501 — Re: [PATCH 03/10] ALSA: add AXD Audio Processing IP alsa driver

On 08/26/2015 07:37 PM, Mark Brown wrote:
> On Mon, Aug 24, 2015 at 01:39:12PM +0100, Qais Yousef wrote:
>
>> +#define THREAD_COUNT 4
> This is a very generic name that looks likely to collide with something
> else, please namespace.

OK.

>> +#define AXD_INPUT_DESCRIPTORS 10
>> +struct axd_input {
>> +	struct axd_buffer_desc descriptors[AXD_INPUT_DESCRIPTORS];
>> +};
> Where do these numbers come from?  Are they hardware limits or something
> else?

These numbers are what the firmware designed to work with. We had to set 
a limit and we sought 10 to be a good one for our purposes. We don't 
expect to need to change this number.

>> +/* this is required by MIPS ioremap_cachable() */
>> +#include <asm/pgtable.h>
> Don't work around this here, fix it in the relevant header.
>

Will do.

>> +#define AXD_BASE_VADDR		0xD0000000
> This sounds like something that is going to be platform dependant,
> should this be supplied from board configuration?

I don't expect this to change. Can we add the configuration later if we 
hit the need to change it?

>> +extern struct snd_compr_ops axd_compr_ops;
> Prototype shared definitions in headers not in C files please so we know
> the definition matches.

OK.

>> +static struct snd_soc_dai_driver axd_dai[] = {
>> +	{
> Why an array with only one entry?

Will fix it.

>> +	if (!*offp) {
>> +		unsigned int flags = axd_platform_lock();
>> +		unsigned int log_offset = ioread32(log_addr);
>> +		unsigned int log_wrapped = ioread32(log_addr + 8);
>> +		char __iomem *log_buff = (char __iomem *)(log_addr + 12);
>> +
>> +		/* new read from beginning, fill up our internal buffer */
>> +		if (!log_wrapped) {
>> +			memcpy_fromio(axd->log_rbuf, log_buff, log_offset);
>> +			axd->log_rbuf_rem = log_offset;
>> +		} else {
>> +			char __iomem *pos = log_buff + log_offset;
>> +			unsigned int rem = log_size - log_offset;
>> +
>> +			memcpy_fromio(axd->log_rbuf, pos, rem);
>> +			memcpy_fromio(axd->log_rbuf + rem, log_buff, log_offset);
>> +			axd->log_rbuf_rem = log_size;
>> +		}
>> +		axd_platform_unlock(flags);
> I didn't see the lock being taken?

The lock is the first line in the block (unsigned int flags = 
axd_platform_lock()). I'll tidy it up to make it more readable.

>> +static ssize_t axd_write_mask(struct file *filep,
>> +				const char __user *buff, size_t count, loff_t *offp)
>> +{
>> +	struct axd_dev *axd = filep->f_inode->i_private;
>> +	unsigned int mask;
>> +	char buffer[32] = {};
>> +	int ret;
>> +
>> +	/* ensure we always have null at the end */
>> +	ret = copy_from_user(buffer, buff, min(31u, count));
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (!kstrtouint(buffer, 0, &mask))
>> +		axd_write_reg(&axd->cmd, AXD_REG_DEBUG_MASK, mask);
> What are we writing here?  If we're going behind the driver's back on
> something that might confuse it it's generally better to taint the
> kernel so we know dodgy stuff happened later on.

The debug mask will cause AXD firmware to provide more or less debug 
information. We are not going behind the driver's back.

>> +static void axd_debugfs_create(struct axd_dev *axd)
>> +{
>> +	axd->debugfs = debugfs_create_dir(dev_name(axd->dev), NULL);
>> +	if (IS_ERR_OR_NULL(axd->debugfs)) {
>> +		dev_err(axd->dev, "failed to create debugfs node\n");
>> +		return;
>> +	}
> It'd be nicer to create this under the relevant ASoC debugfs directory
> so it's easier to find.

Sure. I'll try to find an example and follow what it does.

>> +#ifdef CONFIG_CRYPTO_LZO
>> +#include <linux/crypto.h>
> This include should be with all the other includes, not down here.

Was trying to reduce the ifdefery. Will fix.

>> +	size = axd->fw_size;
>> +	cached_fw_base = (char *)CAC_ADDR((int)axd->fw_base_m);
>> +	ret = crypto_comp_decompress(tfm, fw->data + 8,
>> +				fw->size - 8, cached_fw_base, &size);
>> +	if (ret)
>> +		dev_err(axd->dev, "Failed to decompress the firmware\n");
> Print return codes if you get them.

Will do.

>> +
>> +	if (size != axd->fw_size) {
>> +		dev_err(axd->dev, "Uncompressed file size doesn't match reported file size\n");
>> +		ret = -EINVAL;
>> +	}
> Should we be checking this if the decompression failed?

Nope. I'll fix it.

>> +}
>> +#else /* !CONFIG_CRYPTO_LZO */
>> +static int decompress_fw(struct axd_dev *axd, const struct firmware *fw)
> Blank lines between things please.

OK.

>> +{
>> +	dev_err(axd->dev, "The firmware must be lzo decompressed first, compile driver again with CONFIG_CRYPTO_LZO enabled in kernel or do the decompression in user space.\n");
> Please split this up into a few prints for wrapping, similarly in
> several other places.

OK. I thought the convention for strings to leave them as is to allow 
grepping. I'll fix it.

>> +	return -EIO;
> -ENOTSUPP.

OK.

>> +		return -EIO;
>> +	}
>> +	/*
> More vertical blanks missing.

OK.

>
>> +	 * We copy through the cache, fw will do the necessary cache
>> +	 * flushes and syncing at startup.
>> +	 * Copying from uncached makes it more difficult for the
>> +	 * firmware to keep the caches coherent with memory when it sets
>> +	 * tlbs and start running.
>> +	 */
>> +	memcpy_toio((void *)cached_fw_base, fw->data, fw->size);
> Why the cast here?  I'm also not seeing where we handled the copying to
> I/O in the decompression case?

I couldn't avoid the cast. If cached_fw_base is 'void *' I'll get a 
warning when initialising cached_fw_base from CAC_ADDR().
So I'll have to either cast here or there, I chose here.
If I pass axd->fw_base_m I encounter the issue described in the commit 
message.

Good point. When decompressing crypto_comp_decompress() will write 
directly to the memory. It is safe but it doesn't go through the correct 
API. Not sure what I can do here.

>> +	dev_info(axd->dev, "Loading firmware at 0x%p ...\n", axd->fw_base_m);
> This should be _dbg() at most, otherwise it's going to get noisy.
>
>> +	t0_new_pc = (unsigned long) axd->fw_base_m + (t0_new_pc - AXD_BASE_VADDR);
> Those casts look fishy...

I am happy to try something else. axd->fw_base_m is of type void * 
__iomem but we want to do some arithmetic on it.
Is there a better way to do it?

>
>> +	for (i = 0; i < AXD_LDFW_RETRIES; i++) {
>> +		ret = axd_wait_ready(axd_cmd->message);
>> +		if (!ret) {
>> +			/*
>> +			 * Let the firmware know the address of the buffer
>> +			 * region
>> +			 */
>> +			ret = axd_write_reg(axd_cmd,
>> +					AXD_REG_BUFFER_BASE, axd->buf_base_p);
>> +			if (ret) {
>> +				dev_err(axd->dev,
>> +					"Failed to setup buffers base address\n");
> Again print errors please.
>
>> +				goto out;
>> +			}
>> +			return 0;
>> +
>> +		}
>> +	}
> I'm not seeing any diagnostics if we fall out of the retry loop here?

Will add one.

>
>> +static void axd_reset(struct work_struct *work)
>> +{
>> +	unsigned int major, minor, patch;
>> +	int i;
>> +
>> +	struct axd_dev *axd = container_of(work, struct axd_dev, watchdogwork);
>> +
>> +
>> +	/* if we got a fatal error, don't reset if watchdog is disabled */
>> +	if (unlikely(!axd->cmd.watchdogenabled))
>> +		return;
> There's generally no need for unlikely() annotations outside of hot
> paths.

OK.

>> +	/* stop the watchdog timer until we restart */
>> +	del_timer(&axd->watchdogtimer);
> I'd expect del_timer_sync() to make sure that the timer stopped.

OK.

>> +	if (!axd_get_flag(&axd->cmd.fw_stopped_flg)) {
>> +		/* ping the firmware by requesting its version info */
>> +		axd_cmd_get_version(&axd->cmd, &major, &minor, &patch);
>> +		if (!major && !minor && !patch) {
>> +			dev_warn(axd->dev, "Firmware stopped responding...\n");
>> +			axd_set_flag(&axd->cmd.fw_stopped_flg, 1);
>> +		} else {
>> +			goto out;
>> +		}
>> +	}
> It might be useful to display the firmware version we loaded.

OK.

>
>> +	axd_platform_print_regs();
>> +	dev_warn(axd->dev, "Reloading AXD firmware...\n");
> This is going to get noisy and isn't adding much.

OK.

>
>> +	/* wake up any task sleeping on command response */
>> +	wake_up(&axd->cmd.wait);
>> +	/* give chance to user land tasks to react to the crash */
>> +	ssleep(2);
> This looks horribly racy, I'd expect us to be trashing and/or killing
> off any active work and resources here.

OK. I was trying to play nicely by giving the chance to userland to 
repond to -ERESTART which would be sent from aborting any pending 
reads/writes.

Are you suggesting to send SIGKILL using force_sig()?

>
>> +static void axd_watchdog_timer(unsigned long arg)
>> +{
>> +	struct axd_dev *axd = (struct axd_dev *)arg;
>> +
>> +	/* skip if watchdog is not enabled */
>> +	if (unlikely(!axd->cmd.watchdogenabled))
>> +		goto out;
>> +
>> +	schedule_work(&axd->watchdogwork);
>> +	return;
>> +out:
>> +	mod_timer(&axd->watchdogtimer, jiffies + WATCHDOG_TIMEOUT);
>> +}
> So we have a timer that just schedules some work?  Why not just
> schedule_delayed_work()?

Either wasn't there the time this was first written or was missed. 
Either case thanks for the suggestion I'll change it.

>
>> +	/*
>> +	 * Verify that the firmware is ready. In normal cases the firmware
>> +	 * should start immediately, but to be more robust we do this
>> +	 * verification and give the firmware a chance of 3 seconds to be ready
>> +	 * otherwise we exit in failure.
>> +	 */
>> +	for (i = 0; i < AXD_LDFW_RETRIES; i++) {
>> +		axd_cmd_get_version(&axd->cmd, &major, &minor, &patch);
>> +		if (major || minor || patch) {
>> +			/* firmware is ready */
>> +			break;
>> +		}
>> +		/* if we couldn't read the version after 3 tries, error */
>> +		if (i == AXD_LDFW_RETRIES - 1) {
>> +			dev_err(axd->dev, "Failed to communicate with the firmware\n");
>> +			ret = -EIO;
>> +			goto error;
>> +		}
>> +		/* wait for 10 ms for the firmware to start */
>> +		msleep(10);
>> +	}
>> +	dev_info(axd->dev, "Running firmware version %u.%u.%u %s\n",
>> +				major, minor, patch, axd_hdr_get_build_str());
> Why is this code not shared with the restart case?

I didn't think it's necessary but I see how it can be better to move it 
inside axd_fw_start() now.

>
>> +	ret = of_property_read_u32_array(of_node, "gic-irq", val, 2);
>> +	if (ret) {
>> +		dev_err(&pdev->dev,
>> +				"'gic-irq' parameter must be set\n");
>> +		return ret;
>> +	}
> This appears to have a DT binding but the binding is not documented.
> All new DT bindings must be documented.  I'm concerned that some of the
> properties being read from DT may not be ideal here...

It is documented on a different patch. Sorry I think I just added the DT 
maintainers to the CC for that patch and sent it to the ALSA list. I'll 
be more careful in the next series to include all ALSA maintainers for 
all patches.

Yes the DT will need to be enhanced. There's a separate discussion 
generated by one of the patches on this series about how IPI should be 
defined in DT.

See this

     https://lkml.org/lkml/2015/8/26/713

Again sorry for not explicitly adding you to the CC list for all the 
patches.

Thanks,
Qais

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1214657 — Re: [PATCH 03/10] ALSA: add AXD Audio Processing IP alsa driver

[Multipart message — attachments visible in raw view] — view raw

On Thu, Aug 27, 2015 at 01:15:51PM +0100, Qais Yousef wrote:
> On 08/26/2015 07:37 PM, Mark Brown wrote:
> >On Mon, Aug 24, 2015 at 01:39:12PM +0100, Qais Yousef wrote:

> >>+#define AXD_INPUT_DESCRIPTORS 10
> >>+struct axd_input {
> >>+	struct axd_buffer_desc descriptors[AXD_INPUT_DESCRIPTORS];
> >>+};
> >Where do these numbers come from?  Are they hardware limits or something
> >else?

> These numbers are what the firmware designed to work with. We had to set a
> limit and we sought 10 to be a good one for our purposes. We don't expect to
> need to change this number.

So we have hard coded numbers in the firmware that we need in the driver
but we can't read those numbers back from the firmware.  That's sad.

> >>+#define AXD_BASE_VADDR		0xD0000000

> >This sounds like something that is going to be platform dependant,
> >should this be supplied from board configuration?

> I don't expect this to change. Can we add the configuration later if we hit
> the need to change it?

It should be trivial to make things configurable shouldn't it?

> >>+	if (!*offp) {
> >>+		unsigned int flags = axd_platform_lock();
> >>+		unsigned int log_offset = ioread32(log_addr);
> >>+		unsigned int log_wrapped = ioread32(log_addr + 8);
> >>+		char __iomem *log_buff = (char __iomem *)(log_addr + 12);
> >>+
> >>+		/* new read from beginning, fill up our internal buffer */
> >>+		if (!log_wrapped) {
> >>+			memcpy_fromio(axd->log_rbuf, log_buff, log_offset);
> >>+			axd->log_rbuf_rem = log_offset;
> >>+		} else {
> >>+			char __iomem *pos = log_buff + log_offset;
> >>+			unsigned int rem = log_size - log_offset;
> >>+
> >>+			memcpy_fromio(axd->log_rbuf, pos, rem);
> >>+			memcpy_fromio(axd->log_rbuf + rem, log_buff, log_offset);
> >>+			axd->log_rbuf_rem = log_size;
> >>+		}
> >>+		axd_platform_unlock(flags);
> >I didn't see the lock being taken?

> The lock is the first line in the block (unsigned int flags =
> axd_platform_lock()). I'll tidy it up to make it more readable.

It's very bad practice to bury lock taking in with the variable
declaration.

> >>+#ifdef CONFIG_CRYPTO_LZO
> >>+#include <linux/crypto.h>
> >This include should be with all the other includes, not down here.

> Was trying to reduce the ifdefery. Will fix.

You don't need any ifdefs for the include, you can just include the
header.

> >>+{
> >>+	dev_err(axd->dev, "The firmware must be lzo decompressed first, compile driver again with CONFIG_CRYPTO_LZO enabled in kernel or do the decompression in user space.\n");
> >Please split this up into a few prints for wrapping, similarly in
> >several other places.

> OK. I thought the convention for strings to leave them as is to allow
> grepping. I'll fix it.

You should keep strings that are displayed as a single string together
but if you are splitting something in the output then that split won't
hurt grepping in the source.

> >>+	 * We copy through the cache, fw will do the necessary cache
> >>+	 * flushes and syncing at startup.
> >>+	 * Copying from uncached makes it more difficult for the
> >>+	 * firmware to keep the caches coherent with memory when it sets
> >>+	 * tlbs and start running.
> >>+	 */
> >>+	memcpy_toio((void *)cached_fw_base, fw->data, fw->size);

> >Why the cast here?  I'm also not seeing where we handled the copying to
> >I/O in the decompression case?

> I couldn't avoid the cast. If cached_fw_base is 'void *' I'll get a warning
> when initialising cached_fw_base from CAC_ADDR().

Why do you get a warning from that?  Perhaps the warnings are trying to
tell us something...

> Good point. When decompressing crypto_comp_decompress() will write directly
> to the memory. It is safe but it doesn't go through the correct API. Not
> sure what I can do here.

Uncompress to a buffer then write that buffer to the final destination?

> >>+	dev_info(axd->dev, "Loading firmware at 0x%p ...\n", axd->fw_base_m);
> >This should be _dbg() at most, otherwise it's going to get noisy.

> >>+	t0_new_pc = (unsigned long) axd->fw_base_m + (t0_new_pc - AXD_BASE_VADDR);

> >Those casts look fishy...

> I am happy to try something else. axd->fw_base_m is of type void * __iomem
> but we want to do some arithmetic on it.
> Is there a better way to do it?

Pointer arithmetic or converting it to a number?

> >>+	/* wake up any task sleeping on command response */
> >>+	wake_up(&axd->cmd.wait);
> >>+	/* give chance to user land tasks to react to the crash */
> >>+	ssleep(2);

> >This looks horribly racy, I'd expect us to be trashing and/or killing
> >off any active work and resources here.

> OK. I was trying to play nicely by giving the chance to userland to repond
> to -ERESTART which would be sent from aborting any pending reads/writes.

> Are you suggesting to send SIGKILL using force_sig()?

No, I'm suggesting tearing down the kernel side of any work and kicking
errors back to userspace if it continues to interact with anything that
was ongoing.

[toc] | [prev] | [next] | [standalone]

#1215238 — Re: [PATCH 03/10] ALSA: add AXD Audio Processing IP alsa driver

On 08/27/2015 04:32 PM, Mark Brown wrote:
> On Thu, Aug 27, 2015 at 01:15:51PM +0100, Qais Yousef wrote:
>> On 08/26/2015 07:37 PM, Mark Brown wrote:
>>> On Mon, Aug 24, 2015 at 01:39:12PM +0100, Qais Yousef wrote:
>>>> +#define AXD_INPUT_DESCRIPTORS 10
>>>> +struct axd_input {
>>>> +	struct axd_buffer_desc descriptors[AXD_INPUT_DESCRIPTORS];
>>>> +};
>>> Where do these numbers come from?  Are they hardware limits or something
>>> else?
>> These numbers are what the firmware designed to work with. We had to set a
>> limit and we sought 10 to be a good one for our purposes. We don't expect to
>> need to change this number.
> So we have hard coded numbers in the firmware that we need in the driver
> but we can't read those numbers back from the firmware.  That's sad.
>
>>>> +#define AXD_BASE_VADDR		0xD0000000
>>> This sounds like something that is going to be platform dependant,
>>> should this be supplied from board configuration?
>> I don't expect this to change. Can we add the configuration later if we hit
>> the need to change it?
> It should be trivial to make things configurable shouldn't it?

Yes and I am all with configurability but I don't think it makes sense 
here. AXD will always have its own MMU and will not share virtual 
address space, so the possibility of us wanting to move this somewhere 
else is really very thin. Also I don't think this is the kind of detail 
we need to concern the user with. I'll see if I can make the binary 
header parsing more flexible so we can add more info like this and the 
one above in the future and be more future proof.

>>>> +	if (!*offp) {
>>>> +		unsigned int flags = axd_platform_lock();
>>>> +		unsigned int log_offset = ioread32(log_addr);
>>>> +		unsigned int log_wrapped = ioread32(log_addr + 8);
>>>> +		char __iomem *log_buff = (char __iomem *)(log_addr + 12);
>>>> +
>>>> +		/* new read from beginning, fill up our internal buffer */
>>>> +		if (!log_wrapped) {
>>>> +			memcpy_fromio(axd->log_rbuf, log_buff, log_offset);
>>>> +			axd->log_rbuf_rem = log_offset;
>>>> +		} else {
>>>> +			char __iomem *pos = log_buff + log_offset;
>>>> +			unsigned int rem = log_size - log_offset;
>>>> +
>>>> +			memcpy_fromio(axd->log_rbuf, pos, rem);
>>>> +			memcpy_fromio(axd->log_rbuf + rem, log_buff, log_offset);
>>>> +			axd->log_rbuf_rem = log_size;
>>>> +		}
>>>> +		axd_platform_unlock(flags);
>>> I didn't see the lock being taken?
>> The lock is the first line in the block (unsigned int flags =
>> axd_platform_lock()). I'll tidy it up to make it more readable.
> It's very bad practice to bury lock taking in with the variable
> declaration.

Yes. I'll fix it.

>>>> +#ifdef CONFIG_CRYPTO_LZO
>>>> +#include <linux/crypto.h>
>>> This include should be with all the other includes, not down here.
>> Was trying to reduce the ifdefery. Will fix.
> You don't need any ifdefs for the include, you can just include the
> header.
>
>>>> +{
>>>> +	dev_err(axd->dev, "The firmware must be lzo decompressed first, compile driver again with CONFIG_CRYPTO_LZO enabled in kernel or do the decompression in user space.\n");
>>> Please split this up into a few prints for wrapping, similarly in
>>> several other places.
>> OK. I thought the convention for strings to leave them as is to allow
>> grepping. I'll fix it.
> You should keep strings that are displayed as a single string together
> but if you are splitting something in the output then that split won't
> hurt grepping in the source.
>
>>>> +	 * We copy through the cache, fw will do the necessary cache
>>>> +	 * flushes and syncing at startup.
>>>> +	 * Copying from uncached makes it more difficult for the
>>>> +	 * firmware to keep the caches coherent with memory when it sets
>>>> +	 * tlbs and start running.
>>>> +	 */
>>>> +	memcpy_toio((void *)cached_fw_base, fw->data, fw->size);
>>> Why the cast here?  I'm also not seeing where we handled the copying to
>>> I/O in the decompression case?
>> I couldn't avoid the cast. If cached_fw_base is 'void *' I'll get a warning
>> when initialising cached_fw_base from CAC_ADDR().
> Why do you get a warning from that?  Perhaps the warnings are trying to
> tell us something...

Because we try to assign an int to a pointer. So the error is 'makes 
pointer from integer without a cast'. To convert an address from 
uncached to cached we need to convert to an int as in MIPS it's a case 
of adding or subtracting a value then convert this value back to it's 
original form.
I'll see if I can find a better way to fix the coherency issue when we 
copy through uncached.

>
>> Good point. When decompressing crypto_comp_decompress() will write directly
>> to the memory. It is safe but it doesn't go through the correct API. Not
>> sure what I can do here.
> Uncompress to a buffer then write that buffer to the final destination?

Yes but the binary could be multi MiB so we can't get a temp buffer that 
large. If the crypto API allows decompressing in steps we can use a 
small buffer to move the data iteratively. I'll have a look.

>
>>>> +	dev_info(axd->dev, "Loading firmware at 0x%p ...\n", axd->fw_base_m);
>>> This should be _dbg() at most, otherwise it's going to get noisy.
>>>> +	t0_new_pc = (unsigned long) axd->fw_base_m + (t0_new_pc - AXD_BASE_VADDR);
>>> Those casts look fishy...
>> I am happy to try something else. axd->fw_base_m is of type void * __iomem
>> but we want to do some arithmetic on it.
>> Is there a better way to do it?
> Pointer arithmetic or converting it to a number?

We are just converting to a number.

>
>>>> +	/* wake up any task sleeping on command response */
>>>> +	wake_up(&axd->cmd.wait);
>>>> +	/* give chance to user land tasks to react to the crash */
>>>> +	ssleep(2);
>>> This looks horribly racy, I'd expect us to be trashing and/or killing
>>> off any active work and resources here.
>> OK. I was trying to play nicely by giving the chance to userland to repond
>> to -ERESTART which would be sent from aborting any pending reads/writes.
>> Are you suggesting to send SIGKILL using force_sig()?
> No, I'm suggesting tearing down the kernel side of any work and kicking
> errors back to userspace if it continues to interact with anything that
> was ongoing.

OK. This is what we do (see my other email about abort). I'll have a 
think for a way to get rid of the ssleep(). Any ideas are welcome.

Thanks,
Qais
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1218232 — Re: [PATCH 03/10] ALSA: add AXD Audio Processing IP alsa driver

[Multipart message — attachments visible in raw view] — view raw

On Fri, Aug 28, 2015 at 10:22:57AM +0100, Qais Yousef wrote:
> On 08/27/2015 04:32 PM, Mark Brown wrote:
> >On Thu, Aug 27, 2015 at 01:15:51PM +0100, Qais Yousef wrote:

> >>>>+#define AXD_BASE_VADDR		0xD0000000

> >>>This sounds like something that is going to be platform dependant,
> >>>should this be supplied from board configuration?

> >>I don't expect this to change. Can we add the configuration later if we hit
> >>the need to change it?

> >It should be trivial to make things configurable shouldn't it?

> Yes and I am all with configurability but I don't think it makes sense here.
> AXD will always have its own MMU and will not share virtual address space,
> so the possibility of us wanting to move this somewhere else is really very
> thin. Also I don't think this is the kind of detail we need to concern the
> user with. I'll see if I can make the binary header parsing more flexible so
> we can add more info like this and the one above in the future and be more
> future proof.

So this is a virtual address in the memory map of the DSP?  That's not
what I thought it was.

> >>>>+	memcpy_toio((void *)cached_fw_base, fw->data, fw->size);

> >>>Why the cast here?  I'm also not seeing where we handled the copying to
> >>>I/O in the decompression case?

> >>I couldn't avoid the cast. If cached_fw_base is 'void *' I'll get a warning
> >>when initialising cached_fw_base from CAC_ADDR().

> >Why do you get a warning from that?  Perhaps the warnings are trying to
> >tell us something...

> Because we try to assign an int to a pointer. So the error is 'makes pointer
> from integer without a cast'. To convert an address from uncached to cached
> we need to convert to an int as in MIPS it's a case of adding or subtracting
> a value then convert this value back to it's original form.
> I'll see if I can find a better way to fix the coherency issue when we copy
> through uncached.

Why can't you just use pointer arithmmetic?

> >>Good point. When decompressing crypto_comp_decompress() will write directly
> >>to the memory. It is safe but it doesn't go through the correct API. Not
> >>sure what I can do here.

> >Uncompress to a buffer then write that buffer to the final destination?

> Yes but the binary could be multi MiB so we can't get a temp buffer that
> large. If the crypto API allows decompressing in steps we can use a small
> buffer to move the data iteratively. I'll have a look.

A few megabytes doesn't seem like that big an ask (it's not *nice* but
it's doable with vmalloc()).  Iteratively copying is nicer though.

> >>>>+	/* wake up any task sleeping on command response */
> >>>>+	wake_up(&axd->cmd.wait);
> >>>>+	/* give chance to user land tasks to react to the crash */
> >>>>+	ssleep(2);
> >>>This looks horribly racy, I'd expect us to be trashing and/or killing
> >>>off any active work and resources here.
> >>OK. I was trying to play nicely by giving the chance to userland to repond
> >>to -ERESTART which would be sent from aborting any pending reads/writes.
> >>Are you suggesting to send SIGKILL using force_sig()?
> >No, I'm suggesting tearing down the kernel side of any work and kicking
> >errors back to userspace if it continues to interact with anything that
> >was ongoing.

> OK. This is what we do (see my other email about abort). I'll have a think
> for a way to get rid of the ssleep(). Any ideas are welcome.

Just delete it?

[toc] | [prev] | [next] | [standalone]

#1214145 — Re: [PATCH 06/10] ALSA: axd: add basic files for sending/receiving axd cmds

[Multipart message — attachments visible in raw view] — view raw

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_irq(struct axd_cmd *cmd, unsigned int irqnum)
> +{
> +	flush_workqueue(cmd->in_workq);

_sync()

> +	destroy_workqueue(cmd->in_workq);
> +	flush_workqueue(cmd->out_workq);
> +	destroy_workqueue(cmd->out_workq);
> +	free_irq(irqnum, cmd);

We're freeing the interrupts after we destroy the workqueue which means
we could try to schedule new work after destruction.

> +	/*
> +	 * Based on the defined axd_pipe->buf_size and number of input pipes
> +	 * supported by the firmware, we calculate the number of descriptors we
> +	 * need to use using this formula:
> +	 *
> +	 *	axd_pipe->buf_size * num_desc = total_size / num_inputs
> +	 */
> +	num_desc = total_size / (cmd->num_inputs * axd_pipe->buf_size);

I'm not sure that was an especially tricky line of code to follow...  am
I missing something here?

I've stopped reviewing here mostly because it's the end of my day and
this patch is 72K which is enormous for something that's not just lots
of defines or whatever and actually needs reading in considerable detail
given all the tricky concurrency stuff you're doing.  Please split this
code up into multiple patches for ease of review.  For example all the
queue management and allocation seems rather separate to the interrupt
handling.  

It also feels like there's room for pruning the code, perhaps sharing
more of it between input and output paths and removing some layers of
abstraction.  

[toc] | [prev] | [next] | [standalone]

#1214662 — 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 
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.

>
>> +	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.

>
>> +	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 error code for the default case.

>
>> +		case 2:
>> +			dev_warn(axd->dev, "Failed to set last configuration command\n");
>> +			break;
> Does the configuration command notice?

Yes. When send a configuration command we expect a response back that it 
was service (by setting resopnse_flg in AXD_INT_CTRL), we timeout if we 
don't get one and report an error to the caller.

This error code could mean other things as well so I might modify this 
message to be more descriptive.

>
>> +	/*
>> +	 * 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?

This is similar to how the bufferq implementation work. What is the 
other alternative to this? We do want this to be as fast as possible.

What is happening here is that the semaphore count is again controlling 
how many descriptors are available, if nothing is available it will 
cause the caller to block. If it succeeds and more than 1 descriptors is 
available potentially more than one SMP user could reach the later point 
so we hold the spinlock while modifying the shared buf_desc structure. 
The variable we're explicitly protecting is rd_idx.

Maybe my use of the semaphore count to keep track of how many 
descriptors are available and cause the caller to block is the confusing 
part? Would better comments help?

>
>> +void axd_cmd_free_irq(struct axd_cmd *cmd, unsigned int irqnum)
>> +{
>> +	flush_workqueue(cmd->in_workq);
> _sync()

OK.

>> +	destroy_workqueue(cmd->in_workq);
>> +	flush_workqueue(cmd->out_workq);
>> +	destroy_workqueue(cmd->out_workq);
>> +	free_irq(irqnum, cmd);
> We're freeing the interrupts after we destroy the workqueue which means
> we could try to schedule new work after destruction.

Right! I'll move it up.

>
>> +	/*
>> +	 * Based on the defined axd_pipe->buf_size and number of input pipes
>> +	 * supported by the firmware, we calculate the number of descriptors we
>> +	 * need to use using this formula:
>> +	 *
>> +	 *	axd_pipe->buf_size * num_desc = total_size / num_inputs
>> +	 */
>> +	num_desc = total_size / (cmd->num_inputs * axd_pipe->buf_size);
> I'm not sure that was an especially tricky line of code to follow...  am
> I missing something here?

The driver receive a pointer to a contiguous buffer area that it needs 
to divide it into buffers based on its size, number of pipes in the 
system, and the desired buffer size.

We then calculate our buffer queue size or how many out of the available 
descriptors we need.

For example if the total buffer area reserved for inputs is 10KiB and we 
have 1 input pipe and the desired buffer size is 1KiB, then we can use 
all 10 Descriptors AXD provides. If we have 2 input pipes in the system, 
then each 1 will take 5KiB and we need 5 descriptors for each pipe. It 
is equivalent to saying 'the size of input X buffer queue is 5'.

>
> I've stopped reviewing here mostly because it's the end of my day and
> this patch is 72K which is enormous for something that's not just lots
> of defines or whatever and actually needs reading in considerable detail
> given all the tricky concurrency stuff you're doing.  Please split this
> code up into multiple patches for ease of review.  For example all the
> queue management and allocation seems rather separate to the interrupt
> handling.

Thanks a lot for your efforts so far. I'll try to split this into 
smaller chunks though it feels really like it's all one entity but 2K of 
code is quite a lot.

>
> It also feels like there's room for pruning the code, perhaps sharing
> more of it between input and output paths and removing some layers of
> abstraction.

I'll look into that. If there's some specific suggestions in mind I'd 
appreciate hearing them.

Many thanks,
Qais
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web