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

["Paneer Selvam, Arunpravin" <arunpravin.paneerselvam@amd.com>]

On 5/15/2025 9:19 PM, Paneer Selvam, Arunpravin wrote:
>
>
> On 5/12/2025 12:41 PM, Paneer Selvam, Arunpravin wrote:
>>
>>
>> On 5/12/2025 12:39 PM, Christian König wrote:
>>>
>>> On 5/11/25 22:37, Paneer Selvam, Arunpravin wrote:
>>>>
>>>> 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@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@gmail.com>
>>>>>>>>> This looks correct to me.
>>>>>>>>> Reviewed-by: Alex Deucher <alexander.deucher@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@amd.com>
>>> Ok in that case please pick this patch up and make sure that it land 
>>> in amd-staging-drm-next Arun.
>>>
>>> Alex most likely won't follow the discussion till the end.
>> Sure Christian, I will merge this patch into amd-staging-drm-next.
> I see a black screen problem during the amdgpu driver load on the tip 
> of amd-staging-drm-next,
> once that issue is fixed, I will push this patch into 
> amd-staging-drm-next.
I have merged this patch into amd-staging-drm-next.

Thanks,
Arun.
>
> Regards,
> Arun.
>>
>> Thanks,
>> Arun.
>>>
>>> Thanks,
>>> Christian.
>>>
>>>>> 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
>>>>>>>>>>
>>
>