[Intel-gfx] [PATCH 2/2] Add drm buddy manager support to amdgpu driver

[Christian König]

Am 21.09.21 um 17:51 schrieb Paneer Selvam, Arunpravin:
> [AMD Public Use]
>
> Hi Christian,
> Please find my comments.

A better mail client might be helpful for mailing list communication. I 
use Thunderbird, but Outlook with appropriate setting should do as well.

>
> Thanks,
> Arun
> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig at amd.com>
> Sent: Tuesday, September 21, 2021 2:34 PM
> To: Paneer Selvam, Arunpravin <Arunpravin.PaneerSelvam at amd.com>; dri-devel at lists.freedesktop.org; intel-gfx at lists.freedesktop.org; amd-gfx at lists.freedesktop.org; matthew.auld at intel.com; daniel at ffwll.ch; Deucher, Alexander <Alexander.Deucher at amd.com>
> Subject: Re: [PATCH 2/2] Add drm buddy manager support to amdgpu driver
>
> Am 20.09.21 um 21:21 schrieb Arunpravin:
> [SNIP]
>> +	struct list_head blocks;
>> +};
>> +
>> +static inline struct amdgpu_vram_mgr_node *
>> +to_amdgpu_vram_mgr_node(struct ttm_resource *res) {
>> +	return container_of(container_of(res, struct ttm_range_mgr_node, base),
>> +			struct amdgpu_vram_mgr_node, tnode); }
>> +
> Maybe stuff that in a separate amdgpu_vram_mgr.h file together with all the other defines for the vram manager.
>
> Arun - I thought about it, will create a new header file for vram manager

Maybe make that a separate patch before this one here.

>> +	if (mode == DRM_BUDDY_ALLOC_RANGE) {
>> +		r = drm_buddy_alloc_range(mm, &vnode->blocks,
>> +				(uint64_t)place->fpfn << PAGE_SHIFT, pages << PAGE_SHIFT);
> That handling won't work. It's possible that you need contiguous memory in a specific range.
>
> Arun - the existing default backend range handler allocates contiguous nodes in power of 2 finding the MSB's of
> the any given size. We get linked nodes (depends on the requested size) in continuous range of address.
> Example, for the size 768 pages request, we get 512 + 256 range of continuous address in 2 nodes.
>
> It works by passing the fpfn and the requested size, the backend handler calculates the lpfn by adding fpfn + size = lpfn.
> The drawback here are we are not handling the specific lpfn value (as of now it is calculated using the fpfn + requested size)
> and not following the pages_per_node rule.
>
> Please let me know if this won't work for all specific fpfn / lpfn cases

 From your description that sounds like it won't work at all for any cases.

See the fpfn/lpfn specifies the range of allocation. For the most common 
case that's either 0..visible_vram or 0..start_of_some_hw_limitation.

When you always try to allocate the range from 0 you will quickly find 
that you clash with existing allocations.

What you need to do in general is to have a drm_buddy_alloc() which is 
able to limit the returned page to the desired range fpfn..lpfn.

>> +
>> +			do {
>> +				unsigned int order;
>> +
>> +				order = fls(n_pages) - 1;
>> +				BUG_ON(order > mm->max_order);
>> +
>> +				spin_lock(&mgr->lock);
>> +				block = drm_buddy_alloc(mm, order, bar_limit_enabled,
>> +									visible_pfn, mode);
> That doesn't seem to make much sense either. The backend allocator should not care about the BAR size nor the visible_pfn.
>
> Arun - we are sending the BAR limit enable information (in case of APU or large BAR, we take different approach) and visible_pfn
> Information.
>
> In case of bar_limit_enabled is true, I thought visible_pfn required for the backend allocator to compare with the block start address
> and find the desired blocks for the TOP-DOWN and BOTTOM-UP approach (TOP-DOWN - return blocks higher than the visible_pfn limit,
> BOTTOM-UP - return blocks lower than the visible_pfn limit).
>
> In case of bar_limit_enabled is false, we just return the top ordered blocks and bottom most blocks for the TOP-DOWN and BOTTOM-UP
> respectively (suitable for APU and Large BAR case).
>
> Please let me know if we have other way to fix this problem

That is the completely wrong approach. The backend must not care about 
the BAR configuration and visibility of the VRAM.

What it should do instead is to take the fpfn..lpfn range into account 
and make sure that all allocated pages are in the desired range.

BOTTOM-UP vs. TOP-DOWN then just optimizes the algorithm because we 
insert all freed up TOP-DOWN pages at the end and all BOTTOM-UP pages at 
the front and on new allocations walk the lest of free pages from the 
front or back depending on the flag.

>
>> +				spin_unlock(&mgr->lock);
>>    
>> -		vis_usage += amdgpu_vram_mgr_vis_size(adev, &node->mm_nodes[i]);
>> -		amdgpu_vram_mgr_virt_start(&node->base, &node->mm_nodes[i]);
>> -		pages_left -= pages;
>> -		++i;
>> +				if (IS_ERR(block)) {
>> +					r = -ENOSPC;
>> +					goto error_free_blocks;
>> +				}
>>    
>> -		if (pages > pages_left)
>> -			pages = pages_left;
>> +				n_pages -= BIT(order);
>> +
>> +				list_add_tail(&block->link, &vnode->blocks);
>> +
>> +				if (!n_pages)
>> +					break;
>> +			} while (1);
>> +
>> +			pages_left -= pages;
>> +			++i;
>> +
>> +			if (pages > pages_left)
>> +				pages = pages_left;
>> +		}
>>    	}
>> +
>> +	spin_lock(&mgr->lock);
>> +	list_sort(NULL, &vnode->blocks, sort_blocks);
> Why do you sort the list here?
>
> Regards,
> Christian.
>
> Arun - It gave better GLmark2 score when we sort the blocks in ascending order,
> Its not required, I will remove it

Interesting.  Maybe add a TODO comment so that somebody could 
investigate why that happens.

Regards,
Christian.