[alsa-devel] [PATCH 5/6] compress: add the core file

[Takashi Iwai]

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 at linux.intel.com>
> > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart at 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