Received: by 2002:a25:ad19:0:0:0:0:0 with SMTP id y25csp3478966ybi; Tue, 2 Jul 2019 08:27:51 -0700 (PDT) X-Google-Smtp-Source: APXvYqwpnrfhioTboDN/RUUav4C3/r3XYZQ2kvzkqe1qQJYKj9rBOWGIIAVDTS/76asiLRqBpB34 X-Received: by 2002:a17:902:5a2:: with SMTP id f31mr26014630plf.72.1562081271440; Tue, 02 Jul 2019 08:27:51 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1562081271; cv=none; d=google.com; s=arc-20160816; b=c5MSSbZuB5Nd8NvCCaMt/UV1BkSqLwkYNApgEgwBMQ4K1luNaoo1Y1a+HU20oeMtnS ijIcHPgk8owywkFCDVexn1ThrsAB0hFjIssZDLYk2LNSeW3I4gmC/fSVCjpz+kBXlABV z8h2wRoWoJB24RQr2pFVlflB+LK5BbRZFMl0M7unm7HfGcZYm2HKn1Orb6nTCkTluw2i r6oOc1vaXnHP0P107hv9IPItZkRb1jC7qEpSEOMlkk1EZ6p+vpEjcmPvpGeQ3PZVHKa5 q5cgJHDUZDU84Vv9gJlIpWb7agIGLMiYgU1rKY7BEI1ZU1vAKOE8kcIAB9iWs5N69cFV /21g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:dkim-signature:content-transfer-encoding :content-language:in-reply-to:mime-version:user-agent:date :message-id:from:references:cc:to:subject; bh=x5gkT3yANst93Zh4OqckcOeEpw+YeX95j1KUBqZLmoM=; b=WTrp+4Pb0XJatWl57WKHnhio5N/Bn5iGMmnu0q8w57MtXQDp9oiq2+e9iHWHqBg8Wm POcPzT+AuoI/oyxtYBuPWYCHLLsw6jFJJN8g+4EWiusV2GLrGSFPlRKXPmKcOKrN8fad uhvMRYZZC3KMbYIxiW077PkB0BB5MZS00SI2pv3Q3PcpVBZ4QrNAXnh1we/89Vyt4dIw Yn1h7cjrq0xOG+a4jVTKtoyjworifLFcbl9L1A8bJHZx/MNhQ2ndsTO6FLKWeyQQ6X2D UgBeyw5vR4ktypvUrGplvRwtZsAt4HXGI+IkAMgwDQgT2ODx/bL8GHZH86TikY7DBoCl A/Dg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@nvidia.com header.s=n1 header.b=CdWC7kb7; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=nvidia.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id b18si12789015pgm.82.2019.07.02.08.27.35; Tue, 02 Jul 2019 08:27:51 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@nvidia.com header.s=n1 header.b=CdWC7kb7; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=nvidia.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726793AbfGBP1L (ORCPT + 99 others); Tue, 2 Jul 2019 11:27:11 -0400 Received: from hqemgate16.nvidia.com ([216.228.121.65]:5855 "EHLO hqemgate16.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725780AbfGBP1K (ORCPT ); Tue, 2 Jul 2019 11:27:10 -0400 Received: from hqpgpgate101.nvidia.com (Not Verified[216.228.121.13]) by hqemgate16.nvidia.com (using TLS: TLSv1.2, DES-CBC3-SHA) id ; Tue, 02 Jul 2019 08:27:08 -0700 Received: from hqmail.nvidia.com ([172.20.161.6]) by hqpgpgate101.nvidia.com (PGP Universal service); Tue, 02 Jul 2019 08:27:09 -0700 X-PGP-Universal: processed; by hqpgpgate101.nvidia.com on Tue, 02 Jul 2019 08:27:09 -0700 Received: from [10.21.132.148] (10.124.1.5) by HQMAIL107.nvidia.com (172.20.187.13) with Microsoft SMTP Server (TLS) id 15.0.1473.3; Tue, 2 Jul 2019 15:27:07 +0000 Subject: Re: [PATCH v3] dmaengine: tegra-apb: Support per-burst residue granularity To: Dmitry Osipenko , Laxman Dewangan , Vinod Koul , Thierry Reding , Ben Dooks CC: , , References: <20190627194728.8948-1-digetx@gmail.com> <3a5403fe-b81f-993c-e7c0-407387e001d9@gmail.com>

From: Jon Hunter Message-ID: <019762ad-79e0-50ad-76f2-86bc3e107caa@nvidia.com> Date: Tue, 2 Jul 2019 16:27:05 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.2 MIME-Version: 1.0 In-Reply-To: X-Originating-IP: [10.124.1.5] X-ClientProxiedBy: HQMAIL108.nvidia.com (172.18.146.13) To HQMAIL107.nvidia.com (172.20.187.13) Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: quoted-printable DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nvidia.com; s=n1; t=1562081228; bh=x5gkT3yANst93Zh4OqckcOeEpw+YeX95j1KUBqZLmoM=; h=X-PGP-Universal:Subject:To:CC:References:From:Message-ID:Date: User-Agent:MIME-Version:In-Reply-To:X-Originating-IP: X-ClientProxiedBy:Content-Type:Content-Language: Content-Transfer-Encoding; b=CdWC7kb7YRb6Vs+WhhEGYzIsJoJa/3TC6VycDDj36pGQoyQI1VctmEQ0Wh8RgnI65 d+3uVx5iyMTuSwXujD+Bh7juy3UT40zicWPH6foo6+qC3H6DCfF0K6nSx7OuQB61D8 tnt5nj/BuZwgj7RzNjP5rXAgCNR8In+EZZccGV9W53LjnRjwEtP1tjUX/PfC/3OTsn 8RIx6ZADuu7+zcxOpECWucdtPOoMgREi4xDpQGWc16D67E9pzDK/g3R1d1uS2X2Mjp 9hc17mxcAamMOI8jzsszNgsZ+8WJOERV9BtsyETrlkMsSUuzQ7+2t361+b+WllYCcy 2y9qrA881UV8w== Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 02/07/2019 15:41, Dmitry Osipenko wrote: > 02.07.2019 16:41, Jon Hunter =D0=BF=D0=B8=D1=88=D0=B5=D1=82: >> >> On 02/07/2019 14:22, Dmitry Osipenko wrote: >>> 02.07.2019 15:54, Jon Hunter =D0=BF=D0=B8=D1=88=D0=B5=D1=82: >>>> >>>> On 02/07/2019 12:37, Dmitry Osipenko wrote: >>>>> 02.07.2019 14:20, Jon Hunter =D0=BF=D0=B8=D1=88=D0=B5=D1=82: >>>>>> >>>>>> On 27/06/2019 20:47, Dmitry Osipenko wrote: >>>>>>> Tegra's APB DMA engine updates words counter after each transferred= burst >>>>>>> of data, hence it can report transfer's residual with more fidelity= which >>>>>>> may be required in cases like audio playback. In particular this fi= xes >>>>>>> audio stuttering during playback in a chromium web browser. The pat= ch is >>>>>>> based on the original work that was made by Ben Dooks and a patch f= rom >>>>>>> downstream kernel. It was tested on Tegra20 and Tegra30 devices. >>>>>>> >>>>>>> Link: https://lore.kernel.org/lkml/20190424162348.23692-1-ben.dooks= @codethink.co.uk/ >>>>>>> Link: https://nv-tegra.nvidia.com/gitweb/?p=3Dlinux-4.4.git;a=3Dcom= mit;h=3Dc7bba40c6846fbf3eaad35c4472dcc7d8bbc02e5 >>>>>>> Inspired-by: Ben Dooks >>>>>>> Signed-off-by: Dmitry Osipenko >>>>>>> --- >>>>>>> >>>>>>> Changelog: >>>>>>> >>>>>>> v3: Added workaround for a hardware design shortcoming that result= s >>>>>>> in a words counter wraparound before end-of-transfer bit is se= t >>>>>>> in a cyclic mode. >>>>>>> >>>>>>> v2: Addressed review comments made by Jon Hunter to v1. We won't t= ry >>>>>>> to get words count if dma_desc is on free list as it will resu= lt >>>>>>> in a NULL dereference because this case wasn't handled properl= y. >>>>>>> >>>>>>> The residual value is now updated properly, avoiding potential >>>>>>> integer overflow by adding the "bytes" to the "bytes_transferr= ed" >>>>>>> instead of the subtraction. >>>>>>> >>>>>>> drivers/dma/tegra20-apb-dma.c | 69 +++++++++++++++++++++++++++++++= ---- >>>>>>> 1 file changed, 62 insertions(+), 7 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-ap= b-dma.c >>>>>>> index 79e9593815f1..71473eda28ee 100644 >>>>>>> --- a/drivers/dma/tegra20-apb-dma.c >>>>>>> +++ b/drivers/dma/tegra20-apb-dma.c >>>>>>> @@ -152,6 +152,7 @@ struct tegra_dma_sg_req { >>>>>>> bool last_sg; >>>>>>> struct list_head node; >>>>>>> struct tegra_dma_desc *dma_desc; >>>>>>> + unsigned int words_xferred; >>>>>>> }; >>>>>>> =20 >>>>>>> /* >>>>>>> @@ -496,6 +497,7 @@ static void tegra_dma_configure_for_next(struct= tegra_dma_channel *tdc, >>>>>>> tdc_write(tdc, TEGRA_APBDMA_CHAN_CSR, >>>>>>> nsg_req->ch_regs.csr | TEGRA_APBDMA_CSR_ENB); >>>>>>> nsg_req->configured =3D true; >>>>>>> + nsg_req->words_xferred =3D 0; >>>>>>> =20 >>>>>>> tegra_dma_resume(tdc); >>>>>>> } >>>>>>> @@ -511,6 +513,7 @@ static void tdc_start_head_req(struct tegra_dma= _channel *tdc) >>>>>>> typeof(*sg_req), node); >>>>>>> tegra_dma_start(tdc, sg_req); >>>>>>> sg_req->configured =3D true; >>>>>>> + sg_req->words_xferred =3D 0; >>>>>>> tdc->busy =3D true; >>>>>>> } >>>>>>> =20 >>>>>>> @@ -797,6 +800,61 @@ static int tegra_dma_terminate_all(struct dma_= chan *dc) >>>>>>> return 0; >>>>>>> } >>>>>>> =20 >>>>>>> +static unsigned int tegra_dma_sg_bytes_xferred(struct tegra_dma_ch= annel *tdc, >>>>>>> + struct tegra_dma_sg_req *sg_req) >>>>>>> +{ >>>>>>> + unsigned long status, wcount =3D 0; >>>>>>> + >>>>>>> + if (!list_is_first(&sg_req->node, &tdc->pending_sg_req)) >>>>>>> + return 0; >>>>>>> + >>>>>>> + if (tdc->tdma->chip_data->support_separate_wcount_reg) >>>>>>> + wcount =3D tdc_read(tdc, TEGRA_APBDMA_CHAN_WORD_TRANSFER); >>>>>>> + >>>>>>> + status =3D tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS); >>>>>>> + >>>>>>> + if (!tdc->tdma->chip_data->support_separate_wcount_reg) >>>>>>> + wcount =3D status; >>>>>>> + >>>>>>> + if (status & TEGRA_APBDMA_STATUS_ISE_EOC) >>>>>>> + return sg_req->req_len; >>>>>>> + >>>>>>> + wcount =3D get_current_xferred_count(tdc, sg_req, wcount); >>>>>>> + >>>>>>> + if (!wcount) { >>>>>>> + /* >>>>>>> + * If wcount wasn't ever polled for this SG before, then >>>>>>> + * simply assume that transfer hasn't started yet. >>>>>>> + * >>>>>>> + * Otherwise it's the end of the transfer. >>>>>>> + * >>>>>>> + * The alternative would be to poll the status register >>>>>>> + * until EOC bit is set or wcount goes UP. That's so >>>>>>> + * because EOC bit is getting set only after the last >>>>>>> + * burst's completion and counter is less than the actual >>>>>>> + * transfer size by 4 bytes. The counter value wraps around >>>>>>> + * in a cyclic mode before EOC is set(!), so we can't easily >>>>>>> + * distinguish start of transfer from its end. >>>>>>> + */ >>>>>>> + if (sg_req->words_xferred) >>>>>>> + wcount =3D sg_req->req_len - 4; >>>>>>> + >>>>>>> + } else if (wcount < sg_req->words_xferred) { >>>>>>> + /* >>>>>>> + * This case shall not ever happen because EOC bit >>>>>>> + * must be set once next cyclic transfer is started. >>>>>> >>>>>> I am not sure I follow this and why this condition cannot happen for >>>>>> cyclic transfers. What about non-cyclic transfers? >>>>> >>>>> It cannot happen because the EOC bit will be set in that case. The co= unter wraps >>>>> around when the transfer of a last burst happens, EOC bit is guarante= ed to be set >>>>> after completion of the last burst. That's my observation after a tho= rough testing, >>>>> it will be very odd if EOC setting happened completely asynchronously= . >>>> >>>> I see how you know that the EOC is set. Anyway, you check if the EOC i= s >>>> set before and if so return sg_req->req_len prior to this test. >>>> >>>> Maybe I am missing something, but what happens if we are mid block whe= n >>>> dmaengine_tx_status() is called? That happen asynchronously right? >>> >>> >>> Do you mean asynchronously in regards to the ISR? Or something else? >> >> In the sense that the client can call dmaengine_tx_status() at anytime >> to check the status of a transfer. >=20 > Should be alright, I think this patch covers all of possible cases: >=20 > 1) Start of transfer, when wcount=3D0. > 2) Middle of transfer, when wcount!=3D0. > 3) End of transfer, when wcount=3D0. > 4) End of transfer, when wcount!=3D0 and EOC is set. I think I see my problem I read 'wcount < sg_req->req_len' and NOT 'wcount < sg_req->words_xferred'. So yes this should be fine. Jon --=20 nvpublic