[Sound-open-firmware] [PATCH 25/25] component: host/volume/mixer: reset buffers for stop cmd

[Jie, Yang]

> -----Original Message-----
> From: Liam Girdwood [mailto:liam.r.girdwood at linux.intel.com]
> Sent: Wednesday, February 8, 2017 8:17 PM
> To: Keyon Jie <yang.jie at linux.intel.com>
> Cc: sound-open-firmware at alsa-project.org; Jie, Yang <yang.jie at intel.com>;
> Ingalsuo, Seppo <seppo.ingalsuo at intel.com>
> Subject: Re: [Sound-open-firmware] [PATCH 25/25] component:
> host/volume/mixer: reset buffers for stop cmd
> 
> On Tue, 2017-02-07 at 22:03 +0800, Keyon Jie wrote:
> > For command stop, excepting reset components, we may need reset the
> > buffer next to the component, otherwise, if the next start command
> > comes(without reset before it), the buffer pointers may be disordered.
> >
> > Signed-off-by: Keyon Jie <yang.jie at linux.intel.com>
> > ---
> >  src/audio/host.c   | 18 ++++++++++++++++++
> >  src/audio/mixer.c  | 31 ++++++++++++++++++++++++++++++-
> > src/audio/volume.c | 27 ++++++++++++++++++++++++++-
> >  3 files changed, 74 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/audio/host.c b/src/audio/host.c index
> > 28b5e7b..07f4953 100644
> > --- a/src/audio/host.c
> > +++ b/src/audio/host.c
> > @@ -656,6 +656,7 @@ static int host_stop(struct comp_dev *dev)  {
> >  	struct host_data *hd = comp_get_drvdata(dev);
> >  	struct dma_sg_elem *source_elem, *sink_elem, *local_elem;
> > +	struct list_item *clist;
> >
> >  	/* reset buffer pointers */
> >  	if (hd->host_pos)
> > @@ -689,6 +690,23 @@ static int host_stop(struct comp_dev *dev)
> >  	local_elem->src = source_elem->src;
> >  	hd->next_inc = hd->period->size;
> >
> > +	/* now reset downstream buffer */
> > +	list_for_item(clist, &dev->bsink_list) {
> > +		struct comp_buffer *buffer;
> > +
> > +		buffer = container_of(clist, struct comp_buffer, source_list);
> > +
> > +		/* dont reset buffer if the component is not connected */
> > +		if (!buffer->connected)
> > +			continue;
> > +		/* reset buffer next to the component*/
> > +		bzero(buffer->addr, buffer->desc.size);
> > +		buffer->w_ptr = buffer->r_ptr = buffer->addr;
> > +		buffer->end_addr = buffer->addr + buffer->desc.size;
> > +		buffer->free = buffer->desc.size;
> > +		buffer->avail = 0;
> 
> I'm seeing this reset code duplicated in a lot of places. lets create a new
> component API call that does this reset.

OK, I will create inline one comp_buffer_stop() and  put it component.h?

Thanks,
~Keyon

> 
> > +	}
> > +
> >  	dev->state = COMP_STATE_SETUP;
> >  	return 0;
> >  }
> > diff --git a/src/audio/mixer.c b/src/audio/mixer.c index
> > 2d021b7..4dccf7a 100644
> > --- a/src/audio/mixer.c
> > +++ b/src/audio/mixer.c
> > @@ -175,6 +175,31 @@ static struct comp_dev*
> mixer_volume_component(struct comp_dev *mixer)
> >  	return comp_dev;
> >  }
> >
> > +static int mixer_stop(struct comp_dev *dev) {
> > +	struct list_item *clist;
> > +
> > +	/* now reset downstream buffer */
> > +	list_for_item(clist, &dev->bsink_list) {
> > +		struct comp_buffer *buffer;
> > +
> > +		buffer = container_of(clist, struct comp_buffer, source_list);
> > +
> > +		/* dont reset buffer if the component is not connected */
> > +		if (!buffer->connected)
> > +			continue;
> > +		/* reset buffer next to the component*/
> > +		bzero(buffer->addr, buffer->desc.size);
> > +		buffer->w_ptr = buffer->r_ptr = buffer->addr;
> > +		buffer->end_addr = buffer->addr + buffer->desc.size;
> > +		buffer->free = buffer->desc.size;
> > +		buffer->avail = 0;
> > +	}
> > +
> > +	dev->state = COMP_STATE_SETUP;
> > +	return 0;
> > +}
> > +
> >  /* used to pass standard and bespoke commands (with data) to
> > component */  static int mixer_cmd(struct comp_dev *dev, int cmd, void
> > *data)  { @@ -184,7 +209,6 @@ static int mixer_cmd(struct comp_dev
> > *dev, int cmd, void *data)
> >  	switch(cmd) {
> >  	case COMP_CMD_START:
> >  		trace_mixer("MSa");
> > -	case COMP_CMD_STOP:
> >  	case COMP_CMD_PAUSE:
> >  	case COMP_CMD_RELEASE:
> >  	case COMP_CMD_DRAIN:
> > @@ -192,6 +216,11 @@ static int mixer_cmd(struct comp_dev *dev, int cmd,
> void *data)
> >  	case COMP_CMD_RESUME:
> >  		finish = mixer_status_change(dev);
> >  		break;
> > +	case COMP_CMD_STOP:
> > +		finish = mixer_status_change(dev);
> > +		if (finish == 0)
> > +			mixer_stop(dev);
> > +		break;
> >  	case COMP_CMD_VOLUME:
> >  		vol_dev = mixer_volume_component(dev);
> >  		if (vol_dev != NULL)
> > diff --git a/src/audio/volume.c b/src/audio/volume.c index
> > a226334..032e78b 100644
> > --- a/src/audio/volume.c
> > +++ b/src/audio/volume.c
> > @@ -389,6 +389,31 @@ static inline void volume_set_chan_unmute(struct
> comp_dev *dev, int chan)
> >  	cd->tvolume[chan] = cd->mvolume[chan];  }
> >
> > +static int volume_stop(struct comp_dev *dev) {
> > +	struct list_item *clist;
> > +
> > +	/* now reset downstream buffer */
> > +	list_for_item(clist, &dev->bsink_list) {
> > +		struct comp_buffer *buffer;
> > +
> > +		buffer = container_of(clist, struct comp_buffer, source_list);
> > +
> > +		/* dont reset buffer if the component is not connected */
> > +		if (!buffer->connected)
> > +			continue;
> > +		/* reset buffer next to the component*/
> > +		bzero(buffer->addr, buffer->desc.size);
> > +		buffer->w_ptr = buffer->r_ptr = buffer->addr;
> > +		buffer->end_addr = buffer->addr + buffer->desc.size;
> > +		buffer->free = buffer->desc.size;
> > +		buffer->avail = 0;
> > +	}
> > +
> > +	dev->state = COMP_STATE_SETUP;
> > +	return 0;
> > +}
> > +
> >  /* used to pass standard and bespoke commands (with data) to
> > component */  static int volume_cmd(struct comp_dev *dev, int cmd,
> > void *data)  { @@ -432,7 +457,7 @@ static int volume_cmd(struct
> > comp_dev *dev, int cmd, void *data)
> >  		if (dev->state == COMP_STATE_RUNNING ||
> >  				dev->state == COMP_STATE_DRAINING ||
> >  				dev->state == COMP_STATE_PAUSED)
> > -			dev->state = COMP_STATE_SETUP;
> > +			volume_stop(dev);
> >  		break;
> >  	case COMP_CMD_PAUSE:
> >  		/* only support pausing for running */
>