[PATCH 0/4] ALSA: Updates for the CA0132 codec

[PATCH 0/4] ALSA: Updates for the CA0132 codec

[Ian Minett]

From: Ian Minett <ian_minett@creativelabs.com>

Hi,

This series contains the following updates to CA0132:

1: Add firmware loading for the SpeakerEQ DSP effect.
2: Improve DSP transfer timeout calculations.
3: Add stream and effect latency monitoring routines 
4: Update hp unsolicited event handler to manage queued work.

These were based on the latest sound.git, but please let me know if 
they should be against sound-unstable tree instead.

Thanks very much,
- Ian

Signed-off-by: Ian Minett <ian_minett@creativelabs.com>

[PATCH 1/4] ALSA: CA0132: Add SpeakerEQ feature firmware loading

[Ian Minett]

From: Ian Minett <ian_minett@creativelabs.com>

Use request_firmware() to fetch an image containing EQ data. 
This is then loaded onto the DSP chip for use by the SpeakerEQ effect.
The DSP will carry on as normal without the EQ settings in the event that 
the SpeakerEQ f/w is missing or fails to load.

Signed-off-by: Ian Minett <ian_minett@creativelabs.com>

diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c
index 639a282..a4b61a9 100644
--- a/sound/pci/hda/patch_ca0132.c
+++ b/sound/pci/hda/patch_ca0132.c
@@ -73,9 +73,11 @@
 #define SCP_SET    0
 #define SCP_GET    1
 
+#define SPEQ_FILE  "ctspeq.bin"
 #define EFX_FILE   "ctefx.bin"
 
 #ifdef CONFIG_SND_HDA_CODEC_CA0132_DSP
+MODULE_FIRMWARE(SPEQ_FILE);
 MODULE_FIRMWARE(EFX_FILE);
 #endif
 
@@ -2516,6 +2518,42 @@ exit:
 	return status;
 }
 
+/**
+ * Write the SpeakerEQ coefficient data to DSP memories
+ *
+ * @codec: the HDA codec
+ * @x,y:   location to write x and y coeff data to
+ *
+ * Returns zero or a negative error code.
+ */
+static int dspload_get_speakereq_addx(struct hda_codec *codec,
+				unsigned int *x,
+				unsigned int *y)
+{
+	int status = 0;
+	struct { unsigned short y, x; } speakereq_info;
+	unsigned int size = sizeof(speakereq_info);
+
+	snd_printdd(KERN_INFO "dspload_get_speakereq_addx() -- begin");
+	status = dspio_scp(codec, MASTERCONTROL,
+			MASTERCONTROL_QUERY_SPEAKER_EQ_ADDRESS,
+			SCP_GET, NULL, 0, &speakereq_info, &size);
+
+	if (status < 0) {
+		snd_printdd(KERN_INFO "dspload_get_speakereq_addx: SCP Failed");
+		return -EIO;
+	}
+
+	*x = speakereq_info.x;
+	*y = speakereq_info.y;
+	snd_printdd(KERN_INFO "dspload_get_speakereq_addx: X=0x%x Y=0x%x\n",
+		    *x, *y);
+
+	snd_printdd(KERN_INFO "dspload_get_speakereq_addx() -- complete");
+
+	return status;
+}
+
 /*
  * CA0132 DSP download stuffs.
  */
@@ -2602,6 +2640,35 @@ static int dspload_image(struct hda_codec *codec,
 	return status;
 }
 
+static int dspload_speakereq(struct hda_codec *codec)
+{
+	int status = 0;
+	const struct dsp_image_seg *image;
+	unsigned int x, y;
+	const struct firmware *fw_speq;
+
+	snd_printdd(KERN_INFO "dspload_speakereq() -- begin");
+
+	if (request_firmware(&fw_speq, SPEQ_FILE,
+			     codec->bus->card->dev) != 0)
+		return -EIO;
+
+	image = (struct dsp_image_seg *)(fw_speq->data);
+
+	status = dspload_get_speakereq_addx(codec, &x, &y);
+	if (status < 0)
+		goto done;
+
+	status = dspload_image(codec, image, 1, y, 0, 8);
+
+done:
+	release_firmware(fw_speq);
+
+	snd_printdd(KERN_INFO "dspload_speakereq() -- complete");
+
+	return status;
+}
+
 static bool dspload_is_loaded(struct hda_codec *codec)
 {
 	unsigned int data = 0;
@@ -4335,6 +4402,8 @@ static bool ca0132_download_dsp_images(struct hda_codec *codec)
 
 	release_firmware(fw_entry);
 
+	if (dsp_loaded)
+		dspload_speakereq(codec);
 
 	return dsp_loaded;
 }
-- 
1.7.4.1

[PATCH 2/4] ALSA: CA0132: Improve the DSP transfer timeout calculations

[Ian Minett]

From: Ian Minett <ian_minett@creativelabs.com>

Base the DSP firmware transfer and communication timeouts on jiffy values.

Signed-off-by: Ian Minett <ian_minett@creativelabs.com>

diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c
index a4b61a9..b52f00c 100644
--- a/sound/pci/hda/patch_ca0132.c
+++ b/sound/pci/hda/patch_ca0132.c
@@ -785,7 +785,7 @@ static int chipio_send(struct hda_codec *codec,
 		       unsigned int data)
 {
 	unsigned int res;
-	int retry = 50;
+	unsigned long timeout = jiffies + msecs_to_jiffies(1000);
 
 	/* send bits of data specified by reg */
 	do {
@@ -793,7 +793,9 @@ static int chipio_send(struct hda_codec *codec,
 					 reg, data);
 		if (res == VENDOR_STATUS_CHIPIO_OK)
 			return 0;
-	} while (--retry);
+		msleep(20);
+	} while (time_before(jiffies, timeout));
+
 	return -EIO;
 }
 
@@ -1059,14 +1061,15 @@ static int dspio_send(struct hda_codec *codec, unsigned int reg,
 		      unsigned int data)
 {
 	int res;
-	int retry = 50;
+	unsigned long timeout = jiffies + msecs_to_jiffies(1000);
 
 	/* send bits of data specified by reg to dsp */
 	do {
 		res = snd_hda_codec_read(codec, WIDGET_DSP_CTRL, 0, reg, data);
 		if ((res >= 0) && (res != VENDOR_STATUS_DSPIO_BUSY))
 			return res;
-	} while (--retry);
+		msleep(20);
+	} while (time_before(jiffies, timeout));
 
 	return -EIO;
 }
@@ -1298,7 +1301,6 @@ static int dspio_send_scp_message(struct hda_codec *codec,
 				  unsigned int *bytes_returned)
 {
 	struct ca0132_spec *spec = codec->spec;
-	int retry;
 	int status = -1;
 	unsigned int scp_send_size = 0;
 	unsigned int total_size;
@@ -1345,13 +1347,13 @@ static int dspio_send_scp_message(struct hda_codec *codec,
 	}
 
 	if (waiting_for_resp) {
+		unsigned long timeout = jiffies + msecs_to_jiffies(1000);
 		memset(return_buf, 0, return_buf_size);
-		retry = 50;
 		do {
 			msleep(20);
-		} while (spec->wait_scp && (--retry != 0));
+		} while (spec->wait_scp && time_before(jiffies, timeout));
 		waiting_for_resp = false;
-		if (retry != 0) {
+		if (!spec->wait_scp) {
 			ret_msg = (struct scp_msg *)return_buf;
 			memcpy(&ret_msg->hdr, &spec->scp_resp_header, 4);
 			memcpy(&ret_msg->data, spec->scp_resp_data,
@@ -2244,7 +2246,8 @@ static int dspxfr_one_seg(struct hda_codec *codec,
 	u32 chip_addx_remainder;
 	unsigned int run_size_words;
 	const struct dsp_image_seg *hci_write = NULL;
-	int retry;
+	unsigned long timeout;
+	bool dma_active;
 
 	if (fls == NULL)
 		return -EINVAL;
@@ -2362,11 +2365,17 @@ static int dspxfr_one_seg(struct hda_codec *codec,
 			status = dspxfr_hci_write(codec, hci_write);
 			hci_write = NULL;
 		}
-		retry = 5000;
-		while (dsp_is_dma_active(codec, dma_chan)) {
-			if (--retry <= 0)
+
+		timeout = jiffies + msecs_to_jiffies(2000);
+		do {
+			dma_active = dsp_is_dma_active(codec, dma_chan);
+			if (!dma_active)
 				break;
-		}
+			msleep(20);
+		} while (time_before(jiffies, timeout));
+		if (dma_active)
+			break;
+
 		snd_printdd(KERN_INFO "+++++ DMA complete");
 		dma_set_state(dma_engine, DMA_STATE_STOP);
 		dma_reset(dma_engine);
@@ -2683,15 +2692,15 @@ static bool dspload_is_loaded(struct hda_codec *codec)
 
 static bool dspload_wait_loaded(struct hda_codec *codec)
 {
-	int retry = 100;
+	unsigned long timeout = jiffies + msecs_to_jiffies(2000);
 
 	do {
-		msleep(20);
 		if (dspload_is_loaded(codec)) {
 			pr_info("ca0132 DOWNLOAD OK :-) DSP IS RUNNING.\n");
 			return true;
 		}
-	} while (--retry);
+		msleep(20);
+	} while (time_before(jiffies, timeout));
 
 	pr_err("ca0132 DOWNLOAD FAILED!!! DSP IS NOT RUNNING.\n");
 	return false;
-- 
1.7.4.1

[PATCH 3/4] ALSA: CA0132: Add latency monitoring

[Ian Minett]

From: Ian Minett <ian_minett@creativelabs.com>

Add latency monitoring for playback, capture and DSP effects.

Signed-off-by: Ian Minett <ian_minett@creativelabs.com>

diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c
index b52f00c..c797f65 100644
--- a/sound/pci/hda/patch_ca0132.c
+++ b/sound/pci/hda/patch_ca0132.c
@@ -133,6 +133,12 @@ enum {
 /* Effects values size*/
 #define EFFECT_VALS_MAX_COUNT 12
 
+#define DSP_CAPTURE_INIT_LATENCY        0
+#define DSP_CRYSTAL_VOICE_LATENCY       124
+#define DSP_PLAYBACK_INIT_LATENCY       13
+#define DSP_PLAY_ENHANCEMENT_LATENCY    30
+#define DSP_SPEAKER_OUT_LATENCY         7
+
 struct ct_effect {
 	char name[44];
 	hda_nid_t nid;
@@ -2761,8 +2767,27 @@ static void ca0132_cleanup_stream(struct hda_codec *codec, hda_nid_t nid)
 }
 
 /*
- * PCM callbacks
+ * PCM playbacks
  */
+static unsigned int ca0132_get_playback_latency(struct hda_codec *codec)
+{
+	struct ca0132_spec *spec = codec->spec;
+	unsigned int latency = DSP_PLAYBACK_INIT_LATENCY;
+
+	if (!dspload_is_loaded(codec))
+		return 0;
+
+	if (spec->effects_switch[PLAY_ENHANCEMENT - EFFECT_START_NID]) {
+		if ((spec->effects_switch[SURROUND - EFFECT_START_NID]) ||
+		    (spec->effects_switch[DIALOG_PLUS - EFFECT_START_NID]))
+			latency += DSP_PLAY_ENHANCEMENT_LATENCY;
+	}
+
+	if (spec->cur_out_type == SPEAKER_OUT)
+		latency += DSP_SPEAKER_OUT_LATENCY;
+	return latency;
+}
+
 static int ca0132_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
 			struct hda_codec *codec,
 			unsigned int stream_tag,
@@ -2770,6 +2795,16 @@ static int ca0132_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
 			struct snd_pcm_substream *substream)
 {
 	struct ca0132_spec *spec = codec->spec;
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	unsigned int latency = ca0132_get_playback_latency(codec);
+
+	if (spec->dsp_state == DSP_DOWNLOADING) {
+		spec->dsp_stream_id = stream_tag;
+		return 0;
+	}
+
+	runtime->delay = bytes_to_frames(runtime, (latency * runtime->rate *
+					runtime->byte_align) / 1000);
 
 	ca0132_setup_stream(codec, spec->dacs[0], stream_tag, 0, format);
 
@@ -2834,8 +2869,22 @@ static int ca0132_dig_playback_pcm_close(struct hda_pcm_stream *hinfo,
 }
 
 /*
- * Analog capture
+ * PCM capture
  */
+
+static unsigned int ca0132_get_capture_latency(struct hda_codec *codec)
+{
+	struct ca0132_spec *spec = codec->spec;
+	unsigned int latency = DSP_CAPTURE_INIT_LATENCY;
+
+	if (!dspload_is_loaded(codec))
+		return 0;
+
+	if (spec->effects_switch[CRYSTAL_VOICE - EFFECT_START_NID])
+		latency += DSP_CRYSTAL_VOICE_LATENCY;
+	return latency;
+}
+
 static int ca0132_capture_pcm_prepare(struct hda_pcm_stream *hinfo,
 					struct hda_codec *codec,
 					unsigned int stream_tag,
@@ -2843,9 +2892,16 @@ static int ca0132_capture_pcm_prepare(struct hda_pcm_stream *hinfo,
 					struct snd_pcm_substream *substream)
 {
 	struct ca0132_spec *spec = codec->spec;
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	unsigned int latency = ca0132_get_capture_latency(codec);
 
-	ca0132_setup_stream(codec, spec->adcs[substream->number],
-			    stream_tag, 0, format);
+	if (spec->dsp_state == DSP_DOWNLOADING)
+		return 0;
+
+	runtime->delay = bytes_to_frames(runtime, (latency * runtime->rate *
+					runtime->byte_align) / 1000);
+
+	ca0132_setup_stream(codec, hinfo->nid, stream_tag, 0, format);
 
 	return 0;
 }
@@ -3429,6 +3485,33 @@ static int ca0132_voicefx_set(struct hda_codec *codec, int enable)
 	return 1;
 }
 
+static void ca0132_update_latency(struct hda_codec *codec,
+				  unsigned int direction)
+{
+	struct ca0132_spec *spec = codec->spec;
+	unsigned int latency;
+	struct snd_pcm_substream *substr;
+	struct snd_pcm_runtime *runtime;
+
+	if (spec->dsp_state == DSP_DOWNLOADING)
+		return;
+
+	latency = (direction == SNDRV_PCM_STREAM_PLAYBACK)
+			? ca0132_get_playback_latency(codec)
+			: ca0132_get_capture_latency(codec);
+
+	substr = codec->pcm_info->pcm->streams[direction].substream;
+	while (substr) {
+		runtime = substr->runtime;
+		/* update latency only when frame_bits is set */
+		if (runtime && runtime->frame_bits)
+			runtime->delay = bytes_to_frames(runtime,
+					(latency * runtime->rate *
+					runtime->byte_align) / 1000);
+		substr = substr->next;
+	}
+}
+
 /*
  * Set the effects parameters
  */
@@ -3468,6 +3551,9 @@ static int ca0132_effects_set(struct hda_codec *codec, hda_nid_t nid, long val)
 	err = dspio_set_uint_param(codec, ca0132_effects[idx].mid,
 				   ca0132_effects[idx].reqs[0], on);
 
+	ca0132_update_latency(codec, SNDRV_PCM_STREAM_PLAYBACK);
+	ca0132_update_latency(codec, SNDRV_PCM_STREAM_CAPTURE);
+
 	if (err < 0)
 		return 0; /* no changed */
 
-- 
1.7.4.1

[PATCH 4/4] ALSA: CA0132: Update hp unsol event handler

[Ian Minett]

From: Ian Minett <ian_minett@creativelabs.com>

Manage the delayed work queue in hp unsol event handler.

Signed-off-by: Ian Minett <ian_minett@creativelabs.com>

diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c
index c797f65..61d74db 100644
--- a/sound/pci/hda/patch_ca0132.c
+++ b/sound/pci/hda/patch_ca0132.c
@@ -749,6 +749,9 @@ struct ca0132_spec {
 	long voicefx_val;
 	long cur_mic_boost;
 
+	struct hda_codec *codec;
+	struct delayed_work unsol_hp_work;
+
 #ifdef ENABLE_TUNING_CONTROLS
 	long cur_ctl_vals[TUNING_CTLS_COUNT];
 #endif
@@ -3338,6 +3341,14 @@ exit:
 	return err < 0 ? err : 0;
 }
 
+static void ca0132_unsol_hp_delayed(struct work_struct *work)
+{
+	struct ca0132_spec *spec = container_of(
+		to_delayed_work(work), struct ca0132_spec, unsol_hp_work);
+	ca0132_select_out(spec->codec);
+	snd_hda_jack_report_sync(spec->codec);
+}
+
 static void ca0132_set_dmic(struct hda_codec *codec, int enable);
 static int ca0132_mic_boost_set(struct hda_codec *codec, long val);
 static int ca0132_effects_set(struct hda_codec *codec, hda_nid_t nid, long val);
@@ -4540,8 +4551,9 @@ static void ca0132_process_dsp_response(struct hda_codec *codec)
 
 static void ca0132_unsol_event(struct hda_codec *codec, unsigned int res)
 {
-	snd_printdd(KERN_INFO "ca0132_unsol_event: 0x%x\n", res);
+	struct ca0132_spec *spec = codec->spec;
 
+	snd_printdd(KERN_INFO "ca0132_unsol_event: 0x%x\n", res);
 
 	if (((res >> AC_UNSOL_RES_TAG_SHIFT) & 0x3f) == UNSOL_TAG_DSP) {
 		ca0132_process_dsp_response(codec);
@@ -4553,8 +4565,10 @@ static void ca0132_unsol_event(struct hda_codec *codec, unsigned int res)
 
 		switch (res) {
 		case UNSOL_TAG_HP:
-			ca0132_select_out(codec);
-			snd_hda_jack_report_sync(codec);
+			cancel_delayed_work_sync(&spec->unsol_hp_work);
+			queue_delayed_work(codec->bus->workq,
+					   &spec->unsol_hp_work,
+					   msecs_to_jiffies(500));
 			break;
 		case UNSOL_TAG_AMIC1:
 			ca0132_select_mic(codec);
@@ -4729,6 +4743,7 @@ static void ca0132_free(struct hda_codec *codec)
 {
 	struct ca0132_spec *spec = codec->spec;
 
+	cancel_delayed_work_sync(&spec->unsol_hp_work);
 	snd_hda_power_up(codec);
 	snd_hda_sequence_write(codec, spec->base_exit_verbs);
 	ca0132_exit_chip(codec);
@@ -4794,6 +4809,7 @@ static int patch_ca0132(struct hda_codec *codec)
 	if (!spec)
 		return -ENOMEM;
 	codec->spec = spec;
+	spec->codec = codec;
 
 	spec->num_mixers = 1;
 	spec->mixers[0] = ca0132_mixer;
@@ -4804,6 +4820,8 @@ static int patch_ca0132(struct hda_codec *codec)
 	spec->init_verbs[1] = ca0132_init_verbs1;
 	spec->num_init_verbs = 2;
 
+	INIT_DELAYED_WORK(&spec->unsol_hp_work, ca0132_unsol_hp_delayed);
+
 	ca0132_init_chip(codec);
 
 	ca0132_config(codec);
-- 
1.7.4.1

Re: [PATCH 1/4] ALSA: CA0132: Add SpeakerEQ feature firmware loading

[Takashi Iwai]

At Fri, 8 Feb 2013 18:31:42 -0800,
Ian Minett wrote:
> 
> From: Ian Minett <ian_minett@creativelabs.com>
> 
> Use request_firmware() to fetch an image containing EQ data. 
> This is then loaded onto the DSP chip for use by the SpeakerEQ effect.
> The DSP will carry on as normal without the EQ settings in the event that 
> the SpeakerEQ f/w is missing or fails to load.

Well, it's almost close to feature freeze for the next merge window,
I'll postpone this.  (And unless we get the firmware for testing.)

In addition, some questions about the patch:

> 
> Signed-off-by: Ian Minett <ian_minett@creativelabs.com>
> 
> diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c
> index 639a282..a4b61a9 100644
> --- a/sound/pci/hda/patch_ca0132.c
> +++ b/sound/pci/hda/patch_ca0132.c
> @@ -73,9 +73,11 @@
>  #define SCP_SET    0
>  #define SCP_GET    1
>  
> +#define SPEQ_FILE  "ctspeq.bin"
>  #define EFX_FILE   "ctefx.bin"
>  
>  #ifdef CONFIG_SND_HDA_CODEC_CA0132_DSP
> +MODULE_FIRMWARE(SPEQ_FILE);
>  MODULE_FIRMWARE(EFX_FILE);
>  #endif
>  
> @@ -2516,6 +2518,42 @@ exit:
>  	return status;
>  }
>  
> +/**
> + * Write the SpeakerEQ coefficient data to DSP memories
> + *
> + * @codec: the HDA codec
> + * @x,y:   location to write x and y coeff data to
> + *
> + * Returns zero or a negative error code.
> + */
> +static int dspload_get_speakereq_addx(struct hda_codec *codec,
> +				unsigned int *x,
> +				unsigned int *y)
> +{
> +	int status = 0;
> +	struct { unsigned short y, x; } speakereq_info;

Is this endian-independent?

> +	unsigned int size = sizeof(speakereq_info);

This must be 32bit, right?


Takashi

> +	snd_printdd(KERN_INFO "dspload_get_speakereq_addx() -- begin");
> +	status = dspio_scp(codec, MASTERCONTROL,
> +			MASTERCONTROL_QUERY_SPEAKER_EQ_ADDRESS,
> +			SCP_GET, NULL, 0, &speakereq_info, &size);
> +
> +	if (status < 0) {
> +		snd_printdd(KERN_INFO "dspload_get_speakereq_addx: SCP Failed");
> +		return -EIO;
> +	}
> +
> +	*x = speakereq_info.x;
> +	*y = speakereq_info.y;
> +	snd_printdd(KERN_INFO "dspload_get_speakereq_addx: X=0x%x Y=0x%x\n",
> +		    *x, *y);
> +
> +	snd_printdd(KERN_INFO "dspload_get_speakereq_addx() -- complete");
> +
> +	return status;
> +}
> +
>  /*
>   * CA0132 DSP download stuffs.
>   */
> @@ -2602,6 +2640,35 @@ static int dspload_image(struct hda_codec *codec,
>  	return status;
>  }
>  
> +static int dspload_speakereq(struct hda_codec *codec)
> +{
> +	int status = 0;
> +	const struct dsp_image_seg *image;
> +	unsigned int x, y;
> +	const struct firmware *fw_speq;
> +
> +	snd_printdd(KERN_INFO "dspload_speakereq() -- begin");
> +
> +	if (request_firmware(&fw_speq, SPEQ_FILE,
> +			     codec->bus->card->dev) != 0)
> +		return -EIO;
> +
> +	image = (struct dsp_image_seg *)(fw_speq->data);
> +
> +	status = dspload_get_speakereq_addx(codec, &x, &y);
> +	if (status < 0)
> +		goto done;
> +
> +	status = dspload_image(codec, image, 1, y, 0, 8);
> +
> +done:
> +	release_firmware(fw_speq);
> +
> +	snd_printdd(KERN_INFO "dspload_speakereq() -- complete");
> +
> +	return status;
> +}
> +
>  static bool dspload_is_loaded(struct hda_codec *codec)
>  {
>  	unsigned int data = 0;
> @@ -4335,6 +4402,8 @@ static bool ca0132_download_dsp_images(struct hda_codec *codec)
>  
>  	release_firmware(fw_entry);
>  
> +	if (dsp_loaded)
> +		dspload_speakereq(codec);
>  
>  	return dsp_loaded;
>  }
> -- 
> 1.7.4.1
> 

Re: [PATCH 3/4] ALSA: CA0132: Add latency monitoring

[Takashi Iwai]

At Fri, 8 Feb 2013 18:31:44 -0800,
Ian Minett wrote:
> 
> From: Ian Minett <ian_minett@creativelabs.com>
> 
> Add latency monitoring for playback, capture and DSP effects.

Please give a bit more descriptions.
Looking at the patch, it doesn't implement only the latency monitoring
but actually reflecting to the runtime->delay.

Also..


> Signed-off-by: Ian Minett <ian_minett@creativelabs.com>
> 
> diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c
> index b52f00c..c797f65 100644
> --- a/sound/pci/hda/patch_ca0132.c
> +++ b/sound/pci/hda/patch_ca0132.c
> @@ -133,6 +133,12 @@ enum {
>  /* Effects values size*/
>  #define EFFECT_VALS_MAX_COUNT 12
>  
> +#define DSP_CAPTURE_INIT_LATENCY        0
> +#define DSP_CRYSTAL_VOICE_LATENCY       124
> +#define DSP_PLAYBACK_INIT_LATENCY       13
> +#define DSP_PLAY_ENHANCEMENT_LATENCY    30
> +#define DSP_SPEAKER_OUT_LATENCY         7
> +
>  struct ct_effect {
>  	char name[44];
>  	hda_nid_t nid;
> @@ -2761,8 +2767,27 @@ static void ca0132_cleanup_stream(struct hda_codec *codec, hda_nid_t nid)
>  }
>  
>  /*
> - * PCM callbacks
> + * PCM playbacks
>   */
> +static unsigned int ca0132_get_playback_latency(struct hda_codec *codec)
> +{
> +	struct ca0132_spec *spec = codec->spec;
> +	unsigned int latency = DSP_PLAYBACK_INIT_LATENCY;
> +
> +	if (!dspload_is_loaded(codec))
> +		return 0;
> +
> +	if (spec->effects_switch[PLAY_ENHANCEMENT - EFFECT_START_NID]) {
> +		if ((spec->effects_switch[SURROUND - EFFECT_START_NID]) ||
> +		    (spec->effects_switch[DIALOG_PLUS - EFFECT_START_NID]))
> +			latency += DSP_PLAY_ENHANCEMENT_LATENCY;
> +	}
> +
> +	if (spec->cur_out_type == SPEAKER_OUT)
> +		latency += DSP_SPEAKER_OUT_LATENCY;
> +	return latency;
> +}
> +
>  static int ca0132_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
>  			struct hda_codec *codec,
>  			unsigned int stream_tag,
> @@ -2770,6 +2795,16 @@ static int ca0132_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
>  			struct snd_pcm_substream *substream)
>  {
>  	struct ca0132_spec *spec = codec->spec;
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	unsigned int latency = ca0132_get_playback_latency(codec);
> +
> +	if (spec->dsp_state == DSP_DOWNLOADING) {
> +		spec->dsp_stream_id = stream_tag;
> +		return 0;
> +	}
> +
> +	runtime->delay = bytes_to_frames(runtime, (latency * runtime->rate *
> +					runtime->byte_align) / 1000);
>  
>  	ca0132_setup_stream(codec, spec->dacs[0], stream_tag, 0, format);
>  
> @@ -2834,8 +2869,22 @@ static int ca0132_dig_playback_pcm_close(struct hda_pcm_stream *hinfo,
>  }
>  
>  /*
> - * Analog capture
> + * PCM capture
>   */
> +
> +static unsigned int ca0132_get_capture_latency(struct hda_codec *codec)
> +{
> +	struct ca0132_spec *spec = codec->spec;
> +	unsigned int latency = DSP_CAPTURE_INIT_LATENCY;
> +
> +	if (!dspload_is_loaded(codec))
> +		return 0;
> +
> +	if (spec->effects_switch[CRYSTAL_VOICE - EFFECT_START_NID])
> +		latency += DSP_CRYSTAL_VOICE_LATENCY;
> +	return latency;
> +}
> +
>  static int ca0132_capture_pcm_prepare(struct hda_pcm_stream *hinfo,
>  					struct hda_codec *codec,
>  					unsigned int stream_tag,
> @@ -2843,9 +2892,16 @@ static int ca0132_capture_pcm_prepare(struct hda_pcm_stream *hinfo,
>  					struct snd_pcm_substream *substream)
>  {
>  	struct ca0132_spec *spec = codec->spec;
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	unsigned int latency = ca0132_get_capture_latency(codec);
>  
> -	ca0132_setup_stream(codec, spec->adcs[substream->number],
> -			    stream_tag, 0, format);
> +	if (spec->dsp_state == DSP_DOWNLOADING)
> +		return 0;
> +
> +	runtime->delay = bytes_to_frames(runtime, (latency * runtime->rate *
> +					runtime->byte_align) / 1000);
> +
> +	ca0132_setup_stream(codec, hinfo->nid, stream_tag, 0, format);
>  
>  	return 0;
>  }
> @@ -3429,6 +3485,33 @@ static int ca0132_voicefx_set(struct hda_codec *codec, int enable)
>  	return 1;
>  }
>  
> +static void ca0132_update_latency(struct hda_codec *codec,
> +				  unsigned int direction)
> +{
> +	struct ca0132_spec *spec = codec->spec;
> +	unsigned int latency;
> +	struct snd_pcm_substream *substr;
> +	struct snd_pcm_runtime *runtime;
> +
> +	if (spec->dsp_state == DSP_DOWNLOADING)
> +		return;
> +
> +	latency = (direction == SNDRV_PCM_STREAM_PLAYBACK)
> +			? ca0132_get_playback_latency(codec)
> +			: ca0132_get_capture_latency(codec);
> +
> +	substr = codec->pcm_info->pcm->streams[direction].substream;
> +	while (substr) {
> +		runtime = substr->runtime;
> +		/* update latency only when frame_bits is set */
> +		if (runtime && runtime->frame_bits)
> +			runtime->delay = bytes_to_frames(runtime,
> +					(latency * runtime->rate *
> +					runtime->byte_align) / 1000);
> +		substr = substr->next;
> +	}

I guess this isn't needed.  Usually the pointer callback is performed
always before the inquiry of runtime->delay.


Takashi


> +}
> +
>  /*
>   * Set the effects parameters
>   */
> @@ -3468,6 +3551,9 @@ static int ca0132_effects_set(struct hda_codec *codec, hda_nid_t nid, long val)
>  	err = dspio_set_uint_param(codec, ca0132_effects[idx].mid,
>  				   ca0132_effects[idx].reqs[0], on);
>  
> +	ca0132_update_latency(codec, SNDRV_PCM_STREAM_PLAYBACK);
> +	ca0132_update_latency(codec, SNDRV_PCM_STREAM_CAPTURE);
> +
>  	if (err < 0)
>  		return 0; /* no changed */
>  
> -- 
> 1.7.4.1
> 

Re: [PATCH 2/4] ALSA: CA0132: Improve the DSP transfer timeout calculations

[Takashi Iwai]

At Fri, 8 Feb 2013 18:31:43 -0800,
Ian Minett wrote:
> 
> From: Ian Minett <ian_minett@creativelabs.com>
> 
> Base the DSP firmware transfer and communication timeouts on jiffy values.
> 
> Signed-off-by: Ian Minett <ian_minett@creativelabs.com>

OK, applied this one.


thanks,

Takashi

> 
> diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c
> index a4b61a9..b52f00c 100644
> --- a/sound/pci/hda/patch_ca0132.c
> +++ b/sound/pci/hda/patch_ca0132.c
> @@ -785,7 +785,7 @@ static int chipio_send(struct hda_codec *codec,
>  		       unsigned int data)
>  {
>  	unsigned int res;
> -	int retry = 50;
> +	unsigned long timeout = jiffies + msecs_to_jiffies(1000);
>  
>  	/* send bits of data specified by reg */
>  	do {
> @@ -793,7 +793,9 @@ static int chipio_send(struct hda_codec *codec,
>  					 reg, data);
>  		if (res == VENDOR_STATUS_CHIPIO_OK)
>  			return 0;
> -	} while (--retry);
> +		msleep(20);
> +	} while (time_before(jiffies, timeout));
> +
>  	return -EIO;
>  }
>  
> @@ -1059,14 +1061,15 @@ static int dspio_send(struct hda_codec *codec, unsigned int reg,
>  		      unsigned int data)
>  {
>  	int res;
> -	int retry = 50;
> +	unsigned long timeout = jiffies + msecs_to_jiffies(1000);
>  
>  	/* send bits of data specified by reg to dsp */
>  	do {
>  		res = snd_hda_codec_read(codec, WIDGET_DSP_CTRL, 0, reg, data);
>  		if ((res >= 0) && (res != VENDOR_STATUS_DSPIO_BUSY))
>  			return res;
> -	} while (--retry);
> +		msleep(20);
> +	} while (time_before(jiffies, timeout));
>  
>  	return -EIO;
>  }
> @@ -1298,7 +1301,6 @@ static int dspio_send_scp_message(struct hda_codec *codec,
>  				  unsigned int *bytes_returned)
>  {
>  	struct ca0132_spec *spec = codec->spec;
> -	int retry;
>  	int status = -1;
>  	unsigned int scp_send_size = 0;
>  	unsigned int total_size;
> @@ -1345,13 +1347,13 @@ static int dspio_send_scp_message(struct hda_codec *codec,
>  	}
>  
>  	if (waiting_for_resp) {
> +		unsigned long timeout = jiffies + msecs_to_jiffies(1000);
>  		memset(return_buf, 0, return_buf_size);
> -		retry = 50;
>  		do {
>  			msleep(20);
> -		} while (spec->wait_scp && (--retry != 0));
> +		} while (spec->wait_scp && time_before(jiffies, timeout));
>  		waiting_for_resp = false;
> -		if (retry != 0) {
> +		if (!spec->wait_scp) {
>  			ret_msg = (struct scp_msg *)return_buf;
>  			memcpy(&ret_msg->hdr, &spec->scp_resp_header, 4);
>  			memcpy(&ret_msg->data, spec->scp_resp_data,
> @@ -2244,7 +2246,8 @@ static int dspxfr_one_seg(struct hda_codec *codec,
>  	u32 chip_addx_remainder;
>  	unsigned int run_size_words;
>  	const struct dsp_image_seg *hci_write = NULL;
> -	int retry;
> +	unsigned long timeout;
> +	bool dma_active;
>  
>  	if (fls == NULL)
>  		return -EINVAL;
> @@ -2362,11 +2365,17 @@ static int dspxfr_one_seg(struct hda_codec *codec,
>  			status = dspxfr_hci_write(codec, hci_write);
>  			hci_write = NULL;
>  		}
> -		retry = 5000;
> -		while (dsp_is_dma_active(codec, dma_chan)) {
> -			if (--retry <= 0)
> +
> +		timeout = jiffies + msecs_to_jiffies(2000);
> +		do {
> +			dma_active = dsp_is_dma_active(codec, dma_chan);
> +			if (!dma_active)
>  				break;
> -		}
> +			msleep(20);
> +		} while (time_before(jiffies, timeout));
> +		if (dma_active)
> +			break;
> +
>  		snd_printdd(KERN_INFO "+++++ DMA complete");
>  		dma_set_state(dma_engine, DMA_STATE_STOP);
>  		dma_reset(dma_engine);
> @@ -2683,15 +2692,15 @@ static bool dspload_is_loaded(struct hda_codec *codec)
>  
>  static bool dspload_wait_loaded(struct hda_codec *codec)
>  {
> -	int retry = 100;
> +	unsigned long timeout = jiffies + msecs_to_jiffies(2000);
>  
>  	do {
> -		msleep(20);
>  		if (dspload_is_loaded(codec)) {
>  			pr_info("ca0132 DOWNLOAD OK :-) DSP IS RUNNING.\n");
>  			return true;
>  		}
> -	} while (--retry);
> +		msleep(20);
> +	} while (time_before(jiffies, timeout));
>  
>  	pr_err("ca0132 DOWNLOAD FAILED!!! DSP IS NOT RUNNING.\n");
>  	return false;
> -- 
> 1.7.4.1
> 

Re: [PATCH 4/4] ALSA: CA0132: Update hp unsol event handler

[Takashi Iwai]

At Fri, 8 Feb 2013 18:31:45 -0800,
Ian Minett wrote:
> 
> From: Ian Minett <ian_minett@creativelabs.com>
> 
> Manage the delayed work queue in hp unsol event handler.

Could you explain why this is needed at all?
We can't take a patch changing something without reasons.


thanks,

Takashi

> 
> Signed-off-by: Ian Minett <ian_minett@creativelabs.com>
> 
> diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c
> index c797f65..61d74db 100644
> --- a/sound/pci/hda/patch_ca0132.c
> +++ b/sound/pci/hda/patch_ca0132.c
> @@ -749,6 +749,9 @@ struct ca0132_spec {
>  	long voicefx_val;
>  	long cur_mic_boost;
>  
> +	struct hda_codec *codec;
> +	struct delayed_work unsol_hp_work;
> +
>  #ifdef ENABLE_TUNING_CONTROLS
>  	long cur_ctl_vals[TUNING_CTLS_COUNT];
>  #endif
> @@ -3338,6 +3341,14 @@ exit:
>  	return err < 0 ? err : 0;
>  }
>  
> +static void ca0132_unsol_hp_delayed(struct work_struct *work)
> +{
> +	struct ca0132_spec *spec = container_of(
> +		to_delayed_work(work), struct ca0132_spec, unsol_hp_work);
> +	ca0132_select_out(spec->codec);
> +	snd_hda_jack_report_sync(spec->codec);
> +}
> +
>  static void ca0132_set_dmic(struct hda_codec *codec, int enable);
>  static int ca0132_mic_boost_set(struct hda_codec *codec, long val);
>  static int ca0132_effects_set(struct hda_codec *codec, hda_nid_t nid, long val);
> @@ -4540,8 +4551,9 @@ static void ca0132_process_dsp_response(struct hda_codec *codec)
>  
>  static void ca0132_unsol_event(struct hda_codec *codec, unsigned int res)
>  {
> -	snd_printdd(KERN_INFO "ca0132_unsol_event: 0x%x\n", res);
> +	struct ca0132_spec *spec = codec->spec;
>  
> +	snd_printdd(KERN_INFO "ca0132_unsol_event: 0x%x\n", res);
>  
>  	if (((res >> AC_UNSOL_RES_TAG_SHIFT) & 0x3f) == UNSOL_TAG_DSP) {
>  		ca0132_process_dsp_response(codec);
> @@ -4553,8 +4565,10 @@ static void ca0132_unsol_event(struct hda_codec *codec, unsigned int res)
>  
>  		switch (res) {
>  		case UNSOL_TAG_HP:
> -			ca0132_select_out(codec);
> -			snd_hda_jack_report_sync(codec);
> +			cancel_delayed_work_sync(&spec->unsol_hp_work);
> +			queue_delayed_work(codec->bus->workq,
> +					   &spec->unsol_hp_work,
> +					   msecs_to_jiffies(500));
>  			break;
>  		case UNSOL_TAG_AMIC1:
>  			ca0132_select_mic(codec);
> @@ -4729,6 +4743,7 @@ static void ca0132_free(struct hda_codec *codec)
>  {
>  	struct ca0132_spec *spec = codec->spec;
>  
> +	cancel_delayed_work_sync(&spec->unsol_hp_work);
>  	snd_hda_power_up(codec);
>  	snd_hda_sequence_write(codec, spec->base_exit_verbs);
>  	ca0132_exit_chip(codec);
> @@ -4794,6 +4809,7 @@ static int patch_ca0132(struct hda_codec *codec)
>  	if (!spec)
>  		return -ENOMEM;
>  	codec->spec = spec;
> +	spec->codec = codec;
>  
>  	spec->num_mixers = 1;
>  	spec->mixers[0] = ca0132_mixer;
> @@ -4804,6 +4820,8 @@ static int patch_ca0132(struct hda_codec *codec)
>  	spec->init_verbs[1] = ca0132_init_verbs1;
>  	spec->num_init_verbs = 2;
>  
> +	INIT_DELAYED_WORK(&spec->unsol_hp_work, ca0132_unsol_hp_delayed);
> +
>  	ca0132_init_chip(codec);
>  
>  	ca0132_config(codec);
> -- 
> 1.7.4.1
> 

Re: [PATCH 0/4] ALSA: Updates for the CA0132 codec

[Takashi Iwai]

Hi Ian,

At Fri, 8 Feb 2013 18:31:41 -0800,
Ian Minett wrote:
> 
> From: Ian Minett <ian_minett@creativelabs.com>
> 
> Hi,
> 
> This series contains the following updates to CA0132:
> 
> 1: Add firmware loading for the SpeakerEQ DSP effect.
> 2: Improve DSP transfer timeout calculations.
> 3: Add stream and effect latency monitoring routines 
> 4: Update hp unsolicited event handler to manage queued work.

Thanks, I took the one now but leave others.  See my reply to each.

> 
> These were based on the latest sound.git, but please let me know if 
> they should be against sound-unstable tree instead.

sound.git tree for-next branch would be the place to look at.
If I start a new branch for ca0132, I'll let you know.


But, before going further: as I already mailed you, I could get a test
board and started testing.  The result wasn't good.  At the first time
the driver loads, the DSP loading fails always.  When the driver is
reloaded after that, it's working.  So, something is still pretty
fragile.

I checked the code, and fixed a few overlooked things.  Now I wonder
why dma_reset() must be called at each DSP chunk load.  It reallocates
the whole buffer, and I see no reason.  It seems working even if I
comment it out.  Could you clarify?

Another thing I noticed is that I got a short burst noise at the very
first playback.  After that, the driver starts working well.
Is it some uninitialized buffer or so?


thanks,

Takashi

Re: [PATCH 0/4] ALSA: Updates for the CA0132 codec

[Dylan Reid]

On Sun, Feb 10, 2013 at 2:57 AM, Takashi Iwai <tiwai@suse.de> wrote:
> Hi Ian,
>
> At Fri, 8 Feb 2013 18:31:41 -0800,
> Ian Minett wrote:
>>
>> From: Ian Minett <ian_minett@creativelabs.com>
>>
>> Hi,
>>
>> This series contains the following updates to CA0132:
>>
>> 1: Add firmware loading for the SpeakerEQ DSP effect.
>> 2: Improve DSP transfer timeout calculations.
>> 3: Add stream and effect latency monitoring routines
>> 4: Update hp unsolicited event handler to manage queued work.
>
> Thanks, I took the one now but leave others.  See my reply to each.
>
>>
>> These were based on the latest sound.git, but please let me know if
>> they should be against sound-unstable tree instead.
>
> sound.git tree for-next branch would be the place to look at.
> If I start a new branch for ca0132, I'll let you know.
>
>
> But, before going further: as I already mailed you, I could get a test
> board and started testing.  The result wasn't good.  At the first time
> the driver loads, the DSP loading fails always.  When the driver is
> reloaded after that, it's working.  So, something is still pretty
> fragile.

I tried this today and I can't get it to work.  It is failing in
snd_hda_lock_devices called from azx_load_dsp_prepare.  The DSP is
loaded from ca0132_init which is called as the result of open/resume.
Both the check for open control files and attached substreams are
failing. How can that be avoided?

Thanks,

Dylan

>
> I checked the code, and fixed a few overlooked things.  Now I wonder
> why dma_reset() must be called at each DSP chunk load.  It reallocates
> the whole buffer, and I see no reason.  It seems working even if I
> comment it out.  Could you clarify?
>
> Another thing I noticed is that I got a short burst noise at the very
> first playback.  After that, the driver starts working well.
> Is it some uninitialized buffer or so?
>
>
> thanks,
>
> Takashi
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 0/4] ALSA: Updates for the CA0132 codec

[Takashi Iwai]

At Thu, 14 Mar 2013 17:34:34 -0700,
Dylan Reid wrote:
> 
> On Sun, Feb 10, 2013 at 2:57 AM, Takashi Iwai <tiwai@suse.de> wrote:
> > Hi Ian,
> >
> > At Fri, 8 Feb 2013 18:31:41 -0800,
> > Ian Minett wrote:
> >>
> >> From: Ian Minett <ian_minett@creativelabs.com>
> >>
> >> Hi,
> >>
> >> This series contains the following updates to CA0132:
> >>
> >> 1: Add firmware loading for the SpeakerEQ DSP effect.
> >> 2: Improve DSP transfer timeout calculations.
> >> 3: Add stream and effect latency monitoring routines
> >> 4: Update hp unsolicited event handler to manage queued work.
> >
> > Thanks, I took the one now but leave others.  See my reply to each.
> >
> >>
> >> These were based on the latest sound.git, but please let me know if
> >> they should be against sound-unstable tree instead.
> >
> > sound.git tree for-next branch would be the place to look at.
> > If I start a new branch for ca0132, I'll let you know.
> >
> >
> > But, before going further: as I already mailed you, I could get a test
> > board and started testing.  The result wasn't good.  At the first time
> > the driver loads, the DSP loading fails always.  When the driver is
> > reloaded after that, it's working.  So, something is still pretty
> > fragile.
> 
> I tried this today and I can't get it to work.  It is failing in
> snd_hda_lock_devices called from azx_load_dsp_prepare.  The DSP is
> loaded from ca0132_init which is called as the result of open/resume.
> Both the check for open control files and attached substreams are
> failing. How can that be avoided?

snd_hda_lock_devices() is obviously not suitable for the power-saving
or PM resume.  OK, this must be fixed indeed...

How about the patch below?  (Note that it's just compile tested.)


thanks,

Takashi

---
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ALSA: hda - Fix abuse of snd_hda_lock_devices() for DSP
 loader

The current DSP loader code abuses snd_hda_lock_devices() for ensuring
the DSP loader not conflicting with the other normal operations.  But
this trick obviously doesn't work for the PM resume since the streams
are kept opened there where snd_hda_lock_devices() returns -EBUSY.
That means we need another lock mechanism instead of abuse.

This patch provides the new lock state to azx_dev.  Theoretically it's
possible that the DSP loader conflicts with the stream that has been
already assigned for another PCM.  If it's running, the DSP loader
should simply fail.  If not -- it's the case for PM resume --, we
should assign this stream temporarily to the DSP loader, and take it
back to the PCM after finishing DSP loading.  If the PCM is operated
during the DSP loading, it should get an error, too.

Reported-by: Dylan Reid <dgreid@chromium.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/hda_intel.c | 119 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 100 insertions(+), 19 deletions(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 4cea6bb6..61abf8b 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -415,6 +415,8 @@ struct azx_dev {
 	unsigned int opened :1;
 	unsigned int running :1;
 	unsigned int irq_pending :1;
+	unsigned int prepared:1;
+	unsigned int locked:1;
 	/*
 	 * For VIA:
 	 *  A flag to ensure DMA position is 0
@@ -426,8 +428,25 @@ struct azx_dev {
 
 	struct timecounter  azx_tc;
 	struct cyclecounter azx_cc;
+
+#ifdef CONFIG_SND_HDA_DSP_LOADER
+	struct mutex dsp_mutex;
+#endif
 };
 
+/* DSP lock helpers */
+#ifdef CONFIG_SND_HDA_DSP_LOADER
+#define dsp_lock_init(dev)	mutex_init(&(dev)->dsp_mutex)
+#define dsp_lock(dev)		mutex_lock(&(dev)->dsp_mutex)
+#define dsp_unlock(dev)		mutex_unlock(&(dev)->dsp_mutex)
+#define dsp_is_locked(dev)	((dev)->locked)
+#else
+#define dsp_lock_init(dev)	do {} while (0)
+#define dsp_lock(dev)		do {} while (0)
+#define dsp_unlock(dev)		do {} while (0)
+#define dsp_is_locked(dev)	0
+#endif
+
 /* CORB/RIRB */
 struct azx_rb {
 	u32 *buf;		/* CORB/RIRB buffer
@@ -527,6 +546,10 @@ struct azx {
 
 	/* card list (for power_save trigger) */
 	struct list_head list;
+
+#ifdef CONFIG_SND_HDA_DSP_LOADER
+	struct azx_dev saved_azx_dev;
+#endif
 };
 
 #define CREATE_TRACE_POINTS
@@ -1794,7 +1817,8 @@ azx_assign_device(struct azx *chip, struct snd_pcm_substream *substream)
 		nums = chip->capture_streams;
 	}
 	for (i = 0; i < nums; i++, dev++)
-		if (!chip->azx_dev[dev].opened) {
+		if (!chip->azx_dev[dev].opened &&
+		    !dsp_is_locked(&chip->azx_dev[dev])) {
 			res = &chip->azx_dev[dev];
 			if (res->assigned_key == key)
 				break;
@@ -2009,6 +2033,12 @@ static int azx_pcm_hw_params(struct snd_pcm_substream *substream,
 	struct azx_dev *azx_dev = get_azx_dev(substream);
 	int ret;
 
+	dsp_lock(azx_dev);
+	if (dsp_is_locked(azx_dev)) {
+		ret = -EBUSY;
+		goto unlock;
+	}
+
 	mark_runtime_wc(chip, azx_dev, substream, false);
 	azx_dev->bufsize = 0;
 	azx_dev->period_bytes = 0;
@@ -2016,8 +2046,10 @@ static int azx_pcm_hw_params(struct snd_pcm_substream *substream,
 	ret = snd_pcm_lib_malloc_pages(substream,
 					params_buffer_bytes(hw_params));
 	if (ret < 0)
-		return ret;
+		goto unlock;
 	mark_runtime_wc(chip, azx_dev, substream, true);
+ unlock:
+	dsp_unlock(azx_dev);
 	return ret;
 }
 
@@ -2029,16 +2061,21 @@ static int azx_pcm_hw_free(struct snd_pcm_substream *substream)
 	struct hda_pcm_stream *hinfo = apcm->hinfo[substream->stream];
 
 	/* reset BDL address */
-	azx_sd_writel(azx_dev, SD_BDLPL, 0);
-	azx_sd_writel(azx_dev, SD_BDLPU, 0);
-	azx_sd_writel(azx_dev, SD_CTL, 0);
-	azx_dev->bufsize = 0;
-	azx_dev->period_bytes = 0;
-	azx_dev->format_val = 0;
+	dsp_lock(azx_dev);
+	if (!dsp_is_locked(azx_dev)) {
+		azx_sd_writel(azx_dev, SD_BDLPL, 0);
+		azx_sd_writel(azx_dev, SD_BDLPU, 0);
+		azx_sd_writel(azx_dev, SD_CTL, 0);
+		azx_dev->bufsize = 0;
+		azx_dev->period_bytes = 0;
+		azx_dev->format_val = 0;
+	}
 
 	snd_hda_codec_cleanup(apcm->codec, hinfo, substream);
 
 	mark_runtime_wc(chip, azx_dev, substream, false);
+	azx_dev->prepared = 0;
+	dsp_unlock(azx_dev);
 	return snd_pcm_lib_free_pages(substream);
 }
 
@@ -2055,6 +2092,12 @@ static int azx_pcm_prepare(struct snd_pcm_substream *substream)
 		snd_hda_spdif_out_of_nid(apcm->codec, hinfo->nid);
 	unsigned short ctls = spdif ? spdif->ctls : 0;
 
+	dsp_lock(azx_dev);
+	if (dsp_is_locked(azx_dev)) {
+		err = -EBUSY;
+		goto unlock;
+	}
+
 	azx_stream_reset(chip, azx_dev);
 	format_val = snd_hda_calc_stream_format(runtime->rate,
 						runtime->channels,
@@ -2065,7 +2108,8 @@ static int azx_pcm_prepare(struct snd_pcm_substream *substream)
 		snd_printk(KERN_ERR SFX
 			   "%s: invalid format_val, rate=%d, ch=%d, format=%d\n",
 			   pci_name(chip->pci), runtime->rate, runtime->channels, runtime->format);
-		return -EINVAL;
+		err = -EINVAL;
+		goto unlock;
 	}
 
 	bufsize = snd_pcm_lib_buffer_bytes(substream);
@@ -2084,7 +2128,7 @@ static int azx_pcm_prepare(struct snd_pcm_substream *substream)
 		azx_dev->no_period_wakeup = runtime->no_period_wakeup;
 		err = azx_setup_periods(chip, substream, azx_dev);
 		if (err < 0)
-			return err;
+			goto unlock;
 	}
 
 	/* wallclk has 24Mhz clock source */
@@ -2101,8 +2145,14 @@ static int azx_pcm_prepare(struct snd_pcm_substream *substream)
 	if ((chip->driver_caps & AZX_DCAPS_CTX_WORKAROUND) &&
 	    stream_tag > chip->capture_streams)
 		stream_tag -= chip->capture_streams;
-	return snd_hda_codec_prepare(apcm->codec, hinfo, stream_tag,
+	err = snd_hda_codec_prepare(apcm->codec, hinfo, stream_tag,
 				     azx_dev->format_val, substream);
+
+ unlock:
+	if (!err)
+		azx_dev->prepared = 1;
+	dsp_unlock(azx_dev);
+	return err;
 }
 
 static int azx_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
@@ -2117,6 +2167,9 @@ static int azx_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 	azx_dev = get_azx_dev(substream);
 	trace_azx_pcm_trigger(chip, azx_dev, cmd);
 
+	if (!dsp_is_locked(azx_dev) || !azx_dev->prepared)
+		return -EPIPE;
+
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
 		rstart = 1;
@@ -2621,17 +2674,28 @@ static int azx_load_dsp_prepare(struct hda_bus *bus, unsigned int format,
 	struct azx_dev *azx_dev;
 	int err;
 
-	if (snd_hda_lock_devices(bus))
-		return -EBUSY;
+	azx_dev = azx_get_dsp_loader_dev(chip);
+
+	mutex_lock(&chip->open_mutex);
+	dsp_lock(azx_dev);
+	spin_lock_irq(&chip->reg_lock);
+	if (azx_dev->running || azx_dev->locked) {
+		spin_unlock_irq(&chip->reg_lock);
+		err = -EBUSY;
+		goto unlock;
+	}
+	azx_dev->prepared = 0;
+	chip->saved_azx_dev = *azx_dev;
+	azx_dev->locked = 1;
+	spin_unlock_irq(&chip->reg_lock);
 
 	err = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV_SG,
 				  snd_dma_pci_data(chip->pci),
 				  byte_size, bufp);
 	if (err < 0)
-		goto unlock;
+		goto err_alloc;
 
 	mark_pages_wc(chip, bufp, true);
-	azx_dev = azx_get_dsp_loader_dev(chip);
 	azx_dev->bufsize = byte_size;
 	azx_dev->period_bytes = byte_size;
 	azx_dev->format_val = format;
@@ -2649,13 +2713,21 @@ static int azx_load_dsp_prepare(struct hda_bus *bus, unsigned int format,
 		goto error;
 
 	azx_setup_controller(chip, azx_dev);
+	mutex_unlock(&chip->open_mutex);
 	return azx_dev->stream_tag;
 
  error:
 	mark_pages_wc(chip, bufp, false);
 	snd_dma_free_pages(bufp);
-unlock:
-	snd_hda_unlock_devices(bus);
+ err_alloc:
+	spin_lock_irq(&chip->reg_lock);
+	if (azx_dev->opened)
+		*azx_dev = chip->saved_azx_dev;
+	azx_dev->locked = 0;
+	spin_unlock_irq(&chip->reg_lock);
+ unlock:
+	dsp_unlock(azx_dev);
+	mutex_unlock(&chip->open_mutex);
 	return err;
 }
 
@@ -2677,9 +2749,11 @@ static void azx_load_dsp_cleanup(struct hda_bus *bus,
 	struct azx *chip = bus->private_data;
 	struct azx_dev *azx_dev = azx_get_dsp_loader_dev(chip);
 
-	if (!dmab->area)
+	if (!dmab->area || !azx_dev->locked)
 		return;
 
+	mutex_lock(&chip->open_mutex);
+	dsp_lock(azx_dev);
 	/* reset BDL address */
 	azx_sd_writel(azx_dev, SD_BDLPL, 0);
 	azx_sd_writel(azx_dev, SD_BDLPU, 0);
@@ -2692,7 +2766,13 @@ static void azx_load_dsp_cleanup(struct hda_bus *bus,
 	snd_dma_free_pages(dmab);
 	dmab->area = NULL;
 
-	snd_hda_unlock_devices(bus);
+	spin_lock_irq(&chip->reg_lock);
+	if (azx_dev->opened)
+		*azx_dev = chip->saved_azx_dev;
+	azx_dev->locked = 0;
+	spin_unlock_irq(&chip->reg_lock);
+	dsp_unlock(azx_dev);
+	mutex_unlock(&chip->open_mutex);
 }
 #endif /* CONFIG_SND_HDA_DSP_LOADER */
 
@@ -3481,6 +3561,7 @@ static int azx_first_init(struct azx *chip)
 	}
 
 	for (i = 0; i < chip->num_streams; i++) {
+		dsp_lock_init(&chip->azx_dev[i]);
 		/* allocate memory for the BDL for each stream */
 		err = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV,
 					  snd_dma_pci_data(chip->pci),
-- 
1.8.1.4

Re: [PATCH 0/4] ALSA: Updates for the CA0132 codec

[Dylan Reid]

On Fri, Mar 15, 2013 at 1:25 AM, Takashi Iwai <tiwai@suse.de> wrote:
> At Thu, 14 Mar 2013 17:34:34 -0700,
> Dylan Reid wrote:
>>
>> On Sun, Feb 10, 2013 at 2:57 AM, Takashi Iwai <tiwai@suse.de> wrote:
>> > Hi Ian,
>> >
>> > At Fri, 8 Feb 2013 18:31:41 -0800,
>> > Ian Minett wrote:
>> >>
>> >> From: Ian Minett <ian_minett@creativelabs.com>
>> >>
>> >> Hi,
>> >>
>> >> This series contains the following updates to CA0132:
>> >>
>> >> 1: Add firmware loading for the SpeakerEQ DSP effect.
>> >> 2: Improve DSP transfer timeout calculations.
>> >> 3: Add stream and effect latency monitoring routines
>> >> 4: Update hp unsolicited event handler to manage queued work.
>> >
>> > Thanks, I took the one now but leave others.  See my reply to each.
>> >
>> >>
>> >> These were based on the latest sound.git, but please let me know if
>> >> they should be against sound-unstable tree instead.
>> >
>> > sound.git tree for-next branch would be the place to look at.
>> > If I start a new branch for ca0132, I'll let you know.
>> >
>> >
>> > But, before going further: as I already mailed you, I could get a test
>> > board and started testing.  The result wasn't good.  At the first time
>> > the driver loads, the DSP loading fails always.  When the driver is
>> > reloaded after that, it's working.  So, something is still pretty
>> > fragile.
>>
>> I tried this today and I can't get it to work.  It is failing in
>> snd_hda_lock_devices called from azx_load_dsp_prepare.  The DSP is
>> loaded from ca0132_init which is called as the result of open/resume.
>> Both the check for open control files and attached substreams are
>> failing. How can that be avoided?
>
> snd_hda_lock_devices() is obviously not suitable for the power-saving
> or PM resume.  OK, this must be fixed indeed...
>
> How about the patch below?  (Note that it's just compile tested.)

Thanks Takashi.  This works for the init case, but it deadlocks in the
case the load originates from open:
azx_pcm_open -> calls snd_hda_power_up_d3wait -> ... -> ca0132_init ->
azx_load_dsp_prepare

both open and load_dsp_prepare grab open_mutex.

>
>
> thanks,
>
> Takashi
>
> ---
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] ALSA: hda - Fix abuse of snd_hda_lock_devices() for DSP
>  loader
>
> The current DSP loader code abuses snd_hda_lock_devices() for ensuring
> the DSP loader not conflicting with the other normal operations.  But
> this trick obviously doesn't work for the PM resume since the streams
> are kept opened there where snd_hda_lock_devices() returns -EBUSY.
> That means we need another lock mechanism instead of abuse.
>
> This patch provides the new lock state to azx_dev.  Theoretically it's
> possible that the DSP loader conflicts with the stream that has been
> already assigned for another PCM.  If it's running, the DSP loader
> should simply fail.  If not -- it's the case for PM resume --, we
> should assign this stream temporarily to the DSP loader, and take it
> back to the PCM after finishing DSP loading.  If the PCM is operated
> during the DSP loading, it should get an error, too.
>
> Reported-by: Dylan Reid <dgreid@chromium.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  sound/pci/hda/hda_intel.c | 119 ++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 100 insertions(+), 19 deletions(-)
>
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index 4cea6bb6..61abf8b 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -415,6 +415,8 @@ struct azx_dev {
>         unsigned int opened :1;
>         unsigned int running :1;
>         unsigned int irq_pending :1;
> +       unsigned int prepared:1;
> +       unsigned int locked:1;
>         /*
>          * For VIA:
>          *  A flag to ensure DMA position is 0
> @@ -426,8 +428,25 @@ struct azx_dev {
>
>         struct timecounter  azx_tc;
>         struct cyclecounter azx_cc;
> +
> +#ifdef CONFIG_SND_HDA_DSP_LOADER
> +       struct mutex dsp_mutex;
> +#endif
>  };
>
> +/* DSP lock helpers */
> +#ifdef CONFIG_SND_HDA_DSP_LOADER
> +#define dsp_lock_init(dev)     mutex_init(&(dev)->dsp_mutex)
> +#define dsp_lock(dev)          mutex_lock(&(dev)->dsp_mutex)
> +#define dsp_unlock(dev)                mutex_unlock(&(dev)->dsp_mutex)
> +#define dsp_is_locked(dev)     ((dev)->locked)
> +#else
> +#define dsp_lock_init(dev)     do {} while (0)
> +#define dsp_lock(dev)          do {} while (0)
> +#define dsp_unlock(dev)                do {} while (0)
> +#define dsp_is_locked(dev)     0
> +#endif
> +
>  /* CORB/RIRB */
>  struct azx_rb {
>         u32 *buf;               /* CORB/RIRB buffer
> @@ -527,6 +546,10 @@ struct azx {
>
>         /* card list (for power_save trigger) */
>         struct list_head list;
> +
> +#ifdef CONFIG_SND_HDA_DSP_LOADER
> +       struct azx_dev saved_azx_dev;
> +#endif
>  };
>
>  #define CREATE_TRACE_POINTS
> @@ -1794,7 +1817,8 @@ azx_assign_device(struct azx *chip, struct snd_pcm_substream *substream)
>                 nums = chip->capture_streams;
>         }
>         for (i = 0; i < nums; i++, dev++)
> -               if (!chip->azx_dev[dev].opened) {
> +               if (!chip->azx_dev[dev].opened &&
> +                   !dsp_is_locked(&chip->azx_dev[dev])) {
>                         res = &chip->azx_dev[dev];
>                         if (res->assigned_key == key)
>                                 break;
> @@ -2009,6 +2033,12 @@ static int azx_pcm_hw_params(struct snd_pcm_substream *substream,
>         struct azx_dev *azx_dev = get_azx_dev(substream);
>         int ret;
>
> +       dsp_lock(azx_dev);
> +       if (dsp_is_locked(azx_dev)) {
> +               ret = -EBUSY;
> +               goto unlock;
> +       }
> +
>         mark_runtime_wc(chip, azx_dev, substream, false);
>         azx_dev->bufsize = 0;
>         azx_dev->period_bytes = 0;
> @@ -2016,8 +2046,10 @@ static int azx_pcm_hw_params(struct snd_pcm_substream *substream,
>         ret = snd_pcm_lib_malloc_pages(substream,
>                                         params_buffer_bytes(hw_params));
>         if (ret < 0)
> -               return ret;
> +               goto unlock;
>         mark_runtime_wc(chip, azx_dev, substream, true);
> + unlock:
> +       dsp_unlock(azx_dev);
>         return ret;
>  }
>
> @@ -2029,16 +2061,21 @@ static int azx_pcm_hw_free(struct snd_pcm_substream *substream)
>         struct hda_pcm_stream *hinfo = apcm->hinfo[substream->stream];
>
>         /* reset BDL address */
> -       azx_sd_writel(azx_dev, SD_BDLPL, 0);
> -       azx_sd_writel(azx_dev, SD_BDLPU, 0);
> -       azx_sd_writel(azx_dev, SD_CTL, 0);
> -       azx_dev->bufsize = 0;
> -       azx_dev->period_bytes = 0;
> -       azx_dev->format_val = 0;
> +       dsp_lock(azx_dev);
> +       if (!dsp_is_locked(azx_dev)) {
> +               azx_sd_writel(azx_dev, SD_BDLPL, 0);
> +               azx_sd_writel(azx_dev, SD_BDLPU, 0);
> +               azx_sd_writel(azx_dev, SD_CTL, 0);
> +               azx_dev->bufsize = 0;
> +               azx_dev->period_bytes = 0;
> +               azx_dev->format_val = 0;
> +       }
>
>         snd_hda_codec_cleanup(apcm->codec, hinfo, substream);
>
>         mark_runtime_wc(chip, azx_dev, substream, false);
> +       azx_dev->prepared = 0;
> +       dsp_unlock(azx_dev);
>         return snd_pcm_lib_free_pages(substream);
>  }
>
> @@ -2055,6 +2092,12 @@ static int azx_pcm_prepare(struct snd_pcm_substream *substream)
>                 snd_hda_spdif_out_of_nid(apcm->codec, hinfo->nid);
>         unsigned short ctls = spdif ? spdif->ctls : 0;
>
> +       dsp_lock(azx_dev);
> +       if (dsp_is_locked(azx_dev)) {
> +               err = -EBUSY;
> +               goto unlock;
> +       }
> +
>         azx_stream_reset(chip, azx_dev);
>         format_val = snd_hda_calc_stream_format(runtime->rate,
>                                                 runtime->channels,
> @@ -2065,7 +2108,8 @@ static int azx_pcm_prepare(struct snd_pcm_substream *substream)
>                 snd_printk(KERN_ERR SFX
>                            "%s: invalid format_val, rate=%d, ch=%d, format=%d\n",
>                            pci_name(chip->pci), runtime->rate, runtime->channels, runtime->format);
> -               return -EINVAL;
> +               err = -EINVAL;
> +               goto unlock;
>         }
>
>         bufsize = snd_pcm_lib_buffer_bytes(substream);
> @@ -2084,7 +2128,7 @@ static int azx_pcm_prepare(struct snd_pcm_substream *substream)
>                 azx_dev->no_period_wakeup = runtime->no_period_wakeup;
>                 err = azx_setup_periods(chip, substream, azx_dev);
>                 if (err < 0)
> -                       return err;
> +                       goto unlock;
>         }
>
>         /* wallclk has 24Mhz clock source */
> @@ -2101,8 +2145,14 @@ static int azx_pcm_prepare(struct snd_pcm_substream *substream)
>         if ((chip->driver_caps & AZX_DCAPS_CTX_WORKAROUND) &&
>             stream_tag > chip->capture_streams)
>                 stream_tag -= chip->capture_streams;
> -       return snd_hda_codec_prepare(apcm->codec, hinfo, stream_tag,
> +       err = snd_hda_codec_prepare(apcm->codec, hinfo, stream_tag,
>                                      azx_dev->format_val, substream);
> +
> + unlock:
> +       if (!err)
> +               azx_dev->prepared = 1;
> +       dsp_unlock(azx_dev);
> +       return err;
>  }
>
>  static int azx_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
> @@ -2117,6 +2167,9 @@ static int azx_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
>         azx_dev = get_azx_dev(substream);
>         trace_azx_pcm_trigger(chip, azx_dev, cmd);
>
> +       if (!dsp_is_locked(azx_dev) || !azx_dev->prepared)
> +               return -EPIPE;
> +
>         switch (cmd) {
>         case SNDRV_PCM_TRIGGER_START:
>                 rstart = 1;
> @@ -2621,17 +2674,28 @@ static int azx_load_dsp_prepare(struct hda_bus *bus, unsigned int format,
>         struct azx_dev *azx_dev;
>         int err;
>
> -       if (snd_hda_lock_devices(bus))
> -               return -EBUSY;
> +       azx_dev = azx_get_dsp_loader_dev(chip);
> +
> +       mutex_lock(&chip->open_mutex);

This mutex could already be held if this call originates from
azx_pcm_open powering the codec back up.

> +       dsp_lock(azx_dev);
> +       spin_lock_irq(&chip->reg_lock);
> +       if (azx_dev->running || azx_dev->locked) {
> +               spin_unlock_irq(&chip->reg_lock);
> +               err = -EBUSY;
> +               goto unlock;
> +       }
> +       azx_dev->prepared = 0;
> +       chip->saved_azx_dev = *azx_dev;
> +       azx_dev->locked = 1;
> +       spin_unlock_irq(&chip->reg_lock);
>
>         err = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV_SG,
>                                   snd_dma_pci_data(chip->pci),
>                                   byte_size, bufp);
>         if (err < 0)
> -               goto unlock;
> +               goto err_alloc;
>
>         mark_pages_wc(chip, bufp, true);
> -       azx_dev = azx_get_dsp_loader_dev(chip);
>         azx_dev->bufsize = byte_size;
>         azx_dev->period_bytes = byte_size;
>         azx_dev->format_val = format;
> @@ -2649,13 +2713,21 @@ static int azx_load_dsp_prepare(struct hda_bus *bus, unsigned int format,
>                 goto error;
>
>         azx_setup_controller(chip, azx_dev);

Does this need a dsp_unlock(azx_dev)?

> +       mutex_unlock(&chip->open_mutex);
>         return azx_dev->stream_tag;
>
>   error:
>         mark_pages_wc(chip, bufp, false);
>         snd_dma_free_pages(bufp);
> -unlock:
> -       snd_hda_unlock_devices(bus);
> + err_alloc:
> +       spin_lock_irq(&chip->reg_lock);
> +       if (azx_dev->opened)
> +               *azx_dev = chip->saved_azx_dev;
> +       azx_dev->locked = 0;
> +       spin_unlock_irq(&chip->reg_lock);
> + unlock:
> +       dsp_unlock(azx_dev);
> +       mutex_unlock(&chip->open_mutex);
>         return err;
>  }
>
> @@ -2677,9 +2749,11 @@ static void azx_load_dsp_cleanup(struct hda_bus *bus,
>         struct azx *chip = bus->private_data;
>         struct azx_dev *azx_dev = azx_get_dsp_loader_dev(chip);
>
> -       if (!dmab->area)
> +       if (!dmab->area || !azx_dev->locked)
>                 return;
>
> +       mutex_lock(&chip->open_mutex);
> +       dsp_lock(azx_dev);
>         /* reset BDL address */
>         azx_sd_writel(azx_dev, SD_BDLPL, 0);
>         azx_sd_writel(azx_dev, SD_BDLPU, 0);
> @@ -2692,7 +2766,13 @@ static void azx_load_dsp_cleanup(struct hda_bus *bus,
>         snd_dma_free_pages(dmab);
>         dmab->area = NULL;
>
> -       snd_hda_unlock_devices(bus);
> +       spin_lock_irq(&chip->reg_lock);
> +       if (azx_dev->opened)
> +               *azx_dev = chip->saved_azx_dev;
> +       azx_dev->locked = 0;
> +       spin_unlock_irq(&chip->reg_lock);
> +       dsp_unlock(azx_dev);
> +       mutex_unlock(&chip->open_mutex);
>  }
>  #endif /* CONFIG_SND_HDA_DSP_LOADER */
>
> @@ -3481,6 +3561,7 @@ static int azx_first_init(struct azx *chip)
>         }
>
>         for (i = 0; i < chip->num_streams; i++) {
> +               dsp_lock_init(&chip->azx_dev[i]);
>                 /* allocate memory for the BDL for each stream */
>                 err = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV,
>                                           snd_dma_pci_data(chip->pci),
> --
> 1.8.1.4
>

Re: [PATCH 0/4] ALSA: Updates for the CA0132 codec

[Takashi Iwai]

At Fri, 15 Mar 2013 11:39:50 -0700,
Dylan Reid wrote:
> 
> On Fri, Mar 15, 2013 at 1:25 AM, Takashi Iwai <tiwai@suse.de> wrote:
> > At Thu, 14 Mar 2013 17:34:34 -0700,
> > Dylan Reid wrote:
> >>
> >> On Sun, Feb 10, 2013 at 2:57 AM, Takashi Iwai <tiwai@suse.de> wrote:
> >> > Hi Ian,
> >> >
> >> > At Fri, 8 Feb 2013 18:31:41 -0800,
> >> > Ian Minett wrote:
> >> >>
> >> >> From: Ian Minett <ian_minett@creativelabs.com>
> >> >>
> >> >> Hi,
> >> >>
> >> >> This series contains the following updates to CA0132:
> >> >>
> >> >> 1: Add firmware loading for the SpeakerEQ DSP effect.
> >> >> 2: Improve DSP transfer timeout calculations.
> >> >> 3: Add stream and effect latency monitoring routines
> >> >> 4: Update hp unsolicited event handler to manage queued work.
> >> >
> >> > Thanks, I took the one now but leave others.  See my reply to each.
> >> >
> >> >>
> >> >> These were based on the latest sound.git, but please let me know if
> >> >> they should be against sound-unstable tree instead.
> >> >
> >> > sound.git tree for-next branch would be the place to look at.
> >> > If I start a new branch for ca0132, I'll let you know.
> >> >
> >> >
> >> > But, before going further: as I already mailed you, I could get a test
> >> > board and started testing.  The result wasn't good.  At the first time
> >> > the driver loads, the DSP loading fails always.  When the driver is
> >> > reloaded after that, it's working.  So, something is still pretty
> >> > fragile.
> >>
> >> I tried this today and I can't get it to work.  It is failing in
> >> snd_hda_lock_devices called from azx_load_dsp_prepare.  The DSP is
> >> loaded from ca0132_init which is called as the result of open/resume.
> >> Both the check for open control files and attached substreams are
> >> failing. How can that be avoided?
> >
> > snd_hda_lock_devices() is obviously not suitable for the power-saving
> > or PM resume.  OK, this must be fixed indeed...
> >
> > How about the patch below?  (Note that it's just compile tested.)
> 
> Thanks Takashi.  This works for the init case, but it deadlocks in the
> case the load originates from open:
> azx_pcm_open -> calls snd_hda_power_up_d3wait -> ... -> ca0132_init ->
> azx_load_dsp_prepare
> 
> both open and load_dsp_prepare grab open_mutex.

Right.  The revised patch is below.


Takashi

---
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH v2] ALSA: hda - Fix abuse of snd_hda_lock_devices() for DSP loader

The current DSP loader code abuses snd_hda_lock_devices() for ensuring
the DSP loader not conflicting with the other normal operations.  But
this trick obviously doesn't work for the PM resume since the streams
are kept opened there where snd_hda_lock_devices() returns -EBUSY.
That means we need another lock mechanism instead of abuse.

This patch provides the new lock state to azx_dev.  Theoretically it's
possible that the DSP loader conflicts with the stream that has been
already assigned for another PCM.  If it's running, the DSP loader
should simply fail.  If not -- it's the case for PM resume --, we
should assign this stream temporarily to the DSP loader, and take it
back to the PCM after finishing DSP loading.  If the PCM is operated
during the DSP loading, it should get an error, too.

Reported-by: Dylan Reid <dgreid@chromium.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/hda_intel.c | 132 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 109 insertions(+), 23 deletions(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 4cea6bb6..880bdc7 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -415,6 +415,8 @@ struct azx_dev {
 	unsigned int opened :1;
 	unsigned int running :1;
 	unsigned int irq_pending :1;
+	unsigned int prepared:1;
+	unsigned int locked:1;
 	/*
 	 * For VIA:
 	 *  A flag to ensure DMA position is 0
@@ -426,8 +428,25 @@ struct azx_dev {
 
 	struct timecounter  azx_tc;
 	struct cyclecounter azx_cc;
+
+#ifdef CONFIG_SND_HDA_DSP_LOADER
+	struct mutex dsp_mutex;
+#endif
 };
 
+/* DSP lock helpers */
+#ifdef CONFIG_SND_HDA_DSP_LOADER
+#define dsp_lock_init(dev)	mutex_init(&(dev)->dsp_mutex)
+#define dsp_lock(dev)		mutex_lock(&(dev)->dsp_mutex)
+#define dsp_unlock(dev)		mutex_unlock(&(dev)->dsp_mutex)
+#define dsp_is_locked(dev)	(dev)->locked
+#else
+#define dsp_lock_init(dev)	do {} while (0)
+#define dsp_lock(dev)		do {} while (0)
+#define dsp_unlock(dev)		do {} while (0)
+#define dsp_is_locked(dev)	0
+#endif
+
 /* CORB/RIRB */
 struct azx_rb {
 	u32 *buf;		/* CORB/RIRB buffer
@@ -527,6 +546,10 @@ struct azx {
 
 	/* card list (for power_save trigger) */
 	struct list_head list;
+
+#ifdef CONFIG_SND_HDA_DSP_LOADER
+	struct azx_dev saved_azx_dev;
+#endif
 };
 
 #define CREATE_TRACE_POINTS
@@ -1793,15 +1816,25 @@ azx_assign_device(struct azx *chip, struct snd_pcm_substream *substream)
 		dev = chip->capture_index_offset;
 		nums = chip->capture_streams;
 	}
-	for (i = 0; i < nums; i++, dev++)
-		if (!chip->azx_dev[dev].opened) {
-			res = &chip->azx_dev[dev];
-			if (res->assigned_key == key)
-				break;
+	for (i = 0; i < nums; i++, dev++) {
+		struct azx_dev *azx_dev = &chip->azx_dev[dev];
+		dsp_lock(azx_dev);
+		if (!azx_dev->opened && !dsp_is_locked(azx_dev)) {
+			res = azx_dev;
+			if (res->assigned_key == key) {
+				res->opened = 1;
+				res->assigned_key = key;
+				dsp_unlock(azx_dev);
+				return res;
+			}
 		}
+		dsp_unlock(azx_dev);
+	}
 	if (res) {
+		dsp_lock(res);
 		res->opened = 1;
 		res->assigned_key = key;
+		dsp_unlock(res);
 	}
 	return res;
 }
@@ -2009,6 +2042,12 @@ static int azx_pcm_hw_params(struct snd_pcm_substream *substream,
 	struct azx_dev *azx_dev = get_azx_dev(substream);
 	int ret;
 
+	dsp_lock(azx_dev);
+	if (dsp_is_locked(azx_dev)) {
+		ret = -EBUSY;
+		goto unlock;
+	}
+
 	mark_runtime_wc(chip, azx_dev, substream, false);
 	azx_dev->bufsize = 0;
 	azx_dev->period_bytes = 0;
@@ -2016,8 +2055,10 @@ static int azx_pcm_hw_params(struct snd_pcm_substream *substream,
 	ret = snd_pcm_lib_malloc_pages(substream,
 					params_buffer_bytes(hw_params));
 	if (ret < 0)
-		return ret;
+		goto unlock;
 	mark_runtime_wc(chip, azx_dev, substream, true);
+ unlock:
+	dsp_unlock(azx_dev);
 	return ret;
 }
 
@@ -2029,16 +2070,21 @@ static int azx_pcm_hw_free(struct snd_pcm_substream *substream)
 	struct hda_pcm_stream *hinfo = apcm->hinfo[substream->stream];
 
 	/* reset BDL address */
-	azx_sd_writel(azx_dev, SD_BDLPL, 0);
-	azx_sd_writel(azx_dev, SD_BDLPU, 0);
-	azx_sd_writel(azx_dev, SD_CTL, 0);
-	azx_dev->bufsize = 0;
-	azx_dev->period_bytes = 0;
-	azx_dev->format_val = 0;
+	dsp_lock(azx_dev);
+	if (!dsp_is_locked(azx_dev)) {
+		azx_sd_writel(azx_dev, SD_BDLPL, 0);
+		azx_sd_writel(azx_dev, SD_BDLPU, 0);
+		azx_sd_writel(azx_dev, SD_CTL, 0);
+		azx_dev->bufsize = 0;
+		azx_dev->period_bytes = 0;
+		azx_dev->format_val = 0;
+	}
 
 	snd_hda_codec_cleanup(apcm->codec, hinfo, substream);
 
 	mark_runtime_wc(chip, azx_dev, substream, false);
+	azx_dev->prepared = 0;
+	dsp_unlock(azx_dev);
 	return snd_pcm_lib_free_pages(substream);
 }
 
@@ -2055,6 +2101,12 @@ static int azx_pcm_prepare(struct snd_pcm_substream *substream)
 		snd_hda_spdif_out_of_nid(apcm->codec, hinfo->nid);
 	unsigned short ctls = spdif ? spdif->ctls : 0;
 
+	dsp_lock(azx_dev);
+	if (dsp_is_locked(azx_dev)) {
+		err = -EBUSY;
+		goto unlock;
+	}
+
 	azx_stream_reset(chip, azx_dev);
 	format_val = snd_hda_calc_stream_format(runtime->rate,
 						runtime->channels,
@@ -2065,7 +2117,8 @@ static int azx_pcm_prepare(struct snd_pcm_substream *substream)
 		snd_printk(KERN_ERR SFX
 			   "%s: invalid format_val, rate=%d, ch=%d, format=%d\n",
 			   pci_name(chip->pci), runtime->rate, runtime->channels, runtime->format);
-		return -EINVAL;
+		err = -EINVAL;
+		goto unlock;
 	}
 
 	bufsize = snd_pcm_lib_buffer_bytes(substream);
@@ -2084,7 +2137,7 @@ static int azx_pcm_prepare(struct snd_pcm_substream *substream)
 		azx_dev->no_period_wakeup = runtime->no_period_wakeup;
 		err = azx_setup_periods(chip, substream, azx_dev);
 		if (err < 0)
-			return err;
+			goto unlock;
 	}
 
 	/* wallclk has 24Mhz clock source */
@@ -2101,8 +2154,14 @@ static int azx_pcm_prepare(struct snd_pcm_substream *substream)
 	if ((chip->driver_caps & AZX_DCAPS_CTX_WORKAROUND) &&
 	    stream_tag > chip->capture_streams)
 		stream_tag -= chip->capture_streams;
-	return snd_hda_codec_prepare(apcm->codec, hinfo, stream_tag,
+	err = snd_hda_codec_prepare(apcm->codec, hinfo, stream_tag,
 				     azx_dev->format_val, substream);
+
+ unlock:
+	if (!err)
+		azx_dev->prepared = 1;
+	dsp_unlock(azx_dev);
+	return err;
 }
 
 static int azx_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
@@ -2117,6 +2176,9 @@ static int azx_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 	azx_dev = get_azx_dev(substream);
 	trace_azx_pcm_trigger(chip, azx_dev, cmd);
 
+	if (!dsp_is_locked(azx_dev) || !azx_dev->prepared)
+		return -EPIPE;
+
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
 		rstart = 1;
@@ -2621,17 +2683,27 @@ static int azx_load_dsp_prepare(struct hda_bus *bus, unsigned int format,
 	struct azx_dev *azx_dev;
 	int err;
 
-	if (snd_hda_lock_devices(bus))
-		return -EBUSY;
+	azx_dev = azx_get_dsp_loader_dev(chip);
+
+	dsp_lock(azx_dev);
+	spin_lock_irq(&chip->reg_lock);
+	if (azx_dev->running || azx_dev->locked) {
+		spin_unlock_irq(&chip->reg_lock);
+		err = -EBUSY;
+		goto unlock;
+	}
+	azx_dev->prepared = 0;
+	chip->saved_azx_dev = *azx_dev;
+	azx_dev->locked = 1;
+	spin_unlock_irq(&chip->reg_lock);
 
 	err = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV_SG,
 				  snd_dma_pci_data(chip->pci),
 				  byte_size, bufp);
 	if (err < 0)
-		goto unlock;
+		goto err_alloc;
 
 	mark_pages_wc(chip, bufp, true);
-	azx_dev = azx_get_dsp_loader_dev(chip);
 	azx_dev->bufsize = byte_size;
 	azx_dev->period_bytes = byte_size;
 	azx_dev->format_val = format;
@@ -2649,13 +2721,20 @@ static int azx_load_dsp_prepare(struct hda_bus *bus, unsigned int format,
 		goto error;
 
 	azx_setup_controller(chip, azx_dev);
+	mutex_unlock(&chip->open_mutex);
 	return azx_dev->stream_tag;
 
  error:
 	mark_pages_wc(chip, bufp, false);
 	snd_dma_free_pages(bufp);
-unlock:
-	snd_hda_unlock_devices(bus);
+ err_alloc:
+	spin_lock_irq(&chip->reg_lock);
+	if (azx_dev->opened)
+		*azx_dev = chip->saved_azx_dev;
+	azx_dev->locked = 0;
+	spin_unlock_irq(&chip->reg_lock);
+ unlock:
+	dsp_unlock(azx_dev);
 	return err;
 }
 
@@ -2677,9 +2756,10 @@ static void azx_load_dsp_cleanup(struct hda_bus *bus,
 	struct azx *chip = bus->private_data;
 	struct azx_dev *azx_dev = azx_get_dsp_loader_dev(chip);
 
-	if (!dmab->area)
+	if (!dmab->area || !azx_dev->locked)
 		return;
 
+	dsp_lock(azx_dev);
 	/* reset BDL address */
 	azx_sd_writel(azx_dev, SD_BDLPL, 0);
 	azx_sd_writel(azx_dev, SD_BDLPU, 0);
@@ -2692,7 +2772,12 @@ static void azx_load_dsp_cleanup(struct hda_bus *bus,
 	snd_dma_free_pages(dmab);
 	dmab->area = NULL;
 
-	snd_hda_unlock_devices(bus);
+	spin_lock_irq(&chip->reg_lock);
+	if (azx_dev->opened)
+		*azx_dev = chip->saved_azx_dev;
+	azx_dev->locked = 0;
+	spin_unlock_irq(&chip->reg_lock);
+	dsp_unlock(azx_dev);
 }
 #endif /* CONFIG_SND_HDA_DSP_LOADER */
 
@@ -3481,6 +3566,7 @@ static int azx_first_init(struct azx *chip)
 	}
 
 	for (i = 0; i < chip->num_streams; i++) {
+		dsp_lock_init(&chip->azx_dev[i]);
 		/* allocate memory for the BDL for each stream */
 		err = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV,
 					  snd_dma_pci_data(chip->pci),
-- 
1.8.2

Re: [PATCH 0/4] ALSA: Updates for the CA0132 codec

[Dylan Reid]

On Fri, Mar 15, 2013 at 1:23 PM, Takashi Iwai <tiwai@suse.de> wrote:
> At Fri, 15 Mar 2013 11:39:50 -0700,
> Dylan Reid wrote:
>>
>> On Fri, Mar 15, 2013 at 1:25 AM, Takashi Iwai <tiwai@suse.de> wrote:
>> > At Thu, 14 Mar 2013 17:34:34 -0700,
>> > Dylan Reid wrote:
>> >>
>> >> On Sun, Feb 10, 2013 at 2:57 AM, Takashi Iwai <tiwai@suse.de> wrote:
>> >> > Hi Ian,
>> >> >
>> >> > At Fri, 8 Feb 2013 18:31:41 -0800,
>> >> > Ian Minett wrote:
>> >> >>
>> >> >> From: Ian Minett <ian_minett@creativelabs.com>
>> >> >>
>> >> >> Hi,
>> >> >>
>> >> >> This series contains the following updates to CA0132:
>> >> >>
>> >> >> 1: Add firmware loading for the SpeakerEQ DSP effect.
>> >> >> 2: Improve DSP transfer timeout calculations.
>> >> >> 3: Add stream and effect latency monitoring routines
>> >> >> 4: Update hp unsolicited event handler to manage queued work.
>> >> >
>> >> > Thanks, I took the one now but leave others.  See my reply to each.
>> >> >
>> >> >>
>> >> >> These were based on the latest sound.git, but please let me know if
>> >> >> they should be against sound-unstable tree instead.
>> >> >
>> >> > sound.git tree for-next branch would be the place to look at.
>> >> > If I start a new branch for ca0132, I'll let you know.
>> >> >
>> >> >
>> >> > But, before going further: as I already mailed you, I could get a test
>> >> > board and started testing.  The result wasn't good.  At the first time
>> >> > the driver loads, the DSP loading fails always.  When the driver is
>> >> > reloaded after that, it's working.  So, something is still pretty
>> >> > fragile.
>> >>
>> >> I tried this today and I can't get it to work.  It is failing in
>> >> snd_hda_lock_devices called from azx_load_dsp_prepare.  The DSP is
>> >> loaded from ca0132_init which is called as the result of open/resume.
>> >> Both the check for open control files and attached substreams are
>> >> failing. How can that be avoided?
>> >
>> > snd_hda_lock_devices() is obviously not suitable for the power-saving
>> > or PM resume.  OK, this must be fixed indeed...
>> >
>> > How about the patch below?  (Note that it's just compile tested.)
>>
>> Thanks Takashi.  This works for the init case, but it deadlocks in the
>> case the load originates from open:
>> azx_pcm_open -> calls snd_hda_power_up_d3wait -> ... -> ca0132_init ->
>> azx_load_dsp_prepare
>>
>> both open and load_dsp_prepare grab open_mutex.
>
> Right.  The revised patch is below.

With the two changes I noted inline this works.  I tested it at
startup, open/close and suspend/resume.

Thanks for the speedy fixes!

>
>
> Takashi
>
> ---
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH v2] ALSA: hda - Fix abuse of snd_hda_lock_devices() for DSP loader
>
> The current DSP loader code abuses snd_hda_lock_devices() for ensuring
> the DSP loader not conflicting with the other normal operations.  But
> this trick obviously doesn't work for the PM resume since the streams
> are kept opened there where snd_hda_lock_devices() returns -EBUSY.
> That means we need another lock mechanism instead of abuse.
>
> This patch provides the new lock state to azx_dev.  Theoretically it's
> possible that the DSP loader conflicts with the stream that has been
> already assigned for another PCM.  If it's running, the DSP loader
> should simply fail.  If not -- it's the case for PM resume --, we
> should assign this stream temporarily to the DSP loader, and take it
> back to the PCM after finishing DSP loading.  If the PCM is operated
> during the DSP loading, it should get an error, too.
>
> Reported-by: Dylan Reid <dgreid@chromium.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  sound/pci/hda/hda_intel.c | 132 ++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 109 insertions(+), 23 deletions(-)
>
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index 4cea6bb6..880bdc7 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -415,6 +415,8 @@ struct azx_dev {
>         unsigned int opened :1;
>         unsigned int running :1;
>         unsigned int irq_pending :1;
> +       unsigned int prepared:1;
> +       unsigned int locked:1;
>         /*
>          * For VIA:
>          *  A flag to ensure DMA position is 0
> @@ -426,8 +428,25 @@ struct azx_dev {
>
>         struct timecounter  azx_tc;
>         struct cyclecounter azx_cc;
> +
> +#ifdef CONFIG_SND_HDA_DSP_LOADER
> +       struct mutex dsp_mutex;
> +#endif
>  };
>
> +/* DSP lock helpers */
> +#ifdef CONFIG_SND_HDA_DSP_LOADER
> +#define dsp_lock_init(dev)     mutex_init(&(dev)->dsp_mutex)
> +#define dsp_lock(dev)          mutex_lock(&(dev)->dsp_mutex)
> +#define dsp_unlock(dev)                mutex_unlock(&(dev)->dsp_mutex)
> +#define dsp_is_locked(dev)     (dev)->locked
> +#else
> +#define dsp_lock_init(dev)     do {} while (0)
> +#define dsp_lock(dev)          do {} while (0)
> +#define dsp_unlock(dev)                do {} while (0)
> +#define dsp_is_locked(dev)     0
> +#endif
> +
>  /* CORB/RIRB */
>  struct azx_rb {
>         u32 *buf;               /* CORB/RIRB buffer
> @@ -527,6 +546,10 @@ struct azx {
>
>         /* card list (for power_save trigger) */
>         struct list_head list;
> +
> +#ifdef CONFIG_SND_HDA_DSP_LOADER
> +       struct azx_dev saved_azx_dev;
> +#endif
>  };
>
>  #define CREATE_TRACE_POINTS
> @@ -1793,15 +1816,25 @@ azx_assign_device(struct azx *chip, struct snd_pcm_substream *substream)
>                 dev = chip->capture_index_offset;
>                 nums = chip->capture_streams;
>         }
> -       for (i = 0; i < nums; i++, dev++)
> -               if (!chip->azx_dev[dev].opened) {
> -                       res = &chip->azx_dev[dev];
> -                       if (res->assigned_key == key)
> -                               break;
> +       for (i = 0; i < nums; i++, dev++) {
> +               struct azx_dev *azx_dev = &chip->azx_dev[dev];
> +               dsp_lock(azx_dev);
> +               if (!azx_dev->opened && !dsp_is_locked(azx_dev)) {
> +                       res = azx_dev;
> +                       if (res->assigned_key == key) {
> +                               res->opened = 1;
> +                               res->assigned_key = key;
> +                               dsp_unlock(azx_dev);
> +                               return res;
> +                       }
>                 }
> +               dsp_unlock(azx_dev);
> +       }
>         if (res) {
> +               dsp_lock(res);
>                 res->opened = 1;
>                 res->assigned_key = key;
> +               dsp_unlock(res);
>         }
>         return res;
>  }
> @@ -2009,6 +2042,12 @@ static int azx_pcm_hw_params(struct snd_pcm_substream *substream,
>         struct azx_dev *azx_dev = get_azx_dev(substream);
>         int ret;
>
> +       dsp_lock(azx_dev);
> +       if (dsp_is_locked(azx_dev)) {
> +               ret = -EBUSY;
> +               goto unlock;
> +       }
> +
>         mark_runtime_wc(chip, azx_dev, substream, false);
>         azx_dev->bufsize = 0;
>         azx_dev->period_bytes = 0;
> @@ -2016,8 +2055,10 @@ static int azx_pcm_hw_params(struct snd_pcm_substream *substream,
>         ret = snd_pcm_lib_malloc_pages(substream,
>                                         params_buffer_bytes(hw_params));
>         if (ret < 0)
> -               return ret;
> +               goto unlock;
>         mark_runtime_wc(chip, azx_dev, substream, true);
> + unlock:
> +       dsp_unlock(azx_dev);
>         return ret;
>  }
>
> @@ -2029,16 +2070,21 @@ static int azx_pcm_hw_free(struct snd_pcm_substream *substream)
>         struct hda_pcm_stream *hinfo = apcm->hinfo[substream->stream];
>
>         /* reset BDL address */
> -       azx_sd_writel(azx_dev, SD_BDLPL, 0);
> -       azx_sd_writel(azx_dev, SD_BDLPU, 0);
> -       azx_sd_writel(azx_dev, SD_CTL, 0);
> -       azx_dev->bufsize = 0;
> -       azx_dev->period_bytes = 0;
> -       azx_dev->format_val = 0;
> +       dsp_lock(azx_dev);
> +       if (!dsp_is_locked(azx_dev)) {
> +               azx_sd_writel(azx_dev, SD_BDLPL, 0);
> +               azx_sd_writel(azx_dev, SD_BDLPU, 0);
> +               azx_sd_writel(azx_dev, SD_CTL, 0);
> +               azx_dev->bufsize = 0;
> +               azx_dev->period_bytes = 0;
> +               azx_dev->format_val = 0;
> +       }
>
>         snd_hda_codec_cleanup(apcm->codec, hinfo, substream);
>
>         mark_runtime_wc(chip, azx_dev, substream, false);
> +       azx_dev->prepared = 0;
> +       dsp_unlock(azx_dev);
>         return snd_pcm_lib_free_pages(substream);
>  }
>
> @@ -2055,6 +2101,12 @@ static int azx_pcm_prepare(struct snd_pcm_substream *substream)
>                 snd_hda_spdif_out_of_nid(apcm->codec, hinfo->nid);
>         unsigned short ctls = spdif ? spdif->ctls : 0;
>
> +       dsp_lock(azx_dev);
> +       if (dsp_is_locked(azx_dev)) {
> +               err = -EBUSY;
> +               goto unlock;
> +       }
> +
>         azx_stream_reset(chip, azx_dev);
>         format_val = snd_hda_calc_stream_format(runtime->rate,
>                                                 runtime->channels,
> @@ -2065,7 +2117,8 @@ static int azx_pcm_prepare(struct snd_pcm_substream *substream)
>                 snd_printk(KERN_ERR SFX
>                            "%s: invalid format_val, rate=%d, ch=%d, format=%d\n",
>                            pci_name(chip->pci), runtime->rate, runtime->channels, runtime->format);
> -               return -EINVAL;
> +               err = -EINVAL;
> +               goto unlock;
>         }
>
>         bufsize = snd_pcm_lib_buffer_bytes(substream);
> @@ -2084,7 +2137,7 @@ static int azx_pcm_prepare(struct snd_pcm_substream *substream)
>                 azx_dev->no_period_wakeup = runtime->no_period_wakeup;
>                 err = azx_setup_periods(chip, substream, azx_dev);
>                 if (err < 0)
> -                       return err;
> +                       goto unlock;
>         }
>
>         /* wallclk has 24Mhz clock source */
> @@ -2101,8 +2154,14 @@ static int azx_pcm_prepare(struct snd_pcm_substream *substream)
>         if ((chip->driver_caps & AZX_DCAPS_CTX_WORKAROUND) &&
>             stream_tag > chip->capture_streams)
>                 stream_tag -= chip->capture_streams;
> -       return snd_hda_codec_prepare(apcm->codec, hinfo, stream_tag,
> +       err = snd_hda_codec_prepare(apcm->codec, hinfo, stream_tag,
>                                      azx_dev->format_val, substream);
> +
> + unlock:
> +       if (!err)
> +               azx_dev->prepared = 1;
> +       dsp_unlock(azx_dev);
> +       return err;
>  }
>
>  static int azx_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
> @@ -2117,6 +2176,9 @@ static int azx_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
>         azx_dev = get_azx_dev(substream);
>         trace_azx_pcm_trigger(chip, azx_dev, cmd);
>
> +       if (!dsp_is_locked(azx_dev) || !azx_dev->prepared)

I inverted this check. I think the intent here is that either the dsp
load is active or a substream is prepared.  If that's true this should
return when both are false.

> +               return -EPIPE;
> +
>         switch (cmd) {
>         case SNDRV_PCM_TRIGGER_START:
>                 rstart = 1;
> @@ -2621,17 +2683,27 @@ static int azx_load_dsp_prepare(struct hda_bus *bus, unsigned int format,
>         struct azx_dev *azx_dev;
>         int err;
>
> -       if (snd_hda_lock_devices(bus))
> -               return -EBUSY;
> +       azx_dev = azx_get_dsp_loader_dev(chip);
> +
> +       dsp_lock(azx_dev);
> +       spin_lock_irq(&chip->reg_lock);
> +       if (azx_dev->running || azx_dev->locked) {
> +               spin_unlock_irq(&chip->reg_lock);
> +               err = -EBUSY;
> +               goto unlock;
> +       }
> +       azx_dev->prepared = 0;
> +       chip->saved_azx_dev = *azx_dev;
> +       azx_dev->locked = 1;
> +       spin_unlock_irq(&chip->reg_lock);
>
>         err = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV_SG,
>                                   snd_dma_pci_data(chip->pci),
>                                   byte_size, bufp);
>         if (err < 0)
> -               goto unlock;
> +               goto err_alloc;
>
>         mark_pages_wc(chip, bufp, true);
> -       azx_dev = azx_get_dsp_loader_dev(chip);
>         azx_dev->bufsize = byte_size;
>         azx_dev->period_bytes = byte_size;
>         azx_dev->format_val = format;
> @@ -2649,13 +2721,20 @@ static int azx_load_dsp_prepare(struct hda_bus *bus, unsigned int format,
>                 goto error;
>
>         azx_setup_controller(chip, azx_dev);

I added dsp_unlock(azx_dev); here.

> +       mutex_unlock(&chip->open_mutex);
>         return azx_dev->stream_tag;
>
>   error:
>         mark_pages_wc(chip, bufp, false);
>         snd_dma_free_pages(bufp);
> -unlock:
> -       snd_hda_unlock_devices(bus);
> + err_alloc:
> +       spin_lock_irq(&chip->reg_lock);
> +       if (azx_dev->opened)
> +               *azx_dev = chip->saved_azx_dev;
> +       azx_dev->locked = 0;
> +       spin_unlock_irq(&chip->reg_lock);
> + unlock:
> +       dsp_unlock(azx_dev);
>         return err;
>  }
>
> @@ -2677,9 +2756,10 @@ static void azx_load_dsp_cleanup(struct hda_bus *bus,
>         struct azx *chip = bus->private_data;
>         struct azx_dev *azx_dev = azx_get_dsp_loader_dev(chip);
>
> -       if (!dmab->area)
> +       if (!dmab->area || !azx_dev->locked)
>                 return;
>
> +       dsp_lock(azx_dev);
>         /* reset BDL address */
>         azx_sd_writel(azx_dev, SD_BDLPL, 0);
>         azx_sd_writel(azx_dev, SD_BDLPU, 0);
> @@ -2692,7 +2772,12 @@ static void azx_load_dsp_cleanup(struct hda_bus *bus,
>         snd_dma_free_pages(dmab);
>         dmab->area = NULL;
>
> -       snd_hda_unlock_devices(bus);
> +       spin_lock_irq(&chip->reg_lock);
> +       if (azx_dev->opened)
> +               *azx_dev = chip->saved_azx_dev;
> +       azx_dev->locked = 0;
> +       spin_unlock_irq(&chip->reg_lock);
> +       dsp_unlock(azx_dev);
>  }
>  #endif /* CONFIG_SND_HDA_DSP_LOADER */
>
> @@ -3481,6 +3566,7 @@ static int azx_first_init(struct azx *chip)
>         }
>
>         for (i = 0; i < chip->num_streams; i++) {
> +               dsp_lock_init(&chip->azx_dev[i]);
>                 /* allocate memory for the BDL for each stream */
>                 err = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV,
>                                           snd_dma_pci_data(chip->pci),
> --
> 1.8.2
>

Re: [PATCH 0/4] ALSA: Updates for the CA0132 codec

[Takashi Iwai]

At Wed, 20 Mar 2013 10:21:59 -0700,
Dylan Reid wrote:
> 
> On Fri, Mar 15, 2013 at 1:23 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > At Fri, 15 Mar 2013 11:39:50 -0700,
> > Dylan Reid wrote:
> >>
> >> On Fri, Mar 15, 2013 at 1:25 AM, Takashi Iwai <tiwai@suse.de> wrote:
> >> > At Thu, 14 Mar 2013 17:34:34 -0700,
> >> > Dylan Reid wrote:
> >> >>
> >> >> On Sun, Feb 10, 2013 at 2:57 AM, Takashi Iwai <tiwai@suse.de> wrote:
> >> >> > Hi Ian,
> >> >> >
> >> >> > At Fri, 8 Feb 2013 18:31:41 -0800,
> >> >> > Ian Minett wrote:
> >> >> >>
> >> >> >> From: Ian Minett <ian_minett@creativelabs.com>
> >> >> >>
> >> >> >> Hi,
> >> >> >>
> >> >> >> This series contains the following updates to CA0132:
> >> >> >>
> >> >> >> 1: Add firmware loading for the SpeakerEQ DSP effect.
> >> >> >> 2: Improve DSP transfer timeout calculations.
> >> >> >> 3: Add stream and effect latency monitoring routines
> >> >> >> 4: Update hp unsolicited event handler to manage queued work.
> >> >> >
> >> >> > Thanks, I took the one now but leave others.  See my reply to each.
> >> >> >
> >> >> >>
> >> >> >> These were based on the latest sound.git, but please let me know if
> >> >> >> they should be against sound-unstable tree instead.
> >> >> >
> >> >> > sound.git tree for-next branch would be the place to look at.
> >> >> > If I start a new branch for ca0132, I'll let you know.
> >> >> >
> >> >> >
> >> >> > But, before going further: as I already mailed you, I could get a test
> >> >> > board and started testing.  The result wasn't good.  At the first time
> >> >> > the driver loads, the DSP loading fails always.  When the driver is
> >> >> > reloaded after that, it's working.  So, something is still pretty
> >> >> > fragile.
> >> >>
> >> >> I tried this today and I can't get it to work.  It is failing in
> >> >> snd_hda_lock_devices called from azx_load_dsp_prepare.  The DSP is
> >> >> loaded from ca0132_init which is called as the result of open/resume.
> >> >> Both the check for open control files and attached substreams are
> >> >> failing. How can that be avoided?
> >> >
> >> > snd_hda_lock_devices() is obviously not suitable for the power-saving
> >> > or PM resume.  OK, this must be fixed indeed...
> >> >
> >> > How about the patch below?  (Note that it's just compile tested.)
> >>
> >> Thanks Takashi.  This works for the init case, but it deadlocks in the
> >> case the load originates from open:
> >> azx_pcm_open -> calls snd_hda_power_up_d3wait -> ... -> ca0132_init ->
> >> azx_load_dsp_prepare
> >>
> >> both open and load_dsp_prepare grab open_mutex.
> >
> > Right.  The revised patch is below.
> 
> With the two changes I noted inline this works.  I tested it at
> startup, open/close and suspend/resume.
> 
> Thanks for the speedy fixes!

Thanks, I fixed the patch and committed now.


Takashi

> 
> >
> >
> > Takashi
> >
> > ---
> > From: Takashi Iwai <tiwai@suse.de>
> > Subject: [PATCH v2] ALSA: hda - Fix abuse of snd_hda_lock_devices() for DSP loader
> >
> > The current DSP loader code abuses snd_hda_lock_devices() for ensuring
> > the DSP loader not conflicting with the other normal operations.  But
> > this trick obviously doesn't work for the PM resume since the streams
> > are kept opened there where snd_hda_lock_devices() returns -EBUSY.
> > That means we need another lock mechanism instead of abuse.
> >
> > This patch provides the new lock state to azx_dev.  Theoretically it's
> > possible that the DSP loader conflicts with the stream that has been
> > already assigned for another PCM.  If it's running, the DSP loader
> > should simply fail.  If not -- it's the case for PM resume --, we
> > should assign this stream temporarily to the DSP loader, and take it
> > back to the PCM after finishing DSP loading.  If the PCM is operated
> > during the DSP loading, it should get an error, too.
> >
> > Reported-by: Dylan Reid <dgreid@chromium.org>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  sound/pci/hda/hda_intel.c | 132 ++++++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 109 insertions(+), 23 deletions(-)
> >
> > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > index 4cea6bb6..880bdc7 100644
> > --- a/sound/pci/hda/hda_intel.c
> > +++ b/sound/pci/hda/hda_intel.c
> > @@ -415,6 +415,8 @@ struct azx_dev {
> >         unsigned int opened :1;
> >         unsigned int running :1;
> >         unsigned int irq_pending :1;
> > +       unsigned int prepared:1;
> > +       unsigned int locked:1;
> >         /*
> >          * For VIA:
> >          *  A flag to ensure DMA position is 0
> > @@ -426,8 +428,25 @@ struct azx_dev {
> >
> >         struct timecounter  azx_tc;
> >         struct cyclecounter azx_cc;
> > +
> > +#ifdef CONFIG_SND_HDA_DSP_LOADER
> > +       struct mutex dsp_mutex;
> > +#endif
> >  };
> >
> > +/* DSP lock helpers */
> > +#ifdef CONFIG_SND_HDA_DSP_LOADER
> > +#define dsp_lock_init(dev)     mutex_init(&(dev)->dsp_mutex)
> > +#define dsp_lock(dev)          mutex_lock(&(dev)->dsp_mutex)
> > +#define dsp_unlock(dev)                mutex_unlock(&(dev)->dsp_mutex)
> > +#define dsp_is_locked(dev)     (dev)->locked
> > +#else
> > +#define dsp_lock_init(dev)     do {} while (0)
> > +#define dsp_lock(dev)          do {} while (0)
> > +#define dsp_unlock(dev)                do {} while (0)
> > +#define dsp_is_locked(dev)     0
> > +#endif
> > +
> >  /* CORB/RIRB */
> >  struct azx_rb {
> >         u32 *buf;               /* CORB/RIRB buffer
> > @@ -527,6 +546,10 @@ struct azx {
> >
> >         /* card list (for power_save trigger) */
> >         struct list_head list;
> > +
> > +#ifdef CONFIG_SND_HDA_DSP_LOADER
> > +       struct azx_dev saved_azx_dev;
> > +#endif
> >  };
> >
> >  #define CREATE_TRACE_POINTS
> > @@ -1793,15 +1816,25 @@ azx_assign_device(struct azx *chip, struct snd_pcm_substream *substream)
> >                 dev = chip->capture_index_offset;
> >                 nums = chip->capture_streams;
> >         }
> > -       for (i = 0; i < nums; i++, dev++)
> > -               if (!chip->azx_dev[dev].opened) {
> > -                       res = &chip->azx_dev[dev];
> > -                       if (res->assigned_key == key)
> > -                               break;
> > +       for (i = 0; i < nums; i++, dev++) {
> > +               struct azx_dev *azx_dev = &chip->azx_dev[dev];
> > +               dsp_lock(azx_dev);
> > +               if (!azx_dev->opened && !dsp_is_locked(azx_dev)) {
> > +                       res = azx_dev;
> > +                       if (res->assigned_key == key) {
> > +                               res->opened = 1;
> > +                               res->assigned_key = key;
> > +                               dsp_unlock(azx_dev);
> > +                               return res;
> > +                       }
> >                 }
> > +               dsp_unlock(azx_dev);
> > +       }
> >         if (res) {
> > +               dsp_lock(res);
> >                 res->opened = 1;
> >                 res->assigned_key = key;
> > +               dsp_unlock(res);
> >         }
> >         return res;
> >  }
> > @@ -2009,6 +2042,12 @@ static int azx_pcm_hw_params(struct snd_pcm_substream *substream,
> >         struct azx_dev *azx_dev = get_azx_dev(substream);
> >         int ret;
> >
> > +       dsp_lock(azx_dev);
> > +       if (dsp_is_locked(azx_dev)) {
> > +               ret = -EBUSY;
> > +               goto unlock;
> > +       }
> > +
> >         mark_runtime_wc(chip, azx_dev, substream, false);
> >         azx_dev->bufsize = 0;
> >         azx_dev->period_bytes = 0;
> > @@ -2016,8 +2055,10 @@ static int azx_pcm_hw_params(struct snd_pcm_substream *substream,
> >         ret = snd_pcm_lib_malloc_pages(substream,
> >                                         params_buffer_bytes(hw_params));
> >         if (ret < 0)
> > -               return ret;
> > +               goto unlock;
> >         mark_runtime_wc(chip, azx_dev, substream, true);
> > + unlock:
> > +       dsp_unlock(azx_dev);
> >         return ret;
> >  }
> >
> > @@ -2029,16 +2070,21 @@ static int azx_pcm_hw_free(struct snd_pcm_substream *substream)
> >         struct hda_pcm_stream *hinfo = apcm->hinfo[substream->stream];
> >
> >         /* reset BDL address */
> > -       azx_sd_writel(azx_dev, SD_BDLPL, 0);
> > -       azx_sd_writel(azx_dev, SD_BDLPU, 0);
> > -       azx_sd_writel(azx_dev, SD_CTL, 0);
> > -       azx_dev->bufsize = 0;
> > -       azx_dev->period_bytes = 0;
> > -       azx_dev->format_val = 0;
> > +       dsp_lock(azx_dev);
> > +       if (!dsp_is_locked(azx_dev)) {
> > +               azx_sd_writel(azx_dev, SD_BDLPL, 0);
> > +               azx_sd_writel(azx_dev, SD_BDLPU, 0);
> > +               azx_sd_writel(azx_dev, SD_CTL, 0);
> > +               azx_dev->bufsize = 0;
> > +               azx_dev->period_bytes = 0;
> > +               azx_dev->format_val = 0;
> > +       }
> >
> >         snd_hda_codec_cleanup(apcm->codec, hinfo, substream);
> >
> >         mark_runtime_wc(chip, azx_dev, substream, false);
> > +       azx_dev->prepared = 0;
> > +       dsp_unlock(azx_dev);
> >         return snd_pcm_lib_free_pages(substream);
> >  }
> >
> > @@ -2055,6 +2101,12 @@ static int azx_pcm_prepare(struct snd_pcm_substream *substream)
> >                 snd_hda_spdif_out_of_nid(apcm->codec, hinfo->nid);
> >         unsigned short ctls = spdif ? spdif->ctls : 0;
> >
> > +       dsp_lock(azx_dev);
> > +       if (dsp_is_locked(azx_dev)) {
> > +               err = -EBUSY;
> > +               goto unlock;
> > +       }
> > +
> >         azx_stream_reset(chip, azx_dev);
> >         format_val = snd_hda_calc_stream_format(runtime->rate,
> >                                                 runtime->channels,
> > @@ -2065,7 +2117,8 @@ static int azx_pcm_prepare(struct snd_pcm_substream *substream)
> >                 snd_printk(KERN_ERR SFX
> >                            "%s: invalid format_val, rate=%d, ch=%d, format=%d\n",
> >                            pci_name(chip->pci), runtime->rate, runtime->channels, runtime->format);
> > -               return -EINVAL;
> > +               err = -EINVAL;
> > +               goto unlock;
> >         }
> >
> >         bufsize = snd_pcm_lib_buffer_bytes(substream);
> > @@ -2084,7 +2137,7 @@ static int azx_pcm_prepare(struct snd_pcm_substream *substream)
> >                 azx_dev->no_period_wakeup = runtime->no_period_wakeup;
> >                 err = azx_setup_periods(chip, substream, azx_dev);
> >                 if (err < 0)
> > -                       return err;
> > +                       goto unlock;
> >         }
> >
> >         /* wallclk has 24Mhz clock source */
> > @@ -2101,8 +2154,14 @@ static int azx_pcm_prepare(struct snd_pcm_substream *substream)
> >         if ((chip->driver_caps & AZX_DCAPS_CTX_WORKAROUND) &&
> >             stream_tag > chip->capture_streams)
> >                 stream_tag -= chip->capture_streams;
> > -       return snd_hda_codec_prepare(apcm->codec, hinfo, stream_tag,
> > +       err = snd_hda_codec_prepare(apcm->codec, hinfo, stream_tag,
> >                                      azx_dev->format_val, substream);
> > +
> > + unlock:
> > +       if (!err)
> > +               azx_dev->prepared = 1;
> > +       dsp_unlock(azx_dev);
> > +       return err;
> >  }
> >
> >  static int azx_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
> > @@ -2117,6 +2176,9 @@ static int azx_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
> >         azx_dev = get_azx_dev(substream);
> >         trace_azx_pcm_trigger(chip, azx_dev, cmd);
> >
> > +       if (!dsp_is_locked(azx_dev) || !azx_dev->prepared)
> 
> I inverted this check. I think the intent here is that either the dsp
> load is active or a substream is prepared.  If that's true this should
> return when both are false.
> 
> > +               return -EPIPE;
> > +
> >         switch (cmd) {
> >         case SNDRV_PCM_TRIGGER_START:
> >                 rstart = 1;
> > @@ -2621,17 +2683,27 @@ static int azx_load_dsp_prepare(struct hda_bus *bus, unsigned int format,
> >         struct azx_dev *azx_dev;
> >         int err;
> >
> > -       if (snd_hda_lock_devices(bus))
> > -               return -EBUSY;
> > +       azx_dev = azx_get_dsp_loader_dev(chip);
> > +
> > +       dsp_lock(azx_dev);
> > +       spin_lock_irq(&chip->reg_lock);
> > +       if (azx_dev->running || azx_dev->locked) {
> > +               spin_unlock_irq(&chip->reg_lock);
> > +               err = -EBUSY;
> > +               goto unlock;
> > +       }
> > +       azx_dev->prepared = 0;
> > +       chip->saved_azx_dev = *azx_dev;
> > +       azx_dev->locked = 1;
> > +       spin_unlock_irq(&chip->reg_lock);
> >
> >         err = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV_SG,
> >                                   snd_dma_pci_data(chip->pci),
> >                                   byte_size, bufp);
> >         if (err < 0)
> > -               goto unlock;
> > +               goto err_alloc;
> >
> >         mark_pages_wc(chip, bufp, true);
> > -       azx_dev = azx_get_dsp_loader_dev(chip);
> >         azx_dev->bufsize = byte_size;
> >         azx_dev->period_bytes = byte_size;
> >         azx_dev->format_val = format;
> > @@ -2649,13 +2721,20 @@ static int azx_load_dsp_prepare(struct hda_bus *bus, unsigned int format,
> >                 goto error;
> >
> >         azx_setup_controller(chip, azx_dev);
> 
> I added dsp_unlock(azx_dev); here.
> 
> > +       mutex_unlock(&chip->open_mutex);
> >         return azx_dev->stream_tag;
> >
> >   error:
> >         mark_pages_wc(chip, bufp, false);
> >         snd_dma_free_pages(bufp);
> > -unlock:
> > -       snd_hda_unlock_devices(bus);
> > + err_alloc:
> > +       spin_lock_irq(&chip->reg_lock);
> > +       if (azx_dev->opened)
> > +               *azx_dev = chip->saved_azx_dev;
> > +       azx_dev->locked = 0;
> > +       spin_unlock_irq(&chip->reg_lock);
> > + unlock:
> > +       dsp_unlock(azx_dev);
> >         return err;
> >  }
> >
> > @@ -2677,9 +2756,10 @@ static void azx_load_dsp_cleanup(struct hda_bus *bus,
> >         struct azx *chip = bus->private_data;
> >         struct azx_dev *azx_dev = azx_get_dsp_loader_dev(chip);
> >
> > -       if (!dmab->area)
> > +       if (!dmab->area || !azx_dev->locked)
> >                 return;
> >
> > +       dsp_lock(azx_dev);
> >         /* reset BDL address */
> >         azx_sd_writel(azx_dev, SD_BDLPL, 0);
> >         azx_sd_writel(azx_dev, SD_BDLPU, 0);
> > @@ -2692,7 +2772,12 @@ static void azx_load_dsp_cleanup(struct hda_bus *bus,
> >         snd_dma_free_pages(dmab);
> >         dmab->area = NULL;
> >
> > -       snd_hda_unlock_devices(bus);
> > +       spin_lock_irq(&chip->reg_lock);
> > +       if (azx_dev->opened)
> > +               *azx_dev = chip->saved_azx_dev;
> > +       azx_dev->locked = 0;
> > +       spin_unlock_irq(&chip->reg_lock);
> > +       dsp_unlock(azx_dev);
> >  }
> >  #endif /* CONFIG_SND_HDA_DSP_LOADER */
> >
> > @@ -3481,6 +3566,7 @@ static int azx_first_init(struct azx *chip)
> >         }
> >
> >         for (i = 0; i < chip->num_streams; i++) {
> > +               dsp_lock_init(&chip->azx_dev[i]);
> >                 /* allocate memory for the BDL for each stream */
> >                 err = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV,
> >                                           snd_dma_pci_data(chip->pci),
> > --
> > 1.8.2
> >
> 

Re: [PATCH 3/4] ALSA: CA0132: Add latency monitoring

[Dylan Reid]

On Sun, Feb 10, 2013 at 2:51 AM, Takashi Iwai <tiwai@suse.de> wrote:
> At Fri, 8 Feb 2013 18:31:44 -0800,
> Ian Minett wrote:
>>
>> From: Ian Minett <ian_minett@creativelabs.com>
>>
>> Add latency monitoring for playback, capture and DSP effects.
>
> Please give a bit more descriptions.
> Looking at the patch, it doesn't implement only the latency monitoring
> but actually reflecting to the runtime->delay.
>
> Also..
>
>
>> Signed-off-by: Ian Minett <ian_minett@creativelabs.com>
>>
>> diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c
>> index b52f00c..c797f65 100644
>> --- a/sound/pci/hda/patch_ca0132.c
>> +++ b/sound/pci/hda/patch_ca0132.c
>> @@ -133,6 +133,12 @@ enum {
>>  /* Effects values size*/
>>  #define EFFECT_VALS_MAX_COUNT 12
>>
>> +#define DSP_CAPTURE_INIT_LATENCY        0
>> +#define DSP_CRYSTAL_VOICE_LATENCY       124
>> +#define DSP_PLAYBACK_INIT_LATENCY       13
>> +#define DSP_PLAY_ENHANCEMENT_LATENCY    30
>> +#define DSP_SPEAKER_OUT_LATENCY         7
>> +
>>  struct ct_effect {
>>       char name[44];
>>       hda_nid_t nid;
>> @@ -2761,8 +2767,27 @@ static void ca0132_cleanup_stream(struct hda_codec *codec, hda_nid_t nid)
>>  }
>>
>>  /*
>> - * PCM callbacks
>> + * PCM playbacks
>>   */
>> +static unsigned int ca0132_get_playback_latency(struct hda_codec *codec)
>> +{
>> +     struct ca0132_spec *spec = codec->spec;
>> +     unsigned int latency = DSP_PLAYBACK_INIT_LATENCY;
>> +
>> +     if (!dspload_is_loaded(codec))
>> +             return 0;
>> +
>> +     if (spec->effects_switch[PLAY_ENHANCEMENT - EFFECT_START_NID]) {
>> +             if ((spec->effects_switch[SURROUND - EFFECT_START_NID]) ||
>> +                 (spec->effects_switch[DIALOG_PLUS - EFFECT_START_NID]))
>> +                     latency += DSP_PLAY_ENHANCEMENT_LATENCY;
>> +     }
>> +
>> +     if (spec->cur_out_type == SPEAKER_OUT)
>> +             latency += DSP_SPEAKER_OUT_LATENCY;
>> +     return latency;
>> +}
>> +
>>  static int ca0132_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
>>                       struct hda_codec *codec,
>>                       unsigned int stream_tag,
>> @@ -2770,6 +2795,16 @@ static int ca0132_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
>>                       struct snd_pcm_substream *substream)
>>  {
>>       struct ca0132_spec *spec = codec->spec;
>> +     struct snd_pcm_runtime *runtime = substream->runtime;
>> +     unsigned int latency = ca0132_get_playback_latency(codec);
>> +
>> +     if (spec->dsp_state == DSP_DOWNLOADING) {
>> +             spec->dsp_stream_id = stream_tag;
>> +             return 0;
>> +     }
>> +
>> +     runtime->delay = bytes_to_frames(runtime, (latency * runtime->rate *
>> +                                     runtime->byte_align) / 1000);
>>
>>       ca0132_setup_stream(codec, spec->dacs[0], stream_tag, 0, format);
>>
>> @@ -2834,8 +2869,22 @@ static int ca0132_dig_playback_pcm_close(struct hda_pcm_stream *hinfo,
>>  }
>>
>>  /*
>> - * Analog capture
>> + * PCM capture
>>   */
>> +
>> +static unsigned int ca0132_get_capture_latency(struct hda_codec *codec)
>> +{
>> +     struct ca0132_spec *spec = codec->spec;
>> +     unsigned int latency = DSP_CAPTURE_INIT_LATENCY;
>> +
>> +     if (!dspload_is_loaded(codec))
>> +             return 0;
>> +
>> +     if (spec->effects_switch[CRYSTAL_VOICE - EFFECT_START_NID])
>> +             latency += DSP_CRYSTAL_VOICE_LATENCY;
>> +     return latency;
>> +}
>> +
>>  static int ca0132_capture_pcm_prepare(struct hda_pcm_stream *hinfo,
>>                                       struct hda_codec *codec,
>>                                       unsigned int stream_tag,
>> @@ -2843,9 +2892,16 @@ static int ca0132_capture_pcm_prepare(struct hda_pcm_stream *hinfo,
>>                                       struct snd_pcm_substream *substream)
>>  {
>>       struct ca0132_spec *spec = codec->spec;
>> +     struct snd_pcm_runtime *runtime = substream->runtime;
>> +     unsigned int latency = ca0132_get_capture_latency(codec);
>>
>> -     ca0132_setup_stream(codec, spec->adcs[substream->number],
>> -                         stream_tag, 0, format);
>> +     if (spec->dsp_state == DSP_DOWNLOADING)
>> +             return 0;
>> +
>> +     runtime->delay = bytes_to_frames(runtime, (latency * runtime->rate *
>> +                                     runtime->byte_align) / 1000);
>> +
>> +     ca0132_setup_stream(codec, hinfo->nid, stream_tag, 0, format);
>>
>>       return 0;
>>  }
>> @@ -3429,6 +3485,33 @@ static int ca0132_voicefx_set(struct hda_codec *codec, int enable)
>>       return 1;
>>  }
>>
>> +static void ca0132_update_latency(struct hda_codec *codec,
>> +                               unsigned int direction)
>> +{
>> +     struct ca0132_spec *spec = codec->spec;
>> +     unsigned int latency;
>> +     struct snd_pcm_substream *substr;
>> +     struct snd_pcm_runtime *runtime;
>> +
>> +     if (spec->dsp_state == DSP_DOWNLOADING)
>> +             return;
>> +
>> +     latency = (direction == SNDRV_PCM_STREAM_PLAYBACK)
>> +                     ? ca0132_get_playback_latency(codec)
>> +                     : ca0132_get_capture_latency(codec);
>> +
>> +     substr = codec->pcm_info->pcm->streams[direction].substream;
>> +     while (substr) {
>> +             runtime = substr->runtime;
>> +             /* update latency only when frame_bits is set */
>> +             if (runtime && runtime->frame_bits)
>> +                     runtime->delay = bytes_to_frames(runtime,
>> +                                     (latency * runtime->rate *
>> +                                     runtime->byte_align) / 1000);
>> +             substr = substr->next;
>> +     }
>
> I guess this isn't needed.  Usually the pointer callback is performed
> always before the inquiry of runtime->delay.

Thanks for pointing that out Takashi.  This will have no effect, as
azx_get_position now recalculates the delay based on LPIB position.

What was trying to be solved was that there is a large delay in the
codec, and other codecs with integrated DSP.  In this case it is
greater than 100ms, depending on what effects are enabled.  Is there a
good was to add this delay to what is reported?  Maybe allow the codec
to specify a delay that is added to the calculation in
azx_get_position?

>
>
> Takashi
>
>
>> +}
>> +
>>  /*
>>   * Set the effects parameters
>>   */
>> @@ -3468,6 +3551,9 @@ static int ca0132_effects_set(struct hda_codec *codec, hda_nid_t nid, long val)
>>       err = dspio_set_uint_param(codec, ca0132_effects[idx].mid,
>>                                  ca0132_effects[idx].reqs[0], on);
>>
>> +     ca0132_update_latency(codec, SNDRV_PCM_STREAM_PLAYBACK);
>> +     ca0132_update_latency(codec, SNDRV_PCM_STREAM_CAPTURE);
>> +
>>       if (err < 0)
>>               return 0; /* no changed */
>>
>> --
>> 1.7.4.1
>>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 3/4] ALSA: CA0132: Add latency monitoring

[Takashi Iwai]

At Mon, 25 Mar 2013 10:47:31 -0700,
Dylan Reid wrote:
> 
> On Sun, Feb 10, 2013 at 2:51 AM, Takashi Iwai <tiwai@suse.de> wrote:
> > At Fri, 8 Feb 2013 18:31:44 -0800,
> > Ian Minett wrote:
> >>
> >> From: Ian Minett <ian_minett@creativelabs.com>
> >>
> >> Add latency monitoring for playback, capture and DSP effects.
> >
> > Please give a bit more descriptions.
> > Looking at the patch, it doesn't implement only the latency monitoring
> > but actually reflecting to the runtime->delay.
> >
> > Also..
> >
> >
> >> Signed-off-by: Ian Minett <ian_minett@creativelabs.com>
> >>
> >> diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c
> >> index b52f00c..c797f65 100644
> >> --- a/sound/pci/hda/patch_ca0132.c
> >> +++ b/sound/pci/hda/patch_ca0132.c
> >> @@ -133,6 +133,12 @@ enum {
> >>  /* Effects values size*/
> >>  #define EFFECT_VALS_MAX_COUNT 12
> >>
> >> +#define DSP_CAPTURE_INIT_LATENCY        0
> >> +#define DSP_CRYSTAL_VOICE_LATENCY       124
> >> +#define DSP_PLAYBACK_INIT_LATENCY       13
> >> +#define DSP_PLAY_ENHANCEMENT_LATENCY    30
> >> +#define DSP_SPEAKER_OUT_LATENCY         7
> >> +
> >>  struct ct_effect {
> >>       char name[44];
> >>       hda_nid_t nid;
> >> @@ -2761,8 +2767,27 @@ static void ca0132_cleanup_stream(struct hda_codec *codec, hda_nid_t nid)
> >>  }
> >>
> >>  /*
> >> - * PCM callbacks
> >> + * PCM playbacks
> >>   */
> >> +static unsigned int ca0132_get_playback_latency(struct hda_codec *codec)
> >> +{
> >> +     struct ca0132_spec *spec = codec->spec;
> >> +     unsigned int latency = DSP_PLAYBACK_INIT_LATENCY;
> >> +
> >> +     if (!dspload_is_loaded(codec))
> >> +             return 0;
> >> +
> >> +     if (spec->effects_switch[PLAY_ENHANCEMENT - EFFECT_START_NID]) {
> >> +             if ((spec->effects_switch[SURROUND - EFFECT_START_NID]) ||
> >> +                 (spec->effects_switch[DIALOG_PLUS - EFFECT_START_NID]))
> >> +                     latency += DSP_PLAY_ENHANCEMENT_LATENCY;
> >> +     }
> >> +
> >> +     if (spec->cur_out_type == SPEAKER_OUT)
> >> +             latency += DSP_SPEAKER_OUT_LATENCY;
> >> +     return latency;
> >> +}
> >> +
> >>  static int ca0132_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
> >>                       struct hda_codec *codec,
> >>                       unsigned int stream_tag,
> >> @@ -2770,6 +2795,16 @@ static int ca0132_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
> >>                       struct snd_pcm_substream *substream)
> >>  {
> >>       struct ca0132_spec *spec = codec->spec;
> >> +     struct snd_pcm_runtime *runtime = substream->runtime;
> >> +     unsigned int latency = ca0132_get_playback_latency(codec);
> >> +
> >> +     if (spec->dsp_state == DSP_DOWNLOADING) {
> >> +             spec->dsp_stream_id = stream_tag;
> >> +             return 0;
> >> +     }
> >> +
> >> +     runtime->delay = bytes_to_frames(runtime, (latency * runtime->rate *
> >> +                                     runtime->byte_align) / 1000);
> >>
> >>       ca0132_setup_stream(codec, spec->dacs[0], stream_tag, 0, format);
> >>
> >> @@ -2834,8 +2869,22 @@ static int ca0132_dig_playback_pcm_close(struct hda_pcm_stream *hinfo,
> >>  }
> >>
> >>  /*
> >> - * Analog capture
> >> + * PCM capture
> >>   */
> >> +
> >> +static unsigned int ca0132_get_capture_latency(struct hda_codec *codec)
> >> +{
> >> +     struct ca0132_spec *spec = codec->spec;
> >> +     unsigned int latency = DSP_CAPTURE_INIT_LATENCY;
> >> +
> >> +     if (!dspload_is_loaded(codec))
> >> +             return 0;
> >> +
> >> +     if (spec->effects_switch[CRYSTAL_VOICE - EFFECT_START_NID])
> >> +             latency += DSP_CRYSTAL_VOICE_LATENCY;
> >> +     return latency;
> >> +}
> >> +
> >>  static int ca0132_capture_pcm_prepare(struct hda_pcm_stream *hinfo,
> >>                                       struct hda_codec *codec,
> >>                                       unsigned int stream_tag,
> >> @@ -2843,9 +2892,16 @@ static int ca0132_capture_pcm_prepare(struct hda_pcm_stream *hinfo,
> >>                                       struct snd_pcm_substream *substream)
> >>  {
> >>       struct ca0132_spec *spec = codec->spec;
> >> +     struct snd_pcm_runtime *runtime = substream->runtime;
> >> +     unsigned int latency = ca0132_get_capture_latency(codec);
> >>
> >> -     ca0132_setup_stream(codec, spec->adcs[substream->number],
> >> -                         stream_tag, 0, format);
> >> +     if (spec->dsp_state == DSP_DOWNLOADING)
> >> +             return 0;
> >> +
> >> +     runtime->delay = bytes_to_frames(runtime, (latency * runtime->rate *
> >> +                                     runtime->byte_align) / 1000);
> >> +
> >> +     ca0132_setup_stream(codec, hinfo->nid, stream_tag, 0, format);
> >>
> >>       return 0;
> >>  }
> >> @@ -3429,6 +3485,33 @@ static int ca0132_voicefx_set(struct hda_codec *codec, int enable)
> >>       return 1;
> >>  }
> >>
> >> +static void ca0132_update_latency(struct hda_codec *codec,
> >> +                               unsigned int direction)
> >> +{
> >> +     struct ca0132_spec *spec = codec->spec;
> >> +     unsigned int latency;
> >> +     struct snd_pcm_substream *substr;
> >> +     struct snd_pcm_runtime *runtime;
> >> +
> >> +     if (spec->dsp_state == DSP_DOWNLOADING)
> >> +             return;
> >> +
> >> +     latency = (direction == SNDRV_PCM_STREAM_PLAYBACK)
> >> +                     ? ca0132_get_playback_latency(codec)
> >> +                     : ca0132_get_capture_latency(codec);
> >> +
> >> +     substr = codec->pcm_info->pcm->streams[direction].substream;
> >> +     while (substr) {
> >> +             runtime = substr->runtime;
> >> +             /* update latency only when frame_bits is set */
> >> +             if (runtime && runtime->frame_bits)
> >> +                     runtime->delay = bytes_to_frames(runtime,
> >> +                                     (latency * runtime->rate *
> >> +                                     runtime->byte_align) / 1000);
> >> +             substr = substr->next;
> >> +     }
> >
> > I guess this isn't needed.  Usually the pointer callback is performed
> > always before the inquiry of runtime->delay.
> 
> Thanks for pointing that out Takashi.  This will have no effect, as
> azx_get_position now recalculates the delay based on LPIB position.
> 
> What was trying to be solved was that there is a large delay in the
> codec, and other codecs with integrated DSP.  In this case it is
> greater than 100ms, depending on what effects are enabled.  Is there a
> good was to add this delay to what is reported?  Maybe allow the codec
> to specify a delay that is added to the calculation in
> azx_get_position?

I'm fine to add a new codec PCM ops for each azx_get_position call
if it helps.


Takashi

Re: [PATCH 3/4] ALSA: CA0132: Add latency monitoring

[Dylan Reid]

On Tue, Apr 2, 2013 at 2:30 AM, Takashi Iwai <tiwai@suse.de> wrote:
> At Mon, 25 Mar 2013 10:47:31 -0700,
> Dylan Reid wrote:
>>
>> On Sun, Feb 10, 2013 at 2:51 AM, Takashi Iwai <tiwai@suse.de> wrote:
>> > At Fri, 8 Feb 2013 18:31:44 -0800,
>> > Ian Minett wrote:
>> >>
>> >> From: Ian Minett <ian_minett@creativelabs.com>
>> >>
>> >> Add latency monitoring for playback, capture and DSP effects.
>> >
>> > Please give a bit more descriptions.
>> > Looking at the patch, it doesn't implement only the latency monitoring
>> > but actually reflecting to the runtime->delay.
>> >
>> > Also..
>> >
>> >
>> >> Signed-off-by: Ian Minett <ian_minett@creativelabs.com>
>> >>
>> >> diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c
>> >> index b52f00c..c797f65 100644
>> >> --- a/sound/pci/hda/patch_ca0132.c
>> >> +++ b/sound/pci/hda/patch_ca0132.c
>> >> @@ -133,6 +133,12 @@ enum {
>> >>  /* Effects values size*/
>> >>  #define EFFECT_VALS_MAX_COUNT 12
>> >>
>> >> +#define DSP_CAPTURE_INIT_LATENCY        0
>> >> +#define DSP_CRYSTAL_VOICE_LATENCY       124
>> >> +#define DSP_PLAYBACK_INIT_LATENCY       13
>> >> +#define DSP_PLAY_ENHANCEMENT_LATENCY    30
>> >> +#define DSP_SPEAKER_OUT_LATENCY         7
>> >> +
>> >>  struct ct_effect {
>> >>       char name[44];
>> >>       hda_nid_t nid;
>> >> @@ -2761,8 +2767,27 @@ static void ca0132_cleanup_stream(struct hda_codec *codec, hda_nid_t nid)
>> >>  }
>> >>
>> >>  /*
>> >> - * PCM callbacks
>> >> + * PCM playbacks
>> >>   */
>> >> +static unsigned int ca0132_get_playback_latency(struct hda_codec *codec)
>> >> +{
>> >> +     struct ca0132_spec *spec = codec->spec;
>> >> +     unsigned int latency = DSP_PLAYBACK_INIT_LATENCY;
>> >> +
>> >> +     if (!dspload_is_loaded(codec))
>> >> +             return 0;
>> >> +
>> >> +     if (spec->effects_switch[PLAY_ENHANCEMENT - EFFECT_START_NID]) {
>> >> +             if ((spec->effects_switch[SURROUND - EFFECT_START_NID]) ||
>> >> +                 (spec->effects_switch[DIALOG_PLUS - EFFECT_START_NID]))
>> >> +                     latency += DSP_PLAY_ENHANCEMENT_LATENCY;
>> >> +     }
>> >> +
>> >> +     if (spec->cur_out_type == SPEAKER_OUT)
>> >> +             latency += DSP_SPEAKER_OUT_LATENCY;
>> >> +     return latency;
>> >> +}
>> >> +
>> >>  static int ca0132_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
>> >>                       struct hda_codec *codec,
>> >>                       unsigned int stream_tag,
>> >> @@ -2770,6 +2795,16 @@ static int ca0132_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
>> >>                       struct snd_pcm_substream *substream)
>> >>  {
>> >>       struct ca0132_spec *spec = codec->spec;
>> >> +     struct snd_pcm_runtime *runtime = substream->runtime;
>> >> +     unsigned int latency = ca0132_get_playback_latency(codec);
>> >> +
>> >> +     if (spec->dsp_state == DSP_DOWNLOADING) {
>> >> +             spec->dsp_stream_id = stream_tag;
>> >> +             return 0;
>> >> +     }
>> >> +
>> >> +     runtime->delay = bytes_to_frames(runtime, (latency * runtime->rate *
>> >> +                                     runtime->byte_align) / 1000);
>> >>
>> >>       ca0132_setup_stream(codec, spec->dacs[0], stream_tag, 0, format);
>> >>
>> >> @@ -2834,8 +2869,22 @@ static int ca0132_dig_playback_pcm_close(struct hda_pcm_stream *hinfo,
>> >>  }
>> >>
>> >>  /*
>> >> - * Analog capture
>> >> + * PCM capture
>> >>   */
>> >> +
>> >> +static unsigned int ca0132_get_capture_latency(struct hda_codec *codec)
>> >> +{
>> >> +     struct ca0132_spec *spec = codec->spec;
>> >> +     unsigned int latency = DSP_CAPTURE_INIT_LATENCY;
>> >> +
>> >> +     if (!dspload_is_loaded(codec))
>> >> +             return 0;
>> >> +
>> >> +     if (spec->effects_switch[CRYSTAL_VOICE - EFFECT_START_NID])
>> >> +             latency += DSP_CRYSTAL_VOICE_LATENCY;
>> >> +     return latency;
>> >> +}
>> >> +
>> >>  static int ca0132_capture_pcm_prepare(struct hda_pcm_stream *hinfo,
>> >>                                       struct hda_codec *codec,
>> >>                                       unsigned int stream_tag,
>> >> @@ -2843,9 +2892,16 @@ static int ca0132_capture_pcm_prepare(struct hda_pcm_stream *hinfo,
>> >>                                       struct snd_pcm_substream *substream)
>> >>  {
>> >>       struct ca0132_spec *spec = codec->spec;
>> >> +     struct snd_pcm_runtime *runtime = substream->runtime;
>> >> +     unsigned int latency = ca0132_get_capture_latency(codec);
>> >>
>> >> -     ca0132_setup_stream(codec, spec->adcs[substream->number],
>> >> -                         stream_tag, 0, format);
>> >> +     if (spec->dsp_state == DSP_DOWNLOADING)
>> >> +             return 0;
>> >> +
>> >> +     runtime->delay = bytes_to_frames(runtime, (latency * runtime->rate *
>> >> +                                     runtime->byte_align) / 1000);
>> >> +
>> >> +     ca0132_setup_stream(codec, hinfo->nid, stream_tag, 0, format);
>> >>
>> >>       return 0;
>> >>  }
>> >> @@ -3429,6 +3485,33 @@ static int ca0132_voicefx_set(struct hda_codec *codec, int enable)
>> >>       return 1;
>> >>  }
>> >>
>> >> +static void ca0132_update_latency(struct hda_codec *codec,
>> >> +                               unsigned int direction)
>> >> +{
>> >> +     struct ca0132_spec *spec = codec->spec;
>> >> +     unsigned int latency;
>> >> +     struct snd_pcm_substream *substr;
>> >> +     struct snd_pcm_runtime *runtime;
>> >> +
>> >> +     if (spec->dsp_state == DSP_DOWNLOADING)
>> >> +             return;
>> >> +
>> >> +     latency = (direction == SNDRV_PCM_STREAM_PLAYBACK)
>> >> +                     ? ca0132_get_playback_latency(codec)
>> >> +                     : ca0132_get_capture_latency(codec);
>> >> +
>> >> +     substr = codec->pcm_info->pcm->streams[direction].substream;
>> >> +     while (substr) {
>> >> +             runtime = substr->runtime;
>> >> +             /* update latency only when frame_bits is set */
>> >> +             if (runtime && runtime->frame_bits)
>> >> +                     runtime->delay = bytes_to_frames(runtime,
>> >> +                                     (latency * runtime->rate *
>> >> +                                     runtime->byte_align) / 1000);
>> >> +             substr = substr->next;
>> >> +     }
>> >
>> > I guess this isn't needed.  Usually the pointer callback is performed
>> > always before the inquiry of runtime->delay.
>>
>> Thanks for pointing that out Takashi.  This will have no effect, as
>> azx_get_position now recalculates the delay based on LPIB position.
>>
>> What was trying to be solved was that there is a large delay in the
>> codec, and other codecs with integrated DSP.  In this case it is
>> greater than 100ms, depending on what effects are enabled.  Is there a
>> good was to add this delay to what is reported?  Maybe allow the codec
>> to specify a delay that is added to the calculation in
>> azx_get_position?
>
> I'm fine to add a new codec PCM ops for each azx_get_position call
> if it helps.

That sounds like the right idea.  I'll take a stab at it and post
later today.  Thinking something like
"snd_hda_codec_get_processing_delay()" and a corresponding patch_op.
Sound right?

>
>
> Takashi

Re: [PATCH 3/4] ALSA: CA0132: Add latency monitoring

[Takashi Iwai]

At Tue, 2 Apr 2013 13:24:31 -0700,
Dylan Reid wrote:
> 
> On Tue, Apr 2, 2013 at 2:30 AM, Takashi Iwai <tiwai@suse.de> wrote:
> > At Mon, 25 Mar 2013 10:47:31 -0700,
> > Dylan Reid wrote:
> >>
> >> On Sun, Feb 10, 2013 at 2:51 AM, Takashi Iwai <tiwai@suse.de> wrote:
> >> > At Fri, 8 Feb 2013 18:31:44 -0800,
> >> > Ian Minett wrote:
> >> >>
> >> >> From: Ian Minett <ian_minett@creativelabs.com>
> >> >>
> >> >> Add latency monitoring for playback, capture and DSP effects.
> >> >
> >> > Please give a bit more descriptions.
> >> > Looking at the patch, it doesn't implement only the latency monitoring
> >> > but actually reflecting to the runtime->delay.
> >> >
> >> > Also..
> >> >
> >> >
> >> >> Signed-off-by: Ian Minett <ian_minett@creativelabs.com>
> >> >>
> >> >> diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c
> >> >> index b52f00c..c797f65 100644
> >> >> --- a/sound/pci/hda/patch_ca0132.c
> >> >> +++ b/sound/pci/hda/patch_ca0132.c
> >> >> @@ -133,6 +133,12 @@ enum {
> >> >>  /* Effects values size*/
> >> >>  #define EFFECT_VALS_MAX_COUNT 12
> >> >>
> >> >> +#define DSP_CAPTURE_INIT_LATENCY        0
> >> >> +#define DSP_CRYSTAL_VOICE_LATENCY       124
> >> >> +#define DSP_PLAYBACK_INIT_LATENCY       13
> >> >> +#define DSP_PLAY_ENHANCEMENT_LATENCY    30
> >> >> +#define DSP_SPEAKER_OUT_LATENCY         7
> >> >> +
> >> >>  struct ct_effect {
> >> >>       char name[44];
> >> >>       hda_nid_t nid;
> >> >> @@ -2761,8 +2767,27 @@ static void ca0132_cleanup_stream(struct hda_codec *codec, hda_nid_t nid)
> >> >>  }
> >> >>
> >> >>  /*
> >> >> - * PCM callbacks
> >> >> + * PCM playbacks
> >> >>   */
> >> >> +static unsigned int ca0132_get_playback_latency(struct hda_codec *codec)
> >> >> +{
> >> >> +     struct ca0132_spec *spec = codec->spec;
> >> >> +     unsigned int latency = DSP_PLAYBACK_INIT_LATENCY;
> >> >> +
> >> >> +     if (!dspload_is_loaded(codec))
> >> >> +             return 0;
> >> >> +
> >> >> +     if (spec->effects_switch[PLAY_ENHANCEMENT - EFFECT_START_NID]) {
> >> >> +             if ((spec->effects_switch[SURROUND - EFFECT_START_NID]) ||
> >> >> +                 (spec->effects_switch[DIALOG_PLUS - EFFECT_START_NID]))
> >> >> +                     latency += DSP_PLAY_ENHANCEMENT_LATENCY;
> >> >> +     }
> >> >> +
> >> >> +     if (spec->cur_out_type == SPEAKER_OUT)
> >> >> +             latency += DSP_SPEAKER_OUT_LATENCY;
> >> >> +     return latency;
> >> >> +}
> >> >> +
> >> >>  static int ca0132_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
> >> >>                       struct hda_codec *codec,
> >> >>                       unsigned int stream_tag,
> >> >> @@ -2770,6 +2795,16 @@ static int ca0132_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
> >> >>                       struct snd_pcm_substream *substream)
> >> >>  {
> >> >>       struct ca0132_spec *spec = codec->spec;
> >> >> +     struct snd_pcm_runtime *runtime = substream->runtime;
> >> >> +     unsigned int latency = ca0132_get_playback_latency(codec);
> >> >> +
> >> >> +     if (spec->dsp_state == DSP_DOWNLOADING) {
> >> >> +             spec->dsp_stream_id = stream_tag;
> >> >> +             return 0;
> >> >> +     }
> >> >> +
> >> >> +     runtime->delay = bytes_to_frames(runtime, (latency * runtime->rate *
> >> >> +                                     runtime->byte_align) / 1000);
> >> >>
> >> >>       ca0132_setup_stream(codec, spec->dacs[0], stream_tag, 0, format);
> >> >>
> >> >> @@ -2834,8 +2869,22 @@ static int ca0132_dig_playback_pcm_close(struct hda_pcm_stream *hinfo,
> >> >>  }
> >> >>
> >> >>  /*
> >> >> - * Analog capture
> >> >> + * PCM capture
> >> >>   */
> >> >> +
> >> >> +static unsigned int ca0132_get_capture_latency(struct hda_codec *codec)
> >> >> +{
> >> >> +     struct ca0132_spec *spec = codec->spec;
> >> >> +     unsigned int latency = DSP_CAPTURE_INIT_LATENCY;
> >> >> +
> >> >> +     if (!dspload_is_loaded(codec))
> >> >> +             return 0;
> >> >> +
> >> >> +     if (spec->effects_switch[CRYSTAL_VOICE - EFFECT_START_NID])
> >> >> +             latency += DSP_CRYSTAL_VOICE_LATENCY;
> >> >> +     return latency;
> >> >> +}
> >> >> +
> >> >>  static int ca0132_capture_pcm_prepare(struct hda_pcm_stream *hinfo,
> >> >>                                       struct hda_codec *codec,
> >> >>                                       unsigned int stream_tag,
> >> >> @@ -2843,9 +2892,16 @@ static int ca0132_capture_pcm_prepare(struct hda_pcm_stream *hinfo,
> >> >>                                       struct snd_pcm_substream *substream)
> >> >>  {
> >> >>       struct ca0132_spec *spec = codec->spec;
> >> >> +     struct snd_pcm_runtime *runtime = substream->runtime;
> >> >> +     unsigned int latency = ca0132_get_capture_latency(codec);
> >> >>
> >> >> -     ca0132_setup_stream(codec, spec->adcs[substream->number],
> >> >> -                         stream_tag, 0, format);
> >> >> +     if (spec->dsp_state == DSP_DOWNLOADING)
> >> >> +             return 0;
> >> >> +
> >> >> +     runtime->delay = bytes_to_frames(runtime, (latency * runtime->rate *
> >> >> +                                     runtime->byte_align) / 1000);
> >> >> +
> >> >> +     ca0132_setup_stream(codec, hinfo->nid, stream_tag, 0, format);
> >> >>
> >> >>       return 0;
> >> >>  }
> >> >> @@ -3429,6 +3485,33 @@ static int ca0132_voicefx_set(struct hda_codec *codec, int enable)
> >> >>       return 1;
> >> >>  }
> >> >>
> >> >> +static void ca0132_update_latency(struct hda_codec *codec,
> >> >> +                               unsigned int direction)
> >> >> +{
> >> >> +     struct ca0132_spec *spec = codec->spec;
> >> >> +     unsigned int latency;
> >> >> +     struct snd_pcm_substream *substr;
> >> >> +     struct snd_pcm_runtime *runtime;
> >> >> +
> >> >> +     if (spec->dsp_state == DSP_DOWNLOADING)
> >> >> +             return;
> >> >> +
> >> >> +     latency = (direction == SNDRV_PCM_STREAM_PLAYBACK)
> >> >> +                     ? ca0132_get_playback_latency(codec)
> >> >> +                     : ca0132_get_capture_latency(codec);
> >> >> +
> >> >> +     substr = codec->pcm_info->pcm->streams[direction].substream;
> >> >> +     while (substr) {
> >> >> +             runtime = substr->runtime;
> >> >> +             /* update latency only when frame_bits is set */
> >> >> +             if (runtime && runtime->frame_bits)
> >> >> +                     runtime->delay = bytes_to_frames(runtime,
> >> >> +                                     (latency * runtime->rate *
> >> >> +                                     runtime->byte_align) / 1000);
> >> >> +             substr = substr->next;
> >> >> +     }
> >> >
> >> > I guess this isn't needed.  Usually the pointer callback is performed
> >> > always before the inquiry of runtime->delay.
> >>
> >> Thanks for pointing that out Takashi.  This will have no effect, as
> >> azx_get_position now recalculates the delay based on LPIB position.
> >>
> >> What was trying to be solved was that there is a large delay in the
> >> codec, and other codecs with integrated DSP.  In this case it is
> >> greater than 100ms, depending on what effects are enabled.  Is there a
> >> good was to add this delay to what is reported?  Maybe allow the codec
> >> to specify a delay that is added to the calculation in
> >> azx_get_position?
> >
> > I'm fine to add a new codec PCM ops for each azx_get_position call
> > if it helps.
> 
> That sounds like the right idea.  I'll take a stab at it and post
> later today.  Thinking something like
> "snd_hda_codec_get_processing_delay()" and a corresponding patch_op.
> Sound right?

Yes.


Takashi