[PATCH 4/4] amdgpu/dc: remove bitmap implementation in gpio_service

[Felix Kuehling]

On 2017-10-02 11:49 PM, Dave Airlie wrote:
> From: Dave Airlie <airlied at redhat.com>
>
> This handrolls a bit map implementation (linux/bitmap.h),
> but it also actually doesn't need it, the max value greppable
> in the code is 31 for a gpio count. So just use a uint32_t for now.
>
> This should probably migrate to using the linux/bitops.h operations,
> but for now just rip out the bitmap implementation and fail if we

I think bitops are atomic, which may not be needed here. We've also run
into issues with bitops on other CPU architectures with KFD, because
they're 64-bit ops that can fail with incorrect alignment. We ended up
converting some of them to shifts and masks for that reason.

Regards,
  Felix

>> 32 bits.
> Signed-off-by: Dave Airlie <airlied at redhat.com>
> ---
>  drivers/gpu/drm/amd/display/dc/gpio/gpio_service.c | 85 +++-------------------
>  drivers/gpu/drm/amd/display/dc/gpio/gpio_service.h |  8 +-
>  2 files changed, 11 insertions(+), 82 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/gpio/gpio_service.c b/drivers/gpu/drm/amd/display/dc/gpio/gpio_service.c
> index d4e5ef6..95df7d4 100644
> --- a/drivers/gpu/drm/amd/display/dc/gpio/gpio_service.c
> +++ b/drivers/gpu/drm/amd/display/dc/gpio/gpio_service.c
> @@ -56,8 +56,7 @@ struct gpio_service *dal_gpio_service_create(
>  	struct dc_context *ctx)
>  {
>  	struct gpio_service *service;
> -
> -	uint32_t index_of_id;
> +	int i;
>  
>  	service = kzalloc(sizeof(struct gpio_service), GFP_KERNEL);
>  
> @@ -78,62 +77,16 @@ struct gpio_service *dal_gpio_service_create(
>  		goto failure_1;
>  	}
>  
> -	/* allocate and initialize business storage */
> -	{
> -		const uint32_t bits_per_uint = sizeof(uint32_t) << 3;
> -
> -		index_of_id = 0;
> -		service->ctx = ctx;
> -
> -		do {
> -			uint32_t number_of_bits =
> -				service->factory.number_of_pins[index_of_id];
> -
> -			uint32_t number_of_uints =
> -				(number_of_bits + bits_per_uint - 1) /
> -				bits_per_uint;
> -
> -			uint32_t *slot;
> -
> -			if (number_of_bits) {
> -				uint32_t index_of_uint = 0;
> +	service->ctx = ctx;
>  
> -				slot = kzalloc(number_of_uints * sizeof(uint32_t),
> -					       GFP_KERNEL);
> -
> -				if (!slot) {
> -					BREAK_TO_DEBUGGER();
> -					goto failure_2;
> -				}
> -
> -				do {
> -					slot[index_of_uint] = 0;
> -
> -					++index_of_uint;
> -				} while (index_of_uint < number_of_uints);
> -			} else
> -				slot = NULL;
> -
> -			service->busyness[index_of_id] = slot;
> -
> -			++index_of_id;
> -		} while (index_of_id < GPIO_ID_COUNT);
> +	for (i = 0; i < GPIO_ID_COUNT; i++) {
> +		if (service->factory.number_of_pins[i] > 32) {
> +			BREAK_TO_DEBUGGER();
> +			goto failure_1;
> +		}
>  	}
> -
>  	return service;
>  
> -failure_2:
> -	while (index_of_id) {
> -		uint32_t *slot;
> -
> -		--index_of_id;
> -
> -		slot = service->busyness[index_of_id];
> -
> -		if (slot)
> -			kfree(slot);
> -	};
> -
>  failure_1:
>  	kfree(service);
>  
> @@ -164,20 +117,6 @@ void dal_gpio_service_destroy(
>  		return;
>  	}
>  
> -	/* free business storage */
> -	{
> -		uint32_t index_of_id = 0;
> -
> -		do {
> -			uint32_t *slot = (*ptr)->busyness[index_of_id];
> -
> -			if (slot)
> -				kfree(slot);
> -
> -			++index_of_id;
> -		} while (index_of_id < GPIO_ID_COUNT);
> -	}
> -
>  	kfree(*ptr);
>  
>  	*ptr = NULL;
> @@ -195,9 +134,7 @@ static bool is_pin_busy(
>  {
>  	const uint32_t bits_per_uint = sizeof(uint32_t) << 3;
>  
> -	const uint32_t *slot = service->busyness[id] + (en / bits_per_uint);
> -
> -	return 0 != (*slot & (1 << (en % bits_per_uint)));
> +	return 0 != (service->busyness[id] & (1 << (en % bits_per_uint)));
>  }
>  
>  static void set_pin_busy(
> @@ -207,8 +144,7 @@ static void set_pin_busy(
>  {
>  	const uint32_t bits_per_uint = sizeof(uint32_t) << 3;
>  
> -	service->busyness[id][en / bits_per_uint] |=
> -		(1 << (en % bits_per_uint));
> +	service->busyness[id] |= (1 << (en % bits_per_uint));
>  }
>  
>  static void set_pin_free(
> @@ -218,8 +154,7 @@ static void set_pin_free(
>  {
>  	const uint32_t bits_per_uint = sizeof(uint32_t) << 3;
>  
> -	service->busyness[id][en / bits_per_uint] &=
> -		~(1 << (en % bits_per_uint));
> +	service->busyness[id] &= ~(1 << (en % bits_per_uint));
>  }
>  
>  enum gpio_result dal_gpio_service_open(
> diff --git a/drivers/gpu/drm/amd/display/dc/gpio/gpio_service.h b/drivers/gpu/drm/amd/display/dc/gpio/gpio_service.h
> index c7f3081..96c52be 100644
> --- a/drivers/gpu/drm/amd/display/dc/gpio/gpio_service.h
> +++ b/drivers/gpu/drm/amd/display/dc/gpio/gpio_service.h
> @@ -33,13 +33,7 @@ struct gpio_service {
>  	struct dc_context *ctx;
>  	struct hw_translate translate;
>  	struct hw_factory factory;
> -	/*
> -	 * @brief
> -	 * Business storage.
> -	 * For each member of 'enum gpio_id',
> -	 * store array of bits (packed into uint32_t slots),
> -	 * index individual bit by 'en' value */
> -	uint32_t *busyness[GPIO_ID_COUNT];
> +	uint32_t busyness[GPIO_ID_COUNT];
>  };
>  
>  enum gpio_result dal_gpio_service_open(