diff mbox series

[08/12] ASoC: SOF: Generic probe compress operations

Message ID 20200124190413.18154-9-cezary.rojewski@intel.com (mailing list archive)
State New, archived
Headers show
Series ASoC: SOF: Data probing | expand

Commit Message

Cezary Rojewski Jan. 24, 2020, 7:04 p.m. UTC 
Define system-agnostic probe compress flow which serves as a base for
actual, hardware-dependent implementations.
As per firmware spec, maximum of one extraction stream is allowed, while
for injection, there can be plenty.

Apart from probe_pointer, all probe compress operations are mandatory.
Copy operation is defined as unified as its flow should be shared across
all SOF systems.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/sof/Kconfig    |   9 +++
 sound/soc/sof/Makefile   |   1 +
 sound/soc/sof/compress.c | 139 +++++++++++++++++++++++++++++++++++++++
 sound/soc/sof/compress.h |  29 ++++++++
 sound/soc/sof/ops.h      |  43 ++++++++++++
 sound/soc/sof/sof-priv.h |  21 ++++++
 6 files changed, 242 insertions(+)
 create mode 100644 sound/soc/sof/compress.c
 create mode 100644 sound/soc/sof/compress.h

Comments

Pierre-Louis Bossart Jan. 24, 2020, 7:41 p.m. UTC | #1 
> +config SND_SOC_SOF_DEBUG_PROBES
> +	bool "SOF enable data probing"
> +	select SND_SOC_COMPRESS
> +	help
> +	  This option enables the data probing feature that can be used to
> +	  gather data directly from specific points of the audio pipeline.
> +	  Say Y if you want to enable probes.
> +	  If unsure, select "N".
> +
>   endif ## SND_SOC_SOF_DEBUG
>   
>   endif ## SND_SOC_SOF_DEVELOPER_SUPPORT

This one is interesting.

Do we want to limit the PROBES to developers? Or do we want to enable 
probes on production firmware as well - which could be really useful for 
people tuning stuff on platform using production keys, i.e. without the 
ability to re-generate the firmware on their own.

And if the firmware does not include support for probes, we should 
detect it and I didn't see anything in this series that checks this 
capability? And if the firmware does not report it then it's a miss in 
the design.
Pierre-Louis Bossart Jan. 24, 2020, 8 p.m. UTC | #2 
> +int sof_probe_compr_open(struct snd_compr_stream *cstream,
> +		struct snd_soc_dai *dai)
> +{
> +	struct snd_sof_dev *sdev =
> +				snd_soc_component_get_drvdata(dai->component);
> +	int ret;
> +
> +	ret = snd_sof_probe_compr_assign(sdev, cstream, dai);
> +	if (ret < 0) {
> +		dev_err(dai->dev, "Failed to assign probe stream: %d\n", ret);
> +		return ret;
> +	}
> +
> +	sdev->extractor = ret;

could you either rename the 'extractor' field to something meaningful or 
add a comment on what is stored in this field? A stream tag? a device 
number? It's only used once for the init.
Cezary Rojewski Jan. 27, 2020, 12:23 p.m. UTC | #3 
On 2020-01-24 20:41, Pierre-Louis Bossart wrote:
> 
>> +config SND_SOC_SOF_DEBUG_PROBES
>> +    bool "SOF enable data probing"
>> +    select SND_SOC_COMPRESS
>> +    help
>> +      This option enables the data probing feature that can be used to
>> +      gather data directly from specific points of the audio pipeline.
>> +      Say Y if you want to enable probes.
>> +      If unsure, select "N".
>> +
>>   endif ## SND_SOC_SOF_DEBUG
>>   endif ## SND_SOC_SOF_DEVELOPER_SUPPORT
> 
> This one is interesting.
> 
> Do we want to limit the PROBES to developers? Or do we want to enable 
> probes on production firmware as well - which could be really useful for 
> people tuning stuff on platform using production keys, i.e. without the 
> ability to re-generate the firmware on their own.
> 
> And if the firmware does not include support for probes, we should 
> detect it and I didn't see anything in this series that checks this 
> capability? And if the firmware does not report it then it's a miss in 
> the design.

The only config I cared about is: _DEBUG as probes are a debug feature. 
Since the _DEVELOPER_SUPPORT is a superset of debug and several other 
features, it's obvious it ended up as a requirement.

Czarek
Cezary Rojewski Jan. 27, 2020, 12:28 p.m. UTC | #4 
On 2020-01-24 21:00, Pierre-Louis Bossart wrote:
> 
>> +int sof_probe_compr_open(struct snd_compr_stream *cstream,
>> +        struct snd_soc_dai *dai)
>> +{
>> +    struct snd_sof_dev *sdev =
>> +                snd_soc_component_get_drvdata(dai->component);
>> +    int ret;
>> +
>> +    ret = snd_sof_probe_compr_assign(sdev, cstream, dai);
>> +    if (ret < 0) {
>> +        dev_err(dai->dev, "Failed to assign probe stream: %d\n", ret);
>> +        return ret;
>> +    }
>> +
>> +    sdev->extractor = ret;
> 
> could you either rename the 'extractor' field to something meaningful or 
> add a comment on what is stored in this field? A stream tag? a device 
> number? It's only used once for the init.

'extractor' is _very_ meaningful and explicit so the naming should stay. 
Rewording to stream_tag / dev num or anything of that sort would achieve 
the exact opposite: ambiguity. Code around 'extractor' usage clearly 
states what it is for. I'd rather prefer such descriptions to stay 
within Documentation - which will be released on a later date as SOF's 
probes are very, very fresh subject.

Czarek
Pierre-Louis Bossart Jan. 27, 2020, 3:56 p.m. UTC | #5 
On 1/27/20 6:28 AM, Cezary Rojewski wrote:
> On 2020-01-24 21:00, Pierre-Louis Bossart wrote:
>>
>>> +int sof_probe_compr_open(struct snd_compr_stream *cstream,
>>> +        struct snd_soc_dai *dai)
>>> +{
>>> +    struct snd_sof_dev *sdev =
>>> +                snd_soc_component_get_drvdata(dai->component);
>>> +    int ret;
>>> +
>>> +    ret = snd_sof_probe_compr_assign(sdev, cstream, dai);
>>> +    if (ret < 0) {
>>> +        dev_err(dai->dev, "Failed to assign probe stream: %d\n", ret);
>>> +        return ret;
>>> +    }
>>> +
>>> +    sdev->extractor = ret;
>>
>> could you either rename the 'extractor' field to something meaningful 
>> or add a comment on what is stored in this field? A stream tag? a 
>> device number? It's only used once for the init.
> 
> 'extractor' is _very_ meaningful and explicit so the naming should stay. 
> Rewording to stream_tag / dev num or anything of that sort would achieve 
> the exact opposite: ambiguity. Code around 'extractor' usage clearly 
> states what it is for. I'd rather prefer such descriptions to stay 
> within Documentation - which will be released on a later date as SOF's 
> probes are very, very fresh subject.

'extractor' is an integer, and there's nothing that tells me what values 
it can take, and what it corresponds to.

You also store the result of 'probe_compr_assign' but don't use it in a 
free. The only place where I see it used is

+    ret = sof_ipc_probe_init(sdev, sdev->extractor, rtd->dma_bytes);

And then if look back at Patch 7, I see this:

+int sof_ipc_probe_init(struct snd_sof_dev *sdev,
+		u32 stream_tag, size_t buffer_size)

so your 'extractor' is really an 'extractor_stream_tag'.

BTW please try and compile with W=1, you'll see kernel-doc errors with 
missing parameters, thanks!
diff mbox series

Patch

diff --git a/sound/soc/sof/Kconfig b/sound/soc/sof/Kconfig
index 827b0ec92522..0fca86164472 100644
--- a/sound/soc/sof/Kconfig
+++ b/sound/soc/sof/Kconfig
@@ -171,6 +171,15 @@  config SND_SOC_SOF_DEBUG_RETAIN_DSP_CONTEXT
 	  Say Y if you want to retain DSP context for FW exceptions.
 	  If unsure, select "N".
 
+config SND_SOC_SOF_DEBUG_PROBES
+	bool "SOF enable data probing"
+	select SND_SOC_COMPRESS
+	help
+	  This option enables the data probing feature that can be used to
+	  gather data directly from specific points of the audio pipeline.
+	  Say Y if you want to enable probes.
+	  If unsure, select "N".
+
 endif ## SND_SOC_SOF_DEBUG
 
 endif ## SND_SOC_SOF_DEVELOPER_SUPPORT
diff --git a/sound/soc/sof/Makefile b/sound/soc/sof/Makefile
index 60f7f13218a7..4486ea5b2c71 100644
--- a/sound/soc/sof/Makefile
+++ b/sound/soc/sof/Makefile
@@ -4,6 +4,7 @@  ccflags-y += -DDEBUG
 
 snd-sof-objs := core.o ops.o loader.o ipc.o pcm.o pm.o debug.o topology.o\
 		control.o probe.o trace.o utils.o sof-audio.o
+snd-sof-$(CONFIG_SND_SOC_SOF_DEBUG_PROBES) += compress.o
 
 snd-sof-pci-objs := sof-pci-dev.o
 snd-sof-acpi-objs := sof-acpi-dev.o
diff --git a/sound/soc/sof/compress.c b/sound/soc/sof/compress.c
new file mode 100644
index 000000000000..6bb057da0ac4
--- /dev/null
+++ b/sound/soc/sof/compress.c
@@ -0,0 +1,139 @@ 
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+//
+// This file is provided under a dual BSD/GPLv2 license.  When using or
+// redistributing this file, you may do so under either license.
+//
+// Copyright(c) 2019 Intel Corporation. All rights reserved.
+//
+// Author: Cezary Rojewski <cezary.rojewski@intel.com>
+//
+
+#include <sound/soc.h>
+#include "compress.h"
+#include "ops.h"
+#include "probe.h"
+
+int sof_probe_compr_open(struct snd_compr_stream *cstream,
+		struct snd_soc_dai *dai)
+{
+	struct snd_sof_dev *sdev =
+				snd_soc_component_get_drvdata(dai->component);
+	int ret;
+
+	ret = snd_sof_probe_compr_assign(sdev, cstream, dai);
+	if (ret < 0) {
+		dev_err(dai->dev, "Failed to assign probe stream: %d\n", ret);
+		return ret;
+	}
+
+	sdev->extractor = ret;
+	return 0;
+}
+EXPORT_SYMBOL(sof_probe_compr_open);
+
+int sof_probe_compr_free(struct snd_compr_stream *cstream,
+		struct snd_soc_dai *dai)
+{
+	struct snd_sof_dev *sdev =
+				snd_soc_component_get_drvdata(dai->component);
+	struct sof_probe_point_desc *desc;
+	size_t num_desc;
+	int i, ret;
+
+	/* disconnect all probe points */
+	ret = sof_ipc_probe_points_info(sdev, &desc, &num_desc);
+	if (ret < 0) {
+		dev_err(dai->dev, "Failed to get probe points: %d\n", ret);
+		goto exit;
+	}
+
+	for (i = 0; i < num_desc; i++)
+		sof_ipc_probe_points_remove(sdev, &desc[i].buffer_id, 1);
+	kfree(desc);
+
+exit:
+	ret = sof_ipc_probe_deinit(sdev);
+	if (ret < 0)
+		dev_err(dai->dev, "Failed to deinit probe: %d\n", ret);
+
+	snd_compr_free_pages(cstream);
+
+	return snd_sof_probe_compr_free(sdev, cstream, dai);
+}
+EXPORT_SYMBOL(sof_probe_compr_free);
+
+int sof_probe_compr_set_params(struct snd_compr_stream *cstream,
+		struct snd_compr_params *params, struct snd_soc_dai *dai)
+{
+	struct snd_compr_runtime *rtd = cstream->runtime;
+	struct snd_sof_dev *sdev =
+				snd_soc_component_get_drvdata(dai->component);
+	int ret;
+
+	cstream->dma_buffer.dev.type = SNDRV_DMA_TYPE_DEV_SG;
+	cstream->dma_buffer.dev.dev = sdev->dev;
+	ret = snd_compr_malloc_pages(cstream, rtd->buffer_size);
+	if (ret < 0)
+		return ret;
+
+	ret = snd_sof_probe_compr_set_params(sdev, cstream, params, dai);
+	if (ret < 0)
+		return ret;
+
+	ret = sof_ipc_probe_init(sdev, sdev->extractor, rtd->dma_bytes);
+	if (ret < 0) {
+		dev_err(dai->dev, "Failed to init probe: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(sof_probe_compr_set_params);
+
+int sof_probe_compr_trigger(struct snd_compr_stream *cstream, int cmd,
+		struct snd_soc_dai *dai)
+{
+	struct snd_sof_dev *sdev =
+				snd_soc_component_get_drvdata(dai->component);
+
+	return snd_sof_probe_compr_trigger(sdev, cstream, cmd, dai);
+}
+EXPORT_SYMBOL(sof_probe_compr_trigger);
+
+int sof_probe_compr_pointer(struct snd_compr_stream *cstream,
+		struct snd_compr_tstamp *tstamp, struct snd_soc_dai *dai)
+{
+	struct snd_sof_dev *sdev =
+				snd_soc_component_get_drvdata(dai->component);
+
+	return snd_sof_probe_compr_pointer(sdev, cstream, tstamp, dai);
+}
+EXPORT_SYMBOL(sof_probe_compr_pointer);
+
+int sof_probe_compr_copy(struct snd_compr_stream *cstream,
+		char __user *buf, size_t count)
+{
+	struct snd_compr_runtime *rtd = cstream->runtime;
+	unsigned int offset, n;
+	void *ptr;
+	int ret;
+
+	if (count > rtd->buffer_size)
+		count = rtd->buffer_size;
+
+	div_u64_rem(rtd->total_bytes_transferred, rtd->buffer_size, &offset);
+	ptr = rtd->dma_area + offset;
+	n = rtd->buffer_size - offset;
+
+	if (count < n) {
+		ret = copy_to_user(buf, ptr, count);
+	} else {
+		ret = copy_to_user(buf, ptr, n);
+		ret += copy_to_user(buf + n, rtd->dma_area, count - n);
+	}
+
+	if (ret)
+		return count - ret;
+	return count;
+}
+EXPORT_SYMBOL(sof_probe_compr_copy);
diff --git a/sound/soc/sof/compress.h b/sound/soc/sof/compress.h
new file mode 100644
index 000000000000..7df1a5d60d78
--- /dev/null
+++ b/sound/soc/sof/compress.h
@@ -0,0 +1,29 @@ 
+/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) */
+/*
+ * This file is provided under a dual BSD/GPLv2 license.  When using or
+ * redistributing this file, you may do so under either license.
+ *
+ * Copyright(c) 2019 Intel Corporation. All rights reserved.
+ *
+ * Author: Cezary Rojewski <cezary.rojewski@intel.com>
+ */
+
+#ifndef __SOF_COMPRESS_H
+#define __SOF_COMPRESS_H
+
+#include <sound/compress_driver.h>
+
+int sof_probe_compr_open(struct snd_compr_stream *cstream,
+		struct snd_soc_dai *dai);
+int sof_probe_compr_free(struct snd_compr_stream *cstream,
+		struct snd_soc_dai *dai);
+int sof_probe_compr_set_params(struct snd_compr_stream *cstream,
+		struct snd_compr_params *params, struct snd_soc_dai *dai);
+int sof_probe_compr_trigger(struct snd_compr_stream *cstream, int cmd,
+		struct snd_soc_dai *dai);
+int sof_probe_compr_pointer(struct snd_compr_stream *cstream,
+		struct snd_compr_tstamp *tstamp, struct snd_soc_dai *dai);
+int sof_probe_compr_copy(struct snd_compr_stream *cstream,
+		char __user *buf, size_t count);
+
+#endif
diff --git a/sound/soc/sof/ops.h b/sound/soc/sof/ops.h
index 7f532bcc8e9d..a771500ac442 100644
--- a/sound/soc/sof/ops.h
+++ b/sound/soc/sof/ops.h
@@ -393,6 +393,49 @@  snd_sof_pcm_platform_pointer(struct snd_sof_dev *sdev,
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES)
+static inline int
+snd_sof_probe_compr_assign(struct snd_sof_dev *sdev,
+		struct snd_compr_stream *cstream, struct snd_soc_dai *dai)
+{
+	return sof_ops(sdev)->probe_assign(sdev, cstream, dai);
+}
+
+static inline int
+snd_sof_probe_compr_free(struct snd_sof_dev *sdev,
+		struct snd_compr_stream *cstream, struct snd_soc_dai *dai)
+{
+	return sof_ops(sdev)->probe_free(sdev, cstream, dai);
+}
+
+static inline int
+snd_sof_probe_compr_set_params(struct snd_sof_dev *sdev,
+		struct snd_compr_stream *cstream,
+		struct snd_compr_params *params, struct snd_soc_dai *dai)
+{
+	return sof_ops(sdev)->probe_set_params(sdev, cstream, params, dai);
+}
+
+static inline int
+snd_sof_probe_compr_trigger(struct snd_sof_dev *sdev,
+		struct snd_compr_stream *cstream, int cmd,
+		struct snd_soc_dai *dai)
+{
+	return sof_ops(sdev)->probe_trigger(sdev, cstream, cmd, dai);
+}
+
+static inline int
+snd_sof_probe_compr_pointer(struct snd_sof_dev *sdev,
+		struct snd_compr_stream *cstream,
+		struct snd_compr_tstamp *tstamp, struct snd_soc_dai *dai)
+{
+	if (sof_ops(sdev) && sof_ops(sdev)->probe_pointer)
+		return sof_ops(sdev)->probe_pointer(sdev, cstream, tstamp, dai);
+
+	return 0;
+}
+#endif
+
 /* machine driver */
 static inline int
 snd_sof_machine_register(struct snd_sof_dev *sdev, void *pdata)
diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h
index d9188b82acdc..47e6b48d6f03 100644
--- a/sound/soc/sof/sof-priv.h
+++ b/sound/soc/sof/sof-priv.h
@@ -170,6 +170,27 @@  struct snd_sof_dsp_ops {
 	snd_pcm_uframes_t (*pcm_pointer)(struct snd_sof_dev *sdev,
 					 struct snd_pcm_substream *substream); /* optional */
 
+#if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES)
+	/* Except for probe_pointer, all probe ops are mandatory */
+	int (*probe_assign)(struct snd_sof_dev *sdev,
+			struct snd_compr_stream *cstream,
+			struct snd_soc_dai *dai); /* mandatory */
+	int (*probe_free)(struct snd_sof_dev *sdev,
+			struct snd_compr_stream *cstream,
+			struct snd_soc_dai *dai); /* mandatory */
+	int (*probe_set_params)(struct snd_sof_dev *sdev,
+			struct snd_compr_stream *cstream,
+			struct snd_compr_params *params,
+			struct snd_soc_dai *dai); /* mandatory */
+	int (*probe_trigger)(struct snd_sof_dev *sdev,
+			struct snd_compr_stream *cstream, int cmd,
+			struct snd_soc_dai *dai); /* mandatory */
+	int (*probe_pointer)(struct snd_sof_dev *sdev,
+			struct snd_compr_stream *cstream,
+			struct snd_compr_tstamp *tstamp,
+			struct snd_soc_dai *dai); /* optional */
+#endif
+
 	/* host read DSP stream data */
 	void (*ipc_msg_data)(struct snd_sof_dev *sdev,
 			     struct snd_pcm_substream *substream,