[PATCH] drm/[amdgpu|radeon]: fix memset on io mem

[Christian König]

Am 17.12.20 um 14:45 schrieb Robin Murphy:
> On 2020-12-17 10:25, Christian König wrote:
>> Am 17.12.20 um 02:07 schrieb Chen Li:
>>> On Wed, 16 Dec 2020 22:19:11 +0800,
>>> Christian König wrote:
>>>> Am 16.12.20 um 14:48 schrieb Chen Li:
>>>>> On Wed, 16 Dec 2020 15:59:37 +0800,
>>>>> Christian König wrote:
>>>>>> [SNIP]
>>>>> Hi, Christian. I'm not sure why this change is a hack here. I 
>>>>> cannot see the problem and wll be grateful if you give more 
>>>>> explainations.
>>>> __memset is supposed to work on those addresses, otherwise you 
>>>> can't use the
>>>> e8860 on your arm64 system.
>>> If __memset is supposed to work on those adresses, why this 
>>> commit(https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftorvalds%2Flinux%2Fcommit%2Fba0b2275a6781b2f3919d931d63329b5548f6d5f&data=04%7C01%7Cchristian.koenig%40amd.com%7C3551ae4972b044bb831608d8a291f81c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637438095114292394%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=xns81uCGfN1tjsVn5LBU8QhmUinZRJQlXz8w%2FJ7%2FGTM%3D&reserved=0) 
>>> is needed? (I also notice drm/radeon didn't take this change though) 
>>> just out of curiosity.
>>
>> We generally accept those patches as cleanup in the kernel with the 
>> hope that we can find a way to work around the userspace restrictions.
>>
>> But when you also have this issue in userspace then there isn't much 
>> we can do for you.
>>
>>>> Replacing the the direct write in the kernel with calls to writel() or
>>>> memset_io() will fix that temporary, but you have a more general 
>>>> problem here.
>>> I cannot see what's the more general problem here :( u mean 
>>> performance?
>>
>> No, not performance. See standards like OpenGL, Vulkan as well as 
>> VA-API and VDPAU require that you can mmap() device memory and 
>> execute memset/memcpy on the memory from userspace.
>>
>> If your ARM base board can't do that for some then you can't use the 
>> hardware with that board.
>
> If the VRAM lives in a prefetchable PCI bar then on most sane 
> Arm-based systems I believe it should be able to mmap() to userspace 
> with the Normal memory type, where unaligned accesses and such are 
> allowed, as opposed to the Device memory type intended for MMIO 
> mappings, which has more restrictions but stricter ordering guarantees.

Do you have some background why some ARM boards fail with that?

We had a couple of reports that memset/memcpy fail in userspace (usually 
system just spontaneously reboots or becomes unresponsive), but so far 
nobody could tell us why that happens?

>
> Regardless of what happens elsewhere though, if something is mapped 
> *into the kernel* with ioremap(), then it is fundamentally wrong per 
> the kernel memory model to reference that mapping directly without 
> using I/O accessors. That is not specific to any individual 
> architecture, and Sparse should be screaming about it already. I guess 
> in this case the UVD code needs to pay more attention to whether 
> radeon_bo_kmap() ends up going via ttm_bo_ioremap() or not.

Yes, exactly. That's why we already have memcpy_fromio()/memcpy_toio() 
to upload the firmware and save the state on suspend/resume.

It's just that in this case here we also have IO memory because some 15+ 
years old AGP based hardware doesn't work when you but it in system 
memory :)

So pointing that out is correct and I'm going to clean that up now.

Regards,
Christian.

>
> (I'm assuming the initial fault was memset() with 0 trying to perform 
> "DC ZVA" on a Device-type mapping from ioremap() - FYI a stacktrace on 
> its own without the rest of the error dump showing what actually 
> triggered it isn't overly useful)
>
> Robin.