[PATCH 1/1] drm/amdgpu: return early on error while setting bar0 memtype

[PATCH 1/1] drm/amdgpu: return early on error while setting bar0 memtype

[Nirmoy Das]

We set WC memtype for aper_base but don't check return value
of arch_io_reserve_memtype_wc(). Be more defensive and return
early on error.

Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 073ba2af0b9c..6b25982a9077 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -1032,9 +1032,14 @@ int amdgpu_bo_init(struct amdgpu_device *adev)
 	/* On A+A platform, VRAM can be mapped as WB */
 	if (!adev->gmc.xgmi.connected_to_cpu) {
 		/* reserve PAT memory space to WC for VRAM */
-		arch_io_reserve_memtype_wc(adev->gmc.aper_base,
+		int r = arch_io_reserve_memtype_wc(adev->gmc.aper_base,
 				adev->gmc.aper_size);
 
+		if (r) {
+			DRM_ERROR("Unable to set WC memtype for the aperture base\n");
+			return r;
+		}
+
 		/* Add an MTRR for the VRAM */
 		adev->gmc.vram_mtrr = arch_phys_wc_add(adev->gmc.aper_base,
 				adev->gmc.aper_size);
-- 
2.33.1

Re: [PATCH 1/1] drm/amdgpu: return early on error while setting bar0 memtype

[Christian König]

Am 29.10.21 um 17:09 schrieb Nirmoy Das:
> We set WC memtype for aper_base but don't check return value
> of arch_io_reserve_memtype_wc(). Be more defensive and return
> early on error.
>
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 073ba2af0b9c..6b25982a9077 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -1032,9 +1032,14 @@ int amdgpu_bo_init(struct amdgpu_device *adev)
>   	/* On A+A platform, VRAM can be mapped as WB */
>   	if (!adev->gmc.xgmi.connected_to_cpu) {
>   		/* reserve PAT memory space to WC for VRAM */
> -		arch_io_reserve_memtype_wc(adev->gmc.aper_base,
> +		int r = arch_io_reserve_memtype_wc(adev->gmc.aper_base,
>   				adev->gmc.aper_size);
>   
> +		if (r) {
> +			DRM_ERROR("Unable to set WC memtype for the aperture base\n");

Drop the word "base" and rather write "VRAM aperture".

With that done the patch is Reviewed-by: Christian König 
<christian.koenig@amd.com>

Regards,
Christian.

> +			return r;
> +		}
> +
>   		/* Add an MTRR for the VRAM */
>   		adev->gmc.vram_mtrr = arch_phys_wc_add(adev->gmc.aper_base,
>   				adev->gmc.aper_size);

Re: [PATCH 1/1] drm/amdgpu: return early on error while setting bar0 memtype

[Lazar, Lijo]

On 10/29/2021 8:39 PM, Nirmoy Das wrote:
> We set WC memtype for aper_base but don't check return value
> of arch_io_reserve_memtype_wc(). Be more defensive and return
> early on error.
> 
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 073ba2af0b9c..6b25982a9077 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -1032,9 +1032,14 @@ int amdgpu_bo_init(struct amdgpu_device *adev)
>   	/* On A+A platform, VRAM can be mapped as WB */
>   	if (!adev->gmc.xgmi.connected_to_cpu) {
>   		/* reserve PAT memory space to WC for VRAM */
> -		arch_io_reserve_memtype_wc(adev->gmc.aper_base,
> +		int r = arch_io_reserve_memtype_wc(adev->gmc.aper_base,
>   				adev->gmc.aper_size);

BTW, isn't it more appropriate to use visible vram size? There are cases 
where pci resize rounds aperture to the next higher size > size of 
actual VRAM.

Thanks,
Lijo
>   
> +		if (r) {
> +			DRM_ERROR("Unable to set WC memtype for the aperture base\n");
> +			return r;
> +		}
> +
>   		/* Add an MTRR for the VRAM */
>   		adev->gmc.vram_mtrr = arch_phys_wc_add(adev->gmc.aper_base,
>   				adev->gmc.aper_size);
> 

Re: [PATCH 1/1] drm/amdgpu: return early on error while setting bar0 memtype

[Das, Nirmoy]

On 11/2/2021 9:00 AM, Lazar, Lijo wrote:
>
>
> On 10/29/2021 8:39 PM, Nirmoy Das wrote:
>> We set WC memtype for aper_base but don't check return value
>> of arch_io_reserve_memtype_wc(). Be more defensive and return
>> early on error.
>>
>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> index 073ba2af0b9c..6b25982a9077 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> @@ -1032,9 +1032,14 @@ int amdgpu_bo_init(struct amdgpu_device *adev)
>>       /* On A+A platform, VRAM can be mapped as WB */
>>       if (!adev->gmc.xgmi.connected_to_cpu) {
>>           /* reserve PAT memory space to WC for VRAM */
>> -        arch_io_reserve_memtype_wc(adev->gmc.aper_base,
>> +        int r = arch_io_reserve_memtype_wc(adev->gmc.aper_base,
>>                   adev->gmc.aper_size);
>
> BTW, isn't it more appropriate to use visible vram size? There are 
> cases where pci resize rounds aperture to the next higher size > size 
> of actual VRAM.


Good point, I will update this one and send again.


Regards,

Nirmoy


>
> Thanks,
> Lijo
>>   +        if (r) {
>> +            DRM_ERROR("Unable to set WC memtype for the aperture 
>> base\n");
>> +            return r;
>> +        }
>> +
>>           /* Add an MTRR for the VRAM */
>>           adev->gmc.vram_mtrr = arch_phys_wc_add(adev->gmc.aper_base,
>>                   adev->gmc.aper_size);
>>

Re: [PATCH 1/1] drm/amdgpu: return early on error while setting bar0 memtype

[Christian König]

Am 02.11.21 um 11:11 schrieb Das, Nirmoy:
>
> On 11/2/2021 9:00 AM, Lazar, Lijo wrote:
>>
>>
>> On 10/29/2021 8:39 PM, Nirmoy Das wrote:
>>> We set WC memtype for aper_base but don't check return value
>>> of arch_io_reserve_memtype_wc(). Be more defensive and return
>>> early on error.
>>>
>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 7 ++++++-
>>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> index 073ba2af0b9c..6b25982a9077 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> @@ -1032,9 +1032,14 @@ int amdgpu_bo_init(struct amdgpu_device *adev)
>>>       /* On A+A platform, VRAM can be mapped as WB */
>>>       if (!adev->gmc.xgmi.connected_to_cpu) {
>>>           /* reserve PAT memory space to WC for VRAM */
>>> -        arch_io_reserve_memtype_wc(adev->gmc.aper_base,
>>> +        int r = arch_io_reserve_memtype_wc(adev->gmc.aper_base,
>>>                   adev->gmc.aper_size);
>>
>> BTW, isn't it more appropriate to use visible vram size? There are 
>> cases where pci resize rounds aperture to the next higher size > size 
>> of actual VRAM.
>
>
> Good point, I will update this one and send again.

Not a good idea at all.

The aperture size is rounded up to the next power of two and that's 
exactly what we should stick to if we don't want to get an error code in 
return.

Regards,
Christian.

>
>
> Regards,
>
> Nirmoy
>
>
>>
>> Thanks,
>> Lijo
>>>   +        if (r) {
>>> +            DRM_ERROR("Unable to set WC memtype for the aperture 
>>> base\n");
>>> +            return r;
>>> +        }
>>> +
>>>           /* Add an MTRR for the VRAM */
>>>           adev->gmc.vram_mtrr = arch_phys_wc_add(adev->gmc.aper_base,
>>>                   adev->gmc.aper_size);
>>>

Re: [PATCH 1/1] drm/amdgpu: return early on error while setting bar0 memtype

[Lazar, Lijo]

On 11/2/2021 4:39 PM, Christian König wrote:
> Am 02.11.21 um 11:11 schrieb Das, Nirmoy:
>>
>> On 11/2/2021 9:00 AM, Lazar, Lijo wrote:
>>>
>>>
>>> On 10/29/2021 8:39 PM, Nirmoy Das wrote:
>>>> We set WC memtype for aper_base but don't check return value
>>>> of arch_io_reserve_memtype_wc(). Be more defensive and return
>>>> early on error.
>>>>
>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 7 ++++++-
>>>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>> index 073ba2af0b9c..6b25982a9077 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>> @@ -1032,9 +1032,14 @@ int amdgpu_bo_init(struct amdgpu_device *adev)
>>>>       /* On A+A platform, VRAM can be mapped as WB */
>>>>       if (!adev->gmc.xgmi.connected_to_cpu) {
>>>>           /* reserve PAT memory space to WC for VRAM */
>>>> -        arch_io_reserve_memtype_wc(adev->gmc.aper_base,
>>>> +        int r = arch_io_reserve_memtype_wc(adev->gmc.aper_base,
>>>>                   adev->gmc.aper_size);
>>>
>>> BTW, isn't it more appropriate to use visible vram size? There are 
>>> cases where pci resize rounds aperture to the next higher size > size 
>>> of actual VRAM.
>>
>>
>> Good point, I will update this one and send again.
> 
> Not a good idea at all.
> 
> The aperture size is rounded up to the next power of two and that's 
> exactly what we should stick to if we don't want to get an error code in 
> return.
> 
PCI rebar sizes have its restrictions. It jumps from 4G to 8G to 16G and 
so on. Why we need to map 16G for a card with 12G VRAM? BTW, how it 
increases the failure chance - this mapping happens in page sizes, right?

Thanks,
Lijo

> Regards,
> Christian.
> 
>>
>>
>> Regards,
>>
>> Nirmoy
>>
>>
>>>
>>> Thanks,
>>> Lijo
>>>>   +        if (r) {
>>>> +            DRM_ERROR("Unable to set WC memtype for the aperture 
>>>> base\n");
>>>> +            return r;
>>>> +        }
>>>> +
>>>>           /* Add an MTRR for the VRAM */
>>>>           adev->gmc.vram_mtrr = arch_phys_wc_add(adev->gmc.aper_base,
>>>>                   adev->gmc.aper_size);
>>>>
> 

Re: [PATCH 1/1] drm/amdgpu: return early on error while setting bar0 memtype

[Christian König]

Am 02.11.21 um 12:18 schrieb Lazar, Lijo:
>
>
> On 11/2/2021 4:39 PM, Christian König wrote:
>> Am 02.11.21 um 11:11 schrieb Das, Nirmoy:
>>>
>>> On 11/2/2021 9:00 AM, Lazar, Lijo wrote:
>>>>
>>>>
>>>> On 10/29/2021 8:39 PM, Nirmoy Das wrote:
>>>>> We set WC memtype for aper_base but don't check return value
>>>>> of arch_io_reserve_memtype_wc(). Be more defensive and return
>>>>> early on error.
>>>>>
>>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>>>> ---
>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 7 ++++++-
>>>>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>> index 073ba2af0b9c..6b25982a9077 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>> @@ -1032,9 +1032,14 @@ int amdgpu_bo_init(struct amdgpu_device *adev)
>>>>>       /* On A+A platform, VRAM can be mapped as WB */
>>>>>       if (!adev->gmc.xgmi.connected_to_cpu) {
>>>>>           /* reserve PAT memory space to WC for VRAM */
>>>>> - arch_io_reserve_memtype_wc(adev->gmc.aper_base,
>>>>> +        int r = arch_io_reserve_memtype_wc(adev->gmc.aper_base,
>>>>>                   adev->gmc.aper_size);
>>>>
>>>> BTW, isn't it more appropriate to use visible vram size? There are 
>>>> cases where pci resize rounds aperture to the next higher size > 
>>>> size of actual VRAM.
>>>
>>>
>>> Good point, I will update this one and send again.
>>
>> Not a good idea at all.
>>
>> The aperture size is rounded up to the next power of two and that's 
>> exactly what we should stick to if we don't want to get an error code 
>> in return.
>>
> PCI rebar sizes have its restrictions. It jumps from 4G to 8G to 16G 
> and so on. Why we need to map 16G for a card with 12G VRAM? BTW, how 
> it increases the failure chance - this mapping happens in page sizes, 
> right?
>

Exactly that's the point. This mapping usually happens in power of two 
in the same way as the PCI BAR sizes. So we should use 16GiB even for a 
12GiB card here.

Only some architectures work with page size mappings (e.g. x86 with PAT 
enabled). Then we can indeed use the real VRAM size, but that is 
absolutely not guaranteed as far as I know.

Regards,
Christian.

> Thanks,
> Lijo
>
>> Regards,
>> Christian.
>>
>>>
>>>
>>> Regards,
>>>
>>> Nirmoy
>>>
>>>
>>>>
>>>> Thanks,
>>>> Lijo
>>>>>   +        if (r) {
>>>>> +            DRM_ERROR("Unable to set WC memtype for the aperture 
>>>>> base\n");
>>>>> +            return r;
>>>>> +        }
>>>>> +
>>>>>           /* Add an MTRR for the VRAM */
>>>>>           adev->gmc.vram_mtrr = arch_phys_wc_add(adev->gmc.aper_base,
>>>>>                   adev->gmc.aper_size);
>>>>>
>>

Re: [PATCH 1/1] drm/amdgpu: return early on error while setting bar0 memtype

[Das, Nirmoy]

On 11/2/2021 12:33 PM, Christian König wrote:
> Am 02.11.21 um 12:18 schrieb Lazar, Lijo:
>>
>>
>> On 11/2/2021 4:39 PM, Christian König wrote:
>>> Am 02.11.21 um 11:11 schrieb Das, Nirmoy:
>>>>
>>>> On 11/2/2021 9:00 AM, Lazar, Lijo wrote:
>>>>>
>>>>>
>>>>> On 10/29/2021 8:39 PM, Nirmoy Das wrote:
>>>>>> We set WC memtype for aper_base but don't check return value
>>>>>> of arch_io_reserve_memtype_wc(). Be more defensive and return
>>>>>> early on error.
>>>>>>
>>>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 7 ++++++-
>>>>>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>> index 073ba2af0b9c..6b25982a9077 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>> @@ -1032,9 +1032,14 @@ int amdgpu_bo_init(struct amdgpu_device 
>>>>>> *adev)
>>>>>>       /* On A+A platform, VRAM can be mapped as WB */
>>>>>>       if (!adev->gmc.xgmi.connected_to_cpu) {
>>>>>>           /* reserve PAT memory space to WC for VRAM */
>>>>>> - arch_io_reserve_memtype_wc(adev->gmc.aper_base,
>>>>>> +        int r = arch_io_reserve_memtype_wc(adev->gmc.aper_base,
>>>>>>                   adev->gmc.aper_size);
>>>>>
>>>>> BTW, isn't it more appropriate to use visible vram size? There are 
>>>>> cases where pci resize rounds aperture to the next higher size > 
>>>>> size of actual VRAM.
>>>>
>>>>
>>>> Good point, I will update this one and send again.
>>>
>>> Not a good idea at all.
>>>
>>> The aperture size is rounded up to the next power of two and that's 
>>> exactly what we should stick to if we don't want to get an error 
>>> code in return.
>>>
>> PCI rebar sizes have its restrictions. It jumps from 4G to 8G to 16G 
>> and so on. Why we need to map 16G for a card with 12G VRAM? BTW, how 
>> it increases the failure chance - this mapping happens in page sizes, 
>> right?
>>
>
> Exactly that's the point. This mapping usually happens in power of two 
> in the same way as the PCI BAR sizes. So we should use 16GiB even for 
> a 12GiB card here.
>
> Only some architectures work with page size mappings (e.g. x86 with 
> PAT enabled). Then we can indeed use the real VRAM size, but that is 
> absolutely not guaranteed as far as I know.


Thanks for clarifying this. I will push the patch with your suggested 
changes.


Regards,

Nirmoy

>
> Regards,
> Christian.
>
>> Thanks,
>> Lijo
>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>>
>>>> Regards,
>>>>
>>>> Nirmoy
>>>>
>>>>
>>>>>
>>>>> Thanks,
>>>>> Lijo
>>>>>>   +        if (r) {
>>>>>> +            DRM_ERROR("Unable to set WC memtype for the aperture 
>>>>>> base\n");
>>>>>> +            return r;
>>>>>> +        }
>>>>>> +
>>>>>>           /* Add an MTRR for the VRAM */
>>>>>>           adev->gmc.vram_mtrr = 
>>>>>> arch_phys_wc_add(adev->gmc.aper_base,
>>>>>>                   adev->gmc.aper_size);
>>>>>>
>>>
>