Re: [PATCH v2] drm/amdgpu: use bitmap_clear() in amdgpu_amdkfd_device_init()

[Kuehling, Felix]

On 2026-04-27 22:35, Yury Norov wrote:
> The bitmap_clear() works OK with both compile- and runtime nbits. But the
> comment says it doesn't work, and opencodes the call for nothing.
> Drop the misleading comment, and use bitmap_clear() as it should.

To be fair, that comment was added in 2017 by commit d0b63bb3385c. At 
the time, I believe it was referring to this comment in linux/bitmap.h 
(git show d0b63bb3385c:./include/linux/bitmap.h):

>  * Note that nbits should be always a compile time evaluable constant.
>  * Otherwise many inlines will generate horrible code.
This comment has since been updated to sound less dramatic 
(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=41e7b1661ffbf562d3aa2b7ce4ad283db50b711a):

>  * The generated code is more efficient when nbits is known at
>  * compile-time and at most BITS_PER_LONG.
So maybe reword this commit message to something slightly more 
charitable. ;) How about this:

The recommendation not to use bitmap functions with nbits not being 
compile-time
constants has changed since this code was added. bitmap_clear is more 
efficient than
an open-coded loop with clear_bit.

Other than that, the change looks fine to me.

Regards,
  Felix

> As a side effect, the patch switches from a series of atomics to
> a single non-atomic operation, which is easier on caches.
>
> Signed-off-by: Yury Norov <ynorov@xxxxxxxxxx>
> ---
> v2: don't declare 'i' in the new implementation.
>
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c | 8 ++------
>   1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> index d9e283f3b57d..500976d9087a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> @@ -167,7 +167,6 @@ int amdgpu_amdkfd_drm_client_create(struct amdgpu_device *adev)
>   
>   void amdgpu_amdkfd_device_init(struct amdgpu_device *adev)
>   {
> -	int i;
>   	int last_valid_bit;
>   
>   	amdgpu_amdkfd_gpuvm_init_mem_limits();
> @@ -194,14 +193,11 @@ void amdgpu_amdkfd_device_init(struct amdgpu_device *adev)
>   				  adev->gfx.mec_bitmap[0].queue_bitmap,
>   				  AMDGPU_MAX_QUEUES);
>   
> -		/* According to linux/bitmap.h we shouldn't use bitmap_clear if
> -		 * nbits is not compile time constant
> -		 */
>   		last_valid_bit = 1 /* only first MEC can have compute queues */
>   				* adev->gfx.mec.num_pipe_per_mec
>   				* adev->gfx.mec.num_queue_per_pipe;
> -		for (i = last_valid_bit; i < AMDGPU_MAX_QUEUES; ++i)
> -			clear_bit(i, gpu_resources.cp_queue_bitmap);
> +		bitmap_clear(gpu_resources.cp_queue_bitmap, last_valid_bit,
> +					AMDGPU_MAX_QUEUES - last_valid_bit);
>   
>   		amdgpu_doorbell_get_kfd_info(adev,
>   				&gpu_resources.doorbell_physical_address,