Re: [ALSA patch] [PATCH - alsa-lib 4/4] pcm_file: add infile read support for mmap mode

Re: [ALSA patch] [PATCH - alsa-lib 4/4] pcm_file: add infile read support for mmap mode

[Takashi Iwai]

On Mon, 20 May 2019 18:51:06 +0200,
Miartus, Adam (Arion Recruitment; ADITG/ESM) wrote:
> 
> From: Adam Miartus <amiartus@de.adit-jv.com>
> 
> mmap_begin callback is used to copy data from input file to mmaped buffer
> 
> guard for corner use of api (multiple mmap_begin calls by user) is introduced to check if next continuous buffer was already overwritten
> 
> buffer is overwritten with input file data only in case of stream capture
> 
> Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
> Reviewed-by: Timo Wischer <twischer@de.adit-jv.com>

Can't we copy the data in snd_pcm_file_mmap_commit() just like the
playback case?


thanks,

Takashi

Re: [ALSA patch] [PATCH - alsa-lib 4/4] pcm_file: add infile read support for mmap mode

[Miartus, Adam (Arion Recruitment; ADITG/ESM)]

> On Mon, 20 May 2019 18:51:06 +0200,
> Miartus, Adam (Arion Recruitment; ADITG/ESM) wrote:
> >
> > From: Adam Miartus <amiartus@de.adit-jv.com>
> >
> > mmap_begin callback is used to copy data from input file to mmaped
> > buffer
> >
> > guard for corner use of api (multiple mmap_begin calls by user) is
> > introduced to check if next continuous buffer was already overwritten
> >
> > buffer is overwritten with input file data only in case of stream
> > capture
> >
> > Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
> > Reviewed-by: Timo Wischer <twischer@de.adit-jv.com>
> 
> Can't we copy the data in snd_pcm_file_mmap_commit() just like the
> playback case?
> 
> 
> thanks,
> 
> Takashi

My understanding is that in case of reading data in mmap mode user
would call mmap_begin, read the buffer and then call mmap_commit.

In this case overwriting the buffer in mmap_commit with data from
Input file would be too late. 

Best regards,
Adam

Re: [ALSA patch] [PATCH - alsa-lib 4/4] pcm_file: add infile read support for mmap mode

[Takashi Iwai]

On Tue, 21 May 2019 10:37:43 +0200,
Miartus, Adam (Arion Recruitment; ADITG/ESM) wrote:
> 
> > On Mon, 20 May 2019 18:51:06 +0200,
> > Miartus, Adam (Arion Recruitment; ADITG/ESM) wrote:
> > >
> > > From: Adam Miartus <amiartus@de.adit-jv.com>
> > >
> > > mmap_begin callback is used to copy data from input file to mmaped
> > > buffer
> > >
> > > guard for corner use of api (multiple mmap_begin calls by user) is
> > > introduced to check if next continuous buffer was already overwritten
> > >
> > > buffer is overwritten with input file data only in case of stream
> > > capture
> > >
> > > Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
> > > Reviewed-by: Timo Wischer <twischer@de.adit-jv.com>
> > 
> > Can't we copy the data in snd_pcm_file_mmap_commit() just like the
> > playback case?
> > 
> > 
> > thanks,
> > 
> > Takashi
> 
> My understanding is that in case of reading data in mmap mode user
> would call mmap_begin, read the buffer and then call mmap_commit.
> 
> In this case overwriting the buffer in mmap_commit with data from
> Input file would be too late. 

Hm, OK.  I basically hate to add a new ops, but this might be the only
way if we have to implement that.

OTOH, is this mmap support mandatory?  IOW, is wrapping with the plug
and mmap_emul performance-wise so bad?


thanks,

Takashi

Re: [ALSA patch] [PATCH - alsa-lib 4/4] pcm_file: add infile read support for mmap mode

[Takashi Sakamoto]

Hi,

On Tue, May 21, 2019, at 17:52, Takashi Iwai wrote:
> On Tue, 21 May 2019 10:37:43 +0200,
> Miartus, Adam (Arion Recruitment; ADITG/ESM) wrote:
> > 
> > > On Mon, 20 May 2019 18:51:06 +0200,
> > > Miartus, Adam (Arion Recruitment; ADITG/ESM) wrote:
> > > >
> > > > From: Adam Miartus <amiartus@de.adit-jv.com>
> > > >
> > > > mmap_begin callback is used to copy data from input file to mmaped
> > > > buffer
> > > >
> > > > guard for corner use of api (multiple mmap_begin calls by user) is
> > > > introduced to check if next continuous buffer was already overwritten
> > > >
> > > > buffer is overwritten with input file data only in case of stream
> > > > capture
> > > >
> > > > Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
> > > > Reviewed-by: Timo Wischer <twischer@de.adit-jv.com>
> > > 
> > > Can't we copy the data in snd_pcm_file_mmap_commit() just like the
> > > playback case?
> > > 
> > > 
> > > thanks,
> > > 
> > > Takashi
> > 
> > My understanding is that in case of reading data in mmap mode user
> > would call mmap_begin, read the buffer and then call mmap_commit.
> > 
> > In this case overwriting the buffer in mmap_commit with data from
> > Input file would be too late. 
> 
> Hm, OK.  I basically hate to add a new ops, but this might be the only
> way if we have to implement that.

I'm interested in this patch, however the patch is not blasted by list server.
Would you please re-post your patchset so that the list server delivers them
to subscribers and I can see the contents?

> OTOH, is this mmap support mandatory?  IOW, is wrapping with the plug
> and mmap_emul performance-wise so bad?


Regards

Takashi Sakamoto

Re: [ALSA patch] [PATCH - alsa-lib 4/4] pcm_file: add infile read support for mmap mode

[Miartus, Adam (Arion Recruitment; ADITG/ESM)]

> On Tue, 21 May 2019 10:37:43 +0200,
> Miartus, Adam (Arion Recruitment; ADITG/ESM) wrote:
> >
> > > On Mon, 20 May 2019 18:51:06 +0200,
> > > Miartus, Adam (Arion Recruitment; ADITG/ESM) wrote:
> > > >
> > > > From: Adam Miartus <amiartus@de.adit-jv.com>
> > > >
> > > > mmap_begin callback is used to copy data from input file to mmaped
> > > > buffer
> > > >
> > > > guard for corner use of api (multiple mmap_begin calls by user) is
> > > > introduced to check if next continuous buffer was already overwritten
> > > >
> > > > buffer is overwritten with input file data only in case of stream
> > > > capture
> > > >
> > > > Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
> > > > Reviewed-by: Timo Wischer <twischer@de.adit-jv.com>
> > >
> > > Can't we copy the data in snd_pcm_file_mmap_commit() just like the
> > > playback case?
> > >
> > >
> > > thanks,
> > >
> > > Takashi
> >
> > My understanding is that in case of reading data in mmap mode user
> > would call mmap_begin, read the buffer and then call mmap_commit.
> >
> > In this case overwriting the buffer in mmap_commit with data from
> > Input file would be too late.
> 
> Hm, OK.  I basically hate to add a new ops, but this might be the only
> way if we have to implement that.
> 
> OTOH, is this mmap support mandatory?  IOW, is wrapping with the plug
> and mmap_emul performance-wise so bad?
> 
> 
> thanks,
> 
> Takashi

I understand that extending the api is not ideal, however this approach
in my opinion, simplifies the solution. API could also be used to simplify
code in softvol plugin, which to my understanding loops around the buffer
in avail_update callback. Maybe other plugins could benefit as well.

As for emulation of mmap I was not aware it is possible. We would like to
use the file plugin in mmap mode as additional tool for debugging hw/sw,
with option to overwrite the mmap buffer in read mode with data from file,
in this case I think it would be preferred to access mmap area without
emulation. However, if this can be accomplished without api change just
by alsa configuration, perhaps there is no need for this change.

best regards,

Adam

Re: [ALSA patch] FW: [PATCH - alsa-lib 1/4] pcm_file: add support for infile reading in non interleaved mode

[Takashi Iwai]

On Mon, 20 May 2019 18:49:34 +0200,
Miartus, Adam (Arion Recruitment; ADITG/ESM) wrote:
> 
> From: Adam Miartus <amiartus@de.adit-jv.com>
> 
> add helper function to copy input file data to buffer mapped by areas, in case of an error, do not fill the areas, allowing device read buffer to be provided to api caller
> 
> previously unused rbuf variable is reused for this purpose
> 
> Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
> Reviewed-by: Timo Wischer <twischer@de.adit-jv.com>
> 
> diff --git a/src/pcm/pcm_file.c b/src/pcm/pcm_file.c index 3a19cef..7998b64 100644
> --- a/src/pcm/pcm_file.c
> +++ b/src/pcm/pcm_file.c
> @@ -77,6 +77,7 @@ typedef struct {
>  	snd_pcm_uframes_t appl_ptr;
>  	snd_pcm_uframes_t file_ptr_bytes;
>  	snd_pcm_uframes_t wbuf_size;
> +	snd_pcm_uframes_t rbuf_size;
>  	size_t wbuf_size_bytes;
>  	size_t wbuf_used_bytes;
>  	char *wbuf;
> @@ -266,6 +267,37 @@ static int snd_pcm_file_open_output_file(snd_pcm_file_t *file)
>  	return 0;
>  }
>  
> +/* fill areas with data from input file, return bytes red */ static int 
> +snd_pcm_file_areas_read_infile(snd_pcm_t *pcm, const snd_pcm_channel_area_t *areas,
> +	snd_pcm_uframes_t offset, snd_pcm_uframes_t frames) {

Please follow the standard coding style.

> +	snd_pcm_file_t *file = pcm->private_data;
> +	snd_pcm_channel_area_t areas_if[pcm->channels];
> +	ssize_t bytes;
> +
> +	if (file->ifd < 0)
> +		return -EBADF;
> +
> +	if (file->rbuf == NULL)
> +		return -ENOMEM;
> +
> +	if (file->rbuf_size < frames) {
> +		SYSERR("requested more frames than pcm buffer");
> +		return -ENOMEM;
> +	}
> +
> +	bytes = read(file->ifd, file->rbuf, snd_pcm_frames_to_bytes(pcm, frames));
> +	if (bytes < 0) {
> +		SYSERR("read from file failed, error: %d", bytes);
> +		return bytes;
> +	}
> +
> +	snd_pcm_areas_from_buf(pcm, areas_if, file->rbuf);
> +	snd_pcm_areas_copy(areas, offset, areas_if, 0, pcm->channels, 
> +snd_pcm_bytes_to_frames(pcm, bytes), pcm->format);

Wrong indentation.


thanks,

Takashi

Re: [ALSA patch] FW: [PATCH - alsa-lib 1/4] pcm_file: add support for infile reading in non interleaved mode

[Miartus, Adam (Arion Recruitment; ADITG/ESM)]

> On Mon, 20 May 2019 18:49:34 +0200,
> Miartus, Adam (Arion Recruitment; ADITG/ESM) wrote:
> >
> > From: Adam Miartus <amiartus@de.adit-jv.com>
> >
> > add helper function to copy input file data to buffer mapped by areas, in
> case of an error, do not fill the areas, allowing device read buffer to be
> provided to api caller
> >
> > previously unused rbuf variable is reused for this purpose
> >
> > Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
> > Reviewed-by: Timo Wischer <twischer@de.adit-jv.com>
> >
> > diff --git a/src/pcm/pcm_file.c b/src/pcm/pcm_file.c index
> 3a19cef..7998b64 100644
> > --- a/src/pcm/pcm_file.c
> > +++ b/src/pcm/pcm_file.c
> > @@ -77,6 +77,7 @@ typedef struct {
> >  	snd_pcm_uframes_t appl_ptr;
> >  	snd_pcm_uframes_t file_ptr_bytes;
> >  	snd_pcm_uframes_t wbuf_size;
> > +	snd_pcm_uframes_t rbuf_size;
> >  	size_t wbuf_size_bytes;
> >  	size_t wbuf_used_bytes;
> >  	char *wbuf;
> > @@ -266,6 +267,37 @@ static int
> snd_pcm_file_open_output_file(snd_pcm_file_t *file)
> >  	return 0;
> >  }
> >
> > +/* fill areas with data from input file, return bytes red */ static int
> > +snd_pcm_file_areas_read_infile(snd_pcm_t *pcm, const
> snd_pcm_channel_area_t *areas,
> > +	snd_pcm_uframes_t offset, snd_pcm_uframes_t frames) {
> 
> Please follow the standard coding style.
> 
> > +	snd_pcm_file_t *file = pcm->private_data;
> > +	snd_pcm_channel_area_t areas_if[pcm->channels];
> > +	ssize_t bytes;
> > +
> > +	if (file->ifd < 0)
> > +		return -EBADF;
> > +
> > +	if (file->rbuf == NULL)
> > +		return -ENOMEM;
> > +
> > +	if (file->rbuf_size < frames) {
> > +		SYSERR("requested more frames than pcm buffer");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	bytes = read(file->ifd, file->rbuf, snd_pcm_frames_to_bytes(pcm,
> frames));
> > +	if (bytes < 0) {
> > +		SYSERR("read from file failed, error: %d", bytes);
> > +		return bytes;
> > +	}
> > +
> > +	snd_pcm_areas_from_buf(pcm, areas_if, file->rbuf);
> > +	snd_pcm_areas_copy(areas, offset, areas_if, 0, pcm->channels,
> > +snd_pcm_bytes_to_frames(pcm, bytes), pcm->format);
> 
> Wrong indentation.
> 
> 
> thanks,
> 
> Takashi

Sorry, my email client messed up the whitespace. I re-sent the patch using git send-mail.

Best Regards,

Adam

Re: [ALSA patch] FW: [PATCH - alsa-lib 1/4] pcm_file: add support for infile reading in non interleaved mode

[Takashi Iwai]

On Tue, 21 May 2019 13:48:46 +0200,
Miartus, Adam (Arion Recruitment; ADITG/ESM) wrote:
> 
> > On Mon, 20 May 2019 18:49:34 +0200,
> > Miartus, Adam (Arion Recruitment; ADITG/ESM) wrote:
> > >
> > > From: Adam Miartus <amiartus@de.adit-jv.com>
> > >
> > > add helper function to copy input file data to buffer mapped by areas, in
> > case of an error, do not fill the areas, allowing device read buffer to be
> > provided to api caller
> > >
> > > previously unused rbuf variable is reused for this purpose
> > >
> > > Signed-off-by: Adam Miartus <amiartus@de.adit-jv.com>
> > > Reviewed-by: Timo Wischer <twischer@de.adit-jv.com>
> > >
> > > diff --git a/src/pcm/pcm_file.c b/src/pcm/pcm_file.c index
> > 3a19cef..7998b64 100644
> > > --- a/src/pcm/pcm_file.c
> > > +++ b/src/pcm/pcm_file.c
> > > @@ -77,6 +77,7 @@ typedef struct {
> > >  	snd_pcm_uframes_t appl_ptr;
> > >  	snd_pcm_uframes_t file_ptr_bytes;
> > >  	snd_pcm_uframes_t wbuf_size;
> > > +	snd_pcm_uframes_t rbuf_size;
> > >  	size_t wbuf_size_bytes;
> > >  	size_t wbuf_used_bytes;
> > >  	char *wbuf;
> > > @@ -266,6 +267,37 @@ static int
> > snd_pcm_file_open_output_file(snd_pcm_file_t *file)
> > >  	return 0;
> > >  }
> > >
> > > +/* fill areas with data from input file, return bytes red */ static int
> > > +snd_pcm_file_areas_read_infile(snd_pcm_t *pcm, const
> > snd_pcm_channel_area_t *areas,
> > > +	snd_pcm_uframes_t offset, snd_pcm_uframes_t frames) {
> > 
> > Please follow the standard coding style.
> > 
> > > +	snd_pcm_file_t *file = pcm->private_data;
> > > +	snd_pcm_channel_area_t areas_if[pcm->channels];
> > > +	ssize_t bytes;
> > > +
> > > +	if (file->ifd < 0)
> > > +		return -EBADF;
> > > +
> > > +	if (file->rbuf == NULL)
> > > +		return -ENOMEM;
> > > +
> > > +	if (file->rbuf_size < frames) {
> > > +		SYSERR("requested more frames than pcm buffer");
> > > +		return -ENOMEM;
> > > +	}
> > > +
> > > +	bytes = read(file->ifd, file->rbuf, snd_pcm_frames_to_bytes(pcm,
> > frames));
> > > +	if (bytes < 0) {
> > > +		SYSERR("read from file failed, error: %d", bytes);
> > > +		return bytes;
> > > +	}
> > > +
> > > +	snd_pcm_areas_from_buf(pcm, areas_if, file->rbuf);
> > > +	snd_pcm_areas_copy(areas, offset, areas_if, 0, pcm->channels,
> > > +snd_pcm_bytes_to_frames(pcm, bytes), pcm->format);
> > 
> > Wrong indentation.
> > 
> > 
> > thanks,
> > 
> > Takashi
> 
> Sorry, my email client messed up the whitespace. I re-sent the patch using git send-mail.

Could you resubmit the whole patch set, at best with a cover letter?


thanks,

Takashi