[RFC PATCH 1/2] drm/amdgpu: amdgpu_vram_mgr_new(): Clamp lpfn to total vram

[Paneer Selvam, Arunpravin]

On 5/12/2025 2:03 AM, Paneer Selvam, Arunpravin wrote:
>
>
> On 5/3/2025 5:53 PM, Paneer Selvam, Arunpravin wrote:
>>
>>
>> On 5/2/2025 9:02 PM, John Olender wrote:
>>> On 4/30/25 5:44 PM, Paneer Selvam, Arunpravin wrote:
>>>>
>>>> On 5/1/2025 2:50 AM, Alex Deucher wrote:
>>>>> + Christian
>>>>>
>>>>> On Tue, Apr 29, 2025 at 7:24 AM John Olender <john.olender at gmail.com>
>>>>> wrote:
>>>>>> The drm_mm allocator tolerated being passed end > mm->size, but the
>>>>>> drm_buddy allocator does not.
>>>>>>
>>>>>> Restore the pre-buddy-allocator behavior of allowing such 
>>>>>> placements.
>>>>>>
>>>>>> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3448
>>>>>> Signed-off-by: John Olender <john.olender at gmail.com>
>>>>> This looks correct to me.
>>>>> Reviewed-by: Alex Deucher <alexander.deucher at amd.com>
>>>> I was thinking that we should return an error when lpfn > man->size.
>>>>
>>>> Regards,
>>>> Arun.
>>> This patch restores the previous behavior in the spirit of "Do not 
>>> crash
>>> the kernel".  The existing uvd placements are pretty clear in their
>>> intent and were accepted until the switch to drm_buddy.  I think it's
>>> fair to consider their style as expected.
>>>
>>> With that in mind, I'm not sure amdgpu_vram_mgr is the place this 
>>> change
>>> really belongs.  That is, I think it's worth asking:
>>>
>>> 1) Why does drm_mm accept end > mm->size without complaint?
>>> 2) Why doesn't drm_buddy do the same?
>>
>> I remember that during the development of DRM buddy , we had a 
>> discussion with Intel folks and decided to
>> return an error in DRM buddy when end > mm->size. This was done to 
>> ensure that, at the driver level,  lpfn
>> has the correct value.
>>
>> I will modify this at drm_buddy to match with drm_mm and send the patch.
> After giving it some thought, I think it is more effective to 
> implement this tolerance at the VRAM manager level
> and allow the DRM buddy manager to perform a strict validation, as 
> this is necessary for other graphics drivers
> (e.g., i915).

Reviewed-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam at amd.com>
>
> Regards,
> Arun.
>>
>> Regards,
>> Arun.
>>>
>>> Thanks,
>>> John
>>>
>>>>>> ---
>>>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 2 +-
>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c b/drivers/
>>>>>> gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>>>>>> index 2d7f82e98df9..abdc52b0895a 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>>>>>> @@ -463,7 +463,7 @@ static int amdgpu_vram_mgr_new(struct
>>>>>> ttm_resource_manager *man,
>>>>>>           int r;
>>>>>>
>>>>>>           lpfn = (u64)place->lpfn << PAGE_SHIFT;
>>>>>> -       if (!lpfn)
>>>>>> +       if (!lpfn || lpfn > man->size)
>>>>>>                   lpfn = man->size;
>>>>>>
>>>>>>           fpfn = (u64)place->fpfn << PAGE_SHIFT;
>>>>>> -- 
>>>>>> 2.47.2
>>>>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20250512/ca60c026/attachment-0001.htm>