[alsa-devel] [PATCH v4 5/6] core: add support for compress_offload

[Nallasellan, Singaravelan]

> 
> Please wrap your emails within 80 chars, you have been repeatedly asked to do so
> on this on list. I don't understand how hard it is to follow basic list etiquettes. Its
> difficult to read screwed up formatting
> 
Yes sir. Apologize for the mistake.

> > If someone changes the device mode, this is screwed though there are
> > two types of (PLAYBACK and CAPTURE) devices are registered?
> > Would be better if there are two separate playback and  capture
> > operations registered separately.
> Again, please read the related discussion on the list.
> 
> Compressed devices will not have direction associated with them . They can be
> opened in any mode depending on the mode the DSP supports.
> So /dev/snd/comprC0D0 can be either playback or capture device.
> So a device can be ONLY opened with O_WRONLY or O_RDONLY.
> 
Nice, Does it look like a hack hack hack...
> 
> > O_NONBLOCK is not tested here for non-blocking support.
> Why should it be?
> 1) Any blocking and non blocking should be handled in user space
> 2) Nevertheless, that is not supported, HINT: read the documentation
> 
This is not as per the write system call definition as I understand.  Look like we redefine the
system call here. Support it in the standard way.
> > > +	if (stream->runtime->state != SNDRV_PCM_STATE_SETUP &&
> > > +			stream->runtime->state != SNDRV_PCM_STATE_RUNNING)
> > > {
> > Cant we call write again when the stream is in
> > SNDRV_PCM_STATE_PREPARED state?
> > This code allows only one write after the stream is in
> > SNDRV_PCM_STATE_SETUP state.
> > What if someone wants to write multiple smaller fragments before the
> > start?
> Yes currently write is allowed only once and we *normally* expect a START call. I am
> not sure why someone would need to call "smaller"
> writes multiple times. Remember this is compressed offload where we ideally want
> larger chunks of data otherwise there is no point in using this.
The code allows this to happen.  Is there any minimum size documented anywhere? 

> > > +static unsigned int snd_compr_poll(struct file *f, poll_table *wait) {
> > > +	struct snd_compr_file *data = f->private_data;
> > > +	struct snd_compr_stream *stream;
> > > +	size_t avail;
> > > +	int retval = 0;
> > > +
> > > +	BUG_ON(!data);
> > > +	stream = &data->stream;
> >
> > Can you validate O_NONBLOCK before going into poll_wait?
> > I guess this has to return -EINVAL if the device is not opened with
> > O_NONBLOCK.
> See above
Not a write implementation. 
> 
> 
> > What is the maximum number of fragments supported?
> Whatever dsp supports
IMHO, compress-core needs to query it from each driver and allocate memory 
only if the supported fragments is greater than requested fragments. 
> 
> > Can we check the fragments to that limit?
> Yes, can be checked here as well but I prefer to do these checks in userspace library
Think the driver needs to check and return error if it is not within the maximum
supported by the driver.
> 
> 
> > <Sing>: Can you check stream->ops->set_params for NULL as it is done
> > in the get_params function?
> No set_params is mandatory function to be implemented
Ok. It is my miss. 
> 
> --
> ~Vinod