Re: [PATCH] ALSA: compress: fix drain calls blocking other compress functions

[Vinod Koul <vinod.koul@intel.com>]

On Thu, Oct 24, 2013 at 12:59:18PM +0200, Takashi Iwai wrote:
> At Thu, 24 Oct 2013 14:57:24 +0530,
> Vinod Koul wrote:
> > 
> > On Thu, Oct 24, 2013 at 11:25:35AM +0200, Takashi Iwai wrote:
> > > At Thu, 24 Oct 2013 13:05:41 +0530,
> > > Vinod Koul wrote:
> > > > 
> > > > The drain and drain_notify callback were blocked by low level driver untill the
> > > > draining was complete. Due to this being invoked with big fat mutex held, others
> > > > ops like reading timestamp, calling pause, drop were blocked.
> > > > 
> > > > So to fix this we add a new snd_compr_drain_notify() API. This would be required
> > > > to be invoked by low level driver when drain or partial drain has been completed
> > > > by the DSP. Thus we make the drain and partial_drain callback as non blocking
> > > > and driver returns immediately after notifying DSP.
> > > > The waiting is done while relasing the lock so that other ops can go ahead.
> > > > 
> > > > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > > > CC: stable@vger.kernel.org
> > > > ---
> > > > v2:
> > > >  fix the 80 line warn
> > > >  move the state change to compress_drain()
> > > > 
> > > >  include/sound/compress_driver.h |   12 ++++++++++
> > > >  sound/core/compress_offload.c   |   43 ++++++++++++++++++++++++++++++++++++--
> > > >  2 files changed, 52 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
> > > > index 9031a26..eff5f84 100644
> > > > --- a/include/sound/compress_driver.h
> > > > +++ b/include/sound/compress_driver.h
> > > > @@ -48,6 +48,8 @@ struct snd_compr_ops;
> > > >   *	the ring buffer
> > > >   * @total_bytes_transferred: cumulative bytes transferred by offload DSP
> > > >   * @sleep: poll sleep
> > > > + * @wait: drain wait queuq
> > > > + * @draining: draining wait condition
> > > 
> > > What does this mean exactly?  I thought draining = 1 meaning that the
> > > stream is draining (i.e. waiting for the finish notification), but
> > > it looks opposite in your code below...
> > What i meant was the draining wait conditions which would be used for waiting.
> > Before wait the caller will set to 0 and on completion the callback will set to
> > 1 and wake up.
> 
> Well, it's not so intuitive as the flag being named "draining".
> Please give a more detailed comment (or name it a bit less-confusing
> one :)
hmmm, yes it needs a better name :)
perhaps drain_condition or drain_wake would be apt!
 
> > > > +
> > > > +	wake_up(&stream->runtime->sleep);
> > > > +	mutex_lock(&stream->device->lock);
> > > > +
> > > > +	return retval;
> > > > +}
> > > > +
> > > >  static int snd_compr_drain(struct snd_compr_stream *stream)
> > > >  {
> > > >  	int retval;
> > > > @@ -695,11 +720,17 @@ static int snd_compr_drain(struct snd_compr_stream *stream)
> > > >  	if (stream->runtime->state == SNDRV_PCM_STATE_PREPARED ||
> > > >  			stream->runtime->state == SNDRV_PCM_STATE_SETUP)
> > > >  		return -EPERM;
> > > > +
> > > > +	stream->runtime->draining = 0;
> > > >  	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN);
> > > > -	if (!retval) {
> > > > -		stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
> > > > +	if (retval) {
> > > > +		pr_err("SND_COMPR_TRIGGER_DRAIN failed %d\n", retval);
> > > >  		wake_up(&stream->runtime->sleep);
> > > > +		return retval;
> > > >  	}
> > > > +
> > > > +	retval = snd_compress_wait_for_drain(stream);
> > > > +	stream->runtime->state = SNDRV_PCM_STATE_SETUP;
> > > 
> > > So, the runtime state goes to SETUP even if wait_for_drain() returns
> > > an error?  In other words, compress core expects that the driver takes
> > > care of the proper state change in the error handling?
> > I guess it would make sense to communicate error. I havent added notion of
> > status on drain complete. By adding another status flag in the callback we cna
> > let driver tell us if error occured. Then pass the error in draining value and
> > do the state change accordingly.
> 
> I think it's fine as in the current patch, I just wanted to make sure
> that this runtime state change is the designed behavior.  If so, it'd
> be better to document / comment clearly.
Sure, lets add that explictly!

-- 
~Vinod