Re: [PATCH] compress: add support for gapless playback

[Takashi Iwai <tiwai@suse.de>]

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
>