From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH v2] Fix forward/rewind support in iec958 plugin Date: Mon, 28 Apr 2014 18:10:56 +0200 Message-ID: References: <5358F356.3020701@canonical.com> <1398350406-10347-1-git-send-email-patrakov@gmail.com> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx2.suse.de (cantor2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id 5E7522619F0 for ; Mon, 28 Apr 2014 18:10:56 +0200 (CEST) In-Reply-To: <1398350406-10347-1-git-send-email-patrakov@gmail.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: "Alexander E. Patrakov" Cc: alsa-devel@alsa-project.org List-Id: alsa-devel@alsa-project.org At Thu, 24 Apr 2014 20:40:06 +0600, Alexander E. Patrakov wrote: > > When forwarding or rewinding, the frame counter was not updated. This > could result in corrupted channel status words or misplaced Z-type > preamble. > > Signed-off-by: Alexander E. Patrakov > --- > v2: As suggested by David Henningsson, patch a copy of > snd_pcm_plugin_fast_ops at runtime instead of making all plugin finctions > non-static. But, contrary to his proposal, I made fast_ops just a part of > struct snd_pcm_iec958, because the needed lifetime is exactly the same. > > I still have no hardware to test this, and I strongly suspect that the > patch is wrong, because nobody else uses dynamic allocation of fast_ops. > A potentially suspicious operation would be to place a plug on top of this, > because plug copies fast_ops around. However, each time it closes a slave, > it also resets fast_ops to something sane, or at least I think so. The embedded fast_ops should work fine, so I applied the patch as is now. Let's see whether it really fixes the issues properly... thanks, Takashi > > src/pcm/pcm_iec958.c | 37 ++++++++++++++++++++++++++++++++++++- > src/pcm/pcm_plugin.c | 4 ++-- > src/pcm/pcm_plugin.h | 6 ++++++ > 3 files changed, 44 insertions(+), 3 deletions(-) > > diff --git a/src/pcm/pcm_iec958.c b/src/pcm/pcm_iec958.c > index d81b0a1..0c61fc1 100644 > --- a/src/pcm/pcm_iec958.c > +++ b/src/pcm/pcm_iec958.c > @@ -62,6 +62,7 @@ struct snd_pcm_iec958 { > unsigned char status[24]; > unsigned int byteswap; > unsigned char preamble[3]; /* B/M/W or Z/X/Y */ > + snd_pcm_fast_ops_t fops; > }; > > enum { PREAMBLE_Z, PREAMBLE_X, PREAMBLE_Y }; > @@ -416,6 +417,35 @@ static void snd_pcm_iec958_dump(snd_pcm_t *pcm, snd_output_t *out) > snd_pcm_dump(iec->plug.gen.slave, out); > } > > +static snd_pcm_sframes_t snd_pcm_iec958_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t frames) > +{ > + unsigned int counter_decrement; > + snd_pcm_iec958_t *iec = pcm->private_data; > + snd_pcm_sframes_t result = snd_pcm_plugin_rewind(pcm, frames); > + if (result <= 0) > + return result; > + > + counter_decrement = result % 192; > + iec->counter += 192 - counter_decrement; > + iec->counter %= 192; > + return result; > +} > + > +static snd_pcm_sframes_t snd_pcm_iec958_forward(snd_pcm_t *pcm, snd_pcm_uframes_t frames) > + > +{ > + unsigned int counter_increment; > + snd_pcm_iec958_t *iec = pcm->private_data; > + snd_pcm_sframes_t result = snd_pcm_plugin_rewind(pcm, frames); > + if (result <= 0) > + return result; > + > + counter_increment = result % 192; > + iec->counter += counter_increment; > + iec->counter %= 192; > + return result; > +} > + > static const snd_pcm_ops_t snd_pcm_iec958_ops = { > .close = snd_pcm_generic_close, > .info = snd_pcm_generic_info, > @@ -495,7 +525,12 @@ int snd_pcm_iec958_open(snd_pcm_t **pcmp, const char *name, snd_pcm_format_t sfo > return err; > } > pcm->ops = &snd_pcm_iec958_ops; > - pcm->fast_ops = &snd_pcm_plugin_fast_ops; > + > + iec->fops = snd_pcm_plugin_fast_ops; > + iec->fops.rewind = snd_pcm_iec958_rewind; > + iec->fops.forward = snd_pcm_iec958_forward; > + pcm->fast_ops = &iec->fops; > + > pcm->private_data = iec; > pcm->poll_fd = slave->poll_fd; > pcm->poll_events = slave->poll_events; > diff --git a/src/pcm/pcm_plugin.c b/src/pcm/pcm_plugin.c > index 17157e8..4ddf10c 100644 > --- a/src/pcm/pcm_plugin.c > +++ b/src/pcm/pcm_plugin.c > @@ -201,7 +201,7 @@ static snd_pcm_sframes_t snd_pcm_plugin_rewindable(snd_pcm_t *pcm) > return snd_pcm_mmap_hw_avail(pcm); > } > > -static snd_pcm_sframes_t snd_pcm_plugin_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t frames) > +snd_pcm_sframes_t snd_pcm_plugin_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t frames) > { > snd_pcm_plugin_t *plugin = pcm->private_data; > snd_pcm_sframes_t n = snd_pcm_mmap_hw_avail(pcm); > @@ -229,7 +229,7 @@ static snd_pcm_sframes_t snd_pcm_plugin_forwardable(snd_pcm_t *pcm) > return snd_pcm_mmap_avail(pcm); > } > > -static snd_pcm_sframes_t snd_pcm_plugin_forward(snd_pcm_t *pcm, snd_pcm_uframes_t frames) > +snd_pcm_sframes_t snd_pcm_plugin_forward(snd_pcm_t *pcm, snd_pcm_uframes_t frames) > { > snd_pcm_plugin_t *plugin = pcm->private_data; > snd_pcm_sframes_t n = snd_pcm_mmap_avail(pcm); > diff --git a/src/pcm/pcm_plugin.h b/src/pcm/pcm_plugin.h > index 7ee7c7f..19e82c3 100644 > --- a/src/pcm/pcm_plugin.h > +++ b/src/pcm/pcm_plugin.h > @@ -58,8 +58,14 @@ typedef struct { > snd1_pcm_plugin_undo_read_generic > #define snd_pcm_plugin_undo_write_generic \ > snd1_pcm_plugin_undo_write_generic > +#define snd_pcm_plugin_rewind \ > + snd1_pcm_plugin_rewind > +#define snd_pcm_plugin_forward \ > + snd1_pcm_plugin_forward > > void snd_pcm_plugin_init(snd_pcm_plugin_t *plugin); > +snd_pcm_sframes_t snd_pcm_plugin_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t frames); > +snd_pcm_sframes_t snd_pcm_plugin_forward(snd_pcm_t *pcm, snd_pcm_uframes_t frames); > > extern const snd_pcm_fast_ops_t snd_pcm_plugin_fast_ops; > > -- > 1.9.2 > > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel >