Re: [alsa-devel] [PATCH] compress: add support for gapless playback

At Mon, 11 Feb 2013 04:22:45 -0800, Vinod Koul wrote:

(snip)

--- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -271,7 +271,9 @@ typedef int __bitwise snd_pcm_state_t; #define SNDRV_PCM_STATE_PAUSED ((__force snd_pcm_state_t) 6) /* stream is paused */ #define SNDRV_PCM_STATE_SUSPENDED ((__force snd_pcm_state_t) 7) /* hardware is suspended */ #define SNDRV_PCM_STATE_DISCONNECTED ((__force snd_pcm_state_t) 8) /* hardware is disconnected */ -#define SNDRV_PCM_STATE_LAST SNDRV_PCM_STATE_DISCONNECTED +#define SNDRV_PCM_STATE_NEXT_TRACK ((__force snd_pcm_state_t) 9) /* stream will move to next track */

Isn't this better to be set as another flag instead of the trigger command? This changes the runtime->state, and it may make things complicated. For example, what happens if user triggers NEXT_TRACK, then PAUSE_PUSH and PAUSE_RELEASE?

+#define SNDRV_PCM_STATE_PARTIAL_DRAIN ((__force snd_pcm_state_t) 10) /* stream is draining partially */ +#define SNDRV_PCM_STATE_LAST SNDRV_PCM_STATE_PARTIAL_DRAIN

Well, I'd hesitate to add this for PCM, since there is no counterpart for PCM (yet). Until then, let's avoid touching the PCM API definition but only compress API.

enum { SNDRV_PCM_MMAP_OFFSET_DATA = 0x00000000, diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h index 05341a4..1d63993 100644 --- a/include/uapi/sound/compress_offload.h +++ b/include/uapi/sound/compress_offload.h @@ -30,7 +30,7 @@ #include <sound/compress_params.h>

-#define SNDRV_COMPRESS_VERSION SNDRV_PROTOCOL_VERSION(0, 1, 0) +#define SNDRV_COMPRESS_VERSION SNDRV_PROTOCOL_VERSION(0, 1, 1) /**

• struct snd_compressed_buffer: compressed buffer
• @fragment_size: size of buffer fragment in bytes

@@ -121,6 +121,32 @@ struct snd_compr_codec_caps { struct snd_codec_desc descriptor[MAX_NUM_CODEC_DESCRIPTORS]; };

+enum {

• SNDRV_COMPRESS_ENCODER_PADDING = 1,
• SNDRV_COMPRESS_ENCODER_DELAY = 2,

+};

+/**

• • struct snd_compr_keyvalue: compressed stream key/value pairs
• • @key: key id
• • @value: key value
• */

+struct snd_compr_keyvalue {

• __u32 key;
  

• __u32 value;
  

+};

+/**

• • struct snd_compr_metadata: compressed stream metadata
• • @count: number of keys
• • @keys: pointer to count keys
• */

+struct snd_compr_metadata {

• __u32 count;
• struct snd_compr_keyvalue *keys;

+};

Do you really want that? Then you'll have to provide 32/64bit ioctl conversion functions as well. I wouldn't take that mess but let user repeat single key/value ioctls.

Instead of relying completely on the driver side implementation, you can keep a metadata array in snd_compr_stream or snd_compr internally. Then the ioctl handler would be just like:

static int compr_stream_set_metadata(struct snd_compr_stream *stream, void __user * arg) { struct snd_compr_keyvalue kv;

if (!stream->ops->set_metadata) return -ENXIO; if (copy_from_user(&kv, arg, sizeof(kv))) return -EFAULT; if (kv > SNDRV_COMPRESS_METADATA_LAST) return -EINVAL; stream->metadata[kv.key] = kv.value; return stream->ops->set_metadata(stream); }

As a bonus, we can reduce even get_metadata ops implementation in each driver:

static int compr_stream_get_metadata(struct snd_compr_stream *stream, void __user * arg) { if (!stream->ops->set_metadata) return -ENXIO; if (copy_from_user(&kv, arg, sizeof(kv))) return -EFAULT; if (kv > SNDRV_COMPRESS_METADATA_LAST) return -EINVAL; kv.value = stream->metadata[kv.key]; if (copy_to_user(arg, &kv, sizeof(kv)) return -EFAULT; return 0; }

Of course, this assumes that stream->metadata[] is initialized at each open properly.

Takashi

/**

• compress path ioctl definitions
• SNDRV_COMPRESS_GET_CAPS: Query capability of DSP

@@ -145,6 +171,10 @@ struct snd_compr_codec_caps { struct snd_compr_codec_caps) #define SNDRV_COMPRESS_SET_PARAMS _IOW('C', 0x12, struct snd_compr_params) #define SNDRV_COMPRESS_GET_PARAMS _IOR('C', 0x13, struct snd_codec) +#define SNDRV_COMPRESS_SET_METADATA _IOW('C', 0x14,\

• 				 struct snd_compr_metadata)
  

+#define SNDRV_COMPRESS_GET_METADATA _IOW('C', 0x15,\

• 				 struct snd_compr_metadata)
  

#define SNDRV_COMPRESS_TSTAMP _IOR('C', 0x20, struct snd_compr_tstamp) #define SNDRV_COMPRESS_AVAIL _IOR('C', 0x21, struct snd_compr_avail) #define SNDRV_COMPRESS_PAUSE _IO('C', 0x30) @@ -152,10 +182,14 @@ struct snd_compr_codec_caps { #define SNDRV_COMPRESS_START _IO('C', 0x32) #define SNDRV_COMPRESS_STOP _IO('C', 0x33) #define SNDRV_COMPRESS_DRAIN _IO('C', 0x34) +#define SNDRV_COMPRESS_NEXT_TRACK _IO('C', 0x35) +#define SNDRV_COMPRESS_PARTIAL_DRAIN _IO('C', 0x36) /*

• TODO
• 1. add mmap support

*/ #define SND_COMPR_TRIGGER_DRAIN 7 /*FIXME move this to pcm.h */ +#define SND_COMPR_TRIGGER_NEXT_TRACK 8 +#define SND_COMPR_TRIGGER_PARTIAL_DRAIN 9 #endif diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index ad11dc9..4928209 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -262,9 +262,14 @@ static ssize_t snd_compr_write(struct file *f, const char __user *buf,

stream = &data->stream; mutex_lock(&stream->device->lock);

• /* write is allowed when stream is running or has been steup */

• /* write is allowed when stream is running or has been steup

• * also stream cna be written when next_track info has been setup
  

• * or its has partially drained
  

• */
  

  if (stream->runtime->state != SNDRV_PCM_STATE_SETUP &&

• 	stream->runtime->state != SNDRV_PCM_STATE_RUNNING) {
  

• 	stream->runtime->state != SNDRV_PCM_STATE_RUNNING &&
  

• 	stream->runtime->state != SNDRV_PCM_STATE_NEXT_TRACK &&
  

• 	stream->runtime->state != SNDRV_PCM_STATE_PARTIAL_DRAIN) {
  

  mutex_unlock(&stream->device->lock); return -EBADFD; }

@@ -288,6 +293,9 @@ static ssize_t snd_compr_write(struct file *f, const char __user *buf, stream->runtime->state = SNDRV_PCM_STATE_PREPARED; pr_debug("stream prepared, Houston we are good to go\n"); }

• if (stream->runtime->state == SNDRV_PCM_STATE_NEXT_TRACK ||

• 	stream->runtime->state == SNDRV_PCM_STATE_PARTIAL_DRAIN)
  

• stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
  
  

  mutex_unlock(&stream->device->lock); return retval;

@@ -514,6 +522,78 @@ out: return retval; }

+static int +snd_compr_copy_metadata(struct snd_compr_metadata **arg, unsigned long user) +{

• struct snd_compr_metadata _mdata, *mdata;
• int len;
• if (copy_from_user(&_mdata, (void __user *)user,

• 		sizeof(_mdata)))
  

• return -EFAULT;
  

• len = sizeof(_mdata.count) + _mdata.count * sizeof(*_mdata.keys);
• mdata = kmalloc(len, GFP_KERNEL);
• if (!mdata)

• return -ENOMEM;
  

• if (copy_from_user(mdata, (void __user *)arg, len)) {

• kfree(mdata);
  

• return -EFAULT;
  

• }
• *arg = mdata;
• return len;

+}

+static int +snd_compr_set_metadata(struct snd_compr_stream *stream, unsigned long arg) +{

• struct snd_compr_metadata *mdata;
• int retval;
• if (!stream->ops->set_metadata)

• return -ENXIO;
  

• /*
• • we should allow parameter change only when stream has been
• • opened not in other cases
• */
• retval = snd_compr_copy_metadata(&mdata, arg);
• if (retval <= 0)

• return retval;
  

• retval = stream->ops->set_metadata(stream, mdata);
• kfree(mdata);
• return retval;

+}

+static int +snd_compr_get_metadata(struct snd_compr_stream *stream, unsigned long arg) +{

• struct snd_compr_metadata *mdata;
• int retval, len;
• if (!stream->ops->get_metadata)

• return -ENXIO;
  

• len = snd_compr_copy_metadata(&mdata, arg);
• if (len <= 0)

• return len;
  

• retval = stream->ops->get_metadata(stream, mdata);
• if (retval != 0)

• goto out;
  

• if (copy_to_user((void __user *)arg, mdata, len)) {

• retval = -EFAULT;
  

• goto out;
  

• }

+out:

• kfree(mdata);
• return retval;

+}

static inline int snd_compr_tstamp(struct snd_compr_stream *stream, unsigned long arg) { @@ -594,6 +674,38 @@ static int snd_compr_drain(struct snd_compr_stream *stream) return retval; }

+static int snd_compr_partial_drain(struct snd_compr_stream *stream) +{

• int retval;
• /* we can partially drain streams, only when next track info has been

• * passed to the dsp
  

• */
  

• if (stream->runtime->state != SNDRV_PCM_STATE_NEXT_TRACK)

• return -EPERM;
  

• retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_PARTIAL_DRAIN);
• if (retval != 0)

• return retval;
  

• stream->runtime->state = SNDRV_PCM_STATE_PARTIAL_DRAIN;
• return 0;

+}

+static int snd_compr_next_track(struct snd_compr_stream *stream) +{

• int retval;
• /* only a running stream can transition to next track */
• if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING)

• return -EPERM;
  

• retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_NEXT_TRACK);
• if (retval != 0)

• return retval;
  

• stream->runtime->state = SNDRV_PCM_STATE_NEXT_TRACK;
• return 0;

+}

static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg) { struct snd_compr_file *data = f->private_data; @@ -623,6 +735,12 @@ static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg) case _IOC_NR(SNDRV_COMPRESS_GET_PARAMS): retval = snd_compr_get_params(stream, arg); break;

• case _IOC_NR(SNDRV_COMPRESS_SET_METADATA):

• retval = snd_compr_set_metadata(stream, arg);
  

• break;
  

• case _IOC_NR(SNDRV_COMPRESS_GET_METADATA):

• retval = snd_compr_get_metadata(stream, arg);
  

• break;
  

  case _IOC_NR(SNDRV_COMPRESS_TSTAMP): retval = snd_compr_tstamp(stream, arg); break;

@@ -644,6 +762,13 @@ static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned long arg) case _IOC_NR(SNDRV_COMPRESS_DRAIN): retval = snd_compr_drain(stream); break;

• case _IOC_NR(SNDRV_COMPRESS_PARTIAL_DRAIN):

• retval = snd_compr_partial_drain(stream);
  

• break;
  

• case _IOC_NR(SNDRV_COMPRESS_NEXT_TRACK):

• retval = snd_compr_next_track(stream);
  

• break;
  

• } mutex_unlock(&stream->device->lock); return retval;

-- 1.7.0.4