[alsa-devel] Driver code with mpc5200 pointer problem.
Here's the code computing the mpc5200 dma pointer. Could you please take a look at it and let me know what it is doing wrong.
/* * Freescale MPC5200 Audio DMA * ALSA SoC Platform driver * * Copyright (C) 2008 Secret Lab Technologies Ltd. */
#define DEBUG
#include <linux/init.h> #include <linux/module.h> #include <linux/interrupt.h> #include <linux/device.h> #include <linux/delay.h> #include <linux/of_device.h> #include <linux/of_platform.h> #include <linux/dma-mapping.h>
#include <sound/core.h> #include <sound/pcm.h> #include <sound/pcm_params.h> #include <sound/initval.h> #include <sound/soc.h> #include <sound/soc-of-simple.h>
#include <sysdev/bestcomm/bestcomm.h> #include <sysdev/bestcomm/gen_bd.h> #include <asm/time.h> #include <asm/mpc52xx.h> #include <asm/mpc52xx_psc.h>
#include "mpc5200_dma.h"
MODULE_AUTHOR("Grant Likely grant.likely@secretlab.ca"); MODULE_DESCRIPTION("Freescale MPC5200 PSC in DMA mode ASoC Driver"); MODULE_LICENSE("GPL");
/* * Interrupt handlers */ static irqreturn_t psc_dma_status_irq(int irq, void *_psc_dma) { struct psc_dma *psc_dma = _psc_dma; struct mpc52xx_psc __iomem *regs = psc_dma->psc_regs; u16 isr;
isr = in_be16(®s->mpc52xx_psc_isr);
/* Playback underrun error */ if (psc_dma->playback.active && (isr & MPC52xx_PSC_IMR_TXEMP)) psc_dma->stats.underrun_count++;
/* Capture overrun error */ if (psc_dma->capture.active && (isr & MPC52xx_PSC_IMR_ORERR)) psc_dma->stats.overrun_count++;
out_8(®s->command, 4 << 4); /* reset the error status */
return IRQ_HANDLED; }
/** * psc_dma_bcom_enqueue_next_buffer - Enqueue another audio buffer * @s: pointer to stream private data structure * * Enqueues another audio period buffer into the bestcomm queue. * * Note: The routine must only be called when there is space available in * the queue. Otherwise the enqueue will fail and the audio ring buffer * will get out of sync */ static void psc_dma_bcom_enqueue_next_buffer(struct psc_dma_stream *s) { struct bcom_bd *bd;
/* Prepare and enqueue the next buffer descriptor */ bd = bcom_prepare_next_buffer(s->bcom_task); bd->status = s->period_bytes; bd->data[0] = s->period_next_pt; bcom_submit_next_buffer(s->bcom_task, NULL);
/* Update for next period */ s->period_next_pt += s->period_bytes; if (s->period_next_pt >= s->period_end) s->period_next_pt = s->period_start; }
/* Bestcomm DMA irq handler */ static irqreturn_t psc_dma_bcom_irq(int irq, void *_psc_dma_stream) { struct psc_dma_stream *s = _psc_dma_stream;
/* For each finished period, dequeue the completed period buffer * and enqueue a new one in it's place. */ while (bcom_buffer_done(s->bcom_task)) { bcom_retrieve_buffer(s->bcom_task, NULL, NULL); s->period_current_pt += s->period_bytes; if (s->period_current_pt >= s->period_end) s->period_current_pt = s->period_start; psc_dma_bcom_enqueue_next_buffer(s); bcom_enable(s->bcom_task); }
/* If the stream is active, then also inform the PCM middle layer * of the period finished event. */ if (s->active) { s->jiffies = jiffies; snd_pcm_period_elapsed(s->stream); }
return IRQ_HANDLED; }
/** * psc_dma_startup: create a new substream * * This is the first function called when a stream is opened. * * If this is the first stream open, then grab the IRQ and program most of * the PSC registers. */ int mpc5200_dma_startup(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { struct snd_soc_pcm_runtime *rtd = substream->private_data; struct psc_dma *psc_dma = rtd->dai->cpu_dai->private_data; int rc;
dev_dbg(psc_dma->dev, "psc_dma_startup(substream=%p)\n", substream);
if (!psc_dma->playback.active && !psc_dma->capture.active) { /* Setup the IRQs */ rc = request_irq(psc_dma->irq, &psc_dma_status_irq, IRQF_SHARED, "psc-dma-status", psc_dma); rc |= request_irq(psc_dma->capture.irq, &psc_dma_bcom_irq, IRQF_SHARED, "psc-dma-capture", &psc_dma->capture); rc |= request_irq(psc_dma->playback.irq, &psc_dma_bcom_irq, IRQF_SHARED, "psc-dma-playback", &psc_dma->playback); if (rc) { free_irq(psc_dma->irq, psc_dma); free_irq(psc_dma->capture.irq, &psc_dma->capture); free_irq(psc_dma->playback.irq, &psc_dma->playback); return -ENODEV; } }
return 0; } EXPORT_SYMBOL_GPL(mpc5200_dma_startup);
int mpc5200_dma_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { snd_pcm_set_runtime_buffer(substream, NULL); return 0; } EXPORT_SYMBOL_GPL(mpc5200_dma_hw_free);
/** * psc_dma_trigger: start and stop the DMA transfer. * * This function is called by ALSA to start, stop, pause, and resume the DMA * transfer of data. */ int mpc5200_dma_trigger(struct snd_pcm_substream *substream, int cmd, struct snd_soc_dai *dai) { struct snd_soc_pcm_runtime *rtd = substream->private_data; struct psc_dma *psc_dma = rtd->dai->cpu_dai->private_data; struct snd_pcm_runtime *runtime = substream->runtime; struct psc_dma_stream *s; struct mpc52xx_psc __iomem *regs = psc_dma->psc_regs; u16 imr; u8 psc_cmd; unsigned long flags;
if (substream->pstr->stream == SNDRV_PCM_STREAM_CAPTURE) s = &psc_dma->capture; else s = &psc_dma->playback;
dev_dbg(psc_dma->dev, "psc_dma_trigger(substream=%p, cmd=%i)" " stream_id=%i\n", substream, cmd, substream->pstr->stream);
switch (cmd) { case SNDRV_PCM_TRIGGER_START: s->period_bytes = frames_to_bytes(runtime, runtime->period_size); s->period_start = virt_to_phys(runtime->dma_area); s->period_end = s->period_start + (s->period_bytes * runtime->periods); s->period_next_pt = s->period_start; s->period_current_pt = s->period_start; s->active = 1;
/* First; reset everything */ if (substream->pstr->stream == SNDRV_PCM_STREAM_CAPTURE) { out_8(®s->command, MPC52xx_PSC_RST_RX); out_8(®s->command, MPC52xx_PSC_RST_ERR_STAT); } else { out_8(®s->command, MPC52xx_PSC_RST_TX); out_8(®s->command, MPC52xx_PSC_RST_ERR_STAT); }
/* Next, fill up the bestcomm bd queue and enable DMA. * This will begin filling the PSC's fifo. */ if (substream->pstr->stream == SNDRV_PCM_STREAM_CAPTURE) bcom_gen_bd_rx_reset(s->bcom_task); else bcom_gen_bd_tx_reset(s->bcom_task); while (!bcom_queue_full(s->bcom_task)) psc_dma_bcom_enqueue_next_buffer(s); bcom_enable(s->bcom_task);
/* Due to errata in the dma mode; need to line up enabling * the transmitter with a transition on the frame sync * line */
spin_lock_irqsave(&psc_dma->lock, flags); /* first make sure it is low */ while ((in_8(®s->ipcr_acr.ipcr) & 0x80) != 0) ; /* then wait for the transition to high */ while ((in_8(®s->ipcr_acr.ipcr) & 0x80) == 0) ; /* Finally, enable the PSC. * Receiver must always be enabled; even when we only want * transmit. (see 15.3.2.3 of MPC5200B User's Guide) */ psc_cmd = MPC52xx_PSC_RX_ENABLE; if (substream->pstr->stream == SNDRV_PCM_STREAM_PLAYBACK) psc_cmd |= MPC52xx_PSC_TX_ENABLE; out_8(®s->command, psc_cmd); spin_unlock_irqrestore(&psc_dma->lock, flags);
break;
case SNDRV_PCM_TRIGGER_STOP: /* Turn off the PSC */ s->active = 0; if (substream->pstr->stream == SNDRV_PCM_STREAM_CAPTURE) { if (!psc_dma->playback.active) { out_8(®s->command, 2 << 4); /* reset rx */ out_8(®s->command, 3 << 4); /* reset tx */ out_8(®s->command, 4 << 4); /* reset err */ } } else { out_8(®s->command, 3 << 4); /* reset tx */ out_8(®s->command, 4 << 4); /* reset err */ if (!psc_dma->capture.active) out_8(®s->command, 2 << 4); /* reset rx */ }
bcom_disable(s->bcom_task); while (!bcom_queue_empty(s->bcom_task)) bcom_retrieve_buffer(s->bcom_task, NULL, NULL);
break;
default: dev_dbg(psc_dma->dev, "invalid command\n"); return -EINVAL; }
/* Update interrupt enable settings */ imr = 0; if (psc_dma->playback.active) imr |= MPC52xx_PSC_IMR_TXEMP; if (psc_dma->capture.active) imr |= MPC52xx_PSC_IMR_ORERR; out_be16(®s->isr_imr.imr, imr);
return 0; } EXPORT_SYMBOL_GPL(mpc5200_dma_trigger);
/** * psc_dma_shutdown: shutdown the data transfer on a stream * * Shutdown the PSC if there are no other substreams open. */ void mpc5200_dma_shutdown(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { struct snd_soc_pcm_runtime *rtd = substream->private_data; struct psc_dma *psc_dma = rtd->dai->cpu_dai->private_data;
dev_dbg(psc_dma->dev, "psc_dma_shutdown(substream=%p)\n", substream);
/* * If this is the last active substream, disable the PSC and release * the IRQ. */ if (!psc_dma->playback.active && !psc_dma->capture.active) {
/* Disable all interrupts and reset the PSC */ out_be16(&psc_dma->psc_regs->isr_imr.imr, 0); out_8(&psc_dma->psc_regs->command, 3 << 4); /* reset tx */ out_8(&psc_dma->psc_regs->command, 2 << 4); /* reset rx */ out_8(&psc_dma->psc_regs->command, 1 << 4); /* reset mode */ out_8(&psc_dma->psc_regs->command, 4 << 4); /* reset error */
/* Release irqs */ free_irq(psc_dma->irq, psc_dma); free_irq(psc_dma->capture.irq, &psc_dma->capture); free_irq(psc_dma->playback.irq, &psc_dma->playback); } } EXPORT_SYMBOL_GPL(mpc5200_dma_shutdown);
/* --------------------------------------------------------------------- * The PSC DMA 'ASoC platform' driver * * Can be referenced by an 'ASoC machine' driver * This driver only deals with the audio bus; it doesn't have any * interaction with the attached codec */
static const struct snd_pcm_hardware psc_dma_pcm_hardware = { .info = SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID | SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER, .formats = SNDRV_PCM_FMTBIT_S8 | SNDRV_PCM_FMTBIT_S16_BE | SNDRV_PCM_FMTBIT_S24_BE | SNDRV_PCM_FMTBIT_S32_BE, .rate_min = 8000, .rate_max = 48000, .channels_min = 1, .channels_max = 2, .period_bytes_max = 1024 * 1024, .period_bytes_min = 32, .periods_min = 2, .periods_max = 256, .buffer_bytes_max = 2 * 1024 * 1024, .fifo_size = 0, };
static int psc_dma_pcm_open(struct snd_pcm_substream *substream) { struct snd_soc_pcm_runtime *rtd = substream->private_data; struct psc_dma *psc_dma = rtd->dai->cpu_dai->private_data; struct psc_dma_stream *s;
dev_dbg(psc_dma->dev, "psc_dma_pcm_open(substream=%p)\n", substream);
if (substream->pstr->stream == SNDRV_PCM_STREAM_CAPTURE) s = &psc_dma->capture; else s = &psc_dma->playback;
snd_soc_set_runtime_hwparams(substream, &psc_dma_pcm_hardware);
s->stream = substream; return 0; }
static int psc_dma_pcm_close(struct snd_pcm_substream *substream) { struct snd_soc_pcm_runtime *rtd = substream->private_data; struct psc_dma *psc_dma = rtd->dai->cpu_dai->private_data; struct psc_dma_stream *s;
dev_dbg(psc_dma->dev, "psc_dma_pcm_close(substream=%p)\n", substream);
if (substream->pstr->stream == SNDRV_PCM_STREAM_CAPTURE) s = &psc_dma->capture; else s = &psc_dma->playback;
s->stream = NULL; return 0; }
static snd_pcm_uframes_t psc_dma_pcm_pointer(struct snd_pcm_substream *substream) { struct snd_pcm_runtime *runtime = substream->runtime; struct snd_soc_pcm_runtime *rtd = substream->private_data; struct psc_dma *psc_dma = rtd->dai->cpu_dai->private_data; struct psc_dma_stream *s; dma_addr_t count; snd_pcm_uframes_t frames; int delta;
if (substream->pstr->stream == SNDRV_PCM_STREAM_CAPTURE) s = &psc_dma->capture; else s = &psc_dma->playback;
count = s->period_current_pt - s->period_start;
delta = jiffies - s->jiffies; delta = delta * runtime->rate / HZ; frames = bytes_to_frames(substream->runtime, count);
return frames + delta; }
static struct snd_pcm_ops psc_dma_pcm_ops = { .open = psc_dma_pcm_open, .close = psc_dma_pcm_close, .ioctl = snd_pcm_lib_ioctl, .pointer = psc_dma_pcm_pointer, };
static u64 psc_dma_pcm_dmamask = 0xffffffff; static int psc_dma_pcm_new(struct snd_card *card, struct snd_soc_dai *dai, struct snd_pcm *pcm) { struct snd_soc_pcm_runtime *rtd = pcm->private_data; struct psc_dma *psc_dma = rtd->dai->cpu_dai->private_data; size_t size = psc_dma_pcm_hardware.buffer_bytes_max; int rc = 0;
dev_dbg(rtd->socdev->dev, "psc_dma_pcm_new(card=%p, dai=%p, pcm=%p)\n", card, dai, pcm);
if (!card->dev->dma_mask) card->dev->dma_mask = &psc_dma_pcm_dmamask; if (!card->dev->coherent_dma_mask) card->dev->coherent_dma_mask = 0xffffffff;
if (pcm->streams[0].substream) { rc = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, pcm->card->dev, size, &pcm->streams[0].substream->dma_buffer); if (rc) goto playback_alloc_err; }
if (pcm->streams[1].substream) { rc = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, pcm->card->dev, size, &pcm->streams[1].substream->dma_buffer); if (rc) goto capture_alloc_err; }
if (rtd->socdev->card->codec->ac97) rtd->socdev->card->codec->ac97->private_data = psc_dma;
return 0;
capture_alloc_err: if (pcm->streams[0].substream) snd_dma_free_pages(&pcm->streams[0].substream->dma_buffer); playback_alloc_err: dev_err(card->dev, "Cannot allocate buffer(s)\n"); return -ENOMEM; }
static void psc_dma_pcm_free(struct snd_pcm *pcm) { struct snd_soc_pcm_runtime *rtd = pcm->private_data; struct snd_pcm_substream *substream; int stream;
dev_dbg(rtd->socdev->dev, "psc_dma_pcm_free(pcm=%p)\n", pcm);
for (stream = 0; stream < 2; stream++) { substream = pcm->streams[stream].substream; if (substream) { snd_dma_free_pages(&substream->dma_buffer); substream->dma_buffer.area = NULL; substream->dma_buffer.addr = 0; } } }
struct snd_soc_platform mpc5200_dma_platform = { .name = "mpc5200-psc-audio", .pcm_ops = &psc_dma_pcm_ops, .pcm_new = &psc_dma_pcm_new, .pcm_free = &psc_dma_pcm_free, }; EXPORT_SYMBOL_GPL(mpc5200_dma_platform);
static int __init mpc5200_soc_platform_init(void) { /* Tell the ASoC OF helpers about it */ of_snd_soc_register_platform(&mpc5200_dma_platform); return snd_soc_register_platform(&mpc5200_dma_platform); } module_init(mpc5200_soc_platform_init);
static void __exit mpc5200_soc_platform_exit(void) { snd_soc_unregister_platform(&mpc5200_dma_platform); } module_exit(mpc5200_soc_platform_exit);
-------------------------------------------------------------------------------
/* * linux/sound/mpc5200-ac97.c -- AC97 support for the Freescale MPC52xx chip. * * Copyright 2008 Jon Smirl, Digispeaker * Author: Jon Smirl jonsmirl@gmail.com * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2 as * published by the Free Software Foundation. */
#define DEBUG
#include <linux/init.h> #include <linux/module.h> #include <linux/interrupt.h> #include <linux/device.h> #include <linux/delay.h> #include <linux/of_device.h> #include <linux/of_platform.h> #include <linux/dma-mapping.h>
#include <sound/core.h> #include <sound/pcm.h> #include <sound/pcm_params.h> #include <sound/initval.h> #include <sound/soc.h> #include <sound/soc-of-simple.h>
#include <sysdev/bestcomm/bestcomm.h> #include <sysdev/bestcomm/gen_bd.h> #include <asm/time.h> #include <asm/mpc52xx.h> #include <asm/mpc52xx_psc.h>
#include "mpc5200_dma.h" #include "mpc5200_psc_ac97.h"
MODULE_AUTHOR("Jon Smirl jonsmirl@gmail.com"); MODULE_DESCRIPTION("mpc5200 AC97 module"); MODULE_LICENSE("GPL");
#define DRV_NAME "mpc5200-psc-ac97"
static unsigned short psc_ac97_read(struct snd_ac97 *ac97, unsigned short reg) { struct psc_dma *psc_dma = ac97->private_data; int timeout; unsigned int val;
spin_lock(&psc_dma->lock); //printk("ac97 read: reg %04x\n", reg);
/* Wait for it to be ready */ timeout = 1000; while ((--timeout) && (in_be16(&psc_dma->psc_regs->sr_csr.status) & MPC52xx_PSC_SR_CMDSEND) ) udelay(10);
if (!timeout) { printk(KERN_ERR DRV_NAME ": timeout on ac97 bus (rdy)\n"); return 0xffff; }
/* Do the read */ out_be32(&psc_dma->psc_regs->ac97_cmd, (1<<31) | ((reg & 0x7f) << 24));
/* Wait for the answer */ timeout = 1000; while ((--timeout) && !(in_be16(&psc_dma->psc_regs->sr_csr.status) & MPC52xx_PSC_SR_DATA_VAL) ) udelay(10);
if (!timeout) { printk(KERN_ERR DRV_NAME ": timeout on ac97 read (val) %x\n", in_be16(&psc_dma->psc_regs->sr_csr.status)); return 0xffff; }
/* Get the data */ val = in_be32(&psc_dma->psc_regs->ac97_data); if ( ((val>>24) & 0x7f) != reg ) { printk(KERN_ERR DRV_NAME ": reg echo error on ac97 read\n"); return 0xffff; } val = (val >> 8) & 0xffff;
//printk("ac97 read ok: reg %04x val %04x\n", reg, val);
spin_unlock(&psc_dma->lock); return (unsigned short) val; }
static void psc_ac97_write(struct snd_ac97 *ac97, unsigned short reg, unsigned short val) { struct psc_dma *psc_dma = ac97->private_data; int timeout;
//printk("ac97 write: reg %04x val %04x\n", reg, val); spin_lock(&psc_dma->lock);
/* Wait for it to be ready */ timeout = 1000; while ((--timeout) && (in_be16(&psc_dma->psc_regs->sr_csr.status) & MPC52xx_PSC_SR_CMDSEND) ) udelay(10);
if (!timeout) { printk(KERN_ERR DRV_NAME ": timeout on ac97 write\n"); return; }
/* Write data */ out_be32(&psc_dma->psc_regs->ac97_cmd, ((reg & 0x7f) << 24) | (val << 8));
spin_unlock(&psc_dma->lock); }
static void psc_ac97_cold_reset(struct snd_ac97 *ac97) { struct psc_dma *psc_dma = ac97->private_data;
printk("psc_ac97_cold_reset %p\n", ac97);
/* Do a cold reset */ out_8(&psc_dma->psc_regs->op1, MPC52xx_PSC_OP_RES); udelay(10); out_8(&psc_dma->psc_regs->op0, MPC52xx_PSC_OP_RES); udelay(50);
/* PSC recover from cold reset (cfr user manual, not sure if useful) */ out_be32(&psc_dma->psc_regs->sicr, in_be32(&psc_dma->psc_regs->sicr)); }
static void psc_ac97_warm_reset(struct snd_ac97 *ac97) { printk("psc_ac97_warm_reset\n"); }
struct snd_ac97_bus_ops soc_ac97_ops = { .read = psc_ac97_read, .write = psc_ac97_write, .reset = psc_ac97_cold_reset, .warm_reset = psc_ac97_warm_reset, }; EXPORT_SYMBOL_GPL(soc_ac97_ops);
#ifdef CONFIG_PM static int psc_ac97_suspend(struct snd_soc_dai *dai) { return 0; }
static int psc_ac97_resume(struct snd_soc_dai *dai) { return 0; }
#else #define psc_ac97_suspend NULL #define psc_ac97_resume NULL #endif
static int psc_ac97_hw_analog_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params, struct snd_soc_dai *dai) { struct snd_soc_pcm_runtime *rtd = substream->private_data; struct psc_dma *psc_dma = rtd->dai->cpu_dai->private_data;
dev_dbg(psc_dma->dev, "%s(substream=%p) p_size=%i p_bytes=%i" " periods=%i buffer_size=%i buffer_bytes=%i channels=%i" " rate=%i format=%i\n", __func__, substream, params_period_size(params), params_period_bytes(params), params_periods(params), params_buffer_size(params), params_buffer_bytes(params), params_channels(params), params_rate(params), params_format(params));
// FIXME, need a spinlock to protect access if (params_channels(params) == 1) out_be32(&psc_dma->psc_regs->ac97_slots, 0x01000000); else out_be32(&psc_dma->psc_regs->ac97_slots, 0x03000000);
snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer);
return 0; }
static int psc_ac97_hw_digital_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params, struct snd_soc_dai *dai) { return 0; }
/** * psc_ac97_set_fmt: set the serial format. * * This function is called by the machine driver to tell us what serial * format to use. * * This driver only supports AC97 mode. Return an error if the format is * not SND_SOC_DAIFMT_AC97. * * @format: one of SND_SOC_DAIFMT_xxx */ static int psc_ac97_set_fmt(struct snd_soc_dai *cpu_dai, unsigned int format) { struct psc_dma *psc_dma = cpu_dai->private_data; dev_dbg(psc_dma->dev, "psc_ac97_set_fmt(cpu_dai=%p, format=%i)\n", cpu_dai, format); return (format == SND_SOC_DAIFMT_AC97) ? 0 : -EINVAL; }
/* --------------------------------------------------------------------- * ALSA SoC Bindings * * - Digital Audio Interface (DAI) template * - create/destroy dai hooks */
/** * psc_ac97_dai_template: template CPU Digital Audio Interface */ static struct snd_soc_dai_ops psc_ac97_analog_ops = { .startup = mpc5200_dma_startup, .hw_params = psc_ac97_hw_analog_params, .hw_free = mpc5200_dma_hw_free, .shutdown = mpc5200_dma_shutdown, .trigger = mpc5200_dma_trigger, .set_fmt = psc_ac97_set_fmt, };
static struct snd_soc_dai_ops psc_ac97_digital_ops = { .startup = mpc5200_dma_startup, .hw_params = psc_ac97_hw_digital_params, .hw_free = mpc5200_dma_hw_free, .shutdown = mpc5200_dma_shutdown, .trigger = mpc5200_dma_trigger, .set_fmt = psc_ac97_set_fmt, };
static struct snd_soc_dai psc_ac97_dai_template[] = { { .name = "%s analog", .suspend = psc_ac97_suspend, .resume = psc_ac97_resume, .playback = { .channels_min = 1, .channels_max = 6, .rates = SNDRV_PCM_RATE_8000_48000, .formats = SNDRV_PCM_FMTBIT_S32_BE, }, .capture = { .channels_min = 1, .channels_max = 2, .rates = SNDRV_PCM_RATE_8000_48000, .formats = SNDRV_PCM_FMTBIT_S32_BE, }, .ops = &psc_ac97_analog_ops, }, { .name = "%s digital", .playback = { .channels_min = 1, .channels_max = 2, .rates = SNDRV_PCM_RATE_32000 | \ SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000, .formats = SNDRV_PCM_FORMAT_IEC958_SUBFRAME_BE, }, .ops = &psc_ac97_digital_ops, }};
/* --------------------------------------------------------------------- * Sysfs attributes for debugging */
static ssize_t psc_ac97_status_show(struct device *dev, struct device_attribute *attr, char *buf) { struct psc_dma *psc_dma = dev_get_drvdata(dev);
return sprintf(buf, "status=%.4x sicr=%.8x rfnum=%i rfstat=0x%.4x " "tfnum=%i tfstat=0x%.4x\n", in_be16(&psc_dma->psc_regs->sr_csr.status), in_be32(&psc_dma->psc_regs->sicr), in_be16(&psc_dma->fifo_regs->rfnum) & 0x1ff, in_be16(&psc_dma->fifo_regs->rfstat), in_be16(&psc_dma->fifo_regs->tfnum) & 0x1ff, in_be16(&psc_dma->fifo_regs->tfstat)); }
static int *psc_ac97_get_stat_attr(struct psc_dma *psc_dma, const char *name) { if (strcmp(name, "playback_underrun") == 0) return &psc_dma->stats.underrun_count; if (strcmp(name, "capture_overrun") == 0) return &psc_dma->stats.overrun_count;
return NULL; }
static ssize_t psc_ac97_stat_show(struct device *dev, struct device_attribute *attr, char *buf) { struct psc_dma *psc_dma = dev_get_drvdata(dev); int *attrib;
attrib = psc_ac97_get_stat_attr(psc_dma, attr->attr.name); if (!attrib) return 0;
return sprintf(buf, "%i\n", *attrib); }
static ssize_t psc_ac97_stat_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { struct psc_dma *psc_dma = dev_get_drvdata(dev); int *attrib;
attrib = psc_ac97_get_stat_attr(psc_dma, attr->attr.name); if (!attrib) return 0;
*attrib = simple_strtoul(buf, NULL, 0); return count; }
static DEVICE_ATTR(status, 0644, psc_ac97_status_show, NULL); static DEVICE_ATTR(playback_underrun, 0644, psc_ac97_stat_show, psc_ac97_stat_store); static DEVICE_ATTR(capture_overrun, 0644, psc_ac97_stat_show, psc_ac97_stat_store);
/* --------------------------------------------------------------------- * OF platform bus binding code: * - Probe/remove operations * - OF device match table */ static int __devinit psc_ac97_of_probe(struct of_device *op, const struct of_device_id *match) { phys_addr_t fifo; struct psc_dma *psc_dma; struct resource res; int i, nDAI, size, psc_id, irq, rc; const __be32 *prop; void __iomem *regs;
/* Get the PSC ID */ prop = of_get_property(op->node, "cell-index", &size); if (!prop || size < sizeof *prop) return -ENODEV; psc_id = be32_to_cpu(*prop);
/* Fetch the registers and IRQ of the PSC */ irq = irq_of_parse_and_map(op->node, 0); if (of_address_to_resource(op->node, 0, &res)) { dev_err(&op->dev, "Missing reg property\n"); return -ENODEV; } regs = ioremap(res.start, 1 + res.end - res.start); if (!regs) { dev_err(&op->dev, "Could not map registers\n"); return -ENODEV; }
/* Allocate and initialize the driver private data */ psc_dma = kzalloc(sizeof *psc_dma, GFP_KERNEL); if (!psc_dma) { iounmap(regs); return -ENOMEM; } printk("psc_dma %p\n", psc_dma);
spin_lock_init(&psc_dma->lock); psc_dma->irq = irq; psc_dma->psc_regs = regs; psc_dma->fifo_regs = regs + sizeof *psc_dma->psc_regs; psc_dma->dev = &op->dev; psc_dma->playback.psc_dma = psc_dma; psc_dma->capture.psc_dma = psc_dma; snprintf(psc_dma->name, sizeof psc_dma->name, "PSC%u AC97", psc_id+1);
/* Fill out the CPU DAI structure */ nDAI = ARRAY_SIZE(psc_ac97_dai_template); nDAI = min(nDAI, SOC_OF_SIMPLE_MAX_DAI); for (i = 0; i < nDAI; i++) { memcpy(&psc_dma->dai[i], &psc_ac97_dai_template[i], sizeof(struct snd_soc_dai)); psc_dma->dai[i].private_data = psc_dma; snprintf(psc_dma->stream_name[i], PSC_STREAM_NAME_LEN, psc_ac97_dai_template[i].name, psc_dma->name); psc_dma->dai[i].name = psc_dma->stream_name[i]; psc_dma->dai[i].id = psc_id; }
/* Find the address of the fifo data registers and setup the * DMA tasks */ fifo = res.start + offsetof(struct mpc52xx_psc, buffer.buffer_32); psc_dma->capture.bcom_task = bcom_psc_gen_bd_rx_init(psc_id, 10, fifo, 512); psc_dma->playback.bcom_task = bcom_psc_gen_bd_tx_init(psc_id, 10, fifo); if (!psc_dma->capture.bcom_task || !psc_dma->playback.bcom_task) { dev_err(&op->dev, "Could not allocate bestcomm tasks\n"); iounmap(regs); kfree(psc_dma); return -ENODEV; }
/* Disable all interrupts and reset the PSC */ out_be16(&psc_dma->psc_regs->isr_imr.imr, 0); out_8(&psc_dma->psc_regs->command, MPC52xx_PSC_RST_RX); /* reset receiver */ out_8(&psc_dma->psc_regs->command, MPC52xx_PSC_RST_TX); /* reset transmitter */ out_8(&psc_dma->psc_regs->command, MPC52xx_PSC_RST_ERR_STAT); /* reset error */ out_8(&psc_dma->psc_regs->command, MPC52xx_PSC_SEL_MODE_REG_1); /* reset mode */
/* Do a cold reset of codec */ out_8(&psc_dma->psc_regs->op1, MPC52xx_PSC_OP_RES); udelay(10); out_8(&psc_dma->psc_regs->op0, MPC52xx_PSC_OP_RES); udelay(50);
/* Configure the serial interface mode to AC97 */ psc_dma->sicr = MPC52xx_PSC_SICR_SIM_AC97 | MPC52xx_PSC_SICR_ENAC97; out_be32(&psc_dma->psc_regs->sicr, psc_dma->sicr);
/* No slots active */ out_be32(&psc_dma->psc_regs->ac97_slots, 0x00000000);
/* Set up mode register; * First write: RxRdy (FIFO Alarm) generates rx FIFO irq * Second write: register Normal mode for non loopback */ out_8(&psc_dma->psc_regs->mode, 0); out_8(&psc_dma->psc_regs->mode, 0);
/* Set the TX and RX fifo alarm thresholds */ out_be16(&psc_dma->fifo_regs->rfalarm, 0x100); out_8(&psc_dma->fifo_regs->rfcntl, 0x4); out_be16(&psc_dma->fifo_regs->tfalarm, 0x100); out_8(&psc_dma->fifo_regs->tfcntl, 0x7);
/* Go */ out_8(&psc_dma->psc_regs->command, MPC52xx_PSC_TX_ENABLE); out_8(&psc_dma->psc_regs->command, MPC52xx_PSC_RX_ENABLE);
/* Lookup the IRQ numbers */ psc_dma->playback.irq = bcom_get_task_irq(psc_dma->playback.bcom_task); psc_dma->capture.irq = bcom_get_task_irq(psc_dma->capture.bcom_task);
/* Save what we've done so it can be found again later */ dev_set_drvdata(&op->dev, psc_dma);
/* Register the SYSFS files */ rc = device_create_file(psc_dma->dev, &dev_attr_status); rc |= device_create_file(psc_dma->dev, &dev_attr_capture_overrun); rc |= device_create_file(psc_dma->dev, &dev_attr_playback_underrun); if (rc) dev_info(psc_dma->dev, "error creating sysfs files\n");
rc = snd_soc_register_dais(psc_dma->dai, nDAI); if (rc != 0) { printk("Failed to register DAI\n"); return 0; }
/* Tell the ASoC OF helpers about it */ of_snd_soc_register_cpu_dai(op->node, psc_dma->dai, nDAI);
return 0; }
static int __devexit psc_ac97_of_remove(struct of_device *op) { struct psc_dma *psc_dma = dev_get_drvdata(&op->dev);
dev_dbg(&op->dev, "psc_ac97_remove()\n");
bcom_gen_bd_rx_release(psc_dma->capture.bcom_task); bcom_gen_bd_tx_release(psc_dma->playback.bcom_task);
iounmap(psc_dma->psc_regs); iounmap(psc_dma->fifo_regs); kfree(psc_dma); dev_set_drvdata(&op->dev, NULL);
return 0; }
/* Match table for of_platform binding */ static struct of_device_id psc_ac97_match[] __devinitdata = { { .compatible = "fsl,mpc5200-psc-ac97", }, {} }; MODULE_DEVICE_TABLE(of, psc_ac97_match);
static struct of_platform_driver psc_ac97_driver = { .match_table = psc_ac97_match, .probe = psc_ac97_of_probe, .remove = __devexit_p(psc_ac97_of_remove), .driver = { .name = "mpc5200-psc-ac97", .owner = THIS_MODULE, }, };
/* --------------------------------------------------------------------- * Module setup and teardown; simply register the of_platform driver * for the PSC in AC97 mode. */ static int __init psc_ac97_init(void) { return of_register_platform_driver(&psc_ac97_driver); } module_init(psc_ac97_init);
static void __exit psc_ac97_exit(void) { of_unregister_platform_driver(&psc_ac97_driver); } module_exit(psc_ac97_exit);
On Sun, 26 Apr 2009, Jon Smirl wrote:
Here's the code computing the mpc5200 dma pointer. Could you please take a look at it and let me know what it is doing wrong.
I think that the culprit of your problems is that your code expects that buffer_size / period_size is an integer value (whole period is placed inside ring buffer). But if you do not instruct the high level code of ALSA in open() callback by calling snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS) to do so - see other drivers - then period might be also placed across the buffer_size boundary - which appearently makes your current problems.
I think that bcom_submit_next_buffer() expects continuous memory (thus whole period), right?
Jaroslav
----- Jaroslav Kysela perex@perex.cz Linux Kernel Sound Maintainer ALSA Project, Red Hat, Inc.
On Sun, Apr 26, 2009 at 12:43 PM, Jaroslav Kysela perex@perex.cz wrote:
On Sun, 26 Apr 2009, Jon Smirl wrote:
Here's the code computing the mpc5200 dma pointer. Could you please take a look at it and let me know what it is doing wrong.
I think that the culprit of your problems is that your code expects that buffer_size / period_size is an integer value (whole period is placed inside ring buffer). But if you do not instruct the high level code of ALSA in open() callback by calling snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS) to do so - see other drivers - then period might be also placed across the buffer_size boundary - which appearently makes your current problems.
I think that bcom_submit_next_buffer() expects continuous memory (thus whole period), right?
Yes, it wants continuous memory. I added SNDRV_PCM_HW_PARAM_PERIODS and things are working better. That was a better solution than breaking up operations into multiple DMA requests.
Jaroslav
Jaroslav Kysela perex@perex.cz Linux Kernel Sound Maintainer ALSA Project, Red Hat, Inc.
On Sun, Apr 26, 2009 at 02:14:44PM -0400, Jon Smirl wrote:
Yes, it wants continuous memory. I added SNDRV_PCM_HW_PARAM_PERIODS and things are working better. That was a better solution than breaking up operations into multiple DMA requests.
Does that patch need merging for 2.6.30 (you said I2S was broken)?
On Mon, Apr 27, 2009 at 5:25 AM, Mark Brown broonie@sirena.org.uk wrote:
On Sun, Apr 26, 2009 at 02:14:44PM -0400, Jon Smirl wrote:
Yes, it wants continuous memory. I added SNDRV_PCM_HW_PARAM_PERIODS and things are working better. That was a better solution than breaking up operations into multiple DMA requests.
Does that patch need merging for 2.6.30 (you said I2S was broken)?
Grant and I are the only two active users of I2S and we're passing patches to each other. I don't have I2S completely fixed yet.
I'll post a series today that reorgs things to make my real patches easier to apply. The series has already been posted on linux-ppc and ack'd by Grant. He want it to go in via the ALSA trees. It is a passive change that splits mpc5200_i2s.c into multiple files and breaks out the DMA code. I need to rename a couple of globally visible functions before posting it again.
My real patches fix up i2s and implement ac97.
On Mon, Apr 27, 2009 at 09:50:01AM -0400, Jon Smirl wrote:
On Mon, Apr 27, 2009 at 5:25 AM, Mark Brown broonie@sirena.org.uk wrote:
Does that patch need merging for 2.6.30 (you said I2S was broken)?
Grant and I are the only two active users of I2S and we're passing patches to each other. I don't have I2S completely fixed yet.
I'll post a series today that reorgs things to make my real patches
If you're reorganising the code that's not really suitable for 2.6.30.
My real patches fix up i2s and implement ac97.
It sounds like we need fixed I2S for 2.6.30 and then a 2.6.31 series which does the reorganising and addition of AC97 support.
On Mon, Apr 27, 2009 at 9:57 AM, Mark Brown broonie@sirena.org.uk wrote:
On Mon, Apr 27, 2009 at 09:50:01AM -0400, Jon Smirl wrote:
On Mon, Apr 27, 2009 at 5:25 AM, Mark Brown broonie@sirena.org.uk wrote:
Does that patch need merging for 2.6.30 (you said I2S was broken)?
Grant and I are the only two active users of I2S and we're passing patches to each other. I don't have I2S completely fixed yet.
I'll post a series today that reorgs things to make my real patches
If you're reorganising the code that's not really suitable for 2.6.30.
My real patches fix up i2s and implement ac97.
It sounds like we need fixed I2S for 2.6.30 and then a 2.6.31 series which does the reorganising and addition of AC97 support.
The fixes have been implemented on top of the reorganized code. It is hard to review the fixes right now because no one has the reorganized base.
Is there really a problem with sending the reorg in now? There are only two users of the code and we have both ack'd it.
On Mon, Apr 27, 2009 at 10:05 AM, Mark Brown broonie@sirena.org.uk wrote:
On Mon, Apr 27, 2009 at 10:02:47AM -0400, Jon Smirl wrote:
Is there really a problem with sending the reorg in now? There are only two users of the code and we have both ack'd it.
It's a fairly hefty change for -rc4. From the sounds of it you just need to add the constraint?
it is also broken because of the additional requirement of needing to estimate the current position of the DMA transfer. The changes in pcm_lib.c have made it non-functional. When I went into fix those problems I found a couple more issues. It's ok for i2s to be broken in this release, Grant and I both know it is broken and won't ship anything based on it.
Here's the reorg series on linux-ppc http://ozlabs.org/pipermail/linuxppc-dev/2009-April/071206.html I want to do the rename of the globals that Grant suggested.
The pcm_lib.c changes may have broken other embedded drivers too. And yes, these other drivers are probably implemented wrong and the changes in pcm_lib.c exposed their errors. But they will be non-functional until someone notices and fixes them.
On Mon, Apr 27, 2009 at 10:16:16AM -0400, Jon Smirl wrote:
On Mon, Apr 27, 2009 at 10:05 AM, Mark Brown broonie@sirena.org.uk wrote:
It's a fairly hefty change for -rc4. ?From the sounds of it you just need to add the constraint?
it is also broken because of the additional requirement of needing to estimate the current position of the DMA transfer. The changes in pcm_lib.c have made it non-functional. When I went into fix those problems I found a couple more issues. It's ok for i2s to be broken in this release, Grant and I both know it is broken and won't ship anything based on it.
Well, if you don't see any need to fix it then I guess that's OK. We should probably disable the Kconfig option for the release in case people try to use the driver, though.
The pcm_lib.c changes may have broken other embedded drivers too. And
I see no reason to suspect that embedded drivers will be any more or less affected than any other ALSA driver?
On Mon, Apr 27, 2009 at 10:24 AM, Mark Brown broonie@sirena.org.uk wrote:
On Mon, Apr 27, 2009 at 10:16:16AM -0400, Jon Smirl wrote:
On Mon, Apr 27, 2009 at 10:05 AM, Mark Brown broonie@sirena.org.uk wrote:
It's a fairly hefty change for -rc4. ?From the sounds of it you just need to add the constraint?
it is also broken because of the additional requirement of needing to estimate the current position of the DMA transfer. The changes in pcm_lib.c have made it non-functional. When I went into fix those problems I found a couple more issues. It's ok for i2s to be broken in this release, Grant and I both know it is broken and won't ship anything based on it.
Well, if you don't see any need to fix it then I guess that's OK. We should probably disable the Kconfig option for the release in case people try to use the driver, though.
I'll add a patch setting Kconfig BROKEN. Is it ok to do the reorg after flagging it BROKEN?
The pcm_lib.c changes may have broken other embedded drivers too. And
I see no reason to suspect that embedded drivers will be any more or less affected than any other ALSA driver?
The way I'm looking at the code, any CPU hardware that can't report the position of a partially complete DMA transfer is probably broken. Is the mpc5200 the only CPU that doesn't support this?
At Mon, 27 Apr 2009 10:42:10 -0400, Jon Smirl wrote:
On Mon, Apr 27, 2009 at 10:24 AM, Mark Brown broonie@sirena.org.uk wrote:
On Mon, Apr 27, 2009 at 10:16:16AM -0400, Jon Smirl wrote:
On Mon, Apr 27, 2009 at 10:05 AM, Mark Brown broonie@sirena.org.uk wrote:
It's a fairly hefty change for -rc4. ?From the sounds of it you just need to add the constraint?
it is also broken because of the additional requirement of needing to estimate the current position of the DMA transfer. The changes in pcm_lib.c have made it non-functional. When I went into fix those problems I found a couple more issues. It's ok for i2s to be broken in this release, Grant and I both know it is broken and won't ship anything based on it.
Well, if you don't see any need to fix it then I guess that's OK. We should probably disable the Kconfig option for the release in case people try to use the driver, though.
I'll add a patch setting Kconfig BROKEN. Is it ok to do the reorg after flagging it BROKEN?
The pcm_lib.c changes may have broken other embedded drivers too. And
I see no reason to suspect that embedded drivers will be any more or less affected than any other ALSA driver?
The way I'm looking at the code, any CPU hardware that can't report the position of a partially complete DMA transfer is probably broken. Is the mpc5200 the only CPU that doesn't support this?
Well, the following three are completely different things:
A. the driver doesn't report the current DMA position B. the driver reports the wrong DMA position C. the driver calls snd_pcm_period_elapsed() at wrong timing
AFAIK, the problem with mpc5200 is either B or C. It's not about A.
Takashi
On Mon, Apr 27, 2009 at 10:47 AM, Takashi Iwai tiwai@suse.de wrote:
At Mon, 27 Apr 2009 10:42:10 -0400, Jon Smirl wrote:
On Mon, Apr 27, 2009 at 10:24 AM, Mark Brown broonie@sirena.org.uk wrote:
On Mon, Apr 27, 2009 at 10:16:16AM -0400, Jon Smirl wrote:
On Mon, Apr 27, 2009 at 10:05 AM, Mark Brown broonie@sirena.org.uk wrote:
It's a fairly hefty change for -rc4. ?From the sounds of it you just need to add the constraint?
it is also broken because of the additional requirement of needing to estimate the current position of the DMA transfer. The changes in pcm_lib.c have made it non-functional. When I went into fix those problems I found a couple more issues. It's ok for i2s to be broken in this release, Grant and I both know it is broken and won't ship anything based on it.
Well, if you don't see any need to fix it then I guess that's OK. We should probably disable the Kconfig option for the release in case people try to use the driver, though.
I'll add a patch setting Kconfig BROKEN. Is it ok to do the reorg after flagging it BROKEN?
The pcm_lib.c changes may have broken other embedded drivers too. And
I see no reason to suspect that embedded drivers will be any more or less affected than any other ALSA driver?
The way I'm looking at the code, any CPU hardware that can't report the position of a partially complete DMA transfer is probably broken. Is the mpc5200 the only CPU that doesn't support this?
Well, the following three are completely different things:
A. the driver doesn't report the current DMA position B. the driver reports the wrong DMA position C. the driver calls snd_pcm_period_elapsed() at wrong timing
AFAIK, the problem with mpc5200 is either B or C. It's not about A.
I have fixed all of the other bugs. If I take out the position estimation code it fails.
Jaroslav hasn't commented on this yet....
On Sun, Apr 26, 2009 at 2:31 PM, Jaroslav Kysela perex@perex.cz wrote:
On Sun, 26 Apr 2009, Jon Smirl wrote:
If I take this out I get these errors... ALSA sound/core/pcm_lib.c:264: PCM: hw_ptr skipping! [Q] (pos=11026, delta=5513, period=5513, jdelta=33/37/0)
It means that the period DMA operation is too quick in your case and does not correspond to the timing specified by rate. It might be that the FIFO on path is large or rate set to the codec is not correct. What's HZ value on your system and FIFO size? There is HZ/100 margin in the check now.
FIFO is 512 bytes, HZ is 300. The rate on the codec is correct and the music sounds right.
Takashi
On Mon, 27 Apr 2009, Jon Smirl wrote:
I have fixed all of the other bugs. If I take out the position estimation code it fails.
Jaroslav hasn't commented on this yet....
Appearently, FIFO is too big. I'm working on some changes to take account fifo_size to the jiffies based check. Please, set fifo_size correctly in your driver. USB drivers with big "URB FIFOs" should be changed as well, too.
Jaroslav
On Sun, Apr 26, 2009 at 2:31 PM, Jaroslav Kysela perex@perex.cz wrote:
On Sun, 26 Apr 2009, Jon Smirl wrote:
If I take this out I get these errors... ALSA sound/core/pcm_lib.c:264: PCM: hw_ptr skipping! [Q] (pos=11026, delta=5513, period=5513, jdelta=33/37/0)
It means that the period DMA operation is too quick in your case and does not correspond to the timing specified by rate. It might be that the FIFO on path is large or rate set to the codec is not correct. What's HZ value on your system and FIFO size? There is HZ/100 margin in the check now.
FIFO is 512 bytes, HZ is 300. The rate on the codec is correct and the music sounds right.
----- Jaroslav Kysela perex@perex.cz Linux Kernel Sound Maintainer ALSA Project, Red Hat, Inc.
At Mon, 27 Apr 2009 17:09:27 +0200 (CEST), Jaroslav Kysela wrote:
On Mon, 27 Apr 2009, Jon Smirl wrote:
I have fixed all of the other bugs. If I take out the position estimation code it fails.
Jaroslav hasn't commented on this yet....
Appearently, FIFO is too big. I'm working on some changes to take account fifo_size to the jiffies based check. Please, set fifo_size correctly in your driver. USB drivers with big "URB FIFOs" should be changed as well, too.
Ah good, that's what I was thinking to fix up later :)
thanks,
Takashi
At Mon, 27 Apr 2009 17:11:39 +0200, I wrote:
At Mon, 27 Apr 2009 17:09:27 +0200 (CEST), Jaroslav Kysela wrote:
On Mon, 27 Apr 2009, Jon Smirl wrote:
I have fixed all of the other bugs. If I take out the position estimation code it fails.
Jaroslav hasn't commented on this yet....
Appearently, FIFO is too big. I'm working on some changes to take account fifo_size to the jiffies based check. Please, set fifo_size correctly in your driver. USB drivers with big "URB FIFOs" should be changed as well, too.
Ah good, that's what I was thinking to fix up later :)
After thinking twice, I found that we basically don't need much jiffies testing for that kind of hardware because it can't report bad values anyway.
A test patch is below. It suppose that such hardware should have SNDRV_PCM_INFO_BATCH in runtime->hw.info (although most drivers don't set it yet).
Comments?
Takashi
--- diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 63d088f..a2a792c 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -249,6 +249,12 @@ static int snd_pcm_update_hw_ptr_interrupt(struct snd_pcm_substream *substream) new_hw_ptr = hw_base + pos; } } + /* Skip the jiffies check for hardwares with BATCH flag. + * Such hardware usually just increases the position at each IRQ, + * thus it can't give any strange position. + */ + if (runtime->hw.info & SNDRV_PCM_INFO_BATCH) + goto no_jiffies_check; hdelta = new_hw_ptr - old_hw_ptr; jdelta = jiffies - runtime->hw_ptr_jiffies; if (((hdelta * HZ) / runtime->rate) > jdelta + HZ/100) { @@ -272,6 +278,7 @@ static int snd_pcm_update_hw_ptr_interrupt(struct snd_pcm_substream *substream) hw_base -= hw_base % runtime->buffer_size; delta = 0; } + no_jiffies_check: if (delta > runtime->period_size + runtime->period_size / 2) { hw_ptr_error(substream, "Lost interrupts? "
On Mon, Apr 27, 2009 at 05:09:27PM +0200, Jaroslav Kysela wrote:
Appearently, FIFO is too big. I'm working on some changes to take account fifo_size to the jiffies based check. Please, set fifo_size correctly in your driver. USB drivers with big "URB FIFOs" should be changed as well, too.
Could you expand a bit on what qualifies as "too big" here? I've not been following the thread here or the changes that caused problems.
At Mon, 27 Apr 2009 16:15:12 +0100, Mark Brown wrote:
On Mon, Apr 27, 2009 at 05:09:27PM +0200, Jaroslav Kysela wrote:
Appearently, FIFO is too big. I'm working on some changes to take account fifo_size to the jiffies based check. Please, set fifo_size correctly in your driver. USB drivers with big "URB FIFOs" should be changed as well, too.
Could you expand a bit on what qualifies as "too big" here? I've not been following the thread here or the changes that caused problems.
It's bigger than the pre-defined tolerance of jiffies check in PCM core code.
The new PCM core checks the returned hw_ptr together with jiffies delta whether it's a sane value. When FIFO is used with the ring buffer, the hardware tends to report back earlier before the expected timing.
Takashi
On Tue, Apr 28, 2009 at 08:08:06AM +0200, Takashi Iwai wrote:
Mark Brown wrote:
On Mon, Apr 27, 2009 at 05:09:27PM +0200, Jaroslav Kysela wrote:
Appearently, FIFO is too big. I'm working on some changes to take account
Could you expand a bit on what qualifies as "too big" here? I've not been following the thread here or the changes that caused problems.
It's bigger than the pre-defined tolerance of jiffies check in PCM core code.
Hrm, OK. That may create problems for some embedded systems which can use very large FIFOs (well, FIFO equivalents) to allow the processor to go to sleep when doing things like MP3 playback. I don't think any of those have audio support in mainline yet, though.
The new PCM core checks the returned hw_ptr together with jiffies delta whether it's a sane value. When FIFO is used with the ring buffer, the hardware tends to report back earlier before the expected timing.
But (from other messages in the thread) it's possible to disable these checks if required?
At Tue, 28 Apr 2009 10:00:02 +0100, Mark Brown wrote:
On Tue, Apr 28, 2009 at 08:08:06AM +0200, Takashi Iwai wrote:
Mark Brown wrote:
On Mon, Apr 27, 2009 at 05:09:27PM +0200, Jaroslav Kysela wrote:
Appearently, FIFO is too big. I'm working on some changes to take account
Could you expand a bit on what qualifies as "too big" here? I've not been following the thread here or the changes that caused problems.
It's bigger than the pre-defined tolerance of jiffies check in PCM core code.
Hrm, OK. That may create problems for some embedded systems which can use very large FIFOs (well, FIFO equivalents) to allow the processor to go to sleep when doing things like MP3 playback. I don't think any of those have audio support in mainline yet, though.
The new PCM core checks the returned hw_ptr together with jiffies delta whether it's a sane value. When FIFO is used with the ring buffer, the hardware tends to report back earlier before the expected timing.
But (from other messages in the thread) it's possible to disable these checks if required?
That's being discussed now :)
Takashi
On Mon, 27 Apr 2009, Mark Brown wrote:
On Mon, Apr 27, 2009 at 05:09:27PM +0200, Jaroslav Kysela wrote:
Appearently, FIFO is too big. I'm working on some changes to take account fifo_size to the jiffies based check. Please, set fifo_size correctly in your driver. USB drivers with big "URB FIFOs" should be changed as well, too.
Could you expand a bit on what qualifies as "too big" here? I've not been following the thread here or the changes that caused problems.
The problem is that period_elapsed() callback checks if data are not "consumed" (playback) or "produced" (capture) too quickly using jiffies timing (with HZ/100 margin). In case of large FIFO, the data are buffered, but the real playback position is different.
The question is, if driver's pointer function should estimate more real stream position (taking in account the FIFO) in this case or if upper layers of the ALSA driver and applications should handle the situation using FIFO size value.
I would suggest this:
- implement the stream position corrections in the pointer callback - in this case applications get valid realtime timing source for A/V synchronization etc. - in case of mpc driver, another timing and interrupt source should be used to compute the pointer value not DMA engine - leave current fifo_size without change (there will be only small FIFO values to notify applications about small extra delays before samples reaches DAC or leaves ADC) - add cache_size value to snd_pcm_hardware struct - here will be stored area of buffer which might be cached in the hardware for large FIFOs including prepared USB's URBs (the upper layer should not work with this buffer area - rewind operation etc.)
Other ideas?
Jaroslav
----- Jaroslav Kysela perex@perex.cz Linux Kernel Sound Maintainer ALSA Project, Red Hat, Inc.
At Tue, 28 Apr 2009 10:55:53 +0200 (CEST), Jaroslav Kysela wrote:
On Mon, 27 Apr 2009, Mark Brown wrote:
On Mon, Apr 27, 2009 at 05:09:27PM +0200, Jaroslav Kysela wrote:
Appearently, FIFO is too big. I'm working on some changes to take account fifo_size to the jiffies based check. Please, set fifo_size correctly in your driver. USB drivers with big "URB FIFOs" should be changed as well, too.
Could you expand a bit on what qualifies as "too big" here? I've not been following the thread here or the changes that caused problems.
The problem is that period_elapsed() callback checks if data are not "consumed" (playback) or "produced" (capture) too quickly using jiffies timing (with HZ/100 margin). In case of large FIFO, the data are buffered, but the real playback position is different.
Well, but the hardware accepts the data of period_size already at this point. In that sense, the driver returns the correct hwptr value, i.e. pointer callback behaves in a correct way.
The question is, if driver's pointer function should estimate more real stream position (taking in account the FIFO) in this case or if upper layers of the ALSA driver and applications should handle the situation using FIFO size value.
Right. The real problem is that hwptr doesn't represent the real playing/recording position in these hardware. But, it worked like that, thus there is no reason to break this behavior at this point. The identity of hwptr and the real position wasn't (isn't) strictly defined yet.
I would suggest this:
- implement the stream position corrections in the pointer callback - in this case applications get valid realtime timing source for A/V synchronization etc. - in case of mpc driver, another timing and interrupt source should be used to compute the pointer value not DMA engine
I don't think the pointer callback should be changed for that. We have two different pointer representations, and the pointer callback itself works as is.
- leave current fifo_size without change (there will be only small FIFO values to notify applications about small extra delays before samples reaches DAC or leaves ADC)
fifo_size field is supposed to be constant, and shouldn't be changed dynamically after open. This is a problem if the FIFO size varies depending on the parameter.
- add cache_size value to snd_pcm_hardware struct - here will be stored area of buffer which might be cached in the hardware for large FIFOs including prepared USB's URBs (the upper layer should not work with this buffer area - rewind operation etc.)
I already submitted a similar patch quite ago. Actually, it exposes the delay account in snd_pcm_status() delay calculation. The delay value can be dynamically changed.
The current test patches are found on sound unstable tree topic/pcm-delay branch. git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-unstable-2.6.git
Takashi
On Tue, 28 Apr 2009, Takashi Iwai wrote:
- leave current fifo_size without change (there will be only small FIFO values to notify applications about small extra delays before samples reaches DAC or leaves ADC)
fifo_size field is supposed to be constant, and shouldn't be changed dynamically after open. This is a problem if the FIFO size varies depending on the parameter.
The alsa-lib's documentation states that one configuration must be chosen (thus hw_params should be called) to obtain fifo_size. It might depend on hardware parameters, but is static then. I would not propose to change anything with it.
- add cache_size value to snd_pcm_hardware struct - here will be stored area of buffer which might be cached in the hardware for large FIFOs including prepared USB's URBs (the upper layer should not work with this buffer area - rewind operation etc.)
I already submitted a similar patch quite ago. Actually, it exposes the delay account in snd_pcm_status() delay calculation. The delay value can be dynamically changed.
The current test patches are found on sound unstable tree topic/pcm-delay branch. git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-unstable-2.6.git
Looks good. Could you pick these patches to your stable tree so we can stack other patches on top? I need to add 'delay' to hdelta in current elapsed callback to avoid false alarms.
Also, John should initialize this runtime->delay to correct value in frames in the mpc driver.
Jaroslav
----- Jaroslav Kysela perex@perex.cz Linux Kernel Sound Maintainer ALSA Project, Red Hat, Inc.
At Tue, 28 Apr 2009 11:56:20 +0200 (CEST), Jaroslav Kysela wrote:
On Tue, 28 Apr 2009, Takashi Iwai wrote:
- leave current fifo_size without change (there will be only small FIFO values to notify applications about small extra delays before samples reaches DAC or leaves ADC)
fifo_size field is supposed to be constant, and shouldn't be changed dynamically after open. This is a problem if the FIFO size varies depending on the parameter.
The alsa-lib's documentation states that one configuration must be chosen (thus hw_params should be called) to obtain fifo_size. It might depend on hardware parameters, but is static then. I would not propose to change anything with it.
Good.
- add cache_size value to snd_pcm_hardware struct - here will be stored area of buffer which might be cached in the hardware for large FIFOs including prepared USB's URBs (the upper layer should not work with this buffer area - rewind operation etc.)
I already submitted a similar patch quite ago. Actually, it exposes the delay account in snd_pcm_status() delay calculation. The delay value can be dynamically changed.
The current test patches are found on sound unstable tree topic/pcm-delay branch. git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-unstable-2.6.git
Looks good. Could you pick these patches to your stable tree so we can stack other patches on top? I need to add 'delay' to hdelta in current elapsed callback to avoid false alarms.
Also, John should initialize this runtime->delay to correct value in frames in the mpc driver.
Well, I think it'd be better, at this moment for 2.6.30, to allow problematic drivers to skip the jiffies check. Adding the fundamental change at this late stage is bad, especially if the change wasn't tested much by many others and has an influence on user-side API.
So, as I proposed, we can simply add the pcm info check, and add BATCH flag to needed drivers. Of course, it still requires some more testing, but basically it'll take back to the old behavior and should be safer.
Meanwhile, applying the delay account patch now for 2.6.31 should be good (ealier is better). It doesn't conflict with the info flag check, so we can work parallel.
Takashi
On Tue, 28 Apr 2009, Takashi Iwai wrote:
Well, I think it'd be better, at this moment for 2.6.30, to allow problematic drivers to skip the jiffies check. Adding the fundamental change at this late stage is bad, especially if the change wasn't tested much by many others and has an influence on user-side API.
So, as I proposed, we can simply add the pcm info check, and add BATCH flag to needed drivers. Of course, it still requires some more testing, but basically it'll take back to the old behavior and should be safer.
Meanwhile, applying the delay account patch now for 2.6.31 should be good (ealier is better). It doesn't conflict with the info flag check, so we can work parallel.
Could we just add the runtime->delay variable without touching other code (pcm status), to correct the jiffies based check now? It seems like a good step forward to me and the lowlevel drivers will be more prepared for next PCM midlevel code changes.
Jaroslav
----- Jaroslav Kysela perex@perex.cz Linux Kernel Sound Maintainer ALSA Project, Red Hat, Inc.
At Tue, 28 Apr 2009 12:15:25 +0200 (CEST), Jaroslav Kysela wrote:
On Tue, 28 Apr 2009, Takashi Iwai wrote:
Well, I think it'd be better, at this moment for 2.6.30, to allow problematic drivers to skip the jiffies check. Adding the fundamental change at this late stage is bad, especially if the change wasn't tested much by many others and has an influence on user-side API.
So, as I proposed, we can simply add the pcm info check, and add BATCH flag to needed drivers. Of course, it still requires some more testing, but basically it'll take back to the old behavior and should be safer.
Meanwhile, applying the delay account patch now for 2.6.31 should be good (ealier is better). It doesn't conflict with the info flag check, so we can work parallel.
Could we just add the runtime->delay variable without touching other code (pcm status), to correct the jiffies based check now?
That's possible, of course.
It seems like a good step forward to me and the lowlevel drivers will be more prepared for next PCM midlevel code changes.
Well, but I guess providing the proper FIFO size isn't trivial to each driver. There are many devices, as Mark suggested, which may be broken right now, and we can't determine all h/w specs and test them.
OTOH, adding INFO_BATCH is fairly easy, because it can be found from the pointer callback implementation of each driver. On these devices, the jiffies check doesn't help much anyway because it cannot update the hwptr any other than the period size. So, skipping jiffies check is logically correct as a regression fix.
Of course, it could help to inform whether the period-elapsed call is accurate. But, it's another story from the regression fix, which is a more urgent issue.
So, what I'm suggesting is to fix the 2.6.30 behavior in minimal effort while we introduce a better framework for 2.6.31. Both can go at the same time.
I already did some work for 2.6.30 fix. It's found in sound git tree fix/pcm-jiffies-check branch. You can pull from git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git fix/pcm-jiffies-check
This is still a test branch so it isn't merged yet. It'd be helpful if someone can test it, or let me know this change is unacceptable.
thanks,
Takashi
On Tue, 28 Apr 2009, Takashi Iwai wrote:
At Tue, 28 Apr 2009 12:15:25 +0200 (CEST), Jaroslav Kysela wrote:
On Tue, 28 Apr 2009, Takashi Iwai wrote:
Well, I think it'd be better, at this moment for 2.6.30, to allow problematic drivers to skip the jiffies check. Adding the fundamental change at this late stage is bad, especially if the change wasn't tested much by many others and has an influence on user-side API.
So, as I proposed, we can simply add the pcm info check, and add BATCH flag to needed drivers. Of course, it still requires some more testing, but basically it'll take back to the old behavior and should be safer.
Meanwhile, applying the delay account patch now for 2.6.31 should be good (ealier is better). It doesn't conflict with the info flag check, so we can work parallel.
Could we just add the runtime->delay variable without touching other code (pcm status), to correct the jiffies based check now?
That's possible, of course.
It seems like a good step forward to me and the lowlevel drivers will be more prepared for next PCM midlevel code changes.
Well, but I guess providing the proper FIFO size isn't trivial to each driver. There are many devices, as Mark suggested, which may be broken right now, and we can't determine all h/w specs and test them.
OTOH, adding INFO_BATCH is fairly easy, because it can be found from the pointer callback implementation of each driver. On these devices, the jiffies check doesn't help much anyway because it cannot update the hwptr any other than the period size. So, skipping jiffies check is logically correct as a regression fix.
I don't see much difference between adding INFO_BATCH and adding runtime->delay = (runtime->period_size)-1 to avoid jiffies check for one period to these drivers (with some notice to author of the driver to correct this value).
INFO_BATCH is currently in:
/soc/fsl/mpc5200_psc_i2s.c /soc/au1x/dbdma2.c /soc/sh/dma-sh7760.c /usb/usbaudio.c (which works with the current jiffies check at least for 44100+ rates)
Another (much cleaner) possibility is to make jiffies check conditional to xrun_check() for 2.6.30 and do not bother with the INFO_BATCH flag.
Jiffies check helped me to debug HDA/intel8x0 pointer problems but cannot be triggered until driver is really buggy - the HDA/intel8x0 driver should be fixed now (I know about one intel8x0 pointer problem to this time which triggers this check, but there is still something wrong with returned hardware pointer because audio clicks anyway - no regression from previous state).
Jaroslav
----- Jaroslav Kysela perex@perex.cz Linux Kernel Sound Maintainer ALSA Project, Red Hat, Inc.
At Tue, 28 Apr 2009 13:30:09 +0200 (CEST), Jaroslav Kysela wrote:
On Tue, 28 Apr 2009, Takashi Iwai wrote:
At Tue, 28 Apr 2009 12:15:25 +0200 (CEST), Jaroslav Kysela wrote:
On Tue, 28 Apr 2009, Takashi Iwai wrote:
Well, I think it'd be better, at this moment for 2.6.30, to allow problematic drivers to skip the jiffies check. Adding the fundamental change at this late stage is bad, especially if the change wasn't tested much by many others and has an influence on user-side API.
So, as I proposed, we can simply add the pcm info check, and add BATCH flag to needed drivers. Of course, it still requires some more testing, but basically it'll take back to the old behavior and should be safer.
Meanwhile, applying the delay account patch now for 2.6.31 should be good (ealier is better). It doesn't conflict with the info flag check, so we can work parallel.
Could we just add the runtime->delay variable without touching other code (pcm status), to correct the jiffies based check now?
That's possible, of course.
It seems like a good step forward to me and the lowlevel drivers will be more prepared for next PCM midlevel code changes.
Well, but I guess providing the proper FIFO size isn't trivial to each driver. There are many devices, as Mark suggested, which may be broken right now, and we can't determine all h/w specs and test them.
OTOH, adding INFO_BATCH is fairly easy, because it can be found from the pointer callback implementation of each driver. On these devices, the jiffies check doesn't help much anyway because it cannot update the hwptr any other than the period size. So, skipping jiffies check is logically correct as a regression fix.
I don't see much difference between adding INFO_BATCH and adding runtime->delay = (runtime->period_size)-1 to avoid jiffies check for one period to these drivers (with some notice to author of the driver to correct this value).
INFO_BATCH is the correct flag for such drivers. They should have the flag, independent from the fix.
Also, setting -1 to runtime->delay is not what intended for the delay accounting. Starting from a (sort of) abuse doesn't sound good...
INFO_BATCH is currently in:
/soc/fsl/mpc5200_psc_i2s.c /soc/au1x/dbdma2.c /soc/sh/dma-sh7760.c /usb/usbaudio.c (which works with the current jiffies check at least for 44100+ rates)
Another (much cleaner) possibility is to make jiffies check conditional to xrun_check() for 2.6.30 and do not bother with the INFO_BATCH flag.
Jiffies check helped me to debug HDA/intel8x0 pointer problems but cannot be triggered until driver is really buggy - the HDA/intel8x0 driver should be fixed now (I know about one intel8x0 pointer problem to this time which triggers this check, but there is still something wrong with returned hardware pointer because audio clicks anyway - no regression from previous state).
That makes sense, too. Maybe even safer.
Takashi
At Tue, 28 Apr 2009 14:02:01 +0200, I wrote:
At Tue, 28 Apr 2009 13:30:09 +0200 (CEST), Jaroslav Kysela wrote:
On Tue, 28 Apr 2009, Takashi Iwai wrote:
At Tue, 28 Apr 2009 12:15:25 +0200 (CEST), Jaroslav Kysela wrote:
On Tue, 28 Apr 2009, Takashi Iwai wrote:
Well, I think it'd be better, at this moment for 2.6.30, to allow problematic drivers to skip the jiffies check. Adding the fundamental change at this late stage is bad, especially if the change wasn't tested much by many others and has an influence on user-side API.
So, as I proposed, we can simply add the pcm info check, and add BATCH flag to needed drivers. Of course, it still requires some more testing, but basically it'll take back to the old behavior and should be safer.
Meanwhile, applying the delay account patch now for 2.6.31 should be good (ealier is better). It doesn't conflict with the info flag check, so we can work parallel.
Could we just add the runtime->delay variable without touching other code (pcm status), to correct the jiffies based check now?
That's possible, of course.
It seems like a good step forward to me and the lowlevel drivers will be more prepared for next PCM midlevel code changes.
Well, but I guess providing the proper FIFO size isn't trivial to each driver. There are many devices, as Mark suggested, which may be broken right now, and we can't determine all h/w specs and test them.
OTOH, adding INFO_BATCH is fairly easy, because it can be found from the pointer callback implementation of each driver. On these devices, the jiffies check doesn't help much anyway because it cannot update the hwptr any other than the period size. So, skipping jiffies check is logically correct as a regression fix.
I don't see much difference between adding INFO_BATCH and adding runtime->delay = (runtime->period_size)-1 to avoid jiffies check for one period to these drivers (with some notice to author of the driver to correct this value).
INFO_BATCH is the correct flag for such drivers. They should have the flag, independent from the fix.
So fixed now on sound git tree. The drivers that can't report precise position have now INFO_BATCH flag.
The workaround for jiffies check isn't applied yet. Just this flags, so far.
Takashi
At Tue, 28 Apr 2009 14:02:01 +0200, I wrote:
At Tue, 28 Apr 2009 13:30:09 +0200 (CEST), Jaroslav Kysela wrote:
Another (much cleaner) possibility is to make jiffies check conditional to xrun_check() for 2.6.30 and do not bother with the INFO_BATCH flag.
Are you going to implement it soon? We need a fix in short time, so if you have no time for that, I'll merge my patch as a tentative fix first.
thanks,
Takashi
At Tue, 28 Apr 2009 12:32:25 +0200, I wrote:
I already did some work for 2.6.30 fix. It's found in sound git tree fix/pcm-jiffies-check branch. You can pull from git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git fix/pcm-jiffies-check
This is still a test branch so it isn't merged yet. It'd be helpful if someone can test it, or let me know this change is unacceptable.
Jon, any chance that you try the changes above? It should fix mpc5200 issue on 2.6.30.
thanks,
Takashi
On Mon, May 4, 2009 at 3:30 AM, Takashi Iwai tiwai@suse.de wrote:
At Tue, 28 Apr 2009 12:32:25 +0200, I wrote:
I already did some work for 2.6.30 fix. It's found in sound git tree fix/pcm-jiffies-check branch. You can pull from git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git fix/pcm-jiffies-check
This is still a test branch so it isn't merged yet. It'd be helpful if someone can test it, or let me know this change is unacceptable.
Jon, any chance that you try the changes above? It should fix mpc5200 issue on 2.6.30.
The fix is good. I can take out my jiffies estimation code and no area are generated.
thanks,
Takashi
At Mon, 4 May 2009 08:29:14 -0400, Jon Smirl wrote:
On Mon, May 4, 2009 at 3:30 AM, Takashi Iwai tiwai@suse.de wrote:
At Tue, 28 Apr 2009 12:32:25 +0200, I wrote:
I already did some work for 2.6.30 fix. It's found in sound git tree fix/pcm-jiffies-check branch. You can pull from git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git fix/pcm-jiffies-check
This is still a test branch so it isn't merged yet. It'd be helpful if someone can test it, or let me know this change is unacceptable.
Jon, any chance that you try the changes above? It should fix mpc5200 issue on 2.6.30.
The fix is good. I can take out my jiffies estimation code and no area are generated.
Great. So can we remove BROKEN kconfig again?
thanks,
Takashi
On Mon, May 4, 2009 at 8:43 AM, Takashi Iwai tiwai@suse.de wrote:
At Mon, 4 May 2009 08:29:14 -0400, Jon Smirl wrote:
On Mon, May 4, 2009 at 3:30 AM, Takashi Iwai tiwai@suse.de wrote:
At Tue, 28 Apr 2009 12:32:25 +0200, I wrote:
I already did some work for 2.6.30 fix. It's found in sound git tree fix/pcm-jiffies-check branch. You can pull from git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git fix/pcm-jiffies-check
This is still a test branch so it isn't merged yet. It'd be helpful if someone can test it, or let me know this change is unacceptable.
Jon, any chance that you try the changes above? It should fix mpc5200 issue on 2.6.30.
The fix is good. I can take out my jiffies estimation code and no area are generated.
Great. So can we remove BROKEN kconfig again?
Sure. There are only two current users of the driver and we know what is going on.
I have a large rewrite of the driver for 2.6.31 but it still has bugs that I am having a hard time locating.
On Mon, May 04, 2009 at 08:58:04AM -0400, Jon Smirl wrote:
On Mon, May 4, 2009 at 8:43 AM, Takashi Iwai tiwai@suse.de wrote:
Great. ?So can we remove BROKEN kconfig again?
Sure. There are only two current users of the driver and we know what is going on.
Could you expand on what you mean by "we know what is going on"? If the driver is fixed then there shouldn't really be anything for anyone to know, the driver should just work.
I wouldn't count on you being the only users - you may be the only people visibly talking about the driver in public but that's not quite the same thing as being the only users.
On Mon, May 4, 2009 at 9:00 AM, Mark Brown broonie@sirena.org.uk wrote:
On Mon, May 04, 2009 at 08:58:04AM -0400, Jon Smirl wrote:
On Mon, May 4, 2009 at 8:43 AM, Takashi Iwai tiwai@suse.de wrote:
Great. ?So can we remove BROKEN kconfig again?
Sure. There are only two current users of the driver and we know what is going on.
Could you expand on what you mean by "we know what is going on"? If the driver is fixed then there shouldn't really be anything for anyone to know, the driver should just work.
I wouldn't count on you being the only users - you may be the only people visibly talking about the driver in public but that's not quite the same thing as being the only users.
With the fix the driver is back to working the way it was before the jiffies issue completely broke it.
But there are still problems in the driver. For example it is not terminating streams correctly. I get a little burp of sound at the end of stream that doesn't belong there.
I'm trying to fix the burp. How is stream termination supposed to work? Right now when the driver gets STOP in the trigger function. It immediately stops the DMA engine and resets the I2S channel. I'm not clear on how this get synchronized in order to prevent the partial transfer of a DMA period and allowing the FIFO time to drain.
At Mon, 4 May 2009 09:22:22 -0400, Jon Smirl wrote:
On Mon, May 4, 2009 at 9:00 AM, Mark Brown broonie@sirena.org.uk wrote:
On Mon, May 04, 2009 at 08:58:04AM -0400, Jon Smirl wrote:
On Mon, May 4, 2009 at 8:43 AM, Takashi Iwai tiwai@suse.de wrote:
Great. ?So can we remove BROKEN kconfig again?
Sure. There are only two current users of the driver and we know what is going on.
Could you expand on what you mean by "we know what is going on"? If the driver is fixed then there shouldn't really be anything for anyone to know, the driver should just work.
I wouldn't count on you being the only users - you may be the only people visibly talking about the driver in public but that's not quite the same thing as being the only users.
With the fix the driver is back to working the way it was before the jiffies issue completely broke it.
But there are still problems in the driver. For example it is not terminating streams correctly. I get a little burp of sound at the end of stream that doesn't belong there.
I'm trying to fix the burp. How is stream termination supposed to work? Right now when the driver gets STOP in the trigger function. It immediately stops the DMA engine and resets the I2S channel. I'm not clear on how this get synchronized in order to prevent the partial transfer of a DMA period and allowing the FIFO time to drain.
Any time-consuming operation such as sync can be done in prepare callback since it is always called before (re-)triggering START.
Takashi
At Mon, 4 May 2009 09:22:22 -0400, Jon Smirl wrote:
On Mon, May 4, 2009 at 9:00 AM, Mark Brown broonie@sirena.org.uk wrote:
On Mon, May 04, 2009 at 08:58:04AM -0400, Jon Smirl wrote:
On Mon, May 4, 2009 at 8:43 AM, Takashi Iwai tiwai@suse.de wrote:
Great. ?So can we remove BROKEN kconfig again?
Sure. There are only two current users of the driver and we know what is going on.
Could you expand on what you mean by "we know what is going on"? If the driver is fixed then there shouldn't really be anything for anyone to know, the driver should just work.
I wouldn't count on you being the only users - you may be the only people visibly talking about the driver in public but that's not quite the same thing as being the only users.
With the fix the driver is back to working the way it was before the jiffies issue completely broke it.
OK, now the branch is merged to fix/asoc and topic/asoc branches. I'll submit a pull request for 2.6.30-rc4 soon later.
thanks,
Takashi
At Tue, 28 Apr 2009 11:56:20 +0200 (CEST), Jaroslav Kysela wrote:
- add cache_size value to snd_pcm_hardware struct - here will be stored area of buffer which might be cached in the hardware for large FIFOs including prepared USB's URBs (the upper layer should not work with this buffer area - rewind operation etc.)
I already submitted a similar patch quite ago. Actually, it exposes the delay account in snd_pcm_status() delay calculation. The delay value can be dynamically changed.
The current test patches are found on sound unstable tree topic/pcm-delay branch. git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-unstable-2.6.git
Looks good. Could you pick these patches to your stable tree so we can stack other patches on top?
FYI, the patches appear now in topic/pcm-delay branch of sound git tree, and merged to master branch, too.
Takashi
On Tue, Apr 28, 2009 at 10:55:53AM +0200, Jaroslav Kysela wrote:
On Mon, 27 Apr 2009, Mark Brown wrote:
Could you expand a bit on what qualifies as "too big" here? I've not been following the thread here or the changes that caused problems.
The problem is that period_elapsed() callback checks if data are not "consumed" (playback) or "produced" (capture) too quickly using jiffies timing (with HZ/100 margin). In case of large FIFO, the data are buffered, but the real playback position is different.
Right. As I said in my excellently timed reply to Takashi just now this is liable to cause problems for some embedded applications. There are some which can use a large (tens to hundreds of kilobytes) FIFO to allow audio playback to proceed with the processor and RAM suspended, reducing power consumption during things like MP3 playback which aren't latency sensitive. In latency sensitive cases the application needs to submit less data - there's no requirement to fill.
The question is, if driver's pointer function should estimate more real stream position (taking in account the FIFO) in this case or if upper layers of the ALSA driver and applications should handle the situation using FIFO size value.
Or both, I guess.
I would suggest this:
- implement the stream position corrections in the pointer callback - in this case applications get valid realtime timing source for A/V synchronization etc. - in case of mpc driver, another timing and interrupt source should be used to compute the pointer value not DMA engine
This feels like it's being done at the wrong level - if estimation code is needed it should probably be pushed up the stack, done based on information provided by the driver about how accurate the information it's providing is.
At Mon, 27 Apr 2009 17:09:27 +0200 (CEST), Jaroslav Kysela wrote:
On Mon, 27 Apr 2009, Jon Smirl wrote:
I have fixed all of the other bugs. If I take out the position estimation code it fails.
Jaroslav hasn't commented on this yet....
Appearently, FIFO is too big. I'm working on some changes to take account fifo_size to the jiffies based check. Please, set fifo_size correctly in your driver. USB drivers with big "URB FIFOs" should be changed as well, too.
Does the usb-audio driver really suffer, too?
Takashi
On Mon, 27 Apr 2009, Takashi Iwai wrote:
At Mon, 27 Apr 2009 17:09:27 +0200 (CEST), Jaroslav Kysela wrote:
On Mon, 27 Apr 2009, Jon Smirl wrote:
I have fixed all of the other bugs. If I take out the position estimation code it fails.
Jaroslav hasn't commented on this yet....
Appearently, FIFO is too big. I'm working on some changes to take account fifo_size to the jiffies based check. Please, set fifo_size correctly in your driver. USB drivers with big "URB FIFOs" should be changed as well, too.
Does the usb-audio driver really suffer, too?
It seems not - thanks to nodata URBs. I just verified it with real hardware.
Jaroslav
----- Jaroslav Kysela perex@perex.cz Linux Kernel Sound Maintainer ALSA Project, Red Hat, Inc.
On Mon, Apr 27, 2009 at 11:04 AM, Jon Smirl jonsmirl@gmail.com wrote:
On Mon, Apr 27, 2009 at 10:47 AM, Takashi Iwai tiwai@suse.de wrote:
At Mon, 27 Apr 2009 10:42:10 -0400, Jon Smirl wrote:
On Mon, Apr 27, 2009 at 10:24 AM, Mark Brown broonie@sirena.org.uk wrote:
On Mon, Apr 27, 2009 at 10:16:16AM -0400, Jon Smirl wrote:
On Mon, Apr 27, 2009 at 10:05 AM, Mark Brown broonie@sirena.org.uk wrote:
It's a fairly hefty change for -rc4. ?From the sounds of it you just need to add the constraint?
it is also broken because of the additional requirement of needing to estimate the current position of the DMA transfer. The changes in pcm_lib.c have made it non-functional. When I went into fix those problems I found a couple more issues. It's ok for i2s to be broken in this release, Grant and I both know it is broken and won't ship anything based on it.
Well, if you don't see any need to fix it then I guess that's OK. We should probably disable the Kconfig option for the release in case people try to use the driver, though.
I'll add a patch setting Kconfig BROKEN. Is it ok to do the reorg after flagging it BROKEN?
The pcm_lib.c changes may have broken other embedded drivers too. And
I see no reason to suspect that embedded drivers will be any more or less affected than any other ALSA driver?
The way I'm looking at the code, any CPU hardware that can't report the position of a partially complete DMA transfer is probably broken. Is the mpc5200 the only CPU that doesn't support this?
Well, the following three are completely different things:
A. the driver doesn't report the current DMA position B. the driver reports the wrong DMA position C. the driver calls snd_pcm_period_elapsed() at wrong timing
AFAIK, the problem with mpc5200 is either B or C. It's not about A.
I have fixed all of the other bugs. If I take out the position estimation code it fails.
Jaroslav hasn't commented on this yet....
BTW, my theory is that the FIFO fill is causing this. The initial fill makes it look like the audio hardware is moving too quickly.
I have to sample my DMA position in front of the FIFO fill. CPUs that can get the current position sample it after the FIFO.
On Sun, Apr 26, 2009 at 2:31 PM, Jaroslav Kysela perex@perex.cz wrote:
On Sun, 26 Apr 2009, Jon Smirl wrote:
If I take this out I get these errors... ALSA sound/core/pcm_lib.c:264: PCM: hw_ptr skipping! [Q] (pos=11026, delta=5513, period=5513, jdelta=33/37/0)
It means that the period DMA operation is too quick in your case and does not correspond to the timing specified by rate. It might be that the FIFO on path is large or rate set to the codec is not correct. What's HZ value on your system and FIFO size? There is HZ/100 margin in the check now.
FIFO is 512 bytes, HZ is 300. The rate on the codec is correct and the music sounds right.
Takashi
-- Jon Smirl jonsmirl@gmail.com
On Mon, Apr 27, 2009 at 10:42:10AM -0400, Jon Smirl wrote:
On Mon, Apr 27, 2009 at 10:24 AM, Mark Brown broonie@sirena.org.uk wrote:
Well, if you don't see any need to fix it then I guess that's OK. ?We should probably disable the Kconfig option for the release in case people try to use the driver, though.
I'll add a patch setting Kconfig BROKEN. Is it ok to do the reorg after flagging it BROKEN?
Send a 2.6.30 patch marking it broken and then add a patch removing that to the end of your reorganisation series.
On Sun, Apr 26, 2009 at 10:25 AM, Jon Smirl jonsmirl@gmail.com wrote:
static snd_pcm_uframes_t psc_dma_pcm_pointer(struct snd_pcm_substream *substream) { struct snd_pcm_runtime *runtime = substream->runtime; struct snd_soc_pcm_runtime *rtd = substream->private_data; struct psc_dma *psc_dma = rtd->dai->cpu_dai->private_data; struct psc_dma_stream *s; dma_addr_t count; snd_pcm_uframes_t frames; int delta;
if (substream->pstr->stream == SNDRV_PCM_STREAM_CAPTURE) s = &psc_dma->capture; else s = &psc_dma->playback;
count = s->period_current_pt - s->period_start;
delta = jiffies - s->jiffies; delta = delta * runtime->rate / HZ; frames = bytes_to_frames(substream->runtime, count);
Have you considered using the timebase register instead of jiffies? It's more accurate, and you can use tb_ticks_per_usec.
participants (5)
-
Jaroslav Kysela -
Jon Smirl -
Mark Brown -
Takashi Iwai -
Timur Tabi