From mboxrd@z Thu Jan 1 00:00:00 1970 From: Will Deacon Subject: Re: [PATCH] iommu: arm-smmu: Set SCTLR.HUPCF bit Date: Mon, 29 Oct 2018 19:10:00 +0000 Message-ID: <20181029191000.GD16739@arm.com> References: <20180927224609.19515-1-robdclark@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Content-Disposition: inline In-Reply-To: <20180927224609.19515-1-robdclark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: freedreno-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org Sender: "Freedreno" To: Rob Clark Cc: Robin Murphy , Joerg Roedel , linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, linux-arm-msm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org List-Id: linux-arm-msm@vger.kernel.org T24gVGh1LCBTZXAgMjcsIDIwMTggYXQgMDY6NDY6MDdQTSAtMDQwMCwgUm9iIENsYXJrIHdyb3Rl Ogo+IFdlIHNlZW0gdG8gbmVlZCB0byBzZXQgZWl0aGVyIHRoaXMgb3IgQ0ZDRkcgKHN0YWxsKSwg b3RoZXJ3aXNlIGdwdQo+IGZhdWx0cyB0cmlnZ2VyIHByb2JsZW1zIHdpdGggb3RoZXIgaW4tZmxp Z2h0IHRyYW5zYWN0aW9ucyBmcm9tIHRoZQo+IEdQVSBjYXVzaW5nIENQIGVycm9ycywgZXRjLgo+ IAo+IEluIHRoZSBBUk0gU01NVSBzcGVjLCB0aGUgJ0hpdCB1bmRlciBwcmV2aW91cyBjb250ZXh0 IGZhdWx0JyBiaXQgaXMKPiBkZXNjcmliZWQgYXM6Cj4gCj4gICcwJyAtIFN0YWxsIG9yIHRlcm1p bmF0ZSBzdWJzZXF1ZW50IHRyYW5zYWN0aW9ucyBpbiB0aGUgcHJlc2VuY2UKPiAgICAgICAgb2Yg YW4gb3V0c3RhbmRpbmcgY29udGV4dCBmYXVsdAo+ICAnMScgLSBQcm9jZXNzIGFsbCBzdWJzZXF1 ZW50IHRyYW5zYWN0aW9ucyBpbmRlcGVuZGVudGx5IG9mIGFueQo+ICAgICAgICBvdXRzdGFuZGlu ZyBjb250ZXh0IGZhdWx0Lgo+IAo+IFNpbmNlIHdlIGRvbid0IGVuYWJsZSBDRkNGRyAoc3RhbGwp IHRoZSBiZWhhdmlvciBvZiB0ZXJtaW5hdGluZwo+IG90aGVyIHRyYW5zYWN0aW9ucyBtYWtlcyBz ZW5zZS4gIEFuZCBpcyBwcm9iYWJseSBub3Qgd2hhdCB3ZSB3YW50Cj4gKGFuZCBkZWZpbmF0ZWx5 IG5vdCB3aGF0IHdlIHdhbnQgZm9yIEdQVSkuCj4gCj4gU2lnbmVkLW9mZi1ieTogUm9iIENsYXJr IDxyb2JkY2xhcmtAZ21haWwuY29tPgo+IC0tLQo+IFNvIEkgaGl0IHRoaXMgaXNzdWUgYSBsb25n IHRpbWUgYmFjayBvbiA4MjAgKG1zbTg5OTYpIGFuZCBhdCB0aGUKPiB0aW1lIEkgc29sdmVkIGl0 IHdpdGggYSBwYXRjaCB0aGF0IGVuYWJsZWQgQ0ZDRkcuICBBbmQgaXQgcmVzdXJmYWNlZAo+IG1v cmUgcmVjZW50bHkgb24gc2RtODQ1LiAgQnV0IGF0IHRoZSB0aW1lIENGQ0ZHIHdhcyByZWplY3Rl ZCwgaWlyYwo+IGJlY2F1c2Ugb2YgY29uY2VybiB0aGF0IGl0IHdvdWxkIGNhdXNlIHByb2JsZW1z IG9uIG90aGVyIG5vbi1xY29tCj4gYXJtIHNtbXUgaW1wbGVtZW50YXRpb25zLiAgQW5kIEkgdGhp bmsgSSBmb3Jnb3QgdG8gc2VuZCB0aGlzIHZlcnNpb24KPiBvZiB0aGUgc29sdXRpb24uCj4gCj4g SWYgZW5hYmxpbmcgSFVQQ0YgaXMgYW50aWNpcGF0ZWQgdG8gY2F1c2UgcHJvYmxlbXMgb24gb3Ro ZXIgQVJNCj4gU01NVSBpbXBsZW1lbnRhdGlvbnMsIEkgdGhpbmsgSSBjYW4gY29tZSB1cCB3aXRo IGEgdmFyaWFudCBvZiB0aGlzCj4gcGF0Y2ggd2hpY2ggY29uZGl0aW9uYWxseSBlbmFibGVzIGl0 IGZvciBzbmFwZHJhZ29uLgo+IAo+IEVpdGhlciB3YXksIEknZCByZWFsbHkgbGlrZSB0byBnZXQg c29tZSB2YXJpYW50IG9mIHRoaXMgZml4IG1lcmdlZAo+IChhbmQgcHJvYmFibHkgaXQgd291bGQg YmUgYSBnb29kIGlkZWEgZm9yIHN0YWJsZSBrZXJuZWwgYnJhbmNoZXMKPiB0b28pLCBzaW5jZSBj dXJyZW50IGJlaGF2aW91ciB3aXRoIHRoZSBHUFUgbWVhbnMgZmF1bHRzIHR1cm4gaW50bwo+IGEg ZmFudGFzdGljIGNhc2NhZGUgb2YgZmFpbC4KCkNhbiB5b3UgZGVzY3JpYmUgaG93IHRoaXMgZmFu dGFzdGljIGNhc2NhZGUgb2YgZmFpbCBpbXByb3ZlcyB3aXRoIHRoaXMKcGF0Y2gsIHBsZWFzZT8g SWYgeW91J3JlIGdldHRpbmcgY29udGV4dCBmYXVsdHMgdGhlbiBzb21ldGhpbmcgaGFzIGFscmVh ZHkKZ29uZSBob3JyaWJseSB3cm9uZywgc28gSSdtIHRyeWluZyB0byB3b3JrIG91dCBob3cgdGhp cyBpbXByb3ZlcyB0aGluZ3MuCgo+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2lvbW11L2FybS1zbW11 LXJlZ3MuaCBiL2RyaXZlcnMvaW9tbXUvYXJtLXNtbXUtcmVncy5oCj4gaW5kZXggYTEyMjZlNGFi NWY4Li4yMjkxOTI1ZWI4MDAgMTAwNjQ0Cj4gLS0tIGEvZHJpdmVycy9pb21tdS9hcm0tc21tdS1y ZWdzLmgKPiArKysgYi9kcml2ZXJzL2lvbW11L2FybS1zbW11LXJlZ3MuaAo+IEBAIC0xNzgsNiAr MTc4LDcgQEAgZW51bSBhcm1fc21tdV9zMmNyX3ByaXZjZmcgewo+ICAjZGVmaW5lIEFSTV9TTU1V X0NCX0FUU1IJCTB4OGYwCj4gIAo+ICAjZGVmaW5lIFNDVExSX1MxX0FTSURQTkUJCSgxIDw8IDEy KQo+ICsjZGVmaW5lIFNDVExSX0hVUENGCQkJKDEgPDwgOCkKPiAgI2RlZmluZSBTQ1RMUl9DRkNG RwkJCSgxIDw8IDcpCj4gICNkZWZpbmUgU0NUTFJfQ0ZJRQkJCSgxIDw8IDYpCj4gICNkZWZpbmUg U0NUTFJfQ0ZSRQkJCSgxIDw8IDUpCj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvaW9tbXUvYXJtLXNt bXUuYyBiL2RyaXZlcnMvaW9tbXUvYXJtLXNtbXUuYwo+IGluZGV4IGY3YTk2YmNmOTRhNi4uNDdm ZmM5YWFkZTcyIDEwMDY0NAo+IC0tLSBhL2RyaXZlcnMvaW9tbXUvYXJtLXNtbXUuYwo+ICsrKyBi L2RyaXZlcnMvaW9tbXUvYXJtLXNtbXUuYwo+IEBAIC03MTMsOSArNzEzLDkgQEAgc3RhdGljIHZv aWQgYXJtX3NtbXVfd3JpdGVfY29udGV4dF9iYW5rKHN0cnVjdCBhcm1fc21tdV9kZXZpY2UgKnNt bXUsIGludCBpZHgpCj4gIAlyZWcgPSBTQ1RMUl9DRklFIHwgU0NUTFJfQ0ZSRSB8IFNDVExSX0FG RSB8IFNDVExSX1RSRSB8IFNDVExSX007Cj4gIAlpZiAoc3RhZ2UxKQo+ICAJCXJlZyB8PSBTQ1RM Ul9TMV9BU0lEUE5FOwo+ICsJcmVnIHw9IFNDVExSX0hVUENGOwoKTWF5YmUganVzdCB0aHJvdyB0 aGlzIGludG8gdGhlIGFzc2lnbm1lbnQgYSBjb3VwbGUgb2YgbGluZXMgYWJvdmU/Cgo+ICAJaWYg KElTX0VOQUJMRUQoQ09ORklHX0NQVV9CSUdfRU5ESUFOKSkKPiAgCQlyZWcgfD0gU0NUTFJfRTsK PiAtCgpSYW5kb20gd2hpdGVzcGFjZSBjaGFuZ2UuCgpXaWxsCl9fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fCkZyZWVkcmVubyBtYWlsaW5nIGxpc3QKRnJlZWRy ZW5vQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9t YWlsbWFuL2xpc3RpbmZvL2ZyZWVkcmVubwo= From mboxrd@z Thu Jan 1 00:00:00 1970 From: will.deacon@arm.com (Will Deacon) Date: Mon, 29 Oct 2018 19:10:00 +0000 Subject: [PATCH] iommu: arm-smmu: Set SCTLR.HUPCF bit In-Reply-To: <20180927224609.19515-1-robdclark@gmail.com> References: <20180927224609.19515-1-robdclark@gmail.com> Message-ID: <20181029191000.GD16739@arm.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Thu, Sep 27, 2018 at 06:46:07PM -0400, Rob Clark wrote: > We seem to need to set either this or CFCFG (stall), otherwise gpu > faults trigger problems with other in-flight transactions from the > GPU causing CP errors, etc. > > In the ARM SMMU spec, the 'Hit under previous context fault' bit is > described as: > > '0' - Stall or terminate subsequent transactions in the presence > of an outstanding context fault > '1' - Process all subsequent transactions independently of any > outstanding context fault. > > Since we don't enable CFCFG (stall) the behavior of terminating > other transactions makes sense. And is probably not what we want > (and definately not what we want for GPU). > > Signed-off-by: Rob Clark > --- > So I hit this issue a long time back on 820 (msm8996) and at the > time I solved it with a patch that enabled CFCFG. And it resurfaced > more recently on sdm845. But at the time CFCFG was rejected, iirc > because of concern that it would cause problems on other non-qcom > arm smmu implementations. And I think I forgot to send this version > of the solution. > > If enabling HUPCF is anticipated to cause problems on other ARM > SMMU implementations, I think I can come up with a variant of this > patch which conditionally enables it for snapdragon. > > Either way, I'd really like to get some variant of this fix merged > (and probably it would be a good idea for stable kernel branches > too), since current behaviour with the GPU means faults turn into > a fantastic cascade of fail. Can you describe how this fantastic cascade of fail improves with this patch, please? If you're getting context faults then something has already gone horribly wrong, so I'm trying to work out how this improves things. > diff --git a/drivers/iommu/arm-smmu-regs.h b/drivers/iommu/arm-smmu-regs.h > index a1226e4ab5f8..2291925eb800 100644 > --- a/drivers/iommu/arm-smmu-regs.h > +++ b/drivers/iommu/arm-smmu-regs.h > @@ -178,6 +178,7 @@ enum arm_smmu_s2cr_privcfg { > #define ARM_SMMU_CB_ATSR 0x8f0 > > #define SCTLR_S1_ASIDPNE (1 << 12) > +#define SCTLR_HUPCF (1 << 8) > #define SCTLR_CFCFG (1 << 7) > #define SCTLR_CFIE (1 << 6) > #define SCTLR_CFRE (1 << 5) > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > index f7a96bcf94a6..47ffc9aade72 100644 > --- a/drivers/iommu/arm-smmu.c > +++ b/drivers/iommu/arm-smmu.c > @@ -713,9 +713,9 @@ static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx) > reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M; > if (stage1) > reg |= SCTLR_S1_ASIDPNE; > + reg |= SCTLR_HUPCF; Maybe just throw this into the assignment a couple of lines above? > if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) > reg |= SCTLR_E; > - Random whitespace change. Will From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.3 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5467BC2BC61 for ; Mon, 29 Oct 2018 19:09:56 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 296AC2080A for ; Mon, 29 Oct 2018 19:09:56 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 296AC2080A Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729562AbeJ3D7x (ORCPT ); Mon, 29 Oct 2018 23:59:53 -0400 Received: from foss.arm.com ([217.140.101.70]:45106 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729441AbeJ3D7x (ORCPT ); Mon, 29 Oct 2018 23:59:53 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id B184F341; Mon, 29 Oct 2018 12:09:53 -0700 (PDT) Received: from edgewater-inn.cambridge.arm.com (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id 810B23F6A8; Mon, 29 Oct 2018 12:09:53 -0700 (PDT) Received: by edgewater-inn.cambridge.arm.com (Postfix, from userid 1000) id 985121AE091C; Mon, 29 Oct 2018 19:10:00 +0000 (GMT) Date: Mon, 29 Oct 2018 19:10:00 +0000 From: Will Deacon To: Rob Clark Cc: iommu@lists.linux-foundation.org, Robin Murphy , linux-arm-kernel@lists.infradead.org, freedreno@lists.freedesktop.org, linux-arm-msm@vger.kernel.org, Joerg Roedel , linux-kernel@vger.kernel.org Subject: Re: [PATCH] iommu: arm-smmu: Set SCTLR.HUPCF bit Message-ID: <20181029191000.GD16739@arm.com> References: <20180927224609.19515-1-robdclark@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180927224609.19515-1-robdclark@gmail.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Sep 27, 2018 at 06:46:07PM -0400, Rob Clark wrote: > We seem to need to set either this or CFCFG (stall), otherwise gpu > faults trigger problems with other in-flight transactions from the > GPU causing CP errors, etc. > > In the ARM SMMU spec, the 'Hit under previous context fault' bit is > described as: > > '0' - Stall or terminate subsequent transactions in the presence > of an outstanding context fault > '1' - Process all subsequent transactions independently of any > outstanding context fault. > > Since we don't enable CFCFG (stall) the behavior of terminating > other transactions makes sense. And is probably not what we want > (and definately not what we want for GPU). > > Signed-off-by: Rob Clark > --- > So I hit this issue a long time back on 820 (msm8996) and at the > time I solved it with a patch that enabled CFCFG. And it resurfaced > more recently on sdm845. But at the time CFCFG was rejected, iirc > because of concern that it would cause problems on other non-qcom > arm smmu implementations. And I think I forgot to send this version > of the solution. > > If enabling HUPCF is anticipated to cause problems on other ARM > SMMU implementations, I think I can come up with a variant of this > patch which conditionally enables it for snapdragon. > > Either way, I'd really like to get some variant of this fix merged > (and probably it would be a good idea for stable kernel branches > too), since current behaviour with the GPU means faults turn into > a fantastic cascade of fail. Can you describe how this fantastic cascade of fail improves with this patch, please? If you're getting context faults then something has already gone horribly wrong, so I'm trying to work out how this improves things. > diff --git a/drivers/iommu/arm-smmu-regs.h b/drivers/iommu/arm-smmu-regs.h > index a1226e4ab5f8..2291925eb800 100644 > --- a/drivers/iommu/arm-smmu-regs.h > +++ b/drivers/iommu/arm-smmu-regs.h > @@ -178,6 +178,7 @@ enum arm_smmu_s2cr_privcfg { > #define ARM_SMMU_CB_ATSR 0x8f0 > > #define SCTLR_S1_ASIDPNE (1 << 12) > +#define SCTLR_HUPCF (1 << 8) > #define SCTLR_CFCFG (1 << 7) > #define SCTLR_CFIE (1 << 6) > #define SCTLR_CFRE (1 << 5) > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > index f7a96bcf94a6..47ffc9aade72 100644 > --- a/drivers/iommu/arm-smmu.c > +++ b/drivers/iommu/arm-smmu.c > @@ -713,9 +713,9 @@ static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx) > reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M; > if (stage1) > reg |= SCTLR_S1_ASIDPNE; > + reg |= SCTLR_HUPCF; Maybe just throw this into the assignment a couple of lines above? > if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) > reg |= SCTLR_E; > - Random whitespace change. Will