[Sound-open-firmware] [PATCH 2/2] Disable checking half full of local DMA trace buffer when trace_work is running.

[yanwang]

Hi, Pierre,
	I have updated commit message beased on your comments as v2.
	About this:
> (it could be 90% full or 10% full depending on the defined watermark 
> which should be configurable btw).
	I will submit another patch for it.
	Could you please review it?
	Thanks.

Yan Wang
		
On Fri, 2017-12-01 at 22:53 +0800, yan.wang wrote:
> Hi, Pierre,
>     I will update commit message based on your comments.
>     Thanks.
> 
> Yan Wang
> 
> 
> 
>  
> From: Pierre-Louis Bossart
> Date: 2017-12-01 22:31
> To: yan.wang; sound-open-firmware
> Subject: Re: [Sound-open-firmware] [PATCH 2/2] Disable checking half
> full of local DMA trace buffer when trace_work is running.
> On 12/1/17 2:21 AM, yan.wang at linux.intel.com wrote:
> > 
> > From: Yan Wang <yan.wang at linux.intel.com>
> > 
> > If DMA trace copying is running, it is unnecessary to checking half
> > full
> > local DMA trace buffer.
> > 1. Add one flag in DMA trace data strcuture.
> > 2. Set/unset this flag in trace_work() callback.
> > 3. Add checking for this flag in dtrace_event().
>  
> The commit message is really far from self-explanatory.
> nothing explains under what circumstances the buffer can be half-
> full, 
> and whether it's good or bad.
> Likewise it seems like the feature is that you avoid starting a new 
> trace DMA transfer while an existing one is already underway, which 
> seems like a fundamentally sound thing to do. However it's really 
> conceptually unrelated to the decision on when the transfer occurs
> (it 
> could be 90% full or 10% full depending on the defined watermark -
> which 
> should be configurable btw). The commit message should really be
> "do not check trace buffer fullness while a transfer is taking place"
>  
> > 
> > 
> > Signed-off-by: Yan Wang <yan.wang at linux.intel.com>
> > ---
> >   src/include/reef/dma-trace.h |  1 +
> >   src/lib/dma-trace.c          | 17 +++++++++++++++++
> >   2 files changed, 18 insertions(+)
> > 
> > diff --git a/src/include/reef/dma-trace.h b/src/include/reef/dma-
> > trace.h
> > index 641a4dc..21824f3 100644
> > --- a/src/include/reef/dma-trace.h
> > +++ b/src/include/reef/dma-trace.h
> > @@ -60,6 +60,7 @@ struct dma_trace_data {
> >   uint32_t host_size;
> >   struct work dmat_work;
> >   uint32_t enabled;
> > + uint32_t dmat_work_flag;
> >   spinlock_t lock;
> >   };
> >   
> > diff --git a/src/lib/dma-trace.c b/src/lib/dma-trace.c
> > index 763e955..9dfe7c7 100644
> > --- a/src/lib/dma-trace.c
> > +++ b/src/lib/dma-trace.c
> > @@ -56,6 +56,9 @@ static uint64_t trace_work(void *data, uint64_t
> > delay)
> >   if (avail == 0)
> >   return DMA_TRACE_US;
> >   
> > + /* DMA trace copying is working */
> > + d->dmat_work_flag = 1;
> > +
> >   /* make sure we dont write more than buffer */
> >   if (avail > DMA_TRACE_LOCAL_SIZE)
> >   avail = DMA_TRACE_LOCAL_SIZE;
> > @@ -100,7 +103,12 @@ static uint64_t trace_work(void *data,
> > uint64_t delay)
> >   
> >   out:
> >   spin_lock_irq(&d->lock, flags);
> > +
> >   buffer->avail -= size;
> > +
> > + /* DMA trace copying is done */
> > + d->dmat_work_flag = 0;
> > +
> >   spin_unlock_irq(&d->lock, flags);
> >   
> >   /* reschedule the trace copying work */
> > @@ -138,6 +146,7 @@ int dma_trace_init(struct dma_trace_data *d)
> >   buffer->avail = 0;
> >   d->host_offset = 0;
> >   d->enabled = 0;
> > + d->dmat_work_flag = 0;
> >   
> >   list_init(&d->config.elem_list);
> >   work_init(&d->dmat_work, trace_work, d, WORK_ASYNC);
> > @@ -226,6 +235,14 @@ void dtrace_event(const char *e, uint32_t
> > length)
> >   
> >   spin_lock_irq(&trace_data->lock, flags);
> >   dtrace_add_event(e, length);
> > +
> > + /* if DMA trace copying is working */
> > + /* don't check if local buffer is half full */
> > + if (trace_data->dmat_work_flag) {
> > + spin_unlock_irq(&trace_data->lock, flags);
> > + return;
> > + }
> > +
> >   spin_unlock_irq(&trace_data->lock, flags);
> >   
> >   /* schedule copy now if buffer > 50% full */
> > 
>  
> _______________________________________________
> Sound-open-firmware mailing list
> Sound-open-firmware at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware