At Thu, 24 Nov 2011 09:15:54 +0530, Vinod Koul wrote:
On Wed, 2011-11-23 at 18:03 +0100, Takashi Iwai wrote:
At Tue, 22 Nov 2011 14:21:59 +0530, Vinod Koul wrote:
This patch adds core.c, the file which implements the ioctls and registers the devices
Signed-off-by: Vinod Koul vinod.koul@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
A general note: it's not necessarily to split playback and capture devices. For example, rawmidi has a single device for full-duplex. In the case of PCM, we had to split them because of mmap. The mmap for the capture was supposed not to be read-only, some apps may want to add mark on the buffer. But, if this isn't the case for compression devices, you can have a single device serving for both playback and capture. The direction can be checked by the open flag.
Yes we can have them common. I went with usualy convention of having devices as pcmCxDxp translated to comprCxDxp.
If you don't mind we can keep it as is for now, and once we have capture, mmap and other support built and tested we can merge these?
Well, such a decision should be done as early as possible. Changing the device file assignment means to change the interface to the user-space. So, this is not allowed once when the code is merge to the upstream.
That said, if you want to fiddle it in your tree, I'm fine. But, until the API is fixed, the code can be never merged.
I'm OK to keep the direction-separated devices if it's mandatory, but note that it's a special handling only for ALSA PCM. (OSS PCM doesn't need it, too.)
+#define FORMAT(fmt) "%s: %d: " fmt, __func__, __LINE__ +#define pr_fmt(fmt) KBUILD_MODNAME ": " FORMAT(fmt)
Is this referred anywhere?
This is trick I picked up :) pr_err/debug is define as pr_fmt. These just add the module name, function and line number to what you are printing
Hm, but this also change other pr_*(). If you want only the debug prints, you can use snd_printdd(). This is compiled conditionally, and can show the file, the function and the line number.
+static int snd_compr_open(struct file *f, struct snd_compr *compr, int type) +{
- struct snd_compr_file *data;
- struct snd_compr_runtime *runtime;
- int ret;
- mutex_lock(&device_mutex);
Do you need a global lock for opening a device, too? Just to be sure, since I see no obvious need. (Anyway you don't need to have a lock at this early place before a bunch of kmallocs. The lock should be done as small as possible.)
Thinking on this lock should be only for device open call. That way we prevent race of two opens.
Do you mean the race of opening the same device or among (all) different devices? For the first case, the lock should be device-specific. For the latter case, I don't understand why.
+static int snd_compr_free(struct inode *inode, struct file *f) +{
- struct snd_compr_file *data = f->private_data;
- mutex_lock(&device_mutex);
- data->stream.ops->free(&data->stream);
- kfree(data->stream.runtime->buffer);
- kfree(data->stream.runtime);
- kfree(data);
- mutex_unlock(&device_mutex);
Ditto. Do you need a global lock?
nope
+static int +snd_compr_ioctl_avail(struct snd_compr_stream *stream, unsigned long arg) +{
- struct snd_compr_avail ioctl_avail;
- snd_compr_calc_avail(stream, &ioctl_avail);
- if (copy_to_user((unsigned long __user *)arg,
&ioctl_avail, sizeof(ioctl_avail)))snd_compr_calc_avail() doesn't initialize the pointer, so this may leak some kernel space.
Not sure if I understood this snd_compr_calc_avail() will go and get the tstamp updated from driver by passing the tstamp structure inside this, calculate avail and update that value.
But snd_compr_calc_avail() may return without filling avail->avail, e.g.
if (stream->runtime->bytes_written == 0 && stream->runtime->state == SNDRV_PCM_STATE_SETUP) { pr_debug("detected init and someone forgot to do a write\n"); return stream->runtime->buffer_size; }
So, this leaks the uninitialized space.
+void snd_compr_fragment_elapsed(struct snd_compr_stream *stream) +{
- pr_debug("fragment elapsed called\n");
- if (stream->direction != SNDRV_DEVICE_TYPE_COMPR_PLAYBACK)
return;- wake_up(&stream->runtime->sleep);
+} +EXPORT_SYMBOL_GPL(snd_compr_fragment_elapsed);
+void snd_compr_frame_elapsed(struct snd_compr_stream *stream) +{
- if (stream->direction != SNDRV_DEVICE_TYPE_COMPR_CAPTURE)
return;- wake_up(&stream->runtime->sleep);
+} +EXPORT_SYMBOL_GPL(snd_compr_frame_elapsed);
These are better to be inlined... (And fragment and frame for different directions? Hm, not so intuitive :)
Well we hve different notions for playback and capture. In playback, the application can set any random fragment size and dsp will send a notification by calling snd_compr_fragment_elapsed. But in case of capture, we would want to get data in frames, but application may not have a clue on what is the frame size. So once it has encoded one frame and copied to buffer, it should update its time stamp and call snd_compr_frame_elapsed. And application can know where frame boundary is and read data per frame basis.
The biggest problem is the conflict of terminology. What do you mean as "frame" and what do you mean as "fragment" in your implementation? At least, the term "frame" is already used in ALSA PCM, and I'm not sure whether you use this term for the very same meaning in the above context...
And, another question is how you can guarantee that the capture works in that way. What happens if a hardware updates the data only in the fragments level even for the capture?
- mutex_lock(&stream->device->lock);
- switch (_IOC_NR(cmd)) {
- case _IOC_NR(SNDRV_COMPRESS_IOCTL_VERSION):
put_user(SNDRV_COMPRESS_VERSION,(int __user *)arg) ? -EFAULT : 0;break;- case _IOC_NR(SNDRV_COMPRESS_GET_CAPS):
retval = snd_compr_get_caps(stream, arg);break;- case _IOC_NR(SNDRV_COMPRESS_GET_CODEC_CAPS):
retval = snd_compr_get_codec_caps(stream, arg);break;- case _IOC_NR(SNDRV_COMPRESS_SET_PARAMS):
retval = snd_compr_set_params(stream, arg);break;- case _IOC_NR(SNDRV_COMPRESS_GET_PARAMS):
retval = snd_compr_get_params(stream, arg);break;- case _IOC_NR(SNDRV_COMPRESS_TSTAMP):
retval = snd_compr_tstamp(stream, arg);break;- case _IOC_NR(SNDRV_COMPRESS_AVAIL):
retval = snd_compr_ioctl_avail(stream, arg);break;- case _IOC_NR(SNDRV_COMPRESS_PAUSE):
retval = snd_compr_pause(stream);break;- case _IOC_NR(SNDRV_COMPRESS_RESUME):
retval = snd_compr_resume(stream);break;- case _IOC_NR(SNDRV_COMPRESS_START):
retval = snd_compr_start(stream);break;- case _IOC_NR(SNDRV_COMPRESS_STOP):
retval = snd_compr_stop(stream);break;- case _IOC_NR(SNDRV_COMPRESS_DRAIN):
cmd = SND_COMPR_TRIGGER_DRAIN;What's this?
So we have two notions of stopping the stream.
Yes, but the substitution of cmd is useless in your code:
+ case _IOC_NR(SNDRV_COMPRESS_DRAIN): + cmd = SND_COMPR_TRIGGER_DRAIN; + retval = snd_compr_drain(stream); + break; + } + mutex_unlock(&stream->device->lock); + return retval; +}
thanks,
Takashi