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;
MIME-Version: 1.0
References: <20190110072841.3283-1-ard.biesheuvel@linaro.org> <5d8135de-80fe-9c0e-2206-ecb809f64cdb@daenzer.net>
In-Reply-To: <5d8135de-80fe-9c0e-2206-ecb809f64cdb@daenzer.net>
From:   Ard Biesheuvel <ard.biesheuvel@linaro.org>
Date:   Mon, 14 Jan 2019 11:53:13 +0100
Message-ID: <CAKv+Gu_X-DH6GZ0k8RYuL18rxTwBpb5=Qp9VCZfn_kh+9b3Zkg@mail.gmail.com>
Subject: Re: [RFC PATCH] drm/ttm: force cached mappings for system RAM on ARM
To:     =?UTF-8?Q?Michel_D=C3=A4nzer?= <michel@daenzer.net>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Carsten Haitzler <Carsten.Haitzler@arm.com>,
        David Airlie <airlied@linux.ie>,
        Will Deacon <will.deacon@arm.com>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        Huang Rui <ray.huang@amd.com>,
        Junwei Zhang <Jerry.Zhang@amd.com>,
        Christian Koenig <christian.koenig@amd.com>,
        linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
        =?UTF-8?Q?Bernhard_Rosenkr=C3=A4nzer?= 
        <Bernhard.Rosenkranzer@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, 10 Jan 2019 at 10:34, Michel D=C3=A4nzer <michel@daenzer.net> wrote=
:
>
> On 2019-01-10 8:28 a.m., Ard Biesheuvel wrote:
> > ARM systems do not permit the use of anything other than cached
> > mappings for system memory, since that memory may be mapped in the
> > linear region as well, and the architecture does not permit aliases
> > with mismatched attributes.
> >
> > So short-circuit the evaluation in ttm_io_prot() if the flags include
> > TTM_PL_SYSTEM when running on ARM or arm64, and just return cached
> > attributes immediately.
> >
> > This fixes the radeon and amdgpu [TBC] drivers when running on arm64.
> > Without this change, amdgpu does not start at all, and radeon only
> > produces corrupt display output.
> >
> > Cc: Christian Koenig <christian.koenig@amd.com>
> > Cc: Huang Rui <ray.huang@amd.com>
> > Cc: Junwei Zhang <Jerry.Zhang@amd.com>
> > Cc: David Airlie <airlied@linux.ie>
> > Reported-by: Carsten Haitzler <Carsten.Haitzler@arm.com>
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  drivers/gpu/drm/ttm/ttm_bo_util.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/tt=
m_bo_util.c
> > index 046a6dda690a..0c1eef5f7ae3 100644
> > --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
> > +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
> > @@ -530,6 +530,11 @@ pgprot_t ttm_io_prot(uint32_t caching_flags, pgpro=
t_t tmp)
> >       if (caching_flags & TTM_PL_FLAG_CACHED)
> >               return tmp;
> >
> > +#if defined(__arm__) || defined(__aarch64__)
> > +     /* ARM only permits cached mappings of system memory */
> > +     if (caching_flags & TTM_PL_SYSTEM)
> > +             return tmp;
> > +#endif
> >  #if defined(__i386__) || defined(__x86_64__)
> >       if (caching_flags & TTM_PL_FLAG_WC)
> >               tmp =3D pgprot_writecombine(tmp);
> >
>
> Apart from Christian's concerns, I think this is the wrong place for
> this, because other TTM / driver code will still consider the memory
> uncacheable. E.g. the amdgpu driver will program the GPU to treat the
> memory as uncacheable, so it won't participate in cache coherency
> protocol for it, which is unlikely to work as expected in general if the
> CPU treats the memory as cacheable.
>

Will and I have spent some time digging into this, so allow me to
share some preliminary findings while we carry on and try to fix this
properly.

- The patch above is flawed, i.e., it doesn't do what it intends to
since it uses TTM_PL_SYSTEM instead of TTM_PL_FLAG_SYSTEM. Apologies
for that.
- The existence of a linear region mapping with mismatched attributes
is likely not the culprit here. (We do something similar with
non-cache coherent DMA in other places).
- The reason remapping the CPU side as cacheable does work (which I
did test) is because the GPU's uncacheable accesses (which I assume
are made using the NoSnoop PCIe transaction attribute) are actually
emitted as cacheable in some cases.
  . On my AMD Seattle, with or without SMMU (which is stage 2 only), I
must use cacheable accesses from the CPU side or things are broken.
This might be a h/w flaw, though.
  . On systems with stage 1+2 SMMUs, the driver uses stage 1
translations which always override the memory attributes to cacheable
for DMA coherent devices. This is what is affecting the Cavium
ThunderX2 (although it appears the attributes emitted by the RC may be
incorrect as well.)

The latter issue is a shortcoming in the SMMU driver that we have to
fix, i.e., it should take care not to modify the incoming attributes
of DMA coherent PCIe devices for NoSnoop to be able to work.

So in summary, the mismatch appears to be between the CPU accessing
the vmap region with non-cacheable attributes and the GPU accessing
the same memory with cacheable attributes, resulting in a loss of
coherency and lots of visible corruption.

To be able to debug this further, could you elaborate a bit on
- How does the hardware emit those uncached/wc inbound accesses? Do
they rely on NoSnoop?
- Christian pointed out that some accesses must be uncached even when
not using WC. What kind of accesses are those? And do they access
system RAM?