[PATCH] drm/radeon: Fix screen corruption (v2)

[Robin Murphy]

On 2022-12-15 11:40, Luben Tuikov wrote:
> On 2022-12-15 06:27, Christian König wrote:
>> Am 15.12.22 um 11:19 schrieb Luben Tuikov:
>>> On 2022-12-15 04:46, Christian König wrote:
>>>> Am 15.12.22 um 10:08 schrieb Luben Tuikov:
>>>>> On 2022-12-15 03:07, Christian König wrote:
>>>>>> Am 15.12.22 um 00:08 schrieb Robin Murphy:
>>>>>>> On 2022-12-14 22:02, Alex Deucher wrote:
>>>>>>>> On Wed, Dec 14, 2022 at 4:54 PM Robin Murphy <robin.murphy at arm.com>
>>>>>>>> wrote:
>>>>>>>>> On 2022-12-12 02:08, Luben Tuikov wrote:
>>>>>>>>>> Fix screen corruption on older 32-bit systems using AGP chips.
>>>>>>>>>>
>>>>>>>>>> On older systems with little memory, for instance 1.5 GiB, using an
>>>>>>>>>> AGP chip,
>>>>>>>>>> the device's DMA mask is 0xFFFFFFFF, but the memory mask is
>>>>>>>>>> 0x7FFFFFF, and
>>>>>>>>>> subsequently dma_addressing_limited() returns 0xFFFFFFFF < 0x7FFFFFFF,
>>>>>>>>>> false. As such the result of this static inline isn't suitable for
>>>>>>>>>> the last
>>>>>>>>>> argument to ttm_device_init()--it simply needs to now whether to
>>>>>>>>>> use GFP_DMA32
>>>>>>>>>> when allocating DMA buffers.
>>>>>>>>> This sounds wrong to me. If the issues happen on systems without PAE it
>>>>>>>>> clearly can't have anything to with the actual DMA address size. Not to
>>>>>>>>> mention that AFAICS 32-bit x86 doesn't even have ZONE_DMA32, so
>>>>>>>>> GFP_DMA32 would be functionally meaningless anyway. Although the
>>>>>>>>> reported symptoms initially sounded like they could be caused by DMA
>>>>>>>>> going to the wrong place, that is also equally consistent with a
>>>>>>>>> loss of
>>>>>>>>> cache coherency.
>>>>>>>>>
>>>>>>>>> My (limited) understanding of AGP is that the GART can effectively
>>>>>>>>> alias
>>>>>>>>> memory to a second physical address, so I could well believe that
>>>>>>>>> something somewhere in the driver stack needs to perform some cache
>>>>>>>>> maintenance to avoid coherency issues, and that in these particular
>>>>>>>>> setups whatever that is might be assuming the memory is direct-mapped
>>>>>>>>> and thus going wrong for highmem pages.
>>>>>>>>>
>>>>>>>>> So as I said before, I really think this is not about using
>>>>>>>>> GFP_DMA32 at
>>>>>>>>> all, but about *not* using GFP_HIGHUSER.
>>>>>>>> One of the wonderful features of AGP is that it has to be used with
>>>>>>>> uncached memory.  The aperture basically just provides a remapping of
>>>>>>>> physical pages into a linear aperture that you point the GPU at.  TTM
>>>>>>>> has to jump through quite a few hoops to get uncached memory in the
>>>>>>>> first place, so it's likely that that somehow isn't compatible with
>>>>>>>> HIGHMEM.  Can you get uncached HIGHMEM?
>>>>>>> I guess in principle yes, if you're careful not to use regular
>>>>>>> kmap()/kmap_atomic(), and always use pgprot_noncached() for
>>>>>>> userspace/vmalloc mappings, but clearly that leaves lots of scope for
>>>>>>> slipping up.
>>>>>> I theory we should do exactly that in TTM, but we have very few users
>>>>>> who actually still exercise that functionality.
>>>>>>
>>>>>>> Working backwards from primitives like set_memory_uc(), I see various
>>>>>>> paths in TTM where manipulating the caching state is skipped for
>>>>>>> highmem pages, but I wouldn't even know where to start looking for
>>>>>>> whether the right state is propagated to all the places where they
>>>>>>> might eventually be mapped somewhere.
>>>>>> The tt object has the caching state for the pages and
>>>>>> ttm_prot_from_caching() then uses pgprot_noncached() and co for the
>>>>>> userspace/vmalloc mappings.
>>>>>>
>>>>> The point of this patch is that dma_addressing_limited() is unsuitable as
>>>>> the last parameter to ttm_pool_init(), since if it is "false"--as it is in this
>>>>> particular case--then TTM ends up using HIGHUSER, and we get the screen corruption.
>>>>> (gfp_flags |= GFP_HIGHUSER in in ttm_pool_alloc())
>>>> Well I would rather say that dma_addressing_limited() works, but the
>>>> default value from dma_get_required_mask() is broken.
>>>>
>>> dma_get_required_mask() for his setup of 1.5 GiB of memory returns 0x7FFFFFF.
>>
>> This 0x7FFFFFF mask looks fishy to me. That would only be 128MiB
>> addressable memory (27 bits set)? Or is there another F missing?
> 
> Yeah, I'm missing an F--it is correctly described at the top of the thread above,
> i.e. in the commit of v2 patch.
> 
> 0x7FFF_FFFF, which seems correct, no?
> 
>>> While the dma mask is 0xFFFFFFFF, as set in radeon_device.c in radeon_device_init().
>>>
>>>> 32 bits only work with bounce buffers and we can't use those on graphics
>>>> hardware.
>>>>
>>>>> Is there an objection to this patch, if it fixes the screen corruption?
>>>> Not from my side, but fixing the underlying issues would be better I think.
>>>>
>>> Have they been identified?
>>
>> I'm not 100% sure. I think by using GFP_DMA32 we just work around the
>> issue somehow.
> 
> Right. Using GFP_DMA32, we don't touch high-mem. I was looking at the DRM
> code trying to understand what we do when GFP_DMA32 is not set, and the immediate
> thing I see is that we set GFP_HIGHUSER when use_dma32 is unset in the device struct.
> (Then I got down to the caching attributes...)
> 
> It's be nice if we can find the actual issue--what else would it show us that needs fixing...?
> 
> So what do we do with this patch?
> 
> Shouldn't leave it in a limbo--some OSes ship their kernel
> with 33b3ad3788ab ("drm/radeon: handle PCIe root ports with addressing limitations") wholly
> reverted.

Removing dma_addressing_limited() is still wrong, for the reasons given 
in that commit. What we need is an *additional* condition that 
encapsulates "also pass use_dma32 for AGP devices because it avoids some 
weird coherency issue with 32-bit highmem that isn't worth trying to 
debug further".

Thanks,
Robin.