Hey Romain,
I submitted this ages ago, but got distracted. Thanks for reviving.
Note that i remember Krzysztof had some review remarks, it's probably
worth digging up the old thread and making sure you address those.
On Wed, 2017-02-01 at 13:43 +0100, Romain Perier wrote:
> From: Sjoerd Simons <sjoerd.simons at collabora.co.uk>
>
> When a transfer completes there is a small window between the
> descriptor
> being unset as the current active one in the thread and it being
> marked
> as done. This causes the residue to be incorrectly set when
> pl330_tx_status is run in that window. Practically this caused
> issue for me with audio playback as the residue goes up during a
> transfer (as the in-progress transfer is no longer accounted for),
> which makes the higher levels think the audio ringbuffer wrapped
> around
> and thus did a sudden big jump forward.
>
> To resolve this, add a field protected by the dma engine lock to
> indicate the transfer is done but the status hasn't been updated yet.
>
> Signed-off-by: Sjoerd Simons <sjoerd.simons at collabora.co.uk>
> Signed-off-by: Romain Perier <romain.perier at collabora.com>
>
> Series-to: Vinod Koul <vinod.koul at intel.com>
> Series-cc: dmaengine at vger.kernel.org
> Series-cc: linux-kernel at vger.kernel.org
> Series-cc: Krzysztof Kozlowski <k.kozlowski at samsung.com>
> Series-cc: linux-rockchip at lists.infradead.org
> Series-cc: linux-samsung-soc at vger.kernel.org
> Series-cc:??linux-arm-kernel at vger.kernel.org
These are patman metadata, you probably want to drop these from the
commit message ;)
> ?drivers/dma/pl330.c | 9 ++++++++-
> ?1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 740bbb9..d942fac 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -521,6 +521,9 @@ struct dma_pl330_desc {
> ?
> ? enum desc_status status;
> ?
> + /* Transfer completed, but not yet moved to DONE state */
> + bool xferred;
> +
> ? int bytes_requested;
> ? bool last;
> ?
> @@ -1508,6 +1511,9 @@ static void dma_pl330_rqcb(struct
> dma_pl330_desc *desc, enum pl330_op_err err)
> ? spin_lock_irqsave(&pch->lock, flags);
> ?
> ? desc->status = DONE;
> + spin_lock(&pch->thread->dmac->lock);
> + desc->xferred = false;
> + spin_unlock(&pch->thread->dmac->lock);
> ?
> ? spin_unlock_irqrestore(&pch->lock, flags);
> ?
> @@ -1640,6 +1646,7 @@ static int pl330_update(struct pl330_dmac
> *pl330)
> ?
> ? /* Detach the req */
> ? descdone = thrd->req[active].desc;
> + descdone->xferred = true;
> ? thrd->req[active].desc = NULL;
> ?
> ? thrd->req_running = -1;
> @@ -2311,7 +2318,7 @@ pl330_tx_status(struct dma_chan *chan,
> dma_cookie_t cookie,
> ?
> ? /* Check in pending list */
> ? list_for_each_entry(desc, &pch->work_list, node) {
> - if (desc->status == DONE)
> + if (desc->xferred || desc->status == DONE)
> ? transferred = desc->bytes_requested;
> ? else if (running && desc == running)
> ? transferred =
--
Sjoerd Simons
Collabora Ltd.
