Re: [PATCH v6 1/6] drm: move the buddy allocator from i915 into common drm

From: "Christian König" <christian.koenig@amd.com>
To: Matthew Auld <matthew.auld@intel.com>,
	Arunpravin <Arunpravin.PaneerSelvam@amd.com>,
	dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org,
	amd-gfx@lists.freedesktop.org
Cc: alexander.deucher@amd.com, tzimmermann@suse.de
Subject: Re: [PATCH v6 1/6] drm: move the buddy allocator from i915 into common drm
Date: Fri, 7 Jan 2022 16:57:48 +0100	[thread overview]
Message-ID: <4053214f-bbb2-b6ad-37e9-4a579792a4b3@amd.com> (raw)
In-Reply-To: <1d1a4278-5aee-20d5-b536-7ca199e85e93@intel.com>

Am 07.01.22 um 16:49 schrieb Matthew Auld:
> On 26/12/2021 22:24, Arunpravin wrote:
>> Move the base i915 buddy allocator code into drm
>> - Move i915_buddy.h to include/drm
>> - Move i915_buddy.c to drm root folder
>> - Rename "i915" string with "drm" string wherever applicable
>> - Rename "I915" string with "DRM" string wherever applicable
>> - Fix header file dependencies
>> - Fix alignment issues
>> - add Makefile support for drm buddy
>> - export functions and write kerneldoc description
>> - Remove i915 selftest config check condition as buddy selftest
>>    will be moved to drm selftest folder
>>
>> cleanup i915 buddy references in i915 driver module
>> and replace with drm buddy
>>
>> v2:
>>    - include header file in alphabetical order(Thomas)
>>    - merged changes listed in the body section into a single patch
>>      to keep the build intact(Christian, Jani)
>>
>> v3:
>>    - make drm buddy a separate module(Thomas, Christian)
>>
>> v4:
>>    - Fix build error reported by kernel test robot <lkp@intel.com>
>>    - removed i915 buddy selftest from i915_mock_selftests.h to
>>      avoid build error
>>    - removed selftests/i915_buddy.c file as we create a new set of
>>      buddy test cases in drm/selftests folder
>>
>> v5:
>>    - Fix merge conflict issue
>>
>> Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@amd.com>
>
> <snip>
>
>> +int drm_buddy_init(struct drm_buddy_mm *mm, u64 size, u64 chunk_size)
>> +{
>> +    unsigned int i;
>> +    u64 offset;
>> +
>> +    if (size < chunk_size)
>> +        return -EINVAL;
>> +
>> +    if (chunk_size < PAGE_SIZE)
>> +        return -EINVAL;
>> +
>> +    if (!is_power_of_2(chunk_size))
>> +        return -EINVAL;
>> +
>> +    size = round_down(size, chunk_size);
>> +
>> +    mm->size = size;
>> +    mm->avail = size;
>> +    mm->chunk_size = chunk_size;
>> +    mm->max_order = ilog2(size) - ilog2(chunk_size);
>> +
>> +    BUG_ON(mm->max_order > DRM_BUDDY_MAX_ORDER);
>> +
>> +    mm->slab_blocks = KMEM_CACHE(drm_buddy_block, 0);
>> +    if (!mm->slab_blocks)
>> +        return -ENOMEM;
>
> It looks like every KMEM_CACHE() also creates a debugfs entry? See the 
> error here[1]. I guess because we end with multiple instances in i915. 
> If so, is it possible to have a single KMEM_CACHE() as part of the 
> buddy module, similar to what i915 was doing previously?

Oh, that is a really good point, this code here doesn't make to much sense.

The value of a KMEM_CACHE() is to allow speeding up allocation of the 
same structure size between different drm_buddy object. If you allocate 
one cache per drm_buddy that makes the whole functionality useless.

Please fix, this is actually a bug.

Christian.

>
> [1] 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fintel-gfx-ci.01.org%2Ftree%2Fdrm-tip%2FTrybot_8217%2Fshard-skl4%2Figt%40i915_selftest%40mock%40memory_region.html&amp;data=04%7C01%7Cchristian.koenig%40amd.com%7C56202fbe886f415c3b8308d9d1f5409c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637771673545453215%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=ZnRmAQo%2BjX414hbqHigL4R18oBDKLIugUQIVcwhFI%2BY%3D&amp;reserved=0