Message-ID: <CAKv+Gu_JyO2KNtdN1-_TcoL_kqs0yswdwBrpAXA6-jyWuDrj+g@mail.gmail.com>
Date:   Mon, 14 Jan 2019 18:32:10 +0100
From:   Ard Biesheuvel <ard.biesheuvel@...aro.org>
To:     "Koenig, Christian" <Christian.Koenig@....com>
Cc:     Michel Dänzer <michel@...nzer.net>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Carsten Haitzler <Carsten.Haitzler@....com>,
        David Airlie <airlied@...ux.ie>,
        Will Deacon <will.deacon@....com>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        "Huang, Ray" <Ray.Huang@....com>,
        "Zhang, Jerry" <Jerry.Zhang@....com>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        Bernhard Rosenkränzer 
        <Bernhard.Rosenkranzer@...aro.org>
Subject: Re: [RFC PATCH] drm/ttm: force cached mappings for system RAM on ARM

On Mon, 14 Jan 2019 at 12:38, Koenig, Christian
<Christian.Koenig@....com> wrote:
>
> Am 14.01.19 um 11:53 schrieb Ard Biesheuvel:
> > On Thu, 10 Jan 2019 at 10:34, Michel Dänzer <michel@...nzer.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@....com>
> >>> Cc: Huang Rui <ray.huang@....com>
> >>> Cc: Junwei Zhang <Jerry.Zhang@....com>
> >>> Cc: David Airlie <airlied@...ux.ie>
> >>> Reported-by: Carsten Haitzler <Carsten.Haitzler@....com>
> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@...aro.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/ttm_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, pgprot_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 = 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).
>
> This is still rather problematic.
>
> The issue is that we often don't create a vmap for a page, but rather
> access the page directly using the linear mapping.
>
> So we would use the wrong access type here.
>

Yes. But how are these accesses done? Are they wrapped in a kmap()?

> > - 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.
>
> Actually it is the other way around. The CPU thinks some data is in the
> cache and the GPU only updates the system memory version because the
> snoop flag is not set.
>

That doesn't seem to be what is happening. As far as we can tell from
our experiments, all inbound transactions are always cacheable, and so
the only way to make things work is to ensure that the CPU uses the
same attributes.

> > 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?
>
> The GPU has a separate page walker in the MC and the page tables there
> have a bits saying if the access should go to the PCIe bus and if yes if
> the snoop bit should be set.
>
> > - 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?
>
> On some hardware generations we have a buggy engine which fails to
> forward the snoop bit and because of this the system memory page used by
> this engine must be uncached. But this only applies if you use ROCm in a
> specific configuration.
>

OK. I don't know what that means tbh. Does this apply to both radeon and amdgpu?

Powered by blists - more mailing lists