Re: [Intel-gfx] [PATCH v3 06/12] drm/ttm: Add a generic TTM memcpy move for page-based iomem

From: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
To: "Christian König" <christian.koenig@amd.com>,
	"Matthew Auld" <matthew.william.auld@gmail.com>
Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	ML dri-devel <dri-devel@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH v3 06/12] drm/ttm: Add a generic TTM memcpy move for page-based iomem
Date: Wed, 26 May 2021 12:57:10 +0200	[thread overview]
Message-ID: <fbbb896e7e886e4d41b3a319c6aa7c5bb2e8ad4e.camel@linux.intel.com> (raw)
In-Reply-To: <9f8fe3af-6d49-f652-70bc-36a27007d821@amd.com>

On Wed, 2021-05-26 at 12:45 +0200, Christian König wrote:
> Am 26.05.21 um 09:39 schrieb Thomas Hellström:
> > [SNIP]
> > > > I think the long term goal is to use memremap all over the
> > > > place, to
> > > > just not have to bother with the __iomem annotation. But to do
> > > > that io-
> > > > mapping.h needs to support memremap. But for now we need to be
> > > > strict
> > > > about __iomem unless we're in arch specific code. That's why
> > > > that
> > > > dma_buf_map thing was created, but TTM memcpy was never fully
> > > > adapted.
> > > 
> > > I don't think that this will work. __iomem annotation is there 
> > > because we have architectures where you need to use special CPU 
> > > instructions for iomem access.
> > > 
> > > That won't go away just because we use memremap().
> > 
> > That's true, but can we ever support those with TTM, given that we 
> > allow user-space mmaping that transparently may change to an iomap?
> > Given that, and what's written here
> > 
> >  
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flwn.net%2FArticles%2F653585%2F&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C1cdcfe9d20e740308c9e08d92019785b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637576116034492654%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=e2BFGQJcElwVxrvHcnALDWscHN7ernLekGvXHqWBcwY%3D&amp;reserved=0
> >  
> > 
> > 
> > To me it sounds like if an architecture can't support memremap, we 
> > can't support it with TTM either.
> 
> That was also my argument, but this is unfortunately not true.
> 
> We already have architectures where the __iomem approach is mandatory
> for kernel mappings, but work fine for userspace (don't ask me how
> that 
> works, idk).

Ugh. :/

> 
> That's the reason why we had to fix the UVD firmware upload in the
> kernel:
> 
> commit ba0b2275a6781b2f3919d931d63329b5548f6d5f
> Author: Christian König <christian.koenig@amd.com>
> Date:   Tue Aug 23 11:00:17 2016 +0200
> 
>      drm/amdgpu: use memcpy_to/fromio for UVD fw upload
> 
> > 
> > In any case for this particular patch, to avoid potential
> > regressions, 
> > OK if I just add an ioremap() in case the memremap fails?
> 
> Well because of the issues outlined above I would actually prefer if
> we 
> can keep the __iomem annotation around.

Well, we'd do that. Since we use the dma_buf_map unconditionally.

So what would happen in the above case, would be:

- memremap would fail. (Otherwise I'd be terribly confused)
- we retry with ioremap and the dma-buf-map member is_iomem would thus
be set
- memcpy would do the right thing, based on is_iomem.

/Thomas

> 
> Christian.
> 
> > 
> > /Thomas
> > 
> > 
> > > 
> > > Christian.
> > > 
> > > > 
> > > > As for limited arch support for memremap cached, It looks like
> > > > we only
> > > > need to or in "backup" mapping modes in the memremap flags, and
> > > > we'd
> > > > mimic the previous behaviour.
> > > > 
> > > > /Thomas
> > > > 
> > > > 
> > > > > > /Thomas
> > > > > > 
> > > > > > 
> > > > 
> > > 
> 