drm/amdgpu: avoid integer overflow warning in amdgpu_device_resize_fb_bar()

  From: Arnd Bergmann <arnd@arndb.de>

On 32-bit architectures comparing a resource against a value larger than
U32_MAX can cause a warning:

drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:1344:18: error: result of comparison of constant 4294967296 with expression of type 'resource_size_t' (aka 'unsigned int') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
                    res->start > 0x100000000ull)
                    ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~

The compiler is right that this cannot happen in this configuration, which
is ok, so just add a cast to shut up the warning.

Fixes: 31b8adab3247e ("drm/amdgpu: require a root bus window above 4GB for BAR resize")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

Am 03.07.23 um 14:35 schrieb Arnd Bergmann:
> From: Arnd Bergmann <arnd@arndb.de>
>
> On 32-bit architectures comparing a resource against a value larger than
> U32_MAX can cause a warning:
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:1344:18: error: result of comparison of constant 4294967296 with expression of type 'resource_size_t' (aka 'unsigned int') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
>                      res->start > 0x100000000ull)
>                      ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~
>
> The compiler is right that this cannot happen in this configuration, which
> is ok, so just add a cast to shut up the warning.

Well it doesn't make sense to compile that driver on systems with only 
32bit phys_addr_t in the first place.

It might be cleaner to just not build the whole driver on such systems 
or at least leave out this function.

Regards,
Christian

>
> Fixes: 31b8adab3247e ("drm/amdgpu: require a root bus window above 4GB for BAR resize")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 7f069e1731fee..abd13942aac5d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -1341,7 +1341,7 @@ int amdgpu_device_resize_fb_bar(struct amdgpu_device *adev)
>   
>   	pci_bus_for_each_resource(root, res, i) {
>   		if (res && res->flags & (IORESOURCE_MEM | IORESOURCE_MEM_64) &&
> -		    res->start > 0x100000000ull)
> +		    (u64)res->start > 0x100000000ull)
>   			break;
>   	}
>
  

On Tue, Jul 4, 2023, at 08:54, Christian König wrote:
> Am 03.07.23 um 14:35 schrieb Arnd Bergmann:
>> From: Arnd Bergmann <arnd@arndb.de>
>>
>> On 32-bit architectures comparing a resource against a value larger than
>> U32_MAX can cause a warning:
>>
>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:1344:18: error: result of comparison of constant 4294967296 with expression of type 'resource_size_t' (aka 'unsigned int') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
>>                      res->start > 0x100000000ull)
>>                      ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~
>>
>> The compiler is right that this cannot happen in this configuration, which
>> is ok, so just add a cast to shut up the warning.
>
> Well it doesn't make sense to compile that driver on systems with only 
> 32bit phys_addr_t in the first place.

Not sure I understand the specific requirement. Do you mean the entire
amdgpu driver requires 64-bit BAR addressing, or just the bits that
resize the BARs?

> It might be cleaner to just not build the whole driver on such systems 
> or at least leave out this function.

How about this version? This also addresses the build failure, but
I don't know if this makes any sense:

--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1325,6 +1325,9 @@ int amdgpu_device_resize_fb_bar(struct amdgpu_device *adev)
        u16 cmd;
        int r;
 
+       if (!IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT))
+               return 0;
+
        /* Bypass for VF */
        if (amdgpu_sriov_vf(adev))
                return 0;

     Arnd
  

Am 04.07.23 um 14:24 schrieb Arnd Bergmann:
> On Tue, Jul 4, 2023, at 08:54, Christian König wrote:
>> Am 03.07.23 um 14:35 schrieb Arnd Bergmann:
>>> From: Arnd Bergmann <arnd@arndb.de>
>>>
>>> On 32-bit architectures comparing a resource against a value larger than
>>> U32_MAX can cause a warning:
>>>
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:1344:18: error: result of comparison of constant 4294967296 with expression of type 'resource_size_t' (aka 'unsigned int') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
>>>                       res->start > 0x100000000ull)
>>>                       ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~
>>>
>>> The compiler is right that this cannot happen in this configuration, which
>>> is ok, so just add a cast to shut up the warning.
>> Well it doesn't make sense to compile that driver on systems with only
>> 32bit phys_addr_t in the first place.
> Not sure I understand the specific requirement. Do you mean the entire
> amdgpu driver requires 64-bit BAR addressing, or just the bits that
> resize the BARs?

Well a bit of both.

Modern AMD GPUs have 16GiB of local memory (VRAM), making those 
accessible to a CPU which can only handle 32bit addresses by resizing 
the BAR is impossible to begin with.

But going a step further even without resizing it is pretty hard to get 
that hardware working on such an architecture.

>> It might be cleaner to just not build the whole driver on such systems
>> or at least leave out this function.
> How about this version? This also addresses the build failure, but
> I don't know if this makes any sense:
>
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -1325,6 +1325,9 @@ int amdgpu_device_resize_fb_bar(struct amdgpu_device *adev)
>          u16 cmd;
>          int r;
>   
> +       if (!IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT))
> +               return 0;
> +

Yes, if that suppresses the warning as well then that makes perfect 
sense to me.

Regards,
Christian.

>          /* Bypass for VF */
>          if (amdgpu_sriov_vf(adev))
>                  return 0;
>
>       Arnd
  

On Tue, Jul 4, 2023, at 14:33, Christian König wrote:
> Am 04.07.23 um 14:24 schrieb Arnd Bergmann:
>> On Tue, Jul 4, 2023, at 08:54, Christian König wrote:
>>> Am 03.07.23 um 14:35 schrieb Arnd Bergmann:
>> Not sure I understand the specific requirement. Do you mean the entire
>> amdgpu driver requires 64-bit BAR addressing, or just the bits that
>> resize the BARs?
>
> Well a bit of both.
>
> Modern AMD GPUs have 16GiB of local memory (VRAM), making those 
> accessible to a CPU which can only handle 32bit addresses by resizing 
> the BAR is impossible to begin with.
>
> But going a step further even without resizing it is pretty hard to get 
> that hardware working on such an architecture.

I'd still like to understand this part better, as we have a lot of
arm64 chips with somewhat flawed PCIe implementations, often with
a tiny 64-bit memory space, but otherwise probably capable of
using a GPU.

What exactly do you expect to happen here?

a) Use only part of the VRAM but otherwise work as expected
b) Access all of the VRAM, but at a performance cost for
   bank switching?
c) Require kernel changes to make a) or b) work, otherwise
   fail to load
d) have no chance of working even with driver changes

>>> It might be cleaner to just not build the whole driver on such systems
>>> or at least leave out this function.
>> How about this version? This also addresses the build failure, but
>> I don't know if this makes any sense:
>>
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -1325,6 +1325,9 @@ int amdgpu_device_resize_fb_bar(struct amdgpu_device *adev)
>>          u16 cmd;
>>          int r;
>>   
>> +       if (!IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT))
>> +               return 0;
>> +
>
> Yes, if that suppresses the warning as well then that makes perfect 
> sense to me.

Ok, I'll send that as a v2 then.

    Arnd
  

Am 04.07.23 um 16:31 schrieb Arnd Bergmann:
> On Tue, Jul 4, 2023, at 14:33, Christian König wrote:
>> Am 04.07.23 um 14:24 schrieb Arnd Bergmann:
>>> On Tue, Jul 4, 2023, at 08:54, Christian König wrote:
>>>> Am 03.07.23 um 14:35 schrieb Arnd Bergmann:
>>> Not sure I understand the specific requirement. Do you mean the entire
>>> amdgpu driver requires 64-bit BAR addressing, or just the bits that
>>> resize the BARs?
>> Well a bit of both.
>>
>> Modern AMD GPUs have 16GiB of local memory (VRAM), making those
>> accessible to a CPU which can only handle 32bit addresses by resizing
>> the BAR is impossible to begin with.
>>
>> But going a step further even without resizing it is pretty hard to get
>> that hardware working on such an architecture.
> I'd still like to understand this part better, as we have a lot of
> arm64 chips with somewhat flawed PCIe implementations, often with
> a tiny 64-bit memory space, but otherwise probably capable of
> using a GPU.

Yeah, those are unfortunately very well known to us :(

> What exactly do you expect to happen here?
>
> a) Use only part of the VRAM but otherwise work as expected
> b) Access all of the VRAM, but at a performance cost for
>     bank switching?

We have tons of x86 systems where we can't resize the BAR (because of 
lack of BIOS setup of the root PCIe windows). So bank switching is still 
perfectly supported.

> c) Require kernel changes to make a) or b) work, otherwise
>     fail to load
> d) have no chance of working even with driver changes

Yeah, that is usually what happens on those arm64 system with flawed 
PCIe implementations.

The problem is not even BAR resize, basically we already had tons of 
customers which came to us and complained that amdgpu doesn't load or 
crashes the system after a few seconds.

After investigating (which sometimes even includes involving engineers 
from ARM) we usually find that those boards doesn't even remotely comply 
to the PCIe specification, both regarding power as well as functional 
things like DMA coherency.

Regards,
Christian.

>
>>>> It might be cleaner to just not build the whole driver on such systems
>>>> or at least leave out this function.
>>> How about this version? This also addresses the build failure, but
>>> I don't know if this makes any sense:
>>>
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -1325,6 +1325,9 @@ int amdgpu_device_resize_fb_bar(struct amdgpu_device *adev)
>>>           u16 cmd;
>>>           int r;
>>>    
>>> +       if (!IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT))
>>> +               return 0;
>>> +
>> Yes, if that suppresses the warning as well then that makes perfect
>> sense to me.
> Ok, I'll send that as a v2 then.
>
>      Arnd
  

On Tue, Jul 4, 2023, at 16:51, Christian König wrote:
> Am 04.07.23 um 16:31 schrieb Arnd Bergmann:
>> On Tue, Jul 4, 2023, at 14:33, Christian König wrote:
>>>
>>> Modern AMD GPUs have 16GiB of local memory (VRAM), making those
>>> accessible to a CPU which can only handle 32bit addresses by resizing
>>> the BAR is impossible to begin with.
>>>
>>> But going a step further even without resizing it is pretty hard to get
>>> that hardware working on such an architecture.
>> I'd still like to understand this part better, as we have a lot of
>> arm64 chips with somewhat flawed PCIe implementations, often with
>> a tiny 64-bit memory space, but otherwise probably capable of
>> using a GPU.
>
> Yeah, those are unfortunately very well known to us :(
>
>> What exactly do you expect to happen here?
>>
>> a) Use only part of the VRAM but otherwise work as expected
>> b) Access all of the VRAM, but at a performance cost for
>>     bank switching?
>
> We have tons of x86 systems where we can't resize the BAR (because of 
> lack of BIOS setup of the root PCIe windows). So bank switching is still 
> perfectly supported.

Ok, good.

> After investigating (which sometimes even includes involving engineers 
> from ARM) we usually find that those boards doesn't even remotely comply 
> to the PCIe specification, both regarding power as well as functional 
> things like DMA coherency.

Makes sense, the power usage is clearly going to make this
impossible on a lot of boards. I would have expected noncoherent
DMA to be a solvable problem, since that generally works with
all drivers that use the dma-mapping interfaces correctly,
but I understand that drivers/gpu/* often does its own thing
here, which may make that harder.

     Arnd
  

Am 04.07.23 um 17:24 schrieb Arnd Bergmann:
> On Tue, Jul 4, 2023, at 16:51, Christian König wrote:
>> Am 04.07.23 um 16:31 schrieb Arnd Bergmann:
>>> On Tue, Jul 4, 2023, at 14:33, Christian König wrote:
>>>> Modern AMD GPUs have 16GiB of local memory (VRAM), making those
>>>> accessible to a CPU which can only handle 32bit addresses by resizing
>>>> the BAR is impossible to begin with.
>>>>
>>>> But going a step further even without resizing it is pretty hard to get
>>>> that hardware working on such an architecture.
>>> I'd still like to understand this part better, as we have a lot of
>>> arm64 chips with somewhat flawed PCIe implementations, often with
>>> a tiny 64-bit memory space, but otherwise probably capable of
>>> using a GPU.
>> Yeah, those are unfortunately very well known to us :(
>>
>>> What exactly do you expect to happen here?
>>>
>>> a) Use only part of the VRAM but otherwise work as expected
>>> b) Access all of the VRAM, but at a performance cost for
>>>      bank switching?
>> We have tons of x86 systems where we can't resize the BAR (because of
>> lack of BIOS setup of the root PCIe windows). So bank switching is still
>> perfectly supported.
> Ok, good.
>
>> After investigating (which sometimes even includes involving engineers
>> from ARM) we usually find that those boards doesn't even remotely comply
>> to the PCIe specification, both regarding power as well as functional
>> things like DMA coherency.
> Makes sense, the power usage is clearly going to make this
> impossible on a lot of boards. I would have expected noncoherent
> DMA to be a solvable problem, since that generally works with
> all drivers that use the dma-mapping interfaces correctly,
> but I understand that drivers/gpu/* often does its own thing
> here, which may make that harder.

Yeah, I've heard that before. The problem is simply that the dma-mapping 
interface can't handle those cases.

User space APIs like Vulkan and some OpenGL extensions make a coherent 
memory model between GPU and CPU mandatory.

In other words you have things like ring buffers between code running on 
the GPU and code running on the CPU and the kernel is not even involved 
in that communication.

This is all based on the PCIe specification which makes it quite clear 
that things like snooping caches is mandatory for a compliant root complex.

There has been success to some degree by making everything uncached, but 
then the performance just sucks so badly that you can practically forget 
it as well.

Regards,
Christian.

>
>       Arnd