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@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
> Signed-off-by: Romain Perier <romain.perier@xxxxxxxxxxxxx>
>
> Series-to: Vinod Koul <vinod.koul@xxxxxxxxx>
> Series-cc: dmaengine@xxxxxxxxxxxxxxx
> Series-cc: linux-kernel@xxxxxxxxxxxxxxx
> Series-cc: Krzysztof Kozlowski <k.kozlowski@xxxxxxxxxxx>
> Series-cc: linux-rockchip@xxxxxxxxxxxxxxxxxxx
> Series-cc: linux-samsung-soc@xxxxxxxxxxxxxxx
> Series-cc: linux-arm-kernel@xxxxxxxxxxxxxxx
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.
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
