[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 at 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);
>>>>>>
>>>
>