From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vinod Koul Subject: Re: [PATCH] ALSA: compress: fix drain calls blocking other compress functions Date: Thu, 24 Oct 2013 16:05:05 +0530 Message-ID: <20131024103505.GX14013@intel.com> References: <1382545021-26918-1-git-send-email-vinod.koul@intel.com> <1382600141-9866-1-git-send-email-vinod.koul@intel.com> <20131024092724.GW14013@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by alsa0.perex.cz (Postfix) with ESMTP id 5E3A0261A3A for ; Thu, 24 Oct 2013 13:29:14 +0200 (CEST) Content-Disposition: inline In-Reply-To: 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: Takashi Iwai Cc: alsa-devel@alsa-project.org, broonie@kernel.org, lgirdwood@gmail.com List-Id: alsa-devel@alsa-project.org 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 > > > > 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